diff --git a/src/node/handlers/dispatch.rs b/src/node/handlers/dispatch.rs index 4f9f9d9..a0909d0 100644 --- a/src/node/handlers/dispatch.rs +++ b/src/node/handlers/dispatch.rs @@ -109,13 +109,29 @@ impl Node { } // MMP teardown log (before we drop the peer) + let peer_name = self.peer_aliases.get(node_addr) + .cloned() + .unwrap_or_else(|| peer.identity().short_npub()); if let Some(mmp) = peer.mmp() { - let name = self.peer_aliases.get(node_addr) - .cloned() - .unwrap_or_else(|| peer.identity().short_npub()); - Self::log_mmp_teardown(&name, mmp); + Self::log_mmp_teardown(&peer_name, mmp); } + // Remove any end-to-end session associated with this peer. + // + // Sessions are tracked separately from peers (self.sessions vs self.peers). + // Leaving a stale session alive after removing the peer causes: + // 1. check_session_mmp_reports() keeps logging stale "MMP session metrics" + // with frozen counters until purge_idle_sessions() eventually fires. + // 2. initiate_session() finds is_established() == true on the stale entry + // and silently returns Ok(()), preventing a new session from being + // established even after the link layer reconnects successfully. + if let Some(session_entry) = self.sessions.remove(node_addr) + && let Some(mmp) = session_entry.mmp() + { + Self::log_session_mmp_teardown(&peer_name, mmp); + } + self.pending_tun_packets.remove(node_addr); + let link_id = peer.link_id(); let transport_id = peer.transport_id(); diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index e92c8dd..b88746b 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -17,15 +17,29 @@ impl Node { /// For each peer configured with AutoConnect policy, creates a link and /// peer entry, then starts the Noise handshake by sending the first message. pub(super) async fn initiate_peer_connections(&mut self) { - // Build display name map from all configured peers (alias or short npub) - for peer_config in self.config.peers() { - if let Ok(identity) = PeerIdentity::from_npub(&peer_config.npub) { - let name = peer_config - .alias - .clone() - .unwrap_or_else(|| identity.short_npub()); - self.peer_aliases.insert(*identity.node_addr(), name); - } + // Build display name map from all configured peers (alias or short npub), + // and pre-seed the identity cache from each peer's npub so that TUN packets + // addressed to a configured peer can be dispatched (and trigger session + // initiation) immediately on startup — without waiting for the link-layer + // handshake to complete first. + let peer_identities: Vec<(PeerIdentity, Option)> = self + .config + .peers() + .iter() + .filter_map(|pc| { + PeerIdentity::from_npub(&pc.npub) + .ok() + .map(|id| (id, pc.alias.clone())) + }) + .collect(); + + for (identity, alias) in peer_identities { + let name = alias.unwrap_or_else(|| identity.short_npub()); + self.peer_aliases.insert(*identity.node_addr(), name); + // Pre-seed identity cache. The parity may be wrong (npub is x-only) + // but will be corrected to the real value when the peer is promoted + // after a successful Noise handshake. + self.register_identity(*identity.node_addr(), identity.pubkey_full()); } // Collect peer configs to avoid borrow conflicts diff --git a/src/node/retry.rs b/src/node/retry.rs index c5354da..9473796 100644 --- a/src/node/retry.rs +++ b/src/node/retry.rs @@ -129,6 +129,12 @@ impl Node { /// /// Looks up the peer in auto-connect config and checks `auto_reconnect`. /// If enabled, feeds the peer into the retry system with unlimited retries. + /// + /// If a retry entry already exists (e.g. from a previous failed handshake + /// attempt during an earlier reconnect cycle), the existing retry count is + /// preserved and incremented rather than reset to zero. This ensures + /// exponential backoff accumulates across repeated link-dead events instead + /// of resetting to the base interval on every peer removal. pub(super) fn schedule_reconnect(&mut self, node_addr: NodeAddr, now_ms: u64) { // Find peer in auto-connect config let peer_config = self @@ -155,6 +161,24 @@ impl Node { let base_interval_ms = self.config.node.retry.base_interval_secs * 1000; let max_backoff_ms = self.config.node.retry.max_backoff_secs * 1000; + let peer_name = self.peer_display_name(&node_addr); + + // If we already have accumulated backoff from previous failed attempts, + // preserve and bump it rather than resetting to zero. This prevents the + // exponential backoff from being discarded on each link-dead cycle. + if let Some(state) = self.retry_pending.get_mut(&node_addr) { + state.reconnect = true; + state.retry_count += 1; + let delay = state.backoff_ms(base_interval_ms, max_backoff_ms); + state.retry_after_ms = now_ms + delay; + info!( + peer = %peer_name, + retry = state.retry_count, + delay_secs = delay / 1000, + "Scheduling auto-reconnect after link-dead removal (backoff preserved)" + ); + return; + } let mut state = RetryState::new(pc); state.reconnect = true; @@ -162,7 +186,7 @@ impl Node { state.retry_after_ms = now_ms + delay; info!( - peer = %self.peer_display_name(&node_addr), + peer = %peer_name, delay_secs = delay / 1000, "Scheduling auto-reconnect after link-dead removal" ); diff --git a/src/node/tests/disconnect.rs b/src/node/tests/disconnect.rs index a5ad34f..567543d 100644 --- a/src/node/tests/disconnect.rs +++ b/src/node/tests/disconnect.rs @@ -228,6 +228,94 @@ async fn test_disconnect_chain_partition() { cleanup_nodes(&mut nodes).await; } +/// Removing a peer via disconnect must also remove the associated end-to-end session. +/// +/// Regression test for issue #5: `remove_active_peer` previously left the +/// `SessionEntry` alive in `self.sessions` after evicting the peer from +/// `self.peers`. This caused: +/// 1. Stale "MMP session metrics" logs with frozen counters until +/// `purge_idle_sessions` eventually fired (up to idle_timeout_secs later). +/// 2. `initiate_session` silently returning `Ok(())` on the stale Established +/// entry's guard check, preventing a new session from being created even +/// after the link layer reconnected successfully. +#[tokio::test] +async fn test_disconnect_clears_session() { + use crate::identity::Identity; + use crate::node::session::{EndToEndState, SessionEntry}; + use crate::noise::HandshakeState; + + // Two-node topology: 0 -- 1. + let edges = vec![(0, 1)]; + let mut nodes = run_tree_test(2, &edges, false).await; + verify_tree_convergence(&nodes); + + let node0_addr = *nodes[0].node.node_addr(); + let node1_addr = *nodes[1].node.node_addr(); + + // Inject a synthetic Established session entry into node 1's session table + // to simulate the state after a completed XK handshake with node 0. + let remote_identity = Identity::generate(); + { + let our_identity = nodes[1].node.identity().clone(); + + let mut initiator = HandshakeState::new_initiator( + our_identity.keypair(), + remote_identity.pubkey_full(), + ); + let mut responder = HandshakeState::new_responder(remote_identity.keypair()); + let mut init_epoch = [0u8; 8]; + rand::Rng::fill_bytes(&mut rand::rng(), &mut init_epoch); + initiator.set_local_epoch(init_epoch); + let mut resp_epoch = [0u8; 8]; + rand::Rng::fill_bytes(&mut rand::rng(), &mut resp_epoch); + responder.set_local_epoch(resp_epoch); + let msg1 = initiator.write_message_1().unwrap(); + responder.read_message_1(&msg1).unwrap(); + let msg2 = responder.write_message_2().unwrap(); + initiator.read_message_2(&msg2).unwrap(); + let session = initiator.into_session().unwrap(); + + let entry = SessionEntry::new( + node0_addr, + remote_identity.pubkey_full(), + EndToEndState::Established(session), + 1_000, + true, + ); + nodes[1].node.sessions.insert(node0_addr, entry); + } + + assert_eq!(nodes[1].node.session_count(), 1, "Session should exist before disconnect"); + assert_eq!(nodes[1].node.peer_count(), 1, "Peer should exist before disconnect"); + + // Node 0 sends Disconnect to node 1. + let disconnect = crate::protocol::Disconnect::new(DisconnectReason::Shutdown); + nodes[0] + .node + .send_encrypted_link_message(&node1_addr, &disconnect.encode()) + .await + .expect("Failed to send disconnect"); + + tokio::time::sleep(Duration::from_millis(50)).await; + process_available_packets(&mut nodes).await; + + // Peer must be gone. + assert_eq!( + nodes[1].node.peer_count(), 0, + "Peer should be removed after disconnect" + ); + + // Session must also be gone — core regression check for issue #5. + // Before the fix, session_count() would still be 1 here because + // remove_active_peer didn't remove self.sessions[node0_addr]. + assert_eq!( + nodes[1].node.session_count(), 0, + "Session must be cleaned up when peer is removed (regression: issue #5)" + ); + + cleanup_nodes(&mut nodes).await; +} + /// Verify that different disconnect reasons are handled correctly. /// /// Sends each reason code and verifies the peer is removed regardless. diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index ea8d5a4..98eeaaf 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -702,6 +702,90 @@ fn test_schedule_retry_skips_connected_peer() { ); } +/// Test that schedule_reconnect preserves accumulated backoff across link-dead cycles. +/// +/// Regression test for issue #5: previously `schedule_reconnect` always created a +/// fresh `RetryState` with `retry_count=0`, discarding any backoff accumulated by +/// prior failed handshake attempts. On repeated link-dead evictions the node would +/// restart exponential backoff from the base interval every time instead of +/// continuing to back off. +#[test] +fn test_schedule_reconnect_preserves_backoff() { + let peer_identity = Identity::generate(); + let peer_npub = peer_identity.npub(); + let peer_node_addr = *PeerIdentity::from_npub(&peer_npub).unwrap().node_addr(); + + let mut config = Config::new(); + config.peers.push(crate::config::PeerConfig::new( + peer_npub, + "udp", + "10.0.0.2:2121", + )); + + let mut node = Node::new(config).unwrap(); + + // Simulate two stale handshake timeouts incrementing the retry count. + node.schedule_retry(peer_node_addr, 1_000); // count=1, delay=10s + node.schedule_retry(peer_node_addr, 11_000); // count=2, delay=20s + { + let state = node.retry_pending.get(&peer_node_addr).unwrap(); + assert_eq!(state.retry_count, 2, "Two failures should yield count=2"); + } + + // Now simulate a link-dead removal triggering schedule_reconnect. + // The existing retry entry (count=2) should be preserved and bumped to 3, + // NOT reset to 0 as it was before the fix. + node.schedule_reconnect(peer_node_addr, 31_000); + + let state = node.retry_pending.get(&peer_node_addr).unwrap(); + assert!( + state.reconnect, + "Entry should be marked as reconnect" + ); + assert_eq!( + state.retry_count, 3, + "schedule_reconnect should increment existing count (was 2), not reset to 0 (regression: issue #5)" + ); + + // With count=3, backoff should be 5s * 2^3 = 40s. + let base_ms = node.config.node.retry.base_interval_secs * 1000; + let max_ms = node.config.node.retry.max_backoff_secs * 1000; + let expected_delay = state.backoff_ms(base_ms, max_ms); + assert_eq!( + state.retry_after_ms, 31_000 + expected_delay, + "retry_after_ms should reflect count=3 backoff" + ); +} + +/// Test that schedule_reconnect on a fresh peer (no prior retry entry) starts at count=0. +#[test] +fn test_schedule_reconnect_fresh_state() { + let peer_identity = Identity::generate(); + let peer_npub = peer_identity.npub(); + let peer_node_addr = *PeerIdentity::from_npub(&peer_npub).unwrap().node_addr(); + + let mut config = Config::new(); + config.peers.push(crate::config::PeerConfig::new( + peer_npub, + "udp", + "10.0.0.2:2121", + )); + + let mut node = Node::new(config).unwrap(); + + // No prior retry entry — first reconnect should use base delay. + node.schedule_reconnect(peer_node_addr, 1_000); + + let state = node.retry_pending.get(&peer_node_addr).unwrap(); + assert!(state.reconnect, "Entry should be marked as reconnect"); + assert_eq!(state.retry_count, 0, "Fresh reconnect should start at count=0"); + // Base delay: 5s * 2^0 = 5s + let base_ms = node.config.node.retry.base_interval_secs * 1000; + let max_ms = node.config.node.retry.max_backoff_secs * 1000; + let expected_delay = state.backoff_ms(base_ms, max_ms); + assert_eq!(state.retry_after_ms, 1_000 + expected_delay); +} + /// Test that promote_connection clears retry_pending. #[test] fn test_promote_clears_retry_pending() {