fix(networkd): a transient refresh failure at startup returns before the retry loop, latching LinkSource::Unknown for the process lifetime

[vibechoom]

Found by an adversarial combined-tree audit of 9c9a1d8..269a6d4; survived two independent refuters and re-verified in 269a6d4.

Filing rather than fixing — the current behavior is deliberate and commented as such, so changing it is a design call, not a chore. See the design question at the bottom.

Where

crates/hytte-services/src/networkd.rs:340-345, inside the LinkBackend::Networkd arm:

if let Err(e) = refresh(&links_writer, &primary_writer, &source_writer).await {
    tracing::info!(error = ?e, "networkd unreachable at startup; service inert");
    return;
}
loop {
    match listen(&links_writer, &primary_writer, &source_writer).await {
        ...
    }
    tokio::time::sleep(Duration::from_secs(2)).await;
}

The return is before the retry loop. A single failed refresh at startup ends the task for the entire process lifetime. source stays LinkSource::Unknown, so the panel shows "No link manager has answered yet" forever and no retry ever happens — the 2 s retry loop below is unreachable from this path.

Why that's a bug and not just a policy

The surrounding comment justifies it:

isn't running on this host and no NM is present either — log once at info and stay inert rather than hammering dbus in a 2s retry loop for the rest of the process lifetime.

But this code is inside the LinkBackend::Networkd arm — reached only after the backend probe already determined networkd is the backend on this host. The "networkd isn't running here" case is LinkBackend::None, which is handled separately at :357-361 and correctly publishes LinkSource::Unavailable.

So the guard defends against a condition that the probe has already ruled out, while catching the condition it should tolerate: a transient refresh failure microseconds after the probe's own call succeeded (networkd restarting, a D-Bus hiccup, ServiceUnknown during systemctl restart systemd-networkd). One unlucky moment at boot and the network panel is permanently wrong until the shell is restarted.

Note this is the same class as #613 (wifi backend selection not re-entrant, so a transient probe failure needs a restart to recover) — one layer over, in networkd rather than wifi_backend. #613 is currently held deliberately: #609's error! line exists to measure how often the transient failure fires, and shipping a retry destroys the measurement before it's taken. That reasoning applies to wifi_backend; whether it also applies here is the open question.

A related refuted claim, recorded so nobody re-files it: the audit also proposed that LinkSource::Unavailable is only reachable from a failed networkd read. Two independent refuters killed that on the source — Unavailable is published from the LinkBackend::None arm as a positive finding, as documented. That part is fine.

Severity

Medium. Narrow trigger (a failure in a window of microseconds), but the consequence is total and silent for the rest of the session: the link panel is stuck on an indeterminate state with no recovery short of restarting the shell.

Design question for @annikahannig

Two defensible shapes, and I don't want to pick unilaterally:

1. Retry here too — move the refresh failure into the loop so it retries every 2 s like listen already does. Simple, symmetric with listen's existing behavior, and the "don't hammer dbus" worry is largely moot because the probe already told us networkd is present. Risk: on a pathological host we do poll a dead service every 2 s forever.
2. Bounded retry, then inert — try N times with backoff, then give up and log at error! (not info!) so the give-up is visible in journalctl, matching the pattern fix(wifi): a failed backend probe is no longer indistinguishable from "no backend" (#607) #609 established for an inconclusive probe.

I lean (2): it fixes the transient case, keeps a ceiling on D-Bus traffic, and — like #609's error! line — leaves evidence in the log that this path fired, which is what would let us close #613 on data rather than guesswork. Happy to build either.

Refs #608, #609, #610, #613.

[vibechoom]

Cross-linking: this question just became load-bearing for a second subsystem.

Mara confirmed on #607 that she hit the wireless equivalent in the wild (a restart cured it, no recurrence since), which lifted the evidence hold on #613. I've scoped #613 to its contained half — retry the wireless backend probe before committing, which needs no cancellation because nothing has been spawned yet — and dispatched it. The invasive half is now #633.

That build needs a retry policy, and it is the same question this issue asks for networkd's startup refresh. The two should not diverge: a user debugging one and reading the journal from the other shouldn't find two different behaviours for the same class of transient D-Bus failure.

So I've told the #613 builder to implement my stated lean here — bounded retry with backoff, then give up loudly at error! — and to structure it so the policy is a small localised edit rather than a rewrite, precisely so your answer can ratify or override it cheaply.

Nothing is foreclosed. If you prefer retry-forever, say so and both sites change together. The only thing I've decided unilaterally is not to leave Mara's bug unfixed while the question sits open — and I picked the conservative option, since a bounded retry that logs its give-up is strictly more debuggable than one that polls a dead bus in silence.

This issue itself stays unbuilt and unblocked-on-me: networkd.rs:340-345's return-before-the-retry-loop is still there, and the LinkBackend::Networkd arm is still reached only after the probe has already established that networkd is the backend, which is what makes the early return wrong.

[vibechoom]

Retracting my lean on this issue — it was wrong, and the answer is already in the tree.

I recommended bounded retry then give up loudly, and carried that into #613's build. An adversarial review of the resulting PR (#634) found the counter-example in the very file I told the builder to copy the shape from.

crates/hytte-services/src/networkd.rs:283-289, on the inconclusive-probe path:

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.

So networkd already answered this question, and chose never-go-inert — it commits optimistically and lets the self-healing watcher sort it out. I proposed the opposite policy for its sibling without noticing the file had reasoned it through first.

Why bounded is wrong here specifically

A bound means that whenever the bus takes longer than the bound to come up, the user lands in exactly #607's reported state — Wi-Fi section gone, systemctl --user restart trollshell the only cure. That is the bug the work closes, reintroduced on a longer fuse. And both wireless watchers self-heal on their own (wifi_nm.rs:1050 re-polls discovery every 5s; wifi/watcher.rs:57's 'discovery: loop), so there is nothing to protect by stopping.

The review also caught that my stated justification was circular: "the give-up is logged at error! precisely so a wrong bound is visible in the journal" argues for having a bound from the visibility of the bound being wrong. An unbounded retry that logs periodically gets identical visibility with no latch.

One thing my reasoning did get right, and it survives: the retry must stay observable. A quiet self-heal would trade a visible permanent bug for an invisible intermittent one. Unbounded-plus-periodic-error! satisfies that; bounded was never required for it.

What this means for this issue

#634 is switching to unbounded retry. For consistency, networkd's :340-345 early return should go the same way — retry rather than go inert — which is now the conservative option rather than the adventurous one, since it matches what the same file already does 60 lines above on the sibling path.

Worth noting the two sites differ slightly in what "keep trying" means: networkd's LinkBackend::Networkd arm has already established that networkd is the backend, so it should retry refresh and enter its existing listen loop. There is no backend-choice ambiguity to resolve, which makes it simpler than the wifi case, not harder.

@annikahannig — this no longer needs a decision from you unless you disagree. The tree's own precedent settles it, and I'd rather flag that I got it backwards than have you arbitrate a question that was already answered. Still worth your veto if you want the bound for a reason I haven't considered.