mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-09 00:04:54 +00:00
Fix stale session cleanup and identity cache pre-seeding
Cherry-picked from v0l PR #6 (issue #5): 1. remove_active_peer() now removes the end-to-end session from self.sessions when evicting a peer. The stale Established entry caused initiate_session() to silently return Ok(()) via the is_established() guard, preventing session re-establishment after link-layer reconnection. 2. initiate_peer_connections() pre-seeds the identity cache from configured peer npubs at startup, so TUN packets can be dispatched immediately without waiting for handshake completion. 3. schedule_reconnect() preserves accumulated backoff when a retry entry already exists, preventing exponential backoff reset on repeated link-dead cycles. Includes regression tests for all three fixes.
This commit is contained in:
@@ -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();
|
||||
|
||||
|
||||
+23
-9
@@ -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<String>)> = 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
|
||||
|
||||
+25
-1
@@ -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"
|
||||
);
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Reference in New Issue
Block a user