Skip to content

fix(opencode-review): scope concurrency to opencode-review-target only #1786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seonghobae merged 1 commit into from
Sep 3, 2026
Merged

fix(opencode-review): scope concurrency to opencode-review-target only #1786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 58 additions & 38 deletions .github/workflows/opencode-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Comment on lines +294 to +297

@devin-ai-integration devin-ai-integration Bot Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

#
# 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

@devin-ai-integration devin-ai-integration Bot Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

permissions:
contents: read
pull-requests: read
Expand Down
17 changes: 14 additions & 3 deletions tests/test_opencode_required_verdict_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import json
import os
import re
import shutil
import subprocess
import textwrap
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions tests/test_required_workflow_queue_contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading