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..3feccbf1a8 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", 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); + // 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); + }); +});