From 139f2af9b008e6421d06197eec6e3db82812b15a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 9 Sep 2026 18:54:00 +0000 Subject: [PATCH] docs(netmon): describe the scoped reaction, and make two tests observe it Two tests passed with the thing they exist to check removed, and seven places still described the reaction as node-wide. The exclusion test captured the peer's last-heartbeat-sent timestamp, fired a change, and asserted it had not moved. Splitting the fan-out into an attempt and a success made that assertion vacuous: delete the filter that excludes connection-oriented transports and the stream peer is selected, its send fails at the connection-readiness gate, the sent timestamp is untouched, and the assertion passes anyway. The attempt timestamp is the observation that sees the exclusion, because the fan-out writes it for every peer it picks, before any send. Asserting on that fails when the filter is removed. That test's own justification was also stale on this base: the write it called unbounded is now bounded by the writer task, so the filter is kept for a different reason, that widening the fan-out should be its own change with its own evidence. The test comment now says so, and says that widening it is the edit that would record the decision. The probe's bind address reached the sampler through three sites and no test touched any of them; substituting a null at either end left the suite green. The new test starts a UDP transport on a loopback address, pins a peer onto it with a numeric endpoint, publishes the snapshot, and asserts the probe target carries the transport's bind address. It needs no privileges and no route. Nulling either end fails it. On the prose: four places in the handler module plus two operator-facing pages still described the old node-wide reaction. One is the doc summary of a function whose own name and signature say it acts on the peers that moved. Another is an intra-doc link to a name the scoping renamed away, which resolves nowhere and which nothing catches, since there is no rustdoc gate. The pacing constant's rationale said the reaction drops every peer's socket. It drops the socket of each peer the change names; what makes the pacing argument hold is that a cleanly flapping interface is the worst case, because a medium change moves the whole table at once, so the scoped set is every peer anyway. The bound is unchanged and the argument is now exact. The test that pins the pacing carried the same sentence. The statement that nothing is left stranded by the scoping rested on one of the two ways a peer can be absent from the sample. The other is a peer whose transport address is not a numeric endpoint, which never reaches the sample at all while holding a connected socket. That is transient where the node dialled out, because the address is replaced by the observed numeric source on the first authentic frame, but it is a different argument from the one written. Which side supplies the address on an inbound peering is not established here, so the sentence does not claim the group is empty. The reference page also said the sampling cost is three syscalls per peer per sample, bounded by the peer limit. It is five, read off the sampling function rather than measured, and the bound does not hold when that limit is zero, which the configuration defines as unlimited. The upgrade note is the part with a consequence. The new cross-field check refuses startup when eight settling rounds of the debounce interval meet or exceed the liveness timeout. Detection is on by default, so a node that shortened its liveness timeout to one or two seconds for fast failover is refused after the upgrade, citing a key its operator never set. A timeout of zero is exempt. The changelog now says so, with the three ways out. --- CHANGELOG.md | 31 +++++++--- docs/reference/configuration.md | 27 +++++---- src/node/handlers/netmon.rs | 39 +++++++----- src/node/netmon/mod.rs | 13 ++-- src/node/netmon/tests.rs | 9 +-- src/node/tests/netmon.rs | 102 ++++++++++++++++++++++++++++++-- 6 files changed, 172 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36c26c38..6a560d60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,15 +69,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 medium-change detection added below, which is exactly that missing signal. Dropping the sockets is self-healing rather than disruptive: the wildcard listen socket resolves a route per packet, so sends keep working immediately, - and a correctly-bound connected socket is reinstalled on a later tick. Every - peer on a connectionless transport is also heartbeated at once, so the far - side re-pins to the new source address rather than waiting out its own - heartbeat interval. A peer on a connection-oriented transport keeps the - periodic heartbeat instead. That was because such a send awaited an unbounded - `write_all` on a stream the medium change had very likely just stranded, and - this reaction runs on the rx loop; the writer-task change below removes that - hazard, so widening the fan-out to those transports is now open work rather - than something the design forbids. Measured on a live + and a correctly-bound connected socket is reinstalled on a later tick. The + reaction is scoped to the peers the change names: only their sockets are + dropped, and each of those on a connectionless transport is heartbeated at + once, so the far side re-pins to the new source address rather than waiting + out its own heartbeat interval. A peer on a connection-oriented transport + keeps the periodic heartbeat instead. That was because such a send awaited an + unbounded `write_all` on a stream the medium change had very likely just + stranded, and this reaction runs on the rx loop; the writer-task change below + removes that hazard, so widening the fan-out to those transports is now open + work rather than something the design forbids. Measured on a live node, a WLAN/LAN switch in either direction now costs no reconnection at all — the Noise session, tree position and routes survive it. Linux and macOS (the platforms with the connected-socket fast path); elsewhere the heartbeat alone @@ -128,6 +129,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 rebind described under Fixed above. Bluetooth is not covered: an adapter's state is not an IP attachment and is invisible to this detector. + **Upgrade note: this couples a new key to one that has already shipped.** A + handover is ridden out for up to eight settling rounds of + `node.netmon.debounce_ms` before a change is reported, and if that worst case + reaches `node.link_dead_timeout_secs` the reaper tears the peering down + before the change is ever acted on, so the node refuses to start rather than + run in that shape. At the shipped defaults the margin is wide (8 × 250ms = 2s + against 30s), but detection is on by default, so **a node that shortened + `node.link_dead_timeout_secs` to 1 or 2 seconds for fast failover will be + refused at startup after the upgrade**, naming a `node.netmon.*` key its + operator never set. Raise the timeout, lower `debounce_ms` so eight rounds + stay under it, or set `node.netmon.enabled: false`. A + `node.link_dead_timeout_secs` of 0 is exempt from the check. ## [0.5.1] - 2026-09-06 diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 6e1964b9..1cb2bdee 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -218,7 +218,6 @@ keys in different blocks: **shortening `link_dead_timeout_secs` for fast failover can make an untouched `debounce_ms` illegal.** The refusal names both values and the multiplier. - Established UDP peers use a per-peer `connect()`-ed socket for the send fast path. `connect(2)` makes the kernel resolve the route once and pin the local source address to whichever interface carried it then; it never re-evaluates. @@ -227,14 +226,15 @@ from an abandoned address while the peer answers where it last heard the node the peering reports itself connected and carries nothing until `link_dead_timeout_secs` tears it down, typically 60–90s per switch. -On a detected change the node drops those sockets (the wildcard listen socket -resolves a route per packet, so sends keep working, and a correctly bound -connected socket is reinstalled on a later tick) and heartbeats every peer on a -connectionless transport at once so the far side re-pins to the new source -address. A peer reached over TCP, Tor, Nym or BLE is left to its periodic -heartbeat, since sending to it here would block the node's receive loop on a -stream the medium change has very likely just stranded. No peering is torn -down: sessions, tree positions and routes survive the switch. +On a detected change the node drops the sockets of the peers the change names +(the wildcard listen socket resolves a route per packet, so sends keep working, +and a correctly bound connected socket is reinstalled on a later tick) and +heartbeats those of them on a connectionless transport at once so the far side +re-pins to the new source address. A peer reached over TCP, Tor, Nym or BLE is +left to its periodic heartbeat, since sending to it here would block the node's +receive loop on a stream the medium change has very likely just stranded. A peer +the change does not name is left alone entirely. No peering is torn down: +sessions, tree positions and routes survive the switch. **What counts as a change.** For each peer whose transport address is a numeric IP endpoint, the node asks the kernel which local address it would use to reach @@ -275,8 +275,13 @@ 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 three syscalls per peer per sample, bounded by -`node.limits.max_peers`, with no packets sent and no name resolution. +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. +`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. Detection uses the best backend the platform has: diff --git a/src/node/handlers/netmon.rs b/src/node/handlers/netmon.rs index 42a8912b..fb77735d 100644 --- a/src/node/handlers/netmon.rs +++ b/src/node/handlers/netmon.rs @@ -36,14 +36,14 @@ //! wildcard listen socket resolves a route per packet, so sends keep working //! immediately, and `activate_connected_udp_sessions` reinstalls a //! correctly-bound connected socket on a later tick. -//! 2. **Heartbeat every peer whose send path cannot block.** The frame leaves -//! over the new path and carries the node's new source address, so the far -//! side re-pins on receipt instead of waiting out its own +//! 2. **Heartbeat each moved peer whose send path cannot block.** The frame +//! leaves over the new path and carries the node's new source address, so +//! the far side re-pins on receipt instead of waiting out its own //! `heartbeat_interval_secs`. Without it the forward direction is fixed but //! the reverse still points at the old address until the node next happens //! to send. This runs on the rx loop, and it covers the connectionless //! transports only — see -//! [`Node::heartbeat_all_peers_after_net_change`] for what a peer on a +//! [`Node::heartbeat_moved_peers_after_net_change`] for what a peer on a //! connection-oriented transport gets instead, and for why that filter has //! outlived the reason it was written for. //! @@ -65,10 +65,19 @@ //! Scoped, the only peer in the set is the roamer itself — whose connected //! socket `dataplane::encrypted` has already cleared on the address change. //! -//! Nothing is left stranded by the narrowing, because a peer absent from the -//! set is one whose local source address the kernel still resolves to the same -//! place. That is the whole content of the fingerprint: a peer that did not -//! move is a peer whose socket is not stale. +//! A peer is absent from that set for one of two reasons. The first is the +//! one the narrowing rests on: its local source address still resolves to the +//! same place, which is the whole content of the fingerprint — a peer that did +//! not move is a peer whose socket is not stale. The second is that it never +//! reached the sample. `PeerRow::probe_target` parses the peer's current +//! address, so a peer still carrying the hostname it was configured with is +//! `None` there and is skipped while any `connect()`-ed socket it holds stays +//! pinned; the node-wide reaction repaired that peer as collateral and this one +//! does not. Where the node dialled out, that is transient — +//! `set_current_addr` replaces the configured string with the observed numeric +//! source on the first authentic frame. Which side supplies `current_addr` on +//! an inbound peering is not established here, so the second group is not +//! claimed to be empty in general. use std::time::Instant; @@ -101,8 +110,8 @@ impl Node { ); } - /// Drop every per-peer `connect()`-ed UDP socket, returning how many were - /// released. + /// Drop the per-peer `connect()`-ed UDP socket of each peer that moved, + /// returning how many were released. /// /// See the module docs for why they are stale: `connect(2)` pins the local /// source address to the interface that carried the route at connect time, @@ -130,11 +139,11 @@ impl Node { 0 } - /// Send one heartbeat to every peer whose send path cannot block, so each - /// learns the node's new source address in one RTT rather than at the next - /// due interval. Returns how many sends actually succeeded, which is what - /// the operator log reports — a count of peers *selected* would read the - /// same whether every frame left or none did, and a medium change is + /// Send one heartbeat to each moved peer whose send path cannot block, so + /// each learns the node's new source address in one RTT rather than at the + /// next due interval. Returns how many sends actually succeeded, which is + /// what the operator log reports — a count of peers *selected* would read + /// the same whether every frame left or none did, and a medium change is /// exactly when sends start failing. /// /// The filter was written for a hazard that no longer exists, and it is diff --git a/src/node/netmon/mod.rs b/src/node/netmon/mod.rs index ec449a1e..0e45451c 100644 --- a/src/node/netmon/mod.rs +++ b/src/node/netmon/mod.rs @@ -169,11 +169,14 @@ pub(crate) const MAX_DEBOUNCE_ROUNDS: u32 = 8; /// Minimum spacing between two reported changes. /// -/// The reaction is not free: it drops every peer's connected UDP socket (each -/// carrying a drain thread) and sends a heartbeat per peer. An interface that -/// flaps cleanly — settling between each transition, so the debounce reports -/// each one — could otherwise drive that several times a second across up to -/// `node.limits.max_peers` peers, which is thread churn rather than recovery. +/// The reaction is not free: it drops the connected UDP socket of each peer +/// the change names (each carrying a drain thread) and sends that peer a +/// heartbeat. An interface flapping cleanly is the worst case for that, because +/// a medium change moves the whole table at once, so the set the reaction is +/// scoped to is every peer: settling between each transition, so the debounce +/// reports each one, could otherwise drive it several times a second across up +/// to `node.limits.max_peers` peers, which is thread churn rather than +/// recovery. /// /// A genuine change is delayed by at most this long, against a /// `link_dead_timeout_secs` measured in tens of seconds, so the trade is diff --git a/src/node/netmon/tests.rs b/src/node/netmon/tests.rs index ae90cb42..30134a3a 100644 --- a/src/node/netmon/tests.rs +++ b/src/node/netmon/tests.rs @@ -720,10 +720,11 @@ async fn a_route_change_alone_reaches_the_watcher() { .expect("a route change must reach the watcher well inside 5s"); } -/// Reactions are paced. Dropping every peer's connected socket and heartbeating -/// each of them is not free, so an interface that flaps cleanly — settling -/// between transitions, which defeats the debounce — must not drive that -/// several times a second across the whole peer set. +/// Reactions are paced. Dropping a moved peer's connected socket and +/// heartbeating it is not free, and an interface flapping cleanly is the worst +/// case for that, because a medium change moves the whole table at once. So an +/// interface settling between transitions, which defeats the debounce, must not +/// drive that several times a second across the whole peer set. #[tokio::test(start_paused = true)] async fn reports_are_spaced_out_under_clean_flapping() { let a = all_from(&[peer(1)], Some(v4(192, 168, 1, 10))); diff --git a/src/node/tests/netmon.rs b/src/node/tests/netmon.rs index 0bd9cc6d..2ba23b27 100644 --- a/src/node/tests/netmon.rs +++ b/src/node/tests/netmon.rs @@ -255,11 +255,23 @@ async fn every_peer_is_heartbeated_so_the_far_side_re_pins() { /// A peer on a connection-oriented transport is deliberately left out of the /// immediate fan-out. /// -/// Its send would await `write_all` on a stream the medium change has very -/// likely just stranded — unbounded, and on the rx loop, where it would hold -/// every other arm of the select behind it. Such a peer keeps the periodic -/// heartbeat it had before this detector existed. If this ever starts passing -/// because the peer *was* heartbeated, the rx loop has a new way to stall. +/// The hazard the filter was written for is gone: every connection-oriented +/// send now enqueues onto its connection's bounded queue and returns, so none +/// of them can await the wire from the rx loop any more. The exclusion is kept +/// anyway, so that widening the fan-out is its own change with its own +/// evidence rather than a side effect of the one that bounded the write. Such +/// a peer keeps the periodic heartbeat it had before this detector existed. +/// +/// **So this test guards a deliberate boundary, not a stall.** If the fan-out +/// is widened on purpose, this test is the thing to change, and changing it is +/// how that decision gets recorded. +/// +/// The attempt stamp is the observation that sees the exclusion. The fan-out +/// records it for every peer it picks, before the send, and records the sent +/// stamp only for a send that returned. A connection-oriented send fails at +/// the readiness gate, so the sent stamp would sit still either way — whether +/// the peer was excluded or picked and failed — and on its own it cannot tell +/// the two apart. #[tokio::test] async fn a_peer_on_a_connection_oriented_transport_is_left_to_the_periodic_heartbeat() { let mut nodes = run_tree_test(2, &[(0, 1)], false).await; @@ -309,6 +321,15 @@ async fn a_peer_on_a_connection_oriented_transport_is_left_to_the_periodic_heart before, after, "a connection-oriented peer must not be heartbeated from the rx loop" ); + assert!( + nodes[0] + .node + .get_peer(&addr_1) + .unwrap() + .last_heartbeat_attempt() + .is_none(), + "a connection-oriented peer must not even be attempted from the rx loop" + ); cleanup_nodes(&mut nodes).await; } @@ -572,3 +593,74 @@ async fn a_heartbeat_that_failed_is_not_counted_and_does_not_suppress_the_next() cleanup_nodes(&mut nodes).await; } + +/// The transport's bind address has to reach the probe, or the detector asks +/// the routing table a different question than the send path answers. +/// +/// `open_connected_fd` binds `transports.udp.bind_addr` verbatim before it +/// connects, so under a non-wildcard bind the source is pinned to that address +/// whatever the route says. A probe left unconstrained takes the kernel's +/// choice instead, the two answers differ permanently, and every first-seen +/// peer reports a move that never happened. Nothing renders the field, so +/// substituting `None` at either the publish or the read leaves the rest of +/// the suite green. +/// +/// The transport is started on `127.0.0.1:0`: `start_async` fills `local_addr` +/// from the socket the kernel actually bound, which is what the publish reads +/// and what the `!is_unspecified()` filter admits. No privileges are needed. +#[tokio::test] +async fn a_transports_bind_address_reaches_the_probe_target() { + let mut nodes = run_tree_test(2, &[(0, 1)], false).await; + verify_tree_convergence(&nodes); + + let addr_1 = *nodes[1].node.node_addr(); + + let bound_id = TransportId::new(99); + let (tx, _rx) = packet_channel(64); + let mut udp = crate::transport::udp::UdpTransport::new( + bound_id, + None, + crate::config::UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }, + tx, + ); + udp.start_async() + .await + .expect("bind a UDP socket on loopback"); + assert_eq!( + udp.local_addr().map(|sa| sa.ip()), + Some(std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST)), + "precondition: the transport is bound to a real address, not the wildcard" + ); + nodes[0] + .node + .transports + .insert(bound_id, TransportHandle::Udp(udp)); + + // The harness peers sit on a synthetic `loopback:1` address, which is + // correctly not probeable. Re-pin onto the bound transport with a numeric + // endpoint so the row reaches the probe at all. + nodes[0] + .node + .peers + .get_mut(&addr_1) + .expect("peer 1 is established") + .set_current_addr(bound_id, TransportAddr::from_string("10.0.0.2:2121")); + + // The snapshot is published from the tick, which is its only writer. + nodes[0].node.record_stats_history(); + let snapshot = nodes[0].node.entities_snapshot.load_full(); + let target = crate::node::netmon::probe_targets(&snapshot) + .into_iter() + .find(|t| t.peer == addr_1) + .expect("a peer with a numeric endpoint must be probeable"); + assert_eq!( + target.bind, + Some(std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST)), + "the transport's bind address must reach the probe target, not stop at the row" + ); + + cleanup_nodes(&mut nodes).await; +}