Fix spanning tree coordinate loop: reject parents whose ancestry contains us

evaluate_parent() did not check whether a candidate peer's ancestry
path already contained our own node_addr. Two nodes (e.g., sidecar and
VPS) could each select the other as parent, creating an alternating
coordinate loop that grew unbounded on each TreeAnnounce exchange.

Add loop detection in two places:
- evaluate_parent(): skip candidates whose ancestry contains us
- handle_tree_announce(): detect when current parent's updated ancestry
  contains us and drop the parent instead of propagating the loop
This commit is contained in:
Johnathan Corgan
2026-02-28 13:21:44 +00:00
parent 32054271f5
commit 5c1cbb4c30
3 changed files with 72 additions and 0 deletions
+22
View File
@@ -234,6 +234,28 @@ impl Node {
} else if !self.tree_state.is_root()
&& *self.tree_state.my_declaration().parent_id() == *from
{
// Check for loop: if parent's ancestry now contains us, drop parent
if let Some(parent_coords) = self.tree_state.peer_coords(from) {
if parent_coords.contains(self.identity.node_addr()) {
warn!(
parent = %self.peer_display_name(from),
"Parent ancestry contains us — loop detected, dropping parent"
);
let peer_costs: HashMap<NodeAddr, f64> = self.peers.iter()
.map(|(addr, peer)| (*addr, peer.link_cost()))
.collect();
if self.tree_state.handle_parent_lost(&peer_costs) {
if let Err(e) = self.tree_state.sign_declaration(&self.identity) {
warn!(error = %e, "Failed to sign declaration after loop detection");
return;
}
self.coord_cache.clear();
self.send_tree_announce_to_all().await;
}
return;
}
}
// Our parent's ancestry changed but we're keeping the same parent.
// Recompute our own coordinates (which derive from parent's ancestry)
// and re-announce so downstream nodes stay current.
+4
View File
@@ -336,6 +336,10 @@ impl TreeState {
if *coords.root_id() != smallest_root {
continue;
}
// Reject candidates whose ancestry contains us (would create a loop)
if coords.contains(&self.my_node_addr) {
continue;
}
let cost = peer_costs.get(peer_id).copied().unwrap_or(1.0);
let eff_depth = coords.depth() as f64 + cost;
match &best_peer {
+46
View File
@@ -490,6 +490,52 @@ fn test_evaluate_parent_depth_threshold() {
assert_eq!(result, Some(peer3));
}
#[test]
fn test_evaluate_parent_rejects_loop_candidate() {
// Node 5 with peer 1 whose ancestry contains node 5 — selecting
// peer 1 would create a coordinate loop. evaluate_parent must skip it.
let my_node = make_node_addr(5);
let mut state = TreeState::new(my_node);
let peer1 = make_node_addr(1);
let root = make_node_addr(0);
// Peer 1's ancestry: [1, 5, 0] — contains us (node 5)
state.update_peer(
ParentDeclaration::new(peer1, my_node, 1, 1000),
make_coords(&[1, 5, 0]),
);
// Should return None — the only candidate creates a loop
assert_eq!(state.evaluate_parent(&HashMap::new()), None);
}
#[test]
fn test_evaluate_parent_picks_loop_free_over_loopy() {
// Two peers reach the same root. Peer 1's ancestry contains us (loop),
// peer 2's does not. Should pick peer 2 even though peer 1 is shallower.
let my_node = make_node_addr(5);
let mut state = TreeState::new(my_node);
let peer1 = make_node_addr(1);
let peer2 = make_node_addr(2);
let root = make_node_addr(0);
// Peer 1: depth 2, but ancestry contains us — loop
state.update_peer(
ParentDeclaration::new(peer1, my_node, 1, 1000),
make_coords(&[1, 5, 0]),
);
// Peer 2: depth 3, loop-free
state.update_peer(
ParentDeclaration::new(peer2, make_node_addr(3), 1, 1000),
make_coords(&[2, 3, 4, 0]),
);
let result = state.evaluate_parent(&HashMap::new());
assert_eq!(result, Some(peer2));
}
#[test]
fn test_handle_parent_lost_finds_alternative() {
let my_node = make_node_addr(5);