diff --git a/src/node/tree.rs b/src/node/tree.rs index 7e4abb7..73df275 100644 --- a/src/node/tree.rs +++ b/src/node/tree.rs @@ -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 = 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. diff --git a/src/tree/state.rs b/src/tree/state.rs index f8dad36..4402b76 100644 --- a/src/tree/state.rs +++ b/src/tree/state.rs @@ -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 { diff --git a/src/tree/tests.rs b/src/tree/tests.rs index 56f5343..26b4635 100644 --- a/src/tree/tests.rs +++ b/src/tree/tests.rs @@ -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);