From 22efea68ccab391f229f3cfc86ce0920f8b1a7ca Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:31:25 -0700 Subject: [PATCH] fix(review): make the visual-preview poll budget durable per head SHA Closes #6323 MAX_PREVIEW_POLLS (processors.ts) was meant to bound the visual-preview self-poll to 5 attempts before giving up on a preview deploy that never becomes discoverable. It did nothing in practice: the counter only lived inside the dedicated recapture-preview job chain's own `attempt` payload field, and at least three other re-review triggers (CI-completion webhooks, deployment_status webhooks, the sweep pass) call reReviewStoredPullRequest without threading it through -- each one independently re-arms a fresh 5-attempt budget whenever buildCapture reports previewPending. Confirmed live on JSONbored/metagraphed#6036: 7 check-runs completing over ~8 minutes, each capable of re-arming the countdown, produced 12+ re-review comment edits over 52+ minutes on a single PR, still ongoing when observed -- pure wasted Browser Rendering + queue/webhook cost on a PR whose "after" side could never resolve anyway. New src/review/visual/preview-poll-budget.ts tracks attempts durably, keyed by head SHA, in an R2 marker mirroring actions-fallback.ts's own isFallbackDispatchInFlight/markFallbackDispatched pattern exactly (fail-open reads, best-effort writes, a 24h max-age fail-safe expiry). buildCapture consults + increments it before treating a still-building preview as poll-worthy; once exhausted for a head, previewPending becomes false and the honest "review manually" FAILED placeholder shows instead of an eternal loading spinner. Because every caller already only reschedules when previewPending is true, no other call site needs to change -- the fix is fully contained to where the budget check lives. processors.ts's local MAX_PREVIEW_POLLS is consolidated into the new module's exported MAX_PREVIEW_POLL_ATTEMPTS (one source of truth) and left in place as a now-redundant secondary bound for the dedicated self-poll job chain -- harmless, but no longer the thing that actually stops a never-resolving preview from polling forever. --- src/queue/processors.ts | 11 ++- src/review/visual/capture.ts | 18 +++- src/review/visual/preview-poll-budget.ts | 79 +++++++++++++++ test/unit/preview-poll-budget.test.ts | 121 +++++++++++++++++++++++ test/unit/visual-capture.test.ts | 57 +++++++++++ 5 files changed, 280 insertions(+), 6 deletions(-) create mode 100644 src/review/visual/preview-poll-budget.ts create mode 100644 test/unit/preview-poll-budget.test.ts diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2ce46b6881..9a3acada9e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -446,6 +446,7 @@ export { export { processJob } from "./job-dispatch"; import { isVisualPath } from "../review/visual/paths"; import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture"; +import { MAX_PREVIEW_POLL_ATTEMPTS } from "../review/visual/preview-poll-budget"; import { clearFallbackDispatchMarker, fallbackShotFileName, @@ -3826,10 +3827,12 @@ async function maybeForceFreshRebase( 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, up to MAX_PREVIEW_POLLS times (so a -// never-resolving preview can't poll forever ~ 5×90s = 7.5min). +// 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; -const MAX_PREVIEW_POLLS = 5; /** * Coalesce CI-completion re-reviews: claims a per-PR window and returns true if this PR was already re-reviewed @@ -10097,7 +10100,7 @@ async function maybePublishPrPublicSurface( const previewPollAttempt = webhook.previewPollAttempt ?? 0; if ( capture.previewPending && - previewPollAttempt < MAX_PREVIEW_POLLS + previewPollAttempt < MAX_PREVIEW_POLL_ATTEMPTS ) { await env.JOBS.send( { diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index db1fe4d0f5..3c89673502 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -20,6 +20,7 @@ import { isSafeHttpUrl } from "../content-lane/safe-url"; import { downscaleForVision } from "./image-downscale"; import type { GitHubRateLimitAdmissionKey } from "../../github/client"; import { dispatchVisualCaptureFallback, fallbackShotR2Key, isFallbackDispatchInFlight, markFallbackDispatched } from "./actions-fallback"; +import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "./preview-poll-budget"; import { findPreviewUrlFromChecks, findPreviewUrlFromPrComments, @@ -522,8 +523,21 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge } if (!previewBase && target.headSha) { const buildState = await getPreviewBuildState({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey }); - if (buildState === "failed") previewFailed = true; - else if (buildState === "building" || buildState === "succeeded") previewPending = true; + if (buildState === "failed") { + previewFailed = true; + } else if (buildState === "building" || buildState === "succeeded") { + // #6323: bound how many times ANY trigger treats this head as worth another attempt, not just + // the dedicated self-poll job chain -- see preview-poll-budget.ts's own doc comment for the bug + // this fixes. Past the budget, give up honestly (the FAILED placeholder card, "review manually") + // rather than an eternally-spinning "loading" placeholder that never resolves. + const attempts = await previewPollAttemptCount(env, target.headSha); + if (attempts >= MAX_PREVIEW_POLL_ATTEMPTS) { + previewFailed = true; + } else { + await recordPreviewPollAttempt(env, target.headSha); + previewPending = true; + } + } } } } diff --git a/src/review/visual/preview-poll-budget.ts b/src/review/visual/preview-poll-budget.ts new file mode 100644 index 0000000000..559b963110 --- /dev/null +++ b/src/review/visual/preview-poll-budget.ts @@ -0,0 +1,79 @@ +// Durable visual-preview poll budget (#6323): bounds how many times buildCapture will treat a still-building +// preview deploy as "keep trying" for a given head SHA, REGARDLESS of which trigger (the dedicated self-poll +// job chain, a CI-completion webhook, a deployment_status webhook, or a sweep pass) caused this particular +// buildCapture call. +// +// The pre-existing MAX_PREVIEW_POLLS cap in processors.ts only bounded the self-poll job chain's OWN +// `attempt` payload field. Every OTHER re-review trigger calls reReviewStoredPullRequest without threading +// that counter through at all, so it silently reads back as 0 -- each one independently re-arms a fresh +// 5-attempt budget. A repo whose CI never produces a discoverable preview deployment (buildCapture's +// discovery chain finds nothing to attach a URL to) then gets polled far more than 5 times total, for as +// long as ANY of those other triggers keeps firing. Confirmed live: JSONbored/metagraphed#6036 -- 12+ +// re-review comment edits over 52+ minutes on a single PR, still ongoing when observed. +// +// This module makes the budget durable and keyed by headSha instead of by job-chain payload, mirroring +// actions-fallback.ts's own isFallbackDispatchInFlight/markFallbackDispatched R2-marker pattern exactly +// (same fail-open-on-read-error contract, same best-effort-write contract, same max-age fail-safe expiry so +// a marker can never block a genuinely NEW attempt forever). buildCapture consults + increments this before +// treating a "still building" preview-build state as poll-worthy; once the budget is exhausted for a head, +// buildCapture stops signaling previewPending for it, so EVERY caller's existing "only reschedule when +// previewPending" logic naturally stops rescheduling too -- no other call site needs to change. +import { sha256Hex } from "../../utils/crypto"; + +const BUDGET_R2_NAMESPACE = "loopover/preview-poll-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. +const BUDGET_MARKER_MAX_AGE_MS = 24 * 60 * 60 * 1000; +// The total number of "still building, keep trying" attempts allowed per head SHA across ALL triggers +// 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; + +type BudgetMarker = { count: number; firstAttemptAt: number }; + +async function budgetR2Key(headSha: string): Promise { + const fingerprint = await sha256Hex(`${headSha}:preview-poll-budget`); + return `${BUDGET_R2_NAMESPACE}${fingerprint.slice(0, 40)}.json`; +} + +/** Shared read path for both public functions below. Returns null (fail-open toward "no attempts yet") on + * any read error, a malformed marker, or one older than BUDGET_MARKER_MAX_AGE_MS -- a stale marker is + * treated as absent, not as "budget still exhausted from a previous, unrelated review cycle". */ +async function readBudgetMarker(env: Env, headSha: string): Promise { + if (!env.REVIEW_AUDIT) return null; + try { + const object = await env.REVIEW_AUDIT.get(await budgetR2Key(headSha)); + if (!object) return null; + const marker = JSON.parse(await new Response(object.body).text()) as Partial; + if (typeof marker.count !== "number" || typeof marker.firstAttemptAt !== "number") return null; + if (Date.now() - marker.firstAttemptAt >= BUDGET_MARKER_MAX_AGE_MS) return null; + return { count: marker.count, firstAttemptAt: marker.firstAttemptAt }; + } catch { + return null; + } +} + +/** 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))?.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 + * 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 { + if (!env.REVIEW_AUDIT) return; + try { + const existing = await readBudgetMarker(env, headSha); + const marker: BudgetMarker = { count: (existing?.count ?? 0) + 1, firstAttemptAt: existing?.firstAttemptAt ?? Date.now() }; + await env.REVIEW_AUDIT.put(await budgetR2Key(headSha), JSON.stringify(marker), { httpMetadata: { contentType: "application/json" } }); + } catch { + // best effort -- see doc comment above + } +} diff --git a/test/unit/preview-poll-budget.test.ts b/test/unit/preview-poll-budget.test.ts new file mode 100644 index 0000000000..646abaa878 --- /dev/null +++ b/test/unit/preview-poll-budget.test.ts @@ -0,0 +1,121 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "../../src/review/visual/preview-poll-budget"; +import { createTestEnv } from "../helpers/d1"; + +const HEAD_SHA = "budget-head-sha-1234567890"; + +function memoryBudgetStore(options: { failGet?: boolean; failPut?: boolean; forcedValue?: string } = {}): R2Bucket { + const store = new Map(); + return { + async get(key: string) { + if (options.failGet) throw new Error("simulated budget-marker read failure"); + // forcedValue bypasses the real per-key store entirely -- used to simulate a corrupted/malformed stored + // marker without needing to know the module's own private R2-key derivation. + if (options.forcedValue !== undefined) return { body: new Response(options.forcedValue).body } as unknown as R2ObjectBody; + const value = store.get(key); + return value === undefined ? null : ({ body: new Response(value).body } as unknown as R2ObjectBody); + }, + async put(key: string, value: unknown) { + if (options.failPut) throw new Error("simulated budget-marker write failure"); + store.set(key, await new Response(value as BodyInit).text()); + return { key } as unknown as R2Object; + }, + } as unknown as R2Bucket; +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("previewPollAttemptCount / recordPreviewPollAttempt (#6323 -- durable per-headSha preview-poll budget)", () => { + it("0 when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("0 when no marker has ever been recorded", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("1 immediately after a single recordPreviewPollAttempt", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + await recordPreviewPollAttempt(env, HEAD_SHA); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(1); + }); + + it("accumulates across repeated attempts, matching MAX_PREVIEW_POLL_ATTEMPTS' own scale", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) { + await recordPreviewPollAttempt(env, HEAD_SHA); + } + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS); + }); + + it("tracks DIFFERENT head SHAs independently -- a new push resets the budget", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + await recordPreviewPollAttempt(env, "old-head-sha"); + await recordPreviewPollAttempt(env, "old-head-sha"); + await expect(previewPollAttemptCount(env, "old-head-sha")).resolves.toBe(2); + await expect(previewPollAttemptCount(env, "new-head-sha")).resolves.toBe(0); + }); + + it("0 once the marker is older than the max age (an abandoned/long-stale PR)", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + vi.useFakeTimers(); + try { + await recordPreviewPollAttempt(env, HEAD_SHA); + vi.advanceTimersByTime(25 * 60 * 60 * 1000); // past the 24-hour max age + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("preserves the original firstAttemptAt across increments -- the max age expires from the FIRST attempt, not the latest", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore() }); + vi.useFakeTimers(); + try { + await recordPreviewPollAttempt(env, HEAD_SHA); + vi.advanceTimersByTime(23 * 60 * 60 * 1000); // still within the 24h window + await recordPreviewPollAttempt(env, HEAD_SHA); // a SECOND attempt, ~23h after the first + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(2); + vi.advanceTimersByTime(2 * 60 * 60 * 1000); // now ~25h after the FIRST attempt (past max age) + // If firstAttemptAt were wrongly reset on the second write, this would still read as fresh (count 2). + // It must instead expire, proving the ORIGINAL firstAttemptAt was preserved. + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("0 when the stored marker isn't valid JSON at all", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ forcedValue: "{not valid json" }) }); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("0 when the stored marker is missing its count/firstAttemptAt fields", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ forcedValue: JSON.stringify({ unrelated: true }) }) }); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("previewPollAttemptCount fails open (0) when the R2 read itself throws", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ failGet: true }) }); + await expect(previewPollAttemptCount(env, HEAD_SHA)).resolves.toBe(0); + }); + + it("recordPreviewPollAttempt never throws when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(recordPreviewPollAttempt(env, HEAD_SHA)).resolves.toBeUndefined(); + }); + + it("recordPreviewPollAttempt never throws (best-effort) when the R2 write itself fails", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ failPut: true }) }); + await expect(recordPreviewPollAttempt(env, HEAD_SHA)).resolves.toBeUndefined(); + }); + + it("recordPreviewPollAttempt never throws (best-effort) when the read INSIDE the write path fails", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryBudgetStore({ failGet: true }) }); + await expect(recordPreviewPollAttempt(env, HEAD_SHA)).resolves.toBeUndefined(); + }); +}); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 148746b872..22bb42aafc 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -8,6 +8,7 @@ import { fallbackShotR2Key, markFallbackDispatched } from "../../src/review/visu import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; import type { CaptureRoute } from "../../src/review/visual/capture"; import * as pixelDiffModule from "../../src/review/visual/pixel-diff"; +import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "../../src/review/visual/preview-poll-budget"; import * as previewUrlModule from "../../src/review/visual/preview-url"; import * as scrollGifModule from "../../src/review/visual/scroll-gif"; import * as shotModule from "../../src/review/visual/shot"; @@ -269,6 +270,62 @@ describe("visual capture preview discovery", () => { expect(result.previewPending).toBe(true); }); + it("#6323: a 'building' buildState records ONE preview-poll attempt for this head SHA", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + return Response.json({ check_runs: [{ name: "Cloudflare Workers Builds", status: "in_progress" }] }); + } + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 14, headSha: "budget-head-1", previewFromChecks: true }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(true); + await expect(previewPollAttemptCount(env, "budget-head-1")).resolves.toBe(1); + }); + + it("#6323: past MAX_PREVIEW_POLL_ATTEMPTS for this head SHA, gives up honestly instead of polling forever -- REGARDLESS of which trigger called buildCapture", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + return Response.json({ check_runs: [{ name: "Cloudflare Workers Builds", status: "in_progress" }] }); + } + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + // Pre-exhaust the durable budget for this head -- simulates several PRIOR buildCapture calls, regardless + // of whether they came from the dedicated self-poll job chain, a CI-completion webhook, a + // deployment_status webhook, or a sweep pass (this module doesn't know or care which). + for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) { + await recordPreviewPollAttempt(env, "budget-head-2"); + } + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 15, headSha: "budget-head-2", previewFromChecks: true }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(false); + expect(result.routes[0]?.afterUrl).toContain("placeholder=failed"); + // The exhausted attempt itself is NOT recorded again -- the count stays capped, not incremented forever. + await expect(previewPollAttemptCount(env, "budget-head-2")).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS); + }); + it("leaves the capture non-pending when no matching preview check run exists at all (buildState 'absent')", async () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString();