ci: a newer main push supersedes the run in flight - #1766
mobeenabdullah wants to merge 3 commits into
Conversation
The Integration and CI workflows keyed their concurrency group on `main` by commit, so no push there ever superseded another. Every merge got a full run to completion, Integration being three dialects for twenty to thirty-five minutes, and at the current merge rate the queue could only grow: fifty runs sat queued across the workflows with seven in progress, and nothing on `main` had reported for over half an hour, including the commit that fixed the failure everyone was waiting to see fixed. A verdict that arrives after the next several merges is not a verdict anyone acts on, and every open pull request read as blocked meanwhile. This reverses #688, which chose per-commit grouping deliberately. Its concern was real: a shared group cancelled five of eight main runs, and a required check reporting `cancelled` looked the same as one that failed, which trains people to merge past red. Two things make the two distinguishable now, and both are what let this change be made rather than only wished for. A leg that overruns its budget fails rather than cancels, since #1744. So on `main` a cancelled run means exactly one thing: superseded by a newer push. And `scripts/verify-merge.mjs` now reads the base's current head after a merge. A cancelled job on a merge commit the base has moved past is reported under `superseded`, naming the head revision whose run answers for it, and does not block. A cancelled job on the head itself still blocks, and so does a failure anywhere; supersession excuses nothing that actually failed. Three tests hold those three cases, and the first fails with the branch removed. The middle of three quick merges is displaced before it starts, which #688 counted as a missing verdict. It is the intended outcome here: the newest push's run includes that commit and is the one that describes `main`. Both workflow comments record the decision and the reasoning, and the verifying-merged-work rule says how to read a cancelled job now.
…supersedes-integration
|
@codex review |
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Second reader on the #688 reversal: the reasoning holds. Per-commit grouping bought "cancelled cannot be mistaken for failed" at the price of a queue that cannot drain at this merge rate, and #1744 plus the One hole, small but in the direction a verification gate must not err: |
Supersession alone cannot tell a concurrency cancel from one made by hand. A run somebody cancelled on a merge commit that was later merged past has a newer head and, without more, would be filed as superseded and stop blocking. The witness that separates them is a run of the same job on the superseding head: a concurrency group leaves one behind and a manual cancel does not. So the script reads the head's check-runs when there is a head to read, and files a cancelled job as superseded only if the head has a run of that name. Without it the job stays what it looks like and blocks. One test holds the case, and fails with the witness requirement removed.
|
The manual-cancel case is closed in @codex review |
![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: eeb72b2b65
ℹ️ 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".
|
Closing in favour of a different mechanism, decided with the maintainer after this round. The four findings here are all real, and together they are the signal: each one adds another rule for telling what a The replacement keeps #688's per-commit groups and never cancels anything. Each run asks at START whether a newer run of the same workflow exists on One finding survives the switch and is closed in the replacement rather than deferred: the inert-push hole ( Thank you for the four; each pointed at the same conclusion. |
* ci: a run a newer push has overtaken skips itself The CI and Integration workflows key their concurrency on `main` by commit, so every merge keeps a verdict of its own and nothing is ever cancelled. #688 chose that deliberately, because a cancelled required check looked the same as a failed one. It could not survive the merge rate: each Integration run is three dialects for half an hour, and fifty runs sat queued with nothing on `main` able to report for over half an hour. Cancelling the older run was tried in #1766 and closed. `cancelled` then had to mean superseded, or stopped by hand, or timed out, and every reader of the check needed rules to tell those apart. Review found four more ways it could lie after two were already guarded, and the next round would have found a seventh. That is an overloaded signal, not an incomplete implementation. So nothing is cancelled. A composite action asks, at the START of a run on `main`, whether a newer push already has a run of the same workflow; if so the run's jobs are skipped. A skipped job is already how this repository says "this commit cannot affect me", it already passes the merge gate, and here it is literally true: the newer run's verdict includes this commit. `cancelled` keeps meaning what it says. Queued runs start, ask and stop within seconds, and only the newest tests, so the queue stays short with per-commit groups kept exactly as #688 left them. One hole this opens is closed in the same change. CI's `changes` job diffs a push against `event.before`, the previous commit. Once a code push can skip itself, a README-only push landing after it would diff against that push alone, read as inert, and leave the code untested. The action therefore also names the last commit whose substantive job actually ran, found by walking completed runs newest-first, and `changes` diffs against that on `main`. Integration has no inert path and needs only the first answer. The gate learns supersession as a second reason a skip is acceptable, kept separate from inert so a superseded run is never described as having touched only inert paths. The ceiling guard, which held every job in integration.yml to a floor sized for a suite leg, now holds the jobs that run the budget wrapper; the pre-job asks a question and holding it to an hour's ceiling would let a hung request keep a runner for an hour. Verified the guard still fails when a real leg's ceiling drops. Permissions: `actions: read` is granted to the asking job alone, so no job that builds or tests holds it. * ci: bind the substantive job name in jq and count only verdicts as tested gh api --jq takes one filter and no --arg, so the jobs lookup exited before reading anything and every main push fell back to event.before. The name is now bound by jq over the raw response, only success and failure count as a tested base, and the run listing reads one page instead of the workflow's whole history. * ci: name the skipped integration matrix for what it is and let verify-merge read it A job-level if is applied before the matrix expands, so a superseded run reported one skipped check with an empty dialect and no postgres or mysql check at all, and verify-merge called both required checks absent on every superseded merge commit. The job now names itself 'Integration (superseded: postgres, mysql)' in that case; verify-merge accepts that name, skipped, as the two legs' stand-in and reports each leg it answered for. A test pins the string to the workflow's own text. * ci: run everything when no tested base is known, and skip the comment scan when superseded An unknown tested base fell back to event.before, which is the hole the last-tested-sha output exists to close; an empty base now runs the full workflow. The comment-convention job hung off nothing and kept a runner for up to ten minutes on every overtaken run while the gate waited; it now needs the changes job and skips on supersession alone. * ci: one literal-named integration job per dialect, sharing one setup action A matrix job skipped at the job level reports one check named with its unevaluated name expression, needs and all; measured on a probe run. So the postgres and mysql legs are now two jobs with literal names that survive a skip, each booting only its own database, and the setup steps the three legs shared as copies live in .github/actions/integration-setup. verify-merge needs no stand-in: the required names are present, skipped. * test(scripts): every workflow and local action manifest parses as YAML A job name expression carrying an unquoted ': ' reached a pushed branch and would have loaded the workflow as nothing on main; no local gate reads .yml. Every file under .github/workflows and each action.yml is now parsed with js-yaml, with the offending shape pinned as the negative case. * ci: a newer run covers this one only while it can still reach a verdict A newer run already cancelled or timed out tested nothing, so a run it would have superseded now does its own work. A covering run cancelled later is recovered by the next push, whose diff base is the last commit that reached a verdict. * ci: an integration leg falls open when the supersession job fails GitHub skips every dependent of a failed job, skipped passes the merge gate, and the integration workflow has no aggregate gate of its own, so a failed supersession question would have read as three acceptable required checks with nothing run. The legs now run unless the workflow itself was cancelled.
1 participant
Why main cannot report
The Integration and CI workflows key their concurrency group on
mainby commit, so no push there ever supersedes another. Every merge runs to completion — Integration is 3 dialects × 20–35 min — and at the current merge rate the queue can only grow.Measured at 07:46Z: 50 runs queued across all workflows, 7 in progress, Integration runs from 07:12Z still going, and nothing on
mainhad reported for over half an hour. That included #1761, the commit that fixed the failure everyone was waiting to see fixed. Every open PR read as BLOCKED meanwhile.This reverses #688, on purpose
#688 chose per-commit grouping deliberately, and its concern was real: a shared group cancelled 5 of 8 main runs, and a required check reporting
cancelledlooked the same as one that failed — which trains people to merge past red.Two things make them distinguishable now, and both are what let this change be made rather than wished for:
main,cancelledmeans exactly one thing: superseded by a newer push.verify-merge.mjsreads the base's current head after a merge. A cancelled job on a merge commit the base has moved past is reported undersuperseded, naming the head revision whose run answers for it, and does not block. A cancelled job on the head itself still blocks. Afailureanywhere still blocks; supersession excuses nothing that actually failed.The middle of three quick merges is displaced before it starts, which #688 counted as a lost verdict. Here it is the intended outcome: the newest push's run includes that commit and is the one that describes
main.What changed
integration.yml,ci.yml:group: …-${{ github.ref }}for both events. Comments record the decision and the reasoning.scripts/verify-merge.mjs:gateVerdicttakessupersededBy; the call site readsrepos/…/commits/<base>after a merge. Reported the waysecondReviewerandmaintainerApprovalalready are: never a blocker..claude/rules/verifying-merged-work.md: how to read a cancelled job now.Three tests: superseded is reported not blocked; cancelled on HEAD still blocks; failure on a superseded commit still blocks. The first fails with the branch removed.
Coordinated with the session that owns #1744's budget guard; the verify-merge interaction was their catch. CI-only, no changeset. scripts 1,281 · lint 0 · comments 0.