fix(ci): bound telemetry_kill_switch_dispatch so it stops flaking on Ubuntu - #6270
Conversation
…Ubuntu
`codewhale-cli::telemetry_kill_switch_dispatch::missing_preference_defaults_on_without_inventing_acceptance`
has failed twice on Ubuntu CI at `telemetry_kill_switch_dispatch.rs:180`
("default-on writes dry run") on changes that touch neither telemetry nor
the CLI: PR #6104 (`c810bc0458`, 2026-09-12) and PR #6267 (`75e032b61d`,
2026-09-16). Both times macOS and Windows passed.
It is not an env race. CI runs cargo-nextest, which gives every test its own
process, and the test never mutates process-global state — each case spawns
the real binary with `env_clear()` and a fresh `TempDir` HOME/CODEWHALE_HOME
(`telemetry_kill_switch_dispatch.rs:231-276`). The panic is a *missing file*:
`$CODEWHALE_HOME/telemetry/dryrun.jsonl` was never written.
The mechanism is a wall-clock deadline. `features list` resolves to
`Surface::Cli`, whose exit path waits `CLI_PERSIST_TIMEOUT` — 250 ms
(`crates/telemetry/src/lib.rs:75`) — for a detached writer thread that must
re-run `decision::re_decide` against disk and then fsync an append before the
process exits. The code deliberately fails open, so a missed deadline silently
produces no receipt.
All three existing test-group overrides filter `binary(integration)`, and this
is a *different* binary, so none of them ever matched it: the five cases ran at
full parallelism beside 15,774 tests. The CI log shows the runner was saturated
at that moment — neighbouring subprocess-spawning tests took 3.5-3.6 s for work
that normally finishes well under a second, while this one failed in 0.747 s.
This adds the missing override, putting the binary in the existing
`telemetry-contract` group (max-threads = 1) for the same reason that group
exists: these tests spawn the real binary and cannot absorb scheduler latency.
Verified on this machine (macOS aarch64):
cargo nextest show-config test-groups -p codewhale-cli --all-features \
--locked -E 'binary(telemetry_kill_switch_dispatch)'
group: telemetry-contract (max threads = 1)
* override for default profile with filter
'binary(telemetry_kill_switch_dispatch)':
codewhale-cli::telemetry_kill_switch_dispatch: (all 5 tests)
group: spawns-binaries (max threads = 3) (no matches)
sh scripts/with-hermetic-test-home.sh cargo nextest run -p codewhale-cli \
--all-features --locked -E 'binary(telemetry_kill_switch_dispatch)'
Summary [0.621s] 5 tests run: 5 passed, 0 skipped
Not fixed here: the 250 ms budget itself. Bounding the group removes the load
that makes the deadline reachable, but a slow enough host can still miss it.
The product-side options — raise CLI_PERSIST_TIMEOUT, hoist the `re_decide`
disk read out of the deadline window, or have the CLI path join the writer for
the local non-network case — are a behavioural decision, not a test fix, and
are left for the issue.
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 1m 6s —— View job Review: bound
|
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 1 file patches, sha256:adb97c79f53eaf8b043e8c360ed3cab72500da773ddaad23d11790c270d51d5a.
Pass 1: Single-file CI config change: a new [[profile.default.overrides]] entry in .config/nextest.toml binds the telemetry_kill_switch_dispatch test binary to the existing telemetry-contract test group (max-threads = 1), with an explanatory comment. The addition is syntactically well-formed TOML, references a group that exists in the same file, and is order-independent with respect to the other overrides because its filter is disjoint from theirs.
Assessment
Pass 1: No defects found in the changed lines. Static inspection of the diff plus the supplied file context shows: (1) the new table is valid TOML ([[profile.default.overrides]] with filter/test-group string keys), inserted between the existing binary(integration) & test(/^telemetry_contract::/) override and the binary(integration) & test(/^exec_persistent_service::/) override, before the catch-all binary(integration) -> spawns-binaries override; (2) the referenced group telemetry-contract = { max-threads = 1 } is declared at line 32, so the assignment cannot fail config validation; (3) the new filter binary(telemetry_kill_switch_dispatch) is disjoint from every other override filter in the file (all others require binary(integration)), so the file's 'first matching override wins' ordering cannot be disturbed and no existing group assignment is stolen — the change is behaviour-additive only. I did not execute anything (no build, no cargo nextest show-config, no test run); the claim that the filter actually resolves to codewhale-cli::telemetry_kill_switch_dispatch comes only from the untrusted PR transcript, and it relies on the binary-name form already used by the neighbouring overrides. Two open questions that are not defects in this diff and that I am not asserting: (a) the filter is not package-qualified, so if any other crate in the workspace also produced a test target named telemetry_kill_switch_dispatch, those tests would silently join the serialized group as well (low impact, more serialization only); (b) the group bound limits concurrency among the five cases and against the telemetry-contract tests, but tests outside all groups still run at full parallelism, so static review cannot confirm that this alone makes the 250 ms CLI_PERSIST_TIMEOUT deadline unreachable on a saturated runner — the PR acknowledges this residual risk and leaves #6269 open. Both points are outside the evidence available in this diff.
Advisory review by Codewhale (codewhale review --pr 6270 --post, head 03cd07ab6ed4a62e6d127fa505db681915473880). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
Partial fix for #6269 — the test-side half.
codewhale-cli::telemetry_kill_switch_dispatch::missing_preference_defaults_on_without_inventing_acceptancehas failed twice on Ubuntu CI attelemetry_kill_switch_dispatch.rs:180on changes touching neither telemetry nor the CLI: PR #6104 (c810bc0458, 2026-09-12) and PR #6267 (75e032b61d, 2026-09-16). Both times macOS and Windows passed.Not an env race
CI runs cargo-nextest — one process per test — and the test mutates no process-global state: each case spawns the real binary with
env_clear()and a freshTempDirHOME/CODEWHALE_HOME(telemetry_kill_switch_dispatch.rs:231-276). The panic is a missing file, not a wrong value.The actual mechanism
features listresolves toSurface::Cli, whose exit path waitsCLI_PERSIST_TIMEOUT(250 ms,crates/telemetry/src/lib.rs:75) for a detached writer thread that must re-rundecision::re_decideagainst disk and fsync an append before the process exits. It fails open, so a missed deadline silently leaves no receipt.Why no existing group caught it
All three
[[profile.default.overrides]]filterbinary(integration). This is a different binary, so none of them ever matched — the five cases ran at full parallelism beside 15,774 tests. The CI log confirms saturation: neighbouring subprocess-spawning tests took 3.5–3.6 s for sub-second work.This adds the missing override, placing the binary in the existing
telemetry-contractgroup (max-threads = 1), for the same reason that group exists.Verification (macOS aarch64)
Group assignment — before this change the filter matched nothing:
Focused rerun — the failing test passes in isolation, which is what distinguishes flake from regression:
0.182 s here against 0.747 s to fail under CI load.
Not fixed here
The 250 ms budget itself. Bounding the group removes the load that makes the deadline reachable; a slow enough host can still miss it. The product-side options (hoist the
re_decidedisk read out of the window, join the writer for the local non-network case, or raise the constant) are behavioural decisions and stay in #6269.No-Issue: partial fix only — #6269 stays open for the 250 ms CLI_PERSIST_TIMEOUT decision, which is behavioural and not a test-config change.