mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-06 03:28:24 +00:00
refactor(transport): classify send failures centrally
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.
This commit is contained in:
@@ -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),
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user