ci: a run a newer push has overtaken skips itself - #1778
Conversation
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.
|
@codex review |
|
Warning Review limit reachedNext included review available in 2 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 (11)
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. |
![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: 79b0b39a44
ℹ️ 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".
…sted 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.
…-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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80949995c2
ℹ️ 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".
… 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b30d6e831
ℹ️ 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".
…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.
|
@codex review |
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c284e84ad
ℹ️ 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".
|
@codex review |
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4ecc246cc
ℹ️ 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".
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
One-time backlog cancellation, by the founder's decision after this merged. The runs queued on Those 35 commits show
|
|
Live verification, first superseded push after the merge: Integration run 34602571259 for 9e59229 (#1788's merge commit) asked, found ebf8f78 already had a newer run, and skipped. The action's notice: superseded: ebf8f78 already has a newer run of integration.yml on main; skipping. Check-runs on that commit: Integration (postgres), (mysql), (sqlite), each completed/skipped under its own name, which is what verify-merge requires and accepts. The run held a runner for the ask alone. |
|
Live verification, the other half: Integration run 34603323315 for ebf8f78 (the newest push at the time) asked, found nothing newer, ran all three legs in full, and every dialect concluded success. Three superseded pushes before it (9e59229, cd95546, 985a438) skipped under their own names. main's Integration is green under the new workflow, the observation the queue had made impossible since #1735. |
1 participant
Replaces #1766, closed after review found four more ways a
cancelledrun could lie.The problem it solves
The CI and Integration workflows key concurrency on
mainby commit, so every merge keeps a verdict of its own and nothing is ever cancelled (#688, deliberately). At the current merge rate that queue could not drain: Integration is 3 dialects × 20–35 min, and 50 runs sat queued with nothing onmainable to report for over half an hour, including the commit that fixed the failure everyone was waiting on.Why not cancel
#1766 tried it.
cancelledthen had to mean superseded, or stopped by hand, or timed out, and every reader of the check needed rules to tell those apart. Two rules were added; review found four more; the next round would have found a seventh. That is an overloaded signal, not an incomplete implementation.What this does instead
Nothing is cancelled. A composite action (
.github/actions/superseded-on-main) asks at the start of a run onmainwhether 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.
cancelledkeeps meaning what it says. Queued runs start, ask, and stop within seconds; only the newest tests. Per-commit groups stay exactly as #688 left them.The hole this opens, closed in the same change
CI's
changesjob diffs a push againstevent.before. 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. That hole doesn't exist today only because nothing is ever superseded.So the action also names the last commit whose substantive job actually ran, found by walking completed runs newest-first, and
changesdiffs against that onmain. Integration has no inert path and needs only the first answer.Also
ci-gate.mjslearns supersession as a second acceptable reason for a skip, kept separate frominertso the gate never says a superseded run "touched only inert paths." Tests for both.integration.ymlto a floor sized for a suite leg. It now holds the jobs that run the budget wrapper; the pre-job asks a question and 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.actions: readis granted to the asking job alone.verify-merge.mjsneeds no change beyond two comments:skippedalready passes, and the required names are now literal. The inference added in ci: a newer main push supersedes the run in flight #1766 never lands.The skipped matrix, measured
A job-level
ifis evaluated beforestrategy.matrixis applied (GitHub's workflow-syntax reference says so verbatim). A superseded run would therefore report the postgres/mysql matrix as ONE skipped check, andverify-mergewould call both required checks absent on every superseded merge commit. The sqlite leg is not a matrix and keeps its name.The first fix named the matrix job for what happened via a
needs-based expression. A throwaway probe workflow then measured what GitHub actually does with a skipped matrix job: itsnameexpression is not evaluated at all. The check was named with the raw source text,JobLevel (${{ matrix.dialect }}), and theneedsternary appeared verbatim. So that design is gone, and with it theverify-mergestand-in it needed.What replaced it is the plain answer: one job per dialect with a literal name. A literal name survives a skip, so each leg reports as
Integration (postgres)/(mysql)/(sqlite),skipped, which is what the gate already accepts;verify-mergeneeds no change beyond a comment saying why the names are literal. The split also ends both databases booting for both legs, which the matrix had forced ("aservices:entry cannot be chosen per matrix value"): each job declares only the one it connects to. The setup steps the legs had as copies (pnpm, Node, job-scoped Turbo cache, install) now live once in.github/actions/integration-setup.What was wrong in the first revision, and how each was found
gh api --jq --argdoes not exist (accepts 1 arg(s), received 4). The last-tested walk failed silently and fell back toevent.before, the hole it exists to close. Reproduced against a real run id, fixed by binding the name injqover the raw response, and the whole script was then run locally against realmainrun ids in all four branches (superseded, walk, Integration's parenthesised job name, pull request).successandfailurecount as a tested base now. A run cancelled or timed out at setup judged nothing.main.Also fixed on review
event.before, the holelast-tested-shaexists to close. It now runs everything.changesand skips on supersession alone; still ungated on inert, for the reason its comment gives.:inside an expression and would have loaded as nothing.What to watch on the first real superseded push to
mainThat the three Integration checks appear under their own names as
skipped,CI gateand the comment scan skip, andnode scripts/verify-merge.mjs <pr>on that merge commit passes. This PR's own Integration run exercises the split jobs and the shared setup action on the pull-request trigger.CI-only, no changeset. scripts 1,301 · lint 0 · comments 0.