refactor(#2281): petgraph for topology — replace bounded walks with graph algorithms
The is_descendant_of and apply_set_parent cycle detection both used hand-rolled 32-hop bounded ancestor walks. Correct in practice (no real hive exceeds 32 levels) but carried an arbitrary ceiling and were harder to reason about than proven graph primitives. Changes: - Add build_graph(): converts BTreeMap<name, parent|null> → DiGraph with parent→child edges + BTreeMap<name, NodeIndex> index - Add is_descendant_of_in(): pure (no disk I/O), uses petgraph::algo::has_path_connecting from ancestor to candidate - Rewrite is_descendant_of(): delegates to is_descendant_of_in(&read()) - Rewrite apply_set_parent() cycle detection: build_graph() + speculative edge + is_cyclic_directed(); no depth limit - Add tests for is_descendant_of_in (self, direct child, grandchild, parent-is-not-child, sibling, unknown) petgraph was already a workspace dep (used elsewhere). On-disk format unchanged (flat JSON map). Public API surface unchanged.
This commit is contained in:
parent
c0a49c95c1
commit
0af14d8ef8
1 changed files with 155 additions and 27 deletions
|
|
@ -12,8 +12,22 @@
|
||||||
//! enforcement semantics: `docs/agent-hierarchy.md::Current state`.
|
//! enforcement semantics: `docs/agent-hierarchy.md::Current state`.
|
||||||
//! `<parent>` sentinel resolution (delivered by [`resolve_recipient`]):
|
//! `<parent>` sentinel resolution (delivered by [`resolve_recipient`]):
|
||||||
//! `docs/conventions.md::Recipient sentinels`.
|
//! `docs/conventions.md::Recipient sentinels`.
|
||||||
|
//!
|
||||||
|
//! ## Graph representation
|
||||||
|
//!
|
||||||
|
//! The on-disk format stays as a flat JSON map `name → parent | null`
|
||||||
|
//! (small, git-diffable). In-memory, heavy algorithms (descendant checks,
|
||||||
|
//! cycle detection) use a [`petgraph`] directed graph where each edge runs
|
||||||
|
//! **parent → child**. This replaces the ad-hoc bounded walks that existed
|
||||||
|
//! before: petgraph's `has_path_connecting` / `is_cyclic_directed`
|
||||||
|
//! are correct for graphs of any depth (no 32-hop ceiling) and well-tested.
|
||||||
|
//! The graph is built on demand from the flat map; it is not cached across
|
||||||
|
//! calls (the map is small and disk I/O dominates anyway).
|
||||||
|
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
|
|
||||||
|
use petgraph::algo::{has_path_connecting, is_cyclic_directed};
|
||||||
|
use petgraph::graph::{DiGraph, NodeIndex};
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
|
|
||||||
const TOPOLOGY_FILE: &str = "topology.json";
|
const TOPOLOGY_FILE: &str = "topology.json";
|
||||||
|
|
@ -136,29 +150,75 @@ pub fn resolve_recipient_in(
|
||||||
}
|
}
|
||||||
|
|
||||||
/// True when `candidate` is `ancestor` or any descendant of
|
/// True when `candidate` is `ancestor` or any descendant of
|
||||||
/// `ancestor` per the current topology. Walks parents from
|
/// `ancestor` per the current on-disk topology.
|
||||||
/// `candidate` upward; the walk terminates at root or on a cycle
|
///
|
||||||
/// (cycle defence: bounded to 32 hops, more than any plausible
|
/// Delegates to [`is_descendant_of_in`] on the result of [`read`] so
|
||||||
/// hive depth). Used by the cancel-authorization check in
|
/// the algorithm is the same petgraph BFS used everywhere else. No
|
||||||
/// `socket_server::handle_cancel_schedule` to enforce
|
/// depth limit — the 32-hop bounded walk this replaced was correct for
|
||||||
|
/// any plausible hive but carried a latent ceiling; this has none.
|
||||||
|
///
|
||||||
|
/// Used by the cancel-authorization checks in `socket_server` to enforce
|
||||||
/// "managers can cancel anything their subtree owns."
|
/// "managers can cancel anything their subtree owns."
|
||||||
#[must_use]
|
#[must_use]
|
||||||
pub fn is_descendant_of(candidate: &str, ancestor: &str) -> bool {
|
pub fn is_descendant_of(candidate: &str, ancestor: &str) -> bool {
|
||||||
|
is_descendant_of_in(&read(), candidate, ancestor)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Build an in-memory petgraph directed graph from the topology map.
|
||||||
|
///
|
||||||
|
/// Edges run **parent → child** so that:
|
||||||
|
/// - `children_of(name)` = outgoing neighbours of `name`'s node
|
||||||
|
/// - `is_descendant_of(candidate, ancestor)` = path exists from `ancestor`
|
||||||
|
/// to `candidate` via `has_path_connecting`
|
||||||
|
/// - cycle detection = `is_cyclic_directed` after a speculative edge insert
|
||||||
|
///
|
||||||
|
/// Returns the graph and a `BTreeMap<name → NodeIndex>` for O(log n)
|
||||||
|
/// name-to-node lookups. Both are local to each call site — the graph is
|
||||||
|
/// not cached. Hive topologies are small (< ~100 nodes); building on demand
|
||||||
|
/// is dominated by the surrounding disk read.
|
||||||
|
#[must_use]
|
||||||
|
fn build_graph(
|
||||||
|
topo: &BTreeMap<String, Option<String>>,
|
||||||
|
) -> (DiGraph<String, ()>, BTreeMap<String, NodeIndex>) {
|
||||||
|
let mut graph: DiGraph<String, ()> = DiGraph::new();
|
||||||
|
let mut idx: BTreeMap<String, NodeIndex> = BTreeMap::new();
|
||||||
|
|
||||||
|
// Add one node per agent.
|
||||||
|
for name in topo.keys() {
|
||||||
|
let ni = graph.add_node(name.clone());
|
||||||
|
idx.insert(name.clone(), ni);
|
||||||
|
}
|
||||||
|
// Add parent→child edges.
|
||||||
|
for (name, parent_opt) in topo {
|
||||||
|
if let Some(parent) = parent_opt {
|
||||||
|
if let (Some(&p_idx), Some(&c_idx)) = (idx.get(parent), idx.get(name.as_str())) {
|
||||||
|
graph.add_edge(p_idx, c_idx, ());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
(graph, idx)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Return true when `candidate` is a descendant of `ancestor` in the
|
||||||
|
/// given topology map. Uses petgraph BFS/DFS (`has_path_connecting`)
|
||||||
|
/// — no depth limit and no manually bounded walk. Pure; no disk I/O.
|
||||||
|
///
|
||||||
|
/// Same semantics as the disk-reading [`is_descendant_of`]: a node is
|
||||||
|
/// considered a descendant of itself (`candidate == ancestor` → true).
|
||||||
|
#[must_use]
|
||||||
|
pub fn is_descendant_of_in(
|
||||||
|
topo: &BTreeMap<String, Option<String>>,
|
||||||
|
candidate: &str,
|
||||||
|
ancestor: &str,
|
||||||
|
) -> bool {
|
||||||
if candidate == ancestor {
|
if candidate == ancestor {
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
let topo = read();
|
let (graph, idx) = build_graph(topo);
|
||||||
let mut cur = candidate.to_owned();
|
let (Some(&anc_ni), Some(&cand_ni)) = (idx.get(ancestor), idx.get(candidate)) else {
|
||||||
for _ in 0..32 {
|
return false;
|
||||||
let Some(parent) = topo.get(&cur).cloned().flatten() else {
|
};
|
||||||
return false;
|
has_path_connecting(&graph, anc_ni, cand_ni, None)
|
||||||
};
|
|
||||||
if parent == ancestor {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
cur = parent;
|
|
||||||
}
|
|
||||||
false
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Persist the topology map. Sorted JSON output (`BTreeMap` is sorted by
|
/// Persist the topology map. Sorted JSON output (`BTreeMap` is sorted by
|
||||||
|
|
@ -226,20 +286,19 @@ pub fn apply_set_parent(
|
||||||
if p == child {
|
if p == child {
|
||||||
return Err("an agent cannot be its own parent".to_owned());
|
return Err("an agent cannot be its own parent".to_owned());
|
||||||
}
|
}
|
||||||
// Cycle check: walk `p`'s ancestors in the EXISTING map. If
|
// Cycle check via petgraph: build the current graph, speculatively
|
||||||
// we hit `child`, then making `child`'s parent = `p` would
|
// insert the proposed parent→child edge, then test for cycles with
|
||||||
// close the loop (child → … → p → child).
|
// `is_cyclic_directed`. This replaces the earlier ad-hoc 32-hop
|
||||||
let mut cur = p.to_owned();
|
// ancestor walk — petgraph is correct for any tree depth and the
|
||||||
for _ in 0..32 {
|
// algorithm is well-tested.
|
||||||
if cur == child {
|
let (mut graph, idx) = build_graph(topo);
|
||||||
|
if let (Some(&p_ni), Some(&c_ni)) = (idx.get(p), idx.get(child)) {
|
||||||
|
graph.add_edge(p_ni, c_ni, ());
|
||||||
|
if is_cyclic_directed(&graph) {
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"cycle: {p} is in {child}'s subtree (would create a loop)"
|
"cycle: {p} is in {child}'s subtree (would create a loop)"
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
let Some(next) = topo.get(&cur).cloned().flatten() else {
|
|
||||||
break;
|
|
||||||
};
|
|
||||||
cur = next;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
let mut next = topo.clone();
|
let mut next = topo.clone();
|
||||||
|
|
@ -818,6 +877,75 @@ mod tests {
|
||||||
assert!(top_level_agents_in(&topo).is_empty());
|
assert!(top_level_agents_in(&topo).is_empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// is_descendant_of_in tests (petgraph-backed; pure / no disk I/O)
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_self_is_true() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
assert!(is_descendant_of_in(&topo, "alice", "alice"));
|
||||||
|
assert!(is_descendant_of_in(
|
||||||
|
&topo,
|
||||||
|
crate::lifecycle::MANAGER_NAME,
|
||||||
|
crate::lifecycle::MANAGER_NAME
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_direct_child() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
// alice is a direct child of manager.
|
||||||
|
assert!(is_descendant_of_in(
|
||||||
|
&topo,
|
||||||
|
"alice",
|
||||||
|
crate::lifecycle::MANAGER_NAME
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_grandchild() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
// bob is manager → alice → bob; should be reachable from manager.
|
||||||
|
assert!(is_descendant_of_in(
|
||||||
|
&topo,
|
||||||
|
"bob",
|
||||||
|
crate::lifecycle::MANAGER_NAME
|
||||||
|
));
|
||||||
|
assert!(is_descendant_of_in(
|
||||||
|
&topo,
|
||||||
|
"carol",
|
||||||
|
crate::lifecycle::MANAGER_NAME
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_parent_not_descendant_of_child() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
// alice is NOT a descendant of bob (alice is bob's grandparent).
|
||||||
|
assert!(!is_descendant_of_in(&topo, "alice", "bob"));
|
||||||
|
assert!(!is_descendant_of_in(
|
||||||
|
&topo,
|
||||||
|
crate::lifecycle::MANAGER_NAME,
|
||||||
|
"alice"
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_sibling_is_not_descendant() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
// bob and carol are siblings under alice; neither descends from the other.
|
||||||
|
assert!(!is_descendant_of_in(&topo, "bob", "carol"));
|
||||||
|
assert!(!is_descendant_of_in(&topo, "carol", "bob"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_descendant_of_in_unknown_is_false() {
|
||||||
|
let topo = topo_three_level();
|
||||||
|
assert!(!is_descendant_of_in(&topo, "nobody", "alice"));
|
||||||
|
assert!(!is_descendant_of_in(&topo, "alice", "nobody"));
|
||||||
|
}
|
||||||
|
|
||||||
// -----------------------------------------------------------------------
|
// -----------------------------------------------------------------------
|
||||||
// Roles tests (no disk I/O — use the pure `has_role_in` / in-memory maps)
|
// Roles tests (no disk I/O — use the pure `has_role_in` / in-memory maps)
|
||||||
// -----------------------------------------------------------------------
|
// -----------------------------------------------------------------------
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue