fix(client): clean up abort listener in fetchWithTimeout to prevent memory leak - #2151
fix(client): clean up abort listener in fetchWithTimeout to prevent memory leak#2151lchetanlc wants to merge 2 commits 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: 4bef397376
ℹ️ 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 await this.fetch.call(undefined, url, fetchOptions); | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| if (signal) signal.removeEventListener('abort', abort); |
There was a problem hiding this comment.
When a request uses { stream: true } or has a slow/large response body, fetch() resolves as soon as headers arrive, but the SDK consumes the body later via defaultParseResponse() / Stream.fromSSEResponse() using this same internal controller. Removing the caller's abort listener here means a documented request-option AbortSignal aborted after headers are received no longer aborts the in-flight body/stream, so long-running streams can continue indefinitely unless the user also has access to and calls stream.abort().
Useful? React with 👍 / 👎.
|
@chatgpt-codex-connector Great catch! I have updated the PR. Instead of eagerly removing the listener in For standard responses, it cleans up immediately after the body is parsed. For streams, it passes the cleanup hook down to the |
|
Note To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a6a077f94
ℹ️ 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".
| controller: AbortController, | ||
| client?: OpenAI, | ||
| synthesizeEventData?: boolean, | ||
| cleanup?: () => void, |
There was a problem hiding this comment.
For stream: true requests using a request-option signal (especially AbortSignal.timeout() in Deno), makeRequest now installs the abort listener and defaultParseResponse passes the cleanup callback here, but fromSSEResponse never invokes cleanup. After the SSE stream ends, is broken, or errors, the user's signal still retains the listener, so the memory/process-liveness leak this change is meant to fix remains for streaming responses; run the callback from the iterator's finally once the response is no longer being consumed.
Useful? React with 👍 / 👎.
| const abort = this._makeAbort(controller); | ||
| let abortCleanup: (() => void) | undefined; | ||
| if (options.signal) { | ||
| options.signal.addEventListener('abort', abort); |
There was a problem hiding this comment.
Because this listener is installed for every attempt but only the final successfully parsed response returns its actionToRunOnComplete to defaultParseResponse, retry and terminal error paths in makeRequest never remove it. With a request-option AbortSignal.timeout() that gets a retryable 429/500 (or a final API/connection error), the earlier attempt's listener stays attached and Deno can still remain alive until the timeout; call the cleanup in those paths before retryRequest or throwing.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| return { response, options, controller, requestLogID, retryOfRequestLogID, startTime }; | ||
| return { response, options, controller, actionToRunOnComplete: abortCleanup, requestLogID, retryOfRequestLogID, startTime }; |
There was a problem hiding this comment.
For successful requests that return a Response instead of parsed data—APIPromise.asResponse() and __binaryResponse resources such as files/audio—the only cleanup handle is stored in APIResponseProps and later invoked by defaultParseResponse. Those paths either bypass parsing or explicitly skip the cleanup, so a request with AbortSignal.timeout() can still keep Deno alive after the caller has consumed the response; remove the listener when the returned response body is consumed or cancelled.
Useful? React with 👍 / 👎.
1 participant
Fixes #1811
Description
In
fetchWithTimeout, an event listener is added to the user-providedAbortSignalso the SDK can abort the internal fetch request if the user triggers their signal.However, this event listener was never removed upon successful completion of the request. While Node.js ignores
AbortSignaltimers for process lifecycle management, strict runtimes like Deno keep the process alive as long as anAbortSignal.timeout()has an active listener attached.This resulted in SDK calls hanging the entire process until the full timeout duration expired, even if the API responded successfully within milliseconds.
Fix
This PR simply adds
if (signal) signal.removeEventListener('abort', abort);to thefinallyblock offetchWithTimeoutto clean up the orphaned listener, allowing strict JavaScript runtimes to exit cleanly.Testing
pnpm test.