From 31f5a8c1b77e77c42015a9702509d8af69531a04 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 15 Jul 2026 18:49:50 +0000 Subject: [PATCH 1/3] peer: add a per-peer timer store populated by the machine timer actions The control machine already emits SetTimer/CancelTimer actions when it arms the handshake retransmit and timeout deadlines, but their executor arms were a single no-op stub and the machine's Timeout event was never dispatched -- the real work still runs on the legacy tick (check_timeouts, resend_pending_handshakes). Add the storage the time-as-input driver will read: a peer_timers map keyed by LinkId then TimerKind, holding each armed timer's absolute deadline. The SetTimer arm now inserts (overwrite = reschedule) and CancelTimer removes; the outbound msg2 promote cancels the two dial-armed handshake timers, since the machine survives promotion and the entries would otherwise linger in the store. This store is a shadow: it is written and cleared but no driver reads it yet, so behavior is unchanged. The legacy tick stays authoritative until the driver that feeds Timeout and deletes the overlapping tick paths lands. Route every machine removal through a new remove_peer_machine(link) choke-point that drops the timer store alongside the machine, replacing the twelve direct peer_machines.remove sites so no armed timer outlives its machine. TimerKind gains Hash (to key the store) and Ord (for deterministic driver collection); it is internal and never serialized. Test count unchanged at 1631. --- src/node/dataplane/dispatch.rs | 2 +- src/node/dataplane/peer_actions.rs | 40 +++++++++++++----- src/node/handlers/handshake.rs | 21 ++++++++-- src/node/handlers/timeout.rs | 2 +- src/node/lifecycle/mod.rs | 8 ++-- src/node/mod.rs | 22 +++++++++- src/peer/machine.rs | 66 +++++++++++++++++++++++++----- 7 files changed, 128 insertions(+), 33 deletions(-) diff --git a/src/node/dataplane/dispatch.rs b/src/node/dataplane/dispatch.rs index e150942..8238fa5 100644 --- a/src/node/dataplane/dispatch.rs +++ b/src/node/dataplane/dispatch.rs @@ -195,7 +195,7 @@ impl Node { // ever touches the in-flight establish's (distinct) `link_id`; no reader // depends on a stale entry, so removal changes no behavior — it only bounds // the map. - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); if let Some(transport_id) = transport_id { self.cleanup_bootstrap_transport_if_unused(transport_id); } diff --git a/src/node/dataplane/peer_actions.rs b/src/node/dataplane/peer_actions.rs index 8dd2eaf..2b71a6c 100644 --- a/src/node/dataplane/peer_actions.rs +++ b/src/node/dataplane/peer_actions.rs @@ -18,9 +18,14 @@ //! send from `poll_pending_connects`). //! //! Not yet driven, so their arms stay inert stubs: rekey/crypto installs, -//! link-control frames, the timers (`SetTimer`/`CancelTimer` — the legacy tick -//! still runs them), and the connected-UDP plane. `RegisterDecryptSession` is a -//! deliberate no-op — see its arm for the note. +//! link-control frames, and the connected-UDP plane. `RegisterDecryptSession` is +//! a deliberate no-op — see its arm for the note. +//! +//! The timer arms (`SetTimer`/`CancelTimer`) are wired to populate/clear the +//! per-peer timer store (`peer_timers`), but that store is a SHADOW: the legacy +//! tick (`check_timeouts`/`resend_pending_handshakes`) still does all real timer +//! work and no `PeerEvent::Timeout` is fed, so the arms are behavior-neutral +//! until the handshake-kind driver fold cuts the legacy paths over. use crate::PeerIdentity; use crate::node::Node; @@ -145,7 +150,7 @@ impl Node { Err(_e) => { self.links.remove(&link); self.addr_to_link.remove(&(transport_id, remote_addr)); - self.peer_machines.remove(&link); + self.remove_peer_machine(link); return; } } @@ -190,7 +195,7 @@ impl Node { if let Some(idx) = ambient.our_index { let _ = self.index_allocator.free(idx); } - self.peer_machines.remove(&link); + self.remove_peer_machine(link); self.stats_mut() .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); return; @@ -356,7 +361,7 @@ impl Node { self.stats_mut().record_reject(RejectReason::Handshake( HandshakeReject::BadState, )); - self.peer_machines.remove(&promote_link); + self.remove_peer_machine(promote_link); } else { // OLD inbound (`handle_msg1` L587-591): drop the link // + reverse map, free our index, discard the machine, @@ -372,7 +377,7 @@ impl Node { if let Some(idx) = ambient.our_index { let _ = self.index_allocator.free(idx); } - self.peer_machines.remove(&promote_link); + self.remove_peer_machine(promote_link); self.stats_mut().record_reject(RejectReason::Handshake( HandshakeReject::BadState, )); @@ -494,10 +499,23 @@ impl Node { PeerAction::ActivateConnectedUdp | PeerAction::TeardownConnectedUdp => { // Connected-UDP plane ownership (`connected_udp.rs`). } - PeerAction::SetTimer { .. } | PeerAction::CancelTimer { .. } => { - // Timers become actions on the existing quantized tick. - // INERT — the legacy tick timers still run, so driving - // these would double-schedule. + PeerAction::SetTimer { kind, at_ms } => { + // Populate the driver's per-peer timer store (overwrite = + // reschedule). SHADOW ONLY at this rung: the legacy tick + // (`check_timeouts`/`resend_pending_handshakes`) still does + // all real work, and no `PeerEvent::Timeout` is fed yet, so + // this is behavior-neutral. The handshake-kind fold (C5-2b/c) + // adds the driver that reads this store and deletes the + // overlapping legacy paths. + self.peer_timers + .entry(link) + .or_default() + .insert(kind, at_ms); + } + PeerAction::CancelTimer { kind } => { + if let Some(timers) = self.peer_timers.get_mut(&link) { + timers.remove(&kind); + } } PeerAction::ReportLost { peer, kind } => { // The single loss token, routed to the reconciler reflex the diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 87563ab..ee02865 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -7,7 +7,7 @@ use crate::node::dataplane::PeerActionCtx; use crate::node::reject::{HandshakeReject, RejectReason}; use crate::node::{Node, NodeError}; use crate::peer::machine::{ - FailReason, HandshakePhase, PeerAction, PeerEvent, PeerMachine, PeerState, + FailReason, HandshakePhase, PeerAction, PeerEvent, PeerMachine, PeerState, TimerKind, }; use crate::peer::{ActivePeer, PeerConnection}; use crate::proto::fmp::wire::{Msg1Header, Msg2Header, build_msg2}; @@ -974,7 +974,7 @@ impl Node { } self.connections.remove(&link_id); // Drop the machine persisted at dial — this leg never promotes. - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); self.remove_link(&link_id); if let Some(idx) = our_index { let _ = self.index_allocator.free(idx); @@ -1014,7 +1014,7 @@ impl Node { // directly, with no machine). Drop it on entry so none of this block's // exits leave a dangling machine — matching the pre-persistence path, // which created no machine for a cross-connection. - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); // Extract the outbound connection let mut conn = match self.connections.remove(&link_id) { Some(c) => c, @@ -1186,9 +1186,22 @@ impl Node { actions } }; + // The outbound Msg2 Promote step cancels the two dial-armed handshake + // timers (the machine survives promotion, so they would otherwise linger + // in the driver's shadow store) and then promotes. The cancels are + // behavior-neutral at this rung — `peer_timers` is written but not yet + // driven — and `PromoteToActive` is still what performs the promotion. debug_assert_eq!( promote_actions, - vec![PeerAction::PromoteToActive { link: link_id }] + vec![ + PeerAction::CancelTimer { + kind: TimerKind::HandshakeRetransmit + }, + PeerAction::CancelTimer { + kind: TimerKind::HandshakeTimeout + }, + PeerAction::PromoteToActive { link: link_id }, + ] ); // Execute `[PromoteToActive]`. The executor calls `promote_connection`, diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index c1af290..907df03 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -119,7 +119,7 @@ impl Node { // `link_id` and lifetime; drop it here so a reaped handshake leg leaves // no dangling machine. A no-op for promoted peers — `promote_connection` // already removed their connection, so this reaper never runs for them. - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); let transport_id = conn.transport_id(); // Free session index and pending_outbound if allocated diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index ec1c683..b3f35fb 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -588,7 +588,7 @@ impl Node { self.links.remove(&link_id); self.addr_to_link .remove(&(transport_id, remote_addr.clone())); - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); return Err(NodeError::IndexAllocationFailed(e.to_string())); } }; @@ -604,7 +604,7 @@ impl Node { self.links.remove(&link_id); self.addr_to_link .remove(&(transport_id, remote_addr.clone())); - self.peer_machines.remove(&link_id); + self.remove_peer_machine(link_id); return Err(NodeError::HandshakeFailed(e.to_string())); } }; @@ -1225,7 +1225,7 @@ impl Node { ); // Clean up link and dial-time machine on handshake failure self.remove_link(&pending.link_id); - self.peer_machines.remove(&pending.link_id); + self.remove_peer_machine(pending.link_id); } else { // Drive the dial-persisted machine: `Connecting` → // `on_transport_connected` → `start_outbound_handshake`, @@ -1264,7 +1264,7 @@ impl Node { // Clean up link and dial-time machine, then schedule retry self.remove_link(&pending.link_id); self.links.remove(&pending.link_id); - self.peer_machines.remove(&pending.link_id); + self.remove_peer_machine(pending.link_id); self.note_handshake_timeout(*pending.peer_identity.node_addr(), Self::now_ms()); } } diff --git a/src/node/mod.rs b/src/node/mod.rs index cb34989..b490e07 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -37,7 +37,7 @@ use self::reloadable::Reloadable; pub(crate) const REKEY_JITTER_SECS: i64 = 15; use crate::cache::CoordCache; use crate::node::session::SessionEntry; -use crate::peer::machine::PeerMachine; +use crate::peer::machine::{PeerMachine, TimerKind}; use crate::peer::{ActivePeer, PeerConnection}; use crate::proto::bloom::{BloomFilter, BloomState}; use crate::proto::fmp::Fmp; @@ -364,6 +364,15 @@ pub struct Node { #[allow(dead_code)] peer_machines: HashMap, + /// Per-peer timer store, keyed by `LinkId` then `TimerKind`, holding each + /// armed timer's absolute deadline (ms). The sans-IO time-as-input backing + /// for `PeerEvent::Timeout`: populated/cleared by the machine's + /// `SetTimer`/`CancelTimer` actions (`dataplane/peer_actions.rs`) and dropped + /// alongside the machine through the `remove_peer_machine` choke-point. At + /// this rung it is a SHADOW of the legacy tick timers — written but not yet + /// read by any driver (the handshake-kind fold wires the reader). + peer_timers: HashMap>, + // === Peers (Active Phase) === /// Authenticated peers. /// Indexed by NodeAddr (verified identity). @@ -631,6 +640,7 @@ impl Node { child_exit_rx: None, connections: HashMap::new(), peer_machines: HashMap::new(), + peer_timers: HashMap::new(), peers: HashMap::new(), sessions: HashMap::new(), identity_cache: HashMap::new(), @@ -779,6 +789,7 @@ impl Node { child_exit_rx: None, connections: HashMap::new(), peer_machines: HashMap::new(), + peer_timers: HashMap::new(), peers: HashMap::new(), sessions: HashMap::new(), identity_cache: HashMap::new(), @@ -2248,6 +2259,15 @@ impl Node { } } + /// Single choke-point for dropping a per-peer control machine. Also drops the + /// machine's timer store so no armed `SetTimer` outlives it (the store and the + /// machine share the `LinkId` lifetime). Every teardown path routes machine + /// removal here rather than calling `peer_machines.remove` directly. + pub(in crate::node) fn remove_peer_machine(&mut self, link: LinkId) { + self.peer_machines.remove(&link); + self.peer_timers.remove(&link); + } + pub(crate) fn cleanup_bootstrap_transport_if_unused(&mut self, transport_id: TransportId) { if !self .supervisor diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 5305400..b7b9c4d 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -172,7 +172,14 @@ pub(crate) enum FailReason { } /// A timer the machine schedules on the driver's quantized tick. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +/// +/// `Hash` lets it key the driver's per-peer timer store; `Ord` lets the driver +/// collect due kinds deterministically. Note the driver must fire +/// `HandshakeTimeout` before `HandshakeRetransmit` on a same-tick coincidence +/// (a reaped leg must not be resent), which is the reverse of this declaration +/// order — the driver orders explicitly rather than relying on the derived +/// ascending `Ord`. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub(crate) enum TimerKind { HandshakeRetransmit, HandshakeTimeout, @@ -607,7 +614,20 @@ impl PeerMachine { // Net-new: our outbound index (allocated at dial) is the one we // register once promotion resolves. self.our_index = self.conn.our_index(); - vec![PeerAction::PromoteToActive { link: self.link }] + // The machine survives promotion (it becomes the active peer's + // control machine), so cancel the outbound handshake timers here + // or they would linger in the driver's store. A late fire would + // no-op against the non-`Handshaking` state, but leaving them + // armed is a timer leak. + vec![ + PeerAction::CancelTimer { + kind: TimerKind::HandshakeRetransmit, + }, + PeerAction::CancelTimer { + kind: TimerKind::HandshakeTimeout, + }, + PeerAction::PromoteToActive { link: self.link }, + ] } OutboundDecision::CrossConnectionSwap => { // Our outbound wins: swap the peer to the outbound session, @@ -1935,9 +1955,17 @@ mod tests { ); assert_eq!( actions, - vec![PeerAction::PromoteToActive { - link: LinkId::new(1) - }] + vec![ + PeerAction::CancelTimer { + kind: TimerKind::HandshakeRetransmit + }, + PeerAction::CancelTimer { + kind: TimerKind::HandshakeTimeout + }, + PeerAction::PromoteToActive { + link: LinkId::new(1) + } + ] ); actions = m.step( PeerEvent::PromotionResolved { @@ -2051,9 +2079,17 @@ mod tests { ); assert_eq!( promote, - vec![PeerAction::PromoteToActive { - link: LinkId::new(1) - }] + vec![ + PeerAction::CancelTimer { + kind: TimerKind::HandshakeRetransmit + }, + PeerAction::CancelTimer { + kind: TimerKind::HandshakeTimeout + }, + PeerAction::PromoteToActive { + link: LinkId::new(1) + } + ] ); assert_eq!( m.our_index(), @@ -2161,9 +2197,17 @@ mod tests { ); assert_eq!( promote, - vec![PeerAction::PromoteToActive { - link: LinkId::new(1) - }] + vec![ + PeerAction::CancelTimer { + kind: TimerKind::HandshakeRetransmit + }, + PeerAction::CancelTimer { + kind: TimerKind::HandshakeTimeout + }, + PeerAction::PromoteToActive { + link: LinkId::new(1) + } + ] ); assert_eq!(m.our_index(), None); } From 5021197f5ce3e3f785c10e738f2e17224de2dc5f Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 15 Jul 2026 19:38:49 +0000 Subject: [PATCH 2/3] node: drive the handshake msg1 resend from the per-peer machine timer Move the outbound msg1 resend off the unconditional tick function onto the machine-armed retransmit timer. The per-peer machine already arms a HandshakeRetransmit deadline at dial; a new drive_peer_timers fires the due ones (kind-filtered to the retransmit timer) and homes the resend counter on the machine, where the operator-visible count now reads from. The resend decision reads the same operator config as before (interval, backoff, max), the wire bytes and transport target still come from the shell connection, and the pure core computes the backoff schedule. As with the deleted resend_pending_handshakes, the count and reschedule advance only on a successful send: a failed send neither advances the count nor marks the connection failed, it just retries on the next tick. The handshake-timeout timer stays on the legacy check_timeouts path, and the rekey/liveness timers keep their own shell drivers, so drive_peer_timers deliberately fires only the retransmit kind. The show_connections resend count is relocated to read from the machine (the counter's new home) via connection_resend_count; machine-less and inbound connections report 0, matching what the shell connection reported before. Delete resend_pending_handshakes and resend_candidates (the latter also from the LifecycleView trait, its only user). The resend unit test is re-expressed against the machine + timer path, covering the due check and the record-on-success semantics. Known limitation: the first resend interval is armed from the machine's hardcoded 1000ms constant, which equals the config default. Under an operator override of handshake_resend_interval_ms the first resend diverges from the pre-change dial+interval; subsequent resends and the cap remain config-driven. Neutralizing the first-resend override needs the interval threaded into the sync core (the shell arm would be clobbered by the machine's own timer arming at dial), deferred to when the connection and machine entities merge. Test count unchanged at 1631. --- src/control/queries.rs | 2 +- src/node/dataplane/peer_actions.rs | 23 +++-- src/node/dataplane/rx_loop.rs | 2 +- src/node/handlers/timeout.rs | 138 ++++++++++++++++++++--------- src/node/mod.rs | 13 ++- src/node/tests/handshake.rs | 50 +++++++---- src/peer/machine.rs | 30 +++++++ src/proto/fmp/core.rs | 7 -- 8 files changed, 188 insertions(+), 77 deletions(-) diff --git a/src/control/queries.rs b/src/control/queries.rs index fd1d6d3..b5bbe1c 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -1310,7 +1310,7 @@ pub fn show_connections(node: &Node) -> Value { "handshake_state": format!("{}", conn.handshake_state()), "started_at_ms": conn.started_at(), "idle_ms": now.saturating_sub(conn.last_activity()), - "resend_count": conn.resend_count(), + "resend_count": node.connection_resend_count(conn.link_id()), }); if let Some(identity) = conn.expected_identity() { diff --git a/src/node/dataplane/peer_actions.rs b/src/node/dataplane/peer_actions.rs index 2b71a6c..01520b5 100644 --- a/src/node/dataplane/peer_actions.rs +++ b/src/node/dataplane/peer_actions.rs @@ -21,11 +21,11 @@ //! link-control frames, and the connected-UDP plane. `RegisterDecryptSession` is //! a deliberate no-op — see its arm for the note. //! -//! The timer arms (`SetTimer`/`CancelTimer`) are wired to populate/clear the -//! per-peer timer store (`peer_timers`), but that store is a SHADOW: the legacy -//! tick (`check_timeouts`/`resend_pending_handshakes`) still does all real timer -//! work and no `PeerEvent::Timeout` is fed, so the arms are behavior-neutral -//! until the handshake-kind driver fold cuts the legacy paths over. +//! The timer arms (`SetTimer`/`CancelTimer`) populate/clear the per-peer timer +//! store (`peer_timers`). The `HandshakeRetransmit` deadline is read and fired +//! by `drive_peer_timers` (the msg1-resend home). The `HandshakeTimeout` and +//! rekey/liveness kinds are still SHADOW — driven by the legacy `check_timeouts` +//! / rekey shell drivers — so populating them stays behavior-neutral. use crate::PeerIdentity; use crate::node::Node; @@ -500,13 +500,12 @@ impl Node { // Connected-UDP plane ownership (`connected_udp.rs`). } PeerAction::SetTimer { kind, at_ms } => { - // Populate the driver's per-peer timer store (overwrite = - // reschedule). SHADOW ONLY at this rung: the legacy tick - // (`check_timeouts`/`resend_pending_handshakes`) still does - // all real work, and no `PeerEvent::Timeout` is fed yet, so - // this is behavior-neutral. The handshake-kind fold (C5-2b/c) - // adds the driver that reads this store and deletes the - // overlapping legacy paths. + // Populate the per-peer timer store (overwrite = reschedule). + // The `HandshakeRetransmit` deadline is now read + fired by + // `drive_peer_timers` (the msg1-resend home). Other kinds are + // still SHADOW here — `HandshakeTimeout` is driven by the + // legacy `check_timeouts`, and rekey/liveness keep their own + // shell drivers — so populating them stays behavior-neutral. self.peer_timers .entry(link) .or_default() diff --git a/src/node/dataplane/rx_loop.rs b/src/node/dataplane/rx_loop.rs index acf7efb..315a7f7 100644 --- a/src/node/dataplane/rx_loop.rs +++ b/src/node/dataplane/rx_loop.rs @@ -334,7 +334,7 @@ impl Node { self.poll_pending_connects().await; self.poll_nostr_rendezvous().await; self.poll_lan_rendezvous().await; - self.resend_pending_handshakes(now_ms).await; + self.drive_peer_timers(now_ms).await; self.resend_pending_rekeys(now_ms).await; self.resend_pending_session_handshakes(now_ms).await; self.resend_pending_session_msg3(now_ms).await; diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index 907df03..d892717 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -2,7 +2,7 @@ //! and handshake message resend scheduling. use crate::node::Node; -use crate::peer::HandshakeState; +use crate::peer::machine::TimerKind; use crate::proto::fmp::{ ConnAction, ConnSnapshot, LifecycleView, PeerSnapshot, RekeyResendSnapshot, }; @@ -24,28 +24,6 @@ impl LifecycleView for Node { .collect() } - fn resend_candidates(&self, now_ms: u64, max_resends: u32) -> Vec { - self.connections - .iter() - .filter(|(_, conn)| { - conn.is_outbound() - && conn.handshake_state() == HandshakeState::SentMsg1 - && conn.resend_count() < max_resends - && conn.next_resend_at_ms() > 0 - && now_ms >= conn.next_resend_at_ms() - }) - .filter_map(|(link_id, conn)| { - conn.handshake_msg1().map(|msg1| ConnSnapshot { - link: *link_id, - is_outbound: true, - retry_addr: None, - resend_count: conn.resend_count(), - msg1: msg1.to_vec(), - }) - }) - .collect() - } - fn rekey_peers(&self) -> Vec { // The snapshot builder lives in `rekey` beside its drain/dampening // constants; the read-seam unifies here. @@ -137,12 +115,26 @@ impl Node { } } - /// Resend handshake messages for pending connections. + /// Drive the per-peer machine timers that have come due on this tick. /// - /// For outbound connections in SentMsg1 state, resends the stored msg1 - /// with exponential backoff. Called periodically from the RX event loop. - pub(in crate::node) async fn resend_pending_handshakes(&mut self, now_ms: u64) { - if self.connections.is_empty() { + /// The sans-IO machine arms `SetTimer`/`CancelTimer` actions into + /// [`peer_timers`](Node::peer_timers); this is the shell driver that fires + /// the due ones. At this rung only the msg1-retransmit timer is driven (the + /// handshake-timeout timer stays on the legacy [`check_timeouts`](Self::check_timeouts) + /// path until the next fold); the rekey/liveness kinds keep their own shell + /// drivers, so this deliberately kind-filters to `HandshakeRetransmit`. + /// + /// Retransmit is the pre-fold `resend_pending_handshakes` logic, re-homed: + /// the *due* signal is now the machine-armed timer (not the connection's + /// `next_resend_at_ms`), and the resend counter lives on the machine (the + /// operator-visible count reads from there). The wire bytes and transport + /// target still come from the shell connection, the pure core computes the + /// backoff schedule, and — matching the old shell exactly — the count and + /// reschedule advance only on a successful send; a failed send neither + /// advances the count nor marks the connection failed, it just retries next + /// tick. + pub(in crate::node) async fn drive_peer_timers(&mut self, now_ms: u64) { + if self.peer_timers.is_empty() { return; } @@ -150,9 +142,64 @@ impl Node { let interval_ms = self.config().node.rate_limit.handshake_resend_interval_ms; let backoff = self.config().node.rate_limit.handshake_resend_backoff; - // The shell resolves the resend-candidate predicate and copies the - // opaque msg1 bytes; the core computes the backoff schedule. - let candidates = self.resend_candidates(now_ms, max_resends); + // Collect due retransmit timers (kind-filtered). + let due: Vec = self + .peer_timers + .iter() + .filter(|(_, timers)| { + timers + .get(&TimerKind::HandshakeRetransmit) + .is_some_and(|&at_ms| now_ms >= at_ms) + }) + .map(|(link, _)| *link) + .collect(); + if due.is_empty() { + return; + } + + // Classify each due link against the machine + connection. A timer whose + // machine has left `SentMsg1` (promoted/gone) or has hit the resend cap + // is dropped — no more resends, exactly as the old shell stopped + // selecting a capped/settled connection; the handshake-timeout reaper + // takes it from there. + let mut candidates: Vec = Vec::new(); + let mut drop_timers: Vec = Vec::new(); + for link in due { + let armed = match self.peer_machines.get(&link) { + Some(machine) + if machine.is_handshaking_sent_msg1() + && machine.resend_count() < max_resends => + { + machine.resend_count() + } + Some(_) => { + drop_timers.push(link); + continue; + } + None => { + drop_timers.push(link); + continue; + } + }; + match self.connections.get(&link).and_then(|c| c.handshake_msg1()) { + // Armed but the stored wire isn't there yet — leave the timer and + // retry next tick (matches the old candidate filter skipping it). + None => continue, + Some(msg1) => candidates.push(ConnSnapshot { + link, + is_outbound: true, + retry_addr: None, + resend_count: armed, + msg1: msg1.to_vec(), + }), + } + } + for link in drop_timers { + if let Some(timers) = self.peer_timers.get_mut(&link) { + timers.remove(&TimerKind::HandshakeRetransmit); + } + } + for action in self .fmp .poll_resends(candidates, now_ms, interval_ms, backoff) @@ -166,7 +213,6 @@ impl Node { continue; }; - // Get transport and address info from the connection let (transport_id, remote_addr) = match self.connections.get(&link) { Some(conn) => match (conn.transport_id(), conn.source_addr()) { (Some(tid), Some(addr)) => (tid, addr.clone()), @@ -175,7 +221,6 @@ impl Node { None => continue, }; - // Send the stored msg1 let sent = if let Some(transport) = self.transports.get(&transport_id) { match transport.send(&remote_addr, &bytes).await { Ok(_) => true, @@ -192,13 +237,26 @@ impl Node { false }; - if sent && let Some(conn) = self.connections.get_mut(&link) { - conn.record_resend(next_resend_at_ms); - debug!( - link_id = %link, - resend = conn.resend_count(), - "Resent handshake msg1" - ); + if sent { + if let Some(machine) = self.peer_machines.get_mut(&link) { + machine.record_resend(next_resend_at_ms); + debug!( + link_id = %link, + resend = machine.resend_count(), + "Resent handshake msg1" + ); + } + self.peer_timers + .entry(link) + .or_default() + .insert(TimerKind::HandshakeRetransmit, next_resend_at_ms); + } else { + // Failed send: keep retrying at the tick cadence (the old shell + // left next_resend_at_ms unchanged so the connection stayed due). + self.peer_timers + .entry(link) + .or_default() + .insert(TimerKind::HandshakeRetransmit, now_ms); } } } diff --git a/src/node/mod.rs b/src/node/mod.rs index b490e07..fb939b8 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1984,7 +1984,7 @@ impl Node { handshake_state: format!("{}", conn.handshake_state()), started_at_ms: conn.started_at(), last_activity_ms: conn.last_activity(), - resend_count: conn.resend_count(), + resend_count: self.connection_resend_count(conn.link_id()), expected_peer: conn.expected_identity().map(|id| id.npub()), }) .collect(); @@ -2268,6 +2268,17 @@ impl Node { self.peer_timers.remove(&link); } + /// Operator-visible msg1 resend count for a pending handshake `link`, read + /// from the per-peer machine (the counter's home once the resend drive moved + /// off the shell connection). Machine-less connections (inbound legs that + /// never resend, and test-created connections) report 0, matching what the + /// shell connection reported before the counter moved. + pub(crate) fn connection_resend_count(&self, link: LinkId) -> u32 { + self.peer_machines + .get(&link) + .map_or(0, |machine| machine.resend_count()) + } + pub(crate) fn cleanup_bootstrap_transport_if_unused(&mut self, transport_id: TransportId) { if !self .supervisor diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index cc999c5..ddabf11 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -858,28 +858,48 @@ async fn test_resend_scheduling() { ); node.links.insert(link_id, link); node.addr_to_link - .insert((transport_id, remote_addr), link_id); + .insert((transport_id, remote_addr.clone()), link_id); node.pending_outbound .insert((transport_id, our_index.as_u32()), link_id); node.connections.insert(link_id, conn); - // Before resend time: nothing should happen (no transport = can't send, - // but the filter should exclude it because now < next_resend_at) - node.resend_pending_handshakes(now_ms + 500).await; - let conn = node.connections.get(&link_id).unwrap(); - assert_eq!(conn.resend_count(), 0, "No resend before scheduled time"); + // The msg1-resend counter and its due timer live on the per-peer machine. + // Dial it to `SentMsg1` (connectionless: no connect step) and arm its + // retransmit timer at now + 1000ms, mirroring what a real dial arms. + let mut machine = + crate::peer::machine::PeerMachine::new_outbound(link_id, peer_identity, now_ms); + let _ = machine.step( + crate::peer::machine::PeerEvent::Dial { + transport_id, + remote_addr: remote_addr.clone(), + peer_identity, + connection_oriented: false, + }, + now_ms, + &mut node.index_allocator, + ); + node.peer_machines.insert(link_id, machine); + node.peer_timers.entry(link_id).or_default().insert( + crate::peer::machine::TimerKind::HandshakeRetransmit, + now_ms + 1000, + ); - // At resend time: would resend if transport existed. Without transport, - // the send fails silently and resend_count stays at 0. - // This tests the filtering logic — the connection IS a candidate. - node.resend_pending_handshakes(now_ms + 1000).await; - // No transport registered, so send fails — count stays 0. - // That's the expected behavior (transport absence is a transient condition). - let conn = node.connections.get(&link_id).unwrap(); + // Before the scheduled time the timer isn't due, so nothing fires. + node.drive_peer_timers(now_ms + 500).await; assert_eq!( - conn.resend_count(), + node.connection_resend_count(link_id), 0, - "No transport means no resend recorded" + "No resend before scheduled time" + ); + + // At the scheduled time the timer is due, but no transport is registered so + // the send fails. Record-on-success: the count does NOT advance (and the + // connection is not marked failed) — a failed resend just retries next tick. + node.drive_peer_timers(now_ms + 1000).await; + assert_eq!( + node.connection_resend_count(link_id), + 0, + "Failed send records no resend" ); } diff --git a/src/peer/machine.rs b/src/peer/machine.rs index b7b9c4d..f741256 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -446,6 +446,36 @@ impl PeerMachine { self.our_index } + /// The msg1 resend count for this outbound handshake leg. The per-peer + /// machine is the home for this counter — the timer driver advances it via + /// [`record_resend`](Self::record_resend) on each successful resend, and the + /// control-socket connection snapshot reads it here so the operator-visible + /// count follows the machine rather than the (now inert) shell connection. + pub(crate) fn resend_count(&self) -> u32 { + self.conn.resend_count() + } + + /// Record a successful msg1 resend: advance the count and store the next + /// backoff deadline. The driver calls this only after the resend actually + /// went out (record-on-success — a failed send neither advances the count + /// nor reschedules), matching the pre-fold shell semantics. + pub(crate) fn record_resend(&mut self, next_resend_at_ms: u64) { + self.conn.record_resend(next_resend_at_ms); + } + + /// Whether this is an outbound leg parked at `SentMsg1` — the only state in + /// which a msg1 resend is due. Mirrors `on_handshake_retransmit`'s guard so + /// the shell timer driver can gate without reaching into machine state. + pub(crate) fn is_handshaking_sent_msg1(&self) -> bool { + matches!( + self.state, + PeerState::Handshaking { + phase: HandshakePhase::SentMsg1, + .. + } + ) + } + /// The crystallized node address, if identity is known. fn addr(&self) -> Option { self.identity.map(|id| *id.node_addr()) diff --git a/src/proto/fmp/core.rs b/src/proto/fmp/core.rs index a3d1e07..4d46ce2 100644 --- a/src/proto/fmp/core.rs +++ b/src/proto/fmp/core.rs @@ -328,13 +328,6 @@ pub(crate) trait LifecycleView { /// timeout/failed predicate; the core decides retry-then-teardown. fn stale_connections(&self, now_ms: u64, timeout_ms: u64) -> Vec; - /// Snapshot every outbound connection whose stored msg1 is due for a - /// resend as of `now_ms` and still under `max_resends`. The shell resolves - /// the "outbound, in `SentMsg1`, has stored msg1, under budget, past the - /// scheduled time" predicate and copies the opaque msg1 bytes; the core - /// computes the backoff schedule. - fn resend_candidates(&self, now_ms: u64, max_resends: u32) -> Vec; - /// Snapshot every active peer with a session that is healthy, pre-computing /// its rekey-relevant ages and timer predicates (see [`PeerSnapshot`]). The /// shell resolves every clock read here; the core applies the thresholds. From 765819f52b383a81a4f17dd417511aaf354b1384 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 15 Jul 2026 20:23:49 +0000 Subject: [PATCH 3/3] node: reap the handshake timeout through the per-peer machine timer Move the outbound handshake-timeout reap off the unconditional check_timeouts scan onto the machine-armed HandshakeTimeout timer. A new drive_handshake_timeouts (run before the retransmit drive, so a timed-out leg is reaped rather than resent on the same tick) reaps the outbound legs that carry a HandshakeTimeout timer and have idle-timed-out this tick. The timer's presence selects the leg (only outbound legs arm one; IK inbound arms none); the reap threshold is the shell is_timed_out(now, config) predicate, not the timer's stored deadline. The machine arms the timer from a hardcoded constant at dial, which is not authoritative for an operator-tuned handshake_timeout_secs, so reading the threshold from config each tick keeps the reap neutral for any timeout value and on the last_activity clock exactly as before. check_timeouts keeps reaping everything else: failed connections (all of them, promptly) and the idle-timeout of legs without a machine timer (inbound legs, and any machine-less connection). Total coverage is unchanged. check_timeouts runs before the timer drive, so a failed outbound leg is reaped there first, its timers dropped, and the timeout drive never double-fires on it. Split drive_peer_timers into the timeout drive and the (byte-identical) retransmit drive. The dormant machine timeout handler is not dispatched, so the session index is freed once, by cleanup_stale_connection. A regression test drives a timed-out outbound leg to reap; the existing check_timeouts tests continue to anchor the residual sweep. Test count 1631 to 1632. --- src/node/dataplane/peer_actions.rs | 17 +++-- src/node/handlers/timeout.rs | 104 ++++++++++++++++++++++++----- src/node/tests/handshake.rs | 77 +++++++++++++++++++++ 3 files changed, 172 insertions(+), 26 deletions(-) diff --git a/src/node/dataplane/peer_actions.rs b/src/node/dataplane/peer_actions.rs index 01520b5..bd171f6 100644 --- a/src/node/dataplane/peer_actions.rs +++ b/src/node/dataplane/peer_actions.rs @@ -22,10 +22,10 @@ //! a deliberate no-op — see its arm for the note. //! //! The timer arms (`SetTimer`/`CancelTimer`) populate/clear the per-peer timer -//! store (`peer_timers`). The `HandshakeRetransmit` deadline is read and fired -//! by `drive_peer_timers` (the msg1-resend home). The `HandshakeTimeout` and -//! rekey/liveness kinds are still SHADOW — driven by the legacy `check_timeouts` -//! / rekey shell drivers — so populating them stays behavior-neutral. +//! store (`peer_timers`). The `HandshakeRetransmit` and `HandshakeTimeout` +//! deadlines are read and fired by `drive_peer_timers` (the handshake resend + +//! reap home). The rekey/liveness kinds are still SHADOW — driven by their own +//! shell drivers — so populating them stays behavior-neutral. use crate::PeerIdentity; use crate::node::Node; @@ -501,11 +501,10 @@ impl Node { } PeerAction::SetTimer { kind, at_ms } => { // Populate the per-peer timer store (overwrite = reschedule). - // The `HandshakeRetransmit` deadline is now read + fired by - // `drive_peer_timers` (the msg1-resend home). Other kinds are - // still SHADOW here — `HandshakeTimeout` is driven by the - // legacy `check_timeouts`, and rekey/liveness keep their own - // shell drivers — so populating them stays behavior-neutral. + // The `HandshakeRetransmit` and `HandshakeTimeout` deadlines + // are read + fired by `drive_peer_timers`. Rekey/liveness kinds + // are still SHADOW here — they keep their own shell drivers — + // so populating them stays behavior-neutral. self.peer_timers .entry(link) .or_default() diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index d892717..42e6e7c 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -11,9 +11,21 @@ use tracing::{debug, info}; impl LifecycleView for Node { fn stale_connections(&self, now_ms: u64, timeout_ms: u64) -> Vec { + // `is_failed()` legs are always reaped here (~1s), as before. The + // idle-timeout is reaped here ONLY for legs whose timeout is not already + // driven by a machine `HandshakeTimeout` timer — i.e. inbound legs (IK + // inbound arms none) and machine-less test connections. Outbound legs with + // an armed timer are reaped by `drive_handshake_timeouts`, so excluding + // them here avoids a double reap. self.connections .iter() - .filter(|(_, conn)| conn.is_timed_out(now_ms, timeout_ms) || conn.is_failed()) + .filter(|(link_id, conn)| { + conn.is_failed() + || (conn.is_timed_out(now_ms, timeout_ms) + && !self.peer_timers.get(*link_id).is_some_and(|timers| { + timers.contains_key(&TimerKind::HandshakeTimeout) + })) + }) .map(|(link_id, conn)| ConnSnapshot { link: *link_id, is_outbound: conn.is_outbound(), @@ -115,29 +127,87 @@ impl Node { } } - /// Drive the per-peer machine timers that have come due on this tick. + /// Act on the per-peer machine timers this tick. /// /// The sans-IO machine arms `SetTimer`/`CancelTimer` actions into - /// [`peer_timers`](Node::peer_timers); this is the shell driver that fires - /// the due ones. At this rung only the msg1-retransmit timer is driven (the - /// handshake-timeout timer stays on the legacy [`check_timeouts`](Self::check_timeouts) - /// path until the next fold); the rekey/liveness kinds keep their own shell - /// drivers, so this deliberately kind-filters to `HandshakeRetransmit`. - /// - /// Retransmit is the pre-fold `resend_pending_handshakes` logic, re-homed: - /// the *due* signal is now the machine-armed timer (not the connection's - /// `next_resend_at_ms`), and the resend counter lives on the machine (the - /// operator-visible count reads from there). The wire bytes and transport - /// target still come from the shell connection, the pure core computes the - /// backoff schedule, and — matching the old shell exactly — the count and - /// reschedule advance only on a successful send; a failed send neither - /// advances the count nor marks the connection failed, it just retries next - /// tick. + /// [`peer_timers`](Node::peer_timers); this is the shell driver that acts on + /// them: timeout reaps idle-timed-out outbound legs, retransmit resends the + /// due msg1s. Handshake-TIMEOUT is driven before handshake-RETRANSMIT so a + /// timed-out leg is reaped rather than resent on the same tick. The + /// rekey/liveness kinds keep their own shell drivers, so only the two + /// handshake kinds are driven here. pub(in crate::node) async fn drive_peer_timers(&mut self, now_ms: u64) { if self.peer_timers.is_empty() { return; } + self.drive_handshake_timeouts(now_ms); + self.drive_handshake_retransmits(now_ms).await; + } + /// Reap the outbound legs whose machine `HandshakeTimeout` timer marks them + /// as machine-timeout-owned and which have idle-timed-out this tick. + /// + /// The timer's PRESENCE selects the leg (only OUTBOUND legs arm one — IK + /// inbound arms none); the reap THRESHOLD is the shell `is_timed_out(now, + /// config)` predicate, NOT the timer's stored deadline. This matters because + /// the machine arms the timer from a hardcoded constant at dial, which is not + /// authoritative for an operator-tuned `handshake_timeout_secs` — reading the + /// threshold from config each tick keeps the reap neutral for any config, and + /// off the `last_activity` clock exactly as the old `check_timeouts` did. A + /// timed-out leg is reaped by the old Teardown path: the outbound retry + /// reflex, then `cleanup_stale_connection` (which drops the machine + timers). + /// + /// `check_timeouts` keeps reaping everything else — `is_failed()` legs and the + /// idle-timeout of legs without a machine timer (inbound / machine-less). + fn drive_handshake_timeouts(&mut self, now_ms: u64) { + let timeout_ms = self.config().node.rate_limit.handshake_timeout_secs * 1000; + let timer_links: Vec = self + .peer_timers + .iter() + .filter(|(_, timers)| timers.contains_key(&TimerKind::HandshakeTimeout)) + .map(|(link, _)| *link) + .collect(); + for link in timer_links { + let (reap, retry_peer) = match self.connections.get(&link) { + Some(conn) if conn.is_timed_out(now_ms, timeout_ms) => { + let retry_peer = if conn.is_outbound() { + conn.expected_identity().map(|id| *id.node_addr()) + } else { + None + }; + (true, retry_peer) + } + // Not yet idle-timed-out: leave the timer for a later tick. + Some(_) => (false, None), + None => { + // Orphan timer (connection already reaped elsewhere) — drop it. + if let Some(timers) = self.peer_timers.get_mut(&link) { + timers.remove(&TimerKind::HandshakeTimeout); + } + (false, None) + } + }; + if reap { + if let Some(peer) = retry_peer { + self.note_handshake_timeout(peer, now_ms); + } + debug!(link_id = %link, "Handshake connection timed out"); + self.cleanup_stale_connection(link, now_ms); + } + } + } + + /// Fire due handshake-retransmit timers: resend the stored msg1. + /// + /// The pre-fold `resend_pending_handshakes` logic, re-homed: the *due* signal + /// is the machine-armed timer (not the connection's `next_resend_at_ms`), and + /// the resend counter lives on the machine (the operator-visible count reads + /// from there). The wire bytes and transport target still come from the shell + /// connection, the pure core computes the backoff schedule, and — matching the + /// old shell exactly — the count and reschedule advance only on a successful + /// send; a failed send neither advances the count nor marks the connection + /// failed, it just retries next tick. + async fn drive_handshake_retransmits(&mut self, now_ms: u64) { let max_resends = self.config().node.rate_limit.handshake_max_resends; let interval_ms = self.config().node.rate_limit.handshake_resend_interval_ms; let backoff = self.config().node.rate_limit.handshake_resend_backoff; diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index ddabf11..9dcc0a2 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -903,6 +903,83 @@ async fn test_resend_scheduling() { ); } +/// Test that the timer driver reaps an outbound leg whose machine +/// `HandshakeTimeout` timer has come due (the timeout fold). The reap re-checks +/// the shell `is_timed_out` predicate, then tears the connection down exactly as +/// the old `check_timeouts` Teardown path did. +#[tokio::test] +async fn test_handshake_timeout_drive() { + let mut node = make_node(); + let transport_id = TransportId::new(1); + let peer_identity = make_peer_identity(); + let remote_addr = TransportAddr::from_string("10.0.0.2:2121"); + + let dial_ms = 1000u64; + let link_id = node.allocate_link_id(); + let mut conn = PeerConnection::outbound(link_id, peer_identity, dial_ms); + let our_index = node.index_allocator.allocate().unwrap(); + let our_keypair = node.identity().keypair(); + let _ = conn + .start_handshake(our_keypair, node.startup_epoch(), dial_ms) + .unwrap(); + conn.set_our_index(our_index); + conn.set_transport_id(transport_id); + conn.set_source_addr(remote_addr.clone()); + + let link = Link::connectionless( + link_id, + transport_id, + remote_addr.clone(), + LinkDirection::Outbound, + Duration::from_millis(100), + ); + node.links.insert(link_id, link); + node.addr_to_link + .insert((transport_id, remote_addr.clone()), link_id); + node.connections.insert(link_id, conn); + node.pending_outbound + .insert((transport_id, our_index.as_u32()), link_id); + + // Machine in SentMsg1 with a HandshakeTimeout timer armed at dial + 30s. + let mut machine = + crate::peer::machine::PeerMachine::new_outbound(link_id, peer_identity, dial_ms); + let _ = machine.step( + crate::peer::machine::PeerEvent::Dial { + transport_id, + remote_addr: remote_addr.clone(), + peer_identity, + connection_oriented: false, + }, + dial_ms, + &mut node.index_allocator, + ); + node.peer_machines.insert(link_id, machine); + node.peer_timers.entry(link_id).or_default().insert( + crate::peer::machine::TimerKind::HandshakeTimeout, + dial_ms + 30_000, + ); + + assert_eq!(node.connection_count(), 1); + + // Well past dial + 30s: the timer is due and the leg is idle-timed-out. + node.drive_peer_timers(dial_ms + 100_000).await; + + assert_eq!( + node.connection_count(), + 0, + "Timed-out leg reaped by the timer drive" + ); + assert_eq!(node.index_allocator.count(), 0, "Session index freed"); + assert!( + !node.peer_machines.contains_key(&link_id), + "Control machine dropped with the reaped connection" + ); + assert!( + !node.peer_timers.contains_key(&link_id), + "Timer store dropped with the reaped connection" + ); +} + /// Test that msg2 is stored on PeerConnection for responder resend. #[test] fn test_msg2_stored_on_connection() {