docs(contributing): do not cancel the CI run of the PR being merged (4 PRs through this hole in one day) - #9256
Conversation
A cancelled job is neither a pass nor a failure, and two protections go quiet together: pr-gate never reports (so the required context is absent rather than red, which is what invites the bypass), and the changelog fragment check — a step inside lint, conditioned on pull_request — is skipped silently, so the omission stays invisible until release notes are cut. Both were observed on the same day. PerryTS#9169 merged with lint failing and five jobs cancelled, breaking method dispatch and property lookup on main for four and a half hours (PerryTS#9247). PerryTS#9215, PerryTS#9230 and PerryTS#9235 each merged with lint CANCELLED; all three touched crates/, none carried a fragment, and the work is absent from its release notes. States explicitly that the gate is correct and should not be changed: gate in test.yml runs if: always() and treats cancelled as failure, exactly so a cancelled dependency cannot read as green. Every incident has been a bypass of a working gate. Docs only.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesCI Cancellation Guidance
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… red' A gate that never ran is absent from the status list, so it reads as clean under any failure filter — the same way CANCELLED does. 'pr-gate: pass' is a positive assertion that the fan-in ran and every dependency was success or skipped; '0 failing' is satisfied equally by a PR whose gate never executed. Extends the note to release automation, where the same hole exists one level up: a skipped or absent required context satisfies 'not failing', so the dispatch condition has to require conclusion == success.
|
Merged. I am the proximate cause of half the incidents this documents, so let me confirm them rather than merge quietly past them. #9169 is mine. I merged it after a local #9215, #9230 and #9235 are also mine, and the missing-fragment half is accurate. All three touched The distinction in the last paragraph is the part I'd underline for anyone skimming: Also right, and worth resisting: "the gate logic itself is correct and does not need changing." The one thing I would add if you take a follow-up: the release-automation hole named at the end deserves its own check rather than a sentence, because it is the same bug one level up and nobody reads a paragraph at 2am. A required context that is |
Docs only, one paragraph in CONTRIBUTING.md. Raised by @perry-99 during the v0.5.1519 freeze; filed as a PR so Ralph can rule on the policy.
Four PRs through one hole in a single day, confirmed from both ends
lintfailing andcheck/cargo-test/gap-suite-build/gc-stress-build/e2e-scopedcancelled. Its gap shards never ran; it broke method dispatch and property lookup onmainfor four and a half hours, until fix(runtime): a prototype-override receiver must not lose synthesized methods #9247.lintCANCELLED —status=COMPLETED conclusion=CANCELLEDon all three,pr-gateabsent from the rollup entirely. All three touchcrates/, none carries achangelog.d/fragment, and the work is consequently missing from v0.5.1519's release notes (docs(changelog): record the packed-loop throw fast path shipped in v0.5.1519 #9255 records it for the next set).Broken code in through one end, release-note fragments silently dropped out through the other — one mechanism, observed twice, on the same day. One occurrence is an accident; four is a hole.
Those three are mine, which is how the second failure mode surfaced: I went looking for why my own fragments were missing and found that the check enforcing them never ran. That makes it a process defect rather than a contributor error — the convention cannot be violated knowingly if nothing reports the violation.
The rule
Cancelling other runs for capacity is fine and expected on a 20-slot org. Cancelling the run of the PR you are about to merge is not, because a cancelled job is neither a pass nor a failure, and two protections go quiet at once:
pr-gatenever reports — the single required context is absent rather than red, which is what invites the admin bypass, and the bypass is what lands the change.lintconditioned ongithub.event_name == 'pull_request', so a cancelledlintskips it silently, and the omission stays invisible until the notes are cut.The check to teach:
pr-gatepresent and passingNot "nothing is red". Those are different assertions and only the first is worth anything:
CANCELLEDis neither pass nor fail. Both read as clean under any check that looks for failures.pr-gate: passis a positive statement that the fan-in ran and every dependency wassuccessorskipped. "0 failing" is satisfied equally well by a PR whose gate never executed.I made exactly that mistake, twice over: I reported one of these PRs as "29 checks, failing: none" while its
lintsat cancelled. @perry-99 audited their release tooling against this and has made the dispatch condition requirefull-suite-gateconclusionsuccessplus zero cancelled jobs — the same hole exists one level up, because a skipped or absent required context also satisfies "not failing".What this deliberately does not propose
No change to the gate.
gatein.github/workflows/test.yml(line ~3746) already runsif: always(), and its documented verdict is "every other needed job must besuccessorskipped;failureandcancelledfail" — precisely so a cancelled dependency cannot read as green. Every incident here is a bypass of a gate that works. The text says so explicitly, so nobody reads this and goes hunting for a gate bug.Refs #9169, #9247, #9215, #9230, #9235, #9255.
Summary by CodeRabbit