fix(opencode-review): scope concurrency to opencode-review-target only - #1786
Merged
Merged
Conversation
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>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOpenCode 리뷰 워크플로우의 동시성 설정을 워크플로우 수준에서 ChangesOpenCode 리뷰 동시성
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # rapid pushes naturally serialize through one queue instead of | ||
| # spawning N independent per-head groups, which is what actually | ||
| # bounds queue depth here. | ||
| cancel-in-progress: false |
Contributor
There was a problem hiding this comment.
🟡 Delayed events cancel current reviews
When a stale event arrives while the current-head job is pending, cancel-in-progress: false still replaces that pending job. The current head loses its required review.
Prompt for agents
The opencode-review-target job uses one repository-and-PR concurrency group with cancel-in-progress false. GitHub concurrency still allows only one pending job per group and cancels an existing pending job when another member arrives. A delayed old-head pull_request_target run can therefore replace the authoritative current-head job while another job occupies the group. The stale replacement later exits after live-head validation, leaving no current-head review job or event to restart it. Redesign the serialization so stale arrivals cannot evict the authoritative pending job. Preserve the cleanup job's ability to run outside the serialized review job and retain live-head validation before dispatch and polling.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+294
to
+297
| # 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). |
Contributor
There was a problem hiding this comment.
🔍 Concurrency evidence is not durable
The queue-thrashing rationale cites internal memory instead of a repository or Project record. Preserve the measurement and methodology in a durable source.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
No conflicts; picked up main's #1786 (peer 2) -- opencode-review.yml's deadlock-risk fix, same shape as noema-review.yml's e1e7ae8: concurrency moved from workflow-level to job-level scoped to opencode-review-target only, so cancel-superseded-opencode-review-runs is no longer trapped behind the review job's own group. Verified the structure directly (concurrency now lives inside opencode-review-target, no workflow-level block remains). All three central review workflows (strix.yml, opencode-review.yml, noema-review.yml) now correctly separate their long-running review job's concurrency from their cleanup job's admission. Full suite (2705 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
#1786 closes the same-shape bug on main; all three central review workflows now correctly separate cleanup-job admission from the review job's own concurrency group. 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
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, beforepermissions:/jobs:) applied to the ENTIRE run as a unit — every job in the file, including the structurally-separatecancel-superseded-opencode-review-runsjob.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 ownopencode-review-targetjob 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.Fix: moved
concurrency:from workflow-level into job-level, scoped only toopencode-review-target(the job that actually runs the long dispatch+poll). This leavescancel-superseded-opencode-review-runsand 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. Matchesstrix.yml's existing job-scoped-only reference pattern (confirmed it never had workflow-level concurrency).host 1 applied the equivalent fix to
noema-review.ymlon #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.Test plan
coverage run -m pytest tests && coverage report --show-missing— 2681 passed, 1 skipped, 100% coverage.interrogate— 100% docstring coverage.python3 -c "import yaml; yaml.safe_load(open('.github/workflows/opencode-review.yml'))"— parses, confirmed concurrency is job-level not workflow-level.🤖 Generated with Claude Code
Summary by CodeRabbit
버그 수정
테스트