fix(readiness): stop reporting weft's own failures as the user's tests failing - #216
fix(readiness): stop reporting weft's own failures as the user's tests failing#216SoloJiang wants to merge 2 commits into
Conversation
…s failing
Three defects on the verification path, all of the same shape: something
that was NOT a statement about the user's code was recorded as one.
`run_bounded_check` reported a `spawn()` failure as
`Completed{status:"fail"}`. When weft's own exec fails — a missing
binary, a permission, or `fork`/`posix_spawn` returning EAGAIN because
the machine is out of process slots — it told the user their tests were
red. `NotProduced` is the arm that already means "no verdict was
obtained", and it does not fail open: readiness reads it as not-ready.
Every neighbouring early return in that function already went there.
`GitSignatureProbe::sample` started its wall-clock budget BEFORE waiting
on a process-global 4-permit semaphore, so time spent queued was charged
to the Git work. A probe that queued too long returned the same error a
hung `git` returns, `.ok()` turned it into `None`, and readiness read
"the worktree changed" from what was really "the machine was busy".
Admission and execution are now separate budgets: a probe still cannot
queue forever and the fan-out is still capped, but one that gets
scheduled gets its full budget.
A child that dies on a signal on the COMPLETED path is the opposite case
and stays `fail`: it ran, and a segfault or an OOM kill is a real red
verdict. The line is whether a process ran at all.
Together these are what named a different readiness test on macOS CI
almost every run — non-deterministic and never the same test twice,
because which check loses its spawn is whichever one runs when the
runner is tightest.
Also splits `noisy_check_streams_a_bounded_tail_before_its_deadline`,
which asserted a claim about output CONTENT while gating it on a 50ms
wall clock: passing the 2 KB budget takes the printf loop under a
millisecond once scheduled, but 50ms could not reliably clear `sh` being
spawned at all. The truncation semantics move to a pure unit test
decided by arithmetic — coverage that did not previously exist in any
form — and the integration test keeps the streaming assertion with a
deadline sized against process-spawn latency.
`proc_registry::real_child_test_lock` serializes the tests that spawn
real children, so one test's tree-aware reap cannot kill another's.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
`spawnWorker` was `driveDirection` minus its `status !== "exited"` filter and minus the pruning of the stale entry, so it treated an EXITED session as an occupied slot and returned without ever calling `chatOpenWorker`. A lane whose worker had exited could not be restarted by the path whose whole job is starting workers. No caller wanted that, so the copy is deleted rather than repaired in parallel and its call sites go through `driveDirection`, which is idempotent for a live slot. That made `dispatchDirection` and `reviveDirection` identical, so they now share one implementation and keep their distinct names for the call sites' sake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d4fa671df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Ok(BoundedCheckOutcome::NotProduced { | ||
| output_tail: format!("could not run {}: {error}", check.program), | ||
| })); | ||
| }); |
There was a problem hiding this comment.
When spawning fails because a binary is missing, permissions deny execution, or the host returns EAGAIN, this new NotProduced outcome is still converted by run_checks_with_timeout_report into a CheckResult with status: "fail"; record_verification_evidence then persists it with collection_state: "ok" and a summary such as 0/1 checks passed. The lane verdict becomes unknown, but the user-facing evidence ledger still records a fresh failed test—the exact misreport this change intends to eliminate. Omit unavailable results or represent them distinctly through aggregation, and cover that path at the report/evidence level rather than only testing run_bounded_check.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the finding is correct, and it is not fixed on this branch. Verified on 2d4fa67:
run_checks_with_timeout_report,NotProducedarm →status: "fail"(readiness.rs:3048)- summary →
{passed}/{total} checks passed(readiness.rs:3278) collection_state: EVIDENCE_COLLECTION_OKhardcoded on every verification row (readiness.rs:3312)
One correction to the framing: the ledger misreport predates this PR. All three lines exist unchanged in main (2910, 3137-3142, 3174), where NotProduced is already reached by check timeouts and zero-rung suites. What this PR changes is that spawn failures now reach that same path — so it fixes the verdict and widens the reach of a pre-existing ledger defect. It does not introduce it.
The fix, and a trap worth recording before anyone writes it:
Do not identify "never started" by code == -1. A child killed by a signal has status.code() == None and also yields -1 on the completed path. Sniffing the code would reclassify a real segfault or OOM kill as "did not run" — destroying exactly the distinction this PR draws. The signal must come from the BoundedCheckOutcome variant.
Concretely:
CheckResult.statusbecomes an enum (pass/fail/not_produced) rather than a bareString, so the ~6 read sites are compiler-checked instead of string-compared. The doc contract at check.rs:21 currently says"pass" | "fail".- readiness.rs:2960's
result.status == "fail"must become an exhaustive match — as an equality test it treats a third value as not-a-failure and would fall through toCheckEvidence::Passed, which fails open. That is worse than today's bug and is the natural place to walk into it. - The summary needs a three-way partition. Adding a third value alone changes nothing there —
not_producedis still!= "pass", so the row still reads0/1 checks passed.rc.checks.is_empty()at :3193 already has an honest "no checks were produced" string to mirror. collection_statemust be derived per repo from that repo's own outcome, mirroringrecord_execution_evidenceat :1947-1951. Keying it off the whole-report aggregate would mark every repo's row unknown because one repo failed. Stamping itunknownthen makes the existing fail-closed chain —evidence_freshness(store/repo.rs:8733) →evidence_row_dto→change_set::evidence_scan— do the right thing with no new code.
Since the defect is in main and reached today by timeouts, I'd land it as its own PR rather than growing this one: the enum change crosses into the frontend (deriveTestsKind, WorkspaceKanban card urgency and progress-bar color), which is a different review surface from this PR's spawn/probe fixes.
Flagging one operational cost of leaving this PR unmerged: the flake it fixes is currently reddening unrelated PRs — #218 hit bounded_check_evidence_keeps_observed_failures_sticky (Failing vs Passed) on macOS for exactly this reason.
Generated by Claude Code
2 participants
Extracted from #215, where these fixes were trapped inside an ~8500-line permission feature they have nothing to do with. They stand alone, they fix things affecting users today, and they are the root cause of the recurring macOS CI flake.
The shape of it
Three defects on the verification path, all the same shape: something that was not a statement about the user's code was recorded as one.
A check that never started was reported as a failing check.
run_bounded_checkreturnedCompleted{status: "fail"}on aspawn()error — so when weft's own exec fails (a missing binary, a permission, orfork/posix_spawnreturningEAGAINbecause the machine is out of process slots), it told the user their tests were red.NotProducedis the arm that already means "no verdict was obtained", and it does not fail open: readiness reads it as not-ready. Every neighbouring early return in that function already went there; this one was the exception.Queue time was charged to the Git work.
GitSignatureProbe::samplestarted its wall-clock budget before waiting on a process-global 4-permit semaphore. A probe that queued too long returned the same error a hunggitreturns,.ok()turned it intoNone, and readiness read "the worktree changed" from what was really "the machine was busy". Admission and execution are now separate budgets: a probe still cannot queue forever and the fan-out is still capped, but one that gets scheduled gets its full budget.A signal on the completed path stays a failure, and that is the deliberate other half. A child that ran and died on a segfault, an abort or an OOM kill is a real red verdict about the user's code. The line is whether a process ran at all.
Why the macOS flake never repeated the same way
Because which check loses its spawn is whichever one runs when the runner is tightest. Under parallel test load a trivially-passing
exit 0check intermittently spawn-failed and was read asFailing, so a different readiness test was named almost every run. That is why it looked like flakiness rather than a bug, and why--test-threads=1always passed.Test-only changes
noisy_check_streams_a_bounded_tail_before_its_deadlineasserted a claim about output content (the tail is marked truncated) while gating it on a 50 ms wall clock. Passing the 2 KB budget takes theprintfloop well under a millisecond once it is scheduled — what 50 ms could not reliably clear isshbeing spawned at all. The truncation semantics move tooutput_tail_buffer_marks_only_the_writes_that_actually_discarded_bytes, a pure unit test decided by arithmetic with no process and no clock (coverage that did not previously exist in any form), and the integration test keeps the streaming assertion with a deadline sized against process-spawn latency rather than against the loop.proc_registry::real_child_test_lockserializes the tests that spawn real children, so one test's tree-aware reap cannot kill another's.Unrelated frontend fix, same extraction
spawnWorkerwasdriveDirectionminus itsstatus !== "exited"filter and minus the pruning of the stale entry, so it treated an exited session as an occupied slot and returned without ever callingchatOpenWorker— a lane whose worker had exited could not be restarted by the path whose whole job is starting workers. No caller wanted that, so the copy is deleted rather than repaired in parallel. That madedispatchDirectionandreviveDirectionidentical, so they now share one implementation and keep their distinct names for the call sites' sake.Verification
cargo test --lib— 2161 passed. Three failures are environmental to the sandbox and reproduce on the base commit: it runs as root, socheckpoint::mid_restore_failure_rolls_everything_backcannot make a file undeletable viachmod 0o555, and twoproc_registryreap tests depend on container process-group semantics. A fourth,completed_check_reaps_a_setsid_child_after_the_direct_parent_exits, passes in isolation — sandbox load, recorded rather than dropped.cargo test --lib readiness::— 73 passed.pnpm build— clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Xz1Ts3AK1uNbeWZT6YFbzz
Generated by Claude Code