test(wifi): one generous D-Bus reply budget for nm_agent's system tests, not 12 tight ones (#676) - #677
Merged
Merged
Conversation
…ts, not 12 tight ones (#676) Replace 12 duplicated `Duration::from_secs(3)` literals in wifi/nm_agent.rs's system_tests module with one shared DBUS_REPLY_BUDGET const (30s), doc-commented with the reasoning so the next person who thinks "30s seems excessive" reads it before tightening. These timeouts are liveness guards, not latency assertions: nothing in this file claims a GetSecrets round trip should complete within any particular time, only that a hung call must not wedge nix flake check forever. That check runs the same clippy/test invocation alongside two nixosTest VMs and package builds, so CPU contention is the normal condition, not an edge case. A tight budget buys nothing there and costs false reds, as #676 demonstrated on a markdown-only PR. Closes #676
This was referenced Jul 30, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
wifi/nm_agent.rs'ssystem_testsmodule wrapped every real D-Bus round trip intokio::time::timeout(Duration::from_secs(3), …)— 12 copies of the same literal. One of them (non_wireless_setting_is_rejected) flaked CI on a markdown-only PR (#672) withGetSecrets did not return in time: Elapsed(()). Nothing was wrong; something was slow, becausenix flake checkruns this suite in the same invocation as twonixosTestVM builds, full workspace clippy, and package builds — CPU contention there is the normal condition, not an edge case.This replaces all 12 literals with one shared
const DBUS_REPLY_BUDGET: Duration = Duration::from_secs(30);, doc-commented with the framing that made the fix obvious:These timeouts are liveness guards, not latency assertions. No test in this file is trying to prove a D-Bus round trip completes within some duration — that would be a meaningless claim about a sandboxed builder under load. Their only job is to stop a genuinely hung call from wedging the suite (and
nix flake check) forever. A real regression still fails, just later rather than sooner. Raising the budget weakens no assertion; it only reduces how often the suite lies about a real regression. The doc comment on the const spells this out explicitly so the next person who looks at30and thinks "that seems excessive" reads the reasoning before tightening it back down.Why 30s
Comfortably above worst-case CI contention (the failing run needed more than 3s but the timeout, not the call, was the bug — 30s gives an order of magnitude of headroom over a budget that was already demonstrably too tight under load), comfortably below "painful to notice" if a call is genuinely hung (a wedged test still fails within half a minute, not hangs indefinitely). Not a precisely-derived number — the doc comment says as much and invites raising it further if it ever flakes again.
Count verification — 12 replaced, 0 left alone
I checked each of the 12 occurrences individually rather than blanket-replacing:
ephemeral_bus()'s wait for the spawneddbus-daemonto print its listen address on stdout (startup handshake, itself subject to the same CPU contention as everything else in the suite).await_prompt()deadlines (pollsNmAgent'spromptsMutableuntil the agent surfaces aPromptRequest, acrosshappy_path,vpn_happy_path,wep_hint, andcancel_resolves).tokio::time::timeout(...)around an actualGetSecrets/CancelGetSecretsD-Bus call or itsJoinHandle.All 12 are "did this eventually happen" liveness guards over real D-Bus/process I/O — none are latency assertions, and none are unrelated sleeps. Nothing was left alone because nothing in this file's
Duration::from_secs(3)occurrences was the wrong shape for this fix.Also updated the module-level doc comment above
mod system_tests(previously said a marshalling regression "fails fast" — now says it fails bounded, not fast, and points at the const for the reasoning) so it doesn't contradict the new budget.Stability evidence
Ran the actual gated bucket (
cargo test --workspace --features system-tests) underxvfb-run(needed forhytte-ui's GTK unit tests — see below) three times back to back after the change: 53/53 test binaries green, 0 failures, all 6wifi::nm_agent::system_tests::*passing every run.cargo clippy --workspace --all-targets --features system-tests -- -D warnings(withRUSTC_WRAPPERunset) is clean.nix fmtmade zero changes.(Note for anyone reproducing locally: a plain
cargo test --workspace --features system-testswithoutxvfb-runfails early inhytte-reactive's GTKbindtests with "Failed to initialize GTK" — no display server — and never reacheshytte-services. That's expected per this repo's own docs, not a regression; wrap inxvfb-runasCLAUDE.mdsays.)Sibling-bucket sweep (#676 asked for this) — report only, no fix
Grepped
crates/hytte-bus/tests/,crates/hytte-reactive/, andcrates/hytte-ui/for the same shape (timeout(...)/sleep(...)wrapping real I/O in a gated system-test bucket).hytte-reactiveandhytte-uihave no matches — their gated tests are the GTKmod testsblocks compiled undersystem-testsbut don't wrap D-Bus/process I/O in a fixed budget.hytte-bus/tests/does have the same class, un-fixed by #451:crates/hytte-bus/tests/common/mod.rs:87— the sharedephemeral_bus()helper (used by everyhytte-busintegration test file) has the exact sametokio::time::timeout(Duration::from_secs(3), lines.next_line())waiting on the spawneddbus-daemon's startup handshake that this PR just fixed innm_agent.rs's copy of the same helper. Since it's shared, one fix here would coverown.rs,property.rs,proxy.rs,signals.rs,export.rs,connection_reconnect.rs, andcall_fd.rsat once.timeoutonstream.next()), e.g.own.rs:168/324,property.rs:61-381,proxy.rs:29-157,signals.rs:108. These retry within their deadline so they're more resilient than a flat single-shot timeout, but the outer deadlines (Duration::from_secs(1)/(2)/(5)) are tighter than what this PR just judged necessary for nm_agent's flat timeouts, and under the samenix flake checkcontention they're plausibly exposed to the same flake class.proxy.rs:100(tokio::time::sleep(Duration::from_secs(3))inside aSlowpoke::slow()test double) is a deliberately-slow method the test uses to prove a real proxy timeout fires — not a liveness guard, and not in scope for this class of fix.I did not touch any of these files — the issue asked for a report so you can decide whether a follow-up is worth filing, and keeping this PR to the one file it names keeps it obviously safe to review.
Closes #676