diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0ee32760..cc8510de 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,6 +52,8 @@ User-visible changes also need a changelog fragment under `changelog.d/` offline, set `DOCS_SYNC_OPTIONAL=1` (for example `DOCS_SYNC_OPTIONAL=1 npm run build`) to keep the committed content instead. CI and Netlify always use the strict behavior. +Sync fetches time out after 30s by default; set `DOCS_SYNC_FETCH_TIMEOUT_MS` +(milliseconds) to change that. ## Code review requirements diff --git a/changelog.d/fixed-sync-fetch-timeout.md b/changelog.d/fixed-sync-fetch-timeout.md new file mode 100644 index 00000000..09967564 --- /dev/null +++ b/changelog.d/fixed-sync-fetch-timeout.md @@ -0,0 +1 @@ +- Prebuild content sync steps now fail fast with a clear "timed out" error instead of hanging the build when a remote host stalls; tune with `DOCS_SYNC_FETCH_TIMEOUT_MS` (default 30000). diff --git a/scripts/fetch-with-timeout.test.ts b/scripts/fetch-with-timeout.test.ts new file mode 100644 index 00000000..1b5ae19a --- /dev/null +++ b/scripts/fetch-with-timeout.test.ts @@ -0,0 +1,144 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + fetchBytesWithTimeout, + fetchTextWithTimeout, + parseTimeoutMs, +} from "./fetch-with-timeout"; + +// A fetch that never resolves but rejects when its signal aborts. +const hanging = vi.fn( + (_url: string, init?: RequestInit) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new DOMException("aborted", "AbortError")) + ); + }) +) as unknown as typeof fetch; + +// A fetch that answers 200 with headers right away but whose body stream +// never closes and ignores the abort signal (the "never completes the +// response" case from #394). +const stalledBody = vi.fn( + async () => + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("partial")); + }, + }), + { status: 200 } + ) +) as unknown as typeof fetch; + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("fetchTextWithTimeout", () => { + it("returns status fields and the body text", async () => { + const fake = vi.fn( + async () => new Response("ok", { status: 200, statusText: "OK" }) + ) as unknown as typeof fetch; + await expect( + fetchTextWithTimeout("https://x.test/a", {}, 1000, fake) + ).resolves.toEqual({ ok: true, status: 200, statusText: "OK", body: "ok" }); + }); + + it("reports non-2xx responses without throwing", async () => { + const fake = vi.fn( + async () => + new Response("missing", { status: 404, statusText: "Not Found" }) + ) as unknown as typeof fetch; + const res = await fetchTextWithTimeout("https://x.test/a", {}, 1000, fake); + expect(res.ok).toBe(false); + expect(res.status).toBe(404); + expect(res.statusText).toBe("Not Found"); + }); + + it("passes an AbortSignal to fetch", async () => { + const fake = vi.fn(async () => new Response("ok")); + await fetchTextWithTimeout( + "https://x.test/a", + {}, + 1000, + fake as unknown as typeof fetch + ); + const init = (fake.mock.calls[0] as unknown[])[1] as RequestInit; + expect(init.signal).toBeInstanceOf(AbortSignal); + }); + + it("rejects with a timeout message naming the URL when headers never arrive", async () => { + vi.useFakeTimers(); + const p = fetchTextWithTimeout("https://x.test/slow", {}, 5000, hanging); + const assertion = expect(p).rejects.toThrow( + /timed out after 5s fetching https:\/\/x\.test\/slow/ + ); + await vi.advanceTimersByTimeAsync(5000); + await assertion; + }); + + it("times out when the body stalls after headers were received", async () => { + vi.useFakeTimers(); + const p = fetchTextWithTimeout( + "https://x.test/stall", + {}, + 5000, + stalledBody + ); + const assertion = expect(p).rejects.toThrow( + /timed out after 5s fetching https:\/\/x\.test\/stall/ + ); + await vi.advanceTimersByTimeAsync(5000); + await assertion; + }); + + it("does not mask non-timeout errors", async () => { + const fake = vi.fn(async () => { + throw new Error("boom"); + }) as unknown as typeof fetch; + await expect( + fetchTextWithTimeout("https://x.test/a", {}, 1000, fake) + ).rejects.toThrow("boom"); + }); +}); + +describe("fetchBytesWithTimeout", () => { + it("returns the body as a Buffer", async () => { + const fake = vi.fn( + async () => new Response(new Uint8Array([1, 2, 3])) + ) as unknown as typeof fetch; + const res = await fetchBytesWithTimeout("https://x.test/b", {}, 1000, fake); + expect(res.ok).toBe(true); + expect(Buffer.isBuffer(res.body)).toBe(true); + expect([...res.body]).toEqual([1, 2, 3]); + }); + + it("times out when the body stalls after headers were received", async () => { + vi.useFakeTimers(); + const p = fetchBytesWithTimeout( + "https://x.test/stall", + {}, + 5000, + stalledBody + ); + const assertion = expect(p).rejects.toThrow( + /timed out after 5s fetching https:\/\/x\.test\/stall/ + ); + await vi.advanceTimersByTimeAsync(5000); + await assertion; + }); +}); + +describe("parseTimeoutMs", () => { + it("falls back to the default for missing or invalid values", () => { + expect(parseTimeoutMs({})).toBe(30000); + expect(parseTimeoutMs({ DOCS_SYNC_FETCH_TIMEOUT_MS: "abc" })).toBe(30000); + expect(parseTimeoutMs({ DOCS_SYNC_FETCH_TIMEOUT_MS: "0" })).toBe(30000); + expect(parseTimeoutMs({ DOCS_SYNC_FETCH_TIMEOUT_MS: "-5" })).toBe(30000); + expect(parseTimeoutMs({ DOCS_SYNC_FETCH_TIMEOUT_MS: "1.5" })).toBe(30000); + }); + + it("accepts a positive integer", () => { + expect(parseTimeoutMs({ DOCS_SYNC_FETCH_TIMEOUT_MS: "1500" })).toBe(1500); + }); +}); diff --git a/scripts/fetch-with-timeout.ts b/scripts/fetch-with-timeout.ts new file mode 100644 index 00000000..88395f40 --- /dev/null +++ b/scripts/fetch-with-timeout.ts @@ -0,0 +1,93 @@ +/** + * fetch() with a deadline for the prebuild sync scripts, so a stalled remote + * fails the step quickly instead of hanging the build until the job timeout. + * The deadline covers the whole exchange, including reading the body, so a + * remote that sends headers and then stalls mid-body is bounded too. + * Override the default with DOCS_SYNC_FETCH_TIMEOUT_MS (positive integer, ms). + */ +import type { SyncEnv } from "./run-sync-step"; + +const FALLBACK_TIMEOUT_MS = 30000; + +export function parseTimeoutMs(env: SyncEnv = process.env): number { + const raw = env.DOCS_SYNC_FETCH_TIMEOUT_MS?.trim(); + if (!raw || !/^\d+$/.test(raw)) return FALLBACK_TIMEOUT_MS; + const value = Number(raw); + return Number.isSafeInteger(value) && value > 0 ? value : FALLBACK_TIMEOUT_MS; +} + +export const DEFAULT_SYNC_FETCH_TIMEOUT_MS = parseTimeoutMs(); + +export interface TimedResponse { + ok: boolean; + status: number; + statusText: string; + body: T; +} + +async function fetchWithDeadline( + url: string, + init: RequestInit, + timeoutMs: number, + fetchImpl: typeof fetch, + read: (response: Response) => Promise +): Promise> { + const controller = new AbortController(); + let timedOut = false; + let expire: () => void = () => {}; + // Raced against both the fetch and the body read: the body stream of a + // fetch stub may ignore the abort signal, and the deadline must hold anyway. + const deadline = new Promise((_resolve, reject) => { + expire = () => reject(controller.signal.reason); + }); + deadline.catch(() => {}); + const timer = setTimeout(() => { + timedOut = true; + controller.abort(); + expire(); + }, timeoutMs); + let response: Response | undefined; + try { + response = await Promise.race([ + fetchImpl(url, { ...init, signal: controller.signal }), + deadline, + ]); + const body = await Promise.race([read(response), deadline]); + return { + ok: response.ok, + status: response.status, + statusText: response.statusText, + body, + }; + } catch (error) { + if (timedOut) { + response?.body?.cancel().catch(() => {}); + throw new Error(`timed out after ${timeoutMs / 1000}s fetching ${url}`, { + cause: error, + }); + } + throw error; + } finally { + clearTimeout(timer); + } +} + +export function fetchTextWithTimeout( + url: string, + init: RequestInit = {}, + timeoutMs: number = DEFAULT_SYNC_FETCH_TIMEOUT_MS, + fetchImpl: typeof fetch = fetch +): Promise> { + return fetchWithDeadline(url, init, timeoutMs, fetchImpl, r => r.text()); +} + +export function fetchBytesWithTimeout( + url: string, + init: RequestInit = {}, + timeoutMs: number = DEFAULT_SYNC_FETCH_TIMEOUT_MS, + fetchImpl: typeof fetch = fetch +): Promise> { + return fetchWithDeadline(url, init, timeoutMs, fetchImpl, async r => + Buffer.from(await r.arrayBuffer()) + ); +} diff --git a/scripts/sync-hive-docs.ts b/scripts/sync-hive-docs.ts index dc508b0e..ee582fbf 100644 --- a/scripts/sync-hive-docs.ts +++ b/scripts/sync-hive-docs.ts @@ -4,6 +4,7 @@ import { rewriteLinkTarget as rewriteSharedLinkTarget, rewriteMarkdownLinks, } from "./link-rewrite"; +import { fetchTextWithTimeout } from "./fetch-with-timeout"; import { normalizeFences } from "./markdown-fences"; import { scrubLegacyBranding } from "./scrub-legacy-branding"; @@ -217,13 +218,13 @@ function applyLocalHiveOverlays(content: string, source: string): string { } async function fetchText(url: string): Promise { - const response = await fetch(url); + const response = await fetchTextWithTimeout(url); if (!response.ok) { throw new Error( `failed to fetch ${url}: ${response.status} ${response.statusText}` ); } - return response.text(); + return response.body; } /** Resolve a sync target under docsRoot, refusing anything that escapes it. */ diff --git a/scripts/sync-sibling-docs.ts b/scripts/sync-sibling-docs.ts index d36e95b3..5dacc22a 100644 --- a/scripts/sync-sibling-docs.ts +++ b/scripts/sync-sibling-docs.ts @@ -15,6 +15,10 @@ */ import fs from "fs"; import path from "path"; +import { + fetchBytesWithTimeout, + fetchTextWithTimeout, +} from "./fetch-with-timeout"; import { rewriteMarkdownLinks } from "./link-rewrite"; import { normalizeFences } from "./markdown-fences"; import { convertMdxToMarkdown } from "./mdx-to-markdown"; @@ -441,15 +445,15 @@ function rawUrl(r: RepoRef, source: string): string { } async function fetchFile(r: RepoRef, source: string): Promise { - const res = await fetch(rawUrl(r, source)); + const res = await fetchTextWithTimeout(rawUrl(r, source)); if (!res.ok) return null; - return res.text(); + return res.body; } async function fetchBinary(r: RepoRef, source: string): Promise { - const res = await fetch(rawUrl(r, source)); + const res = await fetchBytesWithTimeout(rawUrl(r, source)); if (!res.ok) return null; - return Buffer.from(await res.arrayBuffer()); + return res.body; } /** diff --git a/scripts/update-meeting-recordings.inprocess.test.ts b/scripts/update-meeting-recordings.inprocess.test.ts index 412ec642..d4fc8242 100644 --- a/scripts/update-meeting-recordings.inprocess.test.ts +++ b/scripts/update-meeting-recordings.inprocess.test.ts @@ -115,7 +115,10 @@ describe("update-meeting-recordings (in-process)", () => { await runInProcess(); - expect(fetch).toHaveBeenCalledWith(FEED_URL); + expect(fetch).toHaveBeenCalledWith( + FEED_URL, + expect.objectContaining({ signal: expect.any(AbortSignal) }) + ); expect(logSpy).toHaveBeenCalledWith("meeting recordings: 2"); const data = readData(); expect(data.source).toBe(FEED_URL); diff --git a/scripts/update-meeting-recordings.ts b/scripts/update-meeting-recordings.ts index 889a4aeb..8ec2db8b 100644 --- a/scripts/update-meeting-recordings.ts +++ b/scripts/update-meeting-recordings.ts @@ -1,5 +1,6 @@ import fs from "fs"; import path from "path"; +import { fetchTextWithTimeout } from "./fetch-with-timeout"; const DATA_PATH = path.join(process.cwd(), "data", "meeting-recordings.json"); const FEED_URL = @@ -123,10 +124,10 @@ async function main() { let fetched: Recording[] = []; try { - const response = await fetch(FEED_URL); + const response = await fetchTextWithTimeout(FEED_URL); if (!response.ok) throw new Error(`YouTube feed returned ${response.status}`); - fetched = parseFeed(await response.text()); + fetched = parseFeed(response.body); } catch (error) { console.warn( `meeting recordings feed unavailable; keeping committed data: ${error instanceof Error ? error.message : String(error)}`