ci: cancel superseded pull request runs - #5742
Conversation
The two per-job concurrency groups already cancelled a superseded job when its replacement was queued, but they cancelled pushes to `main`, `next` and `v4` the same way. Those runs publish the coverage later comparisons are measured against, and cancelling one also hides a breakage that is already on the branch. Replace them with a single workflow-level group that only cancels pull request runs. Besides fixing that, it cancels the whole superseded run at once rather than job by job as each replacement is queued — the test matrix is 48 jobs — it covers `upload-coverage`, which had no group of its own and could otherwise publish coverage for a commit already superseded, and a matrix dimension added later cannot be forgotten in a group key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe workflow adds one workflow-level concurrency policy. For pull request events, it groups runs by workflow and ref and cancels in-progress runs. For other events, it assigns each run a unique group. The lint and test jobs no longer define separate concurrency policies. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The workflow now scopes cancellation to pull-request runs while preserving independent non-pull-request runs, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The pull request changes only
✨ 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 |
`cancel-in-progress` only protects the run that is already executing. A concurrency group holds at most one pending run, and GitHub cancels that pending run whenever a newer one enters the group, whatever `cancel-in-progress` says. Keying every run of the workflow on the ref therefore did not deliver what the comment promised: with one branch run executing and a second waiting, a third push would have evicted the second. Group only pull request runs by ref; give everything else `github.run_id`, so each is a group of one that can be neither cancelled nor evicted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
|
Pushed a correction in 2a57d72 before anyone spent time reviewing the first version — the review on the sibling PR (webpack/enhanced-resolve#673) caught a real flaw that this PR had too.
So keying every run on the ref did not actually deliver what this PR claims. With one The group is now: group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.run_id }}
cancel-in-progress: ${{ github.event_name == 'pull_request' }}Pull requests still group by ref and supersede as described above; every other run gets a group of one and can be neither cancelled nor evicted. Generated by Claude Code |
#5741 stopped sharding the test matrix and dropped Node 25, which edited the per-job concurrency group this branch removes in favour of a workflow-level one. Kept the removal: the workflow-level group covers every job, including `upload-coverage`, which never had one. Also drops the job count from the comment. It said 48, #5741 made it 9 within the hour, and a number like that goes stale every time the matrix is touched.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5742 +/- ##
==========================================
+ Coverage 89.38% 90.29% +0.90%
==========================================
Files 13 13
Lines 6169 6241 +72
==========================================
+ Hits 5514 5635 +121
+ Misses 655 606 -49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
nodejs.ymlalready cancelled superseded jobs, but through per-job concurrency groups that also cancel pushes tomain,nextandv4. This replaces them with one workflow-level group that only cancels pull request runs.Why the branch pushes should not be superseded
That is the actual bug here.
main,nextandv4are built on push, and those runs feed the coverage that later comparisons are measured against. A cancelled run also hides a breakage that is already on the branch, which is exactly when you want to know.Note that
cancel-in-progress: falsealone is not enough: a concurrency group holds at most one pending run, and GitHub evicts that pending run whenever a newer one enters the group regardless of the flag — "any existing pending job or workflow in the same concurrency group will be canceled and the new queued job or workflow will take its place." With onemainrun executing and a second waiting, a third push would evict the second. Hencegithub.run_idfor non-pull-request events: each gets a group of one, so it can be neither cancelled nor evicted. (Thanks to the review on the sibling PR for catching this; the first version of this PR had the flaw.)Why one workflow-level group rather than fixing the per-job ones
Adding the condition to the existing blocks would have been a smaller diff, but the per-job arrangement has three other costs:
upload-coveragehas no group at all. Today a superseded run can still reach it and publish coverage for a commit nobody is looking at. The workflow-level group covers it.Happy to switch to the minimal version (keep the per-job groups, just add the condition) if you would rather keep the existing shape.
Verification
actionlint1.7.7 is clean on all three workflows, and I checked that this means something by typo'ing the expression togithub.event_nam, which it catches at exactly that line. The file is also Prettier- and cspell-clean using this repo's own configs.release.ymlis untouched: it already usesconcurrency: ${{ github.workflow }}-${{ github.ref }}, whose string form leavescancel-in-progressfalse, so releases queue instead of cancelling.dependency-review.ymlis untouched too — one short job, nothing to save.Rebased onto #5741 (merge commit), which stopped sharding the test matrix and dropped Node 25. It edited the same per-job group this PR removes; the removal is kept, since the workflow-level group supersedes it. An earlier version of this description quoted a job count, which that PR made wrong within the hour — the count is now gone from both the description and the workflow comment.
The same change is going to
enhanced-resolve(webpack/enhanced-resolve#673) andwebpack-dev-middleware(webpack/webpack-dev-middleware#2412).🤖 Generated with Claude Code
https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
Summary by CodeRabbit