From d3c63a3251b427c7c835b81f77f6c191239d0b3f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 3 Sep 2026 14:51:24 +0900 Subject: [PATCH] fix(opencode-review): drop head-SHA concurrency scoping, structurally close #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 --- .github/workflows/opencode-review.yml | 60 ++++++++++++------ ...st_opencode_required_verdict_regression.py | 61 +++++++++++-------- .../test_required_workflow_queue_contract.py | 27 +++++--- 3 files changed, 95 insertions(+), 53 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 9c2ff1711e..565d63ab62 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -10,31 +10,53 @@ on: # isolated in opencode-review-dispatch.yml on repository_dispatch only. pull_request_target: # `converted_to_draft` is included so a PR going draft mid-poll fires a - # fresh run of this same workflow: the head-scoped concurrency group below - # (`cancel-in-progress: true`) cancels any in-flight non-draft - # "Fail closed without a current-head OpenCode verdict" poll for that - # exact same head. Every non-closed admission path revalidates the live - # PR/head/state before dispatching, exempting, or polling so out-of-order - # draft/ready/closed events cannot publish stale evidence or wait on an - # impossible verdict. + # fresh run of this same workflow. That new run does NOT cancel the old + # one (see the concurrency block below): the in-flight "Fail closed + # without a current-head OpenCode verdict" poll for the prior state + # instead notices the live draft flag itself on its own next iteration + # and self-exits within one poll_interval_seconds. Every non-closed + # admission path revalidates the live PR/head/state before dispatching, + # exempting, or polling so out-of-order draft/ready/closed events cannot + # publish stale evidence or wait on an impossible verdict. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: - # Scoped by exact head SHA (not just PR number) so a delayed, out-of-order - # run for an older head cannot cancel the authoritative run already active - # for a newer head -- GitHub cancels whichever run is currently active in - # the group when a new one starts, with no notion of "older"/"newer", so - # sharing a group across different heads let a stale event retire the - # current head's still-valid run before its own live-head check could ever - # reject it (Devin Review on `#1568`). Same-head events (draft<->ready - # transitions, a synchronize retry) still share one group, so - # `converted_to_draft` still cancels an active same-head verdict poll. + # 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 }}-${{ - 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 permissions: contents: read diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 05993face8..47683878a7 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -560,15 +560,18 @@ def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll( Devin Review on `#1568` found that `converted_to_draft` was missing from this workflow's `pull_request_target.types`, so converting a PR to draft - while an earlier event's "Fail closed" poll was still running never fired - a fresh run to cancel it via the PR-scoped `cancel-in-progress: true` - concurrency group -- the stale non-draft poll kept waiting for a verdict - the now-draft PR can never receive. Adding `converted_to_draft` to the - trigger set lets a fresh run's draft exemption below take over; this test - proves that exemption exits before ever reaching the Reviews API for the - exact `PR_ACTION=converted_to_draft` value GitHub sends for that event - (`PR_DRAFT` is always `"true"` on that event, mirroring GitHub's own - payload). + while an earlier event's "Fail closed" poll was still running left the + stale non-draft poll waiting for a verdict the now-draft PR can never + receive -- nothing re-triggered it to notice sooner. Adding + `converted_to_draft` to the trigger set doesn't cancel that in-flight + poll (the concurrency group is `cancel-in-progress: false`, see the + workflow's own comment); instead it's the in-flight poll's own live-state + recheck (already run every iteration) that notices the draft flag on its + next pass and exits within one `poll_interval_seconds`. This test proves + the step-level exemption logic that recheck relies on exits before ever + reaching the Reviews API for the exact `PR_ACTION=converted_to_draft` + value GitHub sends for that event (`PR_DRAFT` is always `"true"` on that + event, mirroring GitHub's own payload). """ result = _run_fail_closed_step( tmp_path, pr_action="converted_to_draft", pr_draft="true" @@ -595,32 +598,40 @@ def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None: "types: [opened, synchronize, reopened, ready_for_review, " "converted_to_draft, closed]" ) in trigger_block - assert "cancel-in-progress: true" in workflow + assert "cancel-in-progress: false" in workflow -def test_opencode_review_concurrency_group_is_scoped_by_exact_head() -> None: - """The bootstrap concurrency group is keyed by head SHA, not just PR number. +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. - Devin Review on `#1568` found that a delayed, out-of-order run for an - older head could cancel the authoritative run already active for a - newer head: GitHub cancels whichever run is currently active in a + 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 + a newer head (GitHub cancels whichever run is currently active in a concurrency group when a new one starts, with no notion of "older" or - "newer", so a group shared across different heads let a stale event - retire the current head's still-valid run before its own live-head - check could ever reject it. Scoping the group by exact head SHA - isolates different heads from each other while events for the exact - same head (a `converted_to_draft`/`ready_for_review` transition, a - `synchronize` retry) still share one group and can still cancel each - other, which is what lets `converted_to_draft` retire an active - same-head verdict poll. + "newer"), and scoping the group by exact head SHA was the fix landed at + the time. Reverted 2026-09-03 by explicit user directive, refined after + peer review: head-SHA scoping meant every push to a PR got its own group, + so rapid successive pushes no longer cancelled each other's in-flight + runs -- they queued up independently instead, worsening the + self-inflicted queue-thrashing pattern this org measured directly + (236/300 cancelled runs attributed to concurrent push volume). Plain + repo+PR-number scoping combined with `cancel-in-progress: false` + structurally closes the #1568 race instead of just trading it for another + failure mode: nothing in this group is ever preempted regardless of + arrival order, so a late-arriving older-head run can never evict a + current one. The "Fail closed without a current-head OpenCode verdict" + step's own live-head/live-state revalidation (already run every poll + 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. """ workflow = WORKFLOW.read_text(encoding="utf-8") concurrency_block = workflow.split("\n\nconcurrency:\n", 1)[1].split( "\n\npermissions:", 1 )[0] - assert "github.event.pull_request.head.sha || github.run_id" in concurrency_block + 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 - assert "cancel-in-progress: true" in concurrency_block + assert "cancel-in-progress: false" in concurrency_block def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Path) -> None: diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index dcd5e2ef49..c3d3378a80 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -242,7 +242,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert "github.event.pull_request.base.repo.full_name" in concurrency_contract assert "github.repository" in concurrency_contract assert "github.event.pull_request.number" in workflow - if filename != "noema-review.yml": + if filename not in {"noema-review.yml", "opencode-review.yml"}: assert "cancel-in-progress: true" in workflow if filename in { "close-empty-pr.yml", @@ -254,16 +254,25 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: ) elif filename == "opencode-review.yml": assert "opencode-review-bootstrap-" in concurrency_contract - # Unlike the other required pull-request workflows below, this - # group is deliberately also scoped by exact head SHA: a - # delayed, out-of-order run for an older head must not be able - # to cancel the authoritative run already active for a newer - # head (Devin Review on `#1568`). Same-head events still share - # one group and can still cancel each other. + # Deliberately NOT scoped by head SHA and deliberately + # cancel-in-progress: false (reverted/refined 2026-09-03 by + # explicit user directive plus peer review): head-SHA scoping + # (originally added for Devin Review's `#1568` finding) meant + # every push to a PR got its own concurrency group, so rapid + # successive pushes no longer cancelled each other's in-flight + # runs -- they queued up independently instead, worsening the + # self-inflicted queue-thrashing pattern this org measured + # directly (236/300 cancelled runs from concurrent push volume). + # Plain repo+PR-number scoping with cancel-in-progress: false + # structurally closes the #1568 race instead of reopening it: + # nothing in the group is ever preempted, so a late-arriving + # older-head run can never evict a current one at any arrival + # order -- see the workflow's own comment for the full mechanism. assert ( "github.event.pull_request.head.sha || github.run_id" - in concurrency_contract + not in concurrency_contract ) + assert "cancel-in-progress: false" in concurrency_contract elif filename == "noema-review.yml": assert "github.event.workflow_run" not in concurrency_contract assert "noema-review-${{" in concurrency_contract @@ -279,7 +288,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert ( "github.event_name == 'pull_request_target'" in concurrency_contract ) - if filename not in {"noema-review.yml", "opencode-review.yml"}: + if filename != "noema-review.yml": assert "github.event.pull_request.head.sha" not in concurrency_contract assert "format('pr-{0}-{1}'" not in concurrency_contract