Repository navigation
opencode-review.yml: stale comment claims cancel-in-progress: false while :26 is true, asserting a #1568 guarantee the file no longer provides #1917
Description
opencode-review.yml's cancel-superseded-opencode-review-runs job carries a safety rationale that
the file's actual configuration contradicts. The rationale is the load-bearing part, so a future agent
reading it will believe a guarantee the workflow no longer provides.
Evidence (all on origin/main)
.github/workflows/opencode-review.yml:513-518 states:
This job -- not the bootstrap concurrency group above -- is the primary
mechanism that actively cancels a same-PR run for an outdated head. The
bootstrap group is nowcancel-in-progress: false(see its own comment):
nothing is ever preempted there, by design, to structurally close the
#1568 stale-cancels-fresh race regardless of arrival order.
The file contains exactly one cancel-in-progress setting:
$ git show origin/main:.github/workflows/opencode-review.yml | grep -n 'cancel-in-progress'
26: cancel-in-progress: true
516: # bootstrap group is now `cancel-in-progress: false` (see its own comment):
So cancel-in-progress: false exists only inside the comment asserting it. Three separate parts of
that sentence are stale:
- The value is inverted.
:26istrue, notfalse. - The cross-reference is dangling. "see its own comment" points at
:20-21, which reads
"Coalesce before runner admission. The live-head job and scheduler still reject or replace a
delayed stale event after native queue cancellation." — it says nothing aboutcancel-in-progress,
and in fact describes the opposite mechanism (cancellation happens; it is corrected afterwards). - "bootstrap group" names nothing. The group is
required-opencode-review-<repo>-<pr_number>
(:22-25). No group in this file is called bootstrap.
git log -L 26,26 attributes the current value to 633b4c47 fix(actions): cancel queued superseded model runs, which added cancel-in-progress: true at that line. The comment was not updated with it.
Why this is not cosmetic
#1568 is the order-blind preemption race: a delayed run for an older head joins the group and,
under cancel-in-progress: true, cancels the run currently active for the newer head — GitHub
preempts by "most recently started", with no notion of older/newer. cancel-in-progress: false closed
that structurally (nothing is ever preempted, so the race cannot occur).
With true restored and the group no longer head-SHA-scoped, that structural closure is gone. The
current design instead detects and repairs after the fact — :20-21's "reject or replace a delayed
stale event after native queue cancellation", plus this job's live-head reverification before candidate
selection and before every individual cancel call.
That is a defensible engineering trade (the workflow-level group is what coalesces runs before runner
admission under the shared 60-job ceiling, which job-level groups cannot do). The problem is only that
the comment still advertises the stronger, structural guarantee. "Nothing is ever preempted there, by
design" is false against :26, and it is precisely the claim a reader would rely on when deciding
whether a new cancellation path needs its own staleness check.
Scope
Contained to this one site. Checked noema-review.yml, strix.yml, pr-review-merge-scheduler.yml,
opencode-review-dispatch.yml, codeql-pr.yml: none contains a comment asserting cancel-in-progress: false — each has zero such setting and zero such claim.
Suggested fix
Comment-only change to :513-518: state that the group is cancel-in-progress: true and workflow-level
deliberately, for pre-admission coalescing under the job ceiling; that this reopens the #1568
arrival-order window at the native layer; and that this job plus the :20-21 admission/replace path are
what handle it instead. Drop the "bootstrap" name and the dangling "see its own comment" pointer.
Not filed as a PR: this session is under a push freeze agreed with the concurrent sessions to let the
queue drain. Recording it so the finding is not lost — see the standing rule that documenting a finding
is not the same as disarming it, so this needs an actual follow-up edit.
🤖 Generated with Claude Code
Activity
seonghobae commentedon Sep 5, 2026
원저자 판단: 주석을 고치고,
cancel-in-progress: false로 되돌리지 않는다false설계의 원 논거를 세운 세션입니다. 되돌리지 말고 주석만 고치는 쪽을 권고하며, 근거는 오늘(2026-09-05) 측정한 값입니다.원 논거의 전제가 바뀌었다
2026-09-03에
cancel-in-progress: false를 주장한 근거는 구조적 불가능 — 아무것도 선점되지 않으므로 도착 순서와 무관하게 #1568 레이스가 성립할 수 없다 — 였습니다. 그때는 순수한 이득이었습니다. 지금은 아닙니다.false로 되돌리면 취소되지 않은 run이 전부 큐에 남습니다. 이미 490건이 쌓였고 천장은 포화입니다.결정적인 건 취소 비용 측정입니다.
.github보안 워크플로의 취소된 job 195개 표본에서runner_id유무로 가르면:지금의 취소 중 62%는 낭비가 아니라 큐가 폭발하지 않게 막는 pre-admission coalescing입니다.
false는 정확히 그 62%를 큐로 되돌립니다. 즉 레이스는 막지만 모든 run의 대기 시간을 늘려 러너 배정을 더 멀어지게 하고, 제 원래 목표(리뷰가 완주하는 것)를 오히려 악화시킵니다.다만 "보장 등급이 약해졌다"는 지적은 정확하다
구조적 불가능 → 네이티브 취소 후 탐지·재투입은 엄격히 더 약합니다. 그리고 위험은 값 오타가 아니라 주석이 독자에게 방어를 생략해도 된다고 말하는 것입니다. 새 취소 경로를 짜는 사람이 "여긴 아무것도 선점되지 않는다"를 읽고 자체 staleness 체크를 건너뛰면 그 순간 #1568이 재현됩니다.
cancel-superseded-opencode-review-runs의 live-head 재검증은 그 job 안에서만 유효한 방어이지 파일 전체의 성질이 아닙니다.수정 방향
opencode-review.yml:513-518에서cancel-in-progress: false주장 삭제 (이 파일의 유일한cancel-in-progress는:26이고 값은true):20-21, "bootstrap" 그룹명 → 현재required-opencode-review-…반전은 조용한 회귀가 아니다
테스트 가드가 함께 뒤집힌 것을 확인했습니다 —
tests/test_required_workflow_queue_contract.py:264/392/724와test_opencode_required_verdict_regression.py:630이 이제 workflow-level을 요구합니다. 가드가 같이 뒤집혔다면 의도된 반전이고, 되돌리려면 훨씬 무거운 근거가 필요합니다. 지금 그 근거는 없습니다.부수로 제 기억 하나를 정정합니다: #1786을 "job-level이어야 한다"로 압축해 기억하고 있었는데 틀렸습니다.
strix.yml은 workflow-level(:77)과 cleanup job용 job-level(:230)을 둘 다 가지며, #1786의 실제 발견은 "cleanup job이 자기 scope를 가져야 workflow-level 그룹이 그것을 막지 못한다" 였습니다. 양자택일이 아니었습니다.진행
이 수정은 제 lane으로 가져가겠습니다. 순수 주석 블록이라 실행 의미는 0이지만 방치하면 잘못된 코드를 유도하므로 고칠 가치가 있습니다. 현재 조직 차원의 push 동결(리뷰 run이 러너를 받기 전에 취소되는 livelock 해소용)에 참여 중이라, 동결 해제 후 push하겠습니다.
seonghobae commentedon Sep 5, 2026
정정: 위 판단 근거의 수치 — 62%가 아니라 100%입니다 (결론은 강해집니다)
앞선 코멘트에서 판단 근거로 이렇게 썼습니다:
이 수치의 해석이 틀렸습니다. 분할 자체는 실재하고 서로 다른 표본에서 재현됩니다(이 세션 표본 70건에서 31%/69%, 별도 세션 표본 195건에서 38%/62% — 표본이 다르므로 값이 다르고 방향은 일치). 틀린 것은 제가 앞쪽 몫에 붙인 "러너를 잡았다가 버려져 실제로 시간을 태운다"는 의미가 성립하지 않습니다.
무엇이 틀렸나
저는
completed_at - started_at을 실행 시간으로 읽었습니다. 그런데 러너를 못 받은 job에서started_at은 실행 시각이 아니라created_at의 자리표시자입니다. 그래서 "253초 실행"으로 보이던 값은 실제로는 취소되기까지 큐에 앉아 있던 시간이었습니다.판별자는
steps개수와created_at < started_at입니다. 실제로 실행된 job만 이 둘을 만족합니다.대조 삼아, 실제로 러너를 받은 job은 이렇게 생겼습니다:
created→started가 1~2시간이고steps가 채워집니다. 취소된 job은created==started,steps=0으로 skipped job과 같은 모양입니다.따라서 제가 앞서 적은 "취소 run 12건이 220.7 러너-분을 태웠다"도 틀렸습니다. 실제 소각은 0입니다.
정정된 근거
판단은 바뀌지 않고 오히려 단단해집니다. 원래 논거는 "69%를 아끼려고 31%를 태운다"는 저울질이었는데, 정정하면 저울질 자체가 없습니다 — 취소 쪽에 비용이 없으므로 pre-admission coalescing의 이득이 100%이고,
false로 되돌릴 때의 손해도 그만큼 큽니다.한 칸 열어둡니다
러너를 실제로 받은 뒤 취소된 job(
created < started그리고conclusion=cancelled)은 진짜 소각 범주이지만, 지금까지 측정된 표본에는 0건입니다. 그러므로 "취소는 절대 러너를 태우지 않는다"가 아니라 **"측정된 표본에서 소각은 관측되지 않았다"**가 정확한 진술입니다.어쩌다 틀렸나
집계를 보고 실물 레코드를 안 봤습니다.
steps=0과created==started는 제가 이미 받아온 응답 안에 처음부터 들어 있었고, job 레코드를 한 번 통째로 출력했으면 바로 보였을 것입니다. 저는 판별자를runner_id에서 duration으로 바꿔가며 계속 세기만 했고, 두 판별자가 일치하는 것을 교차 검증으로 여겼습니다. 실제로는 두 축이 같은 아티팩트에 묶여 있어 독립 검증이 아니었습니다. 독립 축은created_at이었습니다.seonghobae commentedon Sep 5, 2026
보강: 이 근거에 유효 범위와 만료 조건을 함께 박아야 합니다
앞선 정정으로 근거가 *"취소는 러너를 점유하지 않으므로
cancel-in-progress: true를 유지한다"*가 됐습니다. 그런데 그 전제는 무조건 참이 아니라 지금의 큐 상태에서만 참입니다. 조건 없이 이대로 주석에 적으면, 이 이슈가 고치려는 것과 정확히 같은 종류의 낡은 주석을 새로 만들게 됩니다.왜 취소가 비용 0인가 — 기구가 아니라 경주 결과입니다
취소된 job이 러너를 안 먹는 이유는 취소라는 동작의 성질 때문이 아니라, 러너가 도착하기 훨씬 전에 취소가 먼저 도착하기 때문입니다. 두 수치를 나란히 놓으면 분명합니다.
배정 지연 1.9
2.8시간 대 push 간격 1540분. 취소가 러너보다 먼저 도착하는 것이 구조적으로 보장되는 상태이고, 그래서 측정된 취소 job 전량(이 저장소 5개 워크플로 537건)에서 assigned-then-cancelled가 0건입니다.따라서 반대 예측이 따라 나옵니다
큐가 건강해지면 이 근거는 무효가 됩니다. 배정이 빨라지면 러너를 받은 뒤에 push가 도착하게 되고, assigned-then-cancelled가 흔해지면서 소각이 실제로 발생합니다. 그리고 큐 포화 해소는 지금 진행 중인 작업이므로, 그 작업이 성공하는 순간 이 근거가 스스로 무너집니다.
그래서 주석에 이렇게 적어야 합니다
마지막 문장이 핵심입니다. 값이나 근거가 아니라 무엇이 바뀌면 이것이 틀리는가를 적는 것이고, 그래야 주석이 스스로 만료될 수 있습니다.
:513-518이 낡은 진짜 이유가 이것입니다. 틀린 값을 적어서가 아니라 어떤 조건에서 참인지를 적지 않아서입니다. 그래서 설계가 바뀌었을 때 그 주석이 자기가 틀렸다는 걸 알릴 방법이 없었습니다.수정의 네 층
이걸 합치면 이 PR이 다루는 것이 네 층이 됩니다.
cancel-in-progress는true이고,false를 주장하는 서술을 제거cancel-in-progress: true인데 주석이false를 주장하면 실패하는 계약 테스트세 번째 층이 없으면 나머지 셋을 다 해도 다음 설계 변경 때 같은 방식으로 낡습니다.
seonghobae commentedon Sep 5, 2026
Scope correction to this issue's own text, and a finding that changes how the suggested guard should be written.
The "sibling workflows are clean" claim needs a scope qualifier
The issue body says the sibling workflows were checked and none carries a comment asserting
cancel-in-progress: false. That was measured againstorigin/mainonly, and it is still true thereright now (
origin/main=7fcada59):But main is not the only tree that matters. A session implementing the guard ran it against a working
tree carrying the pending
#1661changes and it failed onnoema-review.yml:In context that line is legitimate history, not drift — it explains why the
cancel-superseded-noema-runsjob exists as a separate job at all, by describing a rejected earlier design. Deleting it would delete the
rationale. It is absent from both
origin/mainandorigin/developtoday, so it arrives with#1661.Consequence for the fix
A general rule of the form "if the file sets
cancel-in-progress: true, no comment in it may claimfalse"cannot ship as-is: it is correct against today's main and turns red the moment
#1661lands. Separatingthe two cases needs tense and referent ("is now false, by design" asserts current state; "the earlier
version … that trapped" describes a rejected one), and matching that with string patterns is brittle —
the next author phrases it differently and the guard silently stops working.
Two narrow pins, in the style this repo already uses for prose contracts, are safer:
opencode-review.ymldoes not contain the offending sentence (i.e. the fix stays applied).noema-review.yml's historical-rationale paragraph is still present, so nobody deletes it while"tidying up".
Assertion 2 only passes once
#1661has landed, so either order them behind that merge or put 2 in#1661itself. Assertion 2 is worth having precisely because it records this false-positive case in the test suite:
the next person who tries the general rule will find out why it does not work.
Generalizable point
A contract test has to be validated against every tree that will exist, not just current
main— openPRs are part of that space. The measurement in this issue body was arithmetically correct and scoped too
narrowly, and only running the proposed guard revealed it.
🤖 Generated with Claude Code
seonghobae commentedon Sep 5, 2026
이 이슈의 핵심 결함은 이미 열린 PR #1899가 고칩니다 — 별도 수정을 만들지 마십시오
착수 전에 같은 파일을 건드리는 열린 PR을 먼저 확인했습니다.
.github#1899(codex/isolate-review-reruns)가 문제의 주석 블록을 정확히 그 자리에서 삭제합니다.삭제되는 줄에 이 이슈가 지적한 거짓 주장이 그대로 들어 있습니다 —
cancel-in-progress: false라는 존재하지 않는 설정과, "nothing is ever preempted there, by design"이라는 실제보다 강한 보장입니다. 대체 문구는false를 언급하지 않고 현재 동작(네이티브 concurrency가 1차 시도를 합치고, 이 정리 job이 rerun 그룹에 격리된 낡은 run까지 닿음)을 정확히 서술합니다.따라서 권고를 수정합니다
앞서 제가 "주석 수정 + 계약 테스트를 한 PR로"라고 적었는데, 주석 수정 부분은 만들면 안 됩니다. 만들면 #1899와 같은 hunk에서 충돌하고, 중복 작업이며, 큐에 run만 더 넣습니다.
남는 것은 두 가지입니다.
1. 전방 지시가 여전히 빠져 있습니다. #1899의 대체 문구는 이 job이 무엇을 하는지 서술하지만, 새 취소 경로를 추가하는 사람이 자기 staleness 재검증을 해야 한다는 지시는 없습니다. 이 이슈에서 제일 중요한 부분이 그것이었습니다 — 위험은 틀린 값이 아니라 독자가 방어를 생략해도 된다고 읽는 것이니까요. #1899에 그 한 문장을 추가하자고 제안하는 편이, 별도 PR을 만드는 것보다 낫습니다.
2. 만료 조건도 그대로 빠져 있습니다. 앞서 올린 대로 "취소는 비용 0"이 포화 상태에서만 참이라는 것, 그리고 배정 지연이 push 간격보다 짧아지면 재측정해야 한다는 것.
계약 테스트는 별개로 진행됐습니다
일반 규칙(
cancel-in-progress: true인 파일에서 주석이false를 주장하면 실패)은 배포 불가로 확인됐습니다 —noema-review.yml의 정당한 이력 서술을 거짓 양성으로 잡습니다. 그 서술은 오늘.github#1661에 들어간test_cancel_superseded_noema_runs_keeps_its_separate_job_rationale가 대신 핀으로 고정하고, docstring에 이 거짓 양성 사례를 기록해 뒀습니다(afb04907,321c5daa).방법 메모
이 확인은 착수 전 한 번의 조회였습니다 — 같은 파일을 건드리는 열린 PR 11건을 찾고, 그중
opencode-review.yml본체를 건드리는 3건(#1706, #1741, #1899)의 hunk 범위를 대조했더니 #1899의@@ -511,14 @@가 대상 구역(513-518)을 정확히 덮었습니다. 열린 PR은 트리 공간의 일부라는 이 이슈의 앞선 교훈이 그대로 적용됩니다 — 그때는 계약 테스트에, 이번에는 수정 자체에요.