fix(client): compose the caller abort signal instead of leaking a listener on it - #2086
fix(client): compose the caller abort signal instead of leaking a listener on it#2086edenbuilds wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e7c6fd4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // signal eventually aborts keeps Deno from exiting (timer stays ref'd via the | ||
| // listener), even when the request finished long before the timeout. { once: true } | ||
| // only helps if abort fires — not on the success path. See openai/openai-node#1811. | ||
| if (signal) signal.removeEventListener('abort', abort); |
There was a problem hiding this comment.
For streaming requests (or any response with a slow body), fetchWithTimeout resolves as soon as the response headers arrive, while defaultParseResponse later consumes the body/stream using the same internal controller. Removing the caller's abort forwarder here means a user-supplied AbortSignal.timeout(...) or manual abort that fires after headers have been received no longer aborts the stream/body read, so long-running streams can ignore caller cancellation and run until the server finishes. Consider keeping this forwarding active until parsing/stream consumption completes, while still removing it on final completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98e75f67b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return new Response(stream, { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers: response.headers, | ||
| }); |
There was a problem hiding this comment.
When a caller supplies an AbortSignal, this replaces the fetch result with a synthetic Response; for redirected requests or custom fetch implementations that set Response.url, the returned object now has url === '' and redirected === false. That means .asResponse() no longer returns the raw response metadata and the SDK's own debug logging that reads response.url loses the final URL whenever a signal is used.
Useful? React with 👍 / 👎.
| const body = response.body; | ||
| let reader: ReadableStreamDefaultReader<Uint8Array> | undefined; | ||
|
|
||
| const stream = new ReadableStream<Uint8Array>({ |
There was a problem hiding this comment.
For callers that pass a signal and then use .asResponse() or a binary endpoint returning APIPromise<Response>, this wraps the fetch body in a default ReadableStream. Native fetch response bodies are byte streams, so download code using response.body.getReader({ mode: 'byob' }) now throws instead of reading; the wrapper needs to preserve byte-stream semantics or avoid replacing raw response bodies.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| if (signal.aborted || response.body == null) { | ||
| cleanup(); | ||
| return response; |
There was a problem hiding this comment.
When a successful JSON response has Content-Length: 0, defaultParseResponse returns without reading the body, but native fetch still exposes a non-null body stream for this case. Because this helper only cleans up immediately for response.body == null, a caller-provided AbortSignal.timeout(...) remains registered after the request completes and can still keep Deno alive until the timeout fires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4823a2e443
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const originalGetReader = body.getReader.bind(body); | ||
| Object.defineProperty(body, 'getReader', { | ||
| configurable: true, | ||
| value: (...args: any[]) => { |
There was a problem hiding this comment.
When streaming responses are consumed through the SDK, _iterSSEMessages calls ReadableStreamToAsyncIterable(response.body), and that helper returns the native stream whenever stream[Symbol.asyncIterator] exists. The fresh evidence in this version is that this helper only wraps getReader/cancel; in Node/Deno native async iteration and return() do not go through those instance methods, so a stream that finishes normally or is exited with break never calls cleanup(), leaving AbortSignal.timeout() listeners alive after a successful stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864b29228a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Object.defineProperty(body, 'getReader', { | ||
| configurable: true, | ||
| value: (...args: any[]) => { |
There was a problem hiding this comment.
When a raw or binary response is consumed with standard stream helpers such as response.body.pipeTo(...) or response.body.values(), those native helpers bypass the overwritten own getReader/cancel/Symbol.asyncIterator methods and read through the stream internals instead. In that scenario the body can finish successfully without ever calling cleanup(), so a caller using AbortSignal.timeout() in Deno still keeps the abort listener alive until the timeout fires. Please hook these native helper paths too, or arrange cleanup independently of monkey-patched reader methods.
Useful? React with 👍 / 👎.
| Object.defineProperty(body, 'getReader', { | ||
| configurable: true, | ||
| value: (...args: any[]) => { |
There was a problem hiding this comment.
When the SDK parses a normal non-streaming response in Deno, defaultParseResponse calls response.json()/response.text(), but Deno's Body implementation drains the stream with its internal acquireReadableStreamDefaultReader / readableStreamCollectIntoUint8Array path rather than the public body.getReader property replaced here. In that common path none of the wrapped read()/cancel() methods run, so AbortSignal.timeout() listeners remain attached after a successful parsed request and Deno can still hang until the timeout; cleanup needs to be tied to parse completion or to a wrapper that observes internal consumption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da36eeed06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| return await original(...args); | ||
| } finally { | ||
| cleanup(); |
There was a problem hiding this comment.
When an .asResponse() caller invokes response.clone(), the platform tees the payload into new, unpatched body streams. If the original is then consumed first, this unconditional cleanup removes the caller's abort forwarder even while the clone is still streaming, so a later caller abort no longer reaches that clone; if only the clone is consumed, cleanup never runs at all. Wrap clone() and track all resulting branches before detaching.
Useful? React with 👍 / 👎.
| const iterator = originalValues(...args); | ||
| return wrapAsyncIteratorWithCleanup(iterator, cleanup); |
There was a problem hiding this comment.
When a raw-response caller uses body.values({ preventCancel: true }) and exits iteration early, the native iterator's return() deliberately releases its lock without cancelling or closing the body, but this wrapper unconditionally calls cleanup() from return(). The body can subsequently continue streaming while a manual caller abort is no longer forwarded to the fetch controller.
Useful? React with 👍 / 👎.
| // Native helpers that bypass the public getReader surface (WHATWG streams). | ||
| const originalPipeTo = (body as any).pipeTo?.bind(body); |
There was a problem hiding this comment.
For raw or binary responses drained through response.body.pipeThrough(...) (or branches returned by tee()), native stream internals bypass the instance getReader, pipeTo, values, and async-iterator methods patched here. Completing or cancelling those derived streams therefore never calls cleanup(), so in Deno an AbortSignal.timeout() listener can still keep the process alive until its timeout fires.
Useful? React with 👍 / 👎.
| // Expose for defaultParseResponse (Deno json/text may not hit body hooks). | ||
| (response as any)[ABORT_FORWARDER_CLEANUP] = cleanup; |
There was a problem hiding this comment.
When the supported custom fetch option returns a valid but non-extensible Response (for example, one sealed by a wrapper or supplied as a host object), assigning this symbol throws before any of the best-effort patching guards run. Thus a successful request made with a caller signal is converted into a rejection; store the cleanup without requiring the returned response object to accept new properties.
Useful? React with 👍 / 👎.
| reader.read = async (...readArgs: any[]) => { | ||
| try { | ||
| const result = await originalRead(...readArgs); | ||
| if (result.done) cleanup(); |
There was a problem hiding this comment.
When a raw or binary caller reads directly with getReader(), the stream may close while returning its final chunk as { done: false, value }; reader.closed is already fulfilled at that point, but cleanup only occurs if the caller performs another read() to receive done: true. Consumers that stop after collecting the advertised byte count have fully drained the body yet retain the abort listener until its timeout, so closure should also be observed through reader.closed.
Useful? React with 👍 / 👎.
| * never call the public body.getReader property). Safe to call multiple times. | ||
| * Do not call for streaming/binary-raw responses still held by the caller. | ||
| */ | ||
| _releaseAbortForwarder(response: Response): void { |
There was a problem hiding this comment.
Because this method is neither private nor protected, the declaration build emits _releaseAbortForwarder(response: Response): void as a new public member of the exported OpenAI class even though its only caller deliberately accesses it through any. Make the parse/cleanup coordination internal rather than expanding the supported consumer API for this implementation detail.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| async pull(controller) { | ||
| reader ??= body.getReader(); | ||
| try { | ||
| const { done, value } = await reader.read(); |
There was a problem hiding this comment.
When the supported custom fetch option returns a Fetch-compatible response whose body is an async iterable rather than a WHATWG ReadableStream—notably node-fetch responses—the initial body.getReader access falls into this fallback, but the fallback's first pull calls the same missing method again. Consequently any request using a caller signal returns a synthetic response whose json(), text(), or stream consumption fails instead of reading the original Node stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f85e234e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| return await originalPipeTo(...args); | ||
| } finally { | ||
| cleanup(); |
There was a problem hiding this comment.
When a raw-response caller uses body.pipeTo(destination, { preventCancel: true }) and the destination aborts or rejects, the source body deliberately remains open and can be read afterward, but this finally removes the caller's abort forwarder. A later manual abort therefore no longer reaches the still-active fetch; only clean up on rejection when preventCancel is not set, or when the source has actually closed.
Useful? React with 👍 / 👎.
| const stream = new ReadableStream<Uint8Array>({ | ||
| async start(controller) { | ||
| try { | ||
| for await (const chunk of body) { | ||
| const bytes = |
There was a problem hiding this comment.
When a custom fetch such as node-fetch returns an async-iterable body without getReader, a ReadableStream invokes start() immediately, and this loop drains the entire upstream body without consulting controller.desiredSize. Thus merely obtaining .asResponse() starts consuming the payload, and a slow consumer of a large or unbounded download can have the whole response buffered in memory; bridge the iterator from pull() instead so consumption follows downstream demand.
Useful? React with 👍 / 👎.
| test('removes abort signal listener after the response body is fully read', async () => { | ||
| // Regression for openai/openai-node#1811: leave the abort forwarder active | ||
| // until the body ends (so streaming aborts still work), then detach so | ||
| // AbortSignal.timeout() does not keep Deno alive for the full timeout. |
There was a problem hiding this comment.
This starts a 265-line handwritten regression block inside a Stainless-generated client test file, with the same client, signal, and listener-spy setup repeated across several cases. Factor the shared setup and place focused handwritten coverage in the repository's handwritten test area so this generated projection remains maintainable and does not accumulate a large manual patch.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7cadd1808
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (reader.closed && typeof reader.closed.then === 'function') { | ||
| reader.closed.then(() => cleanup(), () => cleanup()); |
There was a problem hiding this comment.
When a raw-response caller acquires a reader and calls releaseLock() without consuming or cancelling the body, the reader.closed promise rejects with a TypeError even though the stream remains readable. Treating every rejection as terminal calls cleanup(), so a subsequent caller abort is no longer forwarded to the still-active response body; only actual stream closure or failure should detach the listener.
Useful? React with 👍 / 👎.
| if (iterator?.return) { | ||
| return Promise.resolve(iterator.return(reason)).then(() => undefined); | ||
| } | ||
| return body.cancel?.(reason); |
There was a problem hiding this comment.
For a custom fetch such as node-fetch, retry handling calls CancelReadableStream on this bridged body before it has been read. The wrapper's async iterator is therefore still undefined, and Node readable bodies generally have return()/destroy() through their iterator but no cancel() method, so this branch does nothing while reporting the wrapper as cancelled; the original error response can retain its socket and continue downloading while the next retry starts.
Useful? React with 👍 / 👎.
|
|
||
| // Response body helpers — Deno/undici often drain through internals; these | ||
| // instance methods still wrap the full consume path for the common API. | ||
| for (const method of ['arrayBuffer', 'blob', 'formData', 'json', 'text'] as const) { |
There was a problem hiding this comment.
On supported Bun runtimes, a raw or binary caller can fully consume the response with response.bytes(), which drains the body through runtime internals rather than the patched body instance methods. Because bytes is omitted from this helper-wrapping list, that successful consumption never calls cleanup(), leaving an AbortSignal.timeout() listener attached until its timeout fires.
Useful? React with 👍 / 👎.
| try { | ||
| return await original(...args); | ||
| } finally { | ||
| cleanup(); |
There was a problem hiding this comment.
When a raw-response body is already locked by a reader, calling a helper such as response.text() rejects immediately without consuming or closing the stream, but this unconditional finally still removes the abort forwarder. The existing reader can then continue receiving the active body while a later caller abort is ignored; cleanup should only run after the helper actually consumes the body or leaves it terminal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3329a6522d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!props.options.stream && !props.options.__binaryResponse) { | ||
| releaseAbortCleanup(response); |
There was a problem hiding this comment.
When a non-streaming response contains malformed JSON, response.json() rejects after consuming the body, so control never reaches this cleanup block. On standard streams isReadableStreamTerminal() cannot observe the internal closed state, meaning the helper wrapper also leaves the listener attached; the failed request can therefore keep an AbortSignal.timeout() listener alive until its timeout. Run this cleanup in a finally around SDK-owned non-stream parsing while continuing to exclude streaming and raw binary responses.
Useful? React with 👍 / 👎.
| value: (reason?: any) => { | ||
| cleanup(); | ||
| return originalCancel(reason); |
There was a problem hiding this comment.
When a raw-response caller already holds a reader, response.body.cancel() rejects because the stream is locked and leaves that reader and body active. This hook removes the abort forwarder before learning whether cancellation succeeded, so the existing reader can continue receiving a slow body while a later caller abort is ignored. Clean up only after cancellation succeeds or the stream is otherwise known to be terminal.
Useful? React with 👍 / 👎.
| // AbortSignal.timeout / manual abort mid-stream still reach `controller`. | ||
| // Detach after body completion so AbortSignal.timeout() does not keep Deno | ||
| // alive until the full timeout (#1811). | ||
| return this._detachAbortOnBodyEnd(response, signal, abort); |
There was a problem hiding this comment.
When a caller supplies an external timeout signal but stops a raw streaming result through the documented stream.controller.abort() path before iterating the body, the internal controller terminates the fetch body without invoking any of these body-consumption hooks. Because this call does not arrange cleanup on controller.signal itself, the forwarder on the external signal remains attached and can keep Deno alive until the timeout fires. Detach the forwarder when the internal controller becomes aborted as well as when the body is consumed.
Useful? React with 👍 / 👎.
| let cleaned = false; | ||
| const cleanup = () => { | ||
| if (cleaned) return; | ||
| cleaned = true; | ||
| signal.removeEventListener('abort', abort); |
There was a problem hiding this comment.
After body completion, cleanup() removes the event listener but remains reachable through the symbol stored on the response and through every patched response/body method. The closure therefore continues retaining the caller's signal and the internal abort controller for as long as a raw or withResponse() consumer retains the response, even though neither is needed after cleanup; signals can in turn retain arbitrary application listeners. Remove the stored hook and sever these captured references once cleanup has run.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| // Stream/read failure is terminal for this reader path. | ||
| cleanup(); | ||
| throw err; |
There was a problem hiding this comment.
A reader read() rejection does not necessarily mean the stream failed: for example, a BYOB reader rejects a zero-length or detached view while remaining locked and usable, and a released reader rejects while the body can be acquired again. This catch nevertheless detaches the caller's abort forwarder, so subsequent valid reads of the still-active response ignore a later abort. Only clean up when the underlying stream, rather than the individual read operation, is terminal.
Useful? React with 👍 / 👎.
| const wrapped = new Response(stream, { | ||
| status: response.status, |
There was a problem hiding this comment.
When a supported node-fetch or custom-fetch response exposes an async-iterable body for a 205 or 304 response without an exact Content-Length: 0 header, the fallback reaches this constructor. The Fetch Response constructor rejects any non-null body for those statuses, so the SDK converts the HTTP response into a thrown fetch error and may retry it as a connection failure instead of handling its status. Preserve these responses without constructing a synthetic response with a body.
Useful? React with 👍 / 👎.
|
Pushed Structural change: the whole mechanism now lives in Cleanup is now driven by terminal body state rather than by "a method returned":
Also carried over from the earlier round: Verification: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe814d4d96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reader = body.getReader(); | ||
| return wrapAsyncIterator( | ||
| { | ||
| next: () => reader.read(), | ||
| async return() { |
There was a problem hiding this comment.
When a signaled raw or streaming response is fully consumed with for await, this reader-backed iterator reports done but never calls reader.releaseLock(). The response body therefore remains permanently locked after normal completion, unlike a native ReadableStream iterator, so subsequent operations such as getReader() fail with ERR_INVALID_STATE; release the lock on terminal completion and stream errors.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| if (res.body == null || isStreamTerminal(res.body)) done(); | ||
| throw err; |
There was a problem hiding this comment.
For a response helper that rejects because the underlying body stream actually errored, standard ReadableStream objects do not expose the state property checked here. In runtimes such as Deno where response.text() drains through internal readers, none of the patched stream methods observes that failure, so a non-OK response whose body terminates mid-read leaves the external AbortSignal.timeout() listener attached even after makeRequest converts the read failure into an API error.
Useful? React with 👍 / 👎.
|
Both P2s from the Release the reader lock after async iteration — correct, and it was a real behavioural difference: the reader-backed iterator only released on early Detach after a terminal body-helper rejection — also correct; Both regressions are pinned:
Suite is 1478 passing (23 handwritten abort regressions), with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c22b241b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function attachAbortCleanup( | ||
| response: Response, | ||
| signal: AbortSignal | null | undefined, | ||
| abort: () => void, | ||
| controller?: AbortController, | ||
| ): Response { |
There was a problem hiding this comment.
This introduces a 629-line handwritten body-lifecycle state machine that monkey-patches most public Response and ReadableStream operations inside a generated SDK, despite the repository requirement that handwritten policy remain small. This effectively duplicates platform stream semantics and makes a narrowly scoped listener fix costly to regenerate and maintain; reduce it to focused cleanup hooks at the SDK-owned parse and streaming completion points instead.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The handwritten cleanup layer was iteratively tightened across later commits. The latest Codex pass on this head reported no major issues; happy to trim further if maintainers want a different structure.
| async throw(err?: any) { | ||
| try { | ||
| if (iterator.throw) return await iterator.throw(err); | ||
| throw err; | ||
| } finally { | ||
| done(); | ||
| } |
There was a problem hiding this comment.
If a raw-response consumer injects an exception through the newly exposed async iterator throw() method, the reader-backed iterator has no underlying throw, so this branch throws and calls done() without cancelling or releasing its reader. The body therefore remains active and locked while the external abort forwarder is detached, causing a later caller abort to be ignored; either omit the synthesized method or cancel and release the iterator's reader before completing cleanup.
Useful? React with 👍 / 👎.
| * both so forwarding survives until whichever branch is read last. | ||
| */ | ||
| function hookClone(res: Response, done: () => void) { | ||
| const originalClone = (res as any).clone?.bind(res); |
There was a problem hiding this comment.
When a raw-response caller invokes clone() repeatedly on the same response, each call re-enters hookResponse(res, ...), and this line captures the already wrapped res.clone rather than the original native method. The wrapper chain therefore doubles on successive clones—later calls recursively execute every previous wrapper and repeatedly hook the same response and clone—so a modest number of otherwise legal clones can cause exponential CPU and memory growth or a stack overflow; retain the native clone function once or avoid re-hooking an already hooked response.
Useful? React with 👍 / 👎.
| body.pipeTo(writable, options).catch(() => { | ||
| // errors surface on `transform.readable`, as the spec requires | ||
| }); |
There was a problem hiding this comment.
When a raw-response caller passes invalid pipeThrough options, such as a non-AbortSignal value in options.signal, native pipeThrough throws a TypeError synchronously. Here the async body.pipeTo wrapper converts that validation failure into a rejected promise and this catch swallows it, so pipeThrough returns the transform's readable side even though no pipe was started; reads from that stream can then wait forever instead of reporting the invalid call.
Useful? React with 👍 / 👎.
|
The P1 is right, and chasing it turned up a much smaller fix: the listener isn't needed at all. Root cause. Deno keeps an const c = new AbortController();
const t = AbortSignal.timeout(30_000);
// (a) what the client does today
t.addEventListener('abort', () => c.abort(), { once: true });
await fetch(url, { signal: c.signal }); // process exits after 30.14s
// (b) composed
await fetch(url, { signal: AbortSignal.any([c.signal, t]) }); // exits after 0.03sSo the whole cleanup layer is gone. Net change to handwritten policy: 15 lines in
The three P2s are resolved by deletion — Fallback. Runtimes predating Verification. A local server exercised on Node 24.16, Deno 2.9.4 and Bun 1.2.10, comparing this branch against
Every functional check passes identically on both — parsed JSON, Suite is 1467 passing with 12 handwritten abort regressions, and |
edenbuilds
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cd8503801
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * the runtime predates `AbortSignal.any` (Node < 18.17, Safari < 17.4); callers | ||
| * then fall back to attaching a listener. | ||
| */ | ||
| export function combineAbortSignals( |
There was a problem hiding this comment.
Although the earlier public client method is gone, this new export function is fresh public surface: the package's ./* export map exposes it as openai/internal/abort-signal, and the new test imports that exact path. This makes an implementation detail part of the technically public SDK API even though the listener fix does not require a new consumer-facing export; inline the helper into the generated client or otherwise keep this subpath private.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| // signal outlives the request, and in Deno that keeps an | ||
| // `AbortSignal.timeout()` timer referenced, holding the process open until | ||
| // the timeout fires (#1811). | ||
| const requestSignal = combineAbortSignals(controller.signal, signal); |
There was a problem hiding this comment.
When a high-level ResponseStream, ChatCompletionStream, or AssistantStream is aborted after headers arrive, its controller is supplied here as the caller signal, so the composed fetch signal aborts but the request's own controller remains un-aborted. The core SSE iterator swallows the resulting AbortError, and those helpers then check stream.controller.signal.aborted to decide whether to throw APIUserAbortError; because that flag is now false, an explicit .abort() can be reported as a normal end or an unrelated finalization error instead of an abort. Ensure the controller passed into the returned Stream also reflects caller-signal cancellation.
Useful? React with 👍 / 👎.
| const any = (globalThis as any).AbortSignal?.any; | ||
| if (typeof any !== 'function') return controllerSignal; | ||
| try { | ||
| return any.call((globalThis as any).AbortSignal, [controllerSignal, signal]) as AbortSignal; |
There was a problem hiding this comment.
When a caller legally uses controller.abort(new Error(...)) after streaming headers arrive, AbortSignal.any propagates that ordinary Error verbatim to the fetch body; in Node's fetch, the next body read rejects with that same error, while Stream.fromSSEResponse suppresses only errors recognized by isAbortError. Before this change the forwarder called the internal controller's parameterless abort(), producing an AbortError, so this now makes direct stream iteration and high-level runners report a generic failure rather than cancellation. Preserve the previous normalized abort reason or classify the failure using the caller signal's aborted state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9833010c40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return false; | ||
| } | ||
| try { | ||
| const composed = nativeAbortSignal.any([controller.signal, callerSignal]) as AbortSignal; |
There was a problem hiding this comment.
The remaining non-SSE parsed path still awaits response.json()/response.text() directly, so when a caller signal aborts after headers arrive but before a normal JSON/text body finishes, this composed signal makes fetch reject with callerSignal.reason (for example AbortSignal.timeout() produces TimeoutError, or abort(new Error(...)) produces that Error). Before this change the listener called controller.abort() and body reads surfaced a normalized AbortError; streaming was adjusted to check controller.signal.aborted, but parsed responses are not, so consumers that classify cancellations by AbortError now see an arbitrary failure for the same mid-body abort scenario.
Useful? React with 👍 / 👎.
| if (signal && !composed) { | ||
| signal.addEventListener('abort', abort, { once: true }); |
There was a problem hiding this comment.
When AbortSignal.any is unavailable, this fallback installs the same long-lived listener and only removes it if fetch rejects, so a successful request in that path still leaves AbortSignal.timeout() listened to until it fires. The README still advertises Deno v1.28.0+ as supported, while this helper's own compatibility note only expects AbortSignal.any on newer runtimes, so users on those supported Deno versions continue to hit the process-hang that this PR is meant to fix.
Useful? React with 👍 / 👎.
49b92b2 to
aa2a71f
Compare
|
Both P1s and the reason-normalisation P2s are fixed; history is squashed to a single commit ( P1 — exported helper became public API. Correct, and I'd missed that P1 — request controller no longer recorded a caller abort. Also correct, and the more serious of the two: Object.defineProperty(controller, 'signal', { value: composed, configurable: true });
P2 — custom abort reasons (streaming, then parsed). Both right, and they are the same bug seen from two sides: a composed signal aborts with the caller's reason, so P2 — Deno below 1.38.2. Accurate, and it stays. Nothing can watch a caller's signal without listening to it, and detaching before the body ends would cut off mid-stream aborts, so closing this last gap means reintroducing the body-lifecycle tracking the first P1 asked me to remove. I took the trade rather than hide it: the compatibility note in the code now names Deno < 1.38.2 explicitly instead of only citing Node and Safari, and the PR description states the limitation. Worth noting the listener on a composed signal doesn't help either — Deno re-references the timer for that too (30.1s to exit, same as Verification. Rather than assert parity, I measured it:
Identical on both runtimes, while Deno exits in 0.57s against 30.63s on 1468 tests passing with 13 handwritten abort regressions; |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa2a71f507
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * classify cancellation that way. | ||
| */ | ||
| function asAbortError(error: unknown, signal: AbortSignal): unknown { | ||
| if (!signal.aborted || isAbortError(error)) return error; |
There was a problem hiding this comment.
When a caller aborts after headers arrive but the body subsequently fails for an independent reason—such as a supported custom fetch whose response later reports a connection error—this condition converts that genuine error into an AbortError merely because the signal is now aborted. Before this change, defaultParseResponse propagated the body failure, so concurrent abort/error races can now hide useful diagnostics; only normalize when the read failure is attributable to the signal rather than every error observed after cancellation.
Useful? React with 👍 / 👎.
| } | ||
| try { | ||
| const composed = nativeAbortSignal.any([controller.signal, callerSignal]) as AbortSignal; | ||
| Object.defineProperty(controller, 'signal', { value: composed, configurable: true }); |
There was a problem hiding this comment.
When a caller of the public fetchWithTimeout method, or a fetchWithAuth override, retains controller.signal before this helper runs, redefining the property means a later caller-signal abort affects only the new composed signal: the retained signal remains un-aborted and its listeners never run. Previously forwarding invoked controller.abort(), so every reference to the controller's stable signal observed the cancellation; use a separate composed request signal without changing the exported controller's identity or semantics.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
aa2a71f to
134352d
Compare
|
Both P2s fixed in Normalise only failures caused by the cancellation. Right — keying on if (!signal.aborted || error !== signal.reason || isAbortError(error)) return error;That is exact rather than heuristic. A read cancelled through the signal rejects with the signal's own reason object, so identity attributes the failure to the cancellation with no false positives. I checked that this holds before relying on it — on Node 24.16, Deno 2.9.4 and Bun 1.2.10, for Preserve the controller's signal identity. Also right, and the fix is to stop redefining anything a caller could already be holding. Composition moved to where the controller is created: const controller = createRequestController(options.signal);The swap happens before the controller is handed to Both are pinned by regressions that I confirmed fail without the fix:
Re-verified after the change: cancellation reporting still matches 1470 tests passing with 15 handwritten abort regressions; |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134352d95a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const composed = nativeAbortSignal.any([controller.signal, callerSignal]) as AbortSignal; | ||
| Object.defineProperty(controller, 'signal', { value: composed, configurable: true }); |
There was a problem hiding this comment.
When an .asResponse() or binary-response caller aborts after headers arrive, the composed signal propagates the caller's reason directly to the Fetch body. On native Fetch runtimes, AbortSignal.timeout() therefore makes a subsequent body read reject with TimeoutError, and abort(new Error(...)) exposes that arbitrary error; before this change, the parameterless forwarding call to controller.abort() consistently produced AbortError. The new normalization covers parsed and SSE responses but not these public raw-body paths, so consumers classifying cancellation by AbortError regress.
Useful? React with 👍 / 👎.
| if (isAbortError(e)) return; | ||
| // A caller-supplied signal aborts the request with its own reason, which is | ||
| // not necessarily an AbortError — `AbortSignal.timeout()` gives a TimeoutError. | ||
| if (controller.signal.aborted || isAbortError(e)) return; |
There was a problem hiding this comment.
When a caller abort races with an independent SSE body failure—for example, a custom fetch response reports a connection reset after the caller signal becomes aborted—this new condition suppresses that real error solely because controller.signal.aborted is true. Unlike the identity check added for parsed bodies, it does not verify that the caught exception was caused by cancellation, so stream iteration can silently end instead of exposing the transport failure; restrict the fallback to the signal's reason or another positively identified cancellation error.
Useful? React with 👍 / 👎.
134352d to
2fb31c9
Compare
|
Unrelated SSE failures after cancellation. Fixed — the same identity test the parsed path got: if (isAbortError(e) || (controller.signal.aborted && e === controller.signal.reason)) return;I'd left AbortError semantics for raw bodies. Confirmed, and reproduced rather than reasoned about.
Identical on Deno. So the regression is real for anyone classifying by What I can't see is a way to normalise it that doesn't cost more than it fixes. The rejection is produced inside the caller's own What's left is a genuine trade, so I've documented it instead of hiding it, in the helper and in the commit message: a caller-owned body now rejects with the caller's own reason, which is what If you'd rather have strict preservation on the raw paths, say so and I'll implement it — it means wrapping the body on Everything re-verified after the change: 1471 tests, 16 handwritten abort regressions, eight of the ten cancellation axes byte-identical to |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
edenbuilds
changed the title
fd020e6 to
186060f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186060fd16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const security = options.__security ?? { bearerAuth: true }; | ||
| const controller = new AbortController(); | ||
| const controller = createRequestController(options.signal); |
There was a problem hiding this comment.
When a subclass or provider replaces req.signal in prepareRequest, this composes the controller with the original options.signal instead; fetchWithTimeout then also forwards the replacement signal because it does not match the WeakMap entry. Consequently both signals can cancel the fetch, whereas previously the finalized req.signal was authoritative, so aborting a signal deliberately replaced by the hook still terminates the request. Compose with req.signal after the preparation hooks instead.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
|
Codex left a large set of iterative P1/P2 notes while this branch evolved. The latest Codex pass on the current head reported no major issues. Happy to address any remaining maintainer feedback; the core fix (compose the caller abort signal instead of leaking a listener on it) is unchanged. |
`fetchWithTimeout` forwarded caller aborts to the request's controller with a listener on the caller's signal. That listener has to outlive the fetch call — `fetch` resolves when headers arrive, so detaching there would cut off mid-stream aborts — and nothing ever removed it. Deno keeps an `AbortSignal.timeout()` timer referenced for as long as its signal is listened to, so the orphaned forwarder held the process open until the timeout fired, long after the request finished: a request completing in ~200ms still pinned the process for 30s (openai#1811). Node and Bun are unaffected; their timers stay unref'd regardless of listeners. `AbortSignal.any` records its result as a dependent of the source signals rather than registering a listener on them, so composing removes the need to observe the caller's signal at all — and with it the need to know when a response body has finished. The composed signal becomes the request controller's signal, so the controller stays the single record of whether a request was cancelled. Both readers of a body depend on that: `Stream` tells cancellation apart from failure with it, and `defaultParseResponse` reports a cancelled read as an `AbortError`, since a composed signal aborts with the caller's reason and `AbortSignal.timeout()` produces a `TimeoutError`. Both normalisations key on identity with `signal.reason` rather than on `signal.aborted`, so a read that failed for its own reason keeps its error even when an abort lands in the same moment. A body the caller owns — `.asResponse()`, `.withResponse()`, binary responses — is read outside the SDK, so its rejection carries the caller's reason as is: an `AbortSignal.timeout()` now surfaces a `TimeoutError` where cancellation was previously flattened to an `AbortError`. That matches what `fetch` gives a caller for the same signal, and normalising it would mean wrapping bodies the SDK never reads. Composition happens where the controller is created, before it is handed to anything, so no existing holder of `controller.signal` is left watching a signal that never aborts, and it composes with `req.signal` rather than `options.signal`: `prepareRequest` hooks run earlier and may have replaced it, and the replacement is what reaches `fetch`. A controller supplied by a caller of the public `fetchWithTimeout` is never modified; those requests forward with a listener as before. Mirroring the abort with a listener on the composed signal was the obvious alternative and does not work: Deno re-references the timer for a listener on a composed signal exactly as it does for one on the signal itself. Runtimes predating `AbortSignal.any` (Deno < 1.38.2, Safari < 17.4; every supported Node has it), and polyfilled or cross-realm caller signals — which `AbortSignal.any` ignores rather than rejecting — fall back to the previous listener, now also removed when the fetch itself fails. Those Deno versions keep hanging: nothing can watch a caller's signal without listening to it. Verified against a local server on Node 24.16, Deno 2.9.4 and Bun 1.2.10, comparing this branch with `main` built from the same tree. Cancellation reporting is identical on both across a parsed-body timeout, a parsed-body abort with a custom reason, the same two mid-stream, `stream.controller.abort()`, the request signal's aborted flag, a pre-aborted signal, and a genuine truncated-body failure; caller-owned raw bodies differ as described above. Parsing, `.asResponse()` identity, `clone()`, BYOB reads, `pipeThrough`, `tee`, SSE drain and early break are also unchanged. On Deno the reproduction exits in 0.84s where `main` takes 30.63s, and a signal reused across twelve requests is left holding twelve listeners on `main` against none here. Fixes openai#1811
186060f to
f648b46
Compare
|
Fixed in
const controller = createRequestController(req.signal);Pinned by Also rebased onto 1478 tests, 17 handwritten abort regressions, |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
fetchWithTimeoutforwards caller aborts to the request's controller by attaching a listener to the caller'sAbortSignal. That listener has to outlive thefetchcall —fetchresolves when headers arrive, so detaching there would cut off mid-stream aborts — and nothing ever removes it.That leaks a listener per completed request, which shows up two ways.
On Deno, the process hangs. Deno keeps an
AbortSignal.timeout()timer referenced for as long as its signal is listened to, so the orphaned forwarder holds the process open until the timeout fires — a request completing in ~200ms still pins the process for 30s (#1811). No SDK needed to show it:On every runtime, listeners accumulate on a reused signal. A request-scoped
AbortControllershared across a sequence of calls — a tool-call loop, a stream plus follow-ups — gains one listener per completed request, until Node warnsMaxListenersExceededWarning: 11 abort listeners added to [AbortSignal]. Reported on this issue as hit in production. Twelve sequential requests through one signal, Node 24.16:mainFix
AbortSignal.anyrecords its result as a dependent of the source signals rather than registering a listener on them. Composing therefore removes the need to observe the caller's signal at all, and with it the need to track when a response body has finished. There is no listener left to clean up, so no cleanup point has to be chosen.The composed signal becomes the request controller's signal, rather than being handed to
fetchon its own, so the controller stays the single record of whether a request was cancelled. Both readers of a body depend on that:Streamtells cancellation apart from failure by reading it;defaultParseResponsereports a cancelled read as anAbortError— a composed signal aborts with the caller's reason, andAbortSignal.timeout()produces aTimeoutError, so without this a plain cancellation would surface as an arbitrary failure.Both of those normalisations key on identity with
signal.reason, not onsignal.aborted: a read cancelled through the signal rejects with the signal's own reason object, so a body that failed for its own reason keeps its error even when an abort lands in the same moment.Composition happens where the controller is created, before it is handed to anything, so no existing holder of
controller.signalis left watching a signal that never aborts. A controller supplied by a caller of the publicfetchWithTimeoutis never modified; those requests forward with a listener as before.Mirroring the abort with a listener on the composed signal was the obvious alternative and does not work: Deno re-references the timer for a listener on a composed signal exactly as it does for one on the signal itself (measured: 30.1s to exit, same as
main).Handwritten footprint: 37 lines of code across three files (103 with the explanatory comments), no new exports, no patched platform methods,
dist/client.d.tsunchanged.Two deliberate limits
Bodies the caller owns.
.asResponse(),.withResponse()and binary responses are read outside the SDK, so their rejection carries the caller's reason as is: anAbortSignal.timeout()surfaces aTimeoutErrorwhere cancellation was previously flattened to anAbortError. That is whatfetchgives a caller for the same signal, and normalising it would mean wrapping bodies the SDK never reads. Happy to add that if preferred — it is the one place strict preservation is still possible.Deno < 1.38.2. Runtimes predating
AbortSignal.any(also Safari < 17.4; every Node this package supports has it), and polyfilled or cross-realm caller signals, keep the previous listener — now also removed when the fetch itself fails. Those signals are ignored byAbortSignal.anyrather than rejected, so they are detected explicitly instead of having their aborts silently dropped. On those Deno versions the hang remains: nothing can watch a caller's signal without listening to it, and detaching before the body ends would cut off mid-stream aborts.Validation
A local server exercised on Node 24.16, Deno 2.9.4 and Bun 1.2.10, against
mainbuilt from the same tree.Cancellation reporting, on both Node and Deno:
mainAbortSignal.timeout()fires mid-bodyAbortErrorAbortErrorabort(new Error(...))mid-bodyAbortErrorAbortErrorstream.controller.abort()stream.controller.signal.abortedafter caller aborttruetrueAPIUserAbortErrorAPIUserAbortErrorTypeErrorTypeError.asResponse()body, caller timeout mid-readAbortErrorTimeoutError.asResponse()body, custom reason mid-readAbortErrorErrorThe last two are the documented limit above; the other eight are identical.
Process exit time for the reproduction (12 requests using
AbortSignal.timeout(30s)):mainAlso unchanged on all three runtimes: parsing,
.asResponse()identity and own-property shape,clone(), BYOB reads (skipped on Bun, whose fetch bodies aren't byte streams even for a plainfetch),pipeThrough,tee, full SSE drain, SSE earlybreak, and error statuses.tests/abort-forwarder.test.tstsc --noEmit,ultracite check,npm run buildcleanRelated issue
Fixes #1811