From 5987cbfb6940938b4df46d245ceba8167a0bb67f Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 16 Feb 2026 12:41:45 +0000 Subject: [PATCH] Fix transit node coord_cache expiry breaking multi-hop routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Transit nodes cache destination coordinates when they forward SessionSetup messages (via try_warm_coord_cache). These coord_cache entries have a 5-minute TTL, after which they expire. Once expired, the transit node can no longer forward data packets for that destination — find_next_hop returns None and the node sends CoordsRequired errors back to the source. This creates a permanent routing failure for any multi-hop path after 5 minutes of the initial session establishment, even if traffic is actively flowing. The root cause was that find_next_hop used coord_cache.get(), a read-only lookup that checks expiry but never extends it. Active forwarding did not keep the cache warm. Meanwhile, get_and_touch() existed but only updated last_used without extending expires_at. Fix: - find_next_hop now calls coord_cache.get_and_touch() instead of get() - get_and_touch now calls entry.refresh() instead of entry.touch(), which extends expires_at by the default TTL on each access - find_next_hop signature changed from &self to &mut self to allow the mutable cache access This ensures that as long as traffic flows through a transit node, the coord_cache entries stay warm and routing continues to work. Entries still expire after 5 minutes of inactivity as designed. --- src/cache/coord_cache.rs | 6 ++-- src/node/mod.rs | 10 +++--- src/node/tests/routing.rs | 67 ++++++++++++++++++++++++++++++++------- 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/cache/coord_cache.rs b/src/cache/coord_cache.rs index a02d5b2..f77ca83 100644 --- a/src/cache/coord_cache.rs +++ b/src/cache/coord_cache.rs @@ -111,7 +111,7 @@ impl CoordCache { }) } - /// Look up coordinates and touch (update last_used). + /// Look up coordinates and refresh (update last_used and extend TTL). pub fn get_and_touch( &mut self, addr: &NodeAddr, @@ -125,9 +125,9 @@ impl CoordCache { return None; } - // Touch and return + // Refresh TTL and return if let Some(entry) = self.entries.get_mut(addr) { - entry.touch(current_time_ms); + entry.refresh(current_time_ms, self.default_ttl_ms); Some(entry.coords()) } else { None diff --git a/src/node/mod.rs b/src/node/mod.rs index c6ca73f..919804b 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -896,7 +896,7 @@ impl Node { /// fallback). Without coordinates, the node cannot make loop-free /// forwarding decisions. The caller should signal `CoordsRequired` back /// to the source when `None` is returned for a non-local destination. - pub fn find_next_hop(&self, dest_node_addr: &NodeAddr) -> Option<&ActivePeer> { + pub fn find_next_hop(&mut self, dest_node_addr: &NodeAddr) -> Option<&ActivePeer> { // 1. Local delivery if dest_node_addr == self.node_addr() { return None; @@ -915,17 +915,17 @@ impl Node { .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_millis() as u64) .unwrap_or(0); - let dest_coords = self.coord_cache.get(dest_node_addr, now_ms) - .or_else(|| self.route_cache.get(dest_node_addr).map(|c| c.coords()))?; + let dest_coords = self.coord_cache.get_and_touch(dest_node_addr, now_ms) + .or_else(|| self.route_cache.get(dest_node_addr).map(|c| c.coords()))?.clone(); // 3. Bloom filter candidates — requires dest_coords for loop-free selection let candidates: Vec<&ActivePeer> = self.destination_in_filters(dest_node_addr); if !candidates.is_empty() { - return self.select_best_candidate(&candidates, dest_coords); + return self.select_best_candidate(&candidates, &dest_coords); } // 4. Greedy tree routing fallback - let next_hop_id = self.tree_state.find_next_hop(dest_coords)?; + let next_hop_id = self.tree_state.find_next_hop(&dest_coords)?; self.peers.get(&next_hop_id).filter(|p| p.can_send()) } diff --git a/src/node/tests/routing.rs b/src/node/tests/routing.rs index 32f2a92..f078f77 100644 --- a/src/node/tests/routing.rs +++ b/src/node/tests/routing.rs @@ -16,7 +16,7 @@ use std::collections::HashSet; #[test] fn test_routing_local_delivery() { - let node = make_node(); + let mut node = make_node(); let my_addr = *node.node_addr(); assert!(node.find_next_hop(&my_addr).is_none()); } @@ -43,7 +43,7 @@ fn test_routing_direct_peer() { #[test] fn test_routing_unknown_destination() { - let node = make_node(); + let mut node = make_node(); let unknown = make_node_addr(99); assert!(node.find_next_hop(&unknown).is_none()); } @@ -222,6 +222,51 @@ fn test_routing_tree_no_coords_in_cache() { assert!(node.find_next_hop(&dest).is_none()); } +// === Active routing refreshes coord_cache TTL === + +#[test] +fn test_routing_refreshes_coord_cache_ttl() { + let mut node = make_node(); + let transport_id = TransportId::new(1); + let my_addr = *node.node_addr(); + + // Create a peer + let link_id = LinkId::new(1); + let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let peer_addr = *id.node_addr(); + node.add_connection(conn).unwrap(); + node.promote_connection(link_id, id, 2000).unwrap(); + + // Set up tree coordinates + let dest = make_node_addr(99); + let dest_coords = + TreeCoordinate::from_addrs(vec![dest, peer_addr, my_addr]).unwrap(); + node.tree_state_mut().update_peer( + ParentDeclaration::new(peer_addr, my_addr, 1, 1000), + TreeCoordinate::from_addrs(vec![peer_addr, my_addr]).unwrap(), + ); + + // Insert with a short TTL (10s) — enough to survive until find_next_hop runs + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0); + let short_ttl = 10_000; // 10 seconds + node.coord_cache_mut().insert_with_ttl(dest, dest_coords, now_ms, short_ttl); + let original_expiry = node.coord_cache().get_entry(&dest).unwrap().expires_at(); + + // find_next_hop should succeed and refresh TTL to now + default_ttl (300s) + assert!(node.find_next_hop(&dest).is_some()); + + // The refresh should have extended expires_at beyond the original + let new_expiry = node.coord_cache().get_entry(&dest).unwrap().expires_at(); + assert!( + new_expiry > original_expiry, + "find_next_hop should refresh the coord_cache TTL: original={}, new={}", + original_expiry, new_expiry, + ); +} + // === Bloom filter without coords → no route (loop prevention) === #[test] @@ -369,9 +414,10 @@ async fn test_routing_chain_topology() { // Node 0 should be able to route toward node 3. // The next hop should be node 1 (only peer of node 0). + let node1_addr = *nodes[1].node.node_addr(); + let node2_addr = *nodes[2].node.node_addr(); let hop = nodes[0].node.find_next_hop(&node3_addr); assert!(hop.is_some(), "Node 0 should find route to node 3"); - let node1_addr = *nodes[1].node.node_addr(); assert_eq!( hop.unwrap().node_addr(), &node1_addr, @@ -381,7 +427,6 @@ async fn test_routing_chain_topology() { // Node 3 should route toward node 0 via node 2. let hop = nodes[3].node.find_next_hop(&node0_addr); assert!(hop.is_some(), "Node 3 should find route to node 0"); - let node2_addr = *nodes[2].node.node_addr(); assert_eq!( hop.unwrap().node_addr(), &node2_addr, @@ -466,7 +511,7 @@ fn build_addr_index(nodes: &[TestNode]) -> std::collections::HashMap, src: usize, dst: usize, @@ -586,7 +631,7 @@ async fn test_routing_reachability_100_nodes() { total_pairs += 1; - match simulate_forwarding(&nodes, &addr_index, src, dst) { + match simulate_forwarding(&mut nodes, &addr_index, src, dst) { ForwardResult::Delivered(hops) => { total_hops += hops; if hops > max_hops { @@ -705,7 +750,7 @@ async fn test_routing_stops_after_peer_removal() { // Verify routing works before removal: node 0 → node 3 let addr_index = build_addr_index(&nodes); - match simulate_forwarding(&nodes, &addr_index, 0, 3) { + match simulate_forwarding(&mut nodes, &addr_index, 0, 3) { ForwardResult::Delivered(_) => {} other => panic!("Expected delivery before removal, got {:?}", other), } @@ -746,7 +791,7 @@ async fn test_routing_stops_after_peer_removal() { // still attempt forwarding — but the self-distance check prevents loops. // Either NoRoute or Loop-with-stale-coords is acceptable here; what // matters is that delivery does NOT succeed. - match simulate_forwarding(&nodes, &addr_index, 0, 3) { + match simulate_forwarding(&mut nodes, &addr_index, 0, 3) { ForwardResult::NoRoute { .. } => {} // Expected: can't reach node 3 ForwardResult::Loop { .. } => {} // Also acceptable: stale coords cause loop detection ForwardResult::Delivered(hops) => { @@ -755,7 +800,7 @@ async fn test_routing_stops_after_peer_removal() { } // But routing within the same component still works: node 2 → node 3 - match simulate_forwarding(&nodes, &addr_index, 2, 3) { + match simulate_forwarding(&mut nodes, &addr_index, 2, 3) { ForwardResult::Delivered(_) => {} other => panic!("Expected delivery within component, got {:?}", other), } @@ -898,7 +943,7 @@ async fn test_routing_source_only_coords_100_nodes() { .coord_cache_mut() .insert(*dest_addr, dest_coords.clone(), now_ms); - match simulate_forwarding(&nodes, &addr_index, src, dst) { + match simulate_forwarding(&mut nodes, &addr_index, src, dst) { ForwardResult::Delivered(_) => source_only_delivered += 1, ForwardResult::NoRoute { .. } => source_only_failed += 1, ForwardResult::Loop { .. } => { @@ -940,7 +985,7 @@ async fn test_routing_source_only_coords_100_nodes() { let mut full_cache_failures = 0usize; for &(src, dst) in &sample_pairs { - match simulate_forwarding(&nodes, &addr_index, src, dst) { + match simulate_forwarding(&mut nodes, &addr_index, src, dst) { ForwardResult::Delivered(_) => {} _ => full_cache_failures += 1, }