From d00bd355afc12d8e2d6549d0738fc1c358d6fcd8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:15:10 -0700 Subject: [PATCH 1/2] fix(visual): release the capture retry latch that froze three contributor 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 --- .../0205_visual_capture_retry_pending_at.sql | 18 ++ src/db/repositories.ts | 33 ++- src/db/schema.ts | 6 + src/queue/processors.ts | 130 ++++++--- src/review/visual/preview-poll-budget.ts | 6 + .../visual/visual-capture-retry-latch.ts | 79 +++++ src/types.ts | 5 + test/unit/queue-3.test.ts | 269 ++++++++++++++++++ test/unit/visual-capture-retry-latch.test.ts | 120 ++++++++ 9 files changed, 618 insertions(+), 48 deletions(-) create mode 100644 migrations/0205_visual_capture_retry_pending_at.sql create mode 100644 src/review/visual/visual-capture-retry-latch.ts create mode 100644 test/unit/visual-capture-retry-latch.test.ts diff --git a/migrations/0205_visual_capture_retry_pending_at.sql b/migrations/0205_visual_capture_retry_pending_at.sql new file mode 100644 index 0000000000..524132a23c --- /dev/null +++ b/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; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 075b521182..0e1d825414 100644 --- a/src/db/repositories.ts +++ b/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 { 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), }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 05ddf2de91..c66a9c2626 100644 --- a/src/db/schema.ts +++ b/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). diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 46cfd4453c..65b903abdb 100644 --- a/src/queue/processors.ts +++ b/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( diff --git a/src/review/visual/preview-poll-budget.ts b/src/review/visual/preview-poll-budget.ts index 9c8b834a37..7a792771f2 100644 --- a/src/review/visual/preview-poll-budget.ts +++ b/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 diff --git a/src/review/visual/visual-capture-retry-latch.ts b/src/review/visual/visual-capture-retry-latch.ts new file mode 100644 index 0000000000..4ddb86c52b --- /dev/null +++ b/src/review/visual/visual-capture-retry-latch.ts @@ -0,0 +1,79 @@ +// Is the visual-capture retry latch still live for this head? (#9876) +// +// THE LATCH. `visualCaptureRetryPendingSha` defers the screenshot-table gate's one-shot CLOSE while a bounded +// recapture retry is genuinely still in flight for that exact head (#9030/#9464), so a browserless outage or a +// preview deploy that has not finished building is never mistaken for "this contributor supplied no visual +// evidence". While it is live the gate can neither close the PR nor pass it. +// +// WHY IT NEEDS A CLOCK. A latch released only by code is only as reliable as the reachability of the code that +// releases it, and that reachability has now been wrong in production twice: +// +// #9462 -- `scheduleVisualCaptureRetry` early-returned at the budget, which only skipped re-writing the +// latch. The write from the previous attempt stood forever. +// #9876 -- the fix for that put the clear INSIDE `scheduleVisualCaptureRetry`, but the durable per-head poll +// budget (preview-poll-budget.ts) ends the chain by suppressing `previewPending`, so on the attempt +// that ends it the call site's `previewPending || renderFailed` condition is false and the +// scheduler -- clear and all -- is never called. Three contributor PRs sat frozen for an hour. +// +// Both times the code carried a comment asserting the latch "can never hold a PR forever". Both times that was +// false, because it was a claim about control flow rather than a property of the state. An age bound makes it a +// property of the state: past the deadline the latch is stale by arithmetic, whatever did or did not run. The +// sibling R2 marker in preview-poll-budget.ts already carries exactly this safeguard (BUDGET_MARKER_MAX_AGE_MS) +// for exactly this reason. +// +// This module does NOT replace the explicit releases -- those are still what makes the common case prompt. It +// is the backstop that makes "temporary" true by construction rather than by enumeration. + +import { MAX_PREVIEW_POLL_ATTEMPTS, PREVIEW_POLL_SECONDS } from "./preview-poll-budget"; + +/** How much longer than the theoretical maximum retry chain a latch may live before it is treated as stale. + * + * The chain is at most MAX_PREVIEW_POLL_ATTEMPTS hops PREVIEW_POLL_SECONDS apart, but wall-clock time is not + * the same as job time: a delayed job can sit behind a queue backlog, a re-review of a busy repo, or a restart. + * The multiplier buys that slack generously, because expiring EARLY re-arms the very false-positive close the + * latch exists to prevent, while expiring late only delays a decision that is already overdue. Asymmetric + * risk, so this errs long. */ +const LATCH_DEADLINE_SLACK = 8; + +/** Longest a retry latch can legitimately remain set for one head — derived from the retry budget itself, so a + * change to either budget dimension moves this deadline with it instead of silently invalidating it. At the + * current 5 attempts x 90s that is 60 minutes. */ +export const VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS = MAX_PREVIEW_POLL_ATTEMPTS * PREVIEW_POLL_SECONDS * 1000 * LATCH_DEADLINE_SLACK; + +/** Why the latch is not deferring the gate — surfaced so an expiry is auditable rather than silent. */ +export type VisualCaptureLatchState = + /** No latch recorded for this PR, or it was recorded against an older head that has since been superseded. */ + | { live: false; reason: "absent" } + /** A latch for THIS head, older than any retry chain could be. The retry that justified it is never coming, + * so the gate must evaluate on the evidence it actually has rather than defer again. */ + | { live: false; reason: "expired"; ageMs: number } + /** A retry for this exact head is still plausibly in flight. The gate defers. */ + | { live: true }; + +/** + * PURE: decide whether the latch still defers the screenshot-table gate. + * + * `latchAtIso` absent (or unparseable) while `latchSha` matches reads as EXPIRED, not as live. Two reasons: a + * row written before this column existed carries a latch at least as old as the deploy that added it, so + * "expired" is the honest reading; and any state this function cannot date is a state it cannot bound, which is + * precisely the failure mode the column exists to end. Defaulting the undatable case to "live" would preserve + * the freeze for exactly the rows already stuck in it. + */ +export function visualCaptureRetryLatchState(args: { + latchSha: string | null | undefined; + latchAtIso: string | null | undefined; + headSha: string | null | undefined; + nowMs: number; +}): VisualCaptureLatchState { + if (!args.headSha || !args.latchSha || args.latchSha !== args.headSha) return { live: false, reason: "absent" }; + + const latchedAtMs = args.latchAtIso ? Date.parse(args.latchAtIso) : Number.NaN; + // Number.isNaN covers both a missing timestamp and an unparseable one; an age computed from either is + // meaningless, and the doc comment above explains why that resolves to expired rather than live. + if (Number.isNaN(latchedAtMs)) return { live: false, reason: "expired", ageMs: VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS }; + + // Clamped at 0 so a latch timestamped slightly in the future (clock skew between an ORB and its database) + // reports an honest age of zero rather than a negative one, and stays live. + const ageMs = Math.max(0, args.nowMs - latchedAtMs); + return ageMs >= VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS ? { live: false, reason: "expired", ageMs } : { live: true }; +} diff --git a/src/types.ts b/src/types.ts index 2a980bbfd1..c5992e6bc8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -768,6 +768,11 @@ export type PullRequestRecord = { * CLOSE action defers instead of treating the transient blip as missing evidence. Publish-written; read * straight from the row. */ visualCaptureRetryPendingSha?: string | null | undefined; + /** #9876: when the retry latch above was set, as an ISO timestamp, so its AGE can release it. The explicit + * releases have twice proven unreachable in production, freezing every affected PR; see + * review/visual/visual-capture-retry-latch.ts for why a sha with no timestamp reads as expired rather than + * live. Publish-written alongside the sha; read straight from the row. */ + visualCaptureRetryPendingAt?: string | null | undefined; /** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): the (headSha, * evidenceFingerprint) checkpoint the screenshotTableGate's presence-mode check last satisfied for this PR * (see evaluateScreenshotTableGate's staleness comment). `null`/absent = presence mode has never satisfied diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 57d4097a58..fec5473ad5 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2437,6 +2437,275 @@ describe("queue processors", () => { } }); + // #9876 regression, and the one that reached production: the #9462 clear above is UNREACHABLE in the case + // that actually ends most retry chains. The durable per-head poll budget (preview-poll-budget.ts) terminates + // the chain by suppressing `previewPending` -- so on the attempt that ends it, buildCapture RESOLVES with + // previewPending false, renderFailed false and no real pair. The call site's `previewPending || renderFailed` + // condition is false, scheduleVisualCaptureRetry is never called, and the budget-exhausted clear living + // inside it never runs. The marker written by the earlier failing attempt then stands forever: the gate can + // neither close the PR nor pass it, so nothing ever happens to it again. Three contributor PRs in + // JSONbored/awesome-claude were frozen exactly this way, verified on the production ORB. + // + // Note this test never reaches MAX_PREVIEW_POLL_ATTEMPTS -- that is the whole point. The chain ends while + // attempts remain, which is precisely why an exhaustion-triggered clear cannot be the guarantee. + it("screenshot-table gate (#9876): a marker is CLEARED when the capture concludes, not only when the budget runs out", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockRejectedValue(new Error("browserless connection refused")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + }); + const sentJobs: Array> = []; + env.JOBS = { async send(message: Record) { sentJobs.push(message); } } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 70, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis70" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/70/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/70/reviews")) return Response.json([]); + if (url.includes("/pulls/70/commits")) return Response.json([]); + if (url.endsWith("/pulls/70") && method === "PATCH") return Response.json({ number: 70, state: "closed" }); + if (url.endsWith("/pulls/70")) return Response.json({ number: 70, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis70" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis70/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis70/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis70/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/70/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/70/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 903 }, { status: 201 }); + if (url.includes("/check-runs/903") && method === "PATCH") return Response.json({ id: 903 }); + return new Response("not found", { status: 404 }); + }); + + try { + // Attempt 0 errors -> the marker IS written for this head and a retry is scheduled. + await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-latch-conclusive-first", repoFullName: "JSONbored/gittensory", prNumber: 70, installationId: 123, attempt: 0 }); + const marked = await getPullRequest(env, "JSONbored/gittensory", 70); + expect(marked?.visualCaptureRetryPendingSha).toBe("vis70"); + // #9876: the mark is dated, so its age alone can release it later. + expect(marked?.visualCaptureRetryPendingAt).toEqual(expect.any(String)); + + // The durable budget now ends the chain: the capture RESOLVES with nothing pending and nothing broken, + // while attempts formally remain. Neither branch that could reschedule fires. + buildCaptureSpy.mockResolvedValue({ routes: [], interactions: [], previewPending: false, renderFailed: false }); + await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-latch-conclusive-final", repoFullName: "JSONbored/gittensory", prNumber: 70, installationId: 123, attempt: 1 }); + } finally { + buildCaptureSpy.mockRestore(); + } + + // A conclusive capture answers the question the latch was deferring, so the latch must be gone -- both + // fields, or a dated-but-shaless row would confuse the next read. + const stored = await getPullRequest(env, "JSONbored/gittensory", 70); + expect(stored?.visualCaptureRetryPendingSha).toBeNull(); + expect(stored?.visualCaptureRetryPendingAt).toBeNull(); + // And no further retry was scheduled by that final pass -- the chain really did end here. + expect(sentJobs.filter((job) => job.type === "recapture-preview")).toHaveLength(1); + }); + + // #9876: the conclusive clear is best-effort, mirroring the #9462 clear it backs up -- a cleanup write that + // fails must not crash the pass, because the pass is also what publishes the review. The latch simply stays + // until its age releases it, which is strictly better than losing the whole pass to a failed cleanup. + it("screenshot-table gate (#9876): a failed conclusive clear is swallowed -- the pass still completes", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ routes: [], interactions: [], previewPending: false, renderFailed: false }); + const clearSpy = vi + .spyOn(repositoriesModule, "clearPullRequestVisualCaptureRetryPending") + .mockRejectedValue(new Error("d1 write failed")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + }); + env.JOBS = { async send() {} } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 73, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis73" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/73/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/73/reviews")) return Response.json([]); + if (url.includes("/pulls/73/commits")) return Response.json([]); + if (url.endsWith("/pulls/73") && method === "PATCH") return Response.json({ number: 73, state: "closed" }); + if (url.endsWith("/pulls/73")) return Response.json({ number: 73, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis73" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis73/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis73/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis73/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/73/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/73/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 906 }, { status: 201 }); + if (url.includes("/check-runs/906") && method === "PATCH") return Response.json({ id: 906 }); + return new Response("not found", { status: 404 }); + }); + + try { + await expect( + processJob(env, { type: "recapture-preview", deliveryId: "screenshot-latch-clear-failed", repoFullName: "JSONbored/gittensory", prNumber: 73, installationId: 123, attempt: 1 }), + ).resolves.not.toThrow(); + expect(clearSpy).toHaveBeenCalled(); + } finally { + clearSpy.mockRestore(); + buildCaptureSpy.mockRestore(); + } + }); + + // #9876: the latch means "a retry is coming". The enqueue that makes that true is best-effort by design, so + // writing the latch BEFORE it inverted the meaning -- a failed send left a latch behind with no retry in + // existence that could ever release it, and the gate then deferred on a promise nothing was keeping. + it("screenshot-table gate (#9876): a failed retry enqueue leaves NO latch behind", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockRejectedValue(new Error("browserless connection refused")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + }); + env.JOBS = { async send() { throw new Error("queue unavailable"); } } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 71, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis71" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/71/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/71/reviews")) return Response.json([]); + if (url.includes("/pulls/71/commits")) return Response.json([]); + if (url.endsWith("/pulls/71") && method === "PATCH") return Response.json({ number: 71, state: "closed" }); + if (url.endsWith("/pulls/71")) return Response.json({ number: 71, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis71" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis71/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis71/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis71/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/71/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/71/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 904 }, { status: 201 }); + if (url.includes("/check-runs/904") && method === "PATCH") return Response.json({ id: 904 }); + return new Response("not found", { status: 404 }); + }); + + try { + // The pass still completes -- a queue outage is not a crash. + await expect( + processJob(env, { type: "recapture-preview", deliveryId: "screenshot-latch-enqueue-failed", repoFullName: "JSONbored/gittensory", prNumber: 71, installationId: 123, attempt: 0 }), + ).resolves.not.toThrow(); + } finally { + buildCaptureSpy.mockRestore(); + } + + // No retry exists, so no deferral may be claimed. The gate evaluates the evidence it has instead of + // freezing the PR -- the safe direction for a failure in the thing the latch is a promise about. + const stored = await getPullRequest(env, "JSONbored/gittensory", 71); + expect(stored?.visualCaptureRetryPendingSha).toBeNull(); + expect(stored?.visualCaptureRetryPendingAt).toBeNull(); + }); + + // #9876: the backstop. Even with both releases above wired, a latch is still released only by code that + // runs -- and this latch has now had two separate releases proven unreachable in production. A latch older + // than any possible retry chain is stale by arithmetic, so the gate stops deferring on it whatever did or + // did not run. This is what unfreezes rows written before the timestamp column existed, which is why a sha + // with a NULL date reads as expired rather than live. + it("screenshot-table gate (#9876): an undatable latch expires instead of deferring the gate forever", async () => { + // Screenshots deliberately OFF for this pass: a pass that DOES capture re-stamps the latch itself (and + // rightly so), which would mask the backstop. The stale latch must be released by its own age on a pass + // where nothing recaptures -- exactly the maintenance pass that finds a PR frozen by an old outage. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const seen = { closed: false }; + env.JOBS = { async send() {} } as unknown as Queue; + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 72, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis72" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + // Exactly the production state: a latch for the live head, with no recorded date -- the row predates the + // column. Written directly because no code path can produce it any more, which is the point. + await env.DB.prepare("update pull_requests set visual_capture_retry_pending_sha = ?, visual_capture_retry_pending_at = null where repo_full_name = ? and number = ?") + .bind("vis72", "JSONbored/gittensory", 72) + .run(); + + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/72/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/72/reviews")) return Response.json([]); + if (url.includes("/pulls/72/commits")) return Response.json([]); + if (url.endsWith("/pulls/72") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 72, state: "closed" }); } + if (url.endsWith("/pulls/72")) return Response.json({ number: 72, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis72" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis72/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis72/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis72/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/72/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/72/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 905 }, { status: 201 }); + if (url.includes("/check-runs/905") && method === "PATCH") return Response.json({ id: 905 }); + return new Response("not found", { status: 404 }); + }); + + // A maintenance pass on the frozen row: no capture runs, so only the latch decides. + await processJob(env, { type: "agent-regate-pr", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123, force: true }); + + // The gate reaches a decision instead of deferring for the 51st time. + expect(seen.closed).toBe(true); + // And the expiry is named in the audit trail -- this state used to freeze a PR invisibly, so its + // recurrence must be visible. + const expiredAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.visual_capture_retry_latch_expired", "JSONbored/gittensory#72") + .first<{ n: number }>(); + expect(expiredAudit?.n).toBe(1); + }); + describe("live migrations/** collision recheck (#2550)", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves // the collision hold actually suppresses what would otherwise merge; a negative test proves the check diff --git a/test/unit/visual-capture-retry-latch.test.ts b/test/unit/visual-capture-retry-latch.test.ts new file mode 100644 index 0000000000..b436441fea --- /dev/null +++ b/test/unit/visual-capture-retry-latch.test.ts @@ -0,0 +1,120 @@ +// #9876: the visual-capture retry latch must be releasable by TIME, not only by code paths. +// +// The regression these tests pin is not a wrong branch, it is an unreachable one: twice, the code that +// released this latch could not run in production, and both times a PR froze permanently because the gate +// could neither close nor pass it. So the cases below deliberately assert the ABSENCE of a live latch in every +// state that is not a genuinely-in-flight retry -- including the states a reasonable reading would call +// "unknown" (no timestamp, unparseable timestamp), because "unknown" resolving to "live" is precisely the +// freeze. +import { describe, expect, it } from "vitest"; +import { + visualCaptureRetryLatchState, + VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS, +} from "../../src/review/visual/visual-capture-retry-latch"; +import { MAX_PREVIEW_POLL_ATTEMPTS, PREVIEW_POLL_SECONDS } from "../../src/review/visual/preview-poll-budget"; + +const HEAD = "a".repeat(40); +const OTHER = "b".repeat(40); +const NOW = Date.parse("2026-07-29T18:00:00.000Z"); +const iso = (offsetMs: number): string => new Date(NOW + offsetMs).toISOString(); + +describe("visualCaptureRetryLatchState", () => { + it("is live while a retry for this exact head is plausibly still in flight", () => { + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: iso(-60_000), headSha: HEAD, nowMs: NOW })).toEqual({ live: true }); + }); + + it("is absent when no latch was ever recorded", () => { + expect(visualCaptureRetryLatchState({ latchSha: null, latchAtIso: null, headSha: HEAD, nowMs: NOW })).toEqual({ + live: false, + reason: "absent", + }); + }); + + it("is absent when the latch was recorded against a head that has since been superseded", () => { + // A new commit re-arms the screenshot requirement from scratch, so the old head's latch must not defer the + // gate for the new one. + expect(visualCaptureRetryLatchState({ latchSha: OTHER, latchAtIso: iso(-1000), headSha: HEAD, nowMs: NOW })).toEqual({ + live: false, + reason: "absent", + }); + }); + + it("is absent when the PR has no head SHA at all", () => { + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: iso(-1000), headSha: null, nowMs: NOW })).toEqual({ + live: false, + reason: "absent", + }); + }); + + it("expires once the latch is older than any retry chain could be", () => { + const state = visualCaptureRetryLatchState({ + latchSha: HEAD, + latchAtIso: iso(-VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS - 1), + headSha: HEAD, + nowMs: NOW, + }); + expect(state.live).toBe(false); + expect(state).toMatchObject({ reason: "expired" }); + }); + + it("expires exactly AT the deadline, not one tick after it", () => { + // Pins the boundary as >=, so a latch sitting precisely on the deadline releases rather than surviving + // another whole evaluation cycle. + expect( + visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: iso(-VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS), headSha: HEAD, nowMs: NOW }), + ).toEqual({ live: false, reason: "expired", ageMs: VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS }); + }); + + it("stays live one millisecond before the deadline", () => { + expect( + visualCaptureRetryLatchState({ + latchSha: HEAD, + latchAtIso: iso(-VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS + 1), + headSha: HEAD, + nowMs: NOW, + }), + ).toEqual({ live: true }); + }); + + it("reports the latch's real age when it expires", () => { + const ageMs = VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS + 90_000; + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: iso(-ageMs), headSha: HEAD, nowMs: NOW })).toEqual({ + live: false, + reason: "expired", + ageMs, + }); + }); + + it("treats a latch with NO timestamp as expired, not live", () => { + // The rows that were already frozen when this column shipped are exactly the rows with a sha and no + // timestamp. Defaulting them to "live" would have preserved the freeze for the only PRs that needed it + // lifted, so this is the case the whole migration exists for. + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: null, headSha: HEAD, nowMs: NOW })).toEqual({ + live: false, + reason: "expired", + ageMs: VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS, + }); + }); + + it("treats an unparseable timestamp as expired, not live", () => { + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: "not-a-date", headSha: HEAD, nowMs: NOW })).toEqual({ + live: false, + reason: "expired", + ageMs: VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS, + }); + }); + + it("keeps a future-dated latch live with a clamped age of zero", () => { + // Clock skew between an ORB and its database must not produce a negative age (which would still be < the + // deadline and so read live, but by accident rather than by intent). + expect(visualCaptureRetryLatchState({ latchSha: HEAD, latchAtIso: iso(5 * 60_000), headSha: HEAD, nowMs: NOW })).toEqual({ live: true }); + }); + + it("derives the deadline from the retry budget, so neither can drift from the other", () => { + // The bound is only defensible if it is longer than the chain it bounds. Asserting the relationship rather + // than the literal keeps that true when either budget dimension is retuned. + const longestPossibleChainMs = MAX_PREVIEW_POLL_ATTEMPTS * PREVIEW_POLL_SECONDS * 1000; + expect(VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS).toBeGreaterThan(longestPossibleChainMs); + expect(VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS % longestPossibleChainMs).toBe(0); + }); +}); From cede1a7d22b427c41c06960ae263c073e7da4909 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:37:46 -0700 Subject: [PATCH 2/2] test(visual): thread the required deliveryId through the latch-expiry regate job --- test/unit/queue-3.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index fec5473ad5..3feccbf1a8 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2694,7 +2694,7 @@ describe("queue processors", () => { }); // A maintenance pass on the frozen row: no capture runs, so only the latch decides. - await processJob(env, { type: "agent-regate-pr", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123, force: true }); + await processJob(env, { type: "agent-regate-pr", deliveryId: "screenshot-latch-expired-regate", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123, force: true }); // The gate reaches a decision instead of deferring for the 51st time. expect(seen.closed).toBe(true);