fix(networkd): retry a transient startup refresh failure instead of going inert (#621)

[vibechoom]

Closes #621
Refs #607, #613, #634

The bug

networkd.rs's LinkBackend::Networkd arm seeded the link list with one
refresh and returned if it failed — before the retry loop, so the
loop { listen(…) } below was unreachable from that path. One unlucky
refresh at startup ended the task for the whole process lifetime: source
stayed LinkSource::Unknown, the network panel showed "no link manager has
answered yet" forever, and systemctl --user restart trollshell was the only
cure.

The comment justified the return with "networkd isn't running on this host
and no NM is present either". That condition cannot hold here: this arm is
reached only after probe_link_backend has established that networkd is
the backend. The genuinely-absent case is LinkBackend::None, handled
separately and correctly publishing LinkSource::Unavailable. So the guard
defended against something the probe had already ruled out, while catching the
one thing it should tolerate — a transient failure microseconds after the
probe's own call succeeded (networkd restarting, a D-Bus hiccup,
ServiceUnknown during systemctl restart systemd-networkd).

The policy: unbounded retry, kept observable

Unbounded retry with capped backoff. Attempt 1 is immediate; each retry
waits 0.5 s doubled per elapsed attempt, clamped at 8 s, settling into one
refresh every 8 s for as long as networkd stays unreachable. Once a refresh
lands, the existing listen loop runs exactly as before.

The issue's original lean was bounded retry then inert; it was retracted on
the thread, and the counter-example is in this very file. probe_link_backend's
inconclusive-probe arm already decided the same question the other way:

Picking None here is what latched #607: it leaves the link list permanently
empty even once the bus recovers. The NM watcher re-polls every 5s, so it
self-heals if NM does turn up; the cost if NM genuinely never appears is a
periodic GetDevices failed warn, which beats an inert service.

That makes unbounded the conservative option here, not the adventurous one —
it is what this file already does on the sibling path a few dozen lines above.
It applies even more directly than it does for wifi: a bound would put the user
back in exactly the state #621 reports whenever networkd takes longer than the
bound to come up, i.e. this bug on a longer fuse. And "the give-up is logged, so
a wrong bound is visible" argues for having a bound from the visibility of the
bound being wrong.

This also keeps the two subsystems consistent with #634, which landed the
same unbounded-with-capped-backoff shape for the wifi backend probe. The numbers
are copied from its PROBE_RETRY deliberately, and I read the constant rather
than trusting the description — wifi/mod.rs's PROBE_RETRY is
max_attempts: None, initial: 500ms, max_backoff: 8s, exactly what
STARTUP_REFRESH_RETRY now uses. Same failure class, same journal cadence, so
someone debugging one path while reading the other's journal finds one
behaviour, not two.

What is deliberately not imported from #634: its backend-reselection
machinery. This arm has already elected networkd, so there is no verdict to
re-resolve — refresh either read the links or could not, and "could not" is
never a fact about the host here.

Log cadence

The one requirement that survived the retracted lean is that the retry stays
observable: a quiet self-heal trades a visible permanent bug for an
invisible intermittent one, which is worse for exactly the reporter's situation
(nothing in the UI hinted anything had failed).

• Every failed attempt logs at error! with attempt, retry_in_secs and
  the error — deliberately every attempt rather than every Nth, matching fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634's
  judgment and for the same reason: the backoff caps at 8 s, so a permanently
  unreachable networkd stays loud in the journal instead of degrading into a
  silent poll. It cannot become the 2 s flood the old comment worried about.
• A retry that finally lands logs networkd: startup refresh RECOVERED at
  warn! with attempts=N, so a successful retry never erases the evidence
  that the path fired. Grep tokens are startup refresh FAILED /
  startup refresh RECOVERED, parallel to fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634's
  backend probe was INCONCLUSIVE / backend probe RECOVERED.
• The ordinary first-try success stays silent — attempt == 1 prints nothing,
  so healthy hosts see no new lines at all.

The listen loop's own fixed 2 s retry is untouched: that path covers a
connection that had been established and dropped, it already retried, and it
is not what #621 is about. Noted in a comment so the two cadences don't read as
an oversight.

What changed

One file, crates/hytte-services/src/networkd.rs:

