From 2a93f21c784baa8188729e6671709d5818d18c0d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 23:58:48 -0700 Subject: [PATCH] fix(review): replace the live in-flight dispatch check with a persisted R2 marker The prior fix queried GitHub's own runs API right before dispatching, which has a real gap: a freshly-dispatched run isn't guaranteed to be visible via that API the instant dispatch returns, so a poll landing in that window could still redispatch and cancel the in-flight run. A persisted marker written synchronously on a successful dispatch, cleared on the webhook_run completion (any conclusion), and self-expiring after the workflow's 15-minute timeout closes that gap without depending on API eventual consistency. --- src/queue/processors.ts | 20 +-- src/review/visual/actions-fallback.ts | 104 ++++++++++----- src/review/visual/capture.ts | 21 ++-- test/unit/actions-fallback-webhook.test.ts | 71 ++++++++++- test/unit/actions-fallback.test.ts | 139 +++++++++++++-------- test/unit/visual-capture.test.ts | 20 ++- 6 files changed, 263 insertions(+), 112 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 5abb419a11..1fc9397f7b 100644 --- a/src/queue/processors.ts +++ b/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, diff --git a/src/review/visual/actions-fallback.ts b/src/review/visual/actions-fallback.ts index 9681344e56..ed8d7e2f8a 100644 --- a/src/review/visual/actions-fallback.ts +++ b/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 { + 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 { - const base = `https://api.github.com/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 { + 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 { + 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 { + 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). // --------------------------------------------------------------------------------------------------------- diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 18940f3b98..548985a53e 100644 --- a/src/review/visual/capture.ts +++ b/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,12 +408,15 @@ 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, @@ -421,7 +424,9 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge headSha: target.headSha, routes, rateLimitAdmissionKey, - })); + }); + if (dispatched) await markFallbackDispatched(env, target.headSha); + } if (dispatched) previewPending = true; } diff --git a/test/unit/actions-fallback-webhook.test.ts b/test/unit/actions-fallback-webhook.test.ts index 21a73daf90..5bf58c1ba2 100644 --- a/test/unit/actions-fallback-webhook.test.ts +++ b/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"); diff --git a/test/unit/actions-fallback.test.ts b/test/unit/actions-fallback.test.ts index ce70be9e6c..2a01a82c1d 100644 --- a/test/unit/actions-fallback.test.ts +++ b/test/unit/actions-fallback.test.ts @@ -1,19 +1,45 @@ import { deflateRawSync } from "node:zlib"; import { afterEach, describe, expect, it, vi } from "vitest"; import { clearGitHubResponseCacheForTest } from "../../src/github/client"; +import { sha256Hex } from "../../src/utils/crypto"; +import { createTestEnv } from "../helpers/d1"; import { + clearFallbackDispatchMarker, dispatchVisualCaptureFallback, fallbackShotFileName, fallbackShotR2Key, fetchFallbackArtifactShots, FALLBACK_ARTIFACT_NAME, - hasInFlightFallbackDispatch, + isFallbackDispatchInFlight, isGithubArtifactStorageUrl, + markFallbackDispatched, parseFallbackRunCorrelation, parseZipEntries, slugifyRoutePath, } from "../../src/review/visual/actions-fallback"; +/** A minimal in-memory R2Bucket for the dispatch-marker tests -- get/put/delete only, with optional + * per-operation failure injection, mirroring visual-capture.test.ts's own memoryReviewAudit() pattern. */ +function memoryFallbackMarkerStore(options: { failGet?: boolean; failPut?: boolean; failDelete?: boolean } = {}): R2Bucket { + const store = new Map(); + return { + async get(key: string) { + if (options.failGet) throw new Error("simulated marker read failure"); + 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 marker write failure"); + store.set(key, await new Response(value as BodyInit).text()); + return { key } as unknown as R2Object; + }, + async delete(key: string) { + if (options.failDelete) throw new Error("simulated marker delete failure"); + store.delete(key); + }, + } as unknown as R2Bucket; +} + afterEach(() => { clearGitHubResponseCacheForTest(); vi.unstubAllGlobals(); @@ -350,79 +376,86 @@ describe("dispatchVisualCaptureFallback", () => { }); }); -describe("hasInFlightFallbackDispatch (#4112 review fix -- avoid cancel-in-progress re-dispatch)", () => { +describe("isFallbackDispatchInFlight / markFallbackDispatched / clearFallbackDispatchMarker (#4112 review fix -- persisted R2 sentinel, avoid cancel-in-progress re-dispatch)", () => { const HEAD_SHA = "cafebabecafebabecafebabecafebabecafebabe"; - function runsResponse(runs: Array<{ status?: string; display_title?: string }>): Response { - return Response.json({ workflow_runs: runs }); - } + it("false when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); + }); - it("true when a QUEUED run matches this exact pr+headSha", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "queued", display_title: `gittensory-visual-fallback pr=7 sha=${HEAD_SHA}` }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(true); + it("false when no marker has ever been written", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore() }); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); }); - it("true when an IN_PROGRESS run matches (case-insensitive headSha)", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "in_progress", display_title: `gittensory-visual-fallback pr=7 sha=${HEAD_SHA}` }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA.toUpperCase() }); - expect(inFlight).toBe(true); + it("true immediately after markFallbackDispatched", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore() }); + await markFallbackDispatched(env, HEAD_SHA); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(true); }); - it("false for an empty run list", async () => { - vi.stubGlobal("fetch", async () => runsResponse([])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("false once the marker is older than the max age (abandoned dispatch)", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore() }); + vi.useFakeTimers(); + try { + await markFallbackDispatched(env, HEAD_SHA); + vi.advanceTimersByTime(19 * 60 * 1000); // past the 18-minute max age + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); + } finally { + vi.useRealTimers(); + } }); - it("false when the matching run has already COMPLETED (not queued/in_progress)", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "completed", display_title: `gittensory-visual-fallback pr=7 sha=${HEAD_SHA}` }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("false when the stored marker isn't valid JSON at all (caught by the outer try/catch)", async () => { + const store = memoryFallbackMarkerStore(); + const env = createTestEnv({ REVIEW_AUDIT: store }); + const fingerprint = await sha256Hex(`${HEAD_SHA}:actions-fallback:dispatch-marker`); + const key = `gittensory/fallback-dispatch/${fingerprint.slice(0, 40)}.json`; + await store.put(key, "not json"); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); }); - it("false when an in-progress run exists for a DIFFERENT PR", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "in_progress", display_title: `gittensory-visual-fallback pr=99 sha=${HEAD_SHA}` }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("false when the stored marker parses as JSON but is missing dispatchedAt (the explicit shape check, not the catch)", async () => { + const store = memoryFallbackMarkerStore(); + const env = createTestEnv({ REVIEW_AUDIT: store }); + const fingerprint = await sha256Hex(`${HEAD_SHA}:actions-fallback:dispatch-marker`); + const key = `gittensory/fallback-dispatch/${fingerprint.slice(0, 40)}.json`; + await store.put(key, JSON.stringify({ someOtherField: true })); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); }); - it("false when an in-progress run exists for the same PR but a DIFFERENT headSha (new push)", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "in_progress", display_title: `gittensory-visual-fallback pr=7 sha=${"f".repeat(40)}` }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("false (never throws) when the R2 read itself fails", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore({ failGet: true }) }); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); }); - it("false when the run's display_title doesn't match the expected correlation shape at all", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "in_progress", display_title: "Manually triggered run" }])); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("markFallbackDispatched never throws when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(markFallbackDispatched(env, HEAD_SHA)).resolves.toBeUndefined(); }); - it("false on a non-ok response", async () => { - vi.stubGlobal("fetch", async () => new Response("nope", { status: 500 })); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("markFallbackDispatched never throws (best-effort) when the R2 write itself fails", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore({ failPut: true }) }); + await expect(markFallbackDispatched(env, HEAD_SHA)).resolves.toBeUndefined(); }); - it("false (never throws) on a network failure", async () => { - vi.stubGlobal("fetch", async () => { - throw new Error("network down"); - }); - const inFlight = await hasInFlightFallbackDispatch({ token: "tok", repo: { owner: "acme", repo: "widgets" }, prNumber: 7, headSha: HEAD_SHA }); - expect(inFlight).toBe(false); + it("clearFallbackDispatchMarker makes a subsequent isFallbackDispatchInFlight call false again", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore() }); + await markFallbackDispatched(env, HEAD_SHA); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(true); + await clearFallbackDispatchMarker(env, HEAD_SHA); + await expect(isFallbackDispatchInFlight(env, HEAD_SHA)).resolves.toBe(false); }); - it("true when a rateLimitAdmissionKey is supplied and a match is found", async () => { - vi.stubGlobal("fetch", async () => runsResponse([{ status: "queued", display_title: `gittensory-visual-fallback pr=7 sha=${HEAD_SHA}` }])); - const inFlight = await hasInFlightFallbackDispatch({ - token: "tok", - repo: { owner: "acme", repo: "widgets" }, - prNumber: 7, - headSha: HEAD_SHA, - rateLimitAdmissionKey: "installation:1", - }); - expect(inFlight).toBe(true); + it("clearFallbackDispatchMarker never throws when REVIEW_AUDIT isn't configured", async () => { + const env = createTestEnv(); + await expect(clearFallbackDispatchMarker(env, HEAD_SHA)).resolves.toBeUndefined(); + }); + + it("clearFallbackDispatchMarker never throws (best-effort) when the R2 delete itself fails", async () => { + const env = createTestEnv({ REVIEW_AUDIT: memoryFallbackMarkerStore({ failDelete: true }) }); + await expect(clearFallbackDispatchMarker(env, HEAD_SHA)).resolves.toBeUndefined(); }); }); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 14f7ba42f7..08cf3bffcd 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -4,7 +4,7 @@ import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation, } from "../../src/github/client"; -import { fallbackShotR2Key } from "../../src/review/visual/actions-fallback"; +import { fallbackShotR2Key, markFallbackDispatched } from "../../src/review/visual/actions-fallback"; import { buildCapture, 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"; @@ -1289,14 +1289,11 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f expect(result.previewPending).toBe(true); }); - it("skips dispatching a NEW run when one is already queued/in-progress for this exact pr+headSha, but still marks the capture pending (#4112 review fix)", async () => { + it("skips dispatching a NEW run when one was already dispatched for this exact headSha (persisted marker), but still marks the capture pending (#4112 review fix)", async () => { let dispatchCalled = false; vi.stubGlobal( "fetch", stubNoPreviewFound((url) => { - if (url.includes("/actions/workflows/visual-capture-fallback.yml/runs")) { - return Response.json({ workflow_runs: [{ status: "in_progress", display_title: "gittensory-visual-fallback pr=20 sha=cafebabecafebabecafebabecafebabecafebabe" }] }); - } if (url.includes("/dispatches")) { dispatchCalled = true; return new Response(null, { status: 204 }); @@ -1304,9 +1301,11 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f return null; }), ); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + await markFallbackDispatched(env, "cafebabecafebabecafebabecafebabecafebabe"); const result = await buildCapture( - createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + env, "installation-token", { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main" }, ["apps/gittensory-ui/src/routes/app.index.tsx"], @@ -1318,14 +1317,11 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f expect(result.previewPending).toBe(true); }); - it("dispatches a NEW run when the only in-flight run found is for a DIFFERENT headSha (a later push)", async () => { + it("dispatches a NEW run when the persisted marker is for a DIFFERENT headSha (a later push)", async () => { let dispatchCalled = false; vi.stubGlobal( "fetch", stubNoPreviewFound((url) => { - if (url.includes("/actions/workflows/visual-capture-fallback.yml/runs")) { - return Response.json({ workflow_runs: [{ status: "in_progress", display_title: "gittensory-visual-fallback pr=20 sha=ffffffffffffffffffffffffffffffffffffffff" }] }); - } if (url.includes("/dispatches")) { dispatchCalled = true; return new Response(null, { status: 204 }); @@ -1333,9 +1329,11 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f return null; }), ); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + await markFallbackDispatched(env, "ffffffffffffffffffffffffffffffffffffffff"); const result = await buildCapture( - createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + env, "installation-token", { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main" }, ["apps/gittensory-ui/src/routes/app.index.tsx"],