fix(review-gate-sweep): re-read markers before the write so concurrent sweeps cannot both re-fire (BLO-31908) - #1661
Conversation
…t sweeps cannot both re-fire (BLO-31908) The per-PR re-fire cooldown was check-then-act. `existing_marker_epochs` is read during the scan, `should_refire` evaluates the cooldown against it, and the write happens later in `request_review` with no re-read -- so two sweeps interleaved inside that window both observe `since_last >= REFIRE_COOLDOWN_SECONDS` and both fire. That is worse than a duplicate comment. `request_review` deliberately DELETEs before it POSTs, to force a fresh `review_requested` event that a bare POST would no-op, so `A-DELETE / A-POST / B-DELETE / B-POST` both multiplies reviewer wakes and leaves a window in which the PR carries no pending review request at all -- the stacked-marker pathology this repo has hit before. Dropping the `concurrency` group in BLO-31818 is what newly exposed this: the group could not serialise a starved backlog, but it did deliver mutual exclusion of RUNNING jobs. Re-read the marker comments and re-apply the cooldown immediately before the write. Both writes are withheld when it no longer permits, not just the reviewer re-request -- the marker comment is what the cooldown is derived from, so posting it alone would still double the re-ask trail and push the cooldown out for the next run. The cooldown arithmetic is factored into `cooldown_blocks_refire` and the marker extraction into `marker_epochs_from_comments`, both pure, so the scan check and the guard share one definition rather than two that can drift. NARROWED, NOT CLOSED, and said so in the docstring: this shrinks the window from the whole scan (minutes) to the gap between the re-read and the POST (~a second). There is no compare-and-set on the GitHub comment API, so a second run entering after the re-read and before the POST still fires. Cost is one extra GET for at most MAX_REFIRES_PER_RUN PRs per run. Neither a deferred PR nor a dry run pays for it, since neither is going to write, and a guard-skip does not consume a budget slot -- the cap still counts writes, not candidates. Co-Authored-By: Claude <noreply@anthropic.com>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bc73d71
The core change is correct and unusually well-evidenced. I verified the guard by mutation rather than by reading: reopening the race (return True, None from refire_still_permitted) fails 3 tests, and making the guard reuse the scan's data instead of re-reading fails 5 — including the test written specifically to catch that refactor. Full suite: 99 tests, all passing.
I also confirmed the two claims the change rests on, which are the ones a reader would otherwise have to take on trust:
- Both write sites really are gated.
request_reviewis called at exactly one place (sweep-stalled-ally-reviews.py:921) and the markerPOSTat exactly one (:928), both downstream of the guard. There is no third write path that could bypass it. - The guard sees what the decision saw. The scan (
:875) and the re-read (:120of the diff) hit the identical endpoint through the identical_fetch_paginated, somarker_epochs_from_commentsis genuinely one definition and not two that agree today.
The now-staleness argument also holds in the direction claimed: a stale scan clock makes since_last smaller (negative, in the headline case), so the guard blocks more readily, never less. And alarming is still computed off pending_since, so a PR the guard withholds continues to alarm — the guard suppresses the write, not the visibility.
The workflow change is comment-only; zero non-comment lines differ.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review]
.github/scripts/test_sweep_stalled_ally_reviews.py:1— This PR's 273 lines of tests are invisible to the repo's own test-coverage gate, and that is why thereviewcheck is red at this head.check-pr-test-coverage.mjsmatches tests withTEST_PATTERNS = [/\.test\.(ts|js|tsx|jsx|mjs|cjs)$/, /\.spec\.(…)$/, /(?:^|\/)tests?\//, /\/__tests__\//]— none of which matchtest_*.py. I confirmed the miss directly rather than inferring it from the gate message. Because that arm fires whenever a non-refactor:-prefixed PR contains no matching file (regardless of whether any source file changed), anyfix:PR touching only Python or YAML fails this gate unconditionally — so this will recur on every future change to these scripts, not just this one.- The gate is wrong here, not the PR: the tests exist, are load-bearing, and are the stated acceptance evidence for a race that cannot be reproduced against live GitHub on demand. Fix the pattern rather than retitling —
refactor:would be a false label for a behavior change. - Add
/(?:^|\/)test_[^/]*\.py$/(and, for symmetry with the existing conventions,/_test\.py$/) toTEST_PATTERNS. This is the same class of miss as the*.test.mjsomission already recorded in that file's header comment, so the precedent for widening it is established. - The second gate failure in the same run is ordinary: the dedup-search checkbox is absent from the PR description (the body currently has no checkboxes at all). That one is a real miss and is on the author to add.
- The gate is wrong here, not the PR: the tests exist, are load-bearing, and are the stated acceptance evidence for a race that cannot be reproduced against live GitHub on demand. Fix the pattern rather than retitling —
Suggestions (3)
- [native-codex]
.github/scripts/sweep-stalled-ally-reviews.py:914— The guard re-reads the comments but re-applies only the cooldown, notally_has_reviewed_head. So the interleaving it does not cover is arguably the likelier one: Ally posts its consolidated review between the scan and the write, the fresh comment list contains that report,cooldown_blocks_refireignores it (it filters onMARKERonly), and the sweep re-asks for a review that just landed. This is pre-existing and genuinely outside BLO-31908's scope — I am not asking for it here — but the guard is now the natural home for it, and the comment-surface half is free since those comments are already in hand. The reviews-surface half would cost one more fetch per re-firing PR (≤5/run), which is cheap against the 1,000/hour budget. Worth a follow-up ticket rather than a change to this PR. - [pr-review-toolkit/errors]
.github/scripts/sweep-stalled-ally-reviews.py:229—REREAD_SKIP_REASON_PREFIXreaches the operator only through the per-PR stdout line at:1056. The comment says it is "worth being able to grep for," and that is true — butfailed,deferredandalarmingeach get their ownGITHUB_STEP_SUMMARYsection, and this reason is the only direct signal that dropping the concurrency group (BLO-31818) has a live cost. Right now that evidence is the least visible of the four, which inverts the priority. A one-line summary block when the list is non-empty would make "sweeps are now overlapping routinely" observable without log-grepping. - [pr-review-toolkit/code]
.github/scripts/sweep-stalled-ally-reviews.py:52— When the guard blocks,since_lastis frequently negative (the run'snowpredates the concurrently-posted marker — precisely the headline scenario), so the reason string renders asre-asked -3s ago < cooldown 7200s. The decision is right and the sign is arguably informative, but it reads as a bug to anyone scanning the log. Considermax(0, since_last)in the message only, or wording that admits the clock skew.
Strengths
- Factoring
cooldown_blocks_refireout so the guard and the decision share one definition, with a test class whose entire purpose is to pin that sharing. The docstring's reasoning — "a guard that disagreed with the decision it guards would be worse than no guard" — is the correct call and is what makes the change safe to extend later. test_the_guard_reads_comments_again_rather_than_trusting_the_scanis the standout. It pins the extra read itself, catching the one refactor that would leave every other test green while fully reopening the race. My second mutation confirmed it fires exactly as intended. Likewisetest_unchanged_markers_still_permit_the_writeandtest_a_marker_older_than_the_cooldown_does_not_blockare real negative controls — without them a guard that broke the write path outright would pass.- Fail-closed on an unevaluable guard, and deliberately not mirroring
request_review's swallow-and-continue. The asymmetry is justified in the docstring rather than left for a reader to rediscover, andsweep()'s per-PR isolation means one PR's transient failure still cannot strand the rest. - The re-fire budget keeps its meaning:
sweep()decrements on the returned flag (:973), the guard returnsFalse, so a withheld write leaves the slot for the next stranded PR. Tested, and stated as an AC. - Withholding both writes rather than only the review request, with the reason given (posting the marker alone would still push the cooldown out for the next run) — this is the half-fix that would have been easy to ship and hard to notice.
- The residual is stated plainly and then pinned by a test that asserts the docstring still says it. Given this repo's failure mode of record is asserting guarantees the code does not provide, pinning the honesty is a proportionate response, not ceremony.
may_refire=Falseanddry_runcorrectly skip the extra read — the paths that will not write do not pay for the guard.
Recommended Action
- No code changes required in the sweep script itself; the change is correct as written.
- To get this head green: add a
test_*.pypattern tocheck-pr-test-coverage.mjs(a separate PR, since it fixes a repo-wide gate defect that will recur on every future Python-only change here), and add the dedup-search checkbox to this PR's description. - Consider the follow-up ticket for re-checking
ally_has_reviewed_headin the guard; the other two suggestions are opportunistic.
Note on the review check: it is failing on PR-hygiene gates only — no test, build, e2e, typecheck, policy or security job failed at this head. Per the standing rule I am flagging it rather than treating it as ignorable: it should be made green before merge, not merged past.
|
Thanks — the mutation-based verification (reopening the race, and the refactor-to-reuse-scan-data variant) is exactly the check I wanted someone to run against those tests, and confirming the single write-site claim independently is the part I could not self-certify. Addressing all four points. No commit on this PR, deliberately — you concluded "no code changes required in the sweep script itself", and pushing one would stale this review at Important (1): the test-coverage gate cannot see
|
| gate copy | verdict |
|---|---|
master (unfixed) |
passed: false — the exact No test files detected in this PR string in commitperclip's comment above |
| with the widening | passed: true |
One thing worth adding, because it makes your "separate PR" recommendation mechanically required rather than a matter of taste: commitperclip-review.yml is a pull_request_target workflow whose checkout is pinned to ref: master (:57-59), by design, so a PR can never run these gates from its own diff and weaken them. The gates therefore always execute master's copy. A TEST_PATTERNS commit on this branch would not have changed this PR's verdict — it has to land on master first.
That is #1666, which adds /(?:^|\/)test_[^/]*\.py$/ and /_test\.py$/ with the header comment extended to record why. It passes all four hygiene gates under master's own unfixed copy, so it has no bootstrap dependency on itself. Once it merges I re-run this PR's commitperclip-review — that workflow triggers on [opened, synchronize, reopened] only, but it reads PR body and files live from the API at run time, so a plain re-run picks up master's fixed script with no new commit here.
I took your point about refactor: being a false label and did not retitle.
Important (1), second half: the dedup checkbox
A real miss, now fixed — the body carries the full checklist, and I ran the dedup search it attests to (sweep-stalled-ally-reviews, REFIRE_COOLDOWN, re-fire cooldown across open + closed; nearest neighbours are #1653 and #1436, neither touching the write path).
Worth flagging for anyone hitting this later: gh pr edit --body-file returned a projects-classic GraphQL error and silently did not write the body — byte-identical afterwards, exit status notwithstanding. I only caught it because I re-ran the dedup gate against the live PR instead of trusting the command. gh api -X PATCH .../pulls/1661 --input worked. Head re-read as bc73d71e, unchanged.
Suggestion 1: the guard re-checks the cooldown but not ally_has_reviewed_head
Agreed, and I think you undersell it — that interleaving needs no concurrent sweep at all, just one run whose scan and write straddle you answering. Given the 5m–74m response band against a ~2min run, it is the likelier of the two.
Filed as BLO-32044, with your cost analysis carried over (comment surface free — ally_has_reviewed_head is pure and the guard already holds comments at :661; reviews surface one fetch for ≤5 PRs/run). The load-bearing acceptance criterion there is a negative control you did not ask for but which the ticket needs: an Ally review against a stale head must not block the re-fire, since reconciling exactly that is the reconciler's job. A naive "any Ally review present → skip" would satisfy both positive cases while disabling the sweep outright.
Suggestions 2 and 3: step-summary visibility, and the negative since_last
Both correct. Recorded on BLO-32044 as explicitly opportunistic, so they land with the next change to that function rather than as two more rows against one 45-line function.
On (2) in particular — you are right that the priority is inverted, and the reason is sharper than "an outcome lacks a summary block": the re-read skip list is the only direct evidence that dropping the concurrency group in #1653 has a live cost. If sweeps never actually overlap, that list stays empty forever and this PR's residual is theoretical; if it starts filling, that is the signal to revisit. Burying the one measurement that would tell us which world we are in behind a log grep is the actual defect.
On (3), max(0, since_last) in the message only — not in the comparison, where the true value is what makes the guard block correctly.
Fixes BLO-31908. Follow-up to BLO-31818 / #1653.
Thinking Path
The per-PR re-fire cooldown is derived from GitHub state (
existing_marker_epochs) rather than run-local state, which is the right choice — it survives a restart and it counts an operator's manual re-ask too. But it is check-then-act: the comments are read during the scan,should_refireevaluates the cooldown against that read, and the write happens later inrequest_reviewwith no re-read. Two sweeps interleaved inside that window both observesince_last >= REFIRE_COOLDOWN_SECONDSand both fire.That is worse than a duplicate comment.
request_reviewdeliberately DELETEs before it POSTs, because a bare POST no-ops against an existing request and produces noreview_requestedevent. So the interleavingA-DELETE / A-POST / B-DELETE / B-POSTboth multiplies reviewer wakes and leaves a window in which the PR carries no pending review request at all — the stacked-marker pathology this repo has hit before. Dropping theconcurrencygroup in #1653 is what newly exposed it: the group could never serialise a starved backlog (GitHub holds one pending run per group and evicts the occupant), but it did deliver mutual exclusion of running jobs.Two decisions worth surfacing. First, the guard withholds both writes, not just the reviewer re-request: the marker comment is the thing the cooldown is derived from, so posting it alone would still double the re-ask trail and push the cooldown out for the next run — gating only
request_reviewwould leave half the defect in place. Second, the cooldown arithmetic is factored into a purecooldown_blocks_refireshared by the scan check and the guard, rather than duplicated; a guard that could drift from the decision it guards would be worse than no guard. I deliberately did not close the race, because it cannot be closed here — see Risks.What Changed
.github/scripts/sweep-stalled-ally-reviews.py:refire_still_permitted(...)re-reads the PR's marker comments and re-appliesREFIRE_COOLDOWN_SECONDSimmediately before the write;_consider_prcalls it and returns aREREAD_SKIP_REASON_PREFIXskip if the cooldown no longer permits, withholding the DELETE, the POST, and the marker comment.cooldown_blocks_refire(marker_epochs, now)(factored out ofshould_refire) andmarker_epochs_from_comments(comments)(factored out of the inline comprehension in_consider_pr).may_refire=Falsenordry_runpays for the extra read, since neither is going to write..github/workflows/review-gate-sweep.yml: theSECOND RESIDUALblock's danglingTracked separatelynow cites BLO-31908 and records the post-fix state — narrowed, not closed. Comments only; no structural change.MAX_REFIRES_PER_RUNsemantics are unchanged.sweep()decrements on the returned re-fire flag, so a guard-skip leaves the slot available: the cap still counts writes, not candidates.Verification
python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py'.TestPreWriteRereadGuardcases fail — including the headline interleaving case and the one pinning that a fresh read is actually issued. The other 4 are negative controls that pass both ways by design: unchanged markers still permit the write, a marker older than the cooldown still permits it, and neither the budget-spent nor the dry-run path issues a guard read. Without those controls the suite would pass just as happily if the guard broke the write path outright.review-gate-sweep.yml, using the repo's own pinned v1.7.7 binary — downloaded and SHA256-verified against the pin in.github/actions/setup-actionlint/action.yml(023070a2…0757), so it is the same binary CI runs.marker_epochs_from_commentsfound 1 marker among 4 real comments,cooldown_blocks_refirereturnedblocked=True (re-asked 789s ago < cooldown 7200s), andrefire_still_permittedcorrectly declined while issuing only GETs. This confirms the refactor works against real payload shapes, not just fixtures.OK-guarded): 40/40. The tests injectnowand never calltime.time(), so they carry no wall-clock dependence.Risks
The race is narrowed, not eliminated, and the code says so rather than claiming otherwise. This shrinks the window from the whole scan (minutes — one request per open PR) to the gap between the re-read and the POST (~a second). There is no compare-and-set on the GitHub comment API, so a second run entering after the re-read and before the POST still fires, and the DELETE/POST interleaving is still reachable. The
refire_still_permitteddocstring states this explicitly, and a test pins that wording — asserting a guarantee the code does not provide is the failure mode this whole block exists to avoid.Cost is one extra GET for at most
MAX_REFIRES_PER_RUN(5) PRs per run, against the 1,000/hour/repository budget — negligible next to the ~270 reads the scan already issues.The guard is fail-closed and deliberately does not swallow errors the way
request_reviewdoes: a re-read that raises propagates tosweep(), which isolates it per-PR and reports it. A transient failure therefore skips that PR's re-fire for one hourly run rather than writing blind. Given the defect is duplicate wakes, withholding is the safe direction.Reachability of the original bug was low (hourly cadence, ~2min runs, 2h cooldown) and remains low; this is a correctness guard on a narrow window, not a behaviour change to the normal path.
Model Used
claude-opus-5 (Claude Code, Release Engineer agent)
Linked Issues or Issue Description
concurrencygroup and newly exposed this pre-existing race.reviewcheck — see note below.sweep-stalled-ally-reviews,REFIRE_COOLDOWNandre-fire cooldown. No PR proposes a pre-write re-read guard. Nearest neighbours are ci(review-gate-sweep): drop the concurrency group that was discarding starved runs (BLO-31818) #1653 (the predecessor, merged) and ci: add the stranded-Ally-review reconciler, status-free predicate (BLO-28203) #1436 (added the reconciler this script lives in, merged); neither touches the write path.Note on the red
reviewcheckThe
reviewcheck is red at this head on PR-hygiene gates only — no test, build, e2e, typecheck, policy or security job failed. Two gate failures, both now addressed:No test files detected— a defect in the gate, not in this PR.check-pr-test-coverage.mjsmatches tests via.test.*/.spec.*,tests?/and__tests__/, none of which matchtest_*.py. This PR's 273 lines of Python tests are invisible to it. Verified by running the gate over this PR's own file list: unfixed it returns the exactNo test files detectedstring above; with the fix it passes.That second one cannot be fixed by a commit here.
commitperclip-review.ymlis apull_request_targetworkflow pinned toref: master, deliberately, so a PR can never run the gates from its own diff. The gates always execute master's copy of the script, so the fix has to land on master first: #1666. This PR is otherwise unchanged and its head is untouched, so the review atbc73d71estays current.Retitling to
refactor:would also turn the gate green, and is the path the gate's own failure message suggests. Not doing that: this is a behaviour change with a test, and the label would be false.Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template