fix(opencode-review): drop head-SHA concurrency scoping, structurally close #1568 instead - #1781
Merged
seonghobae merged 1 commit intoSep 3, 2026
Conversation
…lose #1568 instead Explicit user directive (2026-09-03): head-SHA-scoped concurrency groups (added for Devin Review's #1568 finding -- a delayed, out-of-order run for an older head could cancel the authoritative run already active for a newer head) mean every push to a PR gets its own group, so rapid successive pushes no longer cancel each other's in-flight runs -- they queue up independently instead. That directly worsens the self-inflicted queue-thrashing pattern this org measured directly today (236/300 cancelled runs attributed to concurrent push volume). Refined during cross-session review (host 1's finding, independently verified before adopting): the real fix isn't to re-key the group but to stop cancelling within it. cancel-in-progress: true is what actually causes the #1568 wrongful kill, independent of whether SHA is in the group key -- scope by repo+PR-number only, but flip cancel-in-progress to false. With false, nothing in the group is ever preempted regardless of arrival order, so the #1568 race is structurally impossible here, not just less likely. A now-queued older-head run still gets a turn once the active run finishes, but the poll step's own live-head/live-state revalidation (already run every iteration, needed for correctness regardless of this setting) makes it self-exit within one poll_interval_seconds instead of running to completion or publishing stale evidence. Plain repo+PR-number scoping also means rapid pushes naturally serialize through one queue instead of spawning N independent per-head groups, which is what actually bounds queue depth here. Updated the workflow's own concurrency comment and three test assertions (two in test_opencode_required_verdict_regression.py, one in test_required_workflow_queue_contract.py) that pinned the old head-SHA + cancel-in-progress:true shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 35 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 (3)
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 |
seonghobae
deleted the
claude/revert-opencode-review-head-sha-concurrency
branch
September 3, 2026 05:52
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| github.event.pull_request.head.sha || github.run_id }} | ||
| cancel-in-progress: true | ||
| github.event.pull_request.number || github.run_id }} | ||
| cancel-in-progress: false |
Contributor
There was a problem hiding this comment.
🔴 Pending current-head review eviction
When a current-head run is pending, a later stale event under cancel-in-progress: false replaces it in GitHub's single pending slot. The stale run self-exits, leaving no current-head replacement.
Prompt for agents
The workflow-level repository-and-PR concurrency group uses cancel-in-progress: false, but native GitHub concurrency retains only one pending run and replaces it when another run enters the group. A delayed old-head event can therefore evict the pending current-head run. When the stale survivor eventually starts, its live-head guard exits successfully, leaving no run to dispatch or poll for the current head. Redesign this queue to retain pending events, such as the repository's established queue: max pattern, or move coalescing into a worker that always acts on freshly fetched live state. Add an executable regression for an active run, a pending current-head run, and a subsequently arriving stale-head event.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+35
to
+54
| # own group so this couldn't happen -- but it also meant rapid successive | ||
| # pushes to the SAME PR no longer shared a group at all, so they stopped | ||
| # cancelling each other's in-flight runs and instead queued up | ||
| # independently, directly worsening the self-inflicted queue-thrashing | ||
| # pattern this org measured directly (236/300 cancelled runs attributed to | ||
| # concurrent push volume; see internal memory | ||
| # project_queue_thrashing_self_inflicted_2026_09_03). | ||
| # | ||
| # The actual fix is not to re-key the group but to stop cancelling within | ||
| # it: with cancel-in-progress: false, a late-arriving run for an older head | ||
| # never preempts whichever run is already active, at any arrival order -- | ||
| # the #1568 race is structurally impossible here, not just less likely. | ||
| # The now-queued older-head run still gets a turn once the active run | ||
| # finishes, but by then the poll step's own live-head/live-state | ||
| # revalidation (re-run every iteration, already required for correctness | ||
| # regardless of this setting) sees the head has moved and self-exits within | ||
| # one poll_interval_seconds instead of running to completion or publishing | ||
| # stale evidence. Plain repo+PR-number scoping also means rapid pushes | ||
| # naturally serialize through one queue instead of spawning N independent | ||
| # per-head groups, which is what actually bounds queue depth here. |
Contributor
There was a problem hiding this comment.
🔍 Concurrency rationale lacks durable evidence
The queue-depth rationale conflicts with the repository's documented single-pending behavior. It also cites private memory, violating the durable-knowledge convention. Preserve the evidence publicly and revise the explanation.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
One conflict in tests/test_required_workflow_queue_contract.py: main's #1781 (peer 2) landed opencode-review.yml's own version of the same group-without-SHA + cancel-in-progress:false redesign this branch already made to noema-review.yml. Both elif branches for the two files merged automatically; only the trailing "no file has head.sha in its group" exclusion set conflicted, since each of us had only excluded our own file. Resolved by dropping the exclusion entirely -- neither required PR workflow scopes by head SHA any more as of today, so the assertion now applies unconditionally. 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
#1786) Devin Review caught a real deadlock in #1781's redesign, independently confirmed by two peer sessions before I acted on it: the workflow-level concurrency: block (line 23, before permissions:/jobs:) applied to the ENTIRE run as a unit -- every job in the file, including the structurally-separate cancel-superseded-opencode-review-runs job. With cancel-in-progress: false, a new push's ENTIRE run -- cleanup job included -- could not even start until the group freed up, which only happens when the older run's own opencode-review-target job finishes. Since OpenCode/Noema inference deliberately has no wall-clock deadline, a long-running older-head review could then block the newer head's review indefinitely -- the opposite of what #1781 was supposed to fix. Fixed by moving concurrency: from workflow-level into job-level, scoped only to opencode-review-target (the job that actually runs the long dispatch+poll). This leaves cancel-superseded-opencode-review-runs and the lightweight bootstrap/coverage jobs completely unblocked: they start immediately on every push, and the cleanup job's own direct Actions API cancellation is what frees up the job-level slot for the new push's poll -- no deadlock, and the #1568 stale-cancels-fresh race stays structurally closed at the same time. Matches strix.yml's existing job-scoped-only reference pattern (confirmed to never have had workflow-level concurrency). host 1 applied the equivalent fix to noema-review.yml on #1661 (extracting its cleanup into a genuinely separate job) after finding this same class of bug there first. Updated two test files' assertions to match the new job-level placement, plus added explicit regression guards (no top-level `^concurrency:`, a job-level `^ concurrency:` exists) so this can't silently regress back to workflow-level scoping. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
…ign (#1785) Flagged by host 1 (relayed via peer 1): cancel-superseded-opencode-review-runs' own comment still described "exact-head concurrency" protecting the newer run -- that mechanism was removed in #1781 (bootstrap group is now cancel-in-progress: false, scoped by repo+PR-number only). While fixing it, found this job's actual role is more significant than its old "defense in depth" framing suggested: it's a precise, live-head- reverified, API-based sweep that already implements "cancel only outdated runs of the same PR" (re-checks the live head immediately before selecting cancellation candidates AND immediately before every individual cancel call), so it's immune to #1568's order-blind-preemption bug by construction. Now that the bootstrap group no longer auto-cancels anything natively, this job is the primary active-cancellation path, not a backup. Rewrote the comment to say so. Co-authored-by: Claude Sonnet 5 <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
Explicit user directive (2026-09-03): head-SHA-scoped concurrency groups (added for Devin Review's #1568 finding) mean every push to a PR gets its own group, so rapid successive pushes no longer cancel each other's in-flight runs — they queue up independently instead, worsening the self-inflicted queue-thrashing pattern this org measured directly today (236/300 cancelled runs attributed to concurrent push volume).
Refined during cross-session review (a peer relayed host 1's finding; I independently verified the mechanics before adopting it rather than taking the relay at face value): the real fix isn't to re-key the group but to stop cancelling within it.
cancel-in-progress: trueis what actually causes the fix(ci): exempt draft PRs from OpenCode verdict polling #1568 wrongful kill, independent of whether SHA is in the group key — a late-arriving event for an older head can still cancel the currently-active run for a newer head whenever they share a group.repo+PR-numberonly (drop head SHA, per the user's directive), but flipcancel-in-progresstofalse. Withfalse, nothing in the group is ever preempted regardless of arrival order — the fix(ci): exempt draft PRs from OpenCode verdict polling #1568 race is structurally impossible here, not just less likely.poll_interval_secondsinstead of running to completion or publishing stale evidence.Updated the workflow's own concurrency comment and three test assertions that pinned the old head-SHA +
cancel-in-progress: trueshape.Test plan
coverage run -m pytest tests && coverage report --show-missing— 2680 passed, 1 skipped, 100% coverage.interrogate— 100% docstring coverage.python3 -c "import yaml; yaml.safe_load(open('.github/workflows/opencode-review.yml'))"— parses.🤖 Generated with Claude Code