ci(root): stop a main run from cancelling the commit before it - #688
Merged
Merged
Conversation
On `main` `github.ref` is one string for every push, so each merge superseded and cancelled the previous commit's in-flight run. Measured over the last eight commits: `Browser tests` 2 success / 5 cancelled, and the three `Integration` legs 3 success / 5 cancelled — on exactly the same five commits, which is what points at one shared cause rather than two flaky jobs. A required check reporting `cancelled` is indistinguishable from one that failed, so the habit it teaches is to merge past red. That is the cost worth fixing, more than the missing coverage. Cancellation stays for pull requests, where a new push makes the previous verdict irrelevant and the minutes are worth saving. `preview.yml` carries the same setting and is left alone: it triggers on pull requests only.
Collaborator
Author
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![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: bc6110ad2e
ℹ️ 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".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
Switching cancellation off preserved the RUNNING job and not the queued one. A concurrency group holds at most one running and one pending run, so three merges in quick succession displace the middle commit while it is still queued — the same missing verdict, reached a different way. Keyed by `github.sha` on push there is nothing to displace. Pull requests keep the shared per-ref group, where superseding is what you want.
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
This was referenced Sep 11, 2026
mobeenabdullah
added a commit
that referenced
this pull request
Sep 11, 2026
* 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the mechanism behind task 205. A required check that reports
cancelledis indistinguishable from one that failed, and the habit that teaches — merge past red, the red is never mine — is the actual cost.The measurement
Last eight
origin/maincommits, newest run per name:Browser testsIntegration (postgres / mysql / sqlite)The task file recorded 0 successes in 6 with two outright failures. Those failures are gone — #669 and #663 landed in between — so what remains is entirely cancellation.
The cancellations fall on exactly the same five commits in both workflows:
e1d573e23,3e875bcc2,9368cf6b2,330ce9994,249649eb9. Two independent required checks cancelling together on the same commits is much stronger evidence of one shared cause than either measurement alone.The mechanism
Read from the workflow rather than inferred:
On
main,github.refis the same string for every push. So each merge supersedes and cancels the previous commit's in-flight run, and with several lanes merging, mostmaincommits never reach a verdict.The change
cancel-in-progress: ${{ github.event_name == 'pull_request' }}, inci.ymlandintegration.yml.Cancellation is kept where it is correct: on a pull request a new push makes the previous run's verdict irrelevant, so cancelling saves the minutes and loses nothing. It is dropped on
main, where each commit is a distinct artifact someone will later ask "was that green?" about.Scope is wider than the task's title
integration.ymlcarries the identical shape and also runs onmain, so this covers three more required checks — the task named onlyBrowser tests, and fixing just that would have left the same defect in place for the integration legs.preview.ymlhas the same setting and is deliberately unchanged: it triggers onpull_requestonly, where superseding is correct.What this PR does NOT establish
Task 205 sets the bar at five consecutive green
maincommits with no re-runs, and no PR can demonstrate that — it is only observable after this lands and five more merges happen. Three of the six original data points were cancellations, which are inherently intermittent, so a single green after merge would prove nothing either.The task file stays open with the bar written down and the re-measurement recorded. I would rather say that than let a merged PR imply the bar was met.
Explicitly not done
Making the checks non-required. That converts "browser coverage is broken" into "browser coverage is optional", and the second state is permanent because nothing forces it back.
Notes