Skip to content

main groups by SHA — cancel-in-progress:false does not protect a QUEUED run - #2458

Closed
rbuergi wants to merge 1 commit into
mainfrom
ci/main-group-by-sha
Closed

rbuergi wants to merge 1 commit into
mainfrom
ci/main-group-by-sha

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What #2447 fixed, and what it did not

#2447 exempted main from cancel-in-progress. Correct and necessary — and it does not hold when runners are scarce.

cancel-in-progress: false protects a run that is already running. GitHub still keeps only one pending run per concurrency group, and a newer run supersedes the older pending one regardless of that setting.

While runners are free this is invisible: a run starts within seconds, so "queued" barely exists. When the pool is saturated it is decisive.

Observed today, after #2447 merged

The shared 60-job pool was saturated by a sibling repo (12 concurrent runs in MeshWeaver.Plugins). Main's runs never reached in-progress:

7faac22   pending → cancelled   (never ran — and this IS the #2447 merge commit)
9e427c0   pending → cancelled   (never ran)
667b74d   pending → cancelled
08d82de / 444c6a9 / fe12f91 / 71e3ec2   cancelled

Six consecutive main commits, no completed Build-and-Test between them.

What that cost

main-cd.yml gates delivery on Consolidate test results succeeding for that SHA. With no completed run its gate job stayed skipped:

GATE_RESULT: skipped
gate=skipped reason= publish= complete= green= sha=

Every CD run then reported success having published nothing — every image job simply skipped. Meanwhile plugin modules kept republishing against a newer core, so memex-cloud ran an older image against newer landed modules and every new pod aborted at boot (IHostedService[] -> OpenAICompatibleModelSync — blast radius in #2449). Production served ~11-hour-old code with a wedged rollout, and the first symptom was a user noticing.

The change

- group: build-test-${{ … workflow_dispatch && run_id || github.ref }}
+ group: build-test-${{ … workflow_dispatch && run_id
+                      || github.ref == 'refs/heads/main' && github.sha
+                      || github.ref }}

Each main commit gets its own group, so nothing can supersede it — queued or running — and CD always has a completed run to gate on.

PR runs are untouched: they keep grouping by ref, where superseding is correct and is where #2316's ~28% runner saving comes from.

Cost

During a burst, several main runs proceed in parallel instead of one surviving. That is the point: runner minutes are what buys a publishable image, and the alternative is what happened today.

Worth pairing with the underlying capacity problem (#2457): the bake is the heavy part of CI and belongs on a dedicated worker, not on the shared pool.

🤖 Generated with Claude Code

…ot QUEUED ones

#2447 exempted `main` from cancel-in-progress, which is right and necessary — and it does
not finish the job. `cancel-in-progress: false` protects a run that is ALREADY RUNNING.
GitHub still keeps only ONE pending run per concurrency group and supersedes the older
PENDING one whatever that setting says.

That distinction is invisible while runners are free — a run starts in seconds, so "queued"
barely exists — and decisive when they are not. On 2026-08-26 the shared 60-job pool was
saturated by a sibling repo (12 concurrent runs in MeshWeaver.Plugins). Main's runs never
reached in-progress; they sat pending, and each merge cancelled the previously pending one.
`7faac22` — the very commit that merged the never-cancel fix — went pending -> cancelled
WITHOUT EVER RUNNING, as did `9e427c0` and four before them.

Consequence: six consecutive main commits produced no completed Build-and-Test, so
main-cd's gate stayed `skipped` (`gate=skipped reason= publish= complete= green= sha=`),
every CD run reported success having published nothing, and no image shipped for hours
while production served ~11-hour-old code against newer landed modules.

Grouping `main` by `github.sha` gives each commit its own group, so nothing can supersede
it — queued or running — and CD always gets a completed run to gate on. PR runs keep
grouping by ref: superseding there is correct, and it is where #2316's ~28% runner saving
comes from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 26, 2026 22:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adjusts CI concurrency grouping so each push to main gets its own concurrency group keyed by github.sha, preventing GitHub Actions from superseding queued (pending) main runs when runners are saturated—ensuring Build-and-Test completes per merged commit and CD has a green check to gate on.

Changes:

  • Update concurrency.group to use github.sha when github.ref == 'refs/heads/main', while preserving existing grouping for PRs/other refs and run_id for workflow_dispatch.
  • Add/expand inline documentation explaining why cancel-in-progress: false does not protect queued runs and why SHA grouping is required on main.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@rbuergi

rbuergi commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #2457 (move the bake off the shared pool), and recording the gap on #2412 rather than fixing it this way.

The mechanism I found is real: cancel-in-progress: false protects a run that is already RUNNING; GitHub still keeps only ONE pending run per concurrency group and supersedes the older pending one regardless. While runners are free that is invisible — a run starts in seconds, so "queued" barely exists. Tonight the shared 60-job pool was saturated by a sibling repo, main's runs never reached in-progress, and each merge cancelled the previously PENDING one — including 7faac22, the commit that merged the never-cancel fix itself, which went pending → cancelled without ever running. Seven main commits, no completed run, gate=skipped on every CD run.

Why the fix is still wrong. Grouping main by SHA puts back exactly the demand #2316 deliberately removed — "≈1,199 runner-minutes, ~28% of ALL demand in that window" — and runner capacity is the binding constraint that caused the starvation in the first place. Trading more demand for guaranteed completion, silently, against an explicit dated decision, is the wrong call. The right lever is removing the bake from the shared pool (#2457): the bake is the heavy part of CI and the part that does not need a runner at all.

Noted for #2412's record, since the comment in dotnet-test.yml states it slightly too strongly:

The newest run is NEVER the one cancelled — cancel-in-progress cancels the older members of the group. main's tip therefore always runs to completion.

True when merges pause. Under sustained merges plus a saturated pool, the tip keeps being replaced before it can start, so no tip completes. The hourly reconciler cannot heal it either, because it gates on Consolidate test results being green for the commit it resolves — and there is no completed run to produce that. Worth a sentence in that comment so the next person does not have to re-derive it at 22:00.

@rbuergi rbuergi closed this Aug 26, 2026
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.

2 participants