Repository navigation
ci: stop dropping a commit's validation when the next one lands - #394
Conversation
Merging two PRs a minute apart left the first commit with no unit tests, no compatibility run and no CodeQL analysis. Not a flake, and not the triggers -- `on: push: branches: [main]` is correct and the full twelve-job matrix does run on a quiet push. The loss came from the concurrency groups added in #392. Every workflow keyed its group on `github.ref`, so all of main's pushes shared one group. GitHub cancels a *pending* run whenever a newer one joins the group: `cancel-in-progress: false` protects a run that has already started, never one still queued. Measured on run 34970682137 for e0f0160 (the #393 merge), which finished `cancelled` with **zero jobs**, one second after the next push's run was created. That commit is on main having been validated by nothing at all, along with its CodeQL, Conda and Vendor runs. Non-pull-request events now key on `github.run_id`, giving each run its own group, so nothing coalesces and nothing is dropped. Pull requests still key on the ref and still cancel in progress, which is where the saving belongs and is untouched -- this does not undo #392's fan-out reduction, which came from removing the duplicate callers rather than from cancellation. Per workflow, and they differ on purpose: * unit-tests, python-compatibility, codeql -- per-run for pushes, so every commit on main is validated. unit-tests keeps its literal `unit-tests-` prefix because `github.run_id` inside a called workflow is the *caller's* run id, so without the prefix a callee would sit pending behind its own caller and deadlock. * create-release, cron-conda, cron-vendor -- per-run, never cancelled. These tag, upload and publish, so a dropped pending run is a release or a registry update that never happened. The crons additionally derive their value from *when* they ran rather than from which commit, so two scheduled runs at one SHA are both meaningful. * deploy-pages -- deliberately left keyed on the ref. It publishes a latest state rather than a verdict on a commit, so coalescing is correct, and publishing the older commit's docs after the newer ones would be wrong. Pages also allows only one live deployment. Validated: all seven files parse, zero duplicate keys at any nesting level, and the expression `${{ github.event_name == 'pull_request' && github.ref || github.run_id }}` resolves to the ref on a pull request and to the run id on every other event, per GitHub's value-returning `&&`/`||`.
There was a problem hiding this comment.
🟡 Changes recommended
create-release.yml now uses a per-run concurrency group, which does not serialize release runs and can reintroduce overlapping tag/release races described in the workflow’s own comments.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adjusts GitHub Actions concurrency group keys so that non-PR events (notably push to main) don’t silently lose an entire queued validation run when a newer commit lands shortly after.
Changes:
- Update validation workflows (Unit Tests, Python Compatibility, CodeQL) to key non-PR concurrency groups on
github.run_id, while keeping PR runs keyed ongithub.refand cancellable. - Switch maintenance/publishing workflows (Conda Update, Vendor Update, Create Release) to
run_id-keyed groups with no cancellation to avoid dropped pending runs. - Add explanatory comments, and explicitly retain ref-keyed coalescing for Pages deployments.
File summaries
| File | Description |
|---|---|
| .github/workflows/unit-tests.yml | Use run_id for non-PR concurrency to prevent dropped main-push validation runs. |
| .github/workflows/python-compatibility.yml | Same concurrency strategy as unit tests for compatibility checks. |
| .github/workflows/codeql-analysis.yml | Same concurrency strategy to avoid missing security scans on main pushes. |
| .github/workflows/create-release.yml | Changes concurrency group to run_id with added rationale comment. |
| .github/workflows/cron-conda.yml | Changes concurrency group to run_id with added rationale comment. |
| .github/workflows/cron-vendor.yml | Changes concurrency group to run_id with added rationale comment. |
| .github/workflows/deploy-pages.yml | Keeps ref-keyed concurrency and documents why coalescing is desired for Pages. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot was right, and about create-release it was right in the way that matters. The first pass moved all six non-Pages workflows to per-run groups, which fixed dropped validation runs but silently removed serialisation from the paths that publish. Those are not the same trade: * a dropped *validation* run means a commit nobody ever checked, and there is no second chance at it; * a dropped *publishing* run means a publish that did not happen twice. So per-run is now only on unit-tests, python-compatibility and codeql, and create-release, cron-conda and cron-vendor go back to ref-keyed. create-release is the sharp case: unlike the two crons it has **no** job-level concurrency anywhere, so that group was the only thing stopping two runs tagging and uploading to PyPI and Anaconda at once. Per-run would have allowed exactly the race the file's own comment warned about. Also corrected the cron comments, which claimed the workflow-level group queued superseded runs. It does not, and it is not what protects the repository writes either -- the `vendor-update` and `conda-update` jobs each carry their own `repository-maintenance` group, shared between the two workflows, and that is what actually serialises the commits. The comments now say so, so the next reader does not credit the wrong mechanism. Validated: all seven parse, zero duplicate keys at any nesting level.
|
Copilot's call on The first pass moved all six non-Pages workflows to per-run groups. That fixed the dropped-validation problem but quietly removed serialisation from the paths that publish — and those two are not the same trade:
So per-run now applies only to
So for And Copilot was right about the comments on both crons. They claimed the workflow-level group "queue[s] superseded runs" — it does not, and it is also not what protects the repository writes. That is the Final split, for the record:
All seven files parse with zero duplicate keys at any nesting level. |
JarryShaw
added
the
ci
You reported that main's pushes are no longer getting the full unit test and compatibility run. They are — but not reliably, and when they are lost it is silent. Here is what is actually happening.
The triggers are fine; the concurrency groups are not
on: push: branches: [main]is correct, and a quiet push to main does run everything. Verified ona71dd366d:But
e0f0160a3— the #393 merge, one commit earlier — got nothing:Zero jobs, and cancelled one second after the next push's run was created at
12:55:42Z. Its CodeQL, Conda Update and Vendor Update runs died the same way.The cause is the concurrency groups I added in #392. Every workflow keyed its group on
github.ref, so all of main's pushes shared one group — and GitHub cancels a pending run whenever a newer one joins the group.cancel-in-progress: falseonly protects a run that has already started; it does nothing for one still queued. Soe0f0160a3's run sat pending behinde87ec884e's still-running one, anda71dd366devicted it.Net effect: merge two PRs a minute apart and the first commit reaches main validated by nothing. Exactly the hole you were worried about.
Fix
Non-pull-request events key on
github.run_id, so each run gets its own group — nothing coalesces, nothing is dropped. Pull requests still key on the ref and still cancel in progress, which is where the saving belongs.This does not undo #392. That PR's fan-out reduction came from removing the duplicate callers, not from cancellation, so the job counts per event are unchanged.
The workflows differ on purpose:
unit-testsrun_idpython-compatibilityrun_idcodeqlrun_idcreate-releaserun_idcron-condarun_idcron-vendorrun_iddeploy-pagesref(unchanged)deploy-pagesis deliberately left alone: it publishes a latest state rather than a verdict on a commit, so dropping the pending build and letting the newer one publish is right, and deploying the older commit's docs after the newer ones would be actively wrong. Pages also permits only one live deployment.One trap worth recording:
github.run_idinside a called workflow is the caller's run id, sounit-testskeeps its literalunit-tests-prefix. Without it a callee would share its caller's group and — now that pushes do not cancel — sit pending behind its own caller forever.Validation
All seven files parse; zero duplicate keys at any nesting level (the #375 bug class, checked with a recursive walk of the YAML node tree rather than
safe_load, which silently keeps the last of a duplicate pair). The expression resolves as intended under GitHub's value-returning&&/||:true && ref→refon a pull request;false && ref→false, thenfalse || run_id→run_idon every other event.Separate finding, and it needs your decision — not fixed here
Making the runs happen is only half of "validate no bad changes are being shipped". They also have to matter, and right now they do not:
No check is required to merge. A PR with red unit tests is mergeable today; branch protection only insists the branch be current. So even with this PR merged, nothing stops a failing commit from landing — it just gets a red mark afterwards.
Fixing that means adding required contexts, which is a repo-settings change rather than a code change, so I have left it to you. Note the check names changed in #392, so the list would be the current ones —
Python 3.10…3.15,Integration Python 3.10…3.15,Compat Python 3.10…3.15. Worth choosing deliberately: requiring a context that never reports on some event makes PRs permanently unmergeable, so the safe set is the ones that run on every pull request.