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); + }); +});