fix(client): apply timeout to response body reads#1908
Conversation
Previously the request timeout was cleared as soon as response headers arrived, leaving the body read (response.json(), streaming, etc.) unguarded. A server that sent headers and then stalled mid-body could hang the SDK indefinitely. The abort timer now stays armed through the body read and is re-armed on each chunk, so a stalled body is aborted after the configured timeout of inactivity while a steadily-streaming response is never cut off. The timer is cleared once the body is fully read, cancelled, or errors. Fixes openai#1825
![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: 2714d71129
ℹ️ 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".
| rearmTimeout(); | ||
| const { done, value } = await reader.read(); |
There was a problem hiding this comment.
When the server sends headers and then stalls the body, the new timer now fires from this pull() after makeRequest() has already returned a successful APIResponseProps, so the existing timeout classification/retry path around fetchWithAuth() is bypassed. With the default maxRetries, a stalled JSON or SSE body will surface as the raw abort/read error and will not be retried or normalized as APIConnectionTimeoutError, even though the client documents request timeouts as retryable.
Useful? React with 👍 / 👎.
| // leave an active timer behind. | ||
| clearCurrentTimeout(); | ||
|
|
||
| const reader = response.body.getReader(); |
There was a problem hiding this comment.
In Node 20+ globalThis.ReadableStream exists, so any custom fetch that returns a node-fetch-style response with a Node Readable body reaches this path, but those bodies do not implement getReader(). Previously such responses could still be parsed via their own Response.text()/json() methods; now the request throws TypeError: response.body.getReader is not a function before returning, so custom fetch implementations with non-WHATWG bodies need to be left unwrapped or checked for getReader first.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests
Fixes #1825