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();