• RefreshStep / RefreshRetry / STARTUP_REFRESH_RETRY — the retry decision
  is the pure function RefreshRetry::step(&outcome, attempt) -> RefreshStep,
  so it is unit-testable with no bus. GiveUp stays expressible and tested, so
  a bounded policy remains one field away (max_attempts: Some(n)); only
  the shipped default is unbounded, and a test asserts that. It is constructed
  on the max_attempts: Some(_) path, so clippy does not flag it as dead — no
  allow was needed anywhere in this PR.
• seed_links(policy, …) -> bool drives it and owns the logging. false means
  the policy stopped asking, which the shipped policy never does.
• The LinkBackend::Networkd arm calls it instead of refresh-then-return.
• The false justifying comment is gone, replaced by one stating why a failure
  here is transient by construction; a module-doc section records the same, next
  to the existing network interfaces do not show up in panel #80 and network panel says "Offline / 0 interfaces" when it simply has no link manager to ask (#607 split) #608 sections.

No behaviour change on any other path: LinkBackend::NetworkManager and
LinkBackend::None are untouched, and a host where the first refresh succeeds
takes the identical code path it did before, with no new log lines.

Tests

6 new tests, all hermetic (plain #[cfg(test)], no system-tests, no bus):

a_refresh_that_landed_proceeds_immediately,
a_failed_startup_refresh_retries_instead_of_ending_the_task,
a_bounded_policy_gives_up_once_its_budget_is_spent,
backoff_doubles_and_clamps_to_the_ceiling,
the_shipped_policy_never_goes_inert,
the_shipped_policy_starts_promptly_and_stays_audible.

Falsification. With the fix in place, cargo test -p hytte-services networkd
→ 23 passed, 0 failed. Reintroducing the bug (Err(_) => RefreshStep::GiveUp,
i.e. "go inert on any failure"):

test networkd::tests::a_failed_startup_refresh_retries_instead_of_ending_the_task ... FAILED
test networkd::tests::the_shipped_policy_never_goes_inert ... FAILED

assertion `left == right` failed: the first failed seed must retry, not go inert (#621)
  left: GiveUp
 right: Retry { after: 10ms }

assertion `left == right` failed: attempt 1: a failed seed ended the task instead of retrying
  left: GiveUp
 right: Retry { after: 500ms }

test result: FAILED. 21 passed; 2 failed

The edit was reverted afterwards with git checkout --, and git diff HEAD
confirmed empty before pushing.

What the hermetic tests cannot cover is the wiring — that seed_links is
what the Networkd arm actually calls, and that a real transient D-Bus failure
recovers end-to-end. That needs a live bus; see live-verify below.

Gates

• cargo clippy --workspace --all-targets --features system-tests -- -D warnings
  — clean, exit 0, with RUSTC_WRAPPER unset so sccache cannot skip lint
  emission. (The first run caught a real clippy::unnecessary_wraps on a test
  helper that always returned Ok; fixed by making it a local binding rather
  than silencing the lint.)
• cargo test --workspace (hermetic) — exit 0; hytte-services lib 678 passed,
  0 failed.
• nix fmt — 385 files processed, 0 changed.

Live-verify

CI cannot exercise a transient bus failure — nothing in nix flake check can
make networkd's ListLinks fail and then succeed. Needs a real networkd host:

1. Provoke the race: systemctl restart systemd-networkd and, in the same
   moment, systemctl --user restart trollshell (or start the shell early in a
   fresh boot's session, before networkd has settled).
2. Confirm the network panel's link list populates on its own, without a
   shell restart — pre-fix it would sit on "no link manager has answered yet"
   indefinitely.
3. Confirm the journal shows both halves:

   journalctl --user -u trollshell | grep -E 'startup refresh (FAILED|RECOVERED)'

   Expect at least one FAILED line (with attempt= and retry_in_secs=)
   followed by one RECOVERED line carrying attempts=.
4. Sanity check the healthy path is unchanged: on a normal boot where the first
   refresh succeeds, expect no startup refresh lines at all and the link
   list populated as before.
5. Sanity check the negative case is unchanged: on a host with neither daemon,
   networkd: no link backend available; service inert should still appear
   once, promptly, with no retry lines — that is the LinkBackend::None arm and
   it is untouched.

Adjacent, not fixed here

Noted for follow-up rather than widening this diff:

• RefreshRetry duplicates the shape of wifi::RetryPolicy (fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634). Sharing one
  policy type would need wifi/mod.rs edits (its step is typed to
  Result<BackendChoice, ProbeError> where this one is generic over the
  outcome), which is outside this fix's lane. The doc comments cross-reference
  each other so the two can't silently drift; happy to file it if wanted.
• The listen loop's fixed 2 s retry has no backoff and no ceiling on its
  warn! cadence, so an established-then-dropped networkd connection does log
  every 2 s indefinitely. Pre-existing and out of scope here.

🤖 Generated with Claude Code

[vibechoom]

Adversarial review

CI cannot exercise this change — nothing in nix flake check can make ListLinks fail and then succeed — so I read the shipped code path against the sibling it claims parity with (#634) and against the bus layer underneath it, rather than trusting the tests or the prose.

Verdict

Sound, non-blocking. The diagnosis is correct, the premise holds under checking, the retry loop cannot spin, the 8s ceiling is real, and the state a retried refresh publishes is byte-identical to a first-try one. Two Low findings, both about things the PR says rather than things it does: a shipped-policy test that does not check the property it is named for (and which #634 does check), and a new comment that mischaracterizes the loop it sits above. Neither is a reason to hold the merge.

---

Finding 1 — the shipped-policy tests do not pin either property the log-cadence argument rests on

Severity: Low (maintenance hazard, plus a one-sided drift from #634).

STARTUP_REFRESH_RETRY's doc stakes the every-attempt error! on two facts (crates/hytte-services/src/networkd.rs:371-373): the delay has a nonzero floor, and it reaches an 8s ceiling. Both are true of the shipped constants. Neither is asserted by any test.

• networkd.rs:979 — backoff(1) <= 1s. Passes for initial: Duration::ZERO.
• networkd.rs:980 — max_backoff >= initial. Passes for zero.
• networkd.rs:985 — max_backoff <= 30s. Passes for zero.
• networkd.rs:955-974 (the_shipped_policy_never_goes_inert) — compares step(...) against RefreshStep::Retry { after: STARTUP_REFRESH_RETRY.backoff(attempt) }. That is the same expression step itself computes at networkd.rs:404-406, so the after field is tautological; the assertion only pins "not GiveUp". It would pass against a backoff that returned Duration::ZERO for every input.
• networkd.rs:944-953 (backoff_doubles_and_clamps_to_the_ceiling) — runs on bounded() (networkd.rs:891), a 10ms/40ms test policy, never on the shipped constant.

Failure scenario. A future maintainer edits initial from 500ms toward zero — the single most likely change to this constant, since "the boot race is sub-second" is the argument the PR makes for a prompt first retry. backoff becomes ZERO.saturating_mul(factor).min(8s) = zero for every attempt (networkd.rs:387-392), and tokio::time::sleep(after) at networkd.rs:460 becomes a no-op. Against the dominant failure — ServiceUnknown is classified Permanent (crates/hytte-bus/src/error.rs:81-88), so call() returns at crates/hytte-bus/src/call.rs:131 without even the reconnect wait — the loop then runs as fast as a D-Bus round trip, emitting a ~430-character error! per iteration. That is exactly the flood networkd.rs:371-373 asserts cannot happen. Full test suite still green.

Drift from #634: crates/hytte-services/src/wifi/mod.rs:1216-1221 asserts PROBE_RETRY.step(&inconclusive(), u32::MAX) == ProbeStep::Retry { after: PROBE_RETRY.max_backoff } — non-tautological, and the one assertion in wifi's suite that pins the ceiling on the shipped constant. #645's equivalent replaced max_backoff with backoff(attempt), which removes it. Since "the two must not diverge" is this PR's stated reason for copying the numbers, losing an assertion the sibling has is worth naming.

Refutations I tried and why they failed:

1. Nobody would set initial to zero. But the_shipped_policy_starts_promptly_and_stays_audible exists specifically to stop a future edit from breaking the audibility bargain. It is named for a property it does not check.
2. clippy or CI would catch a spin loop. It would not — there is nothing lint-visible about sleep(Duration::ZERO), and no test exercises seed_links at all.
3. fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634 has the same floor gap, so it is a shared pattern, not a defect. True, and it is why this is Low rather than higher. But fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634 at least keeps the ceiling assertion, so the drift is one-sided, and the fix is additive to both files.

Suggested fix — two lines in the_shipped_policy_starts_promptly_and_stays_audible:

assert!(
    STARTUP_REFRESH_RETRY.backoff(1) >= Duration::from_millis(100),
    "sleep(backoff) in seed_links is the ONLY throttle on an error!-per-attempt loop"
);
assert_eq!(
    STARTUP_REFRESH_RETRY.backoff(u32::MAX),
    STARTUP_REFRESH_RETRY.max_backoff,
    "the every-attempt error! is defensible only because the 8s ceiling is reached"
);

---

Finding 2 — the new comment above the 2s loop is not true of the loop it describes

Severity: Low (comment accuracy). Directly the "is the replacement comment true?" question.

networkd.rs:534-536:

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 subscribes at networkd.rs:569-573 and then does refresh(links_out, primary_out, source_out).await? at networkd.rs:578. That ? propagates, so loop { match listen(...) } at networkd.rs:537-545 also covers a listen whose initial refresh failed — one that established no stream at all. The two paths therefore retry the same call (refresh) at different cadences (0.5→8s capped vs fixed 2s) and different levels (error! vs warn!), separated only by whether one refresh has ever succeeded in this task.

Concrete consequence. networkd answers the probe; seed_links succeeds first try (silent by design, networkd.rs:436); networkd goes down one second later — i.e. the systemctl restart systemd-networkd race the PR body names as the motivating trigger, with the shell winning by a second instead of losing by one. refresh then retracts source to Unknown (networkd.rs:620) and the panel returns to "no link manager has answered yet", the exact #621 symptom — but the journal shows networkd error, retrying in 2s at warn!, and the PR body's own live-verify recipe (grep -E 'startup refresh (FAILED|RECOVERED)') returns nothing. A maintainer following that recipe would conclude the retry path never fired.

Refutations I tried:

1. signals(...).start() runs before the refresh, so the subscription was "established". .start() only spawns a task (crates/hytte-bus/src/signals.rs:147-172); it does not wait for the match rule, and against an absent bus name nothing is established. Generous at best, and it does not rescue the "which already retried" half of the sentence.
2. The substance is right — the 2s loop is untouched and out of scope. Agreed, which is why this is Low and not a defect. But this comment is the only in-tree explanation for why one file now has two cadences for one call, and the reason it gives is not the real one.
3. The split is still meaningful. It is: seed covers "never seeded", the 2s loop covers "seeded once". That is a defensible line — it just is not "established and dropped".

Suggested fix: reword to the true predicate (the 2s loop covers every refresh failure after the first successful seed, established stream or not; #621 is specifically about never getting past the seed), and add one line to the live-verify section noting that a post-seed failure surfaces as networkd error, retrying in 2s, not as startup refresh FAILED.

Related, recorded as considered rather than as an objection: because listen re-refreshes at networkd.rs:578, deleting the early return alone would have closed #621 with a four-line diff and a fixed 2s cadence. The 305 lines still earn their place — distinct grep tokens for the never-seeded case (the retraction's one surviving requirement), and no spawn/teardown of a signals() subscription every 2s against an absent bus name. I am not asking for the smaller diff.

---

Note — unbounded plus no re-election, informational

networkd.rs:397-399 says "could not" is never a fact about the host here — the backend was already elected. True at t=0; I verified it (see clean list, item 7). It stops being true as t grows: a user who runs systemctl disable --now systemd-networkd or migrates to NM mid-session leaves this loop retrying a genuinely absent daemon forever at error!, with a working NM present and never re-probed — networkd.rs:259 returns Networkd without ever calling probe_backend(), so NM's presence was never even established on that path. Pre-fix that host went inert with one info!; post-fix it is equally inert plus ~10.8k error! lines/day.

That is the accepted cost of the retraction's policy and I am not arguing against it. But wifi has #633 tracking exactly this (re-election needs a cancellation primitive that does not exist). networkd has no equivalent issue. If the two subsystems are to stay paired, they should be paired in the backlog too, not only in their constants — worth filing the networkd half.

---

Attacked and found clean

1. Can the loop spin against a fail-fast error? No. ServiceUnknown is Permanent (hytte-bus/src/error.rs:81-88, pinned by the test at :107-121), so call() returns at call.rs:131 without wait_for_reconnect — the fastest failure available. tokio::time::sleep(after) at networkd.rs:460 is unconditional inside the Retry arm, and both other arms return (:444, :472), so no path skips it. Floor is backoff(1) = 500ms.
2. Is the 8s ceiling reached and enforced by code, not just prose? Yes. .min(self.max_backoff) at networkd.rs:391; 500ms * 2^4 = 8s at attempt 5, clamped for every later attempt. checked_shl at :388-390 returns None from attempt 33 on, falling back to a u32::MAX factor which saturating_mul then clamps. Correct at 1, 5, 32, 33, u32::MAX.
3. A week of dead networkd in the journal. ~10.8k lines/day, ~75k/week, roughly 45 MB/week with journald metadata. journald's defaults (RateLimitIntervalSec=30s, RateLimitBurst=10000) never engage at ~4 lines per interval, and SystemMaxUse (10% of /var, 4 GB cap) absorbs it. Loud as designed, not a flood — and identical to what fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634 already ships.
4. fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634 parity, constants and semantics. STARTUP_REFRESH_RETRY (networkd.rs:378-382) is field-for-field PROBE_RETRY (wifi/mod.rs:256-260): None / 500ms / 8s. step (networkd.rs:400-408) is structurally identical to RetryPolicy::step (wifi/mod.rs:279-288) including the attempt >= max guard, so Some(1) means zero retries in both. Log levels match. The only behavioural delta is that networkd stays silent on a first-try success where wifi logs info!(?choice, "wifi: selected backend") (wifi/mod.rs:321) — correct, since networkd had no such line before and refresh has never announced itself. The PR body's parity claim on the constants is accurate; I read both, I did not take the description's word for it.
5. seed_links loop mechanics. attempt starts at 1 (:422) and increments only in the Retry arm, after the sleep (:461). Overflow needs 2^32 iterations at >=8s each (~1e9 years), and a debug-build overflow panic would be caught and restarted by spawn_supervised anyway (hytte-reactive/src/supervisor.rs:95-111). reason (:427-430) is computed on the success path too, but String::new() does not allocate. One caller (:524-533), and it honours the false contract with a bare return.
6. State published by a retried refresh vs a first-try one. Identical. refresh (:612-634) is a pure function of read_networkd_links() and the retry re-enters it unchanged; nothing is carried across iterations. On failure it retracts source to Unknown (:620), which is already what initial_source() published at :496, so set_neq is a no-op and the retry window produces no spurious signal edge. links/primary are left untouched on failure and are empty at startup, so no stale reading is reachable on this path. (The pre-existing sharp edge where ListLinks succeeds but every per-link Describe fails, publishing source = Networkd with primary = None, behaves the same pre- and post-fix and self-corrects on the 5s timer — not introduced here.)
7. "Transient by construction." Holds. All three returns of LinkBackend::Networkd (:259, :276, :289) require read_networkd_links() to have returned Ok, so the module doc at :31-37 and the comment at :510-517 are accurate. This also kills the "a permanent error gets retried forever" worry: an AccessDenied from a bus-policy change, or a ListLinks signature mismatch, would have failed the probe's own read_networkd_links() and routed to :305 (NetworkManager) instead — the Networkd arm is unreachable with a deterministically-failing refresh. Because step is generic over Result<T, E> (:400) it cannot tell permanent from transient, but per the above it never needs to.
8. GiveUp unreachability. Confirmed unreachable: max_attempts: None makes is_some_and false at :403. It is still constructed there, so rustc's dead-code pass does not fire and the PR body's "no allow was needed" claim holds. As a hazard it is mild and correctly flagged in the body — but note the tests pin step, not seed_links: the GiveUp arm's log line and return false (:463-473), and the caller's return (:532), have never executed in production or in a test. A future bounded policy would light them up cold. Same shape as fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634's, so not a divergence.
9. The premise that the old code latched forever. Correct. spawn_supervised does not restart a clean completion (hytte-reactive/src/supervisor.rs:89-93, pinned by clean_completion_runs_exactly_once at :169), so the old return ended the task for the process lifetime.
10. Was the deleted comment really false? Yes, and worse than the PR claims. "no NM is present either" was never established on the dominant path: :259 returns Networkd without ever calling probe_backend().
11. Wall-clock cadence vs fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634's doc. fix(wifi): retry an inconclusive backend probe instead of latching "no backend" (#613) #634 documents a per-attempt wall-clock caveat that fix(networkd): retry a transient startup refresh failure instead of going inert (#621) #645 omits, and I expected that to be a divergence — it is not. wifi's caveat exists because probe_backend queries the bus daemon itself, so a down socket is its failure mode; networkd's failing call is ServiceUnknown from a live bus, which fast-fails, making the flat "one refresh every 8s" accurate for the case that actually reaches this arm. Withdrawn.