fix(visual): release the capture retry latch that froze three contributor PRs - #9879
Merged
Conversation
…utor PRs (#9876) `visualCaptureRetryPendingSha` defers the screenshot-table gate's one-shot CLOSE while a bounded recapture retry is still in flight for a head (#9030), so a browserless blip is never read as "this PR has no visual evidence". It is a latch: once set, only a later code path releases it. That release was unreachable. The #9462 clear lives inside `scheduleVisualCaptureRetry`, which the call site invokes only when `previewPending || renderFailed`. The durable per-head poll budget (#6323) ends the retry chain by SUPPRESSING `previewPending`, so on the attempt that ends it neither branch fires, the scheduler is never called, and the clear it contains never runs. The latch then stands forever: the gate can neither close the PR nor pass it, and nothing ever happens to it again. Verified on the production ORB -- JSONbored/awesome-claude 5701, 5702 and 5704 all had `visual_capture_retry_pending_sha = head_sha`, with six deferral audit events each ~90s apart (PREVIEW_POLL_SECONDS x MAX_PREVIEW_POLL_ATTEMPTS) and then silence. Three holes, one per way the latch outlives its retry: - Release on the CAPTURE OUTCOME, at the call site's conclusive branch, which every ending passes through -- not in a scheduler that by definition stops being called. - Enqueue first, mark only if the send succeeded. The send is best-effort by design, so marking first meant a failed enqueue left a latch behind with no retry in existence to release it. - Record WHEN the latch was set and expire it past the longest possible retry chain, so the bound holds whatever code did or did not run. This latch has now carried two separate "can never hold a PR forever" comments, both false in production; an age bound makes the claim structural rather than a statement about control flow. Mirrors BUDGET_MARKER_MAX_AGE_MS on the sibling R2 marker. A latched sha with no timestamp reads as EXPIRED, which is both the honest reading of a row that predates the column and what unfreezes the PRs already stuck behind one. PREVIEW_POLL_SECONDS moves next to MAX_PREVIEW_POLL_ATTEMPTS: together they are the retry budget the new deadline is derived from, and splitting them would let one move without the bound that depends on it. Closes #9876
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9879 +/- ##
=======================================
Coverage 91.75% 91.75%
=======================================
Files 919 920 +1
Lines 112987 113002 +15
Branches 27193 27198 +5
=======================================
+ Hits 103667 103684 +17
+ Misses 8034 8033 -1
+ Partials 1286 1285 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 #9876
What was happening
Three contributor PRs in
JSONbored/awesome-claude(5701, 5702, 5704) were frozen: the screenshot-table gate could neither close nor pass them, so nothing happened to them at all. Verified on the production ORB —visual_capture_retry_pending_sha = head_shaon all three, six deferral audit events each ~90s apart, then silence.Root cause
visualCaptureRetryPendingShais a latch. It defers the gate’s one-shot CLOSE while a bounded recapture retry is still in flight for that head (#9030), so a browserless blip is not mistaken for missing evidence. Once set, only a later code path releases it.The release added in #9462 lives inside
scheduleVisualCaptureRetry, which the call site invokes only whencapture.previewPending || capture.renderFailed. But the durable per-head poll budget (#6323) ends the retry chain by suppressingpreviewPending— so on the attempt that ends the chain, neither branch fires, the scheduler is never called, and the clear it contains never runs.The clear was therefore unreachable in precisely the case it was written for.
The fix — three holes, one per way the latch outlives its retry
visual_capture_retry_pending_atrecords when it was set; past the longest possible retry chain it is stale by arithmetic, whatever ran. This latch has now carried two separate “can never hold a PR forever” comments, both false in production — an age bound makes that claim structural instead of a statement about control flow. MirrorsBUDGET_MARKER_MAX_AGE_MSon the sibling R2 marker.A latched sha with no timestamp reads as EXPIRED — the honest reading of a row predating the column, and what unfreezes the PRs already stuck behind one.
PREVIEW_POLL_SECONDSmoves next toMAX_PREVIEW_POLL_ATTEMPTS: together they are the budget the new deadline is derived from, and splitting them would let one move without the bound depending on it.Tests
test/unit/visual-capture-retry-latch.test.ts— 12 cases on the pure helper: both deadline boundaries (at / one ms before), superseded head, absent latch, missing and unparseable timestamps, future-dated clamp, and an assertion that the deadline stays derived from the retry budget so the two cannot drift.test/unit/queue-3.test.ts— 4 wiring regressions: the durable-budget freeze itself (marker set, then a conclusive capture while attempts remain — the case an exhaustion-triggered clear can never catch), a failed enqueue leaving no latch, an undatable latch expiring with an audit event instead of deferring again, and a failed conclusive clear not crashing the pass.Changed-line coverage measured at zero uncovered statements and zero uncovered branches across all six changed
src/files.Migration
0205(0204 was taken by #9865 on main). Column is nullable with no backfill and is absent from the GitHub-sync SET clause, like its sibling.