test(iface-binding): assert the fast path and the churn guard

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.
This commit is contained in:
Arjen
2026-09-02 10:13:42 +01:00
parent d6d91addad
commit ee5384f013
3 changed files with 126 additions and 7 deletions
+32 -4
View File
@@ -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());
}
+28 -3
View File
@@ -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.
+66
View File
@@ -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