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; +}