refactor(services): one retry policy for wifi and networkd, with non-tautological shipped-policy tests (#646, #665)

[vibechoom]

Two follow-ups from #645, done in one PR because #646 creates the type that
#665's missing assertions belong on.

#646 — one retry policy instead of two

wifi's RetryPolicy/ProbeStep (#634) and networkd's
RefreshRetry/RefreshStep (#645) were near-identical: the same three fields
with the same meanings, the same shipped constants (None / 500 ms / 8 s), and a
backoff() body that was character-for-character the same checked_shl /
saturating_mul / min sequence. What kept them in sync was a pair of doc
comments cross-referencing each other by hand — and #665 is what that looks
like when it fails.

Where the shared type lives, and why there

New private module crates/hytte-services/src/retry.rs, holding retry::Policy
and retry::Step.

• hytte-services, not hytte-bus or hytte-reactive. Both call sites are
  service modules, both retry "a system daemon was not there yet", and nothing
  outside the crate needs the type. A private mod retry; keeps it that way.
• A module of its own, next to the existing mod eds_retry. That module is
  this crate's precedent for "shared resilience primitives, pure, hermetically
  testable". It is deliberately not extended to hold this: eds_retry is
  blocking / std::sync::mpsc-shaped and carries a failure-streak detector,
  not a Duration schedule with an attempt budget.
• Named retry::Policy, not RetryPolicy. hytte_bus::RetryPolicy already
  exists and is a different thing (per-call retry: Never / Once /
  Backoff) — and PROBE_RETRY's own docs reference it two paragraphs from
  where they described their own policy. Module-qualifying disambiguates at every
  use site; that doc reference is now an explicit hytte_bus::RetryPolicy::Once
  link that says it is a different, per-call policy.

Not folded in, and said so in the module docs so nobody re-opens them as
oversights:

• hytte_bus's Backoff (hytte-bus/src/connection.rs) — a stateful cursor
  with reset()/next(), no attempt budget, no give-up. chore(services): three hand-rolled exponential backoffs, and three fixed-2s reconnect loops with no ceiling (#645 follow-up) #646 rules it out by
  name; agreed, folding it in would be over-unification.
• wifi/watcher.rs:131's sleep(2s) — deliberately left alone. It is a
  one-shot debounce before a return on the iwd station-removed path, where the
  caller re-establishes the watch. It is not a retry loop, and "fixing" it into
  one would be a regression. chore(services): three hand-rolled exponential backoffs, and three fixed-2s reconnect loops with no ceiling (#645 follow-up) #646 names it for exactly this reason; the new
  module's docs now name it too, so the next person grepping from_secs(2)
  finds the reasoning before the code.

How wifi now weighs inconclusive-vs-failed in the caller

The surviving shape is networkd's generic
step<T, E>(&Result<T, E>, attempt) -> Step, whose Proceed carries no
payload. wifi's old ProbeStep::Commit(BackendChoice) did, which is the only
reason the two types could not already be one.

probe_until_conclusive now reads the verdict off the probe's own Result in
the Proceed arm instead of getting it back out of the step, and the weighing
that used to be implicit in step's signature is an explicit comment at the call
site: any answer is conclusive and commits — including
Ok(BackendChoice::None), "the bus replied and neither daemon is present" —
while ProbeError ("I could not ask") is the only retryable outcome.

This is strictly stronger against #613 than the old shape. With no payload on
Proceed, there is no longer any code path that could manufacture a
BackendChoice out of a ProbeError; the collapse #613 fixed is now
unrepresentable rather than merely tested against. (ProbeError correspondingly
drops out of the module's imports — since #646 nothing outside probe_backend's
own signature names it.)

Proof of no behaviour change

No log line, no delay, and no control-flow branch changed on either side.
Concretely:

• Both shipped constants keep max_attempts: None, initial: 500ms,
  max_backoff: 8s.
• backoff() moved verbatim — same checked_shl / saturating_mul / min.
• step()'s three arms are unchanged apart from the enum path and dropping
  *choice from the Ok arm.
• Every tracing::{info,warn,error}! call in seed_links and
  probe_until_conclusive is byte-identical, including the attempt == 1 /
  BackendChoice::None gating of wifi's three RECOVERED wordings.

The existing tests are the gate, and they pass. Where they moved or changed:

Test	Fate
networkd::backoff_doubles_and_clamps_to_the_ceiling	moved verbatim to retry::tests — identical bounded() helper, identical assertions
wifi::backoff_doubles_and_clamps_to_the_ceiling	dropped: byte-for-byte the same test on what is now the same type
networkd::a_bounded_policy_gives_up_once_its_budget_is_spent + wifi::err_past_the_bound_gives_up	one copy survives in retry::tests under the first name
wifi::err_retries_while_attempts_remain	survives in retry::tests as a_failed_attempt_retries_while_the_budget_lasts
wifi::unbounded_policy_never_gives_up	survives as retry::tests::an_unbounded_policy_never_gives_up, with the tautological after: forever.backoff(attempt) replaced by matches!(.., Retry { .. })
networkd::a_refresh_that_landed_proceeds_immediately, networkd::a_failed_startup_refresh_retries_instead_of_ending_the_task	kept in place, changed only in the enum path — named #621 regression guards over a real anyhow::Result
wifi::ok_commits_on_the_first_attempt, ok_none_commits_and_does_not_retry, inconclusive_probe_never_commits_to_no_backend	kept in place; Commit(choice) expectations become Proceed, because which verdict it was is no longer step's to report — see above for why #613's guard gets stronger, not weaker
networkd::the_shipped_policy_starts_promptly_and_stays_audible	replaced by retry::tests::every_shipped_policy_starts_promptly_and_stays_audible, which runs the same three assertions over both shipped constants
wifi::shipped_policy_is_unbounded_and_starts_promptly	renamed the_shipped_policy_never_goes_inert (matching its networkd sibling); keeps max_attempts == None, sheds the delay assertions to the sweep

Net: every load-bearing assertion still runs; the ones that ran twice now run
once, on the shared type.

#665 — the shipped-policy tests were tautological on the delay

networkd::the_shipped_policy_never_goes_inert asserted

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

The expected after is the expression step computes internally, so it pinned
that step calls backoff — nothing about the number backoff returns. The
companion test bounded the delay only from above.

The falsification, run for real

Before (on 61967dd, the pre-refactor tree): change one field,
STARTUP_REFRESH_RETRY.initial from Duration::from_millis(500) to
Duration::ZERO, and the suite stays green.

test result: ok. 681 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

The resulting behaviour is sleep(Duration::ZERO) on every retry. seed_links
logs every attempt at error!, so that is not a fast retry — it is a tight
loop flooding the journal as fast as the bus can fail, precisely the flood
STARTUP_REFRESH_RETRY's own docs argue is impossible ("logging every attempt
rather than every Nth is safe because the backoff caps at 8s").

I also ran #665's "one-sided drift" claim, and it holds exactly: the same edit
to wifi::PROBE_RETRY.initial on 61967dd does go red —

thread 'wifi::tests::shipped_policy_is_unbounded_and_starts_promptly' panicked at wifi/mod.rs:1216
test result: FAILED. 680 passed; 1 failed

— because #634 compared against the literal PROBE_RETRY.max_backoff, so a
zeroed initial breaks the ceiling equality. #645 mirrored the policy and
replaced that literal with a re-computation, which is what dropped the guard.
wifi still had no nonzero floor of its own, though.

After (this branch), the same initial: Duration::ZERO edit fails for either
constant:

thread 'retry::tests::every_shipped_policy_sleeps_a_nonzero_time_and_reaches_its_ceiling' panicked at retry.rs:236:
networkd::STARTUP_REFRESH_RETRY: the first retry delay is zero — every attempt logs at `error!`,
so this is a tight log flood rather than a backoff (#665)

thread 'retry::tests::every_shipped_policy_sleeps_a_nonzero_time_and_reaches_its_ceiling' panicked at retry.rs:236:
wifi::PROBE_RETRY: the first retry delay is zero — …

And the ceiling half is live too — breaking backoff()'s clamp
(.min(self.max_backoff) → .min(self.max_backoff / 2)) reds both the
mechanism test and the sweep:

assertion `left == right` failed: wifi::PROBE_RETRY: the doubling never actually
reaches `max_backoff`, so the documented ceiling is not the real one (#665)

What now catches it

retry.rs carries a table of every retry policy the crate ships:

#[cfg(test)]
const SHIPPED: &[(&str, Policy)] = &[
    ("wifi::PROBE_RETRY", crate::wifi::PROBE_RETRY),
    ("networkd::STARTUP_REFRESH_RETRY", crate::networkd::STARTUP_REFRESH_RETRY),
];

Two tests sweep it, so a third policy cannot ship without these assertions
coming along — the table is the only route to coverage, and its doc comment
says why:

• every_shipped_policy_sleeps_a_nonzero_time_and_reaches_its_ceiling —
  backoff(1) > Duration::ZERO (the nonzero floor, which neither side had),
  backoff(u32::MAX) == policy.max_backoff, and for unbounded policies
  step(&failed(), u32::MAX) == Retry { after: policy.max_backoff } (the ceiling
  is reached through step, matching fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634's assertion exactly).
• every_shipped_policy_starts_promptly_and_stays_audible — backoff(1) <= 1s,
  max_backoff >= initial, max_backoff <= 30s.

Every comparison is against a literal — a field of the policy under test, or
Duration::ZERO — never against policy.backoff(...). The per-site
the_shipped_policy_never_goes_inert tests keep only the claim that is genuinely
site-specific (max_attempts: None, argued at length on #613 and #621) and now
use matches!(.., Retry { .. }) rather than re-deriving a delay, with a doc
comment pointing at where the delay is actually pinned.

#665 item 2 — the inaccurate comment

networkd.rs's "the 2s retry below … covers a listen that had been established
and dropped" was wrong: listen opens with its own initial refresh, so that
loop also covers a listen that was never established. Corrected in place,
including #665's own qualification that this is milder than #621 (the seed has
already published LinkSource::Networkd, so a later death leaves the list stale,
not indeterminate, and it self-heals) and the observability note that a post-seed
recovery surfaces as that loop's warn! going quiet rather than as a
startup refresh RECOVERED line — so grepping for that line will not evidence a
real self-heal. #645's live-verify section is in a merged PR body and can't be
amended, so the note lives at the code.

What I did not do — #646's second half

The three fixed-2s reconnect loops (networkd::listen's loop at networkd.rs,
mpris/mod.rs:181, bluetooth/mod.rs:201) still retry flat every 2 s with no
ceiling. #646 scopes that as "worth doing only if someone is in the file anyway",
and I left it deliberately:

• It is not a refactor. Giving those loops a capped backoff changes delays and
  log wording (the "retrying in 2s" text becomes false the moment a backoff
  exists), which is exactly what this PR promised not to touch.
• The interesting part is a design question this PR has no answer for: when
  does the attempt counter reset? These loops have no clean success signal —
  listen returning Ok(()) means "the stream ended", which can happen
  immediately. A naive counter would back a healthy-but-flappy daemon off to the
  ceiling and keep it there. Something like "reset if the last listen ran
  longer than N seconds" is a judgement call worth making on its own, not as a
  rider.
• It spans three files, two of which this PR otherwise does not touch.

retry::Policy is now sitting there ready for it, with a longer ceiling. Hence
Refs #646 rather than Closes — everything else in #646 is done.

Gates

• cargo clippy --workspace --all-targets --features system-tests -- -D warnings
  (RUSTC_WRAPPER unset) — clean
• cargo test --workspace — green
• nix fmt — clean

No Cargo.toml / Cargo.lock change; no new dependency.

Live-verify

Nothing to verify on a live session: every line touched is either pure logic
(unit-tested hermetically above) or a comment. The only runtime-visible surface —
seed_links' and probe_until_conclusive' log lines and sleeps — is unchanged
byte-for-byte, which is what the "no behaviour change" table above is arguing.

Closes #665
Refs #646

🤖 Generated with Claude Code