ci: stop three main gates being starved to zero executions (#7205) - #7357
Merged
Conversation
Measured on main, last ~10 runs each:
security-audit 9 cancelled, 0 success <- REQUIRED context
eh-transport 8 cancelled, 0 success
llvm-inprocess 8 cancelled, 0 success
gc-moving-witness 0 cancelled, 2 success <- already fixed
Two of the three had a comment claiming they were already safe:
"cancel superseded PR runs, never main runs -- a busy merge day would
otherwise starve the gate to zero executions." A busy merge day starved
them anyway, by a mechanism the comment did not anticipate.
is not sufficient. GitHub allows at most one
PENDING run per concurrency group and cancels the previously pending one
when a new run enters, regardless of that setting. With the group keyed
on , every main push shares one group, so a merge burst
cancels the intermediate runs. gc-moving-witnesses already carries both
the diagnosis and the fix (#7205); this applies it to the three that
still had the old shape. security-audit additionally had
, which cancels main runs outright.
Keying push runs on the SHA gives every merged commit its own group.
This is CLAUDE.md hazard 3, and it is worth noting how it was found: not
by reading the config -- two of these LOOKED correct -- but by asking
what each workflow's main runs actually concluded. llvm-inprocess is the
gate for the backend #7353 just made the default, and it had never
executed on main.
📝 WalkthroughWalkthroughThree GitHub Actions workflows now group runs by event and commit or reference. Push runs are not cancelled by later runs. Pull-request runs remain cancellable. ChangesCI concurrency behavior
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Measured on
main, last ~10 runs each:security-auditeh-transportllvm-inprocessgc-moving-witnessesTwo of the three had a comment claiming they were already safe:
A busy merge day starved them anyway, by a mechanism that comment didn't anticipate.
cancel-in-progress: falseis not sufficient: GitHub allows at most one pending run per concurrency group and cancels the previously pending one when a new run enters, regardless of that setting. With the group keyed ongithub.ref, every main push shares one group, so a merge burst cancels the intermediates.gc-moving-witnesses.ymlalready carries both the diagnosis and the fix (#7205) — keying push runs ongithub.shaso every merged commit gets its own group. This applies it to the three still on the old shape.security-auditadditionally hadcancel-in-progress: true, which cancels main runs outright.How it was found, because the method matters
Not by reading the config — two of these looked correct, and I'd have skipped them. It came from watching what main runs actually concluded after the flip: a
cancellednotification that looked benign turned out to be the eighth in a row.That's CLAUDE.md's hazard 3 in a form the existing guard was written against and still didn't stop.
cache-warmis also 9/9 cancelled but deliberately left alone — it's an idempotent cache warmer, where cancelling a superseded run is the intended behaviour.Config-only.
Summary by CodeRabbit