fix(review): make the visual-preview poll budget durable per head SHA

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(
               {

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;
+            }
+          }
         }
       }
     }

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<string> {
+  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<BudgetMarker | null> {
+  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<BudgetMarker>;
+    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<number> {
+  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<void> {
+  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
+  }
+}

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<string, string>();
+  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();
+  });
+});

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