fix(review): replace the live in-flight dispatch check with a persisted R2 marker

src/queue/processors.ts

@@ -361,6 +361,7 @@ import { screenshotsAllowed } from "../review/visual-wire";
 import { isVisualPath } from "../review/visual/paths";
 import { buildCapture, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture";
 import {
+  clearFallbackDispatchMarker,
   fallbackShotFileName,
   fallbackShotR2Key,
   fetchFallbackArtifactShots,
@@ -4601,13 +4602,18 @@ async function maybeCaptureOnActionsFallbackWorkflowRun(
   ).workflow_run;
   if (run?.name !== FALLBACK_WORKFLOW_NAME || run?.event !== "workflow_dispatch") return false;
 
-  if (run.conclusion === "success" && run.id && isConvergenceRepoAllowed(env, repoFullName)) {
-    const correlation = parseFallbackRunCorrelation(run.display_title);
-    if (correlation) {
-      const admissionKey = githubRateLimitAdmissionKeyForInstallation(installationId);
-      await storeVisualCaptureFallbackShots(env, repoFullName, installationId, run.id, correlation.prNumber, correlation.headSha, admissionKey);
-      await reReviewStoredPullRequest(env, deliveryId, installationId, repoFullName, correlation.prNumber);
-    }
+  const correlation = parseFallbackRunCorrelation(run.display_title);
+  if (correlation) {
+    // The run has settled -- success, failure, cancelled, or timed_out all mean "no longer in flight," so
+    // clear the dispatch marker regardless of conclusion (#4112 review fix). Otherwise a genuinely failed run
+    // would leave the marker in place for the rest of FALLBACK_DISPATCH_MARKER_MAX_AGE_MS, blocking a retry
+    // that could otherwise succeed immediately.
+    await clearFallbackDispatchMarker(env, correlation.headSha);
+  }
+  if (run.conclusion === "success" && run.id && correlation && isConvergenceRepoAllowed(env, repoFullName)) {
+    const admissionKey = githubRateLimitAdmissionKeyForInstallation(installationId);
+    await storeVisualCaptureFallbackShots(env, repoFullName, installationId, run.id, correlation.prNumber, correlation.headSha, admissionKey);
+    await reReviewStoredPullRequest(env, deliveryId, installationId, repoFullName, correlation.prNumber);
   }
   await recordWebhookEvent(env, {
     deliveryId,

src/review/visual/actions-fallback.ts

@@ -144,45 +144,85 @@ export function parseFallbackRunCorrelation(displayTitle: string | undefined | n
   return { prNumber, headSha: (match[2] as string).toLowerCase() };
 }
 
-/** True when a fallback run for this EXACT (prNumber, headSha) is already queued or in progress -- checked
- *  by buildCapture before dispatching, so the existing recapture-poll retry (every 90s, up to 5 attempts,
- *  see PREVIEW_POLL_SECONDS/MAX_PREVIEW_POLLS in processors.ts) doesn't repeatedly re-dispatch while waiting
- *  for the SAME run's workflow_run completion. That matters because the workflow's own `concurrency: group:
+// ---------------------------------------------------------------------------------------------------------
+// Dispatch in-flight marker -- a persisted R2 sentinel, not a live GitHub API query (#4112 review fix).
+// ---------------------------------------------------------------------------------------------------------
+
+const FALLBACK_DISPATCH_MARKER_NAMESPACE = "gittensory/fallback-dispatch/";
+
+/** The workflow's own `timeout-minutes: 15` (visual-capture-fallback.yml) plus a buffer for GitHub's own
+ *  runner-queueing delay before the job even starts -- a marker older than this is treated as abandoned
+ *  (the run either finished without a webhook ever reaching us, or GitHub silently dropped the dispatch)
+ *  rather than blocking dispatch forever. */
+const FALLBACK_DISPATCH_MARKER_MAX_AGE_MS = 18 * 60 * 1000;
+
+async function fallbackDispatchMarkerR2Key(headSha: string): Promise<string> {
+  const fingerprint = await sha256Hex(`${headSha}:actions-fallback:dispatch-marker`);
+  return `${FALLBACK_DISPATCH_MARKER_NAMESPACE}${fingerprint.slice(0, 40)}.json`;
+}
+
+/** True when a fallback run for this head SHA was dispatched recently enough that it may still be
+ *  queued/in-progress -- checked by buildCapture BEFORE dispatching, so the existing recapture-poll retry
+ *  (every 90s, up to 5 attempts -- see PREVIEW_POLL_SECONDS/MAX_PREVIEW_POLLS in processors.ts, a 7.5-minute
+ *  window comfortably inside the workflow's own 15-minute timeout) doesn't repeatedly re-dispatch while a
+ *  build is still running. That matters because the workflow's own `concurrency: group:
  *  visual-capture-fallback-${{ inputs.head_sha }}` + `cancel-in-progress: true` means a second dispatch for
- *  the same head SHA CANCELS the first -- without this check, a poll firing before a slow build finishes
- *  would cancel-and-restart it every 90s and the fallback could never complete. Queries GitHub's own run
- *  list rather than persisting new dispatch-tracking state, mirroring this pipeline's existing
- *  live-query-don't-persist pattern (getLatestDeploymentStatus, findPreviewUrlFromChecks). Fails OPEN (false,
- *  "nothing in flight") on any error -- a transient list-runs failure should still let the existing
- *  concurrency group be the backstop dedup, not silently stop the fallback from ever being tried. */
-export async function hasInFlightFallbackDispatch(params: {
-  token: string;
-  repo: GitHubRepo;
-  prNumber: number;
-  headSha: string;
-  rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined;
-}): Promise<boolean> {
-  const base = `https://github.com/ghapi/repos/${params.repo.owner}/${params.repo.repo}`;
+ *  the same head SHA CANCELS the first -- without this check, a poll firing well within the 15-minute budget
+ *  would cancel-and-restart the run on every single poll and the fallback could never complete.
+ *
+ *  A PERSISTED marker (not a live GitHub list-runs query) is deliberate: a freshly-dispatched run isn't
+ *  guaranteed to be visible via the Actions API the instant `dispatchVisualCaptureFallback` returns (GitHub's
+ *  own eventual consistency), so a live query taken right after dispatch could itself race and report
+ *  "nothing in flight" moments after a dispatch just succeeded. Writing the marker synchronously on a
+ *  successful dispatch closes that gap. Fails OPEN (false, "nothing in flight") on any read error -- a
+ *  transient R2 failure should still let the existing concurrency group be the backstop dedup, not silently
+ *  stop the fallback from ever being tried. */
+export async function isFallbackDispatchInFlight(env: Env, headSha: string): Promise<boolean> {
+  if (!env.REVIEW_AUDIT) return false;
   try {
-    const response = await timeoutFetch(`${base}/actions/workflows/${FALLBACK_WORKFLOW_FILE}/runs?event=workflow_dispatch&per_page=20`, {
-      headers: githubApiHeaders(params.token),
-      signal: AbortSignal.timeout(DEFAULT_TIMEOUT_MS),
-      githubRateLimitAdmission: params.rateLimitAdmissionKey !== undefined,
-      ...(params.rateLimitAdmissionKey ? { githubRateLimitAdmissionKey: params.rateLimitAdmissionKey } : {}),
-    });
-    if (!response.ok) return false;
-    const payload = (await response.json().catch(() => null)) as { workflow_runs?: Array<{ status?: string; display_title?: string }> } | null;
-    const headSha = params.headSha.toLowerCase();
-    return (payload?.workflow_runs ?? []).some((run) => {
-      if (run.status !== "queued" && run.status !== "in_progress") return false;
-      const correlation = parseFallbackRunCorrelation(run.display_title);
-      return correlation !== null && correlation.prNumber === params.prNumber && correlation.headSha === headSha;
-    });
+    const object = await env.REVIEW_AUDIT.get(await fallbackDispatchMarkerR2Key(headSha));
+    if (!object) return false;
+    const text = await new Response(object.body).text();
+    const marker = JSON.parse(text) as { dispatchedAt?: number };
+    if (typeof marker.dispatchedAt !== "number") return false;
+    return Date.now() - marker.dispatchedAt < FALLBACK_DISPATCH_MARKER_MAX_AGE_MS;
   } catch {
     return false;
   }
 }
 
+/** Record that a fallback dispatch just succeeded for this head SHA, so a subsequent buildCapture call
+ *  (e.g. the next recapture poll) sees it via isFallbackDispatchInFlight instead of re-dispatching. Best
+ *  effort -- a failed write just means the concurrency group's cancel-in-progress behavior is the only
+ *  remaining backstop, same as before this marker existed. */
+export async function markFallbackDispatched(env: Env, headSha: string): Promise<void> {
+  if (!env.REVIEW_AUDIT) return;
+  try {
+    const key = await fallbackDispatchMarkerR2Key(headSha);
+    await env.REVIEW_AUDIT.put(key, JSON.stringify({ dispatchedAt: Date.now() }), {
+      httpMetadata: { contentType: "application/json" },
+    });
+  } catch {
+    // best effort -- see doc comment above
+  }
+}
+
+/** Clear the in-flight marker once the dispatched run has settled (ANY conclusion -- success, failure,
+ *  cancelled, timed_out all mean "no longer in flight"), called from the workflow_run webhook handler in
+ *  processors.ts. Best effort -- if this never runs (a lost webhook delivery), FALLBACK_DISPATCH_MARKER_MAX_AGE_MS
+ *  is the fail-safe expiry so a genuinely stuck marker can't block retries forever. A try/catch (not just a
+ *  `.catch()` on the delete call) matters here: a minimal/partial R2Bucket implementation that doesn't
+ *  implement `delete` at all throws SYNCHRONOUSLY at the call site (`TypeError: ... is not a function`),
+ *  before any `.catch()` on its return value would even attach. */
+export async function clearFallbackDispatchMarker(env: Env, headSha: string): Promise<void> {
+  if (!env.REVIEW_AUDIT) return;
+  try {
+    await env.REVIEW_AUDIT.delete(await fallbackDispatchMarkerR2Key(headSha));
+  } catch {
+    // best effort -- see doc comment above
+  }
+}
+
 // ---------------------------------------------------------------------------------------------------------
 // Minimal ZIP reader -- just enough to read a GitHub Actions artifact (STORED / DEFLATE entries only).
 // ---------------------------------------------------------------------------------------------------------

src/review/visual/capture.ts

@@ -12,7 +12,7 @@
 // default TanStack route convention; those hooks can return if a per-repo visual config is added.
 import { sha256Hex } from "../../utils/crypto";
 import type { GitHubRateLimitAdmissionKey } from "../../github/client";
-import { dispatchVisualCaptureFallback, fallbackShotR2Key, hasInFlightFallbackDispatch } from "./actions-fallback";
+import { dispatchVisualCaptureFallback, fallbackShotR2Key, isFallbackDispatchInFlight, markFallbackDispatched } from "./actions-fallback";
 import {
   findPreviewUrlFromChecks,
   findPreviewUrlFromPrComments,
@@ -408,20 +408,25 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge
     // Never re-dispatch onto an already in-flight run (#4112 review fix): the workflow's own `concurrency:
     // cancel-in-progress: true` group would CANCEL that run the instant a second dispatch for the same head
     // SHA lands, so a recapture-poll retry (every 90s -- see PREVIEW_POLL_SECONDS in processors.ts) firing
-    // before a slower build finishes could cancel-and-restart it forever and never complete. See
-    // hasInFlightFallbackDispatch's own doc comment for the full rationale.
-    const alreadyInFlight = await hasInFlightFallbackDispatch({ token, repo, prNumber: target.prNumber, headSha: target.headSha, rateLimitAdmissionKey });
-    const dispatched =
-      alreadyInFlight ||
-      (await dispatchVisualCaptureFallback({
+    // well within the workflow's 15-minute timeout could cancel-and-restart it on every poll and never
+    // complete. isFallbackDispatchInFlight checks a PERSISTED R2 marker rather than querying GitHub's runs
+    // API live, so there's no eventual-consistency gap right after a dispatch just succeeded -- see its own
+    // doc comment for the full rationale. markFallbackDispatched writes that marker on a successful dispatch;
+    // the webhook handler (processors.ts) clears it once the run settles.
+    const alreadyInFlight = await isFallbackDispatchInFlight(env, target.headSha);
+    let dispatched = alreadyInFlight;
+    if (!dispatched) {
+      dispatched = await dispatchVisualCaptureFallback({
         token,
         repo,
         ref: target.defaultBranchRef,
         prNumber: target.prNumber,
         headSha: target.headSha,
         routes,
         rateLimitAdmissionKey,
-      }));
+      });
+      if (dispatched) await markFallbackDispatched(env, target.headSha);
+    }
     if (dispatched) previewPending = true;
   }
 

test/unit/actions-fallback-webhook.test.ts

@@ -10,7 +10,7 @@ import {
 } from "../../src/db/repositories";
 import { clearInstallationTokenCacheForTest } from "../../src/github/app";
 import { clearGitHubResponseCacheForTest } from "../../src/github/client";
-import { fallbackShotR2Key, FALLBACK_ARTIFACT_NAME } from "../../src/review/visual/actions-fallback";
+import { fallbackShotR2Key, FALLBACK_ARTIFACT_NAME, isFallbackDispatchInFlight, markFallbackDispatched } from "../../src/review/visual/actions-fallback";
 import { processJob } from "../../src/queue/processors";
 import { createTestEnv } from "../helpers/d1";
 
@@ -91,6 +91,9 @@ function memoryReviewAudit(): R2Bucket {
       store.set(key, bytes);
       return { key } as unknown as R2Object;
     },
+    async delete(key: string) {
+      store.delete(key);
+    },
   } as unknown as R2Bucket;
 }
 
@@ -481,6 +484,72 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => {
     expect(artifactsListCalled).toBe(false);
   });
 
+  it("clears the dispatch marker on a FAILED run too (#4112 review fix -- a failed run shouldn't block a retry for the rest of the max-age window)", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: "owner/fallback-repo", REVIEW_AUDIT: memoryReviewAudit() });
+    await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe");
+    await markFallbackDispatched(env, "cafebabecafebabecafebabecafebabecafebabe");
+    await expect(isFallbackDispatchInFlight(env, "cafebabecafebabecafebabecafebabecafebabe")).resolves.toBe(true);
+    vi.stubGlobal("fetch", baseFetchStub({}));
+
+    await processJob(env, {
+      type: "github-webhook",
+      deliveryId: "failed-run-clears-marker",
+      eventName: "workflow_run",
+      payload: {
+        action: "completed",
+        repository: { name: "fallback-repo", full_name: "owner/fallback-repo", owner: { login: "owner" } },
+        installation: { id: 9101 },
+        workflow_run: { id: 590, name: "Gittensory Visual Capture Fallback", event: "workflow_dispatch", conclusion: "failure", display_title: "gittensory-visual-fallback pr=55 sha=cafebabecafebabecafebabecafebabecafebabe" },
+      },
+    } as never);
+
+    await expect(isFallbackDispatchInFlight(env, "cafebabecafebabecafebabecafebabecafebabe")).resolves.toBe(false);
+  });
+
+  it("clears the dispatch marker on a SUCCESSFUL run as well", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: "owner/fallback-repo", REVIEW_AUDIT: memoryReviewAudit() });
+    await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe");
+    await markFallbackDispatched(env, "cafebabecafebabecafebabecafebabecafebabe");
+    vi.stubGlobal("fetch", baseFetchStub({ "/actions/runs/": () => Response.json({ artifacts: [] }) }));
+
+    await processJob(env, {
+      type: "github-webhook",
+      deliveryId: "success-run-clears-marker",
+      eventName: "workflow_run",
+      payload: {
+        action: "completed",
+        repository: { name: "fallback-repo", full_name: "owner/fallback-repo", owner: { login: "owner" } },
+        installation: { id: 9101 },
+        workflow_run: { id: 591, name: "Gittensory Visual Capture Fallback", event: "workflow_dispatch", conclusion: "success", display_title: "gittensory-visual-fallback pr=55 sha=cafebabecafebabecafebabecafebabecafebabe" },
+      },
+    } as never);
+
+    await expect(isFallbackDispatchInFlight(env, "cafebabecafebabecafebabecafebabecafebabe")).resolves.toBe(false);
+  });
+
+  it("does not clear any marker when the run's display_title doesn't correlate to a PR (nothing to key the clear on)", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: "owner/fallback-repo", REVIEW_AUDIT: memoryReviewAudit() });
+    await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe");
+    await markFallbackDispatched(env, "cafebabecafebabecafebabecafebabecafebabe");
+    vi.stubGlobal("fetch", baseFetchStub({}));
+
+    await processJob(env, {
+      type: "github-webhook",
+      deliveryId: "uncorrelated-run-leaves-marker",
+      eventName: "workflow_run",
+      payload: {
+        action: "completed",
+        repository: { name: "fallback-repo", full_name: "owner/fallback-repo", owner: { login: "owner" } },
+        installation: { id: 9101 },
+        workflow_run: { id: 592, name: "Gittensory Visual Capture Fallback", event: "workflow_dispatch", conclusion: "success", display_title: "manually triggered" },
+      },
+    } as never);
+
+    // The marker is keyed by headSha "cafebabe...", which this run's uncorrelated title can't recover --
+    // it must stay untouched (still in flight) rather than being guessed/cleared.
+    await expect(isFallbackDispatchInFlight(env, "cafebabecafebabecafebabecafebabecafebabe")).resolves.toBe(true);
+  });
+
   it("does nothing when the run's display_title doesn't correlate to a PR (never guesses)", async () => {
     const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: "owner/fallback-repo", REVIEW_AUDIT: memoryReviewAudit() });
     await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe");