QVAC-24629 infra: cancel superseded CI runs on push and dispatch - #38
Merged
Conversation
build.yml's concurrency group fell back to github.run_id whenever head_ref was empty. head_ref is only set on pull_request events, so every push and every workflow_dispatch produced a group unique to that run and nothing was ever cancelled. Key the group on the PR number, falling back to github.ref, so pushes and dispatches for the same branch supersede each other. This is the block the QVAC monorepo uses across its addon lanes (.cursor/rules/devops/github-actions.mdc "Concurrency"). build.yml's workflow_dispatch can publish a release (create_release input), so that one path keeps a group of its own, keyed by run_id: a release run is neither cancelled nor cancelling, and a later push to the same branch cannot supersede it half-way through. Guarding cancel-in-progress instead would not work — cancel-in-progress governs whether the INCOMING run cancels others, so it would stop a release from cancelling others while still letting a later push cancel the release. runner-names-validate.yml had no concurrency block at all; it gets the plain form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
Review follow-up. The previous commit keyed every non-PR event on github.ref,
which put all master pushes in one group with cancel-in-progress: true. That is
where the whole risk of this change sat:
- A release or publish job that runs on push to master could be cancelled
mid-flight, leaving a created release with a partial asset set.
- A cron-only lane whose run outlives its interval is killed by the next tick,
so the cache it exists to populate is never saved.
- Back-to-back merges leave the earlier commit with no CI verdict at all, which
is precisely why upstream used the run_id fallback in the first place.
- Self-hosted lanes without an `if: always()` teardown can orphan a server
process holding its port and GPU memory when cancelled mid-test.
- A workflow_dispatch taking a `sha` input — the normal way to bisect — had two
runs for two different commits cancelling each other.
Key the default branch on github.run_id, as upstream does, and keep per-PR and
per-branch dedup everywhere else:
group: ...-${{ github.event.pull_request.number
|| (github.ref == 'refs/heads/master' && github.run_id)
|| github.ref }}
pull_request -> per PR (supersedes)
push/dispatch !master -> per branch (supersedes)
push/dispatch master -> per run (never cancels)
github.run_id is always non-empty, so the && / || pair cannot fall through to
the wrong operand. This keeps the behaviour that was actually asked for — one
run per PR/branch, a re-push or re-dispatch replacing the previous one — while
dropping the part that only ever destroyed signal.
Comments corrected at the same time: several said "push or dispatch" on
workflows that have no workflow_dispatch trigger.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iancris
reviewed
Sep 7, 2026
iancris
reviewed
Sep 7, 2026
Review follow-up (Ian, PR #38). Exempting only `master` left the partial-publish hazard live on any non-master `create_release: true` dispatch, because every publish gate in this file has a branch-free disjunct: release job (645) if: (push && ref == refs/heads/master) || github.event.inputs.create_release == 'true' docker push (226) push: (push && ref == refs/heads/master) || github.event.inputs.create_release == 'true' Create release (683) if: github.event_name == 'workflow_dispatch' || github.ref_name == 'master' Upload release (692) if: github.event_name == 'workflow_dispatch' || github.ref_name == 'master' So a create_release dispatch publishes a GitHub release and pushes ghcr images from ANY ref, while falling to the third arm and being cancellable by a second dispatch on the same ref. `Upload release` creates the release and then loops uploadReleaseAsset one zip at a time, so a cancel in between leaves a published release with a partial asset set — and the tag-immutability ruleset means that tag cannot be deleted or moved to redo it cleanly. Key the exemption on the predicate that actually gates publishing: group: ...-${{ github.event.pull_request.number || ((github.event.inputs.create_release == 'true' || github.ref_name == 'master') && github.run_id) || github.ref }} github.event.inputs is null on push and pull_request, so the added disjunct is inert there. ref_name matches the form the release steps themselves use. The comment previously paraphrased the gate as `github.ref_name == 'master'`, dropping the `github.event_name == 'workflow_dispatch' ||` half that invalidated its conclusion. It now quotes all four gates verbatim, since this block is the only record of why the exemption exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iancris
approved these changes
Sep 7, 2026
sidj-thr
approved these changes
Sep 8, 2026
Proletter
approved these changes
Sep 8, 2026
gianni-cor
approved these changes
Sep 8, 2026
gianni-cor
left a comment
There was a problem hiding this comment.
Reviewed the concurrency changes, including security considerations. Manual default-branch dispatch overlap is accepted behavior; no blocking issues found.
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
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.
Problem
head_refis only set onpull_request. So on a push or aworkflow_dispatchthe group was unique per run, no two runs shared a group, andcancel-in-progress: truehad nothing to act on — a re-dispatch stacked a second 8-job run instead of replacing the first.Fix
pull_requestcreate_release: true(any ref)github.run_idmastergithub.run_idci, plain dispatch)github.refrunner-names-validate.ymlhad no block and gains one. 2 files.Why the exemption keys on the publish predicate, not the branch
Every publish gate in
build.ymlhas a branch-free disjunct:So a
create_release: truedispatch publishes a GitHub release and pushes ghcr images from any ref, not justmaster. An earlier revision of this PR exempted only the branch, which left that path cancellable —Upload releasecreates the release and then loopsuploadReleaseAssetone zip at a time, so a cancel in between leaves a published release with a partial asset set, and thetag-immutabilityruleset means the tag cannot be deleted or moved to redo it cleanly.Guarding
cancel-in-progresson the input does not work either: it governs whether the incoming run cancels others, so it would stop a release run from cancelling other things while still letting a later push cancel the in-flight release.github.event.inputsis null on push and PR events, so the added disjunct is inert there.ref_namematches the form the release steps themselves use, and also covers a tag namedmaster.Proof
Two
workflow_dispatchruns ofbuild.yml(create_release=false) on this branch, against the current tip:The publish-exemption arm is deliberately not tested. Exercising it requires dispatching with
create_release: true, which would cut a real GitHub release and push real ghcr images from a feature branch. The arm is verified by inspection against the four gates quoted above instead.Static: all 5 workflow files parse,
validate-runner-names.mjsok (1 target, 1 workflow).Not changed
check-approvals.yml(cancel-in-progress: false),security-baseline.yml(already correct),reusable-runner-names.yml(workflow_call— inherits the caller's group; its own would serialize every caller).Pre-existing red check — not from this change, not fixed here
validatefails onnode --test .github/scripts/test/runner-names.test.mjs: that file does not exist in this repo. Red since the runner-label catalog landed — master run 33856079417 and the PR that introduced it fail identically. Being handled separately;lib/runner-names.mjshas diverged from the ggml copy that has tests, so it is not a straight port.QVAC-24629. Siblings: qvac-fabric-speech.cpp#219, qvac-fabric-llm.cpp#235, qvac-ext-ggml#83.
🤖 Generated with Claude Code