test(networkd): the shipped-policy retry tests are tautological on the delay — an initial: ZERO edit keeps them green (#645 review)

[vibechoom]

Follow-up from an adversarial review of PR #645, run specifically because CI structurally cannot verify that PR: nix flake check has no way to make networkd's ListLinks fail and then succeed, so green there means "compiles, lints, and its own pure unit tests pass" and nothing more.

The review's verdict was that #645 is sound — the retry loop cannot spin, the 8s ceiling is reached and enforced, checked_shl is correct at every input including u32::MAX, and a retried refresh publishes exactly what a first-try one would. Two things did not survive, both about claims rather than behaviour. I re-verified both against the PR head before filing.

1. The shipped-policy tests cannot fail in the way that matters

the_shipped_policy_never_goes_inert (networkd.rs:964-972 on the PR branch) asserts:

assert_eq!(
    STARTUP_REFRESH_RETRY.step(&failed(), attempt),
    RefreshStep::Retry { after: STARTUP_REFRESH_RETRY.backoff(attempt) },
);

The expected after is the same expression step computes internally. For the delay, that is tautological — it pins that step calls backoff, not that backoff produces a sane number.

The companion test the_shipped_policy_starts_promptly_and_stays_audible (:976-989) does not close the gap: it bounds the delay from above (backoff(1) <= 1s, max_backoff <= 30s) and asserts max_backoff >= initial, but never asserts a nonzero floor and never asserts the ceiling is actually reached.

Concrete falsification. Change one field to initial: Duration::ZERO and the entire suite stays green:

• never_goes_inert — expected after becomes 0, computed after is 0. Passes.
• starts_promptly_and_stays_audible — 0 <= 1s ✓, 8s >= 0 ✓, 8s <= 30s ✓. Passes.
• backoff_doubles_and_clamps_to_the_ceiling — exercises the test-local bounded() policy, not the shipped one. Unaffected.

The resulting behaviour is sleep(Duration::ZERO) on every retry: a tight loop logging at error! as fast as the bus can fail. That is precisely the flood STARTUP_REFRESH_RETRY's own documentation argues is impossible ("logging every attempt rather than every Nth is safe because the backoff caps at 8s"). The doc reasons about a property the tests do not pin.

This is a one-sided drift from the sibling. #634's equivalent test does have the assertion (wifi/mod.rs:1216-1221):

assert_eq!(
    super::PROBE_RETRY.step(&inconclusive(), u32::MAX),
    ProbeStep::Retry { after: super::PROBE_RETRY.max_backoff }
);

That pins after to the literal max_backoff, so it genuinely fails if the ceiling stops being reached. #645 mirrored the policy but dropped this assertion.

Fix: two lines — assert step(&failed(), u32::MAX) == Retry { after: STARTUP_REFRESH_RETRY.max_backoff } (ceiling reached, matching #634 exactly), and assert STARTUP_REFRESH_RETRY.backoff(1) > Duration::ZERO (nonzero floor, which #634 also lacks and should gain).

2. The comment describing the 2s loop is inaccurate

networkd.rs:534-536 says:

The 2s retry below is untouched: it covers a listen that had been established and dropped, which already retried and is not what #621 is about.

listen opens with refresh(links_out, primary_out, source_out).await? at :578 — its "Initial poll". So that loop also covers a listen whose initial refresh failed, i.e. one that was never established. The comment describes a narrower role than the code has.

The practical consequence is about observability, not correctness: if networkd dies shortly after a successful seed, recovery is handled by the 2s loop rather than by seed_links, so it logs warn! every 2s with no backoff and emits no startup refresh RECOVERED line — meaning #645's own live-verify instructions, which tell a verifier to grep for that line, would come back empty for a real self-heal.

Being accurate about what this is not: the review characterised this as reproducing #621's symptom. I do not think that holds. #621's symptom is LinkSource::Unknown latched — "no link manager has answered yet". After a successful seed, refresh has already published LinkSource::Networkd, so a later death leaves the link list stale, not indeterminate. Different failure, milder, and it does self-heal. The comment is still wrong and the live-verify gap is still real.

Fix: correct the comment to say what that loop actually covers, and either add a recovery log there or note in the live-verify section that a post-seed recovery shows up as the listen loop's warn! rather than a RECOVERED line.

Already tracked elsewhere — not re-filed

• The 2s loop's uncapped warn! cadence is chore(services): three hand-rolled exponential backoffs, and three fixed-2s reconnect loops with no ceiling (#645 follow-up) #646, filed earlier today (it covers this loop plus the mpris and bluetooth equivalents). Not duplicated here.
• No re-election after recovery. Unbounded retry means a host that migrates networkd → NetworkManager mid-session loops at error! forever with a working NM never re-probed. wifi has feat(reactive/wifi): switching wireless backend at runtime needs a cancellation primitive that doesn't exist — spawn_supervised returns nothing (#613 phase 2) #633 for exactly this; networkd has no equivalent. Recording it here rather than filing a third issue, since feat(reactive/wifi): switching wireless backend at runtime needs a cancellation primitive that doesn't exist — spawn_supervised returns nothing (#613 phase 2) #633's blocker — a droppable handle for spawn_supervised — is the same prerequisite for both.

Severity

Low. No user-visible defect, and #645 should merge as-is. Item 1 is the one with teeth: it is a guard that would not catch the regression it exists to catch, and the fix is two lines. Item 2 is a comment plus a live-verify note.

Refs #645, #621, #634, #646, #633.