Skip to content

QVAC-24629 infra: cancel superseded CI runs on push and dispatch - #38

Merged
tobi-legan merged 3 commits into
masterfrom
QVAC-24629/ci-concurrency
Sep 8, 2026
Merged

QVAC-24629 infra: cancel superseded CI runs on push and dispatch#38
tobi-legan merged 3 commits into
masterfrom
QVAC-24629/ci-concurrency

Conversation

@tobi-legan

@tobi-legan tobi-legan commented Sep 4, 2026

Copy link
Copy Markdown

Problem

group: ${{ github.workflow }}-${{ github.head_ref && github.ref || github.run_id }}

head_ref is only set on pull_request. So on a push or a workflow_dispatch the group was unique per run, no two runs shared a group, and cancel-in-progress: true had nothing to act on — a re-dispatch stacked a second 8-job run instead of replacing the first.

Fix

concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number || ((github.event.inputs.create_release == 'true' || github.ref_name == 'master') && github.run_id) || github.ref }}
  cancel-in-progress: true
event group behaviour
pull_request PR number supersedes
dispatch with create_release: true (any ref) github.run_id never cancels
push to master github.run_id never cancels
anything else (push to ci, plain dispatch) github.ref supersedes

runner-names-validate.yml had no block and gains one. 2 files.

Why the exemption keys on the publish predicate, not the branch

Every publish gate in build.yml 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: true dispatch publishes a GitHub release and pushes ghcr images from any ref, not just master. An earlier revision of this PR exempted only the branch, which left that path cancellable — 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 the tag cannot be deleted or moved to redo it cleanly.

Guarding cancel-in-progress on 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.inputs is null on push and PR events, so the added disjunct is inert there. ref_name matches the form the release steps themselves use, and also covers a tag named master.

Proof

Two workflow_dispatch runs of build.yml (create_release=false) on this branch, against the current tip:

run created result
34119894976 12:04:31Z cancelled by the next dispatch
34119906908 12:04:39Z took over, then cancelled by hand to free runners

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.mjs ok (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

validate fails on node --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.mjs has 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

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>
@tobi-legan
tobi-legan requested review from a team as code owners September 4, 2026 17:23
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review Status

Current Status: ❌ PENDING
Approvals so far: none

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>
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
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>

@gianni-cor gianni-cor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the concurrency changes, including security considerations. Manual default-branch dispatch overlap is accepted behavior; no blocking issues found.

@tobi-legan
tobi-legan merged commit d3f773a into master Sep 8, 2026
14 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants