Let every main run finish instead of cancelling it before it starts - #9082
Conversation
cancel-in-progress: false does not keep a main run alive, and 27 workflows say it does. GitHub cancels any PENDING run in a concurrency group as soon as a newer one is queued; cancel-in-progress governs only runs already executing. The reference is explicit: '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.' So on main, where pushes land in bursts, the runs in the middle of a burst are cancelled regardless. Both incidents this repo wrote down are that: studio-ui-smoke records a break sitting on main for 14 hours behind four cancelled runs, and the comment added to prevent it cannot. Today four merges in 37 minutes cancelled three consecutive main runs of Backend CI, which completed none, so main went untested across the whole batch. studio-backend-ci.yml and studio-ui-smoke.yml now key their group on github.sha when the ref is main, so each commit gets its own group and nothing supersedes it. Both are here because a regression already went unreported behind cancelled main runs of exactly these two. Pull requests are untouched: the sha term is empty off main, so superseded PR runs are still cancelled, which is the expensive direction to get wrong. The cost is real and worth naming: during a burst, main now runs these two per commit rather than once at the tip. That is what per-commit attribution costs, and it is what the incident comments were asking for. The remaining 26 keep supersession, since paying it repo-wide is not obviously right, but they stop claiming a protection they do not have. A guard asserts the two are grouped per commit on main, that pull requests still get latest-only, and that no workflow claims main protection while sharing a group across main commits. Each is mutation-checked.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e72bd10c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Validate that SHA is selected specifically on main
If the concurrency expression is later reversed to ${{ github.ref != 'refs/heads/main' && github.sha || '' }}, this predicate and every test in this file still pass: the group contains github.sha, github.ref, and refs/heads/main, while cancel-in-progress remains unchanged. Main commits would then share one group again and pending runs would be cancelled, so the new regression guard does not enforce its stated invariant; parse or otherwise validate which conditional branch supplies the SHA rather than checking substrings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I checked your counterexample directly: with the conditional reversed, the old predicate returns True and the new one returns False. It was a substring test standing in for a semantic one.
Fixed in c77aa52. The group is now rendered rather than grepped: two SHAs on refs/heads/main must produce DIFFERENT groups, and two SHAs on one pull request ref must produce the SAME group. Which branch supplies the SHA is what decides both answers. The evaluator raises rather than guessing on syntax it does not model, and a separate test turns that refusal into a named failure so a new expression form cannot silently pass.
Mutation tested against the real workflow: reversing the conditional fails 2 tests, making the SHA unconditional fails the pull request test.
|
Fresh evidence while this PR was being written: a fourth consecutive cancellation, this time caused by merging #9080. Five main commits, four cancelled Backend CI runs, zero completions. Each cancellation lands within a second of the next merge being queued, and Worth stating because it is the same mechanism from the other side: merging this PR will itself cancel |
Widened from the two workflows with written-up incidents to all 28 that run on push to main. Cancelling a pending run is not a saving: the four cancelled Backend CI runs today recorded ZERO jobs each, so nothing was reclaimed, and main simply went unverified across five commits. The two Kaggle workflows are the exception and keep one shared group. They spend an external GPU quota rather than runner minutes, so a superseded run there genuinely should be dropped instead of replayed per commit. The guard is general now rather than a list of two: every workflow triggered on push to main must be grouped per commit, must still key on github.ref so pull requests keep latest-only, and must not claim a main protection while sharing a group. It also asserts the scan found something, since a broken glob would pass everything. Mutation-checked four ways: reverting one workflow to a shared group, applying github.sha unconditionally, pointing the exemption at a missing file, and adding the old claim to a workflow that still shares a group.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6377c5ace2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
|
|
||
| def test_every_workflow_that_runs_on_main_is_grouped_per_commit(): |
There was a problem hiding this comment.
Run the guard on workflow-only changes
When a later PR changes only one of the concurrency blocks this test scans, this guard is never executed: Backend CI's pull_request.paths includes only its own workflow YAML (studio-backend-ci.yml:27-41), while the always-triggered workflow-trigger-lint.yml installs pytest/PyYAML but invokes only test_playwright_suites_run_in_ci.py (workflow-trigger-lint.yml:66-80). Consequently, reverting another workflow such as wheel-smoke.yml to a shared main group can merge without this regression test running; invoke this test from the unfiltered workflow-trigger lint job (or otherwise trigger its current runner for all workflow changes).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is the same gap #9060 hit: the guard reads every workflow's concurrency block, so the pull request that breaks it is one that edits some OTHER workflow's YAML, and no workflow's paths filter contains .github/workflows/**. A revert of wheel-smoke.yml to a shared main group would have collected no test that reads it.
Invoked from workflow-trigger-lint.yml in c77aa52, next to the Playwright coverage guard that sits there for the same reason. Added test_this_guard_runs_on_a_workflow_only_pull_request, which pins both the invocation and the absence of a paths filter on that job, so the placement cannot be undone quietly. Removing the step fails that test.
…ard on workflow-only PRs The predicate was "github.sha" in group, which is not the invariant. Reverse the conditional to github.ref != 'refs/heads/main' && github.sha || '' and the substring is still there, every workflow still names refs/heads/main, and main commits share one group again: the guard passed on the exact regression it exists to catch. It now renders the group for two SHAs on main and requires them to differ, and for two SHAs on one pull request and requires them to match, so which branch supplies the SHA is what decides. The evaluator refuses to guess at syntax it does not model, and a separate test turns that refusal into a named failure rather than a silent pass. Also invoke this file from workflow-trigger-lint.yml. It reads every workflow's concurrency block, so the PR that breaks it edits some other workflow's YAML, and no workflow filters on .github/workflows/**. A revert of wheel-smoke.yml to a shared main group collected no test that reads it. The lint job carries no paths filter by design, which is the same reason the Playwright coverage guard sits there.
for more information, see https://pre-commit.ci
|
@codex review |
|
Fresh numbers from main while this sits open, since they are worse than the ones in the description. Backend CI on main over the last 24 hours: 37 cancelled, 7 failed, 0 successful. Every one of the cancellations recorded zero jobs, so no runner minutes were reclaimed and nothing surfaced anywhere as a failure. Six of them landed in a ten minute window this morning. Backend CI has not reported a pass on main in a day, and the reason is not that anything is broken: the runs are being evicted while pending, before they start. That is the whole shape of the bug. |

cancel-in-progress: falsedoes not keep a main run alive. Twenty-seven workflows said it does.GitHub cancels any pending run in a concurrency group as soon as a newer one is queued.
cancel-in-progressgoverns only runs that are already executing. From the workflow-syntax reference:Every commit on main shares one group (
${{ github.workflow }}-${{ github.ref }}), so on a branch where pushes land in bursts, the runs in the middle are discarded whatevercancel-in-progresssays.Evidence
studio-ui-smoke.ymlalready records one incident in its own comment: a break sat on main "for 14 hours behind four cancelled runs". The comment added to prevent a recurrence cannot prevent it.And today, five merges:
Each cancelled within a second of the next merge being queued.
6453075chad waited 11 minutes. Main Backend CI completed zero times, and the same happened to all 23 workflows on each commit.Nothing was saved by any of it. Every one of those runs recorded
jobs: 0-- they were discarded before allocating a single job, so they consumed no runner minutes. The cancellation bought nothing and cost the repo its main signal.The change
Every workflow that triggers on push to main keys its concurrency group on
github.shawhen the ref is main:Each main commit gets its own group, so nothing supersedes it and every main run finishes.
Pull requests are untouched. The
shaterm is empty off main, so the group is byte-identical to before and superseded PR runs are still cancelled -- that is the direction where cancelling genuinely saves runner time, and the guard asserts it stays that way.One exception. The two Kaggle workflows keep a shared group. They spend an external GPU quota rather than runner minutes, so there a superseded run really should be dropped instead of replayed per commit.
The guard
tests/studio/test_main_runs_survive_merge_bursts.py:Mutation-checked four ways: reverting one workflow to a shared group, applying
github.shaunconditionally, pointing the exemption at a missing file, and re-adding the old claim to a workflow that still shares a group.On cost
This does mean main runs more during a merge burst, and that is the point rather than a side effect: the alternative on offer was never "cheaper CI", it was "no CI on main". The place to win time back is duplicated work, not discarded runs -- the three OS smoke triples carry ~1,766 lines of test logic inline in YAML at 68-83% similarity, which is tracked separately.