diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eb07c40..4a8d7fd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -237,6 +237,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Node lifecycle +- Losing an interface no longer leaves its peers in the routing table. The + peers stayed in the registry, the routes through them stayed selectable, and + the node kept advertising reachability it no longer had — so transit traffic + was dropped in silence and other nodes kept routing toward this one for those + destinations, until the liveness reaper noticed up to + `node.link_dead_timeout_secs` later. Measured on real hardware, a detached + dongle took the node's parent with it and no new parent was chosen for + twenty-seven seconds, with four alternative peers available the whole time. A + transport's detach edge now withdraws every peer whose active link runs over + it, on the same path the liveness reaper uses, so sessions, path MTU, session + indices, the link, the control machine, tree cleanup and re-announce, and + bloom withdrawal unwind exactly as they already did. It is not filtered by + `optional`: whether an interface's absence is normal is a statement about + node health, and says nothing about whether the routes over it still work. + The trade is that an absence shorter than the dead timeout that then recovers + now costs a re-peer where it previously cost nothing, accepted because + black-holing is silent, poisons other nodes' routing and takes the full + timeout to clear, where a re-peer is bounded, visible and self-healing. + +- A local interface flap during a handshake is no longer charged to the remote. + A msg2 send refused because the interface is absent or mid-rebind was treated + as a failed handshake: the link was removed, the reverse-address entry + dropped, the session index freed, the control machine torn down, and the + whole thing recorded under the reject reason that means "the remote sent + something invalid", which is what an operator reading the rejects would have + concluded. The initiator meanwhile resent msg1 into a link that no longer + existed and had to rebuild from nothing. A transport error the daemon is + already working to clear now leaves the half-built link exactly where it is + for that resend to land on; only a terminal error still tears down, and a + link nobody resends to is reaped at `node.rate_limit.handshake_timeout_secs` + like every other abandoned handshake. The rekey msg1 send site keeps its + teardown, which was already benign, and stops reporting a local self-clearing + condition at `warn`. + - A heartbeat whose send failed no longer counts as one that was delivered. The peer's "last heartbeat" timestamp was stamped before the send and left alone whatever came back, so a failure suppressed the next attempt for a diff --git a/docs/design/fips-transport-layer.md b/docs/design/fips-transport-layer.md index a4412f74..0fff5707 100644 --- a/docs/design/fips-transport-layer.md +++ b/docs/design/fips-transport-layer.md @@ -1208,14 +1208,21 @@ the same socket. The supervisor learns about presence (`Event::ChildAbsent` / `Event::ChildPresent`) and republishes health; it does not drive rebinding. -Peer state needs no separate grace period. A send over an absent interface -returns `InterfaceUnavailable` and the peer entry survives untouched, so peers -are already held across a detach and resume when the interface returns; the -liveness reaper is the effective linger bound. A recreated mesh interface -comes back with the same MAC (it is derived from the phy) and Noise sessions -are keyed on the remote peer, so a local rebind is invisible to peers. A -dedicated linger timer would be a second answer to a question already -answered. +Peer state gets no grace period, and needs none. A transport's detach edge +withdraws every peer whose active link runs over it, on the same path the +liveness reaper uses, so those peers and the routes through them are gone at +the edge rather than up to `link_dead_timeout_secs` later — there is nothing +left for a linger timer to bound. A send over an absent interface still +returns `InterfaceUnavailable`, and a *half-built* link is still held on that +error, because the binder is already working to bring the interface back and +the initiator's resend has somewhere to land; an established peer is not held. +The trade is deliberate: an absence shorter than the dead timeout that then +recovers now costs a re-peer where it previously cost nothing, and that is +accepted because black-holing is silent, poisons other nodes' routing and +takes the full timeout to clear, where a re-peer is bounded, visible and +self-healing. A recreated mesh interface comes back with the same MAC (it is +derived from the phy), so the local address peers hold is unchanged across the +rebind. ### Logging diff --git a/src/node/dataplane/peer_actions.rs b/src/node/dataplane/peer_actions.rs index ca065a1f..237933a1 100644 --- a/src/node/dataplane/peer_actions.rs +++ b/src/node/dataplane/peer_actions.rs @@ -187,6 +187,31 @@ impl Node { None => None, }; if let Some(e) = send_err { + // A transient refusal is not a failed handshake. The + // interface under this transport is absent or + // mid-rebind, and the binder is already working to + // bring it back — so the half-built link is left + // exactly as it is for the initiator's msg1 resend to + // land on, rather than being torn down and rebuilt. + // + // Tearing down here charged a *local* interface flap + // to the remote: the reject counter it recorded means + // "the peer sent something invalid", which is a + // different thing entirely and one an operator reads + // as the peer's fault. + // + // Nothing leaks by staying. An initiator that never + // resends leaves a stale connection, which + // `check_timeouts` reaps at `handshake_timeout_secs` + // exactly as it reaps every other abandoned handshake. + if e.is_transient() { + debug!( + link_id = %link, + error = %e, + "Deferred msg2: the transport is between interfaces" + ); + return; + } // Restored pre-refactor msg2-send-failure warn! // (`handle_msg1` L665): the send error text is surfaced // at the executor point where the failure is now handled. diff --git a/src/node/dataplane/rx_loop.rs b/src/node/dataplane/rx_loop.rs index c81b31ab..bb26d78a 100644 --- a/src/node/dataplane/rx_loop.rs +++ b/src/node/dataplane/rx_loop.rs @@ -399,6 +399,28 @@ impl Node { // binds can be the narrow one, and one that detaches // can be the reason the clamp was tight. self.refresh_tun_mss_ceiling(); + + // A peer reachable only through an interface that has + // gone is not reachable. Withdraw it now rather than + // leaving the liveness reaper to notice up to + // `link_dead_timeout_secs` later, during which this + // node both drops transit traffic in silence and keeps + // advertising reachability it does not have. + // + // Not policy-filtered: whether an interface's absence + // is normal is a statement about node *health*, not + // about whether the routes over it still work. + if !edge.present { + let reaped = + self.reap_peers_on_transport(edge.transport_id).await; + if reaped > 0 { + info!( + transport_id = %edge.transport_id, + peers = reaped, + "Withdrew peers whose interface went away" + ); + } + } } } Some(ipv6_packet) = tun_outbound_rx.recv() => { diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 5139e536..2f5c5803 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -933,8 +933,14 @@ impl Node { // Post-`Promoted` shell tail (byte-identical to the pre-refactor // Promoted arm), reached only when promotion succeeded (machine now - // Established); a send/promote failure removed the machine and - // already cleaned up. + // Established). Three outcomes reach this line: a terminal send + // failure or a promote failure removed the machine, leaving it + // absent; a TRANSIENT msg2 send failure aborted the queue before + // `PromoteToActive` ran and deliberately left the half-built leg in + // place, so the machine is still registered at + // `Handshaking{ReceivedMsg1}` (`peer_actions.rs`, the + // `is_transient` branch); or promotion succeeded. The tail below is + // gated on `Established`, so only the third runs it. // // DEFENSIVE CROSS-CONNECTION: the machine's // `PromotionResolved{CrossConnectionWon/Lost}` follow-ups run the @@ -944,11 +950,16 @@ impl Node { // path: Phase-1 `remove_active_peer` removed `peers[addr]`, so // `promote_connection` returns `Promoted`. The full cross-connection // link surgery is wired later; the - // debug_assert below catches any regression that reaches a non- - // Established, non-absent state. + // debug_assert below catches any regression that reaches a state + // other than those three. debug_assert!(matches!( self.peer_machines.get(&link_id).map(|m| m.state()), - Some(PeerState::Established { .. }) | None + Some(PeerState::Established { .. }) + | Some(PeerState::Handshaking { + phase: HandshakePhase::ReceivedMsg1, + .. + }) + | None )); if matches!( self.peer_machines.get(&link_id).map(|m| m.state()), @@ -1061,9 +1072,13 @@ impl Node { // sends msg2 (bytes identical to `wire_msg2`), promotes via // `promote_connection`, feeds PromotionResolved back, and runs the // inert RegisterDecryptSession (register stays in - // `promote_connection`). Its send-failure / promote-failure arms - // run the pre-refactor cleanup and remove the machine, leaving it - // absent (not Established). + // `promote_connection`). Its TERMINAL send-failure and its + // promote-failure arms run the pre-refactor cleanup and remove the + // machine, leaving it absent (not Established); its TRANSIENT + // send-failure arm aborts the queue before `PromoteToActive` and + // leaves the machine registered at `Handshaking{ReceivedMsg1}` for + // the initiator's msg1 resend to land on. The `Established` gate + // below covers all three. let ambient = PeerActionCtx { verified_identity: peer_identity, transport_id: packet.transport_id, @@ -1078,8 +1093,10 @@ impl Node { // Post-`Promoted` shell tail (byte-identical to the pre-refactor // Promoted arm), reached only when promotion succeeded (the machine - // is now Established); a send/promote failure removed the machine - // and already cleaned up. + // is now Established); a terminal send failure or a promote failure + // removed the machine and already cleaned up, and a transient send + // failure left it registered at `Handshaking{ReceivedMsg1}` — which + // is what the gate below is for. if matches!( self.peer_machines.get(&link_id).map(|m| m.state()), Some(PeerState::Established { .. }) diff --git a/src/node/handlers/mmp.rs b/src/node/handlers/mmp.rs index 00d9a112..43c0af8a 100644 --- a/src/node/handlers/mmp.rs +++ b/src/node/handlers/mmp.rs @@ -601,10 +601,71 @@ impl Node { } } - /// Route a link-dead liveness reap through the peer machine + executor. - /// Mirrors [`route_rekey_cadence`](Node::route_rekey_cadence): the - /// shell already decided (the tick sweep's `plan_heartbeats` batch emitted - /// this `ReapPeer` in phase order), so the machine only CONSUMES the decision + /// Reap every active peer reachable only through `transport_id`. + /// + /// Called on a transport's detach edge. Until this existed, losing an + /// interface withdrew nothing: the peers stayed in the registry, the + /// routes through them stayed selectable, and this node kept advertising + /// reachability it no longer had — so transit traffic was dropped in + /// silence, and other nodes kept routing toward us for those destinations, + /// until the liveness reaper noticed up to `link_dead_timeout_secs` later. + /// Measured on real hardware that was 27 seconds of routing through a link + /// that had already gone, with four alternative peers available the whole + /// time. + /// + /// The detach edge is both earlier and more certain than inactivity, so it + /// is the better trigger. This routes through the same + /// [`Self::route_link_dead`] the liveness reaper uses rather than + /// open-coding a second teardown — every consequence of losing a peer + /// (sessions, path MTU, session indices, the link, the control machine, + /// tree cleanup and re-announce, bloom withdrawal) already hangs off that + /// one path, and a parallel one would drift from it. + /// + /// Deliberately undamped. A flapping interface cannot drive a reap storm + /// through here, because `ChurnGuard` stops publishing presence edges + /// after three short-lived bindings and does not resume until one lasts — + /// so the edges this reacts to are already rate-limited at the source, and + /// a second damper here would only add a way for the two to disagree. + /// + /// Returns how many peers were reaped. + pub(in crate::node) async fn reap_peers_on_transport( + &mut self, + transport_id: TransportId, + ) -> usize { + let doomed: Vec = self + .peers + .iter() + .filter(|(_, peer)| peer.transport_id() == Some(transport_id)) + .map(|(node_addr, _)| *node_addr) + .collect(); + + if doomed.is_empty() { + return 0; + } + + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0); + + let reaped = doomed.len(); + for node_addr in doomed { + debug!( + peer = %self.peer_display_name(&node_addr), + %transport_id, + "Removing peer: its interface went away" + ); + self.route_link_dead(node_addr, now_ms).await; + } + reaped + } + + /// Route a link-dead reap through the peer machine + executor. Two callers + /// decide: the tick sweep's `plan_heartbeats` batch emits a `ReapPeer` for a + /// peer that has gone quiet, and [`Self::reap_peers_on_transport`] withdraws + /// a transport's peers when its interface goes away. Mirrors + /// [`route_rekey_cadence`](Node::route_rekey_cadence): the shell has already + /// decided by the time this runs, so the machine only CONSUMES the decision /// via [`PeerEvent::LinkDeadSuspected`]. The resulting executor arms /// (`InvalidateSendState` → `remove_active_peer`, `ReportLost` → /// `note_link_dead`) reproduce the pre-refactor inline reap body exactly. @@ -614,9 +675,10 @@ impl Node { /// no-op, so we return; if the machine is absent (which should be impossible) /// we fall back to the byte-identical inline body under a `debug_assert`. /// - /// `now_ms` is the sweep's hoisted wall-clock ms (the same value the old reap - /// fed `note_link_dead`); it flows to the executor `ReportLost` arm via - /// `ambient.now_ms`. + /// `now_ms` is the caller's hoisted wall-clock ms (the same value the old + /// reap fed `note_link_dead`); it flows to the executor `ReportLost` arm via + /// `ambient.now_ms`. Both callers hoist it once per batch, so every peer + /// removed in one pass carries the same instant. async fn route_link_dead(&mut self, node_addr: NodeAddr, now_ms: u64) { let link = match self.peers.get(&node_addr) { Some(peer) => peer.link_id(), diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index 0c938d54..efd2f512 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -373,11 +373,26 @@ impl Node { ); } Err(e) => { - warn!( - peer = %self.peer_display_name(node_addr), - error = %e, - "Failed to send rekey msg1" - ); + // The teardown here is already benign — this returns before + // `set_rekey_state`, so the cycle simply does not start and + // is retried when rekey next comes due, with nothing torn + // down and nothing charged to the peer. Only the severity + // is wrong for a transport between interfaces, which is a + // local and self-clearing condition the presence machine + // has already reported. + if e.is_transient() { + debug!( + peer = %self.peer_display_name(node_addr), + error = %e, + "Deferred rekey msg1: the transport is between interfaces" + ); + } else { + warn!( + peer = %self.peer_display_name(node_addr), + error = %e, + "Failed to send rekey msg1" + ); + } let _ = self.index_allocator.free(our_index); return; } diff --git a/src/node/mod.rs b/src/node/mod.rs index 893bffeb..8fb65d94 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -138,6 +138,17 @@ pub enum NodeError { #[error("send failed to {node_addr}: {reason}")] SendFailed { node_addr: NodeAddr, reason: String }, + /// A send refused by a condition that is expected to clear on its own. + /// + /// Distinct from [`Self::SendFailed`] because the right response differs: + /// the state built around the send — a half-finished handshake, a route, + /// a queued packet — is worth keeping across a transient refusal and + /// worth tearing down after a terminal one. Carries the transport's own + /// classification ([`TransportError::is_transient`]) rather than a + /// re-derivation of it. + #[error("send to {node_addr} unavailable: {reason}")] + SendUnavailable { node_addr: NodeAddr, reason: String }, + #[error("mtu exceeded forwarding to {node_addr}: packet {packet_size} > mtu {mtu}")] MtuExceeded { node_addr: NodeAddr, @@ -170,6 +181,17 @@ pub enum NodeError { NoOperationalTransports, } +impl NodeError { + /// Whether this failure is expected to clear on its own. + /// + /// Mirrors [`TransportError::is_transient`] across the node boundary, so + /// a caller holding a `NodeError` can ask the same question a caller + /// holding a `TransportError` can, and get the same answer. + pub fn is_transient(&self) -> bool { + matches!(self, Self::SendUnavailable { .. }) + } +} + /// Node operational state. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum NodeState { @@ -3821,6 +3843,14 @@ impl Node { packet_size, mtu, }, + // Preserve the transport's own classification instead of + // flattening every non-MTU failure into one string. A caller + // that wants to keep its half-built state across an interface + // flap can only do that if the distinction survives to it. + other if other.is_transient() => NodeError::SendUnavailable { + node_addr: *node_addr, + reason: format!("transport send: {}", other), + }, other => NodeError::SendFailed { node_addr: *node_addr, reason: format!("transport send: {}", other), diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 32c3b7bb..b02f0d1a 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -2215,3 +2215,181 @@ async fn a_first_epoch_change_against_a_silent_peering_still_restarts_it() { "the replacement must carry the epoch the msg1 announced" ); } + +/// A transient send failure during msg2 leaves the half-built link alone. +/// +/// The interface under the transport is absent or mid-rebind, and the binder +/// is already working to bring it back. Tearing the link down here meant the +/// initiator's msg1 resend had nothing to land on, and — worse — recorded +/// `HandshakeReject::BadState`, a counter whose whole meaning is "the remote +/// sent something invalid". A local interface flap is not the remote's fault, +/// and an operator reading that counter would conclude it was. +/// +/// Nothing leaks by staying: an initiator that never resends leaves a stale +/// connection, which `check_timeouts` reaps at `handshake_timeout_secs` like +/// any other abandoned handshake. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[tokio::test] +async fn a_transient_msg2_failure_keeps_the_link_for_the_retry() { + use crate::config::EthernetConfig; + use crate::proto::fmp::wire::build_msg1; + use crate::transport::TransportHandle; + use crate::transport::ethernet::EthernetTransport; + + let mut node_b = make_node(); + let node_a = make_node(); + let transport_id = TransportId::new(1); + node_b.supervisor.state = NodeState::Running; + + // An interface no host has, so every send off this transport reports + // `InterfaceUnavailable` — the real error, from the real code path, + // rather than a stub that merely returns something transient. + let config = EthernetConfig { + interface: "fips-absent-x0".to_string(), + ethertype: None, + mtu: None, + recv_buf_size: None, + send_buf_size: None, + listen: Some(true), + announce: Some(false), + auto_connect: None, + accept_connections: Some(true), + beacon_interval_secs: None, + optional: Some(true), + }; + let (tx, _rx) = crate::transport::packet_channel(8); + let mut eth = EthernetTransport::new(transport_id, Some("lab".into()), config, tx); + eth.start_async() + .await + .expect("an absent interface is not a start failure"); + node_b + .transports + .insert(transport_id, TransportHandle::Ethernet(eth)); + + let rejects_before = node_b.stats().handshake.snapshot().bad_state; + + let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()); + let mut conn_a = outbound_leg(LinkId::new(1), peer_b_identity, 1000); + let noise_msg1 = conn_a + .start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000) + .unwrap(); + let wire_msg1 = build_msg1(SessionIndex::new(7), &noise_msg1); + let packet = ReceivedPacket::with_timestamp( + transport_id, + TransportAddr::from_string("aa:bb:cc:dd:ee:ff"), + wire_msg1, + 1000, + ); + + node_b.handle_msg1(packet).await; + + assert_eq!( + node_b.link_count(), + 1, + "the link must survive a transport that is merely between interfaces" + ); + assert_eq!( + node_b.stats().handshake.snapshot().bad_state, + rejects_before, + "a local interface flap must not be recorded as the peer's misbehaviour" + ); +} + +/// A transient msg2 failure on the RESTART path leaves the fresh leg pending. +/// +/// The epoch-mismatch restart arm tears the stale peering down in Phase 1 and +/// then sends msg2 for the fresh leg. When the interface under the transport is +/// absent, that send is deferred rather than failed, so `PromoteToActive` never +/// runs and the machine stays registered at `Handshaking{ReceivedMsg1}` — a +/// third outcome the arm's post-promote `debug_assert!` did not admit. Without +/// that assertion widened, this test panics at the assertion in any build with +/// debug assertions on, which is every `cargo test` and every `cargo build` +/// without `--release`. +/// +/// It also pins the deferral's own contract on this arm: the half-built link +/// survives for the initiator's msg1 resend, and a local interface flap is not +/// charged to the peer as `HandshakeReject::BadState`. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[tokio::test] +async fn a_transient_msg2_failure_on_the_restart_path_leaves_the_fresh_leg_pending() { + use crate::config::EthernetConfig; + use crate::peer::machine::{HandshakePhase, PeerState}; + use crate::transport::TransportHandle; + use crate::transport::ethernet::EthernetTransport; + + let transport_id = TransportId::new(1); + let mut node = make_node(); + let initiator = make_node(); + let initiator_addr = node_addr_of(&initiator); + let source_addr = TransportAddr::from_string("aa:bb:cc:dd:ee:ff"); + node.supervisor.state = NodeState::Running; + + // An interface no host has, so every send off this transport reports + // `InterfaceUnavailable` — the real error from the real code path. + let config = EthernetConfig { + interface: "fips-absent-x0".to_string(), + ethertype: None, + mtu: None, + recv_buf_size: None, + send_buf_size: None, + listen: Some(true), + announce: Some(false), + auto_connect: None, + accept_connections: Some(true), + beacon_interval_secs: None, + optional: Some(true), + }; + let (tx, _rx) = crate::transport::packet_channel(8); + let mut eth = EthernetTransport::new(transport_id, Some("lab".into()), config, tx); + eth.start_async() + .await + .expect("an absent interface is not a start failure"); + node.transports + .insert(transport_id, TransportHandle::Ethernet(eth)); + + let stale_link = install_peering_at_a_different_epoch( + &mut node, + &initiator, + transport_id, + &source_addr, + IDLE_SECS, + ); + let rejects_before = node.stats().handshake.snapshot().bad_state; + + node.handle_msg1(ReceivedPacket::with_timestamp( + transport_id, + source_addr.clone(), + genuine_msg1(&initiator, &node), + Node::now_ms(), + )) + .await; + + let fresh_link = node + .addr_to_link + .get(&(transport_id, source_addr.clone())) + .copied() + .expect("the fresh leg's reverse map entry must survive the deferral"); + assert_ne!( + fresh_link, stale_link, + "the restart must have replaced the stale link, not kept it" + ); + assert!( + matches!( + node.peer_machines.get(&fresh_link).map(|m| m.state()), + Some(PeerState::Handshaking { + phase: HandshakePhase::ReceivedMsg1, + .. + }) + ), + "a deferred msg2 must leave the machine pending, not promoted and not gone" + ); + assert!( + node.get_peer(&initiator_addr).is_none(), + "no promotion can have happened: PromoteToActive never ran" + ); + assert_eq!( + node.stats().handshake.snapshot().bad_state, + rejects_before, + "a local interface flap must not be recorded as the peer's misbehaviour" + ); +} diff --git a/src/node/tests/heartbeat.rs b/src/node/tests/heartbeat.rs index 8d66e425..d0b280dc 100644 --- a/src/node/tests/heartbeat.rs +++ b/src/node/tests/heartbeat.rs @@ -338,3 +338,74 @@ async fn a_failing_peer_is_retried_after_the_gap_and_not_before() { cleanup_nodes(&mut nodes).await; } + +// --------------------------------------------------------------------------- +// Reaping peers when their interface goes away +// +// The detach edge is both earlier and more certain than inactivity, so it is +// the better trigger for withdrawing what the interface carried. These drive +// the same real two-node peering the liveness tests use, because a peer only +// reaches the established context the reap acts on by actually peering. +// --------------------------------------------------------------------------- + +/// A peer reachable only through an interface that has gone is withdrawn on +/// the detach edge, without waiting out `link_dead_timeout_secs`. +/// +/// Note what is *not* set here: the link-dead timeout keeps its default, and +/// no time is advanced. The peer is live by every liveness measure and is +/// still withdrawn, because the transport under it is gone — which is the +/// whole distinction this adds. +#[tokio::test] +async fn a_detached_transport_withdraws_the_peers_that_needed_it() { + let mut nodes = run_tree_test(2, &[(0, 1)], false).await; + verify_tree_convergence(&nodes); + + let addr_1 = *nodes[1].node.node_addr(); + let transport_id = nodes[0] + .node + .get_peer(&addr_1) + .expect("peer present") + .transport_id() + .expect("an established peer names its transport"); + + let reaped = nodes[0].node.reap_peers_on_transport(transport_id).await; + + assert_eq!(reaped, 1); + assert!( + nodes[0].node.get_peer(&addr_1).is_none(), + "a peer must not outlive the interface it was reachable through" + ); + + cleanup_nodes(&mut nodes).await; +} + +/// The reap is scoped to the transport that detached. +/// +/// The failure this guards is the one that would make the feature worse than +/// the defect: an interface going away must not withdraw the peers that were +/// never reachable through it, which on a mesh router is most of them. +#[tokio::test] +async fn a_detached_transport_leaves_other_transports_peers_alone() { + let mut nodes = run_tree_test(2, &[(0, 1)], false).await; + verify_tree_convergence(&nodes); + + let addr_1 = *nodes[1].node.node_addr(); + let peer_transport = nodes[0] + .node + .get_peer(&addr_1) + .expect("peer present") + .transport_id() + .expect("an established peer names its transport"); + + // A transport this peer was never reachable through. + let unrelated = TransportId::new(peer_transport.as_u32() + 100); + let reaped = nodes[0].node.reap_peers_on_transport(unrelated).await; + + assert_eq!(reaped, 0, "an unrelated transport withdraws nothing"); + assert!( + nodes[0].node.get_peer(&addr_1).is_some(), + "a peer on a healthy transport must survive another one detaching" + ); + + cleanup_nodes(&mut nodes).await; +} diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 088c959c..3ad643dc 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -229,6 +229,49 @@ pub enum TransportError { Io(#[from] std::io::Error), } +impl TransportError { + /// Whether this failure is expected to clear on its own. + /// + /// The distinction callers need is not *what* went wrong but whether + /// waiting fixes it. A transient failure means the operation was refused + /// by a condition the daemon is already working to resolve, so the state + /// built up around it — a half-finished handshake, a route, a queued + /// packet — is worth keeping. A terminal one means the state is worth + /// tearing down. + /// + /// This lives here, on the error, rather than being re-derived at each + /// call site: `InterfaceUnavailable` used to be flattened into a + /// formatted string on its way out of the transport layer, so every + /// caller downstream saw a generic send failure and could only treat a + /// two-second interface flap exactly as it treated a permanent fault. + /// + /// Deliberately narrow. [`Self::Timeout`] and [`Self::ConnectionRefused`] + /// are *not* transient here: they describe a remote that did not answer, + /// which is a statement about the peer rather than about this node's + /// ability to transmit, and the existing retry paths for them already sit + /// at a different layer. + pub fn is_transient(&self) -> bool { + match self { + // The interface is absent or mid-rebind. The binder is polling for + // it and will bind it the moment it returns. + Self::InterfaceUnavailable { .. } => true, + Self::NotStarted + | Self::AlreadyStarted + | Self::StartFailed(_) + | Self::ShutdownFailed(_) + | Self::LinkFailed(_) + | Self::SendFailed(_) + | Self::RecvFailed(_) + | Self::InvalidAddress(_) + | Self::MtuExceeded { .. } + | Self::Timeout + | Self::ConnectionRefused + | Self::NotSupported(_) + | Self::Io(_) => false, + } + } +} + // ============================================================================ // Transport Type Metadata // ============================================================================ @@ -1687,4 +1730,39 @@ mod tests { assert_eq!(handle.link_mtu(&addr), expected_mtu); assert_eq!(handle.link_mtu(&addr), handle.mtu()); } + + #[test] + fn only_an_absent_interface_classifies_as_transient() { + // The whole point of the classification is that it is narrow. An + // interface the binder is already polling for will come back; nothing + // else on this list resolves itself by waiting, and treating one of + // them as transient would mean holding state open for a fault that is + // never going to clear. + assert!( + TransportError::InterfaceUnavailable { + interface: "eth0".into() + } + .is_transient() + ); + + for terminal in [ + TransportError::NotStarted, + TransportError::AlreadyStarted, + TransportError::StartFailed("no CAP_NET_RAW".into()), + TransportError::SendFailed("ENOBUFS".into()), + TransportError::MtuExceeded { + packet_size: 2000, + mtu: 1500, + }, + // Deliberately terminal: both describe a remote that did not + // answer, not this node's inability to transmit. + TransportError::Timeout, + TransportError::ConnectionRefused, + ] { + assert!( + !terminal.is_transient(), + "{terminal:?} must not be classified transient" + ); + } + } } diff --git a/testing/iface-binding/test.sh b/testing/iface-binding/test.sh index f0a94405..eccf4cce 100755 --- a/testing/iface-binding/test.sh +++ b/testing/iface-binding/test.sh @@ -200,6 +200,21 @@ wait_for() { return 1 } +# Poll until the command prints a value no greater than `want`. +wait_for_at_most() { + local timeout="$1" want="$2"; shift 2 + local i got + for i in $(seq 1 "$timeout"); do + got="$("$@" || true)" + if [ -n "$got" ] && [ "$got" -le "$want" ] 2>/dev/null; then + return 0 + fi + sleep 1 + done + echo " (last value: '${got:-}', wanted <= '$want')" >&2 + return 1 +} + # Poll until the command prints a value that is at least `want`. wait_for_at_least() { local timeout="$1" want="$2"; shift 2 @@ -387,6 +402,20 @@ else echo " which is inside the deadline's reach)" fi +# The peers that interface carried must go with it, and go *now*. +# +# `link_dead_timeout_secs` is at its 30 s default here, so a withdrawal inside +# 15 s can only have come from the detach edge and not from the liveness +# reaper. That gap is the whole point: until the edge drove the teardown, this +# node kept the peer, kept selecting routes through it, and kept advertising +# reachability it no longer had — dropping transit traffic in silence for the +# whole timeout, with alternative paths sitting unused. +if ! wait_for_at_most 15 0 peer_count "$NODE_A"; then + fail "(c) node-a kept a peer that was only reachable over the downed \ +interface; the detach edge did not withdraw it" +fi +pass "(c) the peers the interface carried were withdrawn on the detach edge" + log "Bringing $LAB_IFACE back up on node-a" docker exec "$NODE_A" ip link set "$LAB_IFACE" up @@ -401,6 +430,13 @@ if ! wait_for_at_least 45 2 iface_field "$NODE_A" lab binds; then fi pass "(c) the interface flapped and the daemon followed it both ways" +# And the withdrawal is not a one-way door: the peer comes back over the +# rebound interface on its own, by beacon, with no operator action. +if ! wait_for_at_least 60 1 peer_count "$NODE_A"; then + fail "(c) node-a did not re-peer after its interface came back" +fi +pass "(c) peering re-established over the rebound interface" + # ── (d) destroy and recreate ───────────────────────────────────────────── # # Deleting the netdev outright is the case the old ENXIO beacon-socket reopen