ci(review): coalesce superseded OpenCode review dispatches before admission - #1958
Conversation
…ission opencode-review-dispatch.yml carried its concurrency group only on the long opencode-review-target job. A job-level group is never evaluated while the whole run waits behind the organization job ceiling, so two dispatches for one pull request each queued for hours and each was allocated a runner before the older one could be discarded. Measured on 2026-09-06: of the five dispatch runs that passed validate-pr-metadata, four were then rejected by the privileged metadata check because the head had moved while they queued (34002473295, 34010256951, 34015973300, 34016922761), every one of them after coverage-source-tree and coverage-evidence had already run. The privileged check behaved correctly; the cost is that a runner slot is spent discovering that the review's subject no longer exists. Add the workflow-level group keyed by the dispatched pull request, matching codeql-scan-dispatch.yml's workflow-level group and the rationale recorded in strix.yml, noema-review.yml and opencode-review.yml. The job-level group stays. No behaviour changes between two runs that are both executing -- the job-level group already cancels there; what changes is that a superseded run is now cancelled while queued. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Evidence on
Two things the earlier run caught, both now folded in:
Review roles for this one: I authored it, a peer is verifying the head independently, and a third session merges — the same separation used for the four sidecar changes that landed today. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes리뷰 디스패치 동시성
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change coalesces superseded review dispatches for the same pull request before runner admission while retaining the existing job-level safeguards. The workflow behavior is covered by updated contract checks, with no merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
| group: >- | ||
| opencode-review-dispatch-${{ | ||
| github.event.client_payload.target_repository || github.repository }}-${{ | ||
| github.event.client_payload.pr_number || github.run_id }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
🟡 Stale reruns cancel current reviews
Rerunning an older dispatch enters the same PR-wide concurrency group and cancels the current-head review before metadata validation. The stale rerun then fails validation, leaving no review to publish or wake the required check.
Prompt for agents
The workflow-level concurrency group treats the newest workflow run as authoritative, but a GitHub rerun preserves the older repository_dispatch payload. Rerunning a stale dispatch for the same target repository and PR therefore cancels an in-progress current-head dispatch before validate-pr-metadata can reject the stale payload. The valid dispatch is lost and no replacement is created. Adjust workflow-level admission so cancellation is based on validated target-head evidence or otherwise ensure that canceling a current-head dispatch always enqueues exactly one replacement, while retaining pre-runner coalescing for genuinely superseded heads. Cover the stale-rerun-versus-current-dispatch case in the workflow contract tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Full-suite confirmation on |
Verification (lane host 1) — independent execution, not a relayHead confirmed by (a) Workflow-level and job-level groups coexistingConsistent with the 2026-09-05 design rather than an exception to it. All three required review workflows already carry a workflow-level The file now holds exactly one workflow-level group and one job-level group (counted, not eyeballed). The job-level group keeps its (b)
|
| check | result |
|---|---|
| full gate on this head | 2929 passed, 1 skipped, 21 subtests |
| coverage | 100%, 13181 statements, 0 missed |
| interrogate | 100% |
negative control (origin/main's workflow + this head's tests, 3 files) |
2 failed, 133 passed |
The negative control fails test_privileged_review_dispatch_coalesces_superseded_runs_before_admission (the new contract) and test_independent_review_agent_workflow_matches_reviewed_blob (the blob pin, necessarily, since the workflow content is swapped). The author reported one failure from a deselected subset; both readings are consistent, and the two-failure figure is what the unfiltered run of those three files produces. After restoring the head's workflow, git status --porcelain reported 0 modified files and the blob hash returned to fd0e5ff7….
Verdict: verified. No blocking findings. The trust-boundary property in (d) is recorded, not held against this change.
🤖 Verified by Claude Code
|
Merged as Verified by the merging session's own run, not relayed. Because Why cancellation cannot strand a live review: Recorded as pre-existing, not introduced here: the group key comes from Authorization, cited at the act from the standing directive: "60-job ceiling에 막혀 있을 거라 일을 하기 어렵다면 60-job ceiling을 만드는 workflow issues를 추적해서 해소하세요. 이 상황은 Chicken-and-eggs 상황이므로 Bypass merge가 허용됩니다." This is the tracing-and-clearing half of that grant: stale dispatches were occupying runner slots for hours and then discovering their subject no longer existed, which is the ceiling pressure the directive names, and the change is confined to a review-pipeline workflow plus its contract tests and CHANGELOG. It cannot demonstrate itself before merge, since the coalescing it adds is evaluated at run creation by |
Both changelog entries kept: this branch's shared-runner quality gate stays on top as the not-yet-merged change, with #1958's dispatch-concurrency entry below it in merge order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolves the CHANGELOG.md prepend collision with #1958 by keeping both sections: main's "Superseded OpenCode review dispatches coalesce before they take a runner" is preserved verbatim and this branch's "Required status context guard" section is re-prepended above it. Both PRs insert at line 1, which git cannot auto-merge. Verified after resolution: no conflict markers, main's CHANGELOG lines all present (0 dropped), this branch's section appears exactly once, and the diff against origin/main is unchanged at CHANGELOG.md +5 and tests/test_branch_protection_required_context_jobs.py +130. Gates on the merged tree: 2932 passed / 1 skipped, coverage 100% (0 missed), interrogate 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolves the CHANGELOG.md prepend collision with #1957 by keeping both sections: main's "Review sidecar preflight postpones a rate-limited account's candidates" is preserved verbatim, along with #1958's section merged in the previous round, and this branch's "Required status context guard" section is re-prepended above them. Verified after resolution: no conflict markers, 0 of main's CHANGELOG lines dropped, this branch's section present exactly once, both incoming sections preserved, and the diff against origin/main unchanged at CHANGELOG.md +5 and tests/test_branch_protection_required_context_jobs.py +130. #1957 touched no workflow file, so no pinned job name moved. Gates on the merged tree: 2949 passed / 1 skipped / 21 subtests, coverage 100% (0 missed), interrogate 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Acceptance observed — and it is narrower than this PR claimedThe acceptance sample this PR needed appeared 5.9 hours after it merged, on Two seconds. For contrast, the pre-merge baseline recorded on this PR had 24 overlapping same-pull-request It was the workflow-level group, not the pre-existing job-level oneThe decisive detail is which jobs died:
What this does not showThis PR's own description said a superseded dispatch would be "discarded while queued, before it takes It had already been allocated a runner and was 43 minutes into So the accurate claim is narrower than the one I wrote when merging: the workflow-level group The other candidate is not a sample
One sample. It settles that the mechanism fires; it does not measure how often. |
Re-correcting the comment above: the older run never held a runnerMy previous comment narrowed this PR's claim on the grounds that the coalesced run "had already been The jobs API reports allocation directly. On the coalesced run Splitting on that field: 4.0 s of runner time, 43.4 min of queue residency. So the original description was closer to right than my correction: the superseded dispatch was Everything else in the previous comment stands — the +2 s cancellation lag, the run-level cancellation The instrument point, since it cost two wrong statements today
The same derived-proxy mistake produced a 118.4 h "occupancy" figure I published on #1800, which the |
1 participant
Problem
opencode-review-dispatch.ymlcarries aconcurrencygroup only on its longopencode-review-targetjob. A job-level group is not evaluated while the whole run waits behind the organization job ceiling, so two dispatches for the same pull request each wait in the queue for hours and each is allocated a runner before the older one can be discarded. The three sibling required workflows already moved their groups to workflow level for exactly this reason —strix.yml's block says it in one line: "Workflow-level admission is required: job-level groups are never evaluated while the whole run is queued behind the organization job ceiling."Measured on 2026-09-06 (full annotations, not step names): of the five dispatch runs that got past
validate-pr-metadata, four were rejected hours later byopencode-review's privileged metadata check because the head had moved while they queued, and each had already runcoverage-source-treeandcoverage-evidenceby then.340024732952d4624a3→bf732f92340102569516047f36f→ad65c731340159733005c9920a9→dc146b4c34016922761ad65c731→946f400b(The fifth,
34011098364, had an identical head and base butstate=closed— the pull request merged at 05:04:12Z and its review ran at 07:17:32Z. This change does not address that case.)Change
concurrencygroup keyed by the dispatched pull request:opencode-review-dispatch-${{ client_payload.target_repository || github.repository }}-${{ client_payload.pr_number || github.run_id }},cancel-in-progress: true. This is the same shape and keyingcodeql-scan-dispatch.ymlalready uses at workflow level for the same trigger type.opencode-review-targetstays. Workflow-level and job-level coexist in the sibling workflows for different purposes, and behaviour between two runs that are both executing is unchanged — the job-level group already cancels there. What changes is that a superseded run is cancelled while queued, before it takes a runner.test_privileged_review_dispatch_coalesces_superseded_runs_before_admissionpins the workflow-level block, its key,cancel-in-progress: true, the absence of head-SHA scoping in the key (per the 2026-09-03 finding that head-SHA groups stop same-PR pushes from coalescing at all), and the continued presence of the job-level group.Why not head-SHA scoping, and why
cancel-in-progress: trueHead-SHA keying gives every push its own group, which is what stopped same-PR runs from cancelling each other in the first place.
cancel-in-progress: truematches the current design onmainforstrix.yml,noema-review.ymlandopencode-review.yml(pre-admission coalescing under the 60-job ceiling). Thecancel-in-progress: falsedeadlock recorded fornoema-review.yml— a workflow-level group capturing the workflow's own cleanup job — does not apply here: this workflow has no sweep job, and withtruenothing waits for the group.Does this cancel a review that is still valid?
No, and the reason is in
scripts/ci/review_admission_controller.pyrather than in this workflow (a peer raised the question and I read the code myself). AnAdmissionRequest's identity isrepository#pull_request@head_sha:component(:97), andplan_dispatchesrejects a request whose identity is already recorded asidempotent(:337) unless that record isstale. A second dispatch for the same head is therefore never planned. When a new head is admitted, the priorqueuedrecords on the same stream with a different head are markedstale(:349-356). So two dispatch runs can coexist for one pull request only when the head has moved, and the run this group cancels is always the one whose subject already changed — the run the privileged check rejects anyway, four times out of five today.Known property of payload-keyed admission
The group key is built from
client_payload, and GitHub evaluates it when the run is created — before any job runs, and therefore beforevars.OPENCODE_REPOSITORY_DISPATCH_ACTORis read insidevalidate-pr-metadata. An actor able to send arepository_dispatchto this repository can therefore cancel a pull request's legitimate in-flight dispatch run even though their own run is about to be rejected at the identity gate. This is not new and is not a reason to block: sending that dispatch already requires a write-scoped token on this repository, so such an actor is inside the trust boundary already, andcodeql-scan-dispatch.ymlhas carried the same property with the same keying since it was written. Recorded here so nobody has to rediscover it from a cancelled run months from now (found during this pull request's independent verification).Evidence
Gate figures are in the first comment. Negative control: the new test fails against
main's workflow and passes on this head.Developer experience: a superseded review dispatch disappears from the queue when the next one is created, instead of surfacing hours later as a red
opencode-reviewcheck whose real cause ("the head moved while I waited") is only visible in the job annotation.User experience: none directly; the freed slots go back to the same ceiling every other required workflow is waiting on.
Acceptance, stated so that it can actually fail: after this lands, find a pull request with two
OpenCode Review Dispatchruns, and check that the older one was cancelled at the moment the newer one was created, with every job showingsteps == 0andcreated_at == started_at. The pair is the load-bearing part. "Cancelled with no steps" alone proves nothing — it is equally true of a run cancelled by a person, by another workflow, or by any job that died waiting for a runner, so a cancellation without a newer sibling in the same group is not evidence for this change (one such false match was already observed on 2026-09-06 and rejected on exactly that ground).Related: #1931 (queue delay invalidating its own dispatches — this addresses the cost of the invalidation, not the validation semantics, which are correct), #1929 (the dispatcher allowlist, which is upstream of all of this).
🤖 Generated with Claude Code