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
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.
Route body timeouts through retry handling
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.
Guard custom fetch bodies before calling getReader
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 👍 / 👎.
|
Thanks for investigating stalled response bodies! A complete fix needs coordinated SDK response parsing, timeout classification, and retry handling; the current implementation bypasses retries after headers arrive and also needs to preserve custom fetch compatibility. I’m moving this to SDK triage so the complete response lifecycle can be reviewed together. |
|
Thanks for investigating this; stalled response bodies are a real bug. I traced the current request lifecycle to the shared Castiron TypeScript client template, which clears its timeout as soon as fetch resolves and owns timeout classification and retries. Fixing this safely therefore requires coordinated upstream changes across generated request retries, response parsing, and streaming rather than an SDK-local response wrapper. The proposed wrapper also bypasses APIConnectionTimeoutError/retries after headers, eagerly consumes raw responses, breaks custom fetch bodies without getReader(), and can silently truncate SSE streams. I am marking this upstream so the complete fix can be made in Castiron and propagated safely. |
|
Thanks for investigating this. The canonical response-body timeout fix has now merged upstream in the shared TypeScript client generator, including timeout classification and retry handling while preserving raw responses, streaming, and custom fetch behavior. Because src/client.ts is generated, closing this SDK-local proposal as superseded. The fix will be available after SDK regeneration and release. |
Summary
Tests
Fixes #1825