diff --git a/review-enrichment/src/external-fetch.ts b/review-enrichment/src/external-fetch.ts index a5c3011e3f..c0a8a46ad6 100644 --- a/review-enrichment/src/external-fetch.ts +++ b/review-enrichment/src/external-fetch.ts @@ -7,7 +7,8 @@ export type BoundedFetchFailureReason = | "http_error" | "response_too_large" | "invalid_json" - | "call_cap"; + | "call_cap" + | "circuit_open"; export interface BoundedFetchOk { ok: true; @@ -52,6 +53,73 @@ export function safeEndpointCategory(category: string): string { return safe || "unknown"; } +// Circuit breaker: once an endpointCategory racks up enough consecutive +// remote-health failures, short-circuit further calls for a cooldown window +// instead of re-attempting a currently-unhealthy endpoint from a cold state. +const CIRCUIT_FAILURE_THRESHOLD = 3; +const CIRCUIT_COOLDOWN_MS = 30_000; +const circuits = new Map(); + +export function resetExternalFetchCircuitBreakerForTest(): void { + circuits.clear(); +} + +function isCircuitOpen(endpointCategory: string): boolean { + const circuit = circuits.get(endpointCategory); + return circuit !== undefined && circuit.cooldownUntil > Date.now(); +} + +function recordCircuitFailure(endpointCategory: string): void { + // Always read the map fresh at call time (never accept a pre-captured + // circuit object or close over a variable read earlier in the caller + // before an await) so concurrent calls for the same endpointCategory + // can't race on a stale read. The read+set here has no await between + // them, so this function's own write is race-free by construction. + const circuit = circuits.get(endpointCategory) ?? { + failures: 0, + cooldownUntil: 0, + }; + const failures = circuit.failures + 1; + const cooldownUntil = + failures >= CIRCUIT_FAILURE_THRESHOLD + ? Date.now() + CIRCUIT_COOLDOWN_MS + : circuit.cooldownUntil; + circuits.set(endpointCategory, { failures, cooldownUntil }); +} + +function recordCircuitSuccess(endpointCategory: string): void { + circuits.delete(endpointCategory); +} + +// Not every failure reason should trip the breaker. A plain 404 (or any +// other non-{403,429,5xx} http_error) is often a LEGITIMATE negative +// business result, not a sign the remote service is unhealthy — e.g. +// typosquat.ts calls boundedFetchStatus for many candidate package names +// where a 404 just means "this typo-squat candidate doesn't exist," which +// is the analyzer working correctly, not a failure worth circuit-breaking +// on. Similarly "aborted" is always CALLER-driven (either the caller's +// signal was already aborted before the call started, or a parent signal +// cancelled mid-flight) — never a signal about the remote service's own +// health — so it must never count. "response_too_large" and "invalid_json" +// mean the remote DID respond, just unexpectedly, so they don't count +// either. Only "timeout", "network_error", and http_error with status +// 403/429/5xx (auth/rate-limit/server-error — genuinely indicates the +// remote is unhealthy or blocking us) should trip the breaker. This +// mirrors shouldMarkDegraded's distinction below but is intentionally MORE +// NARROW — shouldMarkDegraded answers "should the caller's diagnostics be +// marked partial" (broader, includes aborted/response_too_large), this +// answers "does this failure indicate the REMOTE SERVICE is unhealthy" +// (narrower); do not conflate the two or reuse shouldMarkDegraded here. +function isRemoteHealthFailure(result: BoundedFetchFailure): boolean { + if (result.reason === "timeout" || result.reason === "network_error") + return true; + if (result.reason === "http_error") { + const status = result.status ?? 0; + return status === 403 || status === 429 || status >= 500; + } + return false; +} + export function externalFetchCacheKey( url: string, options: Pick = {}, @@ -90,6 +158,17 @@ export async function boundedFetchStatus( ): Promise> { const endpointCategory = safeEndpointCategory(options.endpointCategory); const startedAtMs = Date.now(); + if (isCircuitOpen(endpointCategory)) { + const result: BoundedFetchFailure = { + ok: false, + reason: "circuit_open", + bytes: null, + elapsedMs: 0, + endpointCategory, + }; + attachDiagnostics(result, options); + return result; + } const signal = options.signal; if (signal?.aborted) { const result = failure(endpointCategory, "aborted", startedAtMs, null); @@ -119,11 +198,19 @@ export async function boundedFetchStatus( }); const status = response.status; if (!response.ok) { - const result = failure(endpointCategory, "http_error", startedAtMs, null, status); + const result = failure( + endpointCategory, + "http_error", + startedAtMs, + null, + status, + ); attachDiagnostics(result, options); + if (isRemoteHealthFailure(result)) recordCircuitFailure(endpointCategory); return result; } + recordCircuitSuccess(endpointCategory); return { ok: true, status, @@ -134,9 +221,14 @@ export async function boundedFetchStatus( }; } catch { const reason = - timedOut || controller.signal.aborted ? (timedOut ? "timeout" : "aborted") : "network_error"; + timedOut || controller.signal.aborted + ? timedOut + ? "timeout" + : "aborted" + : "network_error"; const result = failure(endpointCategory, reason, startedAtMs, null); attachDiagnostics(result, options); + if (isRemoteHealthFailure(result)) recordCircuitFailure(endpointCategory); return result; } finally { clearTimeout(timer); @@ -150,6 +242,17 @@ export async function boundedFetchText( ): Promise> { const endpointCategory = safeEndpointCategory(options.endpointCategory); const startedAtMs = Date.now(); + if (isCircuitOpen(endpointCategory)) { + const result: BoundedFetchFailure = { + ok: false, + reason: "circuit_open", + bytes: null, + elapsedMs: 0, + endpointCategory, + }; + attachDiagnostics(result, options); + return result; + } const signal = options.signal; if (signal?.aborted) { const result = failure(endpointCategory, "aborted", startedAtMs, null); @@ -179,12 +282,22 @@ export async function boundedFetchText( }); const status = response.status; if (!response.ok) { - const result = failure(endpointCategory, "http_error", startedAtMs, null, status); + const result = failure( + endpointCategory, + "http_error", + startedAtMs, + null, + status, + ); attachDiagnostics(result, options); + if (isRemoteHealthFailure(result)) recordCircuitFailure(endpointCategory); return result; } - const maxBytes = Math.max(1, Math.floor(options.maxBytes ?? DEFAULT_MAX_JSON_BYTES)); + const maxBytes = Math.max( + 1, + Math.floor(options.maxBytes ?? DEFAULT_MAX_JSON_BYTES), + ); const text = await readResponseText(response, maxBytes); if (text === null) { const result = failure( @@ -196,9 +309,11 @@ export async function boundedFetchText( true, ); attachDiagnostics(result, options); + if (isRemoteHealthFailure(result)) recordCircuitFailure(endpointCategory); return result; } + recordCircuitSuccess(endpointCategory); return { ok: true, status, @@ -209,9 +324,14 @@ export async function boundedFetchText( }; } catch { const reason = - timedOut || controller.signal.aborted ? (timedOut ? "timeout" : "aborted") : "network_error"; + timedOut || controller.signal.aborted + ? timedOut + ? "timeout" + : "aborted" + : "network_error"; const result = failure(endpointCategory, reason, startedAtMs, null); attachDiagnostics(result, options); + if (isRemoteHealthFailure(result)) recordCircuitFailure(endpointCategory); return result; } finally { clearTimeout(timer); diff --git a/review-enrichment/test/external-fetch.test.ts b/review-enrichment/test/external-fetch.test.ts index 884dce79f3..4d6917b938 100644 --- a/review-enrichment/test/external-fetch.test.ts +++ b/review-enrichment/test/external-fetch.test.ts @@ -1,27 +1,48 @@ -import { test } from "node:test"; +import { test, mock, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; import { createAnalysisContext } from "../dist/analysis-context.js"; -import { boundedFetchJson, boundedFetchStatus } from "../dist/external-fetch.js"; +import { + boundedFetchJson, + boundedFetchStatus, + boundedFetchText, + resetExternalFetchCircuitBreakerForTest, +} from "../dist/external-fetch.js"; + +beforeEach(() => { + resetExternalFetchCircuitBreakerForTest(); +}); + +afterEach(() => { + resetExternalFetchCircuitBreakerForTest(); + mock.timers.reset(); +}); test("boundedFetchJson aborts slow subcalls and records safe diagnostics", async () => { const diagnostics = {}; const fetchImpl = async (_url, init = {}) => new Promise((_resolve, reject) => { - init.signal?.addEventListener("abort", () => reject(new Error("aborted")), { - once: true, - }); + init.signal?.addEventListener( + "abort", + () => reject(new Error("aborted")), + { + once: true, + }, + ); }); - const result = await boundedFetchJson("https://registry.example.test/private", { - endpointCategory: "npm-packument", - timeoutMs: 5, - body: "sensitive request body should not be attached", - fetchImpl, - diagnostics, - phase: "test-phase", - subcall: "test-subcall", - }); + const result = await boundedFetchJson( + "https://registry.example.test/private", + { + endpointCategory: "npm-packument", + timeoutMs: 5, + body: "sensitive request body should not be attached", + fetchImpl, + diagnostics, + phase: "test-phase", + subcall: "test-subcall", + }, + ); assert.equal(result.ok, false); assert.equal(result.reason, "timeout"); @@ -155,3 +176,329 @@ test("AnalysisContext fetchJson de-dupes identical in-flight calls and caps new }); assert.equal(cappedDiagnostics.partialReason, "osv-query_call_cap"); }); + +test("circuit breaker: a healthy endpoint never opens the circuit", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + return new Response("{}", { status: 200 }); + }; + + for (let index = 0; index < 10; index += 1) { + const result = await boundedFetchText("https://api.example.test/healthy", { + endpointCategory: "npm-version", + fetchImpl, + }); + assert.equal(result.ok, true); + } + + assert.equal(calls, 10); +}); + +test("circuit breaker: opens after threshold consecutive network errors and skips the underlying fetch during cooldown", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + throw new Error("connection refused"); + }; + + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchText("https://api.example.test/down", { + endpointCategory: "osv-query", + fetchImpl, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, "network_error"); + } + assert.equal(calls, 3); + + const openResult = await boundedFetchText("https://api.example.test/down", { + endpointCategory: "osv-query", + fetchImpl, + }); + assert.equal(openResult.ok, false); + assert.equal(openResult.reason, "circuit_open"); + assert.equal(openResult.elapsedMs, 0); + assert.equal( + calls, + 3, + "underlying fetchImpl must not be invoked while the circuit is open", + ); +}); + +test("circuit breaker: opens after threshold consecutive 500s and attaches circuit_open diagnostics", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + return new Response("boom", { status: 500 }); + }; + + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchText("https://api.example.test/500s", { + endpointCategory: "pypi-json", + fetchImpl, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, "http_error"); + } + + const diagnostics = {}; + const openResult = await boundedFetchText("https://api.example.test/500s", { + endpointCategory: "pypi-json", + fetchImpl, + diagnostics, + }); + assert.equal(openResult.ok, false); + assert.equal(openResult.reason, "circuit_open"); + assert.equal(calls, 3); + assert.equal(diagnostics.externalFailureReason, "circuit_open"); + assert.equal(diagnostics.partialStatus, "partial"); + assert.equal(diagnostics.endpointCategory, "pypi-json"); +}); + +test("circuit breaker: recovers after the cooldown elapses", async () => { + mock.timers.enable({ apis: ["Date"] }); + try { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + if (calls <= 3) throw new Error("connection refused"); + return new Response("{}", { status: 200 }); + }; + + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchText( + "https://api.example.test/recovers", + { + endpointCategory: "bundlephobia-size", + fetchImpl, + }, + ); + assert.equal(result.ok, false); + } + assert.equal(calls, 3); + + const stillOpen = await boundedFetchText( + "https://api.example.test/recovers", + { + endpointCategory: "bundlephobia-size", + fetchImpl, + }, + ); + assert.equal(stillOpen.reason, "circuit_open"); + assert.equal(calls, 3); + + // Advance past the 30s cooldown window. + mock.timers.tick(30_001); + + const recovered = await boundedFetchText( + "https://api.example.test/recovers", + { + endpointCategory: "bundlephobia-size", + fetchImpl, + }, + ); + assert.equal(recovered.ok, true); + assert.equal( + calls, + 4, + "the real fetchImpl must be reached again after cooldown", + ); + } finally { + mock.timers.reset(); + } +}); + +test("circuit breaker regression: repeated plain 404s never trip the breaker (typosquat candidate lookups)", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + return new Response(null, { status: 404 }); + }; + + for (let index = 0; index < 10; index += 1) { + const result = await boundedFetchStatus( + "https://registry.example.test/candidate", + { + endpointCategory: "npm-attestations", + fetchImpl, + }, + ); + assert.equal(result.ok, false); + assert.equal(result.reason, "http_error"); + assert.equal(result.status, 404); + } + + // The Nth+1 call must still reach the real fetchImpl, not a circuit_open skip. + assert.equal(calls, 10); + const stillReal = await boundedFetchStatus( + "https://registry.example.test/candidate", + { + endpointCategory: "npm-attestations", + fetchImpl, + }, + ); + assert.equal(stillReal.reason, "http_error"); + assert.equal(calls, 11); +}); + +test("circuit breaker regression: repeated aborted results never trip the breaker", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + return new Response("{}", { status: 200 }); + }; + + for (let index = 0; index < 10; index += 1) { + const controller = new AbortController(); + controller.abort(); + const result = await boundedFetchText("https://api.example.test/aborted", { + endpointCategory: "deps-dev", + signal: controller.signal, + fetchImpl, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, "aborted"); + } + + // fetchImpl was never reached (caller-aborted before dispatch), and the + // circuit is still closed: a normal call goes through to the real fetch. + assert.equal(calls, 0); + const normal = await boundedFetchText("https://api.example.test/aborted", { + endpointCategory: "deps-dev", + fetchImpl, + }); + assert.equal(normal.ok, true); + assert.equal(calls, 1); +}); + +test("circuit breaker: boundedFetchStatus and boundedFetchText/Json share one circuit per endpointCategory", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + throw new Error("connection refused"); + }; + + // Trip the breaker via boundedFetchStatus. + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchStatus("https://api.example.test/shared", { + endpointCategory: "github-commits", + fetchImpl, + }); + assert.equal(result.ok, false); + } + assert.equal(calls, 3); + + // A subsequent boundedFetchJson call for the same category is short-circuited. + const jsonResult = await boundedFetchJson("https://api.example.test/shared", { + endpointCategory: "github-commits", + fetchImpl, + }); + assert.equal(jsonResult.ok, false); + assert.equal(jsonResult.reason, "circuit_open"); + assert.equal( + calls, + 3, + "boundedFetchJson must not invoke fetchImpl while the shared circuit is open", + ); + + // And vice versa: reset, trip via boundedFetchJson (through boundedFetchText), confirm + // boundedFetchStatus for the same category is short-circuited too. + resetExternalFetchCircuitBreakerForTest(); + calls = 0; + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchJson("https://api.example.test/shared-2", { + endpointCategory: "github-heavy-shared", + fetchImpl, + }); + assert.equal(result.ok, false); + } + assert.equal(calls, 3); + + const statusResult = await boundedFetchStatus( + "https://api.example.test/shared-2", + { + endpointCategory: "github-heavy-shared", + fetchImpl, + }, + ); + assert.equal(statusResult.ok, false); + assert.equal(statusResult.reason, "circuit_open"); + assert.equal(calls, 3); +}); + +test("circuit breaker: two different endpointCategory values never cross-contaminate", async () => { + let failingCalls = 0; + const failingFetch = async () => { + failingCalls += 1; + throw new Error("connection refused"); + }; + let healthyCalls = 0; + const healthyFetch = async () => { + healthyCalls += 1; + return new Response("{}", { status: 200 }); + }; + + for (let index = 0; index < 3; index += 1) { + const result = await boundedFetchText("https://api.example.test/failing", { + endpointCategory: "endoflife", + fetchImpl: failingFetch, + }); + assert.equal(result.ok, false); + } + assert.equal(failingCalls, 3); + + const openResult = await boundedFetchText( + "https://api.example.test/failing", + { + endpointCategory: "endoflife", + fetchImpl: failingFetch, + }, + ); + assert.equal(openResult.reason, "circuit_open"); + + // The unrelated category is unaffected and still reaches the real fetch. + const unaffected = await boundedFetchText( + "https://api.example.test/healthy", + { + endpointCategory: "pypi-simple", + fetchImpl: healthyFetch, + }, + ); + assert.equal(unaffected.ok, true); + assert.equal(healthyCalls, 1); +}); + +test("resetExternalFetchCircuitBreakerForTest clears circuit state between tests", async () => { + let calls = 0; + const fetchImpl = async () => { + calls += 1; + throw new Error("connection refused"); + }; + + for (let index = 0; index < 3; index += 1) { + await boundedFetchText("https://api.example.test/leak-check", { + endpointCategory: "npm-version-leak-check", + fetchImpl, + }); + } + const open = await boundedFetchText("https://api.example.test/leak-check", { + endpointCategory: "npm-version-leak-check", + fetchImpl, + }); + assert.equal(open.reason, "circuit_open"); + + resetExternalFetchCircuitBreakerForTest(); + + const afterReset = await boundedFetchText( + "https://api.example.test/leak-check", + { + endpointCategory: "npm-version-leak-check", + fetchImpl, + }, + ); + // After a full reset, the circuit is closed again: this call reaches the + // real (failing) fetchImpl rather than being short-circuited. + assert.equal(afterReset.reason, "network_error"); + assert.equal(calls, 4); +});