Repository navigation
ci: cancel superseded review runs - #45
Conversation
johnny4young
left a comment
There was a problem hiding this comment.
Review: CI cancels superseded runs
What it does: adds a workflow-level concurrency block to ci.yml (group ci-<workflow>-<ref>, cancel-in-progress: true), with docs, a changelog entry, and invariants in tests/workflows.bash.
Verdict: needs a decision before merge (one should-fix that depends on intent, nothing else).
Findings
- 🟠
.github/workflows/ci.yml:9-12: the group also coverspushtomain(refs/heads/main), so a newer merge cancels the previous merge commit's CI. The title says "review runs". If main should keep a result for every commit, see the inline suggestion, which uses a per-SHA group for pushes and cancels only forpull_request. I did not push this because the inline comment in the workflow explicitly mentions main, so it is your call. - 🟡
tests/workflows.bash(theci_grouplambda): the threeci_group.call(...)asserts check a Ruby lambda against itself, not GitHub's evaluation. They can never fail, so they add no protection beyond the exact-string assert above them. Consider dropping them, or replacing them with a comment. - 🟡 This PR and #46 both append to the same spots in
CHANGELOG.md/CONTRIBUTING.md(and touchci.yml/tests/workflows.bash).git merge-treeshows text conflicts in the two docs, so whichever merges second needs a trivial rebase.
Checked and fine
pull_requesthas nobranches:filter, so stacked PRs (base ≠ main) still run. The PR refrefs/pull/N/mergeisolates PRs from each other and from main.release.yml(group: release) andcanary.yml(group: canary-live-feed) keepcancel-in-progress: falseand live in different workflows, so they can never be cancelled by this group. Release's own pushes tomainuseGITHUB_TOKEN, so they don't trigger CI anyway.- Local: full suite passed except
install-transaction.bash, which fails identically onmainbecause this container runs as root (environment-only). Workflow invariants, ShellCheck 0.11.0, shfmt 3.13.1 andbash -nwere clean. CI is green.
Generated by Claude Code
| # Superseded review/main feedback must not occupy the current head's runners. | ||
| concurrency: | ||
| group: ci-${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
🟠 This also cancels main push runs: CI runs on push: branches: [main], github.ref is refs/heads/main for every one of them, and cancel-in-progress: true applies regardless of event. If two PRs merge a few minutes apart, the first merge commit's CI is cancelled, so that commit never gets a full OS/shell matrix result. That hurts bisecting, the status badge, and choosing a known-good commit to release from. The PR title says "review runs". This comment says "review/main feedback", so please confirm main was meant to be included.
If only PR runs should be superseded, note that GitHub keeps at most one pending run per group even with cancel-in-progress: false. A conditional flag alone therefore isn't enough for main. Give push runs a per-commit group instead:
| # Superseded review/main feedback must not occupy the current head's runners. | |
| concurrency: | |
| group: ci-${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| # Superseded PR feedback must not occupy the current head's runners. Pushes to | |
| # main get a per-commit group so every merged commit keeps a full CI result. | |
| concurrency: | |
| group: ci-${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.sha }} | |
| cancel-in-progress: ${{ github.event_name == 'pull_request' }} |
(tests/workflows.bash asserts the exact group string and == true, so it would need the matching update.) Release and canary are unaffected either way: different workflow, own release / canary-live-feed groups with cancel-in-progress: false.
Generated by Claude Code
The shared ci-<workflow>-<ref> group with cancel-in-progress: true also cancelled the previous main push run whenever another commit merged, so that merged commit never got a complete CI result. Pull requests keep their per-ref group and are superseded; pushes get one group per commit and are never cancelled. The workflow invariants now evaluate the configured group and cancel-in-progress expressions for representative pull request and main events instead of comparing a copied lambda with itself, so they fail if main runs become cancellable or PRs share a group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
- compare strings case-insensitively as GitHub expressions do - keep YAML booleans as booleans instead of stringifying them - evaluate a lone expression once and interpolate multi-expression templates - read the workflow name from ci.yml instead of hard-coding it - reject pull_request branches-ignore filters as well as branches - move the CI cancellation note under the pull request guidelines - list CI concurrency groups among the workflow invariants Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
Summary
When a new commit replaces a PR revision, the obsolete CI jobs keep occupying hosted runners. This PR adds workflow-level
concurrencyto the ordinary CI workflow so that superseded pull request runs are cancelled, while every push tomainkeeps its own, never-cancelled run.What changed
.github/workflows/ci.yml:refs/pull/N/merge), so a new revision cancels the previous run. Different PRs, including stacked PRs that target non-default bases, stay independent.main: the group is the commit SHA and cancellation is off, so a later merge can neither cancel nor replace the pending run of an earlier merge. Every merged commit keeps a complete OS/shell matrix result.release.yml(group: release) andcanary.yml(group: canary-live-feed) are untouched and keepcancel-in-progress: false.tests/workflows.bash: the invariants now evaluate the configuredgroupandcancel-in-progressexpressions, using a small evaluator for GitHub's==/!=/&&/||value semantics. They are evaluated for representative events (two revisions of PR 45, PR 46, and twomainpushes) and assert:main, do not;maincommit has its own group;mainruns never do;CHANGELOG.mdandCONTRIBUTING.mddescribe the PR-only cancellation.Why
Superseded PR feedback wastes runners.
mainresults, however, are what bisecting, the status badge and picking a known-good commit to release rely on, so they must never be cancelled.How it was tested
tests/workflows.bashfail with the matching message:cancel-in-progress: true→ "CI must never cancel runs for pushes to main";github.refgroup → "each main commit must keep its own group…";a3d3a2c, as a non-root user: 31/31 passed.git diff --checkare clean.a3d3a2c: all 10 jobs and all 11 checks green, including native macOS Bash 3.2, Linux Fish and Windows Bash/PowerShell.54fcb9a.Review follow-ups
a3d3a2cci: never cancel main runs; supersede only pull request revisions. This applies the review finding that the originalci-<workflow>-<ref>group withcancel-in-progress: truealso cancelled the previousmainpush run on every merge. It also replaces the tautologicalci_grouplambda asserts, which compared a copied lambda with itself and could never fail, with assertions evaluated from the YAML.Decisions taken
pull_requestevents, and groupmainpushes by SHA. A conditionalcancel-in-progressalone would not have been enough, because GitHub keeps only one pending run per group and replaces older pending runs even whencancel-in-progressis false.Known limitations & follow-ups
GITHUB_TOKENdo not trigger CI at all (GitHub behavior). That is unchanged.Merge order
#46 → #47, then this PR (#45). It edits the same spots in
CHANGELOG.mdandCONTRIBUTING.mdas #46 and #47.git merge-treeshows only those text conflicts (theci.ymlandtests/workflows.bashhunks auto-merge). Whichever lands later resolves them at merge time by keeping both entries. Independent of #44, #48, #49 and #41.