From 5c1cbb4c3043daa6840cfc90df7600384a6a9f88 Mon Sep 17 00:00:00 2001
From: Johnathan Corgan <johnathan@corganlabs.com>
Date: Sat, 28 Feb 2026 03:15:28 +0000
Subject: [PATCH] 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
---
 src/node/tree.rs  | 22 ++++++++++++++++++++++
 src/tree/state.rs |  4 ++++
 src/tree/tests.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

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<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.
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);