From f8f2d127243da2bec0c36fd4638e5fa33da6452b Mon Sep 17 00:00:00 2001 From: Arjen <18398758+Origami74@users.noreply.github.com> Date: Wed, 2 Sep 2026 08:54:17 +0100 Subject: [PATCH] refactor(transport): classify send failures centrally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InterfaceUnavailable existed to make absence branchable, and then stopped being branchable at the transport boundary: every non-MTU error was flattened into NodeError::SendFailed { reason: format!(...) }, so no caller downstream could tell a two-second interface flap from a permanent fault. Both got the same treatment, which for a half-built handshake means being torn down and filed as peer misbehaviour. The classification belongs on the error rather than at each call site, and the question worth asking is not what went wrong but whether waiting fixes it: a transient failure was refused by a condition the daemon is already working to resolve, so the state built around it — a half-finished handshake, a route, a queued packet — is worth keeping. TransportError::is_transient answers that once, and NodeError::SendUnavailable carries the answer across the node boundary instead of discarding it. Deliberately narrow: only InterfaceUnavailable. Timeout and ConnectionRefused describe a remote that did not answer, which is a statement about the peer rather than about this node's ability to transmit, and their retry paths sit at a different layer. The test pins that narrowness in both directions, because the failure mode of this abstraction is someone adding a variant to the transient list and quietly making callers hold state open for a fault that will never clear. No behaviour change yet. This is the plumbing half; the callers that should act on it — route withdrawal on detach, and not counting a local interface flap as a handshake reject — are recorded in reference/ and deferred, because both are routing changes that want their own test story. --- src/node/mod.rs | 45 +++++++++++++++++++++++++ src/transport/mod.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/src/node/mod.rs b/src/node/mod.rs index 6fddb30f..5782ed7b 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -137,6 +137,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, @@ -169,6 +180,32 @@ pub enum NodeError { NoOperationalTransports, } +impl Node { + /// Test-only: place a transport into the node's map directly. + /// + /// The snapshot tests live in `crate::control` and so cannot reach the + /// private `transports` field, but the interface-presence block they need + /// to pin only exists on a real interface-bound transport. Mirrors + /// `isolate_peer_acl_for_test`: a narrow hook, so the fixture stays honest + /// rather than the snapshot being hand-authored JSON that nothing + /// produces. + #[cfg(test)] + pub(crate) fn insert_transport_for_test(&mut self, id: TransportId, handle: TransportHandle) { + self.transports.insert(id, handle); + } +} + +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 { @@ -3803,6 +3840,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/transport/mod.rs b/src/transport/mod.rs index 87a5745a..19eb47e4 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -219,6 +219,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 // ============================================================================ @@ -1677,4 +1720,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" + ); + } + } }