From bae2879e7469a75c61deeb48e90aa31c8cb65c63 Mon Sep 17 00:00:00 2001 From: Sinduri Guntupalli Date: Tue, 8 Sep 2026 08:36:14 +0200 Subject: [PATCH] fix(sync): retry transient 5xx errors in fetchWithRetry Fixes #245. - Extend fetchWithRetry to retry HTTP 500, 502, 503, 504 with exponential backoff (1s, 2s, 4s, capped at 30s) - Previously only 429 (rate-limit) was retried; a momentary Discourse 500 or 502 counted as a hard failure and exceeded MAX_TOLERATED_FAILURES=1 - Update pagination tests in refresh-discussions to use fake timers so 5xx chunk failures exhaust retries without real-time waits - Add five new fetchWithRetry unit tests covering the 5xx retry path Signed-off-by: Sinduri Guntupalli --- scripts/discourse-utils.mjs | 29 ++++++--- src/test/scripts/discourse-utils.test.ts | 63 +++++++++++++++++++- src/test/scripts/refresh-discussions.test.ts | 16 ++++- 3 files changed, 97 insertions(+), 11 deletions(-) diff --git a/scripts/discourse-utils.mjs b/scripts/discourse-utils.mjs index aad09f4dd..ecb194c50 100644 --- a/scripts/discourse-utils.mjs +++ b/scripts/discourse-utils.mjs @@ -50,19 +50,34 @@ export function atomicWrite(path, content) { renameSync(tmp, path); } +// Transient server errors worth retrying. 500/502/503/504 are gateway or +// momentary upstream failures; 429 is handled separately via Retry-After. +const RETRYABLE_5XX = new Set([500, 502, 503, 504]); + /** - * Fetch wrapper that retries on HTTP 429 (rate-limited) responses. - * Reads the `Retry-After` response header; falls back to 60 s when absent. - * Caps the wait at 120 s to avoid stalling CI runs indefinitely. + * Fetch wrapper that retries on HTTP 429 (rate-limited) and transient 5xx + * responses (500, 502, 503, 504). + * + * - 429: reads the `Retry-After` header; falls back to 60 s, capped at 120 s. + * - 5xx: exponential backoff (2^attempt seconds), capped at 30 s. + * * Returns the final Response — caller inspects `res.ok` / `res.status`. */ export async function fetchWithRetry(url, options = {}, maxRetries = 3) { for (let attempt = 0; attempt <= maxRetries; attempt++) { const res = await fetch(url, options); - if (res.status !== 429 || attempt === maxRetries) return res; - const header = res.headers.get("Retry-After"); - const seconds = Math.min(parseInt(header ?? "60", 10) || 60, 120); - console.warn(` Rate-limited (429). Waiting ${seconds}s before retry ${attempt + 1}/${maxRetries}…`); + const is429 = res.status === 429; + const is5xx = RETRYABLE_5XX.has(res.status); + if ((!is429 && !is5xx) || attempt === maxRetries) return res; + let seconds; + if (is429) { + const header = res.headers.get("Retry-After"); + seconds = Math.min(parseInt(header ?? "60", 10) || 60, 120); + console.warn(` Rate-limited (429). Waiting ${seconds}s before retry ${attempt + 1}/${maxRetries}…`); + } else { + seconds = Math.min(2 ** attempt, 30); + console.warn(` Server error (${res.status}). Waiting ${seconds}s before retry ${attempt + 1}/${maxRetries}…`); + } await new Promise((r) => setTimeout(r, seconds * 1000)); } return fetch(url, options); // unreachable; satisfies static analysis diff --git a/src/test/scripts/discourse-utils.test.ts b/src/test/scripts/discourse-utils.test.ts index 9016574d1..37addcd8f 100644 --- a/src/test/scripts/discourse-utils.test.ts +++ b/src/test/scripts/discourse-utils.test.ts @@ -102,7 +102,7 @@ describe("fetchWithRetry", () => { expect(fetch).toHaveBeenCalledTimes(1); }); - it("returns non-200 non-429 responses without retrying", async () => { + it("returns 4xx responses without retrying", async () => { const res = new Response("not found", { status: 404 }); vi.mocked(fetch).mockResolvedValue(res); const result = await fetchWithRetry("https://example.com"); @@ -110,6 +110,67 @@ describe("fetchWithRetry", () => { expect(fetch).toHaveBeenCalledTimes(1); }); + it("retries on 500 and returns success when retry succeeds", async () => { + vi.useFakeTimers(); + const err500 = new Response("server error", { status: 500 }); + const ok200 = new Response("ok", { status: 200 }); + vi.mocked(fetch) + .mockResolvedValueOnce(err500) + .mockResolvedValueOnce(ok200); + + const promise = fetchWithRetry("https://example.com", {}, 3); + await vi.runAllTimersAsync(); + const result = await promise; + expect(result.status).toBe(200); + expect(fetch).toHaveBeenCalledTimes(2); + }); + + it("retries on 502 and returns success when retry succeeds", async () => { + vi.useFakeTimers(); + const err502 = new Response("bad gateway", { status: 502 }); + const ok200 = new Response("ok", { status: 200 }); + vi.mocked(fetch) + .mockResolvedValueOnce(err502) + .mockResolvedValueOnce(ok200); + + const promise = fetchWithRetry("https://example.com", {}, 3); + await vi.runAllTimersAsync(); + const result = await promise; + expect(result.status).toBe(200); + expect(fetch).toHaveBeenCalledTimes(2); + }); + + it("returns the final 5xx after exhausting retries", async () => { + vi.useFakeTimers(); + const err502 = new Response("bad gateway", { status: 502 }); + vi.mocked(fetch).mockResolvedValue(err502); + + const promise = fetchWithRetry("https://example.com", {}, 2); + await vi.runAllTimersAsync(); + const result = await promise; + expect(result.status).toBe(502); + // maxRetries=2 means 3 total calls: attempt 0, 1, 2 + expect(fetch).toHaveBeenCalledTimes(3); + }); + + it("uses exponential backoff for 5xx retries", async () => { + vi.useFakeTimers(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const err500 = new Response("server error", { status: 500 }); + const ok200 = new Response("ok", { status: 200 }); + vi.mocked(fetch) + .mockResolvedValueOnce(err500) + .mockResolvedValueOnce(ok200); + + const promise = fetchWithRetry("https://example.com", {}, 3); + await vi.runAllTimersAsync(); + await promise; + // First 5xx retry: attempt=0, backoff = 2^0 = 1s + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Server error (500)")); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("1s")); + warnSpy.mockRestore(); + }); + it("retries on 429 and returns success when retry succeeds", async () => { vi.useFakeTimers(); const rate429 = new Response("rate limited", { diff --git a/src/test/scripts/refresh-discussions.test.ts b/src/test/scripts/refresh-discussions.test.ts index d2d295a41..8e6a06f05 100644 --- a/src/test/scripts/refresh-discussions.test.ts +++ b/src/test/scripts/refresh-discussions.test.ts @@ -309,7 +309,11 @@ describe("fetchTopicPosts pagination", () => { // also tear down the localStorage stub that src/test/setup.ts installs for the // whole suite via vi.stubGlobal, breaking its own beforeEach. const realFetch = globalThis.fetch; + beforeEach(() => { + vi.useFakeTimers(); + }); afterEach(() => { + vi.useRealTimers(); vi.stubGlobal("fetch", realFetch); }); @@ -329,7 +333,9 @@ describe("fetchTopicPosts pagination", () => { it("fails the topic when a chunk returns a non-ok status", async () => { stubDiscourse(45, () => ({ ok: false, status: 500, json: async () => ({}) })); - const result = await fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + const promise = fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + await vi.runAllTimersAsync(); + const result = await promise; expect(result.ok).toBe(false); expect(result.reason).toContain("incomplete post list"); expect(result.reason).toContain("HTTP 500"); @@ -360,7 +366,9 @@ describe("fetchTopicPosts pagination", () => { : { ok: false, status: 502, json: async () => ({}) }, ); - const result = await fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + const promise = fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + await vi.runAllTimersAsync(); + const result = await promise; expect(result.ok).toBe(false); expect(result.posts).toBeUndefined(); }); @@ -368,7 +376,9 @@ describe("fetchTopicPosts pagination", () => { it("names the missing post range so the gap is identifiable", async () => { stubDiscourse(45, () => ({ ok: false, status: 500, json: async () => ({}) })); - const result = await fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + const promise = fetchTopicPosts("1", "https://community.offon.dev/t/a/1"); + await vi.runAllTimersAsync(); + const result = await promise; expect(result.reason).toMatch(/posts 21…40/); });