mirror of
https://github.com/jmcorgan/fips.git
synced 2026-09-14 00:45:08 +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.
feat(node): withdraw a transport's peers when its interface goes away
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 (30 s) later.
Measured on real hardware: a dongle detached at 07:37:19 took the node's parent
with it, and no new parent was chosen until 07:37:46. Twenty-seven seconds
routing through a link that had already gone, with four alternative peers
available the whole time. The alternatives are the point — a mesh that can
route around a dead link should not be the last to hear the link is dead.
The detach edge is both earlier and more certain than inactivity, so it is the
better trigger. reap_peers_on_transport routes through the same
route_link_dead the liveness reaper uses rather than open-coding a second
teardown: every consequence of losing a peer — sessions, path MTU release,
session indices, decrypt-worker unregistration, 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.
Not policy-filtered. Whether an interface's absence is normal is a statement
about node *health*; it says nothing about whether the routes over it still
work. An optional interface's peers are exactly as unreachable.
PathBroken needs no new wiring. Once the peers are gone resolve_next_hop
returns None, which takes the NoRoute path — and that one already synthesises
the routing error, rate limiting included. The cure for the silent drop was to
stop having a route, not to add a second error path.
Deliberately undamped. A flapping interface cannot drive a reap storm through
here: ChurnGuard suppresses `announce` after three short-lived bindings, which
leaves `announced` false, which makes `detached()` return `retract: false` —
so no presence edge is published at all during churn. The edges this reacts to
are already rate-limited at the source, reaping an already-reaped transport is
a no-op, and a second damper would only add a way for the two to disagree.
The trade taken: immediate reaping costs a re-peer for an absence shorter than
the dead timeout that then recovers — a `wifi reload` returns in ~5 s and today
costs nothing, where this costs ~15 s of re-peering. Accepted, because
black-holing is silent, poisons other nodes' routing and needs the full timeout
to clear, where a re-peer is bounded, visible and self-healing. A grace period
remains available if that proves wrong; reference/ records its shape.
The integration assertion is the one that proves the wiring rather than the
unit: link_dead_timeout_secs is left at its 30 s default, so a withdrawal
inside 15 s can only have come from the detach edge. Verified against the
defect — with the reap disabled, that assertion fails and every other case in
the suite still passes.
fix(node): keep a half-built link when msg2 hits a transient transport
A 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, the queued PromoteToActive
aborted — and the whole thing recorded as
RejectReason::Handshake(HandshakeReject::BadState).
That counter means "the remote sent something invalid". A local interface flap
is not the remote's fault, and an operator reading the rejects would conclude
it was. The initiator, meanwhile, resends msg1 into a link that no longer
exists and has to rebuild from nothing.
The binder is already working to bring the interface back, so the half-built
link is now left exactly where it is for that resend to land on. Nothing leaks
by staying: an initiator that never resends leaves a stale connection, which
`check_timeouts` reaps at `handshake_timeout_secs` like every other abandoned
handshake. Only a genuinely terminal error still tears down.
This is the first consumer of `TransportError::is_transient`, which is what
the central classification was for — before it, the distinction did not
survive as far as this call site.
The rekey msg1 send site gets the severity half only. Its teardown was already
benign: it 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 `warn!` was wrong for a local,
self-clearing condition the presence machine has already reported.
The test drives a real absent Ethernet transport rather than a stub, so the
error under test is the one production raises, from the code path that raises
it. Verified against the defect: with the transient branch disabled, the link
is destroyed and the assertion fails.
The deferral gives the epoch-mismatch restart arm in handle_msg1 a third
outcome, and its post-promote debug_assert! did not admit it. That arm's
assertion required the machine to be Established or absent; a transient msg2
failure returns before PromoteToActive and leaves it registered at
Handshaking{ReceivedMsg1}, so a debug build panics there. The assertion is
widened to name that phase exactly, which keeps it red for any other state,
and a_transient_msg2_failure_on_the_restart_path_leaves_the_fresh_leg_pending
covers the arm the existing transient test does not reach. Three comments
around those two arms claimed a send failure always removes the machine; each
now names the transient case as well. Release builds were never affected: the
tail is gated on Established, so a deferred machine simply skips it.
The route_link_dead doc comment is put back on route_link_dead. Inserting
reap_peers_on_transport between the comment and the function it described left
both blocks running together, so rustdoc attached the whole thing to the new
function and route_link_dead lost its documentation. The restored text also
names the second caller this commit adds, and generalises the sentence about
where now_ms comes from, since both callers now hoist it once per batch.
The transport-layer design's grace-period paragraph is rewritten. It argued
that no linger timer was needed because peers survive a detach untouched and
the liveness reaper is the effective bound. The detach reap makes both halves
false, so the paragraph now states the trade it actually makes: peers go at
the edge, a half-built link is still held, and a short absence that recovers
costs a re-peer, which is preferred to silent black-holing.
Changelog entries for both user-visible halves.
The insert_transport_for_test helper is no longer added here. Nothing used it
until two commits later, so cargo clippy --all-targets -- -D warnings failed
on dead code at this point in the history; it now lands with its caller.
This commit is contained in:
Arjen
Johnathan Corgan
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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() => {
|
||||
|
||||
@@ -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 { .. })
|
||||
|
||||
@@ -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<NodeAddr> = 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(),
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user