From 912c736d0c05cca371433971882103928a28855c Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:28:17 +0000 Subject: [PATCH] fix(review): bound the render-failure capture-retry chain with a durable per-head budget scheduleVisualCaptureRetry was bounding its retry chain on args.previewPollAttempt, the recapture-preview job chain's own payload field. Every other trigger that reaches it (CI-completion, deployment_status, the maintenance sweep) calls it without threading that field, so it always read back 0 and never bounded anything for them -- a sustained renderer outage produced an unbounded number of retries and kept re-stamping the visual-capture retry latch, defeating its age bound. Add a second durable, headSha-keyed budget in preview-poll-budget.ts under its own R2 namespace (mirroring the existing preview-poll budget's marker shape, key derivation, and CAS write loop) so a previewPending retry never double-charges it and vice versa. scheduleVisualCaptureRetry now consults that budget instead of the payload field and increments it on every retry it actually enqueues. --- src/queue/processors.ts | 20 +- src/review/visual/preview-poll-budget.ts | 71 ++++-- test/unit/preview-poll-budget.test.ts | 45 +++- test/unit/queue-3.test.ts | 304 ++++++++++++++++++++++- 4 files changed, 410 insertions(+), 30 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9b03e5afe3..84f057c089 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -489,7 +489,7 @@ 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, PREVIEW_POLL_SECONDS } from "../review/visual/preview-poll-budget"; +import { captureRetryAttemptCount, MAX_CAPTURE_RETRY_ATTEMPTS, PREVIEW_POLL_SECONDS, recordCaptureRetryAttempt } from "../review/visual/preview-poll-budget"; import { visualCaptureRetryLatchState, VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS } from "../review/visual/visual-capture-retry-latch"; import { clearFallbackDispatchMarker, @@ -9945,7 +9945,7 @@ async function logTypeLabelSkip(env: Env, repoFullName: string, pullNumber: numb * (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 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 + * #9876) -- once MAX_CAPTURE_RETRY_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. * @@ -9963,7 +9963,17 @@ async function scheduleVisualCaptureRetry( previewPollAttempt: number; }, ): Promise { - if (args.previewPollAttempt >= MAX_PREVIEW_POLL_ATTEMPTS) { + // #10061: the exhaustion decision reads a durable, headSha-keyed counter (captureRetryAttemptCount) that + // THIS function itself increments below on every retry it actually enqueues -- not args.previewPollAttempt, + // the recapture-preview job chain's own payload field. Every OTHER trigger that reaches this function + // (CI-completion, deployment_status, the maintenance sweep) calls it without threading that field, so it + // always read back 0 and never bounded anything for them (the bug this fixes). The counter lives under its + // OWN R2 namespace in preview-poll-budget.ts, separate from buildCapture's own previewPending budget, so + // neither double-charges the other. + /* v8 ignore next -- a recapture-preview job is only ever minted for a PR that had a head SHA, so the falsy + arm is defensive; the clear/mark guards below carry the identical guard for the same reason. */ + const captureRetryAttempts = args.pr.headSha ? await captureRetryAttemptCount(env, args.pr.headSha) : 0; + if (captureRetryAttempts >= MAX_CAPTURE_RETRY_ATTEMPTS) { // #9462: the budget is spent, so the marker the PREVIOUS attempt wrote must be cleared here. Returning // without clearing only skips re-writing it -- it left `visualCaptureRetryPendingSha === headSha` standing // forever (the sole other clear needs a successful capture, which by definition never came), which silently @@ -10015,6 +10025,10 @@ async function scheduleVisualCaptureRetry( }, ); if (enqueued && args.pr.headSha) { + // #10061: increment the durable capture-retry budget for exactly the retries we actually sent, mirroring + // the mark write below -- best-effort in the same direction (a failed increment just means this attempt + // doesn't count toward the cap, degrading toward "keep trying" rather than "stuck"). + await recordCaptureRetryAttempt(env, args.pr.headSha); await markPullRequestVisualCaptureRetryPending(env, args.repoFullName, args.pr.number, args.pr.headSha).catch((error) => { console.log( JSON.stringify({ diff --git a/src/review/visual/preview-poll-budget.ts b/src/review/visual/preview-poll-budget.ts index 7a792771f2..93386d4177 100644 --- a/src/review/visual/preview-poll-budget.ts +++ b/src/review/visual/preview-poll-budget.ts @@ -21,6 +21,12 @@ import { sha256Hex } from "../../utils/crypto"; const BUDGET_R2_NAMESPACE = "loopover/preview-poll-budget/"; +// #10061: a SECOND, independent budget for the capture-RETRY chain (scheduleVisualCaptureRetry's renderFailed / +// thrown-capture retries), living under its own R2 namespace so recording a capture-retry attempt never +// advances -- or is advanced by -- the previewPending budget above, which buildCapture already accounts for +// itself. Keeping the two namespaces (and therefore the two R2 keys, since the key is derived from +// `headSha + ':' + `) apart is what makes the two budgets independently reasonable. +const CAPTURE_RETRY_BUDGET_R2_NAMESPACE = "loopover/capture-retry-budget/"; // A stale marker must eventually stop mattering even if nothing ever explicitly resets it (an abandoned PR, // a repo whose preview pipeline was reconfigured) -- 24h comfortably outlives any real preview-build wait, // well past actions-fallback.ts's own 18-minute dispatch-marker expiry for the same reason. @@ -29,6 +35,11 @@ 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; +// #10061: scheduleVisualCaptureRetry's exhausted-budget behaviour must stay exactly what it was when the +// bound lived in the `attempt` payload field -- same cap, just read from a durable per-head counter instead. +// Sharing the constant (rather than inventing a second, separately-tunable number) keeps that guarantee true +// by construction. +export const MAX_CAPTURE_RETRY_ATTEMPTS = MAX_PREVIEW_POLL_ATTEMPTS; // 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 @@ -48,9 +59,9 @@ type BudgetRead = { marker: BudgetMarker | null; etag: string | null }; // module already accepts for a genuine write failure. const BUDGET_CAS_MAX_ATTEMPTS = 3; -async function budgetR2Key(headSha: string): Promise { - const fingerprint = await sha256Hex(`${headSha}:preview-poll-budget`); - return `${BUDGET_R2_NAMESPACE}${fingerprint.slice(0, 40)}.json`; +async function budgetR2Key(r2Namespace: string, keySuffix: string, headSha: string): Promise { + const fingerprint = await sha256Hex(`${headSha}:${keySuffix}`); + return `${r2Namespace}${fingerprint.slice(0, 40)}.json`; } /** Validate a raw stored payload into a BudgetMarker, or null when it's malformed or older than @@ -63,13 +74,13 @@ function parseBudgetMarker(text: string): BudgetMarker | null { return { count: marker.count, firstAttemptAt: marker.firstAttemptAt }; } -/** Shared read path for both public functions below. Returns a fail-open read (marker null, etag null) on any - * read error or a malformed/stale marker. Also surfaces the object's httpEtag so the increment path can do a +/** Shared read path for every budget below. Returns a fail-open read (marker null, etag null) on any read + * error or a malformed/stale marker. Also surfaces the object's httpEtag so the increment path can do a * compare-and-swap write against exactly the version it read (#7780). */ -async function readBudgetMarker(env: Env, headSha: string): Promise { +async function readBudgetMarker(env: Env, r2Namespace: string, keySuffix: string, headSha: string): Promise { if (!env.REVIEW_AUDIT) return { marker: null, etag: null }; try { - const object = await env.REVIEW_AUDIT.get(await budgetR2Key(headSha)); + const object = await env.REVIEW_AUDIT.get(await budgetR2Key(r2Namespace, keySuffix, headSha)); if (!object) return { marker: null, etag: null }; return { marker: parseBudgetMarker(await new Response(object.body).text()), etag: object.httpEtag }; } catch { @@ -77,25 +88,18 @@ async function readBudgetMarker(env: Env, headSha: string): Promise } } -/** How many preview-poll attempts have already been recorded for `headSha` -- 0 when no marker exists, - * storage is unavailable, or the existing marker has expired. Consulted by buildCapture BEFORE treating a - * "still building" preview state as worth another attempt. */ -export async function previewPollAttemptCount(env: Env, headSha: string): Promise { - return (await readBudgetMarker(env, headSha)).marker?.count ?? 0; -} - -/** Record one more preview-poll attempt for `headSha`, preserving the marker's original `firstAttemptAt` - * across increments so BUDGET_MARKER_MAX_AGE_MS expires from the FIRST attempt in this cycle, not resets on - * every poll (which would let a marker live forever as long as attempts keep arriving inside the window). - * Best effort -- a failed write just means this specific attempt doesn't count toward the budget, degrading +/** Shared increment path for every budget below, preserving the marker's original `firstAttemptAt` across + * increments so BUDGET_MARKER_MAX_AGE_MS expires from the FIRST attempt in this cycle, not resets on every + * attempt (which would let a marker live forever as long as attempts keep arriving inside the window). Best + * effort -- a failed write just means this specific attempt doesn't count toward the budget, degrading * toward "keep trying a bit longer" rather than toward "stuck forever", the safer failure direction for a * budget whose whole purpose is bounding retries, not enabling them. */ -export async function recordPreviewPollAttempt(env: Env, headSha: string): Promise { +async function recordBudgetAttempt(env: Env, r2Namespace: string, keySuffix: string, headSha: string): Promise { if (!env.REVIEW_AUDIT) return; try { - const key = await budgetR2Key(headSha); + const key = await budgetR2Key(r2Namespace, keySuffix, headSha); for (let attempt = 0; attempt < BUDGET_CAS_MAX_ATTEMPTS; attempt += 1) { - const existing = await readBudgetMarker(env, headSha); + const existing = await readBudgetMarker(env, r2Namespace, keySuffix, headSha); const marker: BudgetMarker = { count: (existing.marker?.count ?? 0) + 1, firstAttemptAt: existing.marker?.firstAttemptAt ?? Date.now() }; // Compare-and-swap against exactly the version we just read: only overwrite the existing object if its // etag is unchanged (etagMatches), or -- when we read no object -- only create one if none exists yet @@ -111,3 +115,28 @@ export async function recordPreviewPollAttempt(env: Env, headSha: string): Promi // best effort -- see doc comment above } } + +/** How many preview-poll attempts have already been recorded for `headSha` -- 0 when no marker exists, + * storage is unavailable, or the existing marker has expired. Consulted by buildCapture BEFORE treating a + * "still building" preview state as worth another attempt. */ +export async function previewPollAttemptCount(env: Env, headSha: string): Promise { + return (await readBudgetMarker(env, BUDGET_R2_NAMESPACE, "preview-poll-budget", headSha)).marker?.count ?? 0; +} + +/** Record one more preview-poll attempt for `headSha`. See recordBudgetAttempt for the shared contract. */ +export async function recordPreviewPollAttempt(env: Env, headSha: string): Promise { + return recordBudgetAttempt(env, BUDGET_R2_NAMESPACE, "preview-poll-budget", headSha); +} + +/** How many capture-RETRY attempts (scheduleVisualCaptureRetry's renderFailed / thrown-capture chain) have + * already been recorded for `headSha` -- 0 when no marker exists, storage is unavailable, or the existing + * marker has expired. Lives in its own R2 namespace (CAPTURE_RETRY_BUDGET_R2_NAMESPACE), so it is never + * double-charged by, or double-charges, previewPollAttemptCount's own budget above (#10061). */ +export async function captureRetryAttemptCount(env: Env, headSha: string): Promise { + return (await readBudgetMarker(env, CAPTURE_RETRY_BUDGET_R2_NAMESPACE, "capture-retry-budget", headSha)).marker?.count ?? 0; +} + +/** Record one more capture-retry attempt for `headSha`. See recordBudgetAttempt for the shared contract. */ +export async function recordCaptureRetryAttempt(env: Env, headSha: string): Promise { + return recordBudgetAttempt(env, CAPTURE_RETRY_BUDGET_R2_NAMESPACE, "capture-retry-budget", headSha); +} diff --git a/test/unit/preview-poll-budget.test.ts b/test/unit/preview-poll-budget.test.ts index 36082b9894..0a33476e7d 100644 --- a/test/unit/preview-poll-budget.test.ts +++ b/test/unit/preview-poll-budget.test.ts @@ -1,5 +1,11 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "../../src/review/visual/preview-poll-budget"; +import { + captureRetryAttemptCount, + MAX_PREVIEW_POLL_ATTEMPTS, + previewPollAttemptCount, + recordCaptureRetryAttempt, + recordPreviewPollAttempt, +} from "../../src/review/visual/preview-poll-budget"; import { createTestEnv } from "../helpers/d1"; const HEAD_SHA = "budget-head-sha-1234567890"; @@ -184,3 +190,40 @@ describe("previewPollAttemptCount / recordPreviewPollAttempt (#6323 -- durable p expect(putAttempts).toBe(3); // BUDGET_CAS_MAX_ATTEMPTS }); }); + +describe("captureRetryAttemptCount / recordCaptureRetryAttempt (#10061 -- durable per-headSha capture-retry budget)", () => { + it("0 when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("0 when the stored object is malformed", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ forcedValue: "{not valid json" }) }); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("1 immediately after a single recordCaptureRetryAttempt, and accumulates across repeats", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + await recordCaptureRetryAttempt(env, HEAD_SHA); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(1); + await recordCaptureRetryAttempt(env, HEAD_SHA); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(2); + }); + + // #10061: the whole point of the new namespace is that a previewPending retry (already counted inside + // buildCapture, via recordPreviewPollAttempt) must never double-charge the capture-retry budget, and vice + // versa -- otherwise the two chains would silently steal attempts from each other instead of staying + // independently reasonable. + it("is independent of previewPollAttemptCount for the SAME head SHA -- recording one never advances the other", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + await recordPreviewPollAttempt(env, HEAD_SHA); + await recordPreviewPollAttempt(env, HEAD_SHA); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(2); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + + await recordCaptureRetryAttempt(env, HEAD_SHA); + await expect(captureRetryAttemptCount(env, HEAD_SHA)).resolves.toBe(1); + // The preview-poll count must be untouched by the capture-retry write above. + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(2); + }); +}); diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 6f3f66b4df..2504c0f2d0 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -5,7 +5,7 @@ import { clearReviewSuppressionCacheForTest } from "../../src/review/review-memo import { } from "../../src/github/comments"; import * as repositoriesModule from "../../src/db/repositories"; import * as visualCaptureModule from "../../src/review/visual/capture"; -import { MAX_PREVIEW_POLL_ATTEMPTS } from "../../src/review/visual/preview-poll-budget"; +import { MAX_CAPTURE_RETRY_ATTEMPTS, MAX_PREVIEW_POLL_ATTEMPTS, recordCaptureRetryAttempt } from "../../src/review/visual/preview-poll-budget"; import * as repositorySettingsModule from "../../src/settings/repository-settings"; import { } from "../../src/selfhost/queue-common"; import { @@ -53,6 +53,30 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => { +// #10061: a minimal etag-aware in-memory R2 stand-in for the capture-retry budget (mirrors +// preview-poll-budget.test.ts's own memoryBudgetStore), used by the screenshot-table gate tests below that +// now need a REAL REVIEW_AUDIT binding to seed/observe scheduleVisualCaptureRetry's durable per-head counter. +function memoryBudgetStore(): R2Bucket { + const store = new Map(); + let etagSeq = 0; + return { + async get(key: string) { + const entry = store.get(key); + return entry === undefined ? null : ({ body: new Response(entry.value).body, httpEtag: entry.etag } as unknown as R2ObjectBody); + }, + async put(key: string, value: unknown, putOptions?: R2PutOptions) { + const onlyIf = putOptions?.onlyIf as R2Conditional | undefined; + const current = store.get(key); + if (onlyIf?.etagMatches !== undefined && current?.etag !== onlyIf.etagMatches) return null; + if (onlyIf?.etagDoesNotMatch === "*" && current !== undefined) return null; + etagSeq += 1; + const etag = `etag-${etagSeq}`; + store.set(key, { value: await new Response(value as BodyInit).text(), etag }); + return { key, etag } as unknown as R2Object; + }, + } as unknown as R2Bucket; +} + function queueMinerSnapshot(login: string) { return { source: "gittensor_api" as const, @@ -2169,6 +2193,83 @@ describe("queue processors", () => { expect(sentJobs).toContainEqual(expect.objectContaining({ type: "recapture-preview", repoFullName: "JSONbored/gittensory", prNumber: 64, attempt: 1 })); }); + // #10061 invariant: the previewPending path's own budget accounting (inside buildCapture, via + // previewPollAttemptCount/recordPreviewPollAttempt) is explicitly OUT of scope for this fix and must keep + // behaving exactly as it did before -- a single still-building preview still schedules exactly one + // recapture-preview retry and still marks the latch, unaffected by the NEW capture-retry budget this fix + // adds for the renderFailed / thrown-capture chain. + it("screenshot-table gate (#10061 invariant): a single previewPending capture still schedules exactly one recapture-preview and marks the latch", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValueOnce({ routes: [{ path: "/app", afterUrl: "https://worker.example/loopover/shot?url=x", afterUrlMobile: "https://worker.example/loopover/shot?url=x" }], interactions: [], previewPending: true, renderFailed: false, previewUnobtainable: false }); + 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"); + const seen = { closed: false }; + 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/82/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/82/reviews")) return Response.json([]); + if (url.includes("/pulls/82/commits")) return Response.json([]); + if (url.endsWith("/pulls/82") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 82, state: "closed" }); } + if (url.endsWith("/pulls/82")) return Response.json({ number: 82, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis82" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis82/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis82/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis82/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/82/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/82/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 952 }, { status: 201 }); + if (url.includes("/check-runs/952") && method === "PATCH") return Response.json({ id: 952 }); + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-preview-pending-82", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 82, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis82" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + } finally { + buildCaptureSpy.mockRestore(); + } + + expect(seen.closed).toBe(false); + const stored = await getPullRequest(env, "JSONbored/gittensory", 82); + expect(stored?.visualCaptureRetryPendingSha).toBe("vis82"); + expect(sentJobs).toContainEqual(expect.objectContaining({ type: "recapture-preview", repoFullName: "JSONbored/gittensory", prNumber: 82, attempt: 1 })); + expect(sentJobs.filter((job) => job.type === "recapture-preview")).toHaveLength(1); + }); + // #9464 INVARIANT / #4110 guard: the fix must not neuter the gate. The single distinguishing bit is // `renderFailed` -- an identical no-evidence capture from a HEALTHY renderer is a true negative and must // still close. This is the test that an earlier "infer the blip from zero pairs" attempt broke. @@ -2253,7 +2354,13 @@ describe("queue processors", () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), }); + // #10061: exhaustion is now decided by the durable, headSha-keyed capture-retry counter, not the + // recapture-preview job's own `attempt` payload field -- pre-spend the budget for this head directly. + for (let i = 0; i < MAX_CAPTURE_RETRY_ATTEMPTS; i += 1) { + await recordCaptureRetryAttempt(env, "vis61"); + } const sentJobs: Array> = []; env.JOBS = { async send(message: Record) { sentJobs.push(message); } } as unknown as Queue; await upsertInstallation(env, { @@ -2297,9 +2404,10 @@ describe("queue processors", () => { }); try { - // attempt: MAX_PREVIEW_POLL_ATTEMPTS -> previewPollAttempt >= MAX_PREVIEW_POLL_ATTEMPTS -> the budget - // check bails BEFORE scheduling yet another retry or persisting the marker (see scheduleVisualCaptureRetry). - await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-table-capture-error-exhausted", repoFullName: "JSONbored/gittensory", prNumber: 61, installationId: 123, attempt: MAX_PREVIEW_POLL_ATTEMPTS }); + // The durable captureRetryAttemptCount(env, "vis61") already reads MAX_CAPTURE_RETRY_ATTEMPTS -> the + // budget check bails BEFORE scheduling yet another retry or persisting the marker (see + // scheduleVisualCaptureRetry) regardless of what `attempt` this delivery itself carries. + await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-table-capture-error-exhausted", repoFullName: "JSONbored/gittensory", prNumber: 61, installationId: 123, attempt: 0 }); } finally { buildCaptureSpy.mockRestore(); } @@ -2322,6 +2430,7 @@ describe("queue processors", () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), }); const sentJobs: Array> = []; env.JOBS = { async send(message: Record) { sentJobs.push(message); } } as unknown as Queue; @@ -2365,10 +2474,18 @@ describe("queue processors", () => { }); try { - // Attempt 0 errors -> the marker IS written for this head and a retry is scheduled. + // Attempt 0 errors -> the marker IS written for this head and a retry is scheduled (the durable + // capture-retry counter for "vis62" goes from 0 -> 1). await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-marker-clear-first", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123, attempt: 0 }); expect((await getPullRequest(env, "JSONbored/gittensory", 62))?.visualCaptureRetryPendingSha).toBe("vis62"); + // #10061: spend the REST of the durable budget directly (simulating the remaining independent retries + // that would otherwise have to run through the full webhook pipeline), so the NEXT delivery is the one + // that finds the counter already at MAX_CAPTURE_RETRY_ATTEMPTS and exhausts it. + for (let i = 1; i < MAX_CAPTURE_RETRY_ATTEMPTS; i += 1) { + await recordCaptureRetryAttempt(env, "vis62"); + } + // The final attempt exhausts the budget while the pipeline is still failing. await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-marker-clear-exhausted", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123, attempt: MAX_PREVIEW_POLL_ATTEMPTS }); } finally { @@ -2387,7 +2504,12 @@ describe("queue processors", () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), }); + // #10061: pre-spend the durable capture-retry budget for this head so the delivery below finds it exhausted. + for (let i = 0; i < MAX_CAPTURE_RETRY_ATTEMPTS; i += 1) { + await recordCaptureRetryAttempt(env, "vis63"); + } 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"] }, @@ -2437,6 +2559,178 @@ describe("queue processors", () => { } }); + // #10061: THE regression this fix closes. Independent triggers (a CI-completion webhook, a + // deployment_status webhook, the maintenance sweep) all reach scheduleVisualCaptureRetry WITHOUT threading + // previewPollAttempt, so before this fix each one independently re-armed a fresh 5-attempt budget and a + // sustained renderer outage produced an UNBOUNDED number of retries. Drive MAX_PREVIEW_POLL_ATTEMPTS + 1 + // independent `pull_request` webhook deliveries (each one a fresh delivery carrying no previewPollAttempt, + // exactly like a real CI-completion/deployment_status/sweep trigger) against a renderer that never + // recovers: the durable per-head capture-retry budget must still cap the TOTAL number of recapture-preview + // jobs enqueued across ALL of them at MAX_CAPTURE_RETRY_ATTEMPTS. + it("screenshot-table gate (#10061): independent triggers with NO previewPollAttempt still bound the renderFailed retry chain to MAX_CAPTURE_RETRY_ATTEMPTS jobs total", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [{ path: "/app", afterUrl: "https://worker.example/loopover/shot?url=x", afterUrlMobile: "https://worker.example/loopover/shot?url=x" }], + interactions: [], + previewPending: false, + renderFailed: true, + previewUnobtainable: false, + }); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), + }); + 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"); + 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/80/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/80/reviews")) return Response.json([]); + if (url.includes("/pulls/80/commits")) return Response.json([]); + if (url.endsWith("/pulls/80") && method === "PATCH") return Response.json({ number: 80, state: "closed" }); + if (url.endsWith("/pulls/80")) return Response.json({ number: 80, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis80" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis80/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis80/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis80/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/80/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/80/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 950 }, { status: 201 }); + if (url.includes("/check-runs/950") && method === "PATCH") return Response.json({ id: 950 }); + return new Response("not found", { status: 404 }); + }); + + try { + // MAX_PREVIEW_POLL_ATTEMPTS + 1 independent triggers -- each a plain `pull_request` delivery, so each + // reaches scheduleVisualCaptureRetry with previewPollAttempt undefined (-> 0), exactly like the bug. + for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS + 1; i += 1) { + await processJob(env, { + type: "github-webhook", + deliveryId: `screenshot-multi-trigger-render-failed-80-${i}`, + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 80, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis80" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + } + } finally { + buildCaptureSpy.mockRestore(); + } + + // At most MAX_CAPTURE_RETRY_ATTEMPTS recapture-preview jobs total, no matter how many independent + // triggers fired -- the durable per-head budget, not any one trigger's own payload counter, bounded it. + const recaptureJobs = sentJobs.filter((job) => job.type === "recapture-preview"); + expect(recaptureJobs).toHaveLength(MAX_CAPTURE_RETRY_ATTEMPTS); + }); + + // #10061 regression (named for this bug): the failure mode was not merely "too many retries" -- it was that + // `visual_capture_retry_pending_at` kept getting RE-STAMPED by every independent trigger's mark write, + // continually resetting VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS's deadline so the latch's own age bound could + // never elapse either. Once the durable budget is spent, the LAST trigger must CLEAR both latch columns + // instead of re-stamping them, so the latch genuinely becomes releasable (by this clear, or by its age bound + // as a backstop) instead of being held open forever by a clock that never stops moving. + it("screenshot-table gate (#10061): once independent triggers spend the budget, the latch is cleared instead of the clock being kept moving forever", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [{ path: "/app", afterUrl: "https://worker.example/loopover/shot?url=x", afterUrlMobile: "https://worker.example/loopover/shot?url=x" }], + interactions: [], + previewPending: false, + renderFailed: true, + previewUnobtainable: false, + }); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), + }); + 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"); + 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/81/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/81/reviews")) return Response.json([]); + if (url.includes("/pulls/81/commits")) return Response.json([]); + if (url.endsWith("/pulls/81") && method === "PATCH") return Response.json({ number: 81, state: "closed" }); + if (url.endsWith("/pulls/81")) return Response.json({ number: 81, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis81" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis81/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis81/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis81/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/81/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/81/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 951 }, { status: 201 }); + if (url.includes("/check-runs/951") && method === "PATCH") return Response.json({ id: 951 }); + return new Response("not found", { status: 404 }); + }); + + try { + for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS + 1; i += 1) { + await processJob(env, { + type: "github-webhook", + deliveryId: `screenshot-multi-trigger-latch-81-${i}`, + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 81, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis81" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + } + } finally { + buildCaptureSpy.mockRestore(); + } + + const stored = await getPullRequest(env, "JSONbored/gittensory", 81); + expect(stored?.visualCaptureRetryPendingSha).toBeNull(); + expect(stored?.visualCaptureRetryPendingAt).toBeNull(); + }); + // #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