diff --git a/src/agents/pi-tools.abort.ts b/src/agents/pi-tools.abort.ts index 04152a3588..50d08daf10 100644 --- a/src/agents/pi-tools.abort.ts +++ b/src/agents/pi-tools.abort.ts @@ -1,4 +1,5 @@ import type { AnyAgentTool } from "./pi-tools.types.js"; +import { bindAbortRelay } from "../utils/fetch-timeout.js"; function throwAbortError(): never { const err = new Error("Aborted"); @@ -36,7 +37,7 @@ function combineAbortSignals(a?: AbortSignal, b?: AbortSignal): AbortSignal | un } const controller = new AbortController(); - const onAbort = controller.abort.bind(controller); + const onAbort = bindAbortRelay(controller); a?.addEventListener("abort", onAbort, { once: true }); b?.addEventListener("abort", onAbort, { once: true }); return controller.signal; diff --git a/src/infra/abort-pattern.test.ts b/src/infra/abort-pattern.test.ts index dd83580abe..6e20d3ce2b 100644 --- a/src/infra/abort-pattern.test.ts +++ b/src/infra/abort-pattern.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { bindAbortRelay } from "../utils/fetch-timeout.js"; /** * Regression test for #7174: Memory leak from closure-wrapped controller.abort(). @@ -7,12 +8,13 @@ import { describe, expect, it } from "vitest"; * surrounding lexical scope (controller, timer, locals). In long-running * processes these closures accumulate and prevent GC. * - * The fix is `controller.abort.bind(controller)` which creates a minimal - * bound function with no scope capture. - * - * This test verifies the behavioral equivalence of .bind() for both the - * setTimeout and addEventListener use-cases. + * The fix uses two patterns: + * - setTimeout: `controller.abort.bind(controller)` (safe, no args passed) + * - addEventListener: `bindAbortRelay(controller)` which returns a bound + * function that ignores the Event argument, preserving the default + * AbortError reason. */ + describe("abort pattern: .bind() vs arrow closure (#7174)", () => { it("controller.abort.bind(controller) aborts the signal", () => { const controller = new AbortController(); @@ -31,42 +33,64 @@ describe("abort pattern: .bind() vs arrow closure (#7174)", () => { clearTimeout(timer); }); - it("bound abort works as addEventListener callback and can be removed", () => { + it("bindAbortRelay() preserves default AbortError reason when used as event listener", () => { const parent = new AbortController(); const child = new AbortController(); - const onAbort = child.abort.bind(child); + const onAbort = bindAbortRelay(child); parent.signal.addEventListener("abort", onAbort, { once: true }); - expect(child.signal.aborted).toBe(false); - parent.abort(); + expect(child.signal.aborted).toBe(true); + // The reason must be the default AbortError, not the Event object + expect(child.signal.reason).toBeInstanceOf(DOMException); + expect(child.signal.reason.name).toBe("AbortError"); }); - it("removeEventListener works with saved .bind() reference", () => { + it("raw .abort.bind() leaks Event as reason — bindAbortRelay() does not", () => { + // Demonstrates the bug: .abort.bind() passes the Event as abort reason + const parentA = new AbortController(); + const childA = new AbortController(); + parentA.signal.addEventListener("abort", childA.abort.bind(childA), { once: true }); + parentA.abort(); + // childA.signal.reason is the Event, NOT an AbortError + expect(childA.signal.reason).not.toBeInstanceOf(DOMException); + + // The fix: bindAbortRelay() ignores the Event argument + const parentB = new AbortController(); + const childB = new AbortController(); + parentB.signal.addEventListener("abort", bindAbortRelay(childB), { once: true }); + parentB.abort(); + // childB.signal.reason IS the default AbortError + expect(childB.signal.reason).toBeInstanceOf(DOMException); + expect(childB.signal.reason.name).toBe("AbortError"); + }); + + it("removeEventListener works with saved bindAbortRelay() reference", () => { const parent = new AbortController(); const child = new AbortController(); - const onAbort = child.abort.bind(child); + const onAbort = bindAbortRelay(child); parent.signal.addEventListener("abort", onAbort); - // Remove before parent aborts — child should NOT be aborted parent.signal.removeEventListener("abort", onAbort); parent.abort(); expect(child.signal.aborted).toBe(false); }); - it("bound abort forwards abort through combined signals", () => { + it("bindAbortRelay() forwards abort through combined signals", () => { // Simulates the combineAbortSignals pattern from pi-tools.abort.ts const signalA = new AbortController(); const signalB = new AbortController(); const combined = new AbortController(); - const onAbort = combined.abort.bind(combined); + const onAbort = bindAbortRelay(combined); signalA.signal.addEventListener("abort", onAbort, { once: true }); signalB.signal.addEventListener("abort", onAbort, { once: true }); expect(combined.signal.aborted).toBe(false); signalA.abort(); expect(combined.signal.aborted).toBe(true); + expect(combined.signal.reason).toBeInstanceOf(DOMException); + expect(combined.signal.reason.name).toBe("AbortError"); }); }); diff --git a/src/infra/fetch.ts b/src/infra/fetch.ts index 23791227d8..fe4c7c351a 100644 --- a/src/infra/fetch.ts +++ b/src/infra/fetch.ts @@ -1,3 +1,5 @@ +import { bindAbortRelay } from "../utils/fetch-timeout.js"; + type FetchWithPreconnect = typeof fetch & { preconnect: (url: string, init?: { credentials?: RequestCredentials }) => void; }; @@ -42,7 +44,7 @@ export function wrapFetchWithAbortSignal(fetchImpl: typeof fetch): typeof fetch return fetchImpl(input, patchedInit); } const controller = new AbortController(); - const onAbort = controller.abort.bind(controller); + const onAbort = bindAbortRelay(controller); if (signal.aborted) { controller.abort(); } else { diff --git a/src/infra/net/fetch-guard.ts b/src/infra/net/fetch-guard.ts index ac51bc2faf..b75f468b34 100644 --- a/src/infra/net/fetch-guard.ts +++ b/src/infra/net/fetch-guard.ts @@ -1,5 +1,6 @@ import type { Dispatcher } from "undici"; import { logWarn } from "../../logger.js"; +import { bindAbortRelay } from "../../utils/fetch-timeout.js"; import { closeDispatcher, createPinnedDispatcher, @@ -51,7 +52,7 @@ function buildAbortSignal(params: { timeoutMs?: number; signal?: AbortSignal }): const controller = new AbortController(); const timeoutId = setTimeout(controller.abort.bind(controller), timeoutMs); - const onAbort = controller.abort.bind(controller); + const onAbort = bindAbortRelay(controller); if (signal) { if (signal.aborted) { controller.abort(); diff --git a/src/utils/fetch-timeout.ts b/src/utils/fetch-timeout.ts index f9567fbcdb..150f4e119a 100644 --- a/src/utils/fetch-timeout.ts +++ b/src/utils/fetch-timeout.ts @@ -1,3 +1,16 @@ +/** + * Relay abort without forwarding the Event argument as the abort reason. + * Using .bind() avoids closure scope capture (memory leak prevention). + */ +function relayAbort(this: AbortController) { + this.abort(); +} + +/** Returns a bound abort relay for use as an event listener. */ +export function bindAbortRelay(controller: AbortController): () => void { + return relayAbort.bind(controller); +} + /** * Fetch wrapper that adds timeout support via AbortController. *