mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-10-30 09:16:55 +00:00
fix(scripts): retry a fresh GitHub attachment's transient 404 in the captures gate (#4362)
* fix(scripts): retry a fresh GitHub attachment's transient 404 in the captures gate Reproduced this week: the same attachment 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. The gate already retries 429/5xx with backoff, but treats every other status as final — so a fresh upload lagging GitHub's own read-path failed the check on attempt 1. 404 is now retryable, scoped to a GitHub attachment host (isAttachmentUrl) so a real 404 elsewhere (a typo'd link, a deleted gist) still fails fast. 403 stays non-retryable everywhere, including on an attachment host — it means something else (auth, a private-repo asset another workflow can't see), not propagation lag. Widened the retry window to match: 5 attempts, delays out to 60s, 3-minute deadline (was 3 attempts / 45s) — the observed lag was up to ~3 minutes. Tests: split the old combined 404-and-403 test; added the attachment-host 404 retry case and the non-attachment-host 404 still-final case; updated the backoff test's attempt/delay assertions for the new constants. 73/73 green, oxlint/oxfmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scripts): stop the captures-gate retry tests from sitting through the real backoff Review on #4362: `runCli()`'s no-network preload stubs `fetch` but not time, so the two CLI tests exercising a genuinely-undownloadable capture now sit through the full 5-attempt/110s backoff per asset (two assets each) — 220s+ per test, pushing `bun run test:scripts` from 15s to 446s and `Test (support)` (timeout-minutes: 10) to 10m0s with zero margin. Preload now also collapses every `setTimeout` delay to the next tick, so the retry logic itself still runs for real (same attempt count, same terminal "could not download" outcome) — it just doesn't wait. 73/73 tests, 1.1s total. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scripts): split isRetryableStatus to clear fallow's CRAP-score gate CI's fallow audit flags `isRetryableStatus` at CRAP 30.0 (cyclomatic 5, threshold 30.0) — pre-existing since the original commit, not something the prior test-timing fix touched. Split into `isServerRetryable` (429/5xx) and `isRetryableAttachment404` (404 scoped to the attachment host), each at cyclomatic 2-3, composed by the still-exported `isRetryableStatus`. Same retry decision, no behavior change. 73/73 tests, oxlint/oxfmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
add6783775
commit
8d8ea810b6
@@ -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) {
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user