diff --git a/packages/gittensory-miner/lib/ci-poller.js b/packages/gittensory-miner/lib/ci-poller.js index df0f54c84e..b37fbb41da 100644 --- a/packages/gittensory-miner/lib/ci-poller.js +++ b/packages/gittensory-miner/lib/ci-poller.js @@ -1,3 +1,5 @@ +import { fetchWithRetry } from "./http-retry.js"; + const defaultApiBaseUrl = "https://api.github.com"; const defaultMinIntervalMs = 60_000; const defaultMaxIntervalMs = 5 * 60_000; @@ -82,10 +84,14 @@ function githubError(response, payload) { } async function githubGetJsonResponse(url, options) { - const response = await options.fetchFn(url, { - method: "GET", - headers: githubHeaders(options.githubToken), - }); + // Retry transient network errors / 5xx around this single call (#4829), distinct from the poller's own + // pending-retry loop; the poller's injected sleepFn keeps tests instant. + const response = await fetchWithRetry( + options.fetchFn, + url, + { method: "GET", headers: githubHeaders(options.githubToken) }, + { sleepFn: options.sleepFn }, + ); const payload = await response.json().catch(() => null); if (!response.ok) { throw githubError(response, payload); diff --git a/packages/gittensory-miner/lib/gate-verdict-poller.js b/packages/gittensory-miner/lib/gate-verdict-poller.js index c31eb8a769..ac10364142 100644 --- a/packages/gittensory-miner/lib/gate-verdict-poller.js +++ b/packages/gittensory-miner/lib/gate-verdict-poller.js @@ -10,6 +10,7 @@ // gate verdict are two different signals a caller can record independently. // // Fully testable via injected `fetchFn`/`sleepFn` (mirrors `ci-poller.js`) — no real network in tests. +import { fetchWithRetry } from "./http-retry.js"; /** The typed gate verdicts, decided ones first, `pending` (not-yet-decided) last. */ export const GATE_VERDICTS = Object.freeze(["merge", "close", "hold", "pending"]); @@ -86,7 +87,8 @@ export async function pollGateVerdict(url, options = {}) { let latest = { verdict: "pending", disposition: null, attempts: 0, body: null }; for (let attempt = 0; attempt < maxAttempts; attempt += 1) { - const response = await fetchFn(url, { headers }); + // Retry transient network errors / 5xx around this single call (#4829), distinct from the pending-retry loop. + const response = await fetchWithRetry(fetchFn, url, { headers }, { sleepFn }); if (!response || !response.ok) throw new Error(`gate_verdict_http_${response ? response.status : "error"}`); const body = await response.json(); const disposition = readGateDisposition(body); diff --git a/packages/gittensory-miner/lib/http-retry.d.ts b/packages/gittensory-miner/lib/http-retry.d.ts new file mode 100644 index 0000000000..696e1ebdd5 --- /dev/null +++ b/packages/gittensory-miner/lib/http-retry.d.ts @@ -0,0 +1,12 @@ +export function defaultRetryBackoffMs(attempt: number): number; + +export function fetchWithRetry( + fetchFn: (url: unknown, init?: unknown) => Promise, + url: unknown, + init?: unknown, + options?: { + maxAttempts?: number; + sleepFn?: (ms: number) => Promise; + backoffMs?: (attempt: number) => number; + }, +): Promise; diff --git a/packages/gittensory-miner/lib/http-retry.js b/packages/gittensory-miner/lib/http-retry.js new file mode 100644 index 0000000000..86470966ad --- /dev/null +++ b/packages/gittensory-miner/lib/http-retry.js @@ -0,0 +1,52 @@ +// Bounded retry-with-backoff around a single HTTP call (#4829). The miner's pollers (ci-poller, gate-verdict- +// poller) previously let a single brief 5xx from GitHub kill the whole poll loop, because their own attempt loop +// only re-polls while a conclusion is genuinely "pending", never after a server error. This wraps ONE fetch so a +// transient SERVER error (a 5xx RESPONSE) is retried a bounded number of times, DISTINCT from that pending- +// polling, sleeping an exponential backoff between attempts and giving up after `maxAttempts`. A 2xx/3xx/4xx +// response is returned immediately, and a THROWN error (a network-level failure) propagates unchanged rather than +// being retried — the pollers' existing failure-mode contract (#4281) deliberately bubbles those to the caller. +// Pure control flow over injected `fetchFn`/`sleepFn`/`backoffMs` — no real network or timers in tests. + +const DEFAULT_MAX_ATTEMPTS = 3; +const DEFAULT_BASE_BACKOFF_MS = 500; +const MAX_BACKOFF_MS = 10_000; + +/** Clamp `maxAttempts` to a positive integer, flooring BEFORE the positivity test so a fractional value below 1 + * falls back to the default rather than becoming a 0 that would skip every attempt. */ +function normalizeMaxAttempts(raw) { + const numeric = Math.floor(Number(raw)); + return Number.isFinite(numeric) && numeric >= 1 ? numeric : DEFAULT_MAX_ATTEMPTS; +} + +/** Exponential backoff from a base delay, capped: attempt 1 → base, 2 → 2×base, 3 → 4×base, … ≤ MAX_BACKOFF_MS. */ +export function defaultRetryBackoffMs(attempt) { + return Math.min(MAX_BACKOFF_MS, DEFAULT_BASE_BACKOFF_MS * 2 ** (Math.max(1, attempt) - 1)); +} + +const defaultSleep = (delayMs) => new Promise((resolve) => setTimeout(resolve, delayMs)); + +/** + * Perform `fetchFn(url, init)` with bounded retry on a transient 5xx response. A 5xx is retried (sleeping + * `backoffMs(attempt)` between attempts) up to `maxAttempts`; a 2xx/3xx/4xx response is returned immediately, and + * after the last attempt a lingering 5xx is returned as-is (the caller's own error handling still runs). A THROWN + * error is NOT retried — it propagates to the caller (the pollers' #4281 failure-mode contract). + * + * @param {(url: any, init?: any) => Promise} fetchFn + * @param {any} url + * @param {any} [init] + * @param {{ maxAttempts?: number, sleepFn?: (ms: number) => Promise, backoffMs?: (attempt: number) => number }} [options] + * @returns {Promise} the fetch response + */ +export async function fetchWithRetry(fetchFn, url, init, options = {}) { + const maxAttempts = normalizeMaxAttempts(options.maxAttempts); + const sleepFn = typeof options.sleepFn === "function" ? options.sleepFn : defaultSleep; + const backoffMs = typeof options.backoffMs === "function" ? options.backoffMs : defaultRetryBackoffMs; + for (let attempt = 1; ; attempt += 1) { + // A thrown error is intentionally NOT caught here — it propagates to the caller unchanged. + const response = await fetchFn(url, init); + // Retry only transient SERVER errors (5xx). Return 2xx/3xx/4xx immediately; on the final attempt a lingering + // 5xx is returned as-is. + if (response.status < 500 || attempt >= maxAttempts) return response; + await sleepFn(backoffMs(attempt)); + } +} diff --git a/test/unit/miner-ci-poller.test.ts b/test/unit/miner-ci-poller.test.ts index 0831a4c46e..f15967e78d 100644 --- a/test/unit/miner-ci-poller.test.ts +++ b/test/unit/miner-ci-poller.test.ts @@ -68,6 +68,28 @@ describe("miner CI check-run poller (#2323)", () => { ).toBe(true); }); + it("retries a transient 5xx from GitHub during the poll and completes (#4829)", async () => { + let checkRunsAttempts = 0; + const fetchFn = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.endsWith("/repos/acme/widgets/pulls/42")) return prResponse("head-sha"); + if (url.includes("/check-runs")) { + checkRunsAttempts += 1; + if (checkRunsAttempts === 1) return jsonResponse({}, { status: 503 }); // a brief transient server error + return checksResponse([checkRun("validate", "completed", "success")]); + } + return jsonResponse({}, { status: 404 }); + }); + const result = await pollCheckRuns("acme/widgets", 42, { + apiBaseUrl: API, + githubToken: "github-token", + fetchFn, + sleepFn: () => Promise.resolve(), // no real backoff delay in the test + }); + expect(checkRunsAttempts).toBe(2); // the 503 was retried, then succeeded + expect(result.conclusion).toBe("success"); // the poll completed despite the transient 5xx + }); + it("rejects untrusted apiBaseUrl values before any token-bearing request", async () => { const fetchFn = vi.fn(); for (const apiBaseUrl of [ diff --git a/test/unit/miner-http-retry.test.ts b/test/unit/miner-http-retry.test.ts new file mode 100644 index 0000000000..829250dfda --- /dev/null +++ b/test/unit/miner-http-retry.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it } from "vitest"; +import { defaultRetryBackoffMs, fetchWithRetry } from "../../packages/gittensory-miner/lib/http-retry.js"; + +const noSleep = () => Promise.resolve(); + +/** A fetchFn driven by a scripted list of behaviours: a number ⇒ a response with that status, "throw" ⇒ reject. */ +function scriptedFetch(script: Array) { + let call = 0; + const calls: unknown[] = []; + const fn = async (url: unknown) => { + calls.push(url); + const step = script[Math.min(call, script.length - 1)] ?? 200; + call += 1; + if (step === "throw") throw new Error("network down"); + return { status: step }; + }; + return { fn, get calls() { return calls; } }; +} + +describe("fetchWithRetry (#4829)", () => { + it("returns a 2xx response on the first attempt with no retry", async () => { + const f = scriptedFetch([200]); + const res = await fetchWithRetry(f.fn, "u", undefined, { sleepFn: noSleep }); + expect(res.status).toBe(200); + expect(f.calls).toHaveLength(1); + }); + + it("returns a 4xx immediately without retrying (client errors are not transient)", async () => { + const f = scriptedFetch([404, 200]); + const res = await fetchWithRetry(f.fn, "u", undefined, { sleepFn: noSleep }); + expect(res.status).toBe(404); + expect(f.calls).toHaveLength(1); + }); + + it("retries a transient 5xx and returns the eventual success", async () => { + const f = scriptedFetch([503, 500, 200]); + const res = await fetchWithRetry(f.fn, "u", undefined, { maxAttempts: 5, sleepFn: noSleep }); + expect(res.status).toBe(200); + expect(f.calls).toHaveLength(3); + }); + + it("does NOT retry a thrown network error — it propagates immediately (poller #4281 contract)", async () => { + const f = scriptedFetch(["throw", 200]); + await expect(fetchWithRetry(f.fn, "u", undefined, { sleepFn: noSleep })).rejects.toThrow("network down"); + expect(f.calls).toHaveLength(1); // no retry on a thrown error + }); + + it("returns the last 5xx response once attempts are exhausted (caller handles it)", async () => { + const f = scriptedFetch([500]); + const res = await fetchWithRetry(f.fn, "u", undefined, { maxAttempts: 3, sleepFn: noSleep }); + expect(res.status).toBe(500); + expect(f.calls).toHaveLength(3); + }); + + it("makes exactly one attempt when maxAttempts is 1 (returns the 5xx, no retry)", async () => { + const f = scriptedFetch([500, 200]); + const res = await fetchWithRetry(f.fn, "u", undefined, { maxAttempts: 1, sleepFn: noSleep }); + expect(res.status).toBe(500); + expect(f.calls).toHaveLength(1); + }); + + it("uses the built-in sleep + backoff when none are injected (0ms backoff keeps the test fast)", async () => { + // No sleepFn and no backoffMs ⇒ exercises the default sleep (real setTimeout) with a 0ms delay, via a 5xx retry. + const f = scriptedFetch([500, 200]); + const res = await fetchWithRetry(f.fn, "u", undefined, { maxAttempts: 2, backoffMs: () => 0 }); + expect(res.status).toBe(200); + expect(f.calls).toHaveLength(2); + }); + + it("sleeps the backoff between attempts and floors a bad maxAttempts to the default", async () => { + const delays: number[] = []; + const sleepFn = (ms: number) => { + delays.push(ms); + return Promise.resolve(); + }; + const f = scriptedFetch([500]); // always 5xx ⇒ retries until the (default 3) attempts are used up + // maxAttempts "0.5" floors to 0 ⇒ below 1 ⇒ falls back to the default of 3, so 2 sleeps happen. + await fetchWithRetry(f.fn, "u", undefined, { maxAttempts: 0.5 as unknown as number, sleepFn, backoffMs: (a) => a * 10 }); + expect(f.calls).toHaveLength(3); + expect(delays).toEqual([10, 20]); + }); +}); + +describe("defaultRetryBackoffMs (#4829)", () => { + it("grows exponentially from the base and caps", () => { + expect(defaultRetryBackoffMs(1)).toBe(500); + expect(defaultRetryBackoffMs(2)).toBe(1000); + expect(defaultRetryBackoffMs(3)).toBe(2000); + expect(defaultRetryBackoffMs(100)).toBe(10_000); // capped + expect(defaultRetryBackoffMs(0)).toBe(500); // attempt clamped to >= 1 + }); +});