perf(queue): back off re-evaluating a PR whose verdict never changes - #10204
Merged
Conversation
…ver changes metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes -- about 1.2 per minute -- producing the identical `hold | missing_linked_issue` every time. It is CONFLICTING so it cannot merge, held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never clears. Four such PRs produced 66% of all decision records in a two-hour window, and that window exhausted the installation's GitHub REST quota. The webhook coalescer (#10127) cannot help: this is not a burst but a steady drip of legitimately distinct deliveries arriving long after any window. The missing control is a different kind -- not "collapse events that arrive together" but "stop asking a question whose answer has not changed". Sameness is the DECISION fields (action, reason_code, hold_cause) and deliberately NOT record_digest: #8886 has 56 distinct digests for its 56 identical verdicts, because the digest commits to per-evaluation data. Using it would make every repeat look novel, which is how this went unnoticed. Safety properties, each pinned by a test: the delay is capped so a stuck PR is still revisited; state is keyed on the head SHA so a new commit resets structurally rather than by a rule someone must remember; a changed verdict resets the count; and every uncertain case -- no state, malformed state, no cache, a throwing cache -- fails OPEN. A backoff that engaged on missing information would silently stop reviewing PRs, which is worse than the churn. Mutation testing removed a clamp on the exponent that looked like defence in depth: `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap, so no test could distinguish its presence. The cap is the only thing bounding this and it is tested directly; a second guard nobody can verify is a claim, not a safeguard. The module is registered in STAGED_AHEAD_OF_CONSUMERS. Wiring the skip is deliberately a separate change: it suppresses re-evaluation in processors.ts's hottest path, and getting it wrong stops reviewing PRs rather than merely wasting work. That belongs in a focused diff with its own review, not appended to this one. Refs #10184
Completes the backoff by connecting it, rather than leaving the module staged ahead of its consumers. Wiring it immediately found a design flaw its own 21 tests could not: "same verdict" is NOT "nothing happened". RECORD at the persistDecisionRecord call site -- the single ledger write every verdict passes through, so no path can bypass it, the same reasoning that function uses for its own reevaluation check. Keyed on the head SHA, so a new commit starts clean structurally rather than by a rule someone must remember. SKIP after the actuation-lock claim and before the refresh: the check is then one cache read on a pass that already owns the PR, and a backed-off pass spends nothing. It composes with #10174's reorder. It RETURNS rather than throws -- this is not contention, there is no work to retry, and the published state is already correct -- using `false`, the same "did not re-review" signal this function's other early bail uses. No caller branches on the result, so it cannot drive a retry loop. The lock is released explicitly first; wiring is what surfaced that leak. HOLDS ONLY, and this is the flaw wiring exposed. The force-fresh-rebase test (#9497/#2552) runs three deliberate identical passes to spend the 24h update-branch cap, and the first version swallowed the third: those passes take real actions -- update-branch, cap accounting -- while producing an unchanged verdict. Throttling them suppresses progress, not waste. A `hold` is the one action meaning "the gate declined to act", so repeating it genuinely produces nothing; every other action keeps today's behaviour exactly. That also covers the motivating case precisely -- #8886 is 56 identical HOLDS on one commit. Removed from STAGED_AHEAD_OF_CONSUMERS: it has consumers now. Closes #10184
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10204 +/- ##
==========================================
- Coverage 92.21% 91.34% -0.88%
==========================================
Files 934 935 +1
Lines 114178 114220 +42
Branches 27593 27609 +16
==========================================
- Hits 105294 104334 -960
- Misses 7582 8777 +1195
+ Partials 1302 1109 -193
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This was referenced Jul 31, 2026
Merged
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…r an operator force (#10229) #10204 placed the skip at the entry of the publish-and-maintain pass, past two things it must not run past. Readiness fires options.onReachedReadiness -- which charges regatePullRequest's bounded repair budget -- and then consumes the ONE-SHOT panel-retrigger marker (#7626). A guard sitting after both meant a backed-off pass had already spent a user's "Re-run LoopOver review" click with nothing left to re-trigger it, and had charged a repair attempt for work it never did. The guard now sits between the readiness gate and onReachedReadiness. It must not move EARLIER than readiness either: readiness legitimately defers a pass, and the screenshot-table recapture chain (#10061) depends on those deferrals to bound its retry budget -- a pre-readiness guard truncated it from 5 attempts to 3, caught by that test. options.force is now honoured. An operator's manual re-gate passes force: true and was being silently suppressed; a poll tick (previewPollAttempt) likewise. Backoff exists to stop the machine re-asking itself a settled question and must never suppress a pass a human asked for. The guard is extracted so both publish-and-maintain sites can share it, and its !headSha half is documented as a TSC-enforced early-out rather than a safety guard -- mutation testing confirms no runtime test can distinguish its absence, and an unverifiable guard is a claim, not a safeguard (verdict-stability.ts's own removed exponent clamp made the same point). #10204 shipped this wiring with NO test; test/unit/verdict-stability-wire.test.ts is the first, and pins each defect above as a regression. The webhook path is deliberately still unguarded. Adding it there truncates #10061's recapture budget, and the fix is not another exemption but moving the skip to the verdict-derivation choke point the record half already uses -- scoped in #10227. Closes #10222
24 tasks
20 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10184
metagraphed#8886 was evaluated 56 times on one unchanged commit in 47 minutes — ~1.2/min — producing the identical
hold | missing_linked_issueevery time. It isCONFLICTINGso it cannot merge, held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never clears. A state it cannot leave, with nothing throttling re-entry.Four such PRs produced 66% of all decision records in a two-hour window — the same window that exhausted the installation's GitHub REST quota and stalled 66 queue jobs behind
deferred_by: rate_limit.Why existing coalescing can't help
#10127 collapses simultaneous events. This is a steady drip of legitimately distinct deliveries (CI completions, label writes, sibling activity) arriving long after any window. The missing control is a different kind — not "collapse events that arrive together" but "stop asking a question whose answer has not changed."
Design
Sameness is the decision fields (
action,reason_code,hold_cause), deliberately notrecord_digest: #8886 has 56 distinct digests for 56 identical verdicts because the digest commits to per-evaluation data. Using it would make every repeat look novel — which is how this went unnoticed.persistDecisionRecordcall site — the single ledger write every verdict passes through, so no path bypasses it (the same reasoning that function uses for its own re-evaluation check).false, this function's existing "did not re-review" signal. No caller branches on it, so it cannot drive a retry loop. Releases the lock first.HOLDS ONLY — the flaw that wiring exposed
The first version backed off any repeated verdict. The force-fresh-rebase test (#9497/#2552) caught it: three deliberate identical passes spend the 24h update-branch cap, and my backoff swallowed the third.
"Same verdict" is not "nothing happened." A pass can take real actions — update-branch, cap accounting — while its verdict is unchanged; throttling that suppresses progress, not waste. A
holdis the one action meaning "the gate declined to act", so repeating it genuinely produces nothing. Every other action keeps today's behaviour exactly, and this still covers the motivating case precisely (#8886 is 56 identical holds).This is the whole argument for not shipping the module unwired — 21 passing unit tests could not have found it.
Safety
Capped delay (stuck PRs still revisited, just not 1.2×/min) · fails OPEN on missing/malformed/absent state or a throwing cache · engages only after 3 repeats so an ordinary webhook/sweep race is unaffected.
Verification
Full suite 26,444 passed; the 2 failures are load-sensitive
selfhost-aisubprocess tests unrelated to this change — 240 pass in isolation.typecheck,dead-exports,dead-source-filesclean.Mutation-tested, each caught: remove the cap (liveness — PR never revisited) · fail closed on missing state · don't reset on a changed verdict · drop the
calls-equivalent hold restriction.Mutation testing also deleted code: an exponent clamp that looked like defence-in-depth but that no test could distinguish, because
Math.min(Infinity, cap)is already the cap.