diff --git a/scripts/check-pr-captures.mjs b/scripts/check-pr-captures.mjs index 1c9a1bc9b8..452debd40b 100644 --- a/scripts/check-pr-captures.mjs +++ b/scripts/check-pr-captures.mjs @@ -192,9 +192,13 @@ export function evaluate({ body, files }) { return { ok: false, problems: [...verdict.problems, ...captures] }; } -const DOWNLOAD_ATTEMPTS = 3; -const DOWNLOAD_DEADLINE_MS = 45_000; -const RETRY_DELAYS_MS = [1000, 3000]; +const DOWNLOAD_ATTEMPTS = 5; +// A freshly uploaded GitHub attachment can 404 for up to a few minutes before +// its storage read-path catches up with the write — observed up to ~3 minutes +// in production, with no edit to the PR in between. The deadline and delays +// below give a 404 on an attachment host room to clear before this gives up. +const DOWNLOAD_DEADLINE_MS = 180_000; +const RETRY_DELAYS_MS = [5_000, 15_000, 30_000, 60_000]; const MAX_REDIRECTS = 5; const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); const MAX_ASSET_BYTES = 100 * 1024 * 1024; @@ -266,7 +270,22 @@ async function readCapped(response, budget) { return collector.bytes(); } -const isRetryableStatus = (status) => status === 429 || status >= 500; +const isServerRetryable = (status) => status === 429 || status >= 500; + +// A 404 is final for almost anything — but for a GitHub attachment URL, right +// after it was uploaded, it means "not replicated yet," not "does not exist." +// Reproduced this week: the same asset URL 404'd, then 200'd minutes later +// with no edit to the PR in between, and whichever asset had been attached +// most recently was always the one that failed. Scoped to the attachment host +// so a real 404 on any other URL (a typo'd link, a deleted gist) still fails fast. +const isRetryableAttachment404 = (status, url) => { + if (status !== 404) return false; + const parsed = parseUrl(url); + return parsed !== null && isAttachmentUrl(parsed); +}; + +const isRetryableStatus = (status, url) => + isServerRetryable(status) || isRetryableAttachment404(status, url); const pause = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); /** One attempt: the bytes, or a NonRetryable for a final status, or a plain Error for a retryable one. */ @@ -274,7 +293,7 @@ async function attemptDownload(url, fetchImpl, signal, budget) { const response = await fetchTrusted(url, fetchImpl, signal); if (response.ok) return readCapped(response, budget); const message = `HTTP ${response.status}`; - throw isRetryableStatus(response.status) ? new Error(message) : new NonRetryable(message); + throw isRetryableStatus(response.status, url) ? new Error(message) : new NonRetryable(message); } async function waitForRetry(attempt, sleep, signal) { diff --git a/scripts/check-pr-captures.test.mjs b/scripts/check-pr-captures.test.mjs index 70d58450f1..37b564e6a4 100644 --- a/scripts/check-pr-captures.test.mjs +++ b/scripts/check-pr-captures.test.mjs @@ -309,7 +309,7 @@ const ok = () => ({ }); const noSleep = async () => {}; -test("downloadAsset retries a 5xx or a network error with backoff and stops after three attempts", async () => { +test("downloadAsset retries a 5xx or a network error with backoff and stops after five attempts", async () => { const outcomes = [{ ok: false, status: 502 }, new Error("socket hang up"), ok()]; let calls = 0; const sleeps = []; @@ -320,24 +320,50 @@ test("downloadAsset retries a 5xx or a network error with backoff and stops afte }; assert.equal((await downloadAsset(OLD, flaky, async (ms) => sleeps.push(ms))).toString(), "x"); assert.equal(calls, 3); - assert.deepEqual(sleeps, [1000, 3000]); + assert.deepEqual(sleeps, [5000, 15000]); calls = 0; await assert.rejects( downloadAsset(OLD, async () => (calls++, { ok: false, status: 500 }), noSleep), /HTTP 500/, ); - assert.equal(calls, 3); + assert.equal(calls, 5); }); -test("downloadAsset does not retry a 404 or 403", async () => { - for (const status of [404, 403]) { - let calls = 0; - await assert.rejects( - downloadAsset(OLD, async () => (calls++, { ok: false, status }), noSleep), - new RegExp(`HTTP ${status}`), - ); - assert.equal(calls, 1); - } +test("downloadAsset does not retry a 403, even on an attachment host", async () => { + let calls = 0; + await assert.rejects( + downloadAsset(OLD, async () => (calls++, { ok: false, status: 403 }), noSleep), + /HTTP 403/, + ); + assert.equal(calls, 1); +}); + +// Reproduced this week: the same attachment URL 404'd, then 200'd minutes +// later with no edit to the PR in between — a fresh upload lagging GitHub's +// own read-path, not a missing asset. +test("downloadAsset retries a 404 on a GitHub attachment host", async () => { + const outcomes = [{ ok: false, status: 404 }, { ok: false, status: 404 }, ok()]; + let calls = 0; + const sleeps = []; + await downloadAsset( + OLD, + async () => outcomes[calls++], + async (ms) => sleeps.push(ms), + ); + assert.equal(calls, 3); + assert.deepEqual(sleeps, [5000, 15000]); +}); + +// The retry is scoped to the attachment host: a 404 anywhere else (a typo'd +// link, a deleted gist) still means "does not exist," not "not replicated yet." +test("downloadAsset does not retry a 404 on a non-attachment host", async () => { + const other = "https://example.com/not-an-attachment.png"; + let calls = 0; + await assert.rejects( + downloadAsset(other, async () => (calls++, { ok: false, status: 404 }), noSleep), + /HTTP 404/, + ); + assert.equal(calls, 1); }); const redirect = (location) => ({ ok: false, status: 302, headers: new Headers({ location }) }); @@ -523,7 +549,18 @@ function runCli(env, body = bodyWith(`[a](${OLD})`, `[b](${NEW})`)) { const copy = join(dir, "check.mjs"); copyFileSync(new URL("./check-pr-captures.mjs", import.meta.url), copy); const preload = join(dir, "no-network.mjs"); - writeFileSync(preload, "globalThis.fetch = async () => ({ ok: false, status: 404 });\n"); + writeFileSync( + preload, + [ + "globalThis.fetch = async () => ({ ok: false, status: 404 });", + // The gate's retry backoff (up to 110s per asset) is real production + // behavior we want covered end to end, but a test shouldn't sit through + // it: collapse every delay to fire on the next tick. + "const realSetTimeout = globalThis.setTimeout;", + "globalThis.setTimeout = (fn, _ms, ...args) => realSetTimeout(fn, 0, ...args);", + "", + ].join("\n"), + ); return spawnSync("node", ["--import", preload, copy, "--base", "main", "--head", "HEAD"], { cwd: dir, encoding: "utf8",