From 35f1704930fe3ae88411388e450fc37045a112ff Mon Sep 17 00:00:00 2001 From: HelloThisWorld Date: Thu, 20 Aug 2026 20:56:29 +0800 Subject: [PATCH] fix(runners): replace GC-vulnerable AbortSignal composition in the HTTP client The shared model-API HTTP client composed its total timeout as AbortSignal.any([AbortSignal.timeout(ms), external]) built inline in the fetch call. On Node 20 the composite holds only weak references to its source signals, so an otherwise-unreferenced timeout signal can be garbage collected before its timer fires -- and a request against an endpoint that never answers then hangs forever instead of timing out. This is the root cause of the recurring node-20 CI failure where "a timeout aborts the request deterministically" (contract: ~1.5 s) burned the full 30-second Vitest budget: the abort simply never happened. The flake predates v1.3 (identical failures on the #14 and #16 merge commits) and is GC-timing dependent, which is why it appeared intermittently and almost always on the node 20 runners. Every consumer of safeHttpRequest was exposed: Ollama, OpenAI-compatible endpoints, the managed local model, and registry downloads. The client now creates one explicit AbortController with a real timer per request -- no GC dependence on any Node version -- wired to the external signal and released in a finally, which guarantees the timer and listener never outlive the request. This also fixes the 'abort' listener any() leaked on long-lived external signals (one per request, never unsubscribed), and makes the timeout genuinely TOTAL across redirect hops and body streaming, as the contract always documented. New regression tests pin the replacement mechanism at the client level: never-responding and headers-then-stall endpoints time out deterministically, external aborts classify as cancelled, sequential requests leave zero leaked listeners, and createBoundedAbort's fire/release/propagation semantics are covered directly. The plugin dist bundles are regenerated. Full suite: 1788 tests pass; lint, typecheck, snapshots, smoke-relevant bundle checks all verify. --- CHANGELOG.md | 17 ++ .../specbridge/dist/checksums.json | 8 +- .../specbridge/dist/cli.cjs | 29 +++- .../specbridge/dist/mcp-server.cjs | 29 +++- packages/runners/src/shared/http-client.ts | 64 ++++++- tests/runners/http-timeout.test.ts | 164 ++++++++++++++++++ 6 files changed, 292 insertions(+), 19 deletions(-) create mode 100644 tests/runners/http-timeout.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 85c0583..28a9ce9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,6 +159,23 @@ development is an additional mode, never forced. aggregation with contradiction stops, persistent-failure honesty, and mid-objective interruption resumed to completion. +### Fixed + +- The shared model-API HTTP client no longer composes its total timeout + with `AbortSignal.any([AbortSignal.timeout(ms), external])`. On Node 20 + the composite holds only weak references to its sources, so an + otherwise-unreferenced timeout signal could be garbage collected before + its timer fired — and a request against an endpoint that never answers + (Ollama, OpenAI-compatible, the managed local model, registry downloads) + then hung forever instead of timing out. This was the intermittent + node-20 CI failure where "a timeout aborts the request deterministically" + burned the full 30-second test budget. The client now uses one explicit + `AbortController` with a real timer per request — no GC dependence on any + Node version — released in a `finally`, which also fixes the 'abort' + listener `any()` leaked on long-lived external signals per request, and + makes the timeout genuinely TOTAL across redirect hops and body + streaming, as the contract always documented. + ## 1.2.0 (unreleased) The persistent, local-first, multi-agent orchestrator. v1.1 governed how a diff --git a/integrations/claude-code-plugin/specbridge/dist/checksums.json b/integrations/claude-code-plugin/specbridge/dist/checksums.json index b84ac1f..5046e65 100644 --- a/integrations/claude-code-plugin/specbridge/dist/checksums.json +++ b/integrations/claude-code-plugin/specbridge/dist/checksums.json @@ -7,12 +7,12 @@ "bytes": 153474 }, "cli.cjs": { - "sha256": "036269472c14df8187315e587afb9a7162f210c400b696290619ecbce48e8a1d", - "bytes": 3764381 + "sha256": "c52ae095f7a285e761bc4bdc36fd56e5801d76a56146de7779d3daf36b9cff51", + "bytes": 3764925 }, "mcp-server.cjs": { - "sha256": "852f1faccd9cb171eef9d99c946d14c7e583e4986a32fea512d26882363320d8", - "bytes": 2761255 + "sha256": "9b3408854592716c89f66a32f7e11e466789281623acafe44271bf2e64907edd", + "bytes": 2761796 } } } diff --git a/integrations/claude-code-plugin/specbridge/dist/cli.cjs b/integrations/claude-code-plugin/specbridge/dist/cli.cjs index ce3254a..46d750f 100644 --- a/integrations/claude-code-plugin/specbridge/dist/cli.cjs +++ b/integrations/claude-code-plugin/specbridge/dist/cli.cjs @@ -40039,10 +40039,21 @@ function strictJsonParse2(raw) { return void 0; } } -function composeSignals(timeoutMs, external) { - const signals2 = [AbortSignal.timeout(timeoutMs)]; - if (external !== void 0) signals2.push(external); - return AbortSignal.any(signals2); +function createBoundedAbort(timeoutMs, external) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + const onExternalAbort = () => controller.abort(); + if (external !== void 0) { + if (external.aborted) controller.abort(); + else external.addEventListener("abort", onExternalAbort, { once: true }); + } + return { + signal: controller.signal, + release: () => { + clearTimeout(timer); + external?.removeEventListener("abort", onExternalAbort); + } + }; } async function readBounded(response, maxBytes) { const reader = response.body?.getReader(); @@ -40090,6 +40101,14 @@ function checkRedirectTarget(current, location) { return { ok: true, nextUrl: next }; } async function safeHttpRequest(request) { + const bounded2 = createBoundedAbort(request.timeoutMs, request.signal); + try { + return await performSafeHttpRequest(request, bounded2.signal); + } finally { + bounded2.release(); + } +} +async function performSafeHttpRequest(request, signal) { const started = Date.now(); const duration3 = () => Math.max(0, Date.now() - started); const externalAborted = () => request.signal?.aborted === true; @@ -40112,7 +40131,7 @@ async function safeHttpRequest(request) { response = await fetch(currentUrl.toString(), { method: currentMethod, redirect: "manual", - signal: composeSignals(request.timeoutMs, request.signal), + signal, headers, ...sendBody ? { body: JSON.stringify(request.body) } : {} }); diff --git a/integrations/claude-code-plugin/specbridge/dist/mcp-server.cjs b/integrations/claude-code-plugin/specbridge/dist/mcp-server.cjs index e62a49f..1bba7af 100644 --- a/integrations/claude-code-plugin/specbridge/dist/mcp-server.cjs +++ b/integrations/claude-code-plugin/specbridge/dist/mcp-server.cjs @@ -44008,10 +44008,21 @@ function strictJsonParse2(raw) { return void 0; } } -function composeSignals(timeoutMs, external) { - const signals2 = [AbortSignal.timeout(timeoutMs)]; - if (external !== void 0) signals2.push(external); - return AbortSignal.any(signals2); +function createBoundedAbort(timeoutMs, external) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + const onExternalAbort = () => controller.abort(); + if (external !== void 0) { + if (external.aborted) controller.abort(); + else external.addEventListener("abort", onExternalAbort, { once: true }); + } + return { + signal: controller.signal, + release: () => { + clearTimeout(timer); + external?.removeEventListener("abort", onExternalAbort); + } + }; } async function readBounded(response, maxBytes) { const reader = response.body?.getReader(); @@ -44059,6 +44070,14 @@ function checkRedirectTarget(current, location) { return { ok: true, nextUrl: next }; } async function safeHttpRequest(request) { + const bounded = createBoundedAbort(request.timeoutMs, request.signal); + try { + return await performSafeHttpRequest(request, bounded.signal); + } finally { + bounded.release(); + } +} +async function performSafeHttpRequest(request, signal) { const started = Date.now(); const duration3 = () => Math.max(0, Date.now() - started); const externalAborted = () => request.signal?.aborted === true; @@ -44081,7 +44100,7 @@ async function safeHttpRequest(request) { response = await fetch(currentUrl.toString(), { method: currentMethod, redirect: "manual", - signal: composeSignals(request.timeoutMs, request.signal), + signal, headers, ...sendBody ? { body: JSON.stringify(request.body) } : {} }); diff --git a/packages/runners/src/shared/http-client.ts b/packages/runners/src/shared/http-client.ts index 0c76c5e..82783c8 100644 --- a/packages/runners/src/shared/http-client.ts +++ b/packages/runners/src/shared/http-client.ts @@ -86,10 +86,49 @@ export type SafeHttpResult = bodyExcerpt?: string; }; -function composeSignals(timeoutMs: number, external?: AbortSignal): AbortSignal { - const signals: AbortSignal[] = [AbortSignal.timeout(timeoutMs)]; - if (external !== undefined) signals.push(external); - return AbortSignal.any(signals); +export interface BoundedAbort { + signal: AbortSignal; + /** Detach the timer and the external listener. Always call in `finally`. */ + release: () => void; +} + +/** + * Compose the total-timeout signal with an optional external signal. + * + * Deliberately NOT `AbortSignal.any([AbortSignal.timeout(ms), external])`: + * on Node 20 the composite holds only weak references to its source + * signals, so an otherwise-unreferenced timeout signal can be garbage + * collected before its timer fires — and a request against an endpoint + * that never answers then hangs FOREVER instead of timing out. This is the + * root cause of the intermittent node-20 CI failures where the ollama + * "timeout aborts deterministically" test (contract: ~1.5 s) burned the + * whole 30 s Vitest budget: the abort simply never happened. `any()` also + * leaks one 'abort' listener on the external signal per request, because + * nothing ever unsubscribes the composite. + * + * An explicit controller with a real timer has no GC dependence on any + * Node version, and `release()` (called in the request's `finally`) + * guarantees the timer and the external listener never outlive the + * request. Exported for tests. + */ +export function createBoundedAbort(timeoutMs: number, external?: AbortSignal): BoundedAbort { + const controller = new AbortController(); + // The timer stays ref'd: it is always cleared in release(), so it cannot + // keep the process alive beyond the request — and a ref'd timer cannot + // be skipped the way an unref'd one can when the loop drains. + const timer = setTimeout(() => controller.abort(), timeoutMs); + const onExternalAbort = (): void => controller.abort(); + if (external !== undefined) { + if (external.aborted) controller.abort(); + else external.addEventListener('abort', onExternalAbort, { once: true }); + } + return { + signal: controller.signal, + release: (): void => { + clearTimeout(timer); + external?.removeEventListener('abort', onExternalAbort); + }, + }; } /** Read a body stream up to the limit; abort the connection beyond it. */ @@ -154,6 +193,21 @@ export function checkRedirectTarget(current: URL, location: string): RedirectDec /** One bounded HTTP request. Never throws for transport-level failures. */ export async function safeHttpRequest(request: SafeHttpRequest): Promise { + // One abort scope for the WHOLE request — connect, headers, every + // redirect hop, and body streaming share the total timeout budget, and + // release() in the finally guarantees no timer or listener outlives it. + const bounded = createBoundedAbort(request.timeoutMs, request.signal); + try { + return await performSafeHttpRequest(request, bounded.signal); + } finally { + bounded.release(); + } +} + +async function performSafeHttpRequest( + request: SafeHttpRequest, + signal: AbortSignal, +): Promise { const started = Date.now(); const duration = (): number => Math.max(0, Date.now() - started); const externalAborted = (): boolean => request.signal?.aborted === true; @@ -181,7 +235,7 @@ export async function safeHttpRequest(request: SafeHttpRequest): Promise Promise; +} + +/** A server that accepts requests and never answers (optionally headers-only). */ +async function startHangingServer(mode: 'silent' | 'headers-then-stall'): Promise { + const server: Server = createServer((request, response) => { + void request; + if (mode === 'headers-then-stall') { + response.writeHead(200, { 'content-type': 'application/json' }); + response.write('{"partial":'); + // …and never finish the body. + } + // 'silent': never respond at all; the client's abort is the only exit. + }); + const lifecycle = trackedServerLifecycle(server, `hanging-http-${mode}`); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as AddressInfo; + return { baseUrl: `http://127.0.0.1:${port}`, close: () => lifecycle.close() }; +} + +describe('safeHttpRequest total-timeout abort (node-20 GC regression)', () => { + it('a never-responding endpoint times out deterministically, far under the test budget', async () => { + const server = await startHangingServer('silent'); + try { + const started = Date.now(); + const result = await safeHttpRequest({ + method: 'POST', + url: `${server.baseUrl}/api/chat`, + body: { hang: true }, + timeoutMs: 750, + maxResponseBytes: 1024 * 1024, + }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.kind).toBe('timeout'); + expect(Date.now() - started).toBeLessThan(5_000); + } finally { + await server.close(); + } + }); + + it('the timeout also covers body streaming: headers-then-stall is a timeout, not a hang', async () => { + const server = await startHangingServer('headers-then-stall'); + try { + const started = Date.now(); + const result = await safeHttpRequest({ + method: 'GET', + url: `${server.baseUrl}/api/tags`, + timeoutMs: 750, + maxResponseBytes: 1024 * 1024, + }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.kind).toBe('timeout'); + expect(Date.now() - started).toBeLessThan(5_000); + } finally { + await server.close(); + } + }); + + it('an external abort classifies as cancelled, never as timeout', async () => { + const server = await startHangingServer('silent'); + try { + const controller = new AbortController(); + const pending = safeHttpRequest({ + method: 'GET', + url: `${server.baseUrl}/api/tags`, + timeoutMs: 30_000, + maxResponseBytes: 1024, + signal: controller.signal, + }); + setTimeout(() => controller.abort(), 100); + const result = await pending; + expect(result.ok).toBe(false); + if (!result.ok) expect(result.kind).toBe('cancelled'); + } finally { + await server.close(); + } + }); + + it('sequential requests sharing one external signal leave no abort listeners behind', async () => { + // AbortSignal.any() subscribed to the external signal once per request + // and never unsubscribed — a long-lived controller (a driver run) would + // accumulate one leaked listener per HTTP call. release() must detach. + const server = await startHangingServer('silent'); + try { + const controller = new AbortController(); + for (let index = 0; index < 15; index += 1) { + const result = await safeHttpRequest({ + method: 'GET', + url: `${server.baseUrl}/probe-${index}`, + timeoutMs: 50, + maxResponseBytes: 1024, + signal: controller.signal, + }); + expect(result.ok).toBe(false); + } + expect(getEventListeners(controller.signal, 'abort')).toHaveLength(0); + } finally { + await server.close(); + } + }); +}); + +describe('createBoundedAbort', () => { + it('fires after the timeout and is inert after release()', async () => { + const fired = createBoundedAbort(30); + await new Promise((resolve) => setTimeout(resolve, 80)); + expect(fired.signal.aborted).toBe(true); + fired.release(); + + const released = createBoundedAbort(30); + released.release(); + await new Promise((resolve) => setTimeout(resolve, 80)); + expect(released.signal.aborted).toBe(false); + }); + + it('propagates an external abort and honors an already-aborted external signal', async () => { + const external = new AbortController(); + const bounded = createBoundedAbort(30_000, external.signal); + expect(bounded.signal.aborted).toBe(false); + external.abort(); + expect(bounded.signal.aborted).toBe(true); + bounded.release(); + expect(getEventListeners(external.signal, 'abort')).toHaveLength(0); + + const preAborted = createBoundedAbort(30_000, AbortSignal.abort()); + expect(preAborted.signal.aborted).toBe(true); + preAborted.release(); + }); + + it('release() detaches the external listener without aborting anything', () => { + const external = new AbortController(); + const bounded = createBoundedAbort(30_000, external.signal); + bounded.release(); + expect(getEventListeners(external.signal, 'abort')).toHaveLength(0); + external.abort(); + expect(bounded.signal.aborted).toBe(false); + }); +});