fix(visual): release the capture retry latch that froze three contributor PRs

migrations/0205_visual_capture_retry_pending_at.sql

@@ -0,0 +1,18 @@
+-- #9876: bound the visual-capture retry latch in TIME, not only by the code paths that release it.
+--
+-- `visual_capture_retry_pending_sha` defers the screenshot-table gate's one-shot CLOSE while a bounded
+-- recapture retry is still in flight for that head (#9030), so a browserless blip cannot be mistaken for "this
+-- PR has no visual evidence". It is a latch: once set, ONLY a later code path can release it. Twice now that
+-- release has been proven unreachable in production (#9462, and the durable-budget path this migration is part
+-- of fixing), each time leaving the latch set forever and silently freezing every affected PR -- the gate can
+-- neither close nor pass it, so nothing ever happens to it again.
+--
+-- A timestamp makes the bound structural: a latch older than the longest possible retry chain is stale by
+-- arithmetic, whatever code did or did not run. Same reasoning, and the same fail-safe direction, as
+-- preview-poll-budget.ts's BUDGET_MARKER_MAX_AGE_MS on the sibling R2 marker.
+--
+-- Nullable with no backfill: an existing latch written before this column has no recorded age, and
+-- visual-capture-retry-latch.ts deliberately treats a sha-without-timestamp as EXPIRED. That is the honest
+-- reading (the row predates the column, so its latch is at least as old as this deploy) and it is what
+-- unfreezes the PRs currently stuck behind one.
+ALTER TABLE pull_requests ADD COLUMN visual_capture_retry_pending_at TEXT;

src/db/repositories.ts

@@ -4687,7 +4687,7 @@ export async function markPullRequestVisualCaptureSatisfied(env: Env, fullName:
     // #9030: a proven-successful capture for this head supersedes any earlier "retry pending" marker recorded
     // for the SAME head (an error or a still-building preview on an earlier attempt) -- clearing it here keeps
     // the row's state minimal instead of leaving a now-moot marker sitting alongside a satisfied one.
-    .set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, updatedAt: nowIso() })
+    .set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, updatedAt: nowIso() })
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
@@ -4702,23 +4702,33 @@ export async function markPullRequestVisualCaptureRetryPending(env: Env, fullNam
   const db = getDb(env.DB);
   await db
     .update(pullRequests)
-    .set({ visualCaptureRetryPendingSha: headSha, updatedAt: nowIso() })
+    // #9876: stamp WHEN, so the latch's age can expire it even if every code path that should release it turns
+    // out to be unreachable -- which has now happened twice. Re-stamped on every mark rather than preserved
+    // from the first: each mark means a fresh retry was just scheduled, so a fresh deadline is the accurate
+    // one. (The sibling R2 budget marker deliberately does the opposite -- it preserves firstAttemptAt -- but
+    // it is counting attempts, where resetting would let the count live forever; here the chain is already
+    // bounded by that very count, so re-stamping cannot extend it indefinitely.)
+    .set({ visualCaptureRetryPendingSha: headSha, visualCaptureRetryPendingAt: nowIso(), updatedAt: nowIso() })
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
-/** #9462: clear the retry marker once the bounded recapture budget is EXHAUSTED. Without this the marker set by
- *  the previous attempt outlives the retries that justified it: `scheduleVisualCaptureRetry` early-returns at the
- *  budget, which only skips writing it AGAIN, so `visualCaptureRetryPendingSha === headSha` stayed true forever
- *  for that head. The only other clear is markPullRequestVisualCaptureSatisfied's, which requires a SUCCESSFUL
- *  capture -- exactly the thing that is not happening. A permanently-marked head silently disabled the
- *  screenshotTableGate's close (see the botCaptureRetryPending branch in processors.ts), so the deferral, meant
- *  to be temporary, became a permanent bypass. Not scoped to headSha on the write: the caller has already
- *  established which head it is finishing, and a row whose head moved on has a stale marker worth clearing too. */
+/** Release the retry latch: the retry chain that justified it has ended without a successful capture, so the
+ *  screenshotTableGate must stop deferring and evaluate the evidence actually present.
+ *
+ *  #9462 called this only from `scheduleVisualCaptureRetry`'s budget-exhausted early return. #9876 found that
+ *  path unreachable in the case that matters -- the durable per-head poll budget ends the chain by suppressing
+ *  `previewPending`, so the final attempt never calls that scheduler at all -- and moved the primary call to
+ *  the capture call site's conclusive branch, which every ending passes through. Three contributor PRs sat
+ *  frozen behind the gap, closeable by nobody and passable by nobody.
+ *
+ *  Not scoped to headSha on the write: the caller has already established which head it is finishing, and a row
+ *  whose head moved on has a stale marker worth clearing too. Idempotent -- clearing an absent latch is a
+ *  no-op, which is what makes it safe to call on every conclusive capture. */
 export async function clearPullRequestVisualCaptureRetryPending(env: Env, fullName: string, number: number, headSha: string): Promise<void> {
   const db = getDb(env.DB);
   await db
     .update(pullRequests)
-    .set({ visualCaptureRetryPendingSha: null, updatedAt: nowIso() })
+    .set({ visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, updatedAt: nowIso() })
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.visualCaptureRetryPendingSha, headSha)));
 }
 
