fix(ci): dedupe strix.yml's cancel-superseded-pr-runs cleanup job - #1784
Merged
Conversation
The cleanup job that retires superseded/closed-PR Strix scan runs had no concurrency group of its own, so a push burst spawned N independent instances that each competed 1:1 for the same scarce 60-job Actions-plan admission ceiling the job exists to relieve -- the identical failure class cbd1280 already fixed for current-head-run-coalescer.yml, just not yet applied here. Adds a PR-scoped (repo+PR-number), cancel-in-progress:true concurrency group, matching codeql-pr.yml's established group-key style. cancel-in-progress:true (not queue:max, which the coalescer needs because each of its queued instances carries a different specific expected-head only it can act on) is correct here because every sweep re-verifies live PR state before selecting or cancelling anything, so a fresh instance always fully subsumes whatever an older, not-yet-run instance would have done -- nothing is lost by cancelling a stale queued/running sweep. Originally designed to apply the same fix to opencode-review.yml's sibling cancel-superseded-opencode-review-runs job. Dropped that half after discovering 8141b99 (landed on origin/main mid-session, after the design was written) already restructured that workflow's WORKFLOW-level concurrency group to repo+PR-scoped/cancel-in-progress:false -- a strictly stronger fix that caps the entire run (including this cleanup job) to at most one active instance per PR, for every event type. A job-level dedup on top would have been redundant on the common path, and its planned comment (asserting the outer group "must stay head-scoped") would no longer match the file. Required companion test edit: test_strix_serializes_provider_evidence_per_repository_and_pr isolates the strix: job's own text before locating "concurrency:", since cancel-superseded-pr-runs above it now carries its own block and would otherwise win the naive first-match split. Verified: coverage run -m pytest tests (2680 passed, 1 skipped), coverage report --show-missing (100%), interrogate (100%), actionlint (clean except a pre-existing unrelated shellcheck style note), yaml.safe_load on the touched workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| cancel-superseded-pr-runs-${{ | ||
| github.event.pull_request.base.repo.full_name || github.repository }}-${{ | ||
| github.event.pull_request.number || github.run_id }} | ||
| cancel-in-progress: true |
Contributor
There was a problem hiding this comment.
🔴 Late events cancel current cleanup
When an older webhook arrives late, cancel-in-progress: true cancels newer cleanup before either job verifies live pull-request state. The stale replacement exits, leaving obsolete scans active.
Prompt for agents
Redesign cancel-superseded-pr-runs concurrency so a delayed older pull_request_target event cannot preempt the worker for newer live state and then no-op. The current PR-scoped cancel-in-progress group cancels by admission order, while live_target_matches binds each replacement to its event payload. Consider making every cleanup instance operate idempotently from freshly fetched live PR state, including the live head or closed state, so even a stale event fully subsumes an interrupted worker. Preserve the bounded admission objective and add a regression for an old synchronize event arriving while a current synchronize or close cleanup is running.
Was this helpful? React with 👍 or 👎 to provide feedback.
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
1 participant
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.
Summary
cancel-superseded-pr-runs(the job that retires superseded/closed-PR Strix scan runs) had no concurrency group of its own, so a push burst spawned N independent instances that each competed 1:1 for the same scarce 60-job Actions-plan admission ceiling this job exists to relieve.repo+PR-number),cancel-in-progress: trueconcurrency group, matchingcodeql-pr.yml's established group-key style. This is the identical failure classcbd1280(fix(actions): coalescer needs queue:max, not just cancel-in-progress:false #1775) already fixed forcurrent-head-run-coalescer.yml, just not yet applied here.cancel-in-progress: true(notqueue: max, which the coalescer needs because each of its queued instances carries a different specific expected-head only it can act on) is correct here: every sweep re-verifies live PR state before selecting or cancelling anything, so a fresh instance always fully subsumes whatever an older, not-yet-run instance would have done — nothing is lost by cancelling a stale queued/running sweep.What changed from the original design
The design also proposed the same fix for
opencode-review.yml's siblingcancel-superseded-opencode-review-runsjob. That half was dropped after discovering8141b99(landed onorigin/mainmid-session, after the design was written) already restructuredopencode-review.yml's workflow-level concurrency group to repo+PR-scoped/cancel-in-progress: false— a strictly stronger fix that caps the entire run (bootstrap, poll, and this cleanup job) to at most one active instance per PR, for every event type. A job-level dedup on top would have been redundant on the common path, and its planned comment (asserting the outer group "must stay head-scoped") would no longer have matched the file.strix.ymlhas no such workflow-level group —changed-scopeandcancel-superseded-pr-runsremain fully exposed to push-burst admission waste regardless of the recent#1779/#1781changes (those only touched thestrix:job's own group) — so this half of the fix remains fully necessary.Test plan
coverage run -m pytest tests— 2681 passed, 1 skippedcoverage report --show-missing— 100% (this PR touches noscripts/cifiles; the one required companion test edit still runs green)interrogate— 100%actionlint .github/workflows/strix.yml— clean except one pre-existing, unrelated shellcheck style note (verified present on unmodifiedorigin/maintoo, just at a different line offset)python3 -c "import yaml; yaml.safe_load(open('.github/workflows/strix.yml'))"— OKtest_strix_serializes_provider_evidence_per_repository_and_pr) updated to isolate thestrix:job's own text before locating itsconcurrency:block, sincecancel-superseded-pr-runsabove it now carries its own block and would otherwise win the naive first-match string split.🤖 Generated with Claude Code