From ee5384f013617c8559cecdaaa8538dd961d8c2f0 Mon Sep 17 00:00:00 2001 From: Arjen <18398758+Origami74@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:13:42 +0100 Subject: [PATCH] test(iface-binding): assert the fast path and the churn guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps, two of them in tests that existed and asserted nothing. **The netlink path was never asserted to be in use.** The 1 s poll is a complete fallback and covers every wait in the suite, so the whole thing passed with `open_link_socket()` hardcoded to Err — the fast path could have been dead for a release and no test would have said so. The binder reports which backing it got at startup, so case (g) asks it directly rather than inferring from timing the poll would also satisfy, and the unit test that used to write `let _ = w.is_event_driven();` now asserts it on Linux, where the source is an unprivileged `AF_NETLINK` socket and falling back is a real loss rather than a sandbox's prerogative. **Churn damping had no end-to-end coverage**, which now matters twice over: it bounds the recovery announcements, and since the detach edge withdraws peers it is also the only thing bounding how often that withdrawal fires. Every flap elsewhere in the suite is a single down/up with long settles either side — exactly the shape the damper ignores. Case (h) drives four bindings that each die inside `MIN_STABLE_BINDING`, asserts the guard engages, asserts it then *suppresses* rather than merely counting, and asserts it is not a latch. **`a_poisoned_binding_does_not_strand_the_transport` discarded its result.** `let _ = eth.binding.tasks_alive();` left the entire point unasserted: reading a poisoned lock as "alive" would have the binder believe a dead binding healthy and never rebind, and treating it as an error would strand the transport. `false` is what routes it back through detach and rebind, so say so. `a_stop_racing_a_bind_leaves_nothing_behind` now asserts the error *kind*. `bind_and_spawn` refuses at its presence probe long before the post-store shutdown check, so `is_err()` alone passed on absence and would still pass with that check deleted. The test keeps the coverage it genuinely has — stop raises the flag before teardown, teardown leaves no socket and no loops — and says plainly that the race it is named for needs a bind that succeeds, which needs privilege no unit test has. Both new cases were verified against the defect: with the netlink source forced to Err, (g) fails; with `CHURN_THRESHOLD` raised out of reach, (h) fails. Nothing else in the suite notices either. One case was attempted and removed rather than shipped: `"interface replaced"` cannot be produced deterministically, because the delete that changes an ifindex fires a netlink event the binder acts on within microseconds, so `gone` wins the race. It passed about one run in three. reference/notes.md records the measurement and the two approaches that could work. Also fixes a real bug in the harness: `grep -q` under `set -o pipefail` exits on its first match, `docker logs` takes SIGPIPE, and the pipeline reports failure even though the line was found. That cost two false failures before it was spotted; `log_count` reads the stream to the end. --- src/transport/ethernet/mod.rs | 36 +++++++++++++++-- src/transport/ethernet/watcher.rs | 31 +++++++++++++-- testing/iface-binding/test.sh | 66 +++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 7 deletions(-) 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