@@ -7381,6 +7391,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
     linkedIssueHardRuleViolationReason: row.linkedIssueHardRuleViolationReason,
     visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha,
     visualCaptureRetryPendingSha: row.visualCaptureRetryPendingSha,
+    visualCaptureRetryPendingAt: row.visualCaptureRetryPendingAt,
     screenshotTablePresenceSatisfied: parseJson<{ headSha: string; evidenceFingerprint: string } | null>(row.screenshotTablePresenceSatisfiedJson, null),
   };
 }

src/db/schema.ts

@@ -488,6 +488,12 @@ export const pullRequests = sqliteTable(
     // provided". Cleared by a subsequent successful capture for the same head. loopover-computed
     // (publish-written), omitted from the GitHub-sync SET clause so a later sync cannot clobber it.
     visualCaptureRetryPendingSha: text("visual_capture_retry_pending_sha"),
+    // #9876: when the latch above was set, so its age can expire it. The latch is released only by code paths,
+    // and twice those paths have been unreachable in production (#9462, and the durable-poll-budget case that
+    // added this column), each time freezing every affected PR forever. See visual-capture-retry-latch.ts:
+    // a sha with no timestamp reads as EXPIRED, which is both honest (the row predates the column) and what
+    // releases the PRs already stuck behind one. loopover-computed, written with the sha, cleared with it.
+    visualCaptureRetryPendingAt: text("visual_capture_retry_pending_at"),
     // Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix, follow-up to #2006).
     // JSON `{headSha, evidenceFingerprint}` -- the head SHA and before/after-image-URL fingerprint that last
     // satisfied screenshotTableGate's presence-mode check (see evaluateScreenshotTableGate's staleness comment).

src/queue/processors.ts

@@ -486,7 +486,8 @@ export {
 export { processJob } from "./job-dispatch";
 import { isVisualPath } from "../review/visual/paths";
 import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureInteractionRoute, type CaptureRoute } from "../review/visual/capture";
-import { MAX_PREVIEW_POLL_ATTEMPTS } from "../review/visual/preview-poll-budget";
+import { MAX_PREVIEW_POLL_ATTEMPTS, PREVIEW_POLL_SECONDS } from "../review/visual/preview-poll-budget";
+import { visualCaptureRetryLatchState, VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS } from "../review/visual/visual-capture-retry-latch";
 import {
   clearFallbackDispatchMarker,
   fallbackShotFileName,
@@ -3499,10 +3500,36 @@ async function runAgentMaintenancePlanAndExecute(
   // visualCaptureSatisfiedSha unset, and the very next maintenance pass could close a legitimate visual PR
   // purely because an internal service blipped. visualCaptureRetryPendingSha (set only while a bounded
   // recapture retry is genuinely still scheduled for this exact head -- see the capture block in
-  // maybePublishPrPublicSurface) defers the CLOSE for exactly as long as that retry chance remains; once the
-  // budget is exhausted, the marker is never set again and the gate falls through to its normal, accurate
-  // evaluation on the final attempt -- this can never hold a PR forever.
-  const botCaptureRetryPending = Boolean(pr.headSha) && pr.visualCaptureRetryPendingSha === pr.headSha;
+  // maybePublishPrPublicSurface) defers the CLOSE for exactly as long as that retry chance remains.
+  //
+  // #9876: that deferral used to be bounded only by "some later code path will clear the latch", and this
+  // comment used to assert on that basis that it "can never hold a PR forever". It did -- twice, for the two
+  // separate unreachable-release bugs recorded in visual-capture-retry-latch.ts, most recently freezing three
+  // contributor PRs for an hour each with the gate unable to either close or pass them. The latch is now
+  // bounded by its own AGE as well, so the deferral ends on schedule whether or not the releasing path runs.
+  const latchState = visualCaptureRetryLatchState({
+    latchSha: pr.visualCaptureRetryPendingSha,
+    latchAtIso: pr.visualCaptureRetryPendingAt,
+    headSha: pr.headSha,
+    // The decision clock (#9492), not Date.now(): a replayed decision must re-derive the same latch verdict
+    // from the same recorded instant, or replay could certify a close this run never actually made.
+    nowMs: decisionClock.nowMs,
+  });
+  const botCaptureRetryPending = latchState.live;
+  if (latchState.live === false && latchState.reason === "expired") {
+    // An expiry means the retry that justified the deferral is never arriving. Audited rather than silent:
+    // this is the exact state that used to freeze a PR invisibly, so it must be visible when it recurs.
+    await recordAuditEvent(env, {
+      eventType: "github_app.visual_capture_retry_latch_expired",
+      actor: null,
+      targetKey: `${repoFullName}#${pr.number}`,
+      outcome: "completed",
+      detail:
+        `visual-capture retry latch for this head is ${Math.round(latchState.ageMs / 60000)}m old, past the ` +
+        `${Math.round(VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS / 60000)}m bound on any retry chain -- the retry is not coming, ` +
+        `so the screenshot-table gate is evaluating on the evidence actually present instead of deferring again`,
+    });
+  }
   const screenshotTableMatch =
     screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && !botCaptureRetryPending
       ? { matched: true, reason: screenshotTableGateResult.reason }
@@ -5136,14 +5163,6 @@ async function maybeForceFreshRebase(
 // re-check still catch the settled state.
 const CI_COALESCE_WINDOW_SECONDS = 60;
 
-// Visual preview self-poll (reviewbot PREVIEW_POLL_SECONDS parity): when a PR's preview deploy isn't live at
-// review time, re-review after this delay to re-capture the AFTER shot. The actual attempt CAP
-// (MAX_PREVIEW_POLL_ATTEMPTS) now lives in preview-poll-budget.ts and is enforced INSIDE buildCapture itself,
-// durably per head SHA across every trigger (#6323) -- this local scheduling check below is a harmless,
-// now-redundant secondary bound for the dedicated self-poll job chain specifically; it stays for defense in
-// depth but is no longer the thing that actually stops a never-resolving preview from polling forever.
-const PREVIEW_POLL_SECONDS = 90;
-
 /**
  * Coalesce CI-completion re-reviews: claims a per-PR window and returns true if this PR was already re-reviewed
  * within CI_COALESCE_WINDOW_SECONDS (caller skips). Self-host uses the transient Redis cache. A missing cache or
@@ -9810,10 +9829,15 @@ async function logTypeLabelSkip(env: Env, repoFullName: string, pullNumber: numb
  *  "the preview deploy is still building" (capture.previewPending) and "the capture pipeline itself errored"
  *  (browserless down, timeout, a GitHub hiccup) -- neither means "this PR genuinely has no visual evidence",
  *  so neither should let the screenshotTableGate treat it that way. Persists visualCaptureRetryPendingSha for
- *  the current head ONLY when a retry was actually scheduled (the budget is not yet exhausted) -- once
- *  MAX_PREVIEW_POLL_ATTEMPTS is reached, the marker is deliberately left unset so the gate falls through to its
+ *  the current head ONLY when a retry was actually SENT (budget remaining, and the enqueue itself succeeded --
+ *  #9876) -- once MAX_PREVIEW_POLL_ATTEMPTS is reached, the marker is cleared so the gate falls through to its
  *  normal (accurate) evaluation on this final attempt rather than holding the PR open forever. Best-effort:
- *  either write failing only means this ONE recovery chance is silently missed, never a crash. */
+ *  either write failing only means this ONE recovery chance is silently missed, never a crash.
+ *
+ *  #9876: this function is no longer the ONLY release. The durable per-head poll budget can end the retry chain
+ *  by suppressing `previewPending`, in which case the call site never invokes this function and the clear below
+ *  never runs -- so the call site clears on any conclusive capture, and the latch additionally expires by age.
+ *  Treat the clear here as the prompt path, not the guarantee. */
 async function scheduleVisualCaptureRetry(
   env: Env,
   args: {
@@ -9846,7 +9870,36 @@ async function scheduleVisualCaptureRetry(
     }
     return;
   }
-  if (args.pr.headSha) {
+  // #9876: enqueue FIRST, and mark only if it succeeded. The latch's whole meaning is "a retry is coming"; the
+  // send is best-effort by design, so writing the latch before it inverted that -- a failed enqueue left a
+  // latch behind with no retry in existence to release it, and the gate then deferred on a promise nothing
+  // was keeping. In this order the failure degrades to "no deferral this pass", which is the safe direction:
+  // the gate evaluates the evidence it has rather than freezing the PR.
+  const enqueued = await env.JOBS.send(
+    {
+      type: "recapture-preview",
+      deliveryId: args.webhook.deliveryId,
+      repoFullName: args.repoFullName,
+      prNumber: args.pr.number,
+      installationId: args.installationId,
+      attempt: args.previewPollAttempt + 1,
+    },
+    { delaySeconds: PREVIEW_POLL_SECONDS },
+  ).then(
+    () => true,
+    (error: unknown) => {
+      console.log(
+        JSON.stringify({
+          event: "recapture_enqueue_failed",
+          repoFullName: args.repoFullName,
+          pull: args.pr.number,
+          message: errorMessage(error).slice(0, 120),
+        }),
+      );
+      return false;
+    },
+  );
+  if (enqueued && args.pr.headSha) {
     await markPullRequestVisualCaptureRetryPending(env, args.repoFullName, args.pr.number, args.pr.headSha).catch((error) => {
       console.log(
         JSON.stringify({
@@ -9858,26 +9911,6 @@ async function scheduleVisualCaptureRetry(
       );
     });
   }
-  await env.JOBS.send(
-    {
-      type: "recapture-preview",
-      deliveryId: args.webhook.deliveryId,
-      repoFullName: args.repoFullName,
-      prNumber: args.pr.number,
-      installationId: args.installationId,
-      attempt: args.previewPollAttempt + 1,
-    },
-    { delaySeconds: PREVIEW_POLL_SECONDS },
-  ).catch((error) =>
-    console.log(
-      JSON.stringify({
-        event: "recapture_enqueue_failed",
-        repoFullName: args.repoFullName,
-        pull: args.pr.number,
-        message: errorMessage(error).slice(0, 120),
-      }),
-    ),
-  );
 }
 
 async function maybePublishPrPublicSurface(
@@ -12859,6 +12892,15 @@ async function maybePublishPrPublicSurface(
           // normally -- previewPending false, nothing thrown -- and neither the #9030 nor the #9207 guard
           // fired. The maintenance pass then read "no evidence, no retry pending" and CLOSED the PR one-shot.
           // A browserless outage now degrades to "we could not capture evidence, holding" instead.
+          //
+          // #9876: the `else` below is the half that was missing, and it is the half that froze three
+          // contributor PRs. Reaching it means this capture CONCLUDED -- no successful pair, but also nothing
+          // still pending and nothing broken -- which is a definite answer to the question the latch was
+          // deferring. Without it, a latch written by an earlier attempt outlived the chain that justified it:
+          // the durable per-head poll budget ends that chain by suppressing `previewPending`, so the final
+          // attempt takes neither branch above, `scheduleVisualCaptureRetry` is never called, and the
+          // budget-exhausted clear that #9462 put INSIDE it never runs. The release belongs to the capture
+          // outcome, where every conclusion passes, not to a scheduler that by definition stops being called.
           const previewPollAttempt = webhook.previewPollAttempt ?? 0;
           if (capture.previewPending || capture.renderFailed) {
             await scheduleVisualCaptureRetry(env, {
@@ -12868,6 +12910,20 @@ async function maybePublishPrPublicSurface(
               installationId,
               previewPollAttempt,
             });
+          } else if (pr.headSha) {
+            // Conclusive: release any latch this head still carries. Best-effort and idempotent -- the common
+            // case is that there is no latch to clear, and a failed clear only leaves the age bound in
+            // visual-capture-retry-latch.ts to end the deferral instead of ending it now.
+            await clearPullRequestVisualCaptureRetryPending(env, repoFullName, pr.number, pr.headSha).catch((error) => {
+              console.log(
+                JSON.stringify({
+                  event: "visual_capture_retry_pending_clear_failed",
+                  repoFullName,
+                  pull: pr.number,
+                  message: errorMessage(error).slice(0, 200),
+                }),
+              );
+            });
           }
         } catch (error) {
           console.log(

src/review/visual/preview-poll-budget.ts

@@ -29,6 +29,12 @@ const BUDGET_MARKER_MAX_AGE_MS = 24 * 60 * 60 * 1000;
 // combined -- the single source of truth processors.ts's own scheduling logic also imports, so the two
 // never drift out of sync.
 export const MAX_PREVIEW_POLL_ATTEMPTS = 5;
+// The delay between those attempts (reviewbot PREVIEW_POLL_SECONDS parity): when a PR's preview deploy isn't
+// live at review time, re-review after this long to re-capture the AFTER shot. Lives beside the attempt cap
+// rather than in processors.ts (#9876) because the two together ARE the retry budget: the latch deadline in
+// visual-capture-retry-latch.ts is derived from their product, so keeping them apart would let one move
+// without the bound that depends on it moving too.
+export const PREVIEW_POLL_SECONDS = 90;
 
 type BudgetMarker = { count: number; firstAttemptAt: number };
 // A read of the marker plus the R2 httpEtag it was stored under (null when no object exists yet). The etag is