ci(nightly): make the cross-PR regression net able to finish, and unable to die quietly (#2462) - #2466
Conversation
…ble to die quietly `backend-unit-nightly.yml` has been cancelled every night for at least twelve consecutive nights — the whole queryable history — and has produced no result for any mergeable PR in any of them. The budget never fit the work. Each leg ran SIX full unit suites (three seeds x base and head) inside `timeout-minutes: 25`; one suite is ~15 minutes, so a leg needed ~90 minutes of a 25-minute budget. Every leg for a real PR died at the cap at 25m19s; the only legs that "succeeded" finished in 46 seconds, and those are the merge-conflict short-circuits — i.e. exactly the ones that ran nothing. Nobody noticed because the workflow is structurally incapable of reporting its own death: it is scheduled, so a cancel notifies no one and blocks no PR, and the diff step is gated on the suite step completing, so a capped leg produces no artifact, no diff and no comment. Silence and success are the same shape from outside. This is not flakiness — no leg for a mergeable PR has ever completed at the current suite size. Four changes: 1. A leg is now ONE seed. The matrix fans out over (PR, seed) instead of (PR), so a leg runs base+head for one seed — two suites, which fits a budget, and a capped leg costs one seed of one PR instead of that PR's whole verdict. Base and head for a seed stay in the same workspace, so the regression diff still runs in the job holding the trusted stashed copy of the script rather than needing a fourth job that downloads junit artifacts from an untrusted one. 2. The suites run under xdist (#2461's change, ~9 min/suite measured on these runners against ~15 serial), and finally get `--timeout=300 --timeout-method=signal` — #2019 added that to the PR-blocking workflow after four anonymous cancels in one day and this workflow never got it, so one stalled test consumed a whole leg and died without naming itself. 3. The seed set has ONE definition (`NIGHTLY_SEEDS`), read by both the matrix builder and the completeness rule that consumes it. Restating it in either place is how a fourth seed silently makes every PR permanently unverified, or a removed one silently lowers the bar. 4. It can no longer pass while saying nothing. A sweep that produced no status artifacts while open PRs existed — the exact signature of the twelve dead nights — now `core.setFailed`s. The sweep may say nothing about a PR; it may not say nothing at all and go green. `timeout-minutes: 60` is derived, not picked: ~18 minutes of suite plus ~4 of full-history checkout, merge and install is ~22, and 60 is ~2.7x that, which clears the worst runner starvation actually measured on this repo (1.94x, from #2461's 17.5m shard beside its 9.0m siblings). The residual is stated in the workflow rather than left to be rediscovered: a pathological ~3x runner still reaches the cap, and what changed is that it now costs one seed, the PR is reported UNVERIFIED rather than skipped in silence, and a wholesale failure turns the run red. Because legs are per-seed, a PR's verdict is the union of its seeds, and that aggregation is the one new path that can publish a false ALL-CLEAR. It is therefore a module — `scripts/ci/nightly-verdict.js`, CommonJS because `actions/github-script` loads its body under `require()` — with 9 vitest cases, rather than inline prose no test can execute. Its rule extends #2029's: an INCOMPLETE set is not a clean one, so two green seeds and one that never reported yields `unverified` and leaves any existing sticky untouched, never a tick. Any single regressed seed condemns, because pytest-randomly is here precisely so an order-dependent failure shows up under one seed and not another — averaging that away deletes the signal the seeds exist to produce. The comment now names the seed, since under randomly the seed IS the reproduction instruction. The `comment` job gains `contents: read` and a pinned, sparse checkout of `dev` to load that module. This does not weaken the architecture rule at the top of the file — untrusted PR code must never share a job with a write-capable token — because `dev` at a pinned ref is the same trust level as the workflow file itself; PR code is still never checked out there. AC 3: each leg now writes what it actually did to `$GITHUB_STEP_SUMMARY`, so a capped leg and a leg that legitimately had nothing to run stop looking identical. The step cannot run when the job is cancelled at the cap, and that absence is itself the distinguishing signal. tests/unit/test_2462_nightly_budget.py pins all four properties, reading the COMMAND rather than the comment (this workflow documents its own history at length, and the prose necessarily contains `timeout-minutes: 25`, `for seed in` and the seed literals — a raw substring check would pass on the description of the bug after the fix was reverted). Eight mutations confirmed red then reverted: cap back to 25; drop `-n auto`; add a second seed definition; replace `setFailed` with `info`; repoint the verdict require; delete the module; make the module ESM; drop the step summary. Fixes #2462
Verifying a change to this workflow otherwise means sweeping every open PR (15 today, 45 legs) and posting bot comments on other people's work to prove your own fix. The demonstration should not have a blast radius wider than the thing being demonstrated. Opt-in and guarded on a non-empty input, so the scheduled sweep is byte-for-byte unchanged — an always-on filter with an empty input would reduce the nightly to nothing, which is this issue's own silence arriving by a different door. Filtered with jq `select` rather than by fetching the single PR, so an id that is closed, targets another base, or does not exist yields the honest `has_prs=false` path instead of a leg that fails deep inside the merge step.
Found by running it. The trusted checkout pinned `ref: dev`, and `dev` does not have scripts/ci/nightly-verdict.js yet, so the comment job died with MODULE_NOT_FOUND on the very run meant to demonstrate the fix. `ref: dev` is wrong beyond that one case: a dispatch from a branch runs THAT branch's workflow against dev's module, so the two can disagree silently. `github.sha` is the same trust level — this workflow has only `schedule` and `workflow_dispatch` triggers, both refs a repo writer chose, so it is the commit the executing workflow definition itself came from — and it cannot skew. PR code is still never checked out in that job. Worth recording what the failed run DID prove: all three test legs completed (15.3m, 16.5m, 32.6m) — the first legs of this workflow to finish in the queryable history — and the run went RED on a real problem instead of ending 'cancelled' with nobody told, which is the fourth property this issue asks for.
|
Validation note from the merge run: the other 8 ready PRs merged to dev today, but this one is held back —
Deterministic (3/3 seeds), introduced by this branch — likely the existing #1941 contract test disagreeing with the new |
AC 1 — demonstrated on real runsThe 13th dead night, observed live while writing thisRun 33502721945 — the unmodified workflow, on Not a historical claim from the API: it happened during this work, exactly as predicted. The fixed workflow, scoped to one PRRun 33508038571, The first legs of this workflow to complete in the queryable history. The comment job read all three status files and logged: which is the correct behaviour — a complete, clean, 3-of-3 verdict, and the "only create on bad news" rule means no comment is posted. Confirmed zero What the measurement says about the cap
The derivation in the workflow said ~22 min/leg; measured is 16–19. The starved case is the one that matters and it came in comfortably under. The first dispatch failed, and that was worth more than a green oneThe first attempt (33504247391) had all three legs succeed (15.3m, 16.5m, 32.6m) and then the comment job died: The trusted checkout pinned It also incidentally demonstrated property 4: the run went red on a real problem instead of ending One thing to know before expecting a fixed nightlyThe scheduled run executes the workflow from |
…tring 'fi'
Caught by the regression diff on this PR, not by review.
`test_an_unknown_merge_verdict_writes_no_status_file` sliced the guarded
branch with `guard.split("fi")[0]`, which truncates at the first occurrence
of those two letters ANYWHERE — including inside ordinary English. The moment
a warning message in that branch used the word "unverified", the slice ended
before the `exit 0` the assertion looks for, and the test reported a missing
guard that was in fact present.
Same family as this file's own comment stripping, and as the #2019 guard's
`_command` helper: a check that matches inside words is testing the prose
rather than the code. Bounded on a line that is exactly `fi` instead.
Mutation-checked: removing the `exit 0` from that branch still reds the test.
vybe
left a comment
There was a problem hiding this comment.
Validated via /validate-pr: all substantive checks green (6 pytest legs, regression diff, e2e, build, gitleaks); comment-job trust boundary verified (checkout of github.sha with persist-credentials:false, sparse to scripts/ci); 15 pinned guard tests + 9 vitest cases, mutation-tested. Fixes #2462 closing keyword present — auto-promotes on merge.
What
backend-unit-nightly.yml— the cross-PR regression net — has been cancelled every night for at least twelve consecutive nights, the whole queryable history, and has produced no result for any mergeable PR in any of them.Night thirteen was observed live while writing this fix (run 33502721945, the unmodified workflow on
main):Why it could never work
Each leg ran six full unit suites (three seeds × base and head) inside
timeout-minutes: 25. One suite is ~15 minutes, so a leg needed ~90 minutes of a 25-minute budget. This is not flakiness — no leg for a mergeable PR has ever completed at the current suite size.And it failed silently by construction: the workflow is scheduled, so a cancel notifies nobody and blocks no PR; and the diff step is gated on the suite step completing, so a capped leg produces no artifact, no diff and no comment. Silence and success are the same shape from outside. The only "green" legs were the ones that ran nothing.
Four changes, all four needed
1. A leg is one seed. The matrix fans out over (PR, seed) rather than (PR), so a leg runs base+head for one seed — two suites, which fits a budget. Base and head for a seed stay in the same workspace, so the regression diff still runs in the job holding the trusted stashed copy of the script, rather than needing a fourth job that downloads junit artifacts from an untrusted one.
2. The suites can finish. xdist (#2461's change, ~9 min/suite measured on these runners vs ~15 serial), and finally
--timeout=300 --timeout-method=signal— #2019 added that to the PR-blocking workflow after four anonymous cancels in one day, and this workflow never got it, so one stalled test consumed a whole leg and died without naming itself.3. One definition of the seed set (
NIGHTLY_SEEDS), read by both the matrix builder and the completeness rule that consumes it. Restating it is how a fourth seed silently makes every PR permanently unverified, or a removed one silently lowers the bar.4. It can no longer pass while saying nothing. A sweep that produced no status artifacts while open PRs existed — the exact signature above — now
core.setFaileds. The sweep may say nothing about a PR; it may not say nothing at all and go green.The budget is derived, and its residual is stated
~18 min of suite + ~4 of full-history checkout, merge and install ≈ 22.
timeout-minutes: 60is ~2.7× that, clearing the worst runner starvation actually measured on this repo (1.94×, from #2461's 17.5m shard beside its 9.0m siblings).A pathological ~3× runner still reaches the cap. What changed is blast radius and visibility: it now costs one seed of one PR instead of that PR's whole verdict, the PR is reported unverified rather than skipped in silence, and a wholesale failure turns the run red.
The one new path that can lie
Because legs are per-seed, a PR's verdict is the union of its seeds — and that aggregation is the one place a false ALL-CLEAR can be published. So it is a module,
scripts/ci/nightly-verdict.js, with 9 vitest cases, rather than inlinegithub-scriptprose no test can execute. CommonJS on purpose:actions/github-scriptloads its body underrequire().Its rule extends #2029's — absence of a verdict is its own state — to the seed level:
unverifiedand leaves any existing sticky untouched, never a tick;The
commentjob gainscontents: readand a pinned, sparse checkout ofdevto load the module. This does not weaken the rule at the top of the file — untrusted PR code must never share a job with a write-capable token — becausedevat a pinned ref is the same trust level as the workflow file itself, and PR code is still never checked out there.AC 3 — capped vs nothing-to-run
Each leg writes what it actually did to
$GITHUB_STEP_SUMMARY. The step cannot run when the job is cancelled at the cap, and that absence is itself the distinguishing signal.Verifying a workflow shouldn't spam the team
workflow_dispatchgained an optionalpr_number. Without it, proving a change to this workflow means sweeping all 15 open PRs (45 legs) and posting bot comments on other people's work. Opt-in and guarded on a non-empty input, so the scheduled sweep is byte-for-byte unchanged — an always-on filter with an empty input would reduce the nightly to nothing, which is this issue's own silence arriving by a different door.AC 1 is demonstrated with it: a scoped dispatch against #2465 (my own PR) — results in the comment below.
Guards
tests/unit/test_2462_nightly_budget.py(15 tests) pins all of it, reading the command rather than the comment — this workflow documents its own history at length, so the prose necessarily containstimeout-minutes: 25,for seed inand the seed literals, and a raw substring check would pass on the description of the bug after the fix was reverted.Nine mutations confirmed red then reverted: cap back to 25 · drop
-n auto· add a second seed definition ·setFailed→info· repoint the verdict require · delete the module · make the module ESM · drop the step summary · make the PR filter unconditional.Fixes #2462