From eadc3257d880b393ad117d40de550f549bf55c22 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 13:45:08 +0000 Subject: [PATCH 01/11] fix(control): report a peer silent past the heartbeat interval as stale show_peers, and the peer row the tick publishes for the control socket to serve, printed a peer's connectivity from a state stored on the active peer. That state starts at connected on promotion and nothing outside the tests ever changes it, so every peer read connected for as long as it stayed in the peer map, including one that had stopped answering and was waiting out its link-dead timeout. Both render sites now derive the value from how long the peer has been silent: connected while its idle time is at or below heartbeat_interval_secs, floored at one second, and stale above it. That is the rule discovery already applies when deciding whether to re-dial an active peer on the path it already has, so the rule moves to one helper on Node that the re-dial gate and both renders call. stale was already one of the field's values, so the set of values a client can see does not grow, and the response shape is unchanged. The stored state and its other readers are left as they are. The open-discovery tutorial described reconnecting and disconnected values that never occur, and the advertise-your-node tutorial said a new peer would read active, which the field never reports. Both now describe what the field reports. The new tests insert peers last heard from at chosen times and read show_peers both on the loop and from the tick-published snapshot. Under the default 10 s interval a peer silent for 15 s reads stale and one heard from just now reads connected; with a 30 s interval a peer silent for 15 s still reads connected, so the threshold is the configured interval and not a fixed ten seconds. Both failed on the unfixed code, which reported the silent peer as connected. A third test pins the boundary: connected at exactly the interval, stale one millisecond past it, and a zero interval floored at one second. Restoring the stored read at either render site alone fails both show_peers tests on that render, making the comparison inclusive fails the boundary test, and a fixed ten-second threshold fails the configured-interval and boundary tests. --- CHANGELOG.md | 10 +++ docs/tutorials/advertise-your-node.md | 2 +- docs/tutorials/open-discovery.md | 13 ++-- src/control/queries.rs | 3 +- src/node/lifecycle/mod.rs | 8 +- src/node/mod.rs | 34 +++++++- src/node/tests/control.rs | 108 ++++++++++++++++++++++++++ src/node/tests/heartbeat.rs | 2 +- 8 files changed, 163 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ede84835..3c75f428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 link was created with. The counters cover authenticated link frames only, so they are not expected to match the transport totals in `show_transports`. The response shape is unchanged. +- `show_peers` (`fipsctl show peers`) now reports a peer that has gone quiet + as `stale`. Its `connectivity` was read from a state that nothing outside + the tests ever changed, so every peer read `connected` until it was + removed, including one that had stopped answering tens of seconds earlier. + The value is now derived from how long the peer has been silent: `connected` + while its idle time is at or below `heartbeat_interval_secs`, and `stale` + above it, the same rule that decides whether discovery re-dials an active + peer on the path it already has. The `reconnecting` and `disconnected` + values the open-discovery tutorial described never occurred, and the + tutorial no longer lists them. The response shape is unchanged. ### Changed diff --git a/docs/tutorials/advertise-your-node.md b/docs/tutorials/advertise-your-node.md index b22b4154..7f2ffeb2 100644 --- a/docs/tutorials/advertise-your-node.md +++ b/docs/tutorials/advertise-your-node.md @@ -284,7 +284,7 @@ sudo fipsctl show peers In addition to your configured `test-us01` peer, you may see an entry for `test-us03` (the open-discovery test mesh node). -It will have `connectivity` active and its own +It will have `connectivity` `connected` and its own `transport_addr`. This peering appeared without you configuring anything — the test-mesh open-discovery node saw your advert, dialed the endpoint, and Noise IK established diff --git a/docs/tutorials/open-discovery.md b/docs/tutorials/open-discovery.md index 815897f8..0f13af04 100644 --- a/docs/tutorials/open-discovery.md +++ b/docs/tutorials/open-discovery.md @@ -200,11 +200,14 @@ You should see considerably more entries than before: Each entry has its own `connectivity` state, and every entry that appears here completed a handshake at least once: a peer whose advert was stale, or that NAT traversal never reached, -produces no entry at all rather than a failed one. Healthy links -read `connected`. A link not heard from recently reads `stale` -and still carries traffic; one that dropped and is being retried -reads `reconnecting`, and one explicitly torn down reads -`disconnected`. Neither of the last two can send. +produces no entry at all rather than a failed one. A link heard +from within the last heartbeat interval +(`node.heartbeat_interval_secs`, 10 seconds by default) reads +`connected`. One silent for longer reads `stale`; it still +carries traffic, and it reads `connected` again as soon as the +peer is heard from. A link that stays silent until it is declared +dead is removed, so its entry disappears rather than changing +state. To get a list of just the connected links: diff --git a/src/control/queries.rs b/src/control/queries.rs index 5768e480..c1e03b73 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -250,6 +250,7 @@ pub fn show_peers(node: &Node) -> Value { // start (no peer has SRTT) every peer uses the default link cost of 1.0. let any_peer_has_srtt = node.peers().any(|p| p.has_srtt()); + let now = now_ms(); let peers: Vec = node .peers() .map(|peer| { @@ -267,7 +268,7 @@ pub fn show_peers(node: &Node) -> Value { "npub": peer.npub(), "display_name": node.peer_display_name(&node_addr), "ipv6_addr": format!("{}", peer.address()), - "connectivity": format!("{}", peer.connectivity()), + "connectivity": format!("{}", node.peer_connectivity(peer, now)), "link_id": peer.link_id().as_u64(), "authenticated_at_ms": peer.authenticated_at(), "last_seen_ms": peer.last_seen(), diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 1e40b272..f30cb3d8 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -3171,13 +3171,7 @@ impl Node { let Some(peer) = self.peers.get(peer_node_addr) else { return false; }; - let stale_after_ms = self - .config() - .node - .heartbeat_interval_secs - .saturating_mul(1000) - .max(1000); - peer.idle_time(Self::now_ms()) > stale_after_ms + self.peer_link_is_stale(peer, Self::now_ms()) } fn active_peer_matches_any_candidate( diff --git a/src/node/mod.rs b/src/node/mod.rs index ca4f2877..aff6a43a 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -43,8 +43,8 @@ use self::reloadable::Reloadable; pub(crate) const REKEY_JITTER_SECS: i64 = 15; use crate::cache::CoordCache; use crate::node::session::SessionEntry; -use crate::peer::ActivePeer; use crate::peer::machine::{PeerMachine, TimerKind}; +use crate::peer::{ActivePeer, ConnectivityState}; use crate::proto::bloom::{BloomFilter, BloomState}; use crate::proto::fmp::Fmp; use crate::proto::fmp::wire::{ @@ -2141,6 +2141,7 @@ impl Node { // SRTT) every peer falls back to the default link cost of 1.0. let any_peer_has_srtt = self.peers().any(|p| p.has_srtt()); + let now_ms = Self::now_ms(); let peer_rows: Vec = self .peers() .map(|peer| { @@ -2193,7 +2194,7 @@ impl Node { npub: peer.npub(), display_name: self.peer_display_name(&node_addr), ipv6_addr: format!("{}", peer.address()), - connectivity: format!("{}", peer.connectivity()), + connectivity: format!("{}", self.peer_connectivity(peer, now_ms)), link_id: peer.link_id().as_u64(), authenticated_at_ms: peer.authenticated_at(), last_seen_ms: peer.last_seen(), @@ -2841,6 +2842,35 @@ impl Node { self.peers.values() } + /// Whether an active peer has been silent at `now_ms` for longer than the + /// configured heartbeat interval, floored at one second. + /// + /// The one idle-time liveness rule: the control socket reports such a peer + /// as `stale`, and discovery re-dials it on the path it already has. + pub(in crate::node) fn peer_link_is_stale(&self, peer: &ActivePeer, now_ms: u64) -> bool { + let stale_after_ms = self + .config() + .node + .heartbeat_interval_secs + .saturating_mul(1000) + .max(1000); + peer.idle_time(now_ms) > stale_after_ms + } + + /// Connectivity of an active peer as the control socket reports it: + /// `Stale` when [`Self::peer_link_is_stale`] holds at `now_ms`, otherwise + /// `Connected`. + /// + /// Derived from idle time rather than read from the state stored on the + /// peer, which nothing in production changes after promotion. + pub(crate) fn peer_connectivity(&self, peer: &ActivePeer, now_ms: u64) -> ConnectivityState { + if self.peer_link_is_stale(peer, now_ms) { + ConnectivityState::Stale + } else { + ConnectivityState::Connected + } + } + /// Reference to the Nostr discovery handle if discovery is enabled. /// Used by control queries (`show_peers` per-peer Nostr-traversal /// state) to read failure-state without taking shared ownership. diff --git a/src/node/tests/control.rs b/src/node/tests/control.rs index 4bdf9fb8..dbb08b99 100644 --- a/src/node/tests/control.rs +++ b/src/node/tests/control.rs @@ -6,6 +6,7 @@ //! socket framing. use super::*; +use heartbeat::set_heartbeat_interval; use spanning_tree::{ TestNode, add_loopback_alias, cleanup_nodes, drain_all_packets, make_test_node, process_available_packets, run_tree_test, @@ -421,3 +422,110 @@ async fn show_links_reports_the_traffic_counters_of_the_peer_bound_to_each_link( cleanup_nodes(&mut nodes).await; } + +/// Insert an authenticated peer last heard from at `last_seen_ms` and return +/// its address. +fn insert_peer_last_seen_at(node: &mut Node, link: u64, last_seen_ms: u64) -> NodeAddr { + let identity = PeerIdentity::from_pubkey_full(Identity::generate().pubkey_full()); + let addr = *identity.node_addr(); + node.peers.insert( + addr, + ActivePeer::new(identity, LinkId::new(link), last_seen_ms), + ); + addr +} + +/// The `connectivity` string a `show_peers` response gives the peer at `addr`. +fn connectivity_of(peers: &serde_json::Value, addr: &NodeAddr) -> String { + let addr_hex = hex::encode(addr.as_bytes()); + peers["peers"] + .as_array() + .expect("show_peers returns a peers array") + .iter() + .find(|row| row["node_addr"] == addr_hex.as_str()) + .and_then(|row| row["connectivity"].as_str()) + .expect("show_peers lists the peer with a connectivity string") + .to_string() +} + +/// Render `show_peers` on the loop, then publish a tick and render it again +/// from the snapshot the control socket serves. +fn show_peers_both_renders(node: &mut Node) -> [(&'static str, serde_json::Value); 2] { + let on_loop = crate::control::queries::show_peers(node); + node.record_stats_history(); + let off_loop = crate::control::queries::show_peers_from_handle(&node.control_read_handle()); + [("on-loop", on_loop), ("snapshot", off_loop)] +} + +/// `show_peers` reports a peer silent for longer than the heartbeat interval +/// as `stale`, and a peer heard from just now as `connected`, on both the +/// on-loop render and the tick-published snapshot render. +#[test] +fn show_peers_reports_a_peer_idle_past_the_heartbeat_interval_as_stale() { + let mut node = make_node(); + let interval_ms = node.config().node.heartbeat_interval_secs * 1000; + assert_eq!(interval_ms, 10_000, "the default heartbeat interval"); + let now = Node::now_ms(); + let fresh = insert_peer_last_seen_at(&mut node, 1, now); + let idle = insert_peer_last_seen_at(&mut node, 2, now - interval_ms - 5_000); + + for (render, peers) in show_peers_both_renders(&mut node) { + assert_eq!( + connectivity_of(&peers, &idle), + "stale", + "{render} render, peer silent for 15 s" + ); + assert_eq!( + connectivity_of(&peers, &fresh), + "connected", + "{render} render, peer heard from just now" + ); + } +} + +/// The `stale` threshold is the configured heartbeat interval rather than a +/// fixed ten seconds: with a 30 s interval a peer silent for 15 s still reads +/// `connected`, and one silent for 35 s reads `stale`. +#[test] +fn show_peers_stale_threshold_follows_the_configured_heartbeat_interval() { + let mut node = make_node(); + set_heartbeat_interval(&mut node, 30); + let now = Node::now_ms(); + let quiet = insert_peer_last_seen_at(&mut node, 1, now - 15_000); + let idle = insert_peer_last_seen_at(&mut node, 2, now - 35_000); + + for (render, peers) in show_peers_both_renders(&mut node) { + assert_eq!( + connectivity_of(&peers, &idle), + "stale", + "{render} render, peer silent for 35 s" + ); + assert_eq!( + connectivity_of(&peers, &quiet), + "connected", + "{render} render, peer silent for 15 s" + ); + } +} + +/// The derived connectivity changes at the heartbeat interval exactly: a peer +/// silent for the whole interval still reads `connected`, and one millisecond +/// more reads `stale`. A zero interval is floored at one second, the floor the +/// discovery re-dial gate applies. +#[test] +fn peer_connectivity_turns_stale_one_millisecond_past_the_heartbeat_interval() { + let mut node = make_node(); + let seen = 1_000_000; + let addr = insert_peer_last_seen_at(&mut node, 1, seen); + let at = |node: &Node, now_ms: u64| { + let peer = node.get_peer(&addr).expect("the peer was inserted"); + node.peer_connectivity(peer, now_ms) + }; + + assert_eq!(at(&node, seen + 10_000), ConnectivityState::Connected); + assert_eq!(at(&node, seen + 10_001), ConnectivityState::Stale); + + set_heartbeat_interval(&mut node, 0); + assert_eq!(at(&node, seen + 1_000), ConnectivityState::Connected); + assert_eq!(at(&node, seen + 1_001), ConnectivityState::Stale); +} diff --git a/src/node/tests/heartbeat.rs b/src/node/tests/heartbeat.rs index 11e684f7..008d0968 100644 --- a/src/node/tests/heartbeat.rs +++ b/src/node/tests/heartbeat.rs @@ -37,7 +37,7 @@ fn set_link_dead_timeout(node: &mut crate::node::Node, secs: u64) { /// Set `heartbeat_interval_secs` on an already-constructed node, the same way /// `set_link_dead_timeout` does. -fn set_heartbeat_interval(node: &mut crate::node::Node, secs: u64) { +pub(super) fn set_heartbeat_interval(node: &mut crate::node::Node, secs: u64) { node.replace_context(|ctx| { let mut cfg = (*ctx.config).clone(); cfg.node.heartbeat_interval_secs = secs; From b37f7cdc21f32bc2ba32a2266d95abbc9262a92a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 13:57:03 +0000 Subject: [PATCH 02/11] fix(rekey): keep the rekey cycle when a msg2 fails to authenticate The rekey initiator took its handshake off the peer before reading msg2 and abandoned the whole cycle when the read failed. Nothing authenticates a msg2 ahead of that read, and the index it is dispatched on travels in cleartext in rekey msg1, so anyone on the path who saw the msg1 could answer it first with a msg2 of the right size under that index. That costs more than one cycle. The IK responder commits its new keys when it answers msg1 and cuts over on its own next rekey tick, while the initiator, having abandoned, drops the responder's genuine msg2 at the dispatch lookup. Frames from the responder then miss the initiator's index table at once, frames to the responder fail once its drain window closes, and each end removes the other on the link-dead timeout. With the tick functions driven in-process at one-second steps, the responder removed the link at about 30 s and the initiator at about 31 s, and the initiator's last msg1 resend left the responder holding a peer the initiator no longer had, removed 30 s after that. Read msg2 through a rolling-back variant of the IK read, modelled on the existing XK one, and put the handshake back on the peer when it fails. The msg2 handler keeps the cycle and its dispatch entry in that case and still counts the reject, so the genuine msg2 completes the rekey. The other failures, a missing handshake or one after the read succeeded, still abandon, and if no readable msg2 ever arrives the msg1 resend budget abandons the cycle as before. Each forged msg2 carrying the live index now costs the initiator the msg2 key agreement until the cycle ends, where before only the first one did; the resend budget bounds that. No wire format changes. The new test ages a two-node link past both rekey gates, delivers the rekey msg1, holds the responder's msg2, injects a forged one under the live index, releases the real one, runs a rekey tick on both nodes and checks delivery in both directions. It fails on the unfixed tree at the responder-to-initiator delivery, which is the check that separates the two outcomes, and fails there again with each part of the fix reverted on its own: the handshake put-back, the symmetric-state rollback, and keeping the dispatch entry. --- CHANGELOG.md | 18 +++ src/node/handlers/handshake.rs | 31 ++++- src/node/tests/session.rs | 227 +++++++++++++++++++++++++++++++++ src/noise/handshake.rs | 42 +++++- src/peer/active.rs | 23 +++- 5 files changed, 332 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c75f428..3e4c662e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 interval instead. That retry interval gates only a peer whose last attempt failed, so it cannot clamp a `heartbeat_interval_secs` configured below it. +#### Link rekey + +- A forged rekey msg2 no longer takes the link down. The rekey initiator gave + up its handshake before reading msg2 and abandoned the cycle when the read + failed, although nothing authenticates a msg2 ahead of that read. Anyone on + the path who saw the rekey msg1 go out could answer first with a msg2 of the + right size under the index msg1 carries in cleartext. The responder has + already committed its new session by then and cuts over on its next tick, so + the two ends were left on different keys: frames from the responder were + dropped at once, frames to it failed once its drain window closed, and each + end removed the other on the link-dead timeout about 30 s later. A msg2 that + fails the read now leaves the handshake as it was before the read, along + with the msg1 resend schedule and the msg2 dispatch entry, so the + responder's genuine msg2 still completes the rekey. In exchange, every such + forgery now costs the initiator the msg2 key agreement until the cycle ends, + where before only the first one did; the msg1 resend budget bounds that. The + wire format is unchanged. + #### Control socket - `show_links` (`fipsctl show links`) now reports the traffic a link has diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 5139e536..34727180 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -1183,6 +1183,7 @@ impl Node { // Complete the rekey handshake on the ActivePeer let mut rekey_completed = false; + let mut cycle_kept = false; if let Some(peer) = self.peers.get_mut(&peer_node_addr) { match peer.complete_rekey_msg2(noise_msg2) { Ok((session, remote_epoch)) => { @@ -1222,6 +1223,26 @@ impl Node { ); rekey_completed = true; } + // Nothing authenticated this msg2 before the read, and the + // index it names travels in cleartext in our msg1, so it + // may be a forgery. The responder committed its new + // session when it answered that msg1 and cuts over on its + // own tick, so abandoning here would leave the two ends on + // different keys. The read rolled the handshake back: + // keep the cycle and its dispatch entry so the genuine + // msg2 can still complete it. If no readable msg2 ever + // arrives, the msg1 resend budget abandons the cycle as + // it would for a lost one. + Err(e) if peer.awaits_msg2() => { + debug!( + peer = %display_name, + error = %e, + "Rekey msg2 did not authenticate, keeping the rekey cycle" + ); + cycle_kept = true; + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + } Err(e) => { warn!( peer = %display_name, @@ -1242,14 +1263,16 @@ impl Node { // Feed the control machine the completed-rekey observation so its // shadow index and rekey phase stay coherent. Only on success — - // the failure path above reverts the rekey and leaves the machine - // untouched. The crypto effect already ran inline; this emits no - // action. + // the failure paths above either keep the cycle as it was or + // revert it, and leave the machine untouched. The crypto effect + // already ran inline; this emits no action. if rekey_completed { self.observe_rekey_msg2(&peer_node_addr, header.sender_idx); } - self.pending_outbound.remove(&key); + if !cycle_kept { + self.pending_outbound.remove(&key); + } return; } diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 4e0255e6..46b5e8db 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -1222,6 +1222,233 @@ async fn rekey_cutover_preserves_data_plane() { cleanup_nodes(&mut nodes).await; } +/// A forged rekey msg2 that carries the initiator's live rekey index must not +/// split the link. +/// +/// An on-path observer sees the rekey msg1 go out, reads the initiator's +/// cleartext rekey index from it, and delivers a msg2 of the right size under +/// that index ahead of the responder's real reply. Under IK the forgery cannot +/// authenticate: only the responder's static key produces a msg2 the initiator +/// can read. The IK responder has already committed its new session when it +/// answered msg1 and cuts over on its own next rekey tick, so the initiator has +/// to keep the cycle through the forgery and complete it on the real msg2. An +/// initiator that gives the cycle up instead holds no session matching the one +/// the responder now sends on. +/// +/// Deterministic, no wall-clock wait: both sessions are backdated past node 0's +/// time trigger and node 1's rekey-acceptance floor, node 1 never initiates, +/// and every handshake message is delivered by hand. +#[tokio::test] +async fn forged_rekey_msg2_does_not_split_the_link() { + use crate::noise::HANDSHAKE_MSG2_SIZE; + use crate::proto::fmp::wire::{CommonPrefix, PHASE_MSG2, build_msg2}; + use crate::transport::ReceivedPacket; + use crate::utils::index::SessionIndex; + + const REKEY_AFTER_SECS: u64 = 60; + + // node 0 rekeys on time; node 1 only ever responds. + let mut cfg0 = crate::config::Config::new(); + cfg0.node.rekey.enabled = true; + cfg0.node.rekey.after_secs = REKEY_AFTER_SECS; + cfg0.node.rekey.after_messages = u64::MAX; + let mut cfg1 = crate::config::Config::new(); + cfg1.node.rekey.enabled = true; + cfg1.node.rekey.after_secs = u64::MAX; + cfg1.node.rekey.after_messages = u64::MAX; + + let mut nodes = vec![ + make_test_node_with_config(cfg0, 1280).await, + make_test_node_with_config(cfg1, 1280).await, + ]; + + // FMP peering + FSP session between the two loopback nodes. + initiate_handshake(&mut nodes, 0, 1).await; + drain_all_packets(&mut nodes, false).await; + let node0_addr = *nodes[0].node.node_addr(); + let node1_addr = *nodes[1].node.node_addr(); + assert!(nodes[0].node.get_peer(&node1_addr).is_some()); + assert!(nodes[1].node.get_peer(&node0_addr).is_some()); + populate_all_coord_caches(&mut nodes); + + let node1_pubkey = nodes[1].node.identity().pubkey_full(); + nodes[0] + .node + .initiate_session(node1_addr, node1_pubkey) + .await + .unwrap(); + for _ in 0..4 { + tokio::time::sleep(Duration::from_millis(10)).await; + process_available_packets(&mut nodes).await; + } + for (i, remote) in [(0, node1_addr), (1, node0_addr)] { + assert!( + nodes[i] + .node + .get_session(&remote) + .is_some_and(|s| s.state().is_established()), + "node {i} session established" + ); + } + + // Each node's TUN receiver observes the plaintext the other one sent. + let (tun0_tx, tun0_rx) = std::sync::mpsc::channel(); + nodes[0].node.supervisor.tun_tx = Some(tun0_tx); + let (tun1_tx, tun1_rx) = std::sync::mpsc::channel(); + nodes[1].node.supervisor.tun_tx = Some(tun1_tx); + let fips0 = crate::FipsAddress::from_node_addr(&node0_addr); + let fips1 = crate::FipsAddress::from_node_addr(&node1_addr); + + // Baseline: both directions decode before the rekey, so a failure below + // is the rekey's and not the harness's. + let pre_fwd = build_ipv6_packet(&fips0, &fips1, b"pre-rekey 0 to 1"); + let pre_rev = build_ipv6_packet(&fips1, &fips0, b"pre-rekey 1 to 0"); + nodes[0].node.handle_tun_outbound(pre_fwd.clone()).await; + nodes[1].node.handle_tun_outbound(pre_rev.clone()).await; + for _ in 0..50 { + tokio::time::sleep(Duration::from_millis(10)).await; + if process_available_packets(&mut nodes).await == 0 { + break; + } + } + let got: Vec> = std::iter::from_fn(|| tun1_rx.try_recv().ok()).collect(); + assert_eq!(got, vec![pre_fwd], "baseline node 0 to node 1 must decode"); + let got: Vec> = std::iter::from_fn(|| tun0_rx.try_recv().ok()).collect(); + assert_eq!(got, vec![pre_rev], "baseline node 1 to node 0 must decode"); + + // Age both sessions past both rekey gates: node 0's jittered time trigger, + // and node 1's 30 s floor below which a msg1 is a duplicate, not a rekey. + let age = Duration::from_secs(REKEY_AFTER_SECS + crate::node::REKEY_JITTER_SECS as u64 + 1); + nodes[0] + .node + .get_peer_mut(&node1_addr) + .unwrap() + .test_backdate_session_established(age); + nodes[1] + .node + .get_peer_mut(&node0_addr) + .unwrap() + .test_backdate_session_established(age); + let node0_idx_before = nodes[0].node.get_peer(&node1_addr).unwrap().our_index(); + let node1_idx_before = nodes[1].node.get_peer(&node0_addr).unwrap().our_index(); + + // node 0 starts the rekey; its msg1 lands in node 1's queue. + nodes[0].node.check_rekey().await; + let rekey_idx = nodes[0] + .node + .get_peer(&node1_addr) + .unwrap() + .rekey_our_index() + .expect("node 0 must have started a rekey"); + + // Deliver the msg1 to node 1 only. node 1 answers as the rekey responder + // and commits its new session at once. + assert_eq!( + process_available_packets(&mut nodes[1..]).await, + 1, + "node 1 must have exactly node 0's rekey msg1 queued" + ); + assert!( + nodes[1] + .node + .get_peer(&node0_addr) + .unwrap() + .pending_new_session() + .is_some(), + "node 1 must answer the msg1 as a rekey and hold its new session" + ); + + // Hold node 1's real msg2 back. + let mut held: Vec = + std::iter::from_fn(|| nodes[0].packet_rx.try_recv().ok()).collect(); + assert_eq!(held.len(), 1, "node 0 must have only node 1's msg2 queued"); + let real_msg2 = held.remove(0); + assert_eq!( + CommonPrefix::parse(&real_msg2.data).map(|p| p.phase), + Some(PHASE_MSG2), + "the held packet must be node 1's msg2" + ); + + // The forgery: a well-formed header naming node 0's live rekey index, a + // valid curve point as the ephemeral so the read gets as far as mixing it + // into the handshake, and an epoch ciphertext that cannot authenticate. + // The source is node 1's address, as a spoofed UDP source would be. + let mut forged_noise = Identity::generate().pubkey_full().serialize().to_vec(); + forged_noise.resize(HANDSHAKE_MSG2_SIZE, 0xA5); + let forged = ReceivedPacket::new( + nodes[0].transport_id, + nodes[1].addr.clone(), + build_msg2(SessionIndex::new(0x5EED_F00D), rekey_idx, &forged_noise), + ); + nodes[0].node.handle_msg2(forged).await; + + // Release the real msg2, then run one rekey tick on each node. + nodes[0].node.handle_msg2(real_msg2).await; + nodes[0].node.check_rekey().await; + nodes[1].node.check_rekey().await; + for _ in 0..50 { + tokio::time::sleep(Duration::from_millis(10)).await; + if process_available_packets(&mut nodes).await == 0 { + break; + } + } + + // node 1 cut over to the session it committed at msg1. Without this the + // delivery checks below could pass because no rekey happened at all. + assert_ne!( + nodes[1].node.get_peer(&node0_addr).unwrap().our_index(), + node1_idx_before, + "node 1 must have cut over to its new session" + ); + + let post_fwd = build_ipv6_packet(&fips0, &fips1, b"post-rekey 0 to 1"); + let post_rev = build_ipv6_packet(&fips1, &fips0, b"post-rekey 1 to 0"); + nodes[0].node.handle_tun_outbound(post_fwd.clone()).await; + nodes[1].node.handle_tun_outbound(post_rev.clone()).await; + for _ in 0..50 { + tokio::time::sleep(Duration::from_millis(10)).await; + if process_available_packets(&mut nodes).await == 0 { + break; + } + } + + let got: Vec> = std::iter::from_fn(|| tun1_rx.try_recv().ok()).collect(); + assert_eq!( + got, + vec![post_fwd], + "node 0 to node 1 must decode after the forged msg2" + ); + + // This is the assertion that tells the two outcomes apart; keep it. node 0 + // to node 1 passes either way inside this test, because node 1 keeps its + // previous session through the drain window and still decrypts node 0's + // old-session frames. node 1 to node 0 fails exactly when node 0 lost the + // cycle to the forgery: node 1 now sends on its new session, addressed to + // node 0's rekey index, and node 0 has no session registered under it. + let handshake = &nodes[0].node.stats().handshake; + let (bad_state, unknown) = (handshake.bad_state, handshake.unknown_connection); + let got: Vec> = std::iter::from_fn(|| tun0_rx.try_recv().ok()).collect(); + assert_eq!( + got, + vec![post_rev], + "node 1 to node 0 must decode after the forged msg2 \ + (node 0 handshake rejects: bad_state={bad_state}, unknown_connection={unknown})" + ); + + let peer = nodes[0].node.get_peer(&node1_addr).unwrap(); + assert_ne!( + peer.our_index(), + node0_idx_before, + "node 0 must have completed the rekey on the real msg2 and cut over" + ); + assert!( + !peer.rekey_in_progress(), + "node 0 must not be left mid-rekey" + ); + + cleanup_nodes(&mut nodes).await; +} + #[tokio::test] async fn test_tun_outbound_triggers_session_initiation() { // Two connected nodes, no session yet. diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index 5116707f..51dcbd49 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -15,9 +15,10 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; /// /// Maintains the chaining key (ck), handshake hash (h), and current cipher. /// -/// `Clone` exists for [`HandshakeState::try_read_xk_message_2`], which has to -/// put the pre-read state back after a message that mixed material in before -/// failing to authenticate. +/// `Clone` exists for [`HandshakeState::try_read_message_2`] and +/// [`HandshakeState::try_read_xk_message_2`], which have to put the pre-read +/// state back after a message that mixed material in before failing to +/// authenticate. /// /// `ck` and `h` are cleared on drop, including on the clone above once it /// goes out of scope. `cipher` is skipped because [`CipherState`] clears its @@ -636,6 +637,41 @@ impl HandshakeState { Ok(()) } + /// Read message 2, leaving the handshake untouched when the message + /// does not authenticate. + /// + /// `read_message_2` mixes the sender's ephemeral into the symmetric state, + /// and both DH results into the key, before it authenticates the encrypted + /// epoch, so a message that fails partway leaves a handshake that can + /// never read the genuine msg2 afterwards. A caller that keeps its rekey + /// cycle across a failed read — because the message may be a forgery + /// rather than the responder's corrupt reply — needs the pre-read state + /// back. + /// + /// The saved set is exactly what `read_message_2` writes: `symmetric`, + /// `remote_ephemeral`, `remote_epoch` and `progress`. `remote_static` is + /// not in it, because IK pins the responder's static before msg1 and the + /// read only uses it. **That mirror is manual.** A later edit that adds a + /// write to `read_message_2` without adding it here silently reintroduces + /// the poisoning, and no caller can detect it. + pub fn try_read_message_2(&mut self, message: &[u8]) -> Result<(), NoiseError> { + let symmetric = self.symmetric.clone(); + let remote_ephemeral = self.remote_ephemeral; + let remote_epoch = self.remote_epoch; + let progress = self.progress; + + match self.read_message_2(message) { + Ok(()) => Ok(()), + Err(e) => { + self.symmetric = symmetric; + self.remote_ephemeral = remote_ephemeral; + self.remote_epoch = remote_epoch; + self.progress = progress; + Err(e) + } + } + } + // ======================================================================== // XK Pattern Methods (Session Layer) // ======================================================================== diff --git a/src/peer/active.rs b/src/peer/active.rs index 3216ea1f..fa819454 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -1246,11 +1246,27 @@ impl ActivePeer { self.rekey_our_index } + /// Whether this peer still holds its rekey initiator handshake, waiting + /// on msg2. + /// + /// [`complete_rekey_msg2`](Self::complete_rekey_msg2) keeps the handshake + /// when a msg2 fails to authenticate, so after a failed call this tells + /// the caller the cycle is still intact. + pub fn awaits_msg2(&self) -> bool { + self.rekey_handshake.is_some() + } + /// Complete the rekey by processing msg2 (initiator side). /// - /// Takes the stored handshake state, reads msg2, and returns the + /// Reads msg2 against the stored handshake state and returns the /// completed NoiseSession. Clears the handshake-related fields but /// leaves rekey_our_index for set_pending_session to use. + /// + /// A msg2 that fails the read changes nothing: the handshake goes back + /// in its pre-read state, and the msg1 resend schedule stays as it was. + /// Nothing authenticates a msg2 before this read, so the message may be a + /// forgery naming our rekey index, and the responder's genuine msg2 has + /// to remain readable when it arrives. pub fn complete_rekey_msg2( &mut self, msg2_bytes: &[u8], @@ -1263,7 +1279,10 @@ impl ActivePeer { got: "no handshake state".to_string(), })?; - hs.read_message_2(msg2_bytes)?; + if let Err(e) = hs.try_read_message_2(msg2_bytes) { + self.rekey_handshake = Some(hs); + return Err(e); + } let remote_epoch = hs.remote_epoch(); let session = hs.into_session()?; From ae9a574bc0bfdd6b5b71a5c68317fdd400265bb2 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 14:32:23 +0000 Subject: [PATCH 03/11] fix(transport/udp): let connected sockets join an adopted traversal socket's port A NAT traversal binds its base socket with a plain port-zero bind, and a successful punch adopts that socket as the peer's transport. Connected-UDP activation then opens a per-peer connected socket on the transport's local address. Linux admits that second bind only when the socket already holding the port set a reuse flag, and the traversal socket set neither, so every activation attempt failed with EADDRINUSE at bind. The rx-loop tick retried it on every pass, and the peer never left the unconnected path. Set SO_REUSEPORT and SO_REUSEADDR inside UdpRawSocket::adopt, after the socket is already bound. The order matters. Flags set before a port-zero bind let the kernel hand out a port another flagged socket already holds, so two concurrent traversals could share a port and the kernel would silently split one peer's datagrams across two transports. Flags set after the bind cannot change which port the socket was given; they only let a later socket join it. The listen socket in UdpRawSocket::open already sets its flags after its bind for the same reason. adopt has one caller chain, which ends at traversal adoption, so the STUN probe and configured listeners are untouched. The flags are best-effort, as in open: if setting them fails, adoption still succeeds and only the connected fast path is refused, rather than a working punched peer being torn down. Two tests cover the change, and both fail without it. The first adopts a socket bound the way the traversal path binds, checks that it arrives with neither flag, and requires a connected socket to open on its port and both flags to read back; without the change the open fails with EADDRINUSE at bind. The second adopts a traversal socket into a node peered with a node on a configured listener, runs activation on both, and requires a connected socket on each side, the adopted side's on the adopted socket's own port; without the change the configured side activates, the adopted side does not, and the failure carries the bind error from a direct open. Each part of the change was also removed in turn. Without SO_REUSEPORT the first test fails on that readback while the connected open still succeeds, because either flag on the holder admits the join; without SO_REUSEADDR it fails on that readback the same way; without both, both tests fail at the bind. Flagging the socket before adoption fails the first test's precondition, and running the second with FIPS_CONNECTED_UDP=0 fails its configured-listener assertion first, so neither can pass vacuously. Not established: Darwin behaviour. adopt is shared by the Unix backends, so macOS builds get the flags too, and no measurement covers SO_REUSEPORT set after bind there. FIPS_MACOS_CONNECTED_UDP=0 or FIPS_CONNECTED_UDP=0 turns the fast path off without a rebuild. --- CHANGELOG.md | 6 ++ src/node/tests/bootstrap.rs | 116 +++++++++++++++++++++++++++++++++++ src/transport/udp/io/mod.rs | 44 +++++++++++++ src/transport/udp/io/unix.rs | 11 ++++ 4 files changed, 177 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e4c662e..2555bb44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 decrypt-worker completion path already acted on that return; the in-line decrypt path discarded it, so the socket stayed installed and the send path kept preferring it over the wildcard listen socket. +- A peer reached by NAT traversal now gets its per-peer connected UDP socket. + The adopted traversal socket carried no address-reuse flags, so the connected + socket's bind to the same port was refused with `EADDRINUSE` on every tick and + the peer never left the unconnected path. The flags are now set when the + socket is adopted, after its bind, so the traversal bind still receives a + port no other socket holds. #### Peering diff --git a/src/node/tests/bootstrap.rs b/src/node/tests/bootstrap.rs index 459181b1..3905f1e4 100644 --- a/src/node/tests/bootstrap.rs +++ b/src/node/tests/bootstrap.rs @@ -377,3 +377,119 @@ async fn test_adopted_udp_inherits_mtu_from_named_primary_config() { transport.stop().await.ok(); } } + +/// A peer reached through an adopted traversal socket gets a per-peer +/// connected UDP socket on that socket's own port, as a peer on a configured +/// listener does. The traversal socket comes from a plain bind with no reuse +/// flags, and the kernel refuses the connected socket's bind to a port whose +/// holder did not opt in to sharing it. +#[cfg(target_os = "linux")] +#[tokio::test] +async fn test_connected_udp_activates_on_an_adopted_traversal_transport_and_on_a_configured_one() { + let mut node_a = make_node(); + let mut node_b = make_node(); + + let transport_id_b = TransportId::new(1); + let udp_config = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + mtu: Some(1280), + ..Default::default() + }; + + let (packet_tx_a, packet_rx_a) = packet_channel(64); + let (packet_tx_b, packet_rx_b) = packet_channel(64); + + node_a.supervisor.packet_tx = Some(packet_tx_a.clone()); + node_a.packet_rx = Some(packet_rx_a); + node_a.supervisor.state = NodeState::Running; + + let mut transport_b = UdpTransport::new(transport_id_b, None, udp_config, packet_tx_b.clone()); + transport_b.start_async().await.unwrap(); + + let addr_b = transport_b.local_addr().unwrap(); + node_b.supervisor.packet_tx = Some(packet_tx_b.clone()); + node_b.packet_rx = Some(packet_rx_b); + node_b.supervisor.state = NodeState::Running; + node_b + .transports + .insert(transport_id_b, TransportHandle::Udp(transport_b)); + + let adopted_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); + let handoff = + EstablishedTraversal::new("sess-connected", node_b.npub(), addr_b, adopted_socket) + .with_transport_name("nostr-punched"); + + let result = node_a.adopt_established_traversal(handoff).await.unwrap(); + + tokio::select! { + result = node_b.run_rx_loop() => { + panic!("node_b rx loop exited unexpectedly: {:?}", result); + } + _ = tokio::time::sleep(Duration::from_millis(500)) => {} + } + + tokio::select! { + result = node_a.run_rx_loop() => { + panic!("node_a rx loop exited unexpectedly: {:?}", result); + } + _ = tokio::time::sleep(Duration::from_millis(500)) => {} + } + + let peer_a_node_addr = + *PeerIdentity::from_pubkey_full(node_a.identity().pubkey_full()).node_addr(); + let peer_b_node_addr = + *PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()).node_addr(); + + // Preconditions: both sides are peered, and node_a reaches node_b over + // the adopted transport rather than some other one. + assert_eq!(node_a.peer_count(), 1, "node_a should promote node_b"); + assert_eq!(node_b.peer_count(), 1, "node_b should promote node_a"); + assert!(node_b.get_peer(&peer_a_node_addr).unwrap().has_session()); + let peer_on_a = node_a.get_peer(&peer_b_node_addr).unwrap(); + assert!(peer_on_a.has_session()); + assert_eq!( + peer_on_a.transport_id(), + Some(result.transport_id), + "node_a's peer must be on the adopted transport", + ); + assert!(peer_on_a.current_addr().is_some()); + + node_b.activate_connected_udp_sessions().await; + node_a.activate_connected_udp_sessions().await; + + assert!( + node_b + .get_peer(&peer_a_node_addr) + .unwrap() + .connected_udp() + .is_some(), + "node_b's peer on its configured listener must get a connected UDP socket; \ + if it does not, check whether FIPS_CONNECTED_UDP turns the fast path off here", + ); + + let Some(connected) = node_a.get_peer(&peer_b_node_addr).unwrap().connected_udp() else { + let direct = + match crate::transport::udp::open_connected_fd(result.local_addr, addr_b, 65536, 65536) + { + Ok(_) => "succeeds".to_string(), + Err(e) => format!("fails with {e}"), + }; + panic!( + "node_a's peer on the adopted transport must get a connected UDP socket; \ + opening one on the adopted socket's address directly {direct}" + ); + }; + assert_eq!( + connected.local_addr(), + result.local_addr, + "the connected socket must join the adopted socket's port, not another transport's", + ); + drop(connected); + + for (_, transport) in node_a.transports.iter_mut() { + transport.stop().await.ok(); + } + for (_, transport) in node_b.transports.iter_mut() { + transport.stop().await.ok(); + } +} diff --git a/src/transport/udp/io/mod.rs b/src/transport/udp/io/mod.rs index 8acbb630..eae1001c 100644 --- a/src/transport/udp/io/mod.rs +++ b/src/transport/udp/io/mod.rs @@ -99,6 +99,50 @@ mod tests { ); } + /// The traversal path binds its socket plainly on port zero, so the + /// socket reaches `adopt` carrying neither reuse flag. Adoption has to + /// add them after the fact, or the per-peer connected socket's bind to + /// the same address is refused with `EADDRINUSE`. Either flag on the + /// holder admits that bind on Linux, so the successful open cannot tell + /// whether both were set; each flag is also read back. + #[cfg(target_os = "linux")] + #[test] + fn an_adopted_plain_socket_carries_both_reuse_flags_and_admits_a_connected_socket_on_its_port() + { + use std::os::fd::{AsRawFd, BorrowedFd}; + + let peer = std::net::UdpSocket::bind("127.0.0.1:0").expect("failed to bind the peer"); + let peer_addr = peer.local_addr().expect("peer local address"); + + let plain = std::net::UdpSocket::bind(("0.0.0.0", 0)).expect("failed to bind the holder"); + { + let probe = socket2::SockRef::from(&plain); + assert!( + !probe.reuse_address().expect("read SO_REUSEADDR"), + "precondition: a plain bind must arrive without SO_REUSEADDR", + ); + assert!( + !probe.reuse_port().expect("read SO_REUSEPORT"), + "precondition: a plain bind must arrive without SO_REUSEPORT", + ); + } + + let adopted = UdpRawSocket::adopt(plain, 65536, 65536).expect("failed to adopt the holder"); + + let joined = super::open_connected_fd(adopted.local_addr(), peer_addr, 65536, 65536); + // SAFETY: `adopted` owns this fd and outlives every use of the borrow. + let fd = unsafe { BorrowedFd::borrow_raw(adopted.as_raw_fd()) }; + let flags = socket2::SockRef::from(&fd); + let reuse_address = flags.reuse_address().expect("read SO_REUSEADDR"); + let reuse_port = flags.reuse_port().expect("read SO_REUSEPORT"); + + if let Err(err) = &joined { + panic!("a connected socket must be able to bind the adopted socket's port: {err}"); + } + assert!(reuse_address, "the adopted socket must carry SO_REUSEADDR"); + assert!(reuse_port, "the adopted socket must carry SO_REUSEPORT"); + } + #[tokio::test] async fn test_async_udp_socket_send_recv() { let sock1 = UdpRawSocket::open("127.0.0.1:0".parse().unwrap(), 65536, 65536) diff --git a/src/transport/udp/io/unix.rs b/src/transport/udp/io/unix.rs index 60a4cb43..32341810 100644 --- a/src/transport/udp/io/unix.rs +++ b/src/transport/udp/io/unix.rs @@ -125,6 +125,8 @@ impl UdpRawSocket { /// Adopt an existing bound UDP socket. /// /// This preserves socket identity/NAT mapping created by bootstrap code. + /// The adopted socket is also made joinable by per-peer connected + /// sockets, which bind its local address. pub fn adopt( socket: std::net::UdpSocket, recv_buf_size: usize, @@ -135,6 +137,15 @@ impl UdpRawSocket { sock.set_nonblocking(true) .map_err(|e| TransportError::StartFailed(format!("set nonblocking failed: {}", e)))?; + // A per-peer connected socket later binds this socket's own address, + // and the kernel admits that joiner only when the holder carries a + // reuse flag too. The socket arrives already bound, so setting the + // flags here cannot change which port it was given, unlike flags set + // ahead of a port-zero bind; see the comment in `open`. Best-effort, + // as there: without them only the connected fast path is refused. + let _ = sock.set_reuse_port(true); + let _ = sock.set_reuse_address(true); + sock.set_recv_buffer_size(recv_buf_size) .map_err(|e| TransportError::StartFailed(format!("set recv buffer: {}", e)))?; sock.set_send_buffer_size(send_buf_size) From 17509445d8101ea2d6ff86433d78a1a9a346a8c8 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 14:32:43 +0000 Subject: [PATCH 04/11] test(transport/udp): measure duplicate ephemeral ports for reuse flags set before and after bind A reuse flag set before a port-zero bind lets the kernel hand out a port that another flagged socket already holds; set after the bind it only lets a later socket join. The adoption fix depends on the second half, and the first half shows up only when hundreds of sockets are bound concurrently, which no ordinary test reaches. Add an ignored measurement that binds 500 sockets from eight threads released together, adopting each one as it is bound and holding all of them, over five trials, in three arms: flags set before the bind, a plain bind followed by adopt, and plain binds made while 200 sockets flagged before their own bind are held. It fails if the before-bind arm shows no duplicates, because such a run could not have seen the hazard, and it fails if either other arm shows any. On the development host, port range 32768-60999, three runs with the adoption fix gave before-bind duplicates of [5, 6, 4, 6, 5], [5, 7, 5, 1, 6] and [6, 2, 3, 4, 5] per 500 binds, and zero in both other arms in every trial. A run without the fix, where adopt sets no flags, gave [3, 2, 8, 6, 3] and zeros. Building the adopt arm with flags set before the bind turns it red, at [3, 5, 6, 4, 7]. It is ignored because the counts depend on load and nothing gating should run it. It measures the kernel behaviour the adoption path relies on, not the traversal binds themselves, so a later change that flagged those binds before binding would not turn it red. --- src/transport/udp/io/mod.rs | 125 ++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/src/transport/udp/io/mod.rs b/src/transport/udp/io/mod.rs index eae1001c..64a25cd3 100644 --- a/src/transport/udp/io/mod.rs +++ b/src/transport/udp/io/mod.rs @@ -168,6 +168,131 @@ mod tests { assert_eq!(src, addr1); } + /// Measurement: duplicate local ports among many concurrently held + /// port-zero UDP binds, with reuse flags set before the bind and with + /// reuse flags set after it by `adopt`. + /// + /// A reuse flag set before a port-zero bind lets the kernel hand out a + /// port another flagged socket already holds; set after the bind it only + /// lets a later socket join. Three arms, each run over several trials: + /// + /// - `pre` flags each socket and then binds it, which is the ordering + /// that must not be used for a traversal socket. It is built here from + /// socket2 because no such helper exists in the tree. It must show + /// duplicates: if it shows none, this run could not have seen the + /// hazard, and the measurement fails rather than passing. + /// - `post` binds exactly as the traversal path does and then adopts, + /// so `adopt` flags each socket while other threads are still binding. + /// - `orphan` holds sockets flagged before their bind, as a connected + /// socket is, and counts later plain binds handed one of their ports. + /// + /// Coverage gap: nothing that gates runs this, because the port + /// allocator is probabilistic. It measures the kernel behaviour the + /// adoption path relies on, not the traversal binds themselves, so a + /// change that flagged those binds before binding would not turn it red. + /// + /// Run with: + /// cargo test --lib transport::udp::io::tests::measure_duplicate_ephemeral_ports -- --ignored --nocapture + #[cfg(target_os = "linux")] + #[test] + #[ignore = "probabilistic kernel port-allocator measurement; run explicitly with --ignored --nocapture"] + fn measure_duplicate_ephemeral_ports_for_reuse_flags_set_before_and_after_bind() { + use socket2::{Domain, Protocol, Socket, Type}; + use std::collections::HashSet; + use std::sync::Barrier; + + const N: usize = 500; + const TRIALS: usize = 5; + const THREADS: usize = 8; + const HELD: usize = 200; + const BUF: usize = 65536; + + fn plain_bind() -> std::net::UdpSocket { + std::net::UdpSocket::bind(("0.0.0.0", 0)).expect("plain port-zero bind") + } + + fn flagged_bind() -> std::net::UdpSocket { + let sock = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP)).expect("socket"); + sock.set_reuse_port(true).expect("set SO_REUSEPORT"); + sock.set_reuse_address(true).expect("set SO_REUSEADDR"); + let any: SocketAddr = "0.0.0.0:0".parse().unwrap(); + sock.bind(&any.into()).expect("flagged port-zero bind"); + sock.into() + } + + /// Bind `n` sockets across `THREADS` threads released together, + /// adopting each one as soon as it is bound, and hold every socket + /// until all the threads have finished. + fn bind_concurrently(n: usize, bind: fn() -> std::net::UdpSocket) -> Vec { + let barrier = Barrier::new(THREADS); + std::thread::scope(|s| { + let handles: Vec<_> = (0..THREADS) + .map(|t| { + let share = n / THREADS + usize::from(t < n % THREADS); + let barrier = &barrier; + s.spawn(move || { + barrier.wait(); + (0..share) + .map(|_| UdpRawSocket::adopt(bind(), BUF, BUF).expect("adopt")) + .collect::>() + }) + }) + .collect(); + handles + .into_iter() + .flat_map(|h| h.join().expect("bind thread panicked")) + .collect() + }) + } + + fn duplicates(socks: &[UdpRawSocket]) -> usize { + let distinct: HashSet = socks.iter().map(|s| s.local_addr().port()).collect(); + socks.len() - distinct.len() + } + + let mut pre = Vec::with_capacity(TRIALS); + let mut post = Vec::with_capacity(TRIALS); + let mut orphan = Vec::with_capacity(TRIALS); + for _ in 0..TRIALS { + pre.push(duplicates(&bind_concurrently(N, flagged_bind))); + post.push(duplicates(&bind_concurrently(N, plain_bind))); + + let held: Vec = (0..HELD).map(|_| flagged_bind()).collect(); + let held_ports: HashSet = held + .iter() + .map(|s| s.local_addr().expect("held local address").port()) + .collect(); + let later = bind_concurrently(N, plain_bind); + orphan.push( + later + .iter() + .filter(|s| held_ports.contains(&s.local_addr().port())) + .count(), + ); + } + + eprintln!("duplicate ports per {N} binds, {THREADS} threads, {TRIALS} trials"); + eprintln!(" pre (flags before bind): {pre:?}"); + eprintln!(" post (flags after bind): {post:?}"); + eprintln!(" orphan ({HELD} held pre-flagged, later plain binds): {orphan:?}"); + + assert!( + pre.iter().sum::() > 0, + "the before-bind arm showed no duplicates, so this run could not have seen the \ + hazard at N = {N}; raise N rather than reading the other arms as clean", + ); + assert_eq!( + post.iter().sum::(), + 0, + "flags set after bind: {post:?}" + ); + assert_eq!( + orphan.iter().sum::(), + 0, + "orphan collisions: {orphan:?}" + ); + } + /// Microbench: compare per-packet `recv_from` (single recvmsg syscall + /// task wakeup per datagram — the macOS pre-recvmsg_x baseline) vs /// `recv_batch` (the new recvmsg_x path, up to 32 datagrams per syscall). From 513208b564c6d469dd4a01e61ecf0e699bad49c0 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 15:42:30 +0000 Subject: [PATCH 05/11] refactor(peer): drop send-eligibility checks that could never fail can_send read a connectivity state that nothing outside the tests ever changed after a peer was promoted, so every route, probe and shutdown filter on it kept every peer. The filters go: find_next_hop's direct-peer and tree fallback steps, the probe next-hop preview and its direct-peer flag, the disconnect broadcast at shutdown, and the send-eligibility read on the routing seam, which the candidate selection no longer consults. Node::sendable_peers and Node::sendable_peer_count keep their signatures and still return every peer. The routing seam test that marked a peer reconnecting to keep it out of the candidate set is removed: once no peer in the map can be marked down, removal from the map is what keeps a dead link out of routing, as it already was in the daemon. No behaviour change. --- src/node/handlers/probe.rs | 8 ++--- src/node/lifecycle/mod.rs | 2 +- src/node/mod.rs | 23 ++++++-------- src/node/tests/establish_chartests.rs | 4 +-- src/node/tests/handshake.rs | 2 -- src/node/tests/routing.rs | 45 +-------------------------- src/node/tests/unit.rs | 12 +++---- src/peer/active.rs | 22 ------------- src/proto/routing/core.rs | 18 +++++------ src/proto/routing/tests/core.rs | 27 +++++++--------- src/proto/routing/tests/util.rs | 6 +--- 11 files changed, 42 insertions(+), 127 deletions(-) diff --git a/src/node/handlers/probe.rs b/src/node/handlers/probe.rs index 73071238..b37297a6 100644 --- a/src/node/handlers/probe.rs +++ b/src/node/handlers/probe.rs @@ -350,7 +350,7 @@ impl Node { session_established, session_is_ours: is_ours, session_error: job.session_error.take(), - target_is_direct_peer: self.peers.get(&target).is_some_and(|p| p.can_send()), + target_is_direct_peer: self.peers.contains_key(&target), counters, last_rtt_ms: mmp.and_then(|m| m.metrics.last_rtt_ms()), srtt_ms: mmp.and_then(|m| m.metrics.srtt_ms()), @@ -485,9 +485,7 @@ impl Node { if dest == self.node_addr() { return (None, Some(NoHopReason::Local)); } - if let Some(peer) = self.peers.get(dest) - && peer.can_send() - { + if self.peers.contains_key(dest) { return ( Some(NextHopFacts { node_addr: *dest, @@ -519,7 +517,7 @@ impl Node { let Some(hop) = selected else { return (None, Some(NoHopReason::NoCloserPeer)); }; - if !self.peers.get(&hop).is_some_and(|p| p.can_send()) { + if !self.peers.contains_key(&hop) { return (None, Some(NoHopReason::HopNotSendReady)); } let class = self.classify_forward(dest, &hop); diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 99240cd0..d9811dcb 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -2489,7 +2489,7 @@ impl Node { let peer_addrs: Vec = self .peers .iter() - .filter(|(_, peer)| peer.can_send() && peer.has_session()) + .filter(|(_, peer)| peer.has_session()) .map(|(addr, _)| *addr) .collect(); diff --git a/src/node/mod.rs b/src/node/mod.rs index fc259736..cc015bf5 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -3046,14 +3046,15 @@ impl Node { self.peers.keys() } - /// Iterate over peers that can send traffic. + /// Iterate over peers that can send traffic: every active peer, the same + /// peers as [`Self::peers`]. pub fn sendable_peers(&self) -> impl Iterator { - self.peers.values().filter(|p| p.can_send()) + self.peers.values() } - /// Number of peers that can send traffic. + /// Number of peers that can send traffic, the same as [`Self::peer_count`]. pub fn sendable_peer_count(&self) -> usize { - self.peers.values().filter(|p| p.can_send()).count() + self.peers.len() } // === End-to-End Sessions === @@ -3407,9 +3408,7 @@ impl Node { } // 2. Direct peer - if let Some(peer) = self.peers.get(dest_node_addr) - && peer.can_send() - { + if let Some(peer) = self.peers.get(dest_node_addr) { return Some(peer); } @@ -3426,7 +3425,7 @@ impl Node { // 3. Bloom filter candidates — requires dest_coords for loop-free selection. // If no candidate is strictly closer, fall through to tree routing. // The sans-IO core enumerates borrowed peers over the `RoutingView` - // seam, applies the bloom/send/progress filters, and tracks the + // seam, applies the bloom/progress filters, and tracks the // winner inline; the shell supplies only raw per-peer reads. let next_hop = { let view = NodeRoutingView { @@ -3452,7 +3451,7 @@ impl Node { .tree_state .find_next_hop(&dest_coords, &BTreeSet::new())?; - self.peers.get(&next_hop_id).filter(|p| p.can_send()) + self.peers.get(&next_hop_id) } /// Classify a transit forward by route class from tree coordinates. @@ -3935,7 +3934,7 @@ impl Node { /// Shell-side [`routing::RoutingView`] seam over live `Node` state — the sole /// routing read adapter the shell retains. It hands the sans-IO routing core -/// borrowed peers plus raw `may_reach` / `can_send` / `link_cost` / `coords` +/// borrowed peers plus raw `may_reach` / `link_cost` / `coords` /// reads so selection and error synthesis live in `proto::routing::core`; no /// routing decision logic remains here. /// @@ -3984,10 +3983,6 @@ impl routing::RoutingView for NodeRoutingView<'_> { peer.1.may_reach(dest) } - fn peer_can_send<'a>(&'a self, peer: Self::Peer<'a>) -> bool { - peer.1.can_send() - } - fn peer_link_cost<'a>(&'a self, peer: Self::Peer<'a>) -> f64 { peer.1.link_cost() } diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index eec03aa1..9fb0325b 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -656,11 +656,9 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { ); } - // Both remain single, sendable peers after resolution. + // Both remain single peers after resolution. assert_eq!(node_a.peer_count(), 1); assert_eq!(node_b.peer_count(), 1); - assert!(node_a.get_peer(&node_b_addr).unwrap().can_send()); - assert!(node_b.get_peer(&node_a_addr).unwrap().can_send()); for (_, t) in node_a.transports.iter_mut() { t.stop().await.ok(); diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index b02f0d1a..a98cae5e 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -663,8 +663,6 @@ async fn test_cross_connection_both_initiate() { assert!(peer_b_on_a.has_session(), "Peer B on A should have session"); assert!(peer_a_on_b.has_session(), "Peer A on B should have session"); - assert!(peer_b_on_a.can_send(), "Peer B on A should be sendable"); - assert!(peer_a_on_b.can_send(), "Peer A on B should be sendable"); // Clean up transports for (_, t) in node_a.transports.iter_mut() { diff --git a/src/node/tests/routing.rs b/src/node/tests/routing.rs index a9d51708..fb8fd58b 100644 --- a/src/node/tests/routing.rs +++ b/src/node/tests/routing.rs @@ -1698,42 +1698,6 @@ fn test_seam_bloom_hit_overrides_tree_tiebreak() { ); } -/// `NodeRoutingView::peer_can_send` keeps a down link out of the candidate set. -/// -/// Both peers hold a bloom hit, so the address tie-break would hand the route -/// to the low-address peer; that peer is the one marked reconnecting. Widening -/// `peer_can_send` to `true` lets it back in and hands a down link to the -/// forwarder. -#[test] -fn test_seam_unsendable_bloom_candidate_is_skipped() { - let mut node = make_node(); - let (near, far, dest) = seam_two_equidistant_peers(&mut node); - let low = near.min(far); - let high = near.max(far); - - seam_set_filter(&mut node, &low, &dest); - seam_set_filter(&mut node, &high, &dest); - node.get_peer_mut(&low).unwrap().mark_reconnecting(); - - assert_eq!(node.peers.len(), 2, "fixture: two peers"); - assert!( - !node.get_peer(&low).unwrap().can_send(), - "fixture: low is down" - ); - assert!( - node.get_peer(&high).unwrap().can_send(), - "fixture: high is up" - ); - - let hop = node.find_next_hop(&dest).expect("route exists"); - assert!(hop.can_send(), "a down link must never be returned"); - assert_eq!( - hop.node_addr(), - &high, - "the sendable peer must win despite losing the address tie-break" - ); -} - /// `NodeRoutingView::peer_link_cost` must carry the ETX factor. /// /// SRTT is equal on both peers, so ETX is the only thing that can order them, @@ -1847,12 +1811,11 @@ fn test_seam_peer_without_tree_coords_is_never_selected() { let (near, far, dest) = seam_two_equidistant_peers(&mut node); let tree_pick = near.min(far); - // A third peer: in the peer map, sendable, holding a bloom hit for dest, + // A third peer: in the peer map, holding a bloom hit for dest, // and absent from tree state. let ghost = seam_add_peer(&mut node, 3, TransportId::new(1)); seam_set_filter(&mut node, &ghost, &dest); - assert!(node.get_peer(&ghost).unwrap().can_send()); assert!(node.get_peer(&ghost).unwrap().may_reach(&dest)); assert!( node.tree_state().peer_coords(&ghost).is_none(), @@ -1897,7 +1860,6 @@ fn test_seam_routing_view_reads_match_live_peer_state() { // Make the two peers differ on every predicate, in opposite directions, so // no constant in either direction can satisfy the assertions below. seam_set_filter(&mut node, &near, &dest); - node.get_peer_mut(&far).unwrap().mark_reconnecting(); // Both cost factors off the multiplicative identity: with etx pinned at 1.0 // a cost that reads only the latency half is indistinguishable from the // real one, and this assertion would be blind to it. @@ -1949,10 +1911,6 @@ fn test_seam_routing_view_reads_match_live_peer_state() { "the filter is per-destination" ); - // peer_can_send: far was marked reconnecting. - assert!(view.peer_can_send(near_h)); - assert!(!view.peer_can_send(far_h)); - // peer_link_cost: etx 2.0 * (1.0 + 50ms/100) for near; far has no RTT // sample, so it takes the optimistic 1.0 default. Neither factor is at the // identity, so a cost reading only one half of the product is caught. @@ -1993,7 +1951,6 @@ fn test_seam_routing_view_reads_match_live_peer_state() { let addr = view.peer_addr(*peer); let live = node.peers.get(&addr).unwrap(); assert_eq!(view.peer_may_reach(*peer, &dest), live.may_reach(&dest)); - assert_eq!(view.peer_can_send(*peer), live.can_send()); assert_eq!(view.peer_link_cost(*peer), live.link_cost()); assert_eq!( view.peer_coords(*peer), diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 930a0f4f..30523c8f 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -691,30 +691,30 @@ fn test_node_sendable_peers() { let mut node = make_node(); let transport_id = TransportId::new(1); - // Add a healthy peer + // Add a peer let link_id1 = LinkId::new(1); let identity1 = seed_completed_connection(&mut node, link_id1, transport_id, 1000); let node_addr1 = *identity1.node_addr(); node.promote_connection(link_id1, identity1, 2000).unwrap(); - // Add another peer and mark it stale (still sendable) + // Add another peer let link_id2 = LinkId::new(2); let identity2 = seed_completed_connection(&mut node, link_id2, transport_id, 1000); node.promote_connection(link_id2, identity2, 2000).unwrap(); - // Add a third peer and mark it disconnected (not sendable) + // Add a third peer let link_id3 = LinkId::new(3); let identity3 = seed_completed_connection(&mut node, link_id3, transport_id, 1000); let node_addr3 = *identity3.node_addr(); node.promote_connection(link_id3, identity3, 2000).unwrap(); - node.get_peer_mut(&node_addr3).unwrap().mark_disconnected(); assert_eq!(node.peer_count(), 3); - assert_eq!(node.sendable_peer_count(), 2); + assert_eq!(node.sendable_peer_count(), 3); let sendable: Vec<_> = node.sendable_peers().collect(); - assert_eq!(sendable.len(), 2); + assert_eq!(sendable.len(), 3); assert!(sendable.iter().any(|p| p.node_addr() == &node_addr1)); + assert!(sendable.iter().any(|p| p.node_addr() == &node_addr3)); } // === RX Loop Tests === diff --git a/src/peer/active.rs b/src/peer/active.rs index 6562cced..f79c22c0 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -38,14 +38,6 @@ pub enum ConnectivityState { } impl ConnectivityState { - /// Check if the peer is usable for sending traffic. - pub fn can_send(&self) -> bool { - matches!( - self, - ConnectivityState::Connected | ConnectivityState::Stale - ) - } - /// Check if this is a terminal state requiring cleanup. pub fn is_terminal(&self) -> bool { matches!(self, ConnectivityState::Disconnected) @@ -506,11 +498,6 @@ impl ActivePeer { self.connectivity } - /// Check if peer can receive traffic. - pub fn can_send(&self) -> bool { - self.connectivity.can_send() - } - /// Check if peer is fully healthy. pub fn is_healthy(&self) -> bool { self.connectivity.is_healthy() @@ -1325,11 +1312,6 @@ mod tests { #[test] fn test_connectivity_state_properties() { - assert!(ConnectivityState::Connected.can_send()); - assert!(ConnectivityState::Stale.can_send()); - assert!(!ConnectivityState::Reconnecting.can_send()); - assert!(!ConnectivityState::Disconnected.can_send()); - assert!(ConnectivityState::Connected.is_healthy()); assert!(!ConnectivityState::Stale.is_healthy()); @@ -1345,7 +1327,6 @@ mod tests { assert_eq!(peer.identity().node_addr(), identity.node_addr()); assert_eq!(peer.link_id(), LinkId::new(1)); assert!(peer.is_healthy()); - assert!(peer.can_send()); assert_eq!(peer.authenticated_at(), 1000); assert!(peer.needs_filter_update()); // New peers need filter } @@ -1413,21 +1394,18 @@ mod tests { peer.mark_stale(); assert_eq!(peer.connectivity(), ConnectivityState::Stale); - assert!(peer.can_send()); // Stale can still send // Traffic received brings back to connected peer.touch(2000); assert!(peer.is_healthy()); peer.mark_reconnecting(); - assert!(!peer.can_send()); peer.mark_connected(3000); assert!(peer.is_healthy()); peer.mark_disconnected(); assert!(peer.is_disconnected()); - assert!(!peer.can_send()); } #[test] diff --git a/src/proto/routing/core.rs b/src/proto/routing/core.rs index f956715b..e73caa72 100644 --- a/src/proto/routing/core.rs +++ b/src/proto/routing/core.rs @@ -48,8 +48,6 @@ pub(crate) trait RoutingView { /// Does `peer`'s bloom filter indicate it may reach `dest`? The raw /// per-peer predicate the core filters candidates on. fn peer_may_reach<'a>(&'a self, peer: Self::Peer<'a>, dest: &NodeAddr) -> bool; - /// Can `peer`'s session currently carry a forward? - fn peer_can_send<'a>(&'a self, peer: Self::Peer<'a>) -> bool; /// `peer`'s outgoing link cost (lower is preferred). fn peer_link_cost<'a>(&'a self, peer: Self::Peer<'a>) -> f64; /// `peer`'s tree coordinates, if known. @@ -329,15 +327,15 @@ impl RouteClass { /// Select the best next hop from the active peers that may reach `dest`. /// -/// Enumerates borrowed peers through [`RoutingView`], applies the bloom and -/// send-eligibility filters, and tracks the best hop inline without allocating -/// candidate vectors or cloning coordinates. Only peers strictly closer to the -/// destination than we are (`my_coords`) are eligible — the self-distance check -/// that prevents routing loops. +/// Enumerates borrowed peers through [`RoutingView`], applies the bloom filter, +/// and tracks the best hop inline without allocating candidate vectors or +/// cloning coordinates. Only peers strictly closer to the destination than we +/// are (`my_coords`) are eligible — the self-distance check that prevents +/// routing loops. /// /// Ordering: `(link_cost, distance_to_dest, node_addr)`. Returns the winning -/// peer's address, or `None` when no candidate is send-ready and strictly -/// closer to the destination than us. +/// peer's address, or `None` when no candidate is strictly closer to the +/// destination than us. pub(crate) fn select_best_candidate( rv: &impl RoutingView, dest: &NodeAddr, @@ -349,7 +347,7 @@ pub(crate) fn select_best_candidate( let mut best: Option<(NodeAddr, f64, usize)> = None; rv.for_each_peer(|peer| { - if !rv.peer_may_reach(peer, dest) || !rv.peer_can_send(peer) { + if !rv.peer_may_reach(peer, dest) { return; } diff --git a/src/proto/routing/tests/core.rs b/src/proto/routing/tests/core.rs index f1483f81..75d25bab 100644 --- a/src/proto/routing/tests/core.rs +++ b/src/proto/routing/tests/core.rs @@ -28,14 +28,12 @@ fn mock_peer( addr: u8, dest: NodeAddr, may_reach: bool, - can_send: bool, link_cost: f64, coords: Option<&[u8]>, ) -> MockPeer { MockPeer { addr: make_node_addr(addr), reach: may_reach.then_some(dest).into_iter().collect(), - can_send, link_cost, coords: coords.map(make_coords), } @@ -284,8 +282,8 @@ fn candidate_selection_is_independent_of_peer_enumeration_order() { let root = 0x00; let my_coords = make_coords(&[0x10, root]); let dest_coords = make_coords(&[0x50, root]); - let lower_addr = mock_peer(0x20, dest, true, true, 1.0, Some(&[root])); - let higher_addr = mock_peer(0x30, dest, true, true, 1.0, Some(&[root])); + let lower_addr = mock_peer(0x20, dest, true, 1.0, Some(&[root])); + let higher_addr = mock_peer(0x30, dest, true, 1.0, Some(&[root])); let forward = MockRoutingView { peers: vec![lower_addr.clone(), higher_addr.clone()], @@ -307,7 +305,7 @@ fn candidate_selection_is_independent_of_peer_enumeration_order() { } #[test] -fn candidate_selection_filters_bloom_unsendable_and_missing_coords() { +fn candidate_selection_filters_bloom_and_missing_coords() { let dest = make_node_addr(0x50); let root = 0x00; let my_coords = make_coords(&[0x10, root]); @@ -315,10 +313,9 @@ fn candidate_selection_filters_bloom_unsendable_and_missing_coords() { let eligible = make_node_addr(0x60); let rv = MockRoutingView { peers: vec![ - mock_peer(0x01, dest, true, false, 0.0, Some(&[0x50, root])), - mock_peer(0x02, dest, true, true, 0.0, None), - mock_peer(0x03, dest, false, true, 0.0, Some(&[0x50, root])), - mock_peer(0x60, dest, true, true, 10.0, Some(&[root])), + mock_peer(0x02, dest, true, 0.0, None), + mock_peer(0x03, dest, false, 0.0, Some(&[0x50, root])), + mock_peer(0x60, dest, true, 10.0, Some(&[root])), ], ..MockRoutingView::new(false) }; @@ -338,9 +335,9 @@ fn candidate_must_be_strictly_closer_than_self() { let rv = MockRoutingView { peers: vec![ // A sibling is exactly as far from dest as this node. - mock_peer(0x20, dest, true, true, 1.0, Some(&[0x20, root])), + mock_peer(0x20, dest, true, 1.0, Some(&[0x20, root])), // This descendant of a sibling is farther from dest. - mock_peer(0x21, dest, true, true, 0.5, Some(&[0x21, 0x20, root])), + mock_peer(0x21, dest, true, 0.5, Some(&[0x21, 0x20, root])), ], ..MockRoutingView::new(false) }; @@ -357,12 +354,12 @@ fn candidate_ordering_is_cost_then_distance_then_address() { let rv = MockRoutingView { peers: vec![ // Lowest address loses because distance precedes address. - mock_peer(0x01, dest, true, true, 1.0, Some(&[root])), + mock_peer(0x01, dest, true, 1.0, Some(&[root])), // Closest peer loses because cost is the primary key. - mock_peer(0x02, dest, true, true, 1.0, Some(&[0x50, root])), - mock_peer(0x04, dest, true, true, 0.5, Some(&[root])), + mock_peer(0x02, dest, true, 1.0, Some(&[0x50, root])), + mock_peer(0x04, dest, true, 0.5, Some(&[root])), // Same cost and distance: lower address wins. - mock_peer(0x03, dest, true, true, 0.5, Some(&[root])), + mock_peer(0x03, dest, true, 0.5, Some(&[root])), ], ..MockRoutingView::new(false) }; diff --git a/src/proto/routing/tests/util.rs b/src/proto/routing/tests/util.rs index 25a844fe..c743fb6d 100644 --- a/src/proto/routing/tests/util.rs +++ b/src/proto/routing/tests/util.rs @@ -6,12 +6,11 @@ use crate::testutil::make_node_addr; use crate::{NodeAddr, TreeCoordinate}; /// A mock peer for the candidate-assembly seam: the set of destinations its -/// bloom filter reaches, its send state, link cost, and tree coordinates. +/// bloom filter reaches, its link cost, and tree coordinates. #[derive(Clone)] pub(super) struct MockPeer { pub(super) addr: NodeAddr, pub(super) reach: Vec, - pub(super) can_send: bool, pub(super) link_cost: f64, pub(super) coords: Option, } @@ -60,9 +59,6 @@ impl RoutingView for MockRoutingView { fn peer_may_reach<'a>(&'a self, peer: Self::Peer<'a>, dest: &NodeAddr) -> bool { peer.reach.contains(dest) } - fn peer_can_send<'a>(&'a self, peer: Self::Peer<'a>) -> bool { - peer.can_send - } fn peer_link_cost<'a>(&'a self, peer: Self::Peer<'a>) -> f64 { peer.link_cost } From ad3fcc308119cdb33134505a168f216ed858416b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 15:45:58 +0000 Subject: [PATCH 06/11] refactor(fmp): stop reading peer health in inbound establish and the rekey scan The health conjunct of the inbound rekey gate and the rekey scan filter were always true, for the same reason as the send-eligibility checks: nothing in the daemon changed the stored connectivity after promotion. The gate now states the three conditions it actually tests: rekey enabled, keys established, and old enough to rekey. The comments that described the gate and the scan as reading peer health now match the code. No behaviour change, and nothing on the wire changes. --- src/node/handlers/handshake.rs | 1 - src/node/handlers/rekey.rs | 6 +++--- src/node/tests/establish_chartests.rs | 1 - src/node/tests/handshake.rs | 14 -------------- src/peer/active.rs | 18 ------------------ src/peer/machine.rs | 6 +----- src/proto/fmp/core.rs | 9 +++------ src/proto/fmp/tests/core.rs | 9 +-------- src/proto/fmp/tests/util.rs | 3 +-- 9 files changed, 9 insertions(+), 58 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 2f5c5803..213db09f 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -79,7 +79,6 @@ impl EstablishView for Node { .map(|p| p.session_established_at().elapsed().as_secs()) .unwrap_or(0), has_session: existing.map(|p| p.has_session()).unwrap_or(false), - is_healthy: existing.map(|p| p.is_healthy()).unwrap_or(false), pending_new_session: existing .map(|p| p.pending_new_session().is_some()) .unwrap_or(false), diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index efd2f512..a9fa9150 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -88,7 +88,7 @@ impl Node { after_messages: self.config().node.rekey.after_messages, }; - // The shell snapshots each healthy peer's rekey ages/flags (every clock + // The shell snapshots each peer's rekey ages/flags (every clock // read resolved here); the core decides cutover/drain/trigger with no // clock, phase-grouped to preserve the pre-refactor execution order. // The batch `poll_rekey` + snapshots STAY SHELL-SIDE and BYTE-UNCHANGED: @@ -277,7 +277,7 @@ impl Node { } } - /// Snapshot every healthy peer with a session for the rekey decision, + /// Snapshot every peer with a session for the rekey decision, /// pre-computing its monotonic ages and timer predicates so the pure core /// applies the thresholds without reading a clock (see [`PeerSnapshot`]). /// @@ -286,7 +286,7 @@ impl Node { pub(in crate::node) fn rekey_peer_snapshots(&self) -> Vec { self.peers .iter() - .filter(|(_, peer)| peer.has_session() && peer.is_healthy()) + .filter(|(_, peer)| peer.has_session()) .map(|(node_addr, peer)| PeerSnapshot { addr: *node_addr, has_pending: peer.pending_new_session().is_some(), diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index 9fb0325b..3e4590e8 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -775,7 +775,6 @@ async fn chartest_msg1_rekey_responder_stores_pending_session() { let old_index = { let p = node.get_peer(&sender_addr).expect("peer established"); assert!(p.has_session()); - assert!(p.is_healthy()); assert_eq!(p.remote_epoch(), Some(epoch)); assert!(!p.rekey_in_progress()); assert!(p.pending_new_session().is_none()); diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index a98cae5e..b735a9e6 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -192,13 +192,6 @@ async fn test_two_node_handshake_udp() { node_b.handle_encrypted_frame(encrypted_packet_b).await; - // Verify B's peer was touched (last_seen updated) - let peer_a = node_b.get_peer(&peer_a_node_addr).unwrap(); - assert!( - peer_a.is_healthy(), - "Peer A on B should still be healthy after receiving encrypted frame" - ); - // === Phase 5: Encrypted frame B → A === // Prepend inner header (timestamp + msg_type) as the real send path does @@ -226,13 +219,6 @@ async fn test_two_node_handshake_udp() { node_a.handle_encrypted_frame(encrypted_packet_a).await; - // Verify A's peer was touched - let peer_b = node_a.get_peer(&peer_b_node_addr).unwrap(); - assert!( - peer_b.is_healthy(), - "Peer B on A should still be healthy after receiving encrypted frame" - ); - // Clean up transports for (_, t) in node_a.transports.iter_mut() { t.stop().await.ok(); diff --git a/src/peer/active.rs b/src/peer/active.rs index f79c22c0..7d3a730a 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -42,11 +42,6 @@ impl ConnectivityState { pub fn is_terminal(&self) -> bool { matches!(self, ConnectivityState::Disconnected) } - - /// Check if peer is fully healthy. - pub fn is_healthy(&self) -> bool { - matches!(self, ConnectivityState::Connected) - } } impl fmt::Display for ConnectivityState { @@ -498,11 +493,6 @@ impl ActivePeer { self.connectivity } - /// Check if peer is fully healthy. - pub fn is_healthy(&self) -> bool { - self.connectivity.is_healthy() - } - /// Check if peer is disconnected. pub fn is_disconnected(&self) -> bool { self.connectivity.is_terminal() @@ -1312,9 +1302,6 @@ mod tests { #[test] fn test_connectivity_state_properties() { - assert!(ConnectivityState::Connected.is_healthy()); - assert!(!ConnectivityState::Stale.is_healthy()); - assert!(ConnectivityState::Disconnected.is_terminal()); assert!(!ConnectivityState::Connected.is_terminal()); } @@ -1326,7 +1313,6 @@ mod tests { assert_eq!(peer.identity().node_addr(), identity.node_addr()); assert_eq!(peer.link_id(), LinkId::new(1)); - assert!(peer.is_healthy()); assert_eq!(peer.authenticated_at(), 1000); assert!(peer.needs_filter_update()); // New peers need filter } @@ -1390,19 +1376,15 @@ mod tests { let identity = make_peer_identity(); let mut peer = ActivePeer::new(identity, LinkId::new(1), 1000); - assert!(peer.is_healthy()); - peer.mark_stale(); assert_eq!(peer.connectivity(), ConnectivityState::Stale); // Traffic received brings back to connected peer.touch(2000); - assert!(peer.is_healthy()); peer.mark_reconnecting(); peer.mark_connected(3000); - assert!(peer.is_healthy()); peer.mark_disconnected(); assert!(peer.is_disconnected()); diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 0cc45a5b..b812bc98 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -1407,7 +1407,7 @@ impl PeerMachine { /// the subsequent cadence `Cutover`/`Drain` consume transitions from a /// coherent phase. Emits NO action (nothing left to do). No-op unless the peer /// is in an established-like state (defensive; the shell only initiates on - /// healthy established peers). + /// established peers). fn on_rekey_initiated(&mut self) -> Vec { let addr = match self.addr() { Some(a) => a, @@ -1898,7 +1898,6 @@ mod tests { existing_peer_epoch: None, existing_session_age_secs: 0, has_session: false, - is_healthy: false, pending_new_session: false, rekey_in_progress: false, existing_msg2: None, @@ -2144,7 +2143,6 @@ mod tests { est.has_existing_peer = true; est.existing_peer_epoch = Some([1u8; 8]); est.has_session = true; - est.is_healthy = true; est.existing_session_age_secs = 120; est.rekey_in_progress = true; let wire = wire_outcome(peer, Some([1u8; 8]), 0x77); @@ -2183,7 +2181,6 @@ mod tests { est.has_existing_peer = true; est.existing_peer_epoch = Some([1u8; 8]); est.has_session = true; - est.is_healthy = true; est.existing_session_age_secs = 120; est.rekey_in_progress = true; let wire = wire_outcome(peer, Some([1u8; 8]), 0x77); @@ -2222,7 +2219,6 @@ mod tests { est.has_existing_peer = true; est.existing_peer_epoch = Some([1u8; 8]); est.has_session = true; - est.is_healthy = true; est.existing_session_age_secs = 5; // young session -> duplicate, not rekey est.existing_msg2 = Some(vec![0xC4; 16]); let wire = wire_outcome(peer, Some([1u8; 8]), 0x77); diff --git a/src/proto/fmp/core.rs b/src/proto/fmp/core.rs index 11bab16b..58598c0d 100644 --- a/src/proto/fmp/core.rs +++ b/src/proto/fmp/core.rs @@ -225,8 +225,6 @@ pub(crate) struct EstablishSnapshot { pub existing_session_age_secs: u64, /// The existing peer has an established Noise session. pub has_session: bool, - /// The existing peer's session is healthy. - pub is_healthy: bool, /// The existing peer already holds a pending post-rekey session awaiting /// K-bit cutover. pub pending_new_session: bool, @@ -330,7 +328,7 @@ 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 active peer with a session that is healthy, pre-computing + /// Snapshot every active peer with a session, pre-computing /// its rekey-relevant ages and timer predicates (see [`PeerSnapshot`]). The /// shell resolves every clock read here; the core applies the thresholds. fn rekey_peers(&self) -> Vec; @@ -364,7 +362,7 @@ pub(crate) enum InboundDecision { /// authorize → … → promote sequence as [`Promote`](InboundDecision::Promote). /// `peer` is the teardown / reconnect target. RestartThenPromote { peer: NodeAddr }, - /// Same-epoch rekey msg1 on an aged, healthy session: respond as the rekey + /// Same-epoch rekey msg1 on an aged session: respond as the rekey /// responder. The shell extracts the fresh Noise session from the live /// connection, allocates a new index, sends the rekey msg2, and stores the /// session as the peer's pending (post-rekey) session. `abandon_first` is set @@ -500,7 +498,7 @@ impl Fmp { .collect() } - /// Decide the per-tick rekey choreography for the healthy peers the shell + /// Decide the per-tick rekey choreography for the peers the shell /// snapshotted. Reproduces the pre-refactor priority and phase grouping /// exactly: /// @@ -619,7 +617,6 @@ impl Fmp { // Same epoch (or no epoch captured on either side). let is_rekey = snap.rekey_enabled && snap.has_session - && snap.is_healthy && snap.existing_session_age_secs >= REKEY_MIN_SESSION_AGE_SECS; if !is_rekey { // Duplicate msg1 — resend the stored msg2. diff --git a/src/proto/fmp/tests/core.rs b/src/proto/fmp/tests/core.rs index 03e8a7b9..90ad973a 100644 --- a/src/proto/fmp/tests/core.rs +++ b/src/proto/fmp/tests/core.rs @@ -407,7 +407,6 @@ fn establish_inbound_same_epoch_young_session_resends() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 5; snap.existing_msg2 = Some(vec![0x01, 0x02, 0x03]); let wire = wire_outcome(Some([7u8; 8])); @@ -426,7 +425,6 @@ fn establish_inbound_aged_session_rekey_responds() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 31; let wire = wire_outcome(Some([7u8; 8])); let peer = *wire.peer_identity.node_addr(); @@ -438,14 +436,13 @@ fn establish_inbound_aged_session_rekey_responds() { #[test] fn establish_inbound_rekey_gate_requires_enabled() { - // Aged healthy session but rekey disabled → same-epoch msg1 is a duplicate, + // Aged session but rekey disabled → same-epoch msg1 is a duplicate, // not a rekey. let fmp = Fmp::new(); let mut snap = establish_snapshot(); snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 31; snap.rekey_enabled = false; let wire = wire_outcome(Some([7u8; 8])); @@ -463,7 +460,6 @@ fn establish_inbound_rekey_gate_boundary_at_30s() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; let wire = wire_outcome(Some([7u8; 8])); snap.existing_session_age_secs = 30; @@ -486,7 +482,6 @@ fn establish_inbound_pending_session_rejects() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 31; snap.pending_new_session = true; let wire = wire_outcome(Some([7u8; 8])); @@ -507,7 +502,6 @@ fn establish_inbound_dual_init_we_win_rejects() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 31; snap.rekey_in_progress = true; snap.our_node_addr = make_node_addr(0x00); // minimal → strictly smaller @@ -529,7 +523,6 @@ fn establish_inbound_dual_init_we_lose_responds_with_abandon() { snap.has_existing_peer = true; snap.existing_peer_epoch = Some([7u8; 8]); snap.has_session = true; - snap.is_healthy = true; snap.existing_session_age_secs = 31; snap.rekey_in_progress = true; snap.our_node_addr = max_node_addr(); // strictly larger than any peer addr diff --git a/src/proto/fmp/tests/util.rs b/src/proto/fmp/tests/util.rs index f8c583ad..3684c76c 100644 --- a/src/proto/fmp/tests/util.rs +++ b/src/proto/fmp/tests/util.rs @@ -24,7 +24,7 @@ pub(super) fn rekey_resend_snapshot( } } -/// Build a quiescent `PeerSnapshot` for `addr`: session-healthy but with no +/// Build a quiescent `PeerSnapshot` for `addr`, with no /// pending cutover, no drain, no dampening, zero ages/counter/jitter. Tests set /// only the fields the case exercises. pub(super) fn peer_snapshot(addr_byte: u8) -> PeerSnapshot { @@ -80,7 +80,6 @@ pub(super) fn establish_snapshot() -> EstablishSnapshot { existing_peer_epoch: None, existing_session_age_secs: 0, has_session: false, - is_healthy: false, pending_new_session: false, rekey_in_progress: false, existing_msg2: None, From e6b0725af1d549542ca0d6ee6259e94d93bc82ad Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 15:49:13 +0000 Subject: [PATCH 07/11] refactor(peer): stop storing peer connectivity Removes the stored field, its four setters, the promotion in touch that could not fire, and connectivity(). ConnectivityState is reduced to the two values the control socket reports, which the node now derives from idle time. ConnectivityState::is_terminal and ActivePeer::is_disconnected stay and return false, as they always did in the daemon. Adds the CHANGELOG Removed entry for the library-surface change. --- CHANGELOG.md | 16 +++++++++ src/peer/active.rs | 84 ++++++++-------------------------------------- 2 files changed, 30 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48e34cd0..3c3fb34d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -285,6 +285,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 binder tearing down and rebinding every second while teardown silently declined to abort anything. +### Removed + +- **Source-breaking for consumers of the library crate**: `ActivePeer` no + longer stores a connectivity state. `ActivePeer::connectivity`, `can_send`, + `is_healthy`, `mark_stale`, `mark_reconnecting`, `mark_disconnected` and + `mark_connected` are gone. `ConnectivityState` stays public with only + `Connected` and `Stale`, the values `show_peers` reports, and loses + `can_send` and `is_healthy`. `ConnectivityState::is_terminal`, + `ActivePeer::is_disconnected`, `Node::sendable_peers` and + `Node::sendable_peer_count` keep their signatures and the results they + always had in the daemon: the first two return `false`, and the last two + cover every peer. Nothing in the daemon changed the stored state after a + peer was promoted, so every removed check was already true and the shipped + binaries behave as before. The peer wire and the control-socket response + shape are unchanged. + ### Fixed #### Node lifecycle diff --git a/src/peer/active.rs b/src/peer/active.rs index 7d3a730a..4369481b 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -22,25 +22,24 @@ fn draw_rekey_jitter() -> i64 { rand::rng().random_range(-REKEY_JITTER_SECS..=REKEY_JITTER_SECS) } -/// Connectivity state for an active peer. +/// Connectivity of an active peer, as the control socket reports it. /// -/// This is simpler than the full PeerState since authentication is complete. +/// Not stored on the peer: the node derives it from how long the peer has +/// been silent, compared with the configured heartbeat interval. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ConnectivityState { - /// Peer is fully connected and responsive. + /// Heard from within the heartbeat interval. Connected, - /// Peer hasn't been heard from recently (potential timeout). + /// Silent for longer than the heartbeat interval. Stale, - /// Connection lost, attempting to reconnect. - Reconnecting, - /// Peer has been explicitly disconnected. - Disconnected, } impl ConnectivityState { /// Check if this is a terminal state requiring cleanup. + /// + /// Always false: neither derived state is terminal. pub fn is_terminal(&self) -> bool { - matches!(self, ConnectivityState::Disconnected) + false } } @@ -49,8 +48,6 @@ impl fmt::Display for ConnectivityState { let s = match self { ConnectivityState::Connected => "connected", ConnectivityState::Stale => "stale", - ConnectivityState::Reconnecting => "reconnecting", - ConnectivityState::Disconnected => "disconnected", }; write!(f, "{}", s) } @@ -199,10 +196,6 @@ pub struct ActivePeer { /// Immutable for the same reason as [`ActivePeer::npub`]. short_npub: String, - // === Connection === - /// Current connectivity state. - connectivity: ConnectivityState, - // === Spanning Tree === /// Their latest parent declaration. declaration: Option, @@ -293,7 +286,6 @@ impl ActivePeer { npub: identity.npub(), short_npub: identity.short_npub(), identity, - connectivity: ConnectivityState::Connected, declaration: None, ancestry: None, tree_announce_min_interval_ms: 500, @@ -373,7 +365,6 @@ impl ActivePeer { npub: identity.npub(), short_npub: identity.short_npub(), identity, - connectivity: ConnectivityState::Connected, declaration: None, ancestry: None, tree_announce_min_interval_ms: 500, @@ -488,14 +479,12 @@ impl ActivePeer { self.send.link_id } - /// Get the connectivity state. - pub fn connectivity(&self) -> ConnectivityState { - self.connectivity - } - /// Check if peer is disconnected. + /// + /// Always false: the peer stores no connectivity state, and a peer that + /// goes away is removed from the node rather than marked. pub fn is_disconnected(&self) -> bool { - self.connectivity.is_terminal() + false } // === Session Accessors === @@ -817,33 +806,6 @@ impl ActivePeer { /// Update last seen timestamp. pub fn touch(&mut self, current_time_ms: u64) { self.send.last_seen = current_time_ms; - // If we were stale, receiving traffic makes us connected again - if self.connectivity == ConnectivityState::Stale { - self.connectivity = ConnectivityState::Connected; - } - } - - /// Mark peer as stale (no recent traffic). - pub fn mark_stale(&mut self) { - if self.connectivity == ConnectivityState::Connected { - self.connectivity = ConnectivityState::Stale; - } - } - - /// Mark peer as reconnecting. - pub fn mark_reconnecting(&mut self) { - self.connectivity = ConnectivityState::Reconnecting; - } - - /// Mark peer as disconnected. - pub fn mark_disconnected(&mut self) { - self.connectivity = ConnectivityState::Disconnected; - } - - /// Mark peer as connected (e.g., after successful reconnect). - pub fn mark_connected(&mut self, current_time_ms: u64) { - self.connectivity = ConnectivityState::Connected; - self.send.last_seen = current_time_ms; } /// Update the link ID (e.g., on reconnect). @@ -1302,8 +1264,8 @@ mod tests { #[test] fn test_connectivity_state_properties() { - assert!(ConnectivityState::Disconnected.is_terminal()); assert!(!ConnectivityState::Connected.is_terminal()); + assert!(!ConnectivityState::Stale.is_terminal()); } #[test] @@ -1313,6 +1275,7 @@ mod tests { assert_eq!(peer.identity().node_addr(), identity.node_addr()); assert_eq!(peer.link_id(), LinkId::new(1)); + assert!(!peer.is_disconnected()); assert_eq!(peer.authenticated_at(), 1000); assert!(peer.needs_filter_update()); // New peers need filter } @@ -1371,25 +1334,6 @@ mod tests { assert_eq!(short_first, short_second); } - #[test] - fn test_connectivity_transitions() { - let identity = make_peer_identity(); - let mut peer = ActivePeer::new(identity, LinkId::new(1), 1000); - - peer.mark_stale(); - assert_eq!(peer.connectivity(), ConnectivityState::Stale); - - // Traffic received brings back to connected - peer.touch(2000); - - peer.mark_reconnecting(); - - peer.mark_connected(3000); - - peer.mark_disconnected(); - assert!(peer.is_disconnected()); - } - #[test] fn test_tree_position() { let identity = make_peer_identity(); From cca6ed2c62129299c42f1df55e5e456f7714a743 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 13:45:20 +0000 Subject: [PATCH 08/11] test(netmon): pin that peers trading local sources is a medium change The fingerprint maps each peer to the local address it is reached from, rather than holding the set of addresses the host leaves from. The map is what makes a swap visible: two peers exchanging sources leave the host on the same two addresses, while each peer's connected socket is now pinned to the address the other one uses. Nothing in the suite failed if the comparison collapsed to a set of addresses. This test scripts that swap and requires both peers to be reported, each with its own before and after. --- src/node/netmon/tests.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/node/netmon/tests.rs b/src/node/netmon/tests.rs index 30134a3a..e2ae35dd 100644 --- a/src/node/netmon/tests.rs +++ b/src/node/netmon/tests.rs @@ -251,6 +251,43 @@ async fn a_peer_joining_onto_a_settled_path_is_still_not_a_change() { .await; } +#[tokio::test(start_paused = true)] +async fn local_sources_trading_places_between_peers_is_reported_for_both() { + // The reason the fingerprint is a map from peer to source and not a set of + // sources. Two peers swap the local addresses they are reached from, so + // the host still leaves from exactly the same two addresses, yet each + // peer's connected socket is pinned to the source the other peer now + // uses. A set of addresses is unchanged across the swap and would report + // nothing while both sockets are stale. + let a = Some(v4(192, 168, 1, 10)); + let b = Some(v4(10, 40, 0, 7)); + let before = NetFingerprint::for_test(&[(peer(1), a), (peer(2), b)]); + let swapped = NetFingerprint::for_test(&[(peer(1), b), (peer(2), a)]); + let (sampler, _) = scripted(vec![before, swapped]); + let (tx, mut rx) = mpsc::channel(1); + + tokio::spawn(run_detector(tx, cfg(1, 0), sampler, timer_wake(1))); + + let change = expect_change(&mut rx).await; + assert_eq!(change.summary.moved.len(), 2, "{:?}", change.summary.moved); + assert_eq!( + change.summary.moved, + vec![ + PeerSourceMove { + peer: peer(1), + before: a, + after: b, + }, + PeerSourceMove { + peer: peer(2), + before: b, + after: a, + }, + ], + "each peer must be named with its own source before and after the swap" + ); +} + #[tokio::test(start_paused = true)] async fn churn_during_a_handover_does_not_mask_the_handover() { // Both at once: a peer leaves while the medium moves under the peer that From 3ec75e8385ed179e66c78e9f29f94e931dcf6a7c Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 13:52:29 +0000 Subject: [PATCH 09/11] docs(netmon): state the probe's measured syscall cost The per-peer probe's cost was given as five syscalls read off the code rather than measured, and the backstop timer's as a few syscalls per period. Counted with strace on Linux, a probe that gets an answer makes five (socket, bind, connect, getsockname, close) and one with no route makes four. A build with debug assertions adds an fcntl before each close, from the standard library's open-descriptor check, which is why a test binary counts six. At the 128-peer default a sample is 640 syscalls, and a reported change under the default debounce costs two to nine samples, 1280 to 5760 syscalls. State those figures where the cost is described, and describe the backstop as costing one sample per period. --- docs/reference/configuration.md | 13 +++++++---- src/node/netmon/mod.rs | 39 ++++++++++++++++++++------------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index df31aa2f..8e1db983 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -275,10 +275,15 @@ one would put a DNS lookup on the sample path; the address becomes numeric as soon as an authenticated packet arrives from the peer). A node holding no peers detects nothing, which is correct — it has nothing bound to the old path. -The cost is five non-blocking syscalls per peer per sample, read from the -probe's own code rather than measured: `socket(2)` and `bind(2)`, a `connect(2)` -that sends no packet, a `getsockname(2)`, and the `close(2)` the socket takes on -drop. Nothing goes on the wire and no name is resolved. +The cost is five non-blocking syscalls per peer per sample, counted with +`strace` on Linux: `socket(2)` and `bind(2)`, a `connect(2)` that sends no +packet, a `getsockname(2)`, and the `close(2)` the socket takes on drop. A peer +with no route costs four, because the lookup fails at `connect(2)`. Nothing goes +on the wire and no name is resolved. At the default `max_peers` of 128 that is +640 syscalls per sample. A detected change is resampled until it settles, so +with the default `debounce_ms` it costs between two samples and nine, which is +between 1280 and 5760 syscalls at 128 peers; the backstop timer also takes one +sample every `poll_interval_secs` whether or not anything moved. `node.limits.max_peers` bounds the per-sample total only where it is set: at `max_peers: 0`, which means unlimited, there is no bound and the cost tracks the live peer count instead. diff --git a/src/node/netmon/mod.rs b/src/node/netmon/mod.rs index 0e45451c..8e3956f8 100644 --- a/src/node/netmon/mod.rs +++ b/src/node/netmon/mod.rs @@ -51,9 +51,9 @@ //! One local source address per peer: for every peer whose transport address is //! a numeric IP endpoint, the address the kernel would pick to reach *that //! peer*. A connected-but-never-sending UDP socket makes the kernel run its -//! route lookup and bind the source address it would use; five syscalls, read -//! off [`NetFingerprint::sample`] rather than measured, no packets, no name -//! resolution, and it works identically on every platform std supports. +//! route lookup and bind the source address it would use; five syscalls per +//! peer (see [`NetFingerprint::sample`] for the measured cost), no packets, no +//! name resolution, and it works identically on every platform std supports. //! //! Keying on peers bounds the *reaction* — only the peers a change names are //! acted on — and does not bound the *sampling*. One roaming peer still makes @@ -222,18 +222,26 @@ struct PeerPath { impl NetFingerprint { /// Probe every target and record the local address the kernel picks. /// - /// Five non-blocking syscalls per target: `socket(2)` and `bind(2)` behind - /// `UdpSocket::bind`, a `connect(2)` that sends no packet, a - /// `getsockname(2)`, and the `close(2)` the socket takes on drop. No I/O - /// wait, no name resolution, and no allocation beyond the map. + /// Five non-blocking syscalls per target, counted with `strace -f` on + /// Linux: `socket(2)` and `bind(2)` behind `UdpSocket::bind`, a + /// `connect(2)` that sends no packet, a `getsockname(2)`, and the + /// `close(2)` the socket takes on drop. A target with no route costs four, + /// because `connect(2)` fails and `getsockname(2)` is never reached. A + /// build with debug assertions on adds a sixth to each, the `fcntl(2)` std + /// uses to check a descriptor is still open before closing it, so count + /// against a release build. No I/O wait, no name resolution, and no + /// allocation beyond the map. /// - /// The count matters because a debounced handover resamples: up to - /// `MAX_DEBOUNCE_ROUNDS` rounds plus the settled sample, times the peers - /// held. `node.limits.max_peers` bounds that only where it is set — - /// the value 0 means unlimited, and there the cost tracks the live peer - /// count instead. It runs inline in the detector's own task rather than - /// through `spawn_blocking`, which is what keeps it off every other task - /// regardless. + /// The count matters because a debounced handover resamples: the sample + /// that saw the move, then up to `MAX_DEBOUNCE_ROUNDS` more until two + /// consecutive samples agree. At 128 peers, the `node.limits.max_peers` + /// default, that is 640 syscalls per sample, and a reported change under a + /// non-zero debounce costs from 1280 (settled on the first resample) to + /// 5760 (still moving after every round). `node.limits.max_peers` bounds + /// that only where it is set — the value 0 means unlimited, and there the + /// cost tracks the live peer count instead. It runs inline in the + /// detector's own task rather than through `spawn_blocking`, which is what + /// keeps it off every other task regardless. pub(in crate::node) fn sample(targets: &[ProbeTarget]) -> Self { Self { sources: targets @@ -541,7 +549,8 @@ struct WakeSource { /// either of which would otherwise leave the node noticing nothing at all. /// Keeping the period the poller would have used makes an event-driven /// backend a strict latency improvement rather than a replacement that can - /// regress, for the cost of a few syscalls per period. + /// regress, for the cost of one sample per period: five syscalls per probed + /// peer, or 640 at the default of 128 peers. timer: tokio::time::Interval, } From 3e77aa41ef705be629d61605d40849c6e3f88b16 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 15:48:41 +0000 Subject: [PATCH 10/11] fix(transport): tie each stream connection's tasks to its own pool entry A stream connection's writer and receive loop removed whatever pool entry held their address. Either can outlive its connection, so one that did could tear down a newer connection to the same address: a writer whose write failed after the address had been reconnected, or the receive loop of a connection whose entry had already been replaced. That includes a second inbound TCP connection from the same remote address, whether it arrived on another local address of a wildcard listener or reconnected from the same source port before the old connection's teardown had run. A receive loop that ended on EOF also dropped its entry without stopping the writer, which kept writing to a peer that had gone. Each TCP, Tor and Nym connection now gets an id where it is built, and that one id goes to its writer, its receive loop and its pool entry. Both loops remove only an entry carrying their id, through one shared rule, and a receive loop that removes its own entry now aborts that entry's writer. The onion accept loop likewise stops the writer, as well as the receive task, of an entry it evicts. An insert at an address that already has an entry still replaces it without stopping the replaced connection's tasks, and the inbound counter is not corrected for that case; this change does not touch the inserts. The new tests were run on the unfixed tree first. Red there: the writer error path, TCP and proxied, removed a successor entry marked by its MTU; both receive-loop teardowns removed a successor, and the TCP one decremented for it; a connection displaced through promote_connection removed its successor, on TCP, Tor and Nym; closing the older of two accepted connections sharing a remote address (two client sockets on one 127.0.0.1 port dialing 127.0.0.1 and 127.0.0.2 against a wildcard listener) removed the newer entry; a peer reading after the receive loop tore down received every queued byte (TCP 4201400 of 4201400, proxied 306600 of 306600), with the writer shown parked first; and an entry evicted by the onion accept loop kept its writer running. The connect and accept-loop teardown tests for TCP, Tor and Nym were green there, as expected: they guard the id wiring the fix adds. Four existing tests that build pool entries or call a receive loop directly now pass an id. Break-checks, each reverting one element and running the tests named for it: removing by address again on either writer error path reds that path's writer test; on either receive-loop teardown it reds the successor tests for TCP, Tor and Nym and the two-address inbound test; dropping the writer abort from either teardown, or from the onion eviction, reds the matching stops-its-writer test; a teardown whose id never matches reds the three existing tests that run a receive loop against a hand-built entry and the connect, accept and promote teardown tests; and giving any one of the eight constructors' receive loops a different id from its entry reds that constructor's teardown test. --- CHANGELOG.md | 6 +- src/transport/mod.rs | 1 + src/transport/nym/mod.rs | 159 +++++++++++ src/transport/socks5/pool.rs | 331 +++++++++++++++++++++- src/transport/stream.rs | 51 ++++ src/transport/tcp/mod.rs | 518 ++++++++++++++++++++++++++++++++++- src/transport/tcp/pool.rs | 12 + src/transport/tor/mod.rs | 207 ++++++++++++++ 8 files changed, 1277 insertions(+), 8 deletions(-) create mode 100644 src/transport/stream.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d562842..706e136f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -395,7 +395,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 resynchronise from. BLE was the worst of the four: it awaited the L2CAP write while holding the connection-pool mutex, so one unresponsive peer froze every other BLE operation as well — connects, evictions, and each - receive loop's teardown. + receive loop's teardown. On TCP, Tor and Nym, a connection's writer and + receive loop act only on their own connection: one that outlives its + connection can no longer tear down a newer connection that has taken the + same address, and a receive loop that ends stops its writer rather than + leaving it writing to a peer that has gone. - A per-peer `connect()`-ed UDP socket is no longer left pinned to an interface the host has moved off. Established UDP peers get their own socket for the diff --git a/src/transport/mod.rs b/src/transport/mod.rs index fc650d9e..81a25fee 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -63,6 +63,7 @@ use tor::control::TorMonitoringInfo; use udp::UdpTransport; pub(crate) mod framing; +pub(crate) mod stream; mod stats_common; pub(crate) use stats_common::PoolCounters; diff --git a/src/transport/nym/mod.rs b/src/transport/nym/mod.rs index 124d6ab6..821a1b03 100644 --- a/src/transport/nym/mod.rs +++ b/src/transport/nym/mod.rs @@ -24,6 +24,7 @@ use crate::transport::socks5::{ Socks5Auth, Socks5Dialer, SocksTarget, poll_connecting, proxied_receive_loop, proxied_send_loop, }; +use crate::transport::stream::{ConnId, next_conn_id}; use stats::NymStats; use std::collections::HashMap; @@ -358,12 +359,14 @@ impl NymTransport { let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); let mtu = self.config.mtu(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { nym_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -378,6 +381,7 @@ impl NymTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), "Nym", @@ -391,6 +395,7 @@ impl NymTransport { mtu, established_at: Instant::now(), meta: (), + id, }; let mut pool = self.pool.lock().await; @@ -521,12 +526,14 @@ impl NymTransport { let pool = self.pool.clone(); let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { nym_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -541,6 +548,7 @@ impl NymTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), "Nym", @@ -554,6 +562,7 @@ impl NymTransport { mtu, established_at: Instant::now(), meta: (), + id, }; if let Ok(mut pool) = self.pool.try_lock() { @@ -680,10 +689,12 @@ fn parse_target_addr(addr: &TransportAddr) -> Result, mtu: u16, @@ -693,6 +704,7 @@ async fn nym_receive_loop( reader, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -1029,4 +1041,151 @@ mod tests { nym.stop_async().await.unwrap(); dest.stop_async().await.unwrap(); } + + // ======================================================================== + // Connection identity and failure teardown + // ======================================================================== + + /// Poll `f` every 10ms until it holds or `limit` elapses. + async fn wait_until bool>(mut f: F, limit: Duration) -> bool { + let deadline = Instant::now() + limit; + loop { + if f() { + return true; + } + if Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + /// A destination TCP transport behind a mock SOCKS5 proxy, and a started + /// Nym transport dialing through it. + async fn nym_via_mock_proxy() -> ( + TcpTransport, + crate::transport::PacketRx, + NymTransport, + TransportAddr, + ) { + let (dest_tx, dest_rx) = packet_channel(32); + let dest_config = TcpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }; + let mut dest = TcpTransport::new(TransportId::new(100), None, dest_config, dest_tx); + dest.start_async().await.unwrap(); + let dest_addr = dest.local_addr().unwrap(); + + let mock = MockSocks5Server::new(dest_addr).await.unwrap(); + let proxy_addr = mock.addr(); + let _proxy_handle = mock.spawn(); + + let (nym_tx, _nym_rx) = packet_channel(32); + let nym_config = NymConfig { + socks5_addr: Some(proxy_addr.to_string()), + startup_timeout_secs: Some(5), + connect_timeout_ms: Some(5000), + ..Default::default() + }; + let mut nym = NymTransport::new(TransportId::new(200), None, nym_config, nym_tx); + nym.start_async().await.unwrap(); + let target = TransportAddr::from_string(&dest_addr.to_string()); + (dest, dest_rx, nym, target) + } + + /// A connection displaced from the pool by a newer one at the same address + /// must not remove the newer one when its own receive loop ends. + /// + /// Both are built by `promote_connection`, and the MTU marks which entry + /// is pooled. The last step checks the newer connection still removes its + /// own entry. + #[tokio::test] + async fn nym_displaced_connection_cannot_remove_its_successor() { + let (tx, _rx) = packet_channel(32); + let nym = NymTransport::new(TransportId::new(1), None, make_config(), tx); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let remote = TransportAddr::from_string(&listen.to_string()); + + let a = TcpStream::connect(listen).await.unwrap(); + let (sa, _) = listener.accept().await.unwrap(); + let b = TcpStream::connect(listen).await.unwrap(); + let (sb, _) = listener.accept().await.unwrap(); + + nym.promote_connection(&remote, a, 1400); + nym.promote_connection(&remote, b, 1300); + { + let pool = nym.pool.lock().await; + assert_eq!(pool.len(), 1); + assert_eq!(pool.get(&remote).map(|c| c.mtu), Some(1300)); + } + + drop(sa); + assert!( + wait_until( + || nym.stats().snapshot().recv_errors == 1, + Duration::from_secs(2) + ) + .await, + "the displaced connection's receive loop should have read EOF" + ); + tokio::time::sleep(Duration::from_millis(50)).await; + assert_eq!( + nym.pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1300), + "the displaced connection's teardown removed its successor" + ); + + drop(sb); + assert!( + wait_until( + || nym.stats().snapshot().recv_errors == 2, + Duration::from_secs(2) + ) + .await, + "the newer connection's receive loop should have read EOF" + ); + assert!( + wait_until( + || nym.pool.try_lock().map(|p| p.is_empty()).unwrap_or(false), + Duration::from_secs(2) + ) + .await, + "the newer connection's teardown should remove its own entry" + ); + } + + /// A connection built by connect-on-send removes its own entry when the + /// far side closes. + #[tokio::test] + async fn nym_connect_teardown_removes_its_own_entry() { + let (mut dest, mut dest_rx, mut nym, target) = nym_via_mock_proxy().await; + + let frame = build_msg1_frame(); + nym.send_async(&target, &frame).await.unwrap(); + let received = tokio::time::timeout(Duration::from_secs(5), dest_rx.recv()) + .await + .expect("timeout waiting for packet") + .expect("channel closed"); + assert_eq!(received.data, frame); + assert_eq!(nym.pool.lock().await.len(), 1); + + dest.stop_async().await.unwrap(); + assert!( + wait_until( + || nym.pool.try_lock().map(|p| p.is_empty()).unwrap_or(false), + Duration::from_secs(5) + ) + .await, + "the receive loop should remove its own entry" + ); + assert_eq!( + nym.stats().snapshot().recv_errors, + 1, + "the empty pool must be the receive loop's teardown" + ); + + nym.stop_async().await.unwrap(); + } } diff --git a/src/transport/socks5/pool.rs b/src/transport/socks5/pool.rs index 1d3dab42..969f8268 100644 --- a/src/transport/socks5/pool.rs +++ b/src/transport/socks5/pool.rs @@ -21,6 +21,7 @@ use tracing::{debug, trace}; use tokio::io::AsyncWriteExt; use crate::transport::framing::read_fmp_packet; +use crate::transport::stream::{ConnId, PooledConn, remove_own}; use crate::transport::{ ConnectionState, PacketTx, ReceivedPacket, TransportAddr, TransportError, TransportId, }; @@ -46,6 +47,17 @@ pub(crate) struct ProxiedConnection { pub established_at: Instant, /// Per-transport metadata (tor: `Direction`; nym: `()`). pub meta: M, + /// Identity of this connection, shared with its writer and receive loop. + /// Either loop removes the entry at its address only when the entry + /// carries this id, so a loop that outlives its connection cannot remove + /// a newer connection at the same address. + pub id: ConnId, +} + +impl PooledConn for ProxiedConnection { + fn conn_id(&self) -> ConnId { + self.id + } } /// Shared connection pool: addr -> per-connection state. @@ -152,13 +164,17 @@ pub(crate) const SEND_QUEUE_DEPTH: usize = 64; /// Teardown mirrors [`proxied_receive_loop`]: the pool entry is removed and /// `on_remove` fires only when the removal returned `Some`, taking the /// metadata from the removed entry, so a concurrent `close`/`stop` of the same -/// address cannot double-count. +/// address cannot double-count. The entry is removed only when it carries +/// this connection's `id`. A writer can outlive its entry, and by the time its +/// write fails a newer connection may hold the address; that one is left +/// alone. #[allow(clippy::too_many_arguments)] pub(crate) async fn proxied_send_loop( mut writer: OwnedWriteHalf, mut frames: mpsc::Receiver>, transport_id: TransportId, remote_addr: TransportAddr, + id: ConnId, pool: ProxiedPool, stats: Arc, label: &'static str, @@ -187,7 +203,7 @@ pub(crate) async fn proxied_send_loop( ); let removed = { let mut guard = pool.lock().await; - guard.remove(&remote_addr) + remove_own(&mut guard, &remote_addr, id) }; if let Some(conn) = removed { conn.recv_task.abort(); @@ -233,11 +249,20 @@ pub(crate) async fn proxied_send_loop( /// must not run its cleanup before the accept loop has inserted the pool entry /// and bumped its counter, or the removal finds nothing, `on_remove` never /// fires, and the increment is stranded for the life of the process. +/// +/// `id` is the connection's identity. The cleanup removes the entry at +/// `remote_addr` only when it carries this id, so a loop whose entry has +/// already been replaced by a newer connection at the same address leaves +/// that connection alone. When it does remove its own entry it also stops the +/// entry's writer: the loop ended on EOF, a read error or a missed deadline, +/// and frames still queued for a connection in that state are not worth +/// writing. #[allow(clippy::too_many_arguments)] pub(crate) async fn proxied_receive_loop( mut reader: OwnedReadHalf, transport_id: TransportId, remote_addr: TransportAddr, + id: ConnId, packet_tx: PacketTx, pool: ProxiedPool, mtu: u16, @@ -334,8 +359,308 @@ pub(crate) async fn proxied_receive_loop( // concurrent close/stop teardown of the same address can never // double-count. let mut pool_guard = pool.lock().await; - if let Some(removed) = pool_guard.remove(&remote_addr) { + if let Some(removed) = remove_own(&mut pool_guard, &remote_addr, id) { drop(pool_guard); + removed.send_task.abort(); on_remove(&*stats, &removed.meta); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::transport::packet_channel; + use crate::transport::stream::next_conn_id; + use portable_atomic::{AtomicU64, Ordering}; + use tokio::io::AsyncReadExt; + use tokio::net::TcpListener; + use tokio::time::timeout; + + /// Counters the shared loops write, plus how often `on_remove` fired. + #[derive(Default)] + struct CountingStats { + send_errors: AtomicU64, + recv_errors: AtomicU64, + removed: AtomicU64, + } + + impl ProxiedStats for CountingStats { + fn record_recv(&self, _bytes: usize) {} + fn record_recv_error(&self) { + self.recv_errors.fetch_add(1, Ordering::Relaxed); + } + fn record_send(&self, _bytes: usize) {} + fn record_send_error(&self) { + self.send_errors.fetch_add(1, Ordering::Relaxed); + } + } + + /// The `on_remove` hook the tests pass to both loops. + fn count_removal(stats: &CountingStats, _meta: &()) { + stats.removed.fetch_add(1, Ordering::Relaxed); + } + + /// A pool entry that stands for some other connection at the same + /// address, marked by its MTU. + fn successor() -> ProxiedConnection<()> { + ProxiedConnection { + send_tx: mpsc::channel(1).0, + send_task: tokio::spawn(async {}), + recv_task: tokio::spawn(async {}), + mtu: 1234, + established_at: Instant::now(), + meta: (), + id: next_conn_id(), + } + } + + /// Poll `f` every 10ms until it holds or `limit` elapses. + async fn wait_until bool>(mut f: F, limit: Duration) -> bool { + let deadline = Instant::now() + limit; + loop { + if f() { + return true; + } + if Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + /// A writer whose write fails must not remove a newer connection that has + /// taken its address in the pool, nor run `on_remove` for it. + #[tokio::test] + async fn proxied_writer_error_leaves_a_newer_connection_at_the_same_address() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let client = TcpStream::connect(listen).await.unwrap(); + let (server, _) = listener.accept().await.unwrap(); + socket2::SockRef::from(&server) + .set_linger(Some(Duration::ZERO)) + .unwrap(); + drop(server); + let (_read_half, write_half) = client.into_split(); + let remote = TransportAddr::from_string(&listen.to_string()); + + let pool: ProxiedPool<()> = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(CountingStats::default()); + pool.lock().await.insert(remote.clone(), successor()); + + let (send_tx, send_rx) = mpsc::channel(SEND_QUEUE_DEPTH); + let writer = tokio::spawn(proxied_send_loop( + write_half, + send_rx, + TransportId::new(1), + remote.clone(), + next_conn_id(), + pool.clone(), + stats.clone(), + "Test", + count_removal, + )); + + let frame = vec![0xAB; 114]; + let deadline = Instant::now() + Duration::from_secs(5); + while !writer.is_finished() && Instant::now() < deadline { + let _ = send_tx.try_send(frame.clone()); + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert!(writer.is_finished(), "the writer never hit a write error"); + assert_eq!( + stats.send_errors.load(Ordering::Relaxed), + 1, + "the writer's error path must have run" + ); + + assert_eq!( + pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1234), + "a failed writer removed the newer connection at its address" + ); + assert_eq!(stats.removed.load(Ordering::Relaxed), 0); + } + + /// A receive loop that ends on EOF must stop its writer rather than leave + /// it writing to a peer that has gone. + /// + /// The writer is parked on a peer that does not read, with a full queue. + /// The peer then half-closes, which ends the receive loop, and only + /// afterwards reads. A writer left running delivers every frame it had + /// queued; a stopped one delivers fewer. + #[tokio::test] + async fn proxied_receive_teardown_stops_the_writer() { + let socket = tokio::net::TcpSocket::new_v4().unwrap(); + socket.set_recv_buffer_size(64 * 1024).unwrap(); + socket.bind("127.0.0.1:0".parse().unwrap()).unwrap(); + let listener = socket.listen(8).unwrap(); + let listen = listener.local_addr().unwrap(); + + let client = TcpStream::connect(listen).await.unwrap(); + socket2::SockRef::from(&client) + .set_send_buffer_size(64 * 1024) + .unwrap(); + let (mut peer, _) = listener.accept().await.unwrap(); + let remote = TransportAddr::from_string(&listen.to_string()); + let (read_half, write_half) = client.into_split(); + + let (packet_tx, _packet_rx) = packet_channel(10); + let pool: ProxiedPool<()> = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(CountingStats::default()); + let (send_tx, send_rx) = mpsc::channel(SEND_QUEUE_DEPTH); + let id = next_conn_id(); + let send_task = tokio::spawn(proxied_send_loop( + write_half, + send_rx, + TransportId::new(1), + remote.clone(), + id, + pool.clone(), + stats.clone(), + "Test", + count_removal, + )); + let recv_task = tokio::spawn({ + let pool = pool.clone(); + let stats = stats.clone(); + let remote = remote.clone(); + async move { + proxied_receive_loop( + read_half, + TransportId::new(1), + remote, + id, + packet_tx, + pool, + 1400, + stats, + "Test", + None, + None, + count_removal, + ) + .await; + } + }); + pool.lock().await.insert( + remote.clone(), + ProxiedConnection { + send_tx, + send_task, + recv_task, + mtu: 1400, + established_at: Instant::now(), + meta: (), + id, + }, + ); + + // Fill without stopping at the first refusal, yielding so the writer + // runs, and never keep a sender past the fill. + let frame = vec![0xAB; 1400]; + let mut queued = 0usize; + let mut refused = 0usize; + for _ in 0..8000 { + let sent = { + let guard = pool.lock().await; + guard + .get(&remote) + .map(|c| c.send_tx.try_send(frame.clone()).is_ok()) + }; + if sent == Some(true) { + queued += 1; + } else { + refused += 1; + } + tokio::task::yield_now().await; + } + tokio::time::sleep(Duration::from_millis(300)).await; + let capacity = pool.lock().await.get(&remote).map(|c| c.send_tx.capacity()); + assert_eq!( + capacity, + Some(0), + "setup did not park the writer: queued={queued} refused={refused}" + ); + assert!(refused > 0, "setup never filled the queue: queued={queued}"); + + peer.shutdown().await.unwrap(); + assert!( + wait_until( + || stats.removed.load(Ordering::Relaxed) == 1, + Duration::from_secs(5) + ) + .await, + "the receive loop should have torn the connection down on EOF" + ); + + let mut buf = vec![0u8; 64 * 1024]; + let read = timeout(Duration::from_secs(10), async { + let mut total = 0usize; + loop { + match peer.read(&mut buf).await { + Ok(0) | Err(_) => return total, + Ok(n) => total += n, + } + } + }) + .await + .expect("the connection was never closed toward the peer"); + assert!(read > 0, "the kernel buffers held written frames"); + assert!( + read < queued * frame.len(), + "the writer kept writing after its receive loop tore the connection down: \ + read={read} queued_bytes={}", + queued * frame.len() + ); + } + + /// A receive loop's teardown must leave alone a newer entry at its address, + /// and must not run `on_remove` for it. + #[tokio::test] + async fn proxied_receive_teardown_leaves_a_newer_connection_at_the_same_address() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let client = TcpStream::connect(listen).await.unwrap(); + let (server, peer_addr) = listener.accept().await.unwrap(); + let remote = TransportAddr::from_string(&peer_addr.to_string()); + let (read_half, _write_half) = server.into_split(); + + let (packet_tx, _packet_rx) = packet_channel(10); + let pool: ProxiedPool<()> = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(CountingStats::default()); + pool.lock().await.insert(remote.clone(), successor()); + + drop(client); + proxied_receive_loop( + read_half, + TransportId::new(1), + remote.clone(), + next_conn_id(), + packet_tx, + pool.clone(), + 1400, + stats.clone(), + "Test", + None, + None, + count_removal, + ) + .await; + assert_eq!( + stats.recv_errors.load(Ordering::Relaxed), + 1, + "the loop should have ended on EOF" + ); + + assert_eq!( + pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1234), + "the teardown removed a newer connection at its address" + ); + assert_eq!( + stats.removed.load(Ordering::Relaxed), + 0, + "the teardown ran on_remove for a connection it did not remove" + ); + } +} diff --git a/src/transport/stream.rs b/src/transport/stream.rs new file mode 100644 index 00000000..3bb62864 --- /dev/null +++ b/src/transport/stream.rs @@ -0,0 +1,51 @@ +//! Connection lifecycle rules shared by the stream transports. +//! +//! TCP and the SOCKS5-proxied Tor and Nym transports each give a pooled +//! connection its own writer task and receive loop, and either loop can +//! outlive the pool entry it was created with. The rules for when such a loop +//! may touch the pool are written once here. + +use std::collections::HashMap; + +use portable_atomic::{AtomicU64, Ordering}; + +use crate::transport::TransportAddr; + +/// Identity of one pooled stream connection. +/// +/// The pool is keyed by address, and a newer connection can take an address +/// while an older connection's writer or receive loop is still running. The +/// id tells the two apart. +pub(crate) type ConnId = u64; + +/// Source of connection ids. Process-wide rather than per transport, because +/// the accept loops that build connections are free functions with no +/// transport instance to hold a counter. +static NEXT_CONN_ID: AtomicU64 = AtomicU64::new(1); + +/// Hand out an id no other connection in this process has had. +pub(crate) fn next_conn_id() -> ConnId { + NEXT_CONN_ID.fetch_add(1, Ordering::Relaxed) +} + +/// A pooled stream connection that knows its own [`ConnId`]. +pub(crate) trait PooledConn { + /// The id this connection's writer and receive loop were given. + fn conn_id(&self) -> ConnId; +} + +/// Remove the entry at `addr`, but only if it is connection `id`. +/// +/// This is the only way a connection's own writer or receive loop removes a +/// pool entry. An entry with another id belongs to a newer connection at the +/// same address, and is left alone. +pub(crate) fn remove_own( + pool: &mut HashMap, + addr: &TransportAddr, + id: ConnId, +) -> Option { + if pool.get(addr)?.conn_id() != id { + return None; + } + pool.remove(addr) +} diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index c07e4689..ec7b48e3 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -32,6 +32,7 @@ use super::{ }; use crate::config::TcpConfig; use crate::transport::framing::read_fmp_packet; +use crate::transport::stream::{ConnId, next_conn_id, remove_own}; use pool::{ConnectingEntry, ConnectingPool, ConnectionPool, Direction, TcpConnection}; use stats::TcpStats; @@ -425,12 +426,14 @@ impl TcpTransport { let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); let mtu = mss_mtu; + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tcp_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -450,6 +453,7 @@ impl TcpTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), )); @@ -461,6 +465,7 @@ impl TcpTransport { mtu: mss_mtu, established_at: Instant::now(), direction: Direction::Outbound, + id, }; let mut pool = self.pool.lock().await; @@ -689,12 +694,14 @@ impl TcpTransport { let pool = self.pool.clone(); let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tcp_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mss_mtu, @@ -714,6 +721,7 @@ impl TcpTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), )); @@ -725,6 +733,7 @@ impl TcpTransport { mtu: mss_mtu, established_at: Instant::now(), direction: Direction::Outbound, + id, }; // Use try_lock since we're in a sync context and the pool @@ -932,12 +941,14 @@ async fn accept_loop( // or it would remove nothing and leave an orphaned entry with // a permanently incremented inbound counter. let (ready_tx, ready_rx) = tokio::sync::oneshot::channel(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tcp_receive_loop( read_half, transport_id, recv_addr, + id, recv_packet_tx, recv_pool, conn_mtu, @@ -956,6 +967,7 @@ async fn accept_loop( send_rx, transport_id, remote_addr.clone(), + id, pool.clone(), stats.clone(), )); @@ -967,6 +979,7 @@ async fn accept_loop( mtu: conn_mtu, established_at: Instant::now(), direction: Direction::Inbound, + id, }; let mut pool_guard = pool.lock().await; @@ -1018,6 +1031,10 @@ async fn accept_loop( /// receive task is aborted here rather than left to notice on its own, because /// a half-closed connection is not something either side should keep. /// +/// The entry is removed only when it carries this connection's `id`. A writer +/// can outlive its entry, and by the time its write fails a newer connection +/// may hold the address; that one is left alone. +/// /// Frames are written whole. A partial write followed by an error takes the /// connection down with it, so the peer never sees a frame it cannot /// resynchronise from. @@ -1026,6 +1043,7 @@ async fn tcp_send_loop( mut frames: mpsc::Receiver>, transport_id: TransportId, remote_addr: TransportAddr, + id: ConnId, pool: ConnectionPool, stats: Arc, ) { @@ -1050,11 +1068,12 @@ async fn tcp_send_loop( ); let removed = { let mut pool = pool.lock().await; - pool.remove(&remote_addr) + remove_own(&mut pool, &remote_addr, id) }; + // The removed entry's `send_task` is this task, which returns + // below, so only the receive task needs stopping. if let Some(conn) = removed { conn.recv_task.abort(); - conn.send_task.abort(); match conn.direction { Direction::Inbound => stats.record_pool_inbound_removed(), Direction::Outbound => stats.record_pool_outbound_removed(), @@ -1087,11 +1106,20 @@ async fn tcp_send_loop( /// slot from accept) and `None` for outbound ones. `ready_rx`, when /// present, is the accept loop's readiness barrier: the loop must not run /// its cleanup before the accept loop has inserted the pool entry. +/// +/// `id` is the connection's identity. The cleanup removes the entry at +/// `remote_addr` only when it carries this id, so a loop whose entry has +/// already been replaced by a newer connection at the same address leaves +/// that connection alone. When it does remove its own entry it also stops the +/// entry's writer: the loop ended on EOF, a read error or a missed deadline, +/// and frames still queued for a connection in that state are not worth +/// writing. #[allow(clippy::too_many_arguments)] async fn tcp_receive_loop( mut reader: tokio::net::tcp::OwnedReadHalf, transport_id: TransportId, remote_addr: TransportAddr, + id: ConnId, packet_tx: PacketTx, pool: ConnectionPool, mtu: u16, @@ -1182,9 +1210,10 @@ async fn tcp_receive_loop( // entry actually being removed so a double-cleanup never drives // the counter below zero. let mut pool_guard = pool.lock().await; - let removed = pool_guard.remove(&remote_addr).is_some(); + let removed = remove_own(&mut pool_guard, &remote_addr, id); drop(pool_guard); - if removed { + if let Some(conn) = removed { + conn.send_task.abort(); match direction { Direction::Inbound => stats.record_pool_inbound_removed(), Direction::Outbound => stats.record_pool_outbound_removed(), @@ -2227,6 +2256,7 @@ mod tests { let pool: ConnectionPool = Arc::new(Mutex::new(HashMap::new())); let stats = Arc::new(TcpStats::new()); + let id = next_conn_id(); pool.lock().await.insert( remote.clone(), TcpConnection { @@ -2236,6 +2266,7 @@ mod tests { mtu: 1400, established_at: Instant::now(), direction: Direction::Inbound, + id, }, ); stats.record_pool_inbound_added(); @@ -2248,6 +2279,7 @@ mod tests { read_half, TransportId::new(1), remote.clone(), + id, tx, pool.clone(), 1400, @@ -2321,4 +2353,482 @@ mod tests { drop(client); transport.stop_async().await.unwrap(); } + + // ======================================================================== + // Connection identity and failure teardown + // ======================================================================== + + /// Bind a listener whose accepted sockets get a small receive buffer. + /// + /// Setting `SO_RCVBUF` before `listen` locks the size on every accepted + /// socket, so kernel autotuning cannot grow it past what a test's fill + /// can overrun. + fn capped_deaf_listener() -> TcpListener { + let socket = tokio::net::TcpSocket::new_v4().unwrap(); + socket.set_recv_buffer_size(64 * 1024).unwrap(); + socket.bind("127.0.0.1:0".parse().unwrap()).unwrap(); + socket.listen(8).unwrap() + } + + /// Fill `remote`'s send queue behind a peer that does not read, until the + /// writer is parked in `write_all`, and return how many frames were + /// queued. + /// + /// Every send is attempted whatever the previous one returned, with a + /// yield between sends so the writer runs. A queue still full 300 ms + /// after the last send means the writer could not drain it, which is the + /// state the caller's teardown needs; anything else panics rather than + /// letting the caller pass without it. + async fn park_tcp_writer(t: &TcpTransport, remote: &TransportAddr, frame: &[u8]) -> usize { + let mut queued = 0usize; + let mut refused = 0usize; + for _ in 0..8000 { + match timeout(Duration::from_secs(2), t.send_async(remote, frame)).await { + Ok(Ok(_)) => queued += 1, + Ok(Err(_)) => refused += 1, + Err(_) => panic!("send blocked on a peer that stopped reading"), + } + tokio::task::yield_now().await; + } + tokio::time::sleep(Duration::from_millis(300)).await; + let capacity = t + .pool + .lock() + .await + .get(remote) + .map(|c| c.send_tx.capacity()); + assert_eq!( + capacity, + Some(0), + "setup did not park the writer: queued={queued} refused={refused}" + ); + assert!(refused > 0, "setup never filled the queue: queued={queued}"); + queued + } + + /// Read `stream` to EOF within `limit`, returning the byte count, or + /// `None` if EOF did not arrive in time. + async fn read_to_eof(stream: &mut TcpStream, limit: Duration) -> Option { + use tokio::io::AsyncReadExt; + let mut buf = vec![0u8; 64 * 1024]; + timeout(limit, async { + let mut total = 0usize; + loop { + match stream.read(&mut buf).await { + Ok(0) | Err(_) => return total, + Ok(n) => total += n, + } + } + }) + .await + .ok() + } + + /// A writer whose write fails must not remove a newer connection that has + /// taken its address in the pool. + /// + /// The successor is marked by its MTU. The peer resets the connection, and + /// frames are pushed until the writer's write fails, since the first write + /// after a reset can still succeed. + #[tokio::test] + async fn tcp_writer_error_leaves_a_newer_connection_at_the_same_address() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let client = TcpStream::connect(listen).await.unwrap(); + let (server, _) = listener.accept().await.unwrap(); + socket2::SockRef::from(&server) + .set_linger(Some(Duration::ZERO)) + .unwrap(); + drop(server); + let (_read_half, write_half) = client.into_split(); + let remote = TransportAddr::from_string(&listen.to_string()); + + let pool: ConnectionPool = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(TcpStats::new()); + pool.lock().await.insert( + remote.clone(), + TcpConnection { + send_tx: mpsc::channel(1).0, + send_task: tokio::spawn(async {}), + recv_task: tokio::spawn(async {}), + mtu: 1234, + established_at: Instant::now(), + direction: Direction::Outbound, + id: next_conn_id(), + }, + ); + + let (send_tx, send_rx) = mpsc::channel(pool::SEND_QUEUE_DEPTH); + let writer = tokio::spawn(tcp_send_loop( + write_half, + send_rx, + TransportId::new(1), + remote.clone(), + next_conn_id(), + pool.clone(), + stats.clone(), + )); + + let frame = build_msg1_frame(); + let deadline = Instant::now() + Duration::from_secs(5); + while !writer.is_finished() && Instant::now() < deadline { + let _ = send_tx.try_send(frame.clone()); + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert!(writer.is_finished(), "the writer never hit a write error"); + assert_eq!( + stats.snapshot().send_errors, + 1, + "the writer's error path must have run" + ); + + assert_eq!( + pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1234), + "a failed writer removed the newer connection at its address" + ); + } + + /// A receive loop that ends on EOF must stop its writer rather than leave + /// it writing to a peer that has gone. + /// + /// The writer is parked on a peer that does not read, with a full queue. + /// The peer then half-closes, which ends the receive loop, and only + /// afterwards reads. A writer left running delivers every frame it had + /// queued; a stopped one delivers fewer, since the queue alone holds + /// more frames than the kernel buffers leave unread. + #[tokio::test] + async fn tcp_receive_teardown_stops_the_writer() { + let (tx1, _rx1) = packet_channel(100); + let mut t1 = TcpTransport::new(TransportId::new(1), None, make_outbound_config(), tx1); + t1.start_async().await.unwrap(); + let listener = capped_deaf_listener(); + let remote = TransportAddr::from_string(&listener.local_addr().unwrap().to_string()); + let frame = vec![0xAB; 1400]; + + let queued = park_tcp_writer(&t1, &remote, &frame).await; + let (mut peer, _) = listener.accept().await.unwrap(); + + peer.shutdown().await.unwrap(); + assert!( + wait_until( + || t1.stats().snapshot().pool_outbound == 0, + Duration::from_secs(5) + ) + .await, + "the receive loop should have torn the connection down on EOF" + ); + + let read = read_to_eof(&mut peer, Duration::from_secs(10)) + .await + .expect("the connection was never closed toward the peer"); + assert!(read > 0, "the kernel buffers held written frames"); + assert!( + read < queued * frame.len(), + "the writer kept writing after its receive loop tore the connection down: \ + read={read} queued_bytes={}", + queued * frame.len() + ); + + t1.stop_async().await.unwrap(); + } + + /// A connection displaced from the pool by a newer one at the same address + /// must not remove the newer one when its own receive loop ends. + /// + /// Both are built by `promote_connection`, and the MTU marks which entry + /// is pooled. The last step checks the newer connection still removes its + /// own entry. + #[tokio::test] + async fn tcp_displaced_connection_cannot_remove_its_successor() { + let (tx1, _rx1) = packet_channel(100); + let mut t1 = TcpTransport::new(TransportId::new(1), None, make_outbound_config(), tx1); + t1.start_async().await.unwrap(); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let remote = TransportAddr::from_string(&listen.to_string()); + + let a = TcpStream::connect(listen).await.unwrap(); + let (sa, _) = listener.accept().await.unwrap(); + let b = TcpStream::connect(listen).await.unwrap(); + let (sb, _) = listener.accept().await.unwrap(); + + t1.promote_connection(&remote, a, 1400); + t1.promote_connection(&remote, b, 1300); + { + let pool = t1.pool.lock().await; + assert_eq!(pool.len(), 1); + assert_eq!(pool.get(&remote).map(|c| c.mtu), Some(1300)); + } + + drop(sa); + assert!( + wait_until( + || t1.stats().snapshot().recv_errors == 1, + Duration::from_secs(2) + ) + .await, + "the displaced connection's receive loop should have read EOF" + ); + tokio::time::sleep(Duration::from_millis(50)).await; + assert_eq!( + t1.pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1300), + "the displaced connection's teardown removed its successor" + ); + + drop(sb); + assert!( + wait_until( + || t1.stats().snapshot().recv_errors == 2, + Duration::from_secs(2) + ) + .await, + "the newer connection's receive loop should have read EOF" + ); + assert!( + wait_until( + || t1.pool.try_lock().map(|p| p.is_empty()).unwrap_or(false), + Duration::from_secs(2) + ) + .await, + "the newer connection's teardown should remove its own entry" + ); + + t1.stop_async().await.unwrap(); + } + + /// A receive loop's teardown must leave alone a newer entry at its address, + /// and must not decrement the counter for it. + /// + /// Calls the loop directly against a hand-built successor, so the check is + /// on the teardown alone and not on how a constructor wires it. + #[tokio::test] + async fn tcp_receive_teardown_leaves_a_newer_connection_at_the_same_address() { + let (tx, _rx) = packet_channel(10); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let client = TcpStream::connect(listen).await.unwrap(); + let (server, peer_addr) = listener.accept().await.unwrap(); + let remote = TransportAddr::from_string(&peer_addr.to_string()); + let (read_half, _write_half) = server.into_split(); + + let pool: ConnectionPool = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(TcpStats::new()); + pool.lock().await.insert( + remote.clone(), + TcpConnection { + send_tx: mpsc::channel(1).0, + send_task: tokio::spawn(async {}), + recv_task: tokio::spawn(async {}), + mtu: 1234, + established_at: Instant::now(), + direction: Direction::Outbound, + id: next_conn_id(), + }, + ); + stats.record_pool_outbound_added(); + assert_eq!(stats.snapshot().pool_outbound, 1); + + drop(client); + tcp_receive_loop( + read_half, + TransportId::new(1), + remote.clone(), + next_conn_id(), + tx, + pool.clone(), + 1400, + stats.clone(), + Direction::Outbound, + None, + None, + ) + .await; + assert_eq!( + stats.snapshot().recv_errors, + 1, + "the loop should have ended on EOF" + ); + + assert_eq!( + pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1234), + "the teardown removed a newer connection at its address" + ); + assert_eq!( + stats.snapshot().pool_outbound, + 1, + "the teardown decremented for a connection it did not remove" + ); + } + + /// A connection built by connect-on-send removes its own entry when its + /// receive loop ends. + #[tokio::test] + async fn tcp_connect_teardown_removes_its_own_entry() { + use tokio::io::AsyncReadExt; + let (tx1, _rx1) = packet_channel(100); + let mut t1 = TcpTransport::new(TransportId::new(1), None, make_outbound_config(), tx1); + t1.start_async().await.unwrap(); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let remote = TransportAddr::from_string(&listener.local_addr().unwrap().to_string()); + let frame = build_msg1_frame(); + + t1.send_async(&remote, &frame).await.unwrap(); + let (mut server, _) = listener.accept().await.unwrap(); + let mut buf = vec![0u8; frame.len()]; + timeout(Duration::from_secs(2), server.read_exact(&mut buf)) + .await + .expect("timeout waiting for the frame") + .unwrap(); + assert_eq!(buf, frame); + assert_eq!(t1.stats().snapshot().pool_outbound, 1); + + drop(server); + assert!( + wait_until( + || t1.stats().snapshot().pool_outbound == 0, + Duration::from_secs(2) + ) + .await, + "the receive loop should release the outbound slot" + ); + assert!( + t1.pool.lock().await.is_empty(), + "the receive loop should remove its own entry" + ); + + t1.stop_async().await.unwrap(); + } + + /// Two live inbound connections can share one remote address when they + /// reach a wildcard listener on different local addresses. Closing the + /// older one must not remove the newer one's pool entry. + /// + /// Known gap, not covered here: two live inbound connections that share a + /// remote address still share one pool key. The second accept replaces + /// the first entry without stopping its tasks and counts a second inbound + /// slot, so the inbound counter ends one above the pool once both + /// connections close. This test checks only that the older connection's + /// teardown no longer removes the newer connection's entry. + /// + /// Linux only: it needs `127.0.0.2` on the loopback interface and Linux + /// `SO_REUSEADDR` semantics to bind two client sockets to one port. + #[cfg(target_os = "linux")] + #[tokio::test] + async fn closing_the_older_of_two_inbound_connections_sharing_a_remote_address_keeps_the_newer_entry() + { + use socket2::{Domain, Socket, Type}; + use tokio::io::AsyncReadExt; + + let (tx, mut rx) = packet_channel(100); + let config = TcpConfig { + bind_addr: Some("0.0.0.0:0".to_string()), + mtu: Some(1400), + ..Default::default() + }; + let mut transport = TcpTransport::new(TransportId::new(1), None, config, tx); + transport.start_async().await.unwrap(); + let port = transport.local_addr().unwrap().port(); + + let client = |local: SocketAddr| { + let sock = Socket::new(Domain::IPV4, Type::STREAM, None).unwrap(); + sock.set_reuse_address(true).unwrap(); + sock.bind(&local.into()).unwrap(); + sock + }; + let into_tokio = |sock: Socket| { + let std_stream: std::net::TcpStream = sock.into(); + std_stream.set_nonblocking(true).unwrap(); + TcpStream::from_std(std_stream).unwrap() + }; + let sock_a = client("127.0.0.1:0".parse().unwrap()); + let source = sock_a.local_addr().unwrap().as_socket().unwrap(); + let sock_b = client(source); + let remote = TransportAddr::from_string(&source.to_string()); + let frame = build_msg1_frame(); + + // Admit A before B dials, so B's entry is the one left in the pool. + let target_a: SocketAddr = format!("127.0.0.1:{port}").parse().unwrap(); + sock_a.connect(&target_a.into()).unwrap(); + let mut a = into_tokio(sock_a); + a.write_all(&frame).await.unwrap(); + let first = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for A's frame") + .expect("packet channel closed"); + assert_eq!(first.remote_addr, remote); + assert_eq!(transport.stats().snapshot().connections_accepted, 1); + + let target_b: SocketAddr = format!("127.0.0.2:{port}").parse().unwrap(); + sock_b.connect(&target_b.into()).unwrap(); + let mut b = into_tokio(sock_b); + b.write_all(&frame).await.unwrap(); + let second = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for B's frame") + .expect("packet channel closed"); + assert_eq!( + second.remote_addr, remote, + "B must arrive with the same remote address as A" + ); + assert_eq!(transport.stats().snapshot().connections_accepted, 2); + { + let pool = transport.pool.lock().await; + assert_eq!(pool.len(), 1); + assert!(pool.contains_key(&remote)); + } + + drop(a); + assert!( + wait_until( + || transport.stats().snapshot().recv_errors == 1, + Duration::from_secs(2) + ) + .await, + "A's receive loop should have read EOF" + ); + tokio::time::sleep(Duration::from_millis(50)).await; + + let send_tx = transport + .pool + .lock() + .await + .get(&remote) + .map(|c| c.send_tx.clone()) + .expect("closing the older connection removed the newer connection's entry"); + send_tx.try_send(frame.clone()).unwrap(); + drop(send_tx); + let mut buf = vec![0u8; frame.len()]; + timeout(Duration::from_secs(2), b.read_exact(&mut buf)) + .await + .expect("the surviving entry is not B's live connection") + .unwrap(); + assert_eq!(buf, frame); + + drop(b); + assert!( + wait_until( + || transport.stats().snapshot().recv_errors == 2, + Duration::from_secs(2) + ) + .await, + "B's receive loop should have read EOF" + ); + assert!( + wait_until( + || transport + .pool + .try_lock() + .map(|p| p.is_empty()) + .unwrap_or(false), + Duration::from_secs(2) + ) + .await, + "B's teardown should remove its own entry" + ); + + transport.stop_async().await.unwrap(); + } } diff --git a/src/transport/tcp/pool.rs b/src/transport/tcp/pool.rs index e8e678ca..ccacc4d7 100644 --- a/src/transport/tcp/pool.rs +++ b/src/transport/tcp/pool.rs @@ -10,6 +10,7 @@ use tokio::sync::{Mutex, mpsc}; use tokio::task::JoinHandle; use tokio::time::Instant; +use crate::transport::stream::{ConnId, PooledConn}; use crate::transport::{TransportAddr, TransportError}; /// Direction of a pooled connection, used to drive separate @@ -53,6 +54,17 @@ pub(crate) struct TcpConnection { pub(crate) established_at: Instant, /// Direction of the connection — drives pool-inbound/outbound accounting. pub(crate) direction: Direction, + /// Identity of this connection, shared with its writer and receive loop. + /// Either loop removes the entry at its address only when the entry + /// carries this id, so a loop that outlives its connection cannot remove + /// a newer connection at the same address. + pub(crate) id: ConnId, +} + +impl PooledConn for TcpConnection { + fn conn_id(&self) -> ConnId { + self.id + } } /// Shared connection pool. diff --git a/src/transport/tor/mod.rs b/src/transport/tor/mod.rs index 96becca9..b194f600 100644 --- a/src/transport/tor/mod.rs +++ b/src/transport/tor/mod.rs @@ -34,6 +34,7 @@ use crate::transport::socks5::{ Socks5Auth, Socks5Dialer, SocksTarget, poll_connecting, proxied_receive_loop, proxied_send_loop, }; +use crate::transport::stream::{ConnId, next_conn_id}; use crate::transport::tcp::INBOUND_FIRST_FRAME_TIMEOUT; use control::{ControlAuth, TorControlClient, TorMonitoringInfo}; use stats::TorStats; @@ -756,12 +757,14 @@ impl TorTransport { let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); let mtu = self.config.mtu(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tor_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -781,6 +784,7 @@ impl TorTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), "Tor", @@ -797,6 +801,7 @@ impl TorTransport { mtu, established_at: Instant::now(), meta: Direction::Outbound, + id, }; let mut pool = self.pool.lock().await; @@ -940,12 +945,14 @@ impl TorTransport { let pool = self.pool.clone(); let recv_stats = self.stats.clone(); let remote_addr = addr.clone(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tor_receive_loop( read_half, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -965,6 +972,7 @@ impl TorTransport { send_rx, transport_id, addr.clone(), + id, self.pool.clone(), self.stats.clone(), "Tor", @@ -981,6 +989,7 @@ impl TorTransport { mtu, established_at: Instant::now(), meta: Direction::Outbound, + id, }; // Use try_lock since we're in a sync context and the pool @@ -1096,6 +1105,7 @@ async fn tor_receive_loop( reader: tokio::net::tcp::OwnedReadHalf, transport_id: TransportId, remote_addr: TransportAddr, + id: ConnId, packet_tx: PacketTx, pool: ProxiedPool, mtu: u16, @@ -1108,6 +1118,7 @@ async fn tor_receive_loop( reader, transport_id, remote_addr.clone(), + id, packet_tx, pool, mtu, @@ -1236,12 +1247,14 @@ async fn tor_accept_loop( // nothing and leave an orphaned entry with a permanently incremented // inbound counter. let (ready_tx, ready_rx) = tokio::sync::oneshot::channel(); + let id = next_conn_id(); let recv_task = tokio::spawn(async move { tor_receive_loop( read_half, transport_id, recv_addr, + id, recv_tx, recv_pool, mtu, @@ -1259,6 +1272,7 @@ async fn tor_accept_loop( send_rx, transport_id, remote_addr.clone(), + id, pool.clone(), stats.clone(), "Tor", @@ -1275,6 +1289,7 @@ async fn tor_accept_loop( mtu, established_at: Instant::now(), meta: Direction::Inbound, + id, }; let evicted = { @@ -1289,6 +1304,7 @@ async fn tor_accept_loop( // just inserted and decrement for it, leaking one slot and // orphaning a live connection. old.recv_task.abort(); + old.send_task.abort(); match old.meta { Direction::Inbound => stats.record_pool_inbound_removed(), Direction::Outbound => stats.record_pool_outbound_removed(), @@ -2153,6 +2169,7 @@ mod tests { let pool: ProxiedPool = Arc::new(Mutex::new(HashMap::new())); let stats = Arc::new(TorStats::new()); + let id = next_conn_id(); pool.lock().await.insert( remote.clone(), ProxiedConnection { @@ -2162,6 +2179,7 @@ mod tests { mtu: 1400, established_at: Instant::now(), meta: Direction::Inbound, + id, }, ); stats.record_pool_inbound_added(); @@ -2174,6 +2192,7 @@ mod tests { read_half, TransportId::new(1), remote.clone(), + id, tx, pool.clone(), 1400, @@ -2223,11 +2242,13 @@ mod tests { let recv_pool = pool.clone(); let recv_stats = stats.clone(); let recv_addr = remote.clone(); + let id = next_conn_id(); let mut handle = tokio::spawn(async move { tor_receive_loop( read_half, TransportId::new(1), recv_addr, + id, tx, recv_pool, 1400, @@ -2256,6 +2277,7 @@ mod tests { mtu: 1400, established_at: Instant::now(), meta: Direction::Inbound, + id, }, ); stats.record_pool_inbound_added(); @@ -2313,6 +2335,7 @@ mod tests { mtu: 1400, established_at: Instant::now(), meta: Direction::Inbound, + id: next_conn_id(), }, ); stats.record_pool_inbound_added(); @@ -2349,4 +2372,188 @@ mod tests { accept.abort(); drop(sock); } + + // ======================================================================== + // Connection identity and failure teardown + // ======================================================================== + + /// A connection displaced from the pool by a newer one at the same address + /// must not remove the newer one when its own receive loop ends. + /// + /// Both are built by `promote_connection`, and the MTU marks which entry + /// is pooled. The last step checks the newer connection still removes its + /// own entry. + #[tokio::test] + async fn tor_displaced_connection_cannot_remove_its_successor() { + let (tx, _rx) = packet_channel(32); + let tor = TorTransport::new(TransportId::new(1), None, make_config(), tx); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let remote = TransportAddr::from_string(&listen.to_string()); + + let a = TcpStream::connect(listen).await.unwrap(); + let (sa, _) = listener.accept().await.unwrap(); + let b = TcpStream::connect(listen).await.unwrap(); + let (sb, _) = listener.accept().await.unwrap(); + + tor.promote_connection(&remote, a, 1400); + tor.promote_connection(&remote, b, 1300); + { + let pool = tor.pool.lock().await; + assert_eq!(pool.len(), 1); + assert_eq!(pool.get(&remote).map(|c| c.mtu), Some(1300)); + } + + drop(sa); + assert!( + wait_until( + || tor.stats().snapshot().recv_errors == 1, + Duration::from_secs(2) + ) + .await, + "the displaced connection's receive loop should have read EOF" + ); + tokio::time::sleep(Duration::from_millis(50)).await; + assert_eq!( + tor.pool.lock().await.get(&remote).map(|c| c.mtu), + Some(1300), + "the displaced connection's teardown removed its successor" + ); + + drop(sb); + assert!( + wait_until( + || tor.stats().snapshot().recv_errors == 2, + Duration::from_secs(2) + ) + .await, + "the newer connection's receive loop should have read EOF" + ); + assert!( + wait_until( + || tor.pool.try_lock().map(|p| p.is_empty()).unwrap_or(false), + Duration::from_secs(2) + ) + .await, + "the newer connection's teardown should remove its own entry" + ); + } + + /// A connection admitted by the onion accept loop removes its own entry and + /// releases its inbound slot when its receive loop ends. + #[tokio::test] + async fn onion_accept_teardown_removes_its_own_entry() { + use socket2::{Domain, Socket, Type}; + + let (tx, mut rx) = packet_channel(10); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + + let sock = Socket::new(Domain::IPV4, Type::STREAM, None).unwrap(); + sock.bind(&"127.0.0.1:0".parse::().unwrap().into()) + .unwrap(); + let client_addr = sock.local_addr().unwrap().as_socket().unwrap(); + let remote = TransportAddr::from_string(&client_addr.to_string()); + + let (pool, stats, accept) = spawn_onion_accept_loop(listener, tx, Duration::from_secs(5)); + + sock.connect(&listen.into()).unwrap(); + let std_stream: std::net::TcpStream = sock.into(); + std_stream.set_nonblocking(true).unwrap(); + let mut client = TcpStream::from_std(std_stream).unwrap(); + client.write_all(&build_msg1_frame()).await.unwrap(); + + let packet = tokio::time::timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the frame") + .expect("packet channel closed"); + assert_eq!(packet.remote_addr, remote); + assert!(pool.lock().await.contains_key(&remote)); + assert_eq!(stats.pool_inbound_count(), 1); + + drop(client); + assert!( + wait_until(|| stats.pool_inbound_count() == 0, Duration::from_secs(2)).await, + "the receive loop should release the inbound slot" + ); + assert!( + !pool.lock().await.contains_key(&remote), + "the receive loop should remove its own entry" + ); + + accept.abort(); + } + + /// When the onion accept loop evicts an entry at a colliding address, it + /// must stop that entry's writer as well as its receive task. + /// + /// The stale writer holds a oneshot sender and never finishes, so the + /// sender is dropped only if the task is aborted: dropping its handle + /// alone leaves it running. + #[tokio::test] + async fn evicting_a_colliding_onion_entry_stops_its_writer() { + use socket2::{Domain, Socket, Type}; + + let (tx, _rx) = packet_channel(10); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + + let sock = Socket::new(Domain::IPV4, Type::STREAM, None).unwrap(); + sock.bind(&"127.0.0.1:0".parse::().unwrap().into()) + .unwrap(); + let client_addr = sock.local_addr().unwrap().as_socket().unwrap(); + let remote = TransportAddr::from_string(&client_addr.to_string()); + + let pool: ProxiedPool = Arc::new(Mutex::new(HashMap::new())); + let stats = Arc::new(TorStats::new()); + let (guard_tx, guard_rx) = tokio::sync::oneshot::channel::<()>(); + pool.lock().await.insert( + remote.clone(), + ProxiedConnection { + send_tx: tokio::sync::mpsc::channel(1).0, + send_task: tokio::spawn(async move { + let _guard = guard_tx; + std::future::pending::<()>().await + }), + recv_task: tokio::spawn(std::future::pending::<()>()), + mtu: 1400, + established_at: Instant::now(), + meta: Direction::Inbound, + id: next_conn_id(), + }, + ); + stats.record_pool_inbound_added(); + + let accept = tokio::spawn(tor_accept_loop( + listener, + TransportId::new(1), + tx, + pool.clone(), + 1400, + 64, + Duration::from_secs(5), + stats.clone(), + )); + + sock.connect(&listen.into()).unwrap(); + assert!( + wait_until( + || stats.snapshot().connections_accepted == 1, + Duration::from_secs(2) + ) + .await, + "the colliding connection should have been accepted" + ); + + assert!( + matches!( + tokio::time::timeout(Duration::from_secs(1), guard_rx).await, + Ok(Err(_)) + ), + "the evicted entry's writer was left running" + ); + + accept.abort(); + drop(sock); + } } From 0c932635aecbb816896a4e344dc74b15a6f1f033 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Mon, 14 Sep 2026 16:03:15 +0000 Subject: [PATCH 11/11] fix(transport): let a deliberate close finish writing queued frames Sending on a TCP, Tor or Nym connection only queues the frame for the connection's writer task, and closing the connection aborted that task. A frame sent just before a close was never written if the writer had not run in between, and on a single-threaded runtime it had not. A control-API disconnect queues a Disconnect and then closes the connection, so the Disconnect was lost and the peer kept this node until its own link-dead detection removed it. A deliberate close now drops the connection's queue and aborts only its receive task. The writer writes what was queued and exits, which closes the stream. A detached timer aborts the writer if it is still writing after five seconds, so the close never waits on the peer. Stopping the transport, and every teardown after a connection has failed, still abort the writer at once. A writer that outlives its pool entry this way removes only an entry carrying its own connection id, so it cannot tear down a newer connection at the same address. The new tests were run on the unfixed tree first. Red there: a frame queued just before close did not arrive within two seconds over TCP, Tor or Nym, and after an api_disconnect over TCP the peer still had this node after two seconds of packet processing. Also added, and not red before this change because they guard what it adds: a close whose writer is parked on a peer that has stopped reading returns within 200 ms, and the drain timer both aborts a writer that outlives its bound and ends as soon as the writer exits. Break-checks, each reverting one element and running the tests named for it: aborting the writer again in the TCP close reds the TCP test and the api_disconnect test, and in the Tor or Nym close reds that transport's test; a drain timer that never aborts reds its abort test; a close that awaits the drain reds the 200 ms test; and giving the TCP accept loop's receive loop a different id from its entry reds the TCP test's check that the peer releases its inbound slot after the close. --- CHANGELOG.md | 6 ++- src/node/tests/tcp.rs | 40 ++++++++++++++ src/transport/nym/mod.rs | 57 ++++++++++++++++++-- src/transport/stream.rs | 68 +++++++++++++++++++++++ src/transport/tcp/mod.rs | 114 ++++++++++++++++++++++++++++++++++++--- src/transport/tor/mod.rs | 80 +++++++++++++++++++++++++-- 6 files changed, 350 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 706e136f..22fcfd3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -399,7 +399,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 receive loop act only on their own connection: one that outlives its connection can no longer tear down a newer connection that has taken the same address, and a receive loop that ends stops its writer rather than - leaving it writing to a peer that has gone. + leaving it writing to a peer that has gone. Closing one of their connections + on purpose, as a control-API disconnect does, now lets the writer finish the + frames already queued, within five seconds, instead of discarding them, so a + Disconnect sent just before the close reaches the peer. Stopping the + transport, or a connection that has failed, still discards them. - A per-peer `connect()`-ed UDP socket is no longer left pinned to an interface the host has moved off. Established UDP peers get their own socket for the diff --git a/src/node/tests/tcp.rs b/src/node/tests/tcp.rs index 669106ad..6c14bbf5 100644 --- a/src/node/tests/tcp.rs +++ b/src/node/tests/tcp.rs @@ -459,3 +459,43 @@ async fn test_api_disconnect_closes_the_tcp_connection() { cleanup_nodes(&mut nodes).await; } + +/// The Disconnect `api_disconnect` sends must reach the peer before the TCP +/// connection closes, so the peer forgets this node at once. +/// +/// The send only queues the Disconnect for the connection's writer, and the +/// close follows in the same call. Nothing else removes the peer on node 1 +/// within the wait: its TCP EOF removes only the pool entry, and link-dead +/// detection takes far longer. +#[tokio::test] +async fn api_disconnect_delivers_the_disconnect_before_closing() { + let mut nodes = vec![make_test_node_tcp().await, make_test_node_tcp().await]; + + initiate_handshake(&mut nodes, 0, 1).await; + drain_all_packets(&mut nodes, false).await; + + let addr_0 = *nodes[0].node.node_addr(); + let node1_npub = nodes[1].node.npub(); + assert!( + nodes[1].node.get_peer(&addr_0).is_some(), + "node 1 should have node 0 as peer" + ); + + nodes[0] + .node + .api_disconnect(&node1_npub) + .await + .expect("api_disconnect should succeed"); + + let deadline = std::time::Instant::now() + Duration::from_secs(2); + while nodes[1].node.get_peer(&addr_0).is_some() && std::time::Instant::now() < deadline { + spanning_tree::process_available_packets(&mut nodes).await; + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert!( + nodes[1].node.get_peer(&addr_0).is_none(), + "node 1 never received the Disconnect sent before the close" + ); + + cleanup_nodes(&mut nodes).await; +} diff --git a/src/transport/nym/mod.rs b/src/transport/nym/mod.rs index 821a1b03..f1a5dea0 100644 --- a/src/transport/nym/mod.rs +++ b/src/transport/nym/mod.rs @@ -24,7 +24,7 @@ use crate::transport::socks5::{ Socks5Auth, Socks5Dialer, SocksTarget, poll_connecting, proxied_receive_loop, proxied_send_loop, }; -use crate::transport::stream::{ConnId, next_conn_id}; +use crate::transport::stream::{ConnId, WRITER_DRAIN_TIMEOUT, drain_writer, next_conn_id}; use stats::NymStats; use std::collections::HashMap; @@ -585,11 +585,22 @@ impl NymTransport { } /// Close a specific connection asynchronously. + /// + /// Aborts the receive task and lets the writer finish the frames already + /// queued, within [`WRITER_DRAIN_TIMEOUT`], without waiting for it. This + /// mirrors `TcpTransport::close_connection_async`. pub async fn close_connection_async(&self, addr: &TransportAddr) { let mut pool = self.pool.lock().await; if let Some(conn) = pool.remove(addr) { - conn.recv_task.abort(); - conn.send_task.abort(); + let ProxiedConnection { + send_tx, + send_task, + recv_task, + .. + } = conn; + drop(send_tx); + recv_task.abort(); + drain_writer(send_task, WRITER_DRAIN_TIMEOUT); debug!( transport_id = %self.transport_id, remote_addr = %addr, @@ -1188,4 +1199,44 @@ mod tests { nym.stop_async().await.unwrap(); } + + // ======================================================================== + // Deliberate close finishes the frames already queued + // ======================================================================== + + /// A frame queued immediately before a deliberate close must still reach + /// the peer through the proxy, and the close must still end the + /// connection at the far side. + #[tokio::test] + async fn nym_frame_queued_just_before_close_still_reaches_the_peer() { + let (mut dest, mut dest_rx, mut nym, target) = nym_via_mock_proxy().await; + + let frame = build_msg1_frame(); + nym.send_async(&target, &frame).await.unwrap(); + let first = tokio::time::timeout(Duration::from_secs(2), dest_rx.recv()) + .await + .expect("timeout waiting for the first frame") + .expect("channel closed"); + assert_eq!(first.data, frame); + + nym.send_async(&target, &frame).await.unwrap(); + nym.close_connection_async(&target).await; + + let second = tokio::time::timeout(Duration::from_secs(2), dest_rx.recv()) + .await + .expect("a frame queued just before close was never written") + .expect("channel closed"); + assert_eq!(second.data, frame); + assert!( + wait_until( + || dest.stats().snapshot().pool_inbound == 0, + Duration::from_secs(5) + ) + .await, + "the close must still end the connection once the queue is written" + ); + + nym.stop_async().await.unwrap(); + dest.stop_async().await.unwrap(); + } } diff --git a/src/transport/stream.rs b/src/transport/stream.rs index 3bb62864..103ed68d 100644 --- a/src/transport/stream.rs +++ b/src/transport/stream.rs @@ -6,8 +6,10 @@ //! may touch the pool are written once here. use std::collections::HashMap; +use std::time::Duration; use portable_atomic::{AtomicU64, Ordering}; +use tokio::task::JoinHandle; use crate::transport::TransportAddr; @@ -49,3 +51,69 @@ pub(crate) fn remove_own( } pool.remove(addr) } + +/// How long a deliberately closed connection's writer may keep writing the +/// frames already queued before it is stopped. +/// +/// It bounds only how long the socket and the writer task outlive the close; +/// no caller waits on it. A peer that is still reading drains a full queue in +/// far less, and one that has not drained it by then has stopped reading. +pub(crate) const WRITER_DRAIN_TIMEOUT: Duration = Duration::from_secs(5); + +/// Let a closed connection's writer finish the frames already queued, and stop +/// it if it is still running after `bound`. +/// +/// The caller must already have dropped the connection's queue, so the writer +/// exits once it has written what was queued. The wait runs on its own task, +/// which ends as soon as the writer does; the returned handle is that task's. +pub(crate) fn drain_writer(send_task: JoinHandle<()>, bound: Duration) -> JoinHandle<()> { + tokio::spawn(async move { + let mut send_task = send_task; + if tokio::time::timeout(bound, &mut send_task).await.is_err() { + send_task.abort(); + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A writer still running when the bound expires is stopped, and the + /// timer ends with it. + /// + /// The writer holds a oneshot sender and never finishes, so the sender is + /// dropped only if the task is aborted. + #[tokio::test] + async fn drain_writer_aborts_a_writer_that_outlives_the_bound() { + let (guard_tx, guard_rx) = tokio::sync::oneshot::channel::<()>(); + let writer = tokio::spawn(async move { + let _guard = guard_tx; + std::future::pending::<()>().await + }); + + let timer = drain_writer(writer, Duration::from_millis(50)); + assert!( + matches!( + tokio::time::timeout(Duration::from_secs(1), guard_rx).await, + Ok(Err(_)) + ), + "a writer still running at the bound was left running" + ); + tokio::time::timeout(Duration::from_secs(1), timer) + .await + .expect("the drain timer outlived the writer it stopped") + .unwrap(); + } + + /// The wait ends when the writer does, not when the bound expires. + #[tokio::test] + async fn drain_writer_ends_as_soon_as_the_writer_does() { + let writer = tokio::spawn(async {}); + let timer = drain_writer(writer, Duration::from_secs(30)); + tokio::time::timeout(Duration::from_millis(100), timer) + .await + .expect("the drain timer kept running after the writer exited") + .unwrap(); + } +} diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index ec7b48e3..1c63d545 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -32,7 +32,9 @@ use super::{ }; use crate::config::TcpConfig; use crate::transport::framing::read_fmp_packet; -use crate::transport::stream::{ConnId, next_conn_id, remove_own}; +use crate::transport::stream::{ + ConnId, WRITER_DRAIN_TIMEOUT, drain_writer, next_conn_id, remove_own, +}; use pool::{ConnectingEntry, ConnectingPool, ConnectionPool, Direction, TcpConnection}; use stats::TcpStats; @@ -486,21 +488,35 @@ impl TcpTransport { /// Close a specific connection asynchronously. /// - /// Removes the connection from the pool, aborts its receive task, - /// and drops the write half (sends FIN to remote). + /// Removes the connection from the pool and aborts its receive task. The + /// writer is not aborted: dropping the queue lets it finish writing the + /// frames already queued, such as a Disconnect sent just before this close, + /// and then exit, which drops the write half and sends FIN. A detached + /// timer aborts it if it is still writing after [`WRITER_DRAIN_TIMEOUT`], + /// so this call never waits on the peer. Stopping the transport, and every + /// teardown after a connection has failed, abort the writer instead and + /// discard what it had queued. pub async fn close_connection_async(&self, addr: &TransportAddr) { let mut pool = self.pool.lock().await; if let Some(conn) = pool.remove(addr) { - conn.recv_task.abort(); - conn.send_task.abort(); - match conn.direction { + let TcpConnection { + send_tx, + send_task, + recv_task, + direction, + .. + } = conn; + drop(send_tx); + recv_task.abort(); + drain_writer(send_task, WRITER_DRAIN_TIMEOUT); + match direction { Direction::Inbound => self.stats.record_pool_inbound_removed(), Direction::Outbound => self.stats.record_pool_outbound_removed(), } debug!( transport_id = %self.transport_id, remote_addr = %addr, - direction = ?conn.direction, + direction = ?direction, "TCP connection closed (close_connection)" ); } @@ -2831,4 +2847,88 @@ mod tests { transport.stop_async().await.unwrap(); } + + // ======================================================================== + // Deliberate close finishes the frames already queued + // ======================================================================== + + /// A frame queued immediately before a deliberate close must still be + /// written. + /// + /// Sending only queues the frame for the connection's writer task. A close + /// that aborts that task before it has run discards the frame, which is + /// how a Disconnect sent just before a close, or a handshake message sent + /// just before the losing side of a crossed connection is closed, never + /// reaches the peer. The second half checks the close still closes: the + /// peer sees FIN and releases its inbound slot, so a writer that never + /// exits cannot pass. + #[tokio::test] + async fn a_frame_queued_just_before_close_still_reaches_the_peer() { + let (tx1, _rx1) = packet_channel(100); + let (tx2, mut rx2) = packet_channel(100); + let mut t1 = TcpTransport::new(TransportId::new(1), None, make_outbound_config(), tx1); + let mut t2 = TcpTransport::new(TransportId::new(2), None, make_config(), tx2); + t1.start_async().await.unwrap(); + t2.start_async().await.unwrap(); + let remote = TransportAddr::from_string(&t2.local_addr().unwrap().to_string()); + let frame = build_msg1_frame(); + + // Pool the connection and let its writer go idle. + t1.send_async(&remote, &frame).await.unwrap(); + let first = timeout(Duration::from_secs(2), rx2.recv()) + .await + .expect("timeout waiting for the first frame") + .expect("packet channel closed"); + assert_eq!(first.data, frame); + + // Queue, then close with nothing in between. + t1.send_async(&remote, &frame).await.unwrap(); + t1.close_connection_async(&remote).await; + + let second = timeout(Duration::from_secs(2), rx2.recv()) + .await + .expect("a frame queued just before close was never written") + .expect("packet channel closed"); + assert_eq!(second.data, frame); + + assert!( + wait_until( + || t2.stats().snapshot().pool_inbound == 0, + Duration::from_secs(5) + ) + .await, + "the close must still end the connection once the queue is written" + ); + + t1.stop_async().await.unwrap(); + t2.stop_async().await.unwrap(); + } + + /// A deliberate close must return at once even when the writer cannot + /// finish, because the peer has stopped reading. Draining happens after + /// the close returns, never inside it. + #[tokio::test] + async fn close_does_not_wait_for_a_writer_parked_on_a_deaf_peer() { + let (tx1, _rx1) = packet_channel(100); + let mut t1 = TcpTransport::new(TransportId::new(1), None, make_outbound_config(), tx1); + t1.start_async().await.unwrap(); + let listener = capped_deaf_listener(); + let remote = TransportAddr::from_string(&listener.local_addr().unwrap().to_string()); + let frame = vec![0xAB; 1400]; + + park_tcp_writer(&t1, &remote, &frame).await; + let (_peer, _) = listener.accept().await.unwrap(); + + assert!( + timeout( + Duration::from_millis(200), + t1.close_connection_async(&remote) + ) + .await + .is_ok(), + "close waited on a writer that cannot finish" + ); + + t1.stop_async().await.unwrap(); + } } diff --git a/src/transport/tor/mod.rs b/src/transport/tor/mod.rs index b194f600..ed93dcb6 100644 --- a/src/transport/tor/mod.rs +++ b/src/transport/tor/mod.rs @@ -34,7 +34,7 @@ use crate::transport::socks5::{ Socks5Auth, Socks5Dialer, SocksTarget, poll_connecting, proxied_receive_loop, proxied_send_loop, }; -use crate::transport::stream::{ConnId, next_conn_id}; +use crate::transport::stream::{ConnId, WRITER_DRAIN_TIMEOUT, drain_writer, next_conn_id}; use crate::transport::tcp::INBOUND_FIRST_FRAME_TIMEOUT; use control::{ControlAuth, TorControlClient, TorMonitoringInfo}; use stats::TorStats; @@ -1016,12 +1016,24 @@ impl TorTransport { } /// Close a specific connection asynchronously. + /// + /// Aborts the receive task and lets the writer finish the frames already + /// queued, within [`WRITER_DRAIN_TIMEOUT`], without waiting for it. This + /// mirrors `TcpTransport::close_connection_async`. pub async fn close_connection_async(&self, addr: &TransportAddr) { let mut pool = self.pool.lock().await; if let Some(conn) = pool.remove(addr) { - conn.recv_task.abort(); - conn.send_task.abort(); - match conn.meta { + let ProxiedConnection { + send_tx, + send_task, + recv_task, + meta, + .. + } = conn; + drop(send_tx); + recv_task.abort(); + drain_writer(send_task, WRITER_DRAIN_TIMEOUT); + match meta { Direction::Inbound => self.stats.record_pool_inbound_removed(), Direction::Outbound => self.stats.record_pool_outbound_removed(), } @@ -2556,4 +2568,64 @@ mod tests { accept.abort(); drop(sock); } + + // ======================================================================== + // Deliberate close finishes the frames already queued + // ======================================================================== + + /// A frame queued immediately before a deliberate close must still reach + /// the peer through the proxy, and the close must still end the + /// connection at the far side. + #[tokio::test] + async fn tor_frame_queued_just_before_close_still_reaches_the_peer() { + let (dest_tx, mut dest_rx) = packet_channel(32); + let dest_config = TcpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }; + let mut dest = TcpTransport::new(TransportId::new(100), None, dest_config, dest_tx); + dest.start_async().await.unwrap(); + let dest_addr = dest.local_addr().unwrap(); + + let mock = MockSocks5Server::new(dest_addr).await.unwrap(); + let proxy_addr = mock.addr(); + let _proxy_handle = mock.spawn(); + + let (tor_tx, _tor_rx) = packet_channel(32); + let tor_config = TorConfig { + socks5_addr: Some(proxy_addr.to_string()), + ..Default::default() + }; + let mut tor = TorTransport::new(TransportId::new(200), None, tor_config, tor_tx); + tor.start_async().await.unwrap(); + + let target = TransportAddr::from_string(&dest_addr.to_string()); + let frame = build_msg1_frame(); + tor.send_async(&target, &frame).await.unwrap(); + let first = tokio::time::timeout(Duration::from_secs(2), dest_rx.recv()) + .await + .expect("timeout waiting for the first frame") + .expect("channel closed"); + assert_eq!(first.data, frame); + + tor.send_async(&target, &frame).await.unwrap(); + tor.close_connection_async(&target).await; + + let second = tokio::time::timeout(Duration::from_secs(2), dest_rx.recv()) + .await + .expect("a frame queued just before close was never written") + .expect("channel closed"); + assert_eq!(second.data, frame); + assert!( + wait_until( + || dest.stats().snapshot().pool_inbound == 0, + Duration::from_secs(5) + ) + .await, + "the close must still end the connection once the queue is written" + ); + + tor.stop_async().await.unwrap(); + dest.stop_async().await.unwrap(); + } }