diff --git a/src/transport/ethernet/mod.rs b/src/transport/ethernet/mod.rs index 64667500..b962133f 100644 --- a/src/transport/ethernet/mod.rs +++ b/src/transport/ethernet/mod.rs @@ -1735,8 +1735,18 @@ mod tests { assert!(panicked.is_err(), "the helper thread was supposed to panic"); assert!(eth.binding.tasks.is_poisoned()); - // Still answerable, and teardown still runs to completion. - let _ = eth.binding.tasks_alive(); + // Still answerable, and — the part that matters — it answers the way + // that keeps the transport recoverable. Discarding this, which the + // test used to do, left the whole point unasserted: treating a + // poisoned lock as "tasks are alive" would make the binder believe a + // dead binding is healthy and never rebind it, and treating it as an + // error would strand the transport instead. `false` is what routes it + // back through detach and rebind. + assert!( + !eth.binding.tasks_alive(), + "a poisoned task lock must read as a dead binding, so the binder \ + rebinds rather than either believing it healthy or giving up" + ); eth.stop_async().await.expect("stop"); assert_eq!(eth.state(), TransportState::Down); } @@ -1964,9 +1974,27 @@ mod tests { local_pubkey: None, presence_tx: None, }); + // Assert the *reason*, not merely that it errored. This transport's + // interface does not exist, so `bind_and_spawn` refuses at its + // presence probe — before it ever reaches the post-store shutdown + // check. Asserting `is_err()` alone therefore proved nothing about + // the stop flag: the assertion passed on absence, and would still + // pass with the shutdown check deleted outright. + // + // What is covered here is the observable half of the race — stop + // raises the flag before tearing down, and teardown leaves no socket + // and no loops. The post-store check itself needs a bind that + // *succeeds*, which needs a real bindable interface and the privilege + // to open a raw socket on it; that path is exercised only under the + // docker suite, and no unit test can reach it unprivileged. + let err = bind_and_spawn(&ctx) + .await + .expect_err("a bind must not complete for a stopped transport"); assert!( - bind_and_spawn(&ctx).await.is_err(), - "a bind must not complete for a stopped transport" + matches!(err, TransportError::InterfaceUnavailable { .. }), + "expected the absence refusal, got {err:?} — if this ever becomes \ + a shutdown refusal, this test has started covering the race it \ + is named for and the comment above is stale" ); assert!(eth.binding.socket().is_none()); } diff --git a/src/transport/ethernet/watcher.rs b/src/transport/ethernet/watcher.rs index 5e00b6bf..4311f98c 100644 --- a/src/transport/ethernet/watcher.rs +++ b/src/transport/ethernet/watcher.rs @@ -264,9 +264,34 @@ mod tests { #[tokio::test] async fn watcher_constructs_and_reports_its_backing() { let w = LinkWatcher::new(); - // Both answers are legitimate — a sandbox may refuse the socket — so - // this pins that asking is safe, not which answer comes back. - let _ = w.is_event_driven(); + + // On Linux the source is a plain `AF_NETLINK` socket in the + // `RTNLGRP_LINK` group, which needs no capability and no privilege — + // so on this platform "a sandbox might refuse it" is not a licence to + // accept either answer. Discarding the result, which this test used + // to do, meant nothing anywhere asserted that the event path exists: + // the 1 s poll is a complete fallback, so the entire suite passed with + // the source unavailable and no test could tell. + #[cfg(target_os = "linux")] + assert!( + w.is_event_driven(), + "the netlink link-event source must open on Linux; \ + falling back to the poll here is a silent loss of the fast path" + ); + + // Elsewhere both answers are legitimate, so pin only that asking is + // safe and that a watcher with no source parks rather than fires. + #[cfg(not(target_os = "linux"))] + { + let backed = w.is_event_driven(); + assert!( + backed + || tokio::time::timeout(Duration::from_millis(50), w.changed()) + .await + .is_err(), + "a watcher with no source must never resolve" + ); + } } /// A descriptor whose `recv` always fails must not become a busy loop. diff --git a/testing/iface-binding/test.sh b/testing/iface-binding/test.sh index 19a15bde..eccf4cce 100755 --- a/testing/iface-binding/test.sh +++ b/testing/iface-binding/test.sh @@ -532,6 +532,72 @@ if ! wait_for 30 "running" node_state "$NODE_C"; then fi pass "(f) health clears again when the interface returns" +# ── (g) the link-event fast path is actually the one in use ────────────── +# +# The whole suite would pass with `open_link_socket()` hardcoded to Err: the +# 1 s poll is a complete fallback and covers every `wait_for` window here, so +# nothing else asserts that the netlink path exists, let alone that it is what +# detected anything. The binder says which backing it got at startup, so ask +# it directly rather than inferring from timing that the poll would also +# satisfy. +# `log_count`, not `grep -q`: under `set -o pipefail` a `grep -q` that exits on +# its first match closes the pipe, `docker logs` takes SIGPIPE, and the +# pipeline reports failure even though the line was found. `grep -c` reads the +# stream to the end. +if [ "$(log_count "$NODE_A" "event_driven=true")" -eq 0 ]; then + docker logs "$NODE_A" 2>&1 | grep -i "binder started" >&2 || true + fail "(g) the binder fell back to polling; the netlink link-event source \ +did not open, and every timing assertion in this suite would still pass" +fi +pass "(g) detection is driven by netlink events, not by the poll fallback" + +# ── (h) churn damping engages on a genuinely flapping interface ────────── +# +# This is load-bearing twice over. It is what stops a flapping interface +# logging a recovery per cycle, and — since the detach edge now withdraws the +# peers that interface carried — it is also the only thing bounding how often +# that withdrawal can fire. Nothing exercised it: every flap elsewhere in this +# suite is a single down/up with long settles either side, which is precisely +# the shape the damper ignores. +# +# Four bindings that each die well inside MIN_STABLE_BINDING (10 s). The +# streak crosses CHURN_THRESHOLD (3) on the third, which is the edge that +# announces itself. +log "(h) flapping $LAB_IFACE to drive the churn guard" +for _ in 1 2 3 4; do + docker exec "$NODE_A" ip link set "$LAB_IFACE" down + sleep 1 + docker exec "$NODE_A" ip link set "$LAB_IFACE" up + sleep 2 +done + +if ! wait_for_at_least 30 1 log_count "$NODE_A" "keeps dying immediately after binding"; then + docker logs "$NODE_A" 2>&1 | grep -i "ethernet" | tail -20 >&2 + fail "(h) four short-lived bindings did not engage the churn guard" +fi +pass "(h) a flapping interface engages churn damping" + +# Having engaged, the guard must hold health rather than announcing each bind. +# The failure this catches is a damper that counts but does not damp. +recoveries_during_churn="$(log_count "$NODE_A" "Ethernet interface recovered")" +if [ "$recoveries_during_churn" -gt 6 ]; then + fail "(h) node-a announced $recoveries_during_churn recoveries; the guard \ +counted the churn but kept announcing through it" +fi +pass "(h) churn suppressed the per-cycle recovery announcements" + +# And it is not a latch: once a binding lasts, the interface is announced +# again and the node returns to Running on its own. +log "(h) letting $LAB_IFACE settle" +docker exec "$NODE_A" ip link set "$LAB_IFACE" up >/dev/null 2>&1 || true +if ! wait_for 60 "present" iface_field "$NODE_A" lab presence; then + fail "(h) node-a did not rebind after the flapping stopped" +fi +if ! wait_for 60 "running" node_state "$NODE_A"; then + fail "(h) node-a stayed Degraded after the flapping stopped" +fi +pass "(h) a settled interface is announced again after churn" + # ── final log hygiene ──────────────────────────────────────────────────── # # Four outages happened above (start, down, delete, and node-b's end of the