diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 565d63ab62..dd747f47f8 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -20,44 +20,6 @@ on: # publish stale evidence or wait on an impossible verdict. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] -concurrency: - # Scoped by repository + PR number ONLY (not head SHA) with - # cancel-in-progress: false -- by explicit user directive on 2026-09-03, - # refined after review from two peer sessions to fully close the race this - # is actually protecting against, not just trade one failure mode for - # another. - # - # History: head-SHA scoping was added for Devin Review's `#1568` finding -- - # GitHub cancels whichever run is currently active in a concurrency group - # when a new one starts, with no notion of "older"/"newer", so a delayed, - # out-of-order run for an older head could cancel the authoritative run - # already active for a newer head. Scoping by head SHA gave each push its - # 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. - group: >- - opencode-review-bootstrap-${{ - github.event.pull_request.base.repo.full_name || github.repository }}-${{ - github.event.pull_request.number || github.run_id }} - cancel-in-progress: false - permissions: contents: read pull-requests: read @@ -291,6 +253,64 @@ jobs: name: opencode-review needs: [coverage-evidence] runs-on: ubuntu-24.04 + # Job-level (not workflow-level) on purpose: a workflow-level concurrency + # block applies to the ENTIRE run as a unit -- every job in the file, + # including the structurally-separate cancel-superseded-opencode-review-runs + # job below. That created a real deadlock (Devin Review, 2026-09-03, + # confirmed independently by two peer sessions before I acted on it): 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 this design is supposed to + # fix. Scoping the group to ONLY this job (the one that actually runs the + # long dispatch+poll) leaves cancel-superseded-opencode-review-runs + # completely unblocked: it starts immediately on every push and cancels + # the older run via a direct Actions API call, which releases this job's + # own concurrency slot for the new push's instance -- no deadlock, and the + # #1568 stale-cancels-fresh race stays structurally closed (see + # cancel-in-progress below) at the same time. + concurrency: + group: >- + opencode-review-bootstrap-${{ + github.event.pull_request.base.repo.full_name || github.repository }}-${{ + github.event.pull_request.number || github.run_id }} + # Scoped by repository + PR number ONLY (not head SHA) with + # cancel-in-progress: false -- by explicit user directive on + # 2026-09-03, refined after cross-session review to fully close the + # race this is actually protecting against, not just trade one + # failure mode for another. + # + # History: head-SHA scoping was added for Devin Review's `#1568` + # finding -- GitHub cancels whichever run is currently active in a + # concurrency group when a new one starts, with no notion of + # "older"/"newer", so a delayed, out-of-order run for an older head + # could cancel the authoritative run already active for a newer head. + # Scoping by head SHA gave each push its 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. + cancel-in-progress: false permissions: contents: read pull-requests: read diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 47683878a7..db4e73d1b1 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -4,6 +4,7 @@ import json import os +import re import shutil import subprocess import textwrap @@ -602,7 +603,7 @@ def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None: def test_opencode_review_concurrency_group_is_scoped_by_repo_and_pr_only() -> None: - """The bootstrap group is keyed by repo + PR number only, and never cancels. + """The concurrency group is keyed by repo + PR number only, and never cancels. Devin Review on `#1568` originally found that a delayed, out-of-order run for an older head could cancel the authoritative run already active for @@ -624,10 +625,20 @@ def test_opencode_review_concurrency_group_is_scoped_by_repo_and_pr_only() -> No iteration for correctness) is what makes a now-queued older-head run self-exit quickly once it finally gets its turn, instead of running to completion or publishing stale evidence. + + Also confirms the group is JOB-level (on opencode-review-target only), + not workflow-level: a workflow-level block would capture the + structurally-separate cancel-superseded-opencode-review-runs job too, + deadlocking it behind the very run it's supposed to cancel (Devin + Review, 2026-09-03, confirmed independently before this fix landed). """ workflow = WORKFLOW.read_text(encoding="utf-8") - concurrency_block = workflow.split("\n\nconcurrency:\n", 1)[1].split( - "\n\npermissions:", 1 + assert not re.search(r"(?m)^concurrency:", workflow) + target_job = workflow.split("\n opencode-review-target:\n", 1)[1].split( + "\n cancel-superseded-opencode-review-runs:", 1 + )[0] + concurrency_block = target_job.split(" concurrency:\n", 1)[1].split( + "\n permissions:", 1 )[0] assert "github.event.pull_request.head.sha || github.run_id" not in concurrency_block assert "github.event.pull_request.number || github.run_id" in concurrency_block diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 82bca9448e..bc3943cc1a 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -253,6 +253,13 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: or ("github.event_name == 'pull_request'" in concurrency_contract) ) elif filename == "opencode-review.yml": + # Job-level (scoped to opencode-review-target only), not + # workflow-level: a workflow-level block would capture the + # structurally-separate cancel-superseded-opencode-review-runs + # job too, deadlocking it behind the very run it exists to + # cancel (Devin Review, 2026-09-03). + assert not re.search(r"(?m)^concurrency:", workflow) + assert re.search(r"(?m)^ concurrency:", workflow) assert "opencode-review-bootstrap-" in concurrency_contract # Deliberately NOT scoped by head SHA and deliberately # cancel-in-progress: false (reverted/refined 2026-09-03 by