Let every main run finish instead of cancelling it before it starts by danielhanchen · Pull Request #9082 · unslothai/unsloth · GitHub
Skip to content

Let every main run finish instead of cancelling it before it starts - #9082

Merged
danielhanchen merged 6 commits into
mainfrom
fix-main-pending-cancellation
Aug 17, 2026
Merged

Let every main run finish instead of cancelling it before it starts#9082
danielhanchen merged 6 commits into
mainfrom
fix-main-pending-cancellation

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Aug 17, 2026

Copy link
Copy Markdown
Member

cancel-in-progress: false does 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-progress governs only runs that are already executing. From the workflow-syntax reference:

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

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 whatever cancel-in-progress says.

Evidence

studio-ui-smoke.yml already 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:

09:19:48 -> 09:45:00  cancelled  0ac2e799   Backend CI
09:44:59 -> 09:55:17  cancelled  4d1f9c51   Backend CI
09:55:16 -> 09:56:59  cancelled  9156c36e   Backend CI
09:56:57 -> 10:08:27  cancelled  6453075c   Backend CI
10:08:26 ->           pending    a41f7785   Backend CI

Each cancelled within a second of the next merge being queued. 6453075c had 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.sha when the ref is main:

group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref == 'refs/heads/main' && github.sha || '' }}

Each main commit gets its own group, so nothing supersedes it and every main run finishes.

Pull requests are untouched. The sha term 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:

  1. every workflow triggered on push to main is grouped per commit;
  2. pull requests still get latest-only, so this cannot drift into "never cancel anything";
  3. no workflow claims main protection in a comment while sharing a group across main commits -- read from raw text, since a comment is exactly what YAML discards and the comment is what people act on;
  4. the scan actually matched workflows, since a broken glob would pass the rest.

Mutation-checked four ways: reverting one workflow to a shared group, applying github.sha unconditionally, 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.

danielhanchen and others added 2 commits August 17, 2026 10:06
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.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 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".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@danielhanchen

Copy link
Copy Markdown
Member Author

Fresh evidence while this PR was being written: a fourth consecutive cancellation, this time caused by merging #9080.

09:19:48 -> 09:45:00  cancelled  0ac2e799
09:44:59 -> 09:55:17  cancelled  4d1f9c51
09:55:16 -> 09:56:59  cancelled  9156c36e
09:56:57 -> 10:08:27  cancelled  6453075c
10:08:26 ->           pending    a41f7785

Five main commits, four cancelled Backend CI runs, zero completions. Each cancellation lands within a second of the next merge being queued, and 6453075c had been waiting 11 minutes when it went.

Worth stating because it is the same mechanism from the other side: merging this PR will itself cancel a41f7785's run. That is the last time it happens for these two workflows.

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.
@danielhanchen danielhanchen changed the title Stop a merge burst cancelling main's CI before it starts Let every main run finish instead of cancelling it before it starts Aug 17, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 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():

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

danielhanchen and others added 2 commits August 17, 2026 10:36
…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.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen

Copy link
Copy Markdown
Member Author

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: false spares a run that is already executing and does nothing for one that is still pending, so a merge burst discards the middle of the burst silently.

@chatgpt-codex-connector

Copy link
Copy Markdown

@danielhanchen
danielhanchen merged commit 66188b8 into main Aug 17, 2026
102 of 105 checks passed
@danielhanchen
danielhanchen deleted the fix-main-pending-cancellation branch August 17, 2026 10:59
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.

1 participant