fix(ci): restore PR-scoped concurrency for strix.yml on owner directive - #1779
Merged
Conversation
strix.yml's concurrency group was deliberately repository-wide (not PR-scoped), from 2026-08-24 through today, following a real litellm.RateLimitError storm against the shared NVIDIA NIM key on 2026-08-23/24 (.github#1297) when the group was PR-scoped. This meant one PR's Strix scan queued behind whichever OTHER PR in the same repo happened to be scanning already -- a real, previously-documented cross-PR starvation cause (see queue-congestion investigation). The user gave a direct, explicit, repeated instruction today that every central workflow's concurrency group should use a "workflow name - repository name - PR number" pattern -- already true everywhere except this file. When told strix.yml was the one deliberate exception (with the 2026-08 incident explained), the user explicitly said the historical concern doesn't change their instruction, and separately confirmed the two NVIDIA NIM credentials (NVIDIA_NIM_API_KEY, NVIDIA_NIM_API_KEY_SUB) have independent rate limits rather than a shared pool -- giving materially more headroom than the single-key 2026-08-23/24 incident had. Added the PR number (or client_payload.pr_number, matching the existing repository_dispatch fallback pattern) to the concurrency group key. cancel-in-progress stays false: a same-PR push still queues behind an in-flight scan for that PR rather than cancelling it, preserving that head's scan log -- the separate trusted cancel-superseded-pr-runs cleanup job already retires a genuinely superseded head independently of this native concurrency block. Updated tests/test_required_workflow_queue_contract.py's two contract tests that pinned the old repository-only scoping to assert the new repository+PR scoping instead, preserving the historical rationale in the docstring/comment for future readers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 47 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}
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
Supersedes 31e46db. That fix added the head SHA to the concurrency group to stop a delayed old-head event from cancelling a valid newer-head run (#1568-class hazard) -- correct, but costly: a SHA-scoped group gives every push its own group, so under this org's saturated Actions ceiling a stale per-push run is only retired by a separate cleanup job that shares that same congested admission queue and can itself sit stuck behind it, letting superseded runs pile up. The user pointed at the actual fix directly: the hazard is caused by cancel-in-progress:true killing whichever run is active with no older/newer notion, not by the group lacking SHA. Setting cancel-in-progress:false closes it completely regardless of group scoping -- the active run is never preempted -- while GitHub's unconditional single-pending-slot rule still evicts an intermediate queued push for free. The pre-existing "Reject a stale trigger" step already guarantees whichever instance runs never reports against the wrong commit either way. Independently corroborated same-day: strix.yml's own concurrency fix (64e67ef, #1779) already used this exact shape; a peer session applied the identical pattern to opencode-review.yml after re-deriving the same reasoning independently. Full suite (2704 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
No conflicts; auto-merged main's strix.yml PR-scoped-concurrency fix (#1779, 64e67ef) alongside this branch's noema-review.yml redesign (f9d4373) -- both land on the same cancel-in-progress:false shape for the same reason, independently derived. Full suite (2704 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 3, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
(#1788) PR #1779 changed strix.yml's concurrency group.format() expression from the 2-argument form to the 3-argument form format('{0}-{1}-{2}', ...) to restore PR-scoped concurrency (fixing a queue-saturation chicken-egg problem) while keeping repository+event-class isolation. The bash contract test scripts/ci/test_strix_quick_gate.sh was never updated to match, so two assertions in assert_strix_workflow_pr_trigger_hardened() kept checking for the old 2-argument format('{0}-{1}', ...) literal and now fail on every PR regardless of that PR's own diff. The parallel Python contract in tests/test_required_workflow_queue_contract.py was already correctly updated for the 3-argument form at the time of PR #1779 -- only the bash side drifted, the same class of bug PR #1750 fixes for a stale cron assertion in this same file. Updated both stale assertions to the current 3-argument format('{0}-{1}-{2}', ...) literal, preserving their semantic intent and messages unchanged. Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 Co-authored-by: Claude <noreply@anthropic.com>
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
strix.yml's concurrency group was deliberately repository-wide (not PR-scoped) from 2026-08-24 through today, following a reallitellm.RateLimitErrorstorm against the shared NVIDIA NIM key on 2026-08-23/24 (.github#1297) when the group was PR-scoped. That fixed the storm but starved cross-PR Strix evidence within the same repository instead — a different PR's scan always queued behind whichever scan was already running there.The owner gave a direct, explicit instruction today that every central workflow's concurrency group should use a "workflow name - repository name - PR number" pattern — already true everywhere except this file (see the earlier org-wide concurrency audit on
.github#1768). When told strix.yml was the one deliberate exception, with the 2026-08 incident explained, the owner explicitly said the historical concern doesn't change the instruction, and separately confirmed the two NVIDIA NIM credentials (NVIDIA_NIM_API_KEY,NVIDIA_NIM_API_KEY_SUB) have independent rate limits rather than a shared pool — giving materially more headroom than the single-key incident had.Changes
client_payload.pr_number, matching the existingrepository_dispatchfallback pattern) to the concurrency group key.cancel-in-progressstaysfalse: a same-PR push still queues behind an in-flight scan for that PR rather than cancelling it, preserving that head's scan log — the separate trustedcancel-superseded-pr-runscleanup job already retires a genuinely superseded head independently of this native concurrency block.tests/test_required_workflow_queue_contract.pythat pinned the old repository-only scoping, preserving the historical rationale in the docstring/comment for future readers.Test plan
PYTHONPATH=. python3 -m pytest tests/test_required_workflow_queue_contract.py -q— 73 passedPYTHONPATH=. python3 -m pytest tests -q(full suite) — 2680 passed, 1 skipped, 21 subtests passed, zero regressionspython3 -c "import yaml; yaml.safe_load(...)"on the edited workflow — YAML OK🤖 Generated with Claude Code