fix(client): retry only replayable request content - #3771
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
A one-shot After the first request consumes that internal iterator, Could we either treat generic iterables conservatively unless repeatability is known, or add a regression case with an iterable wrapper whose |
|
@sylvesterkaczmarek Good catch — the stored-generator wrapper reproduced the silent empty-success retry. I addressed it in 53256c2 by treating opaque sync and async iterables as non-replayable unless their repeatability is known here; exact list/tuple containers remain retryable. The parameterized regression now covers stored sync/async iterators across status, timeout, and connection retries. Both Pydantic v1 and v2 client suites pass (213 passed, 2 skipped each), and the full lint/type-check suite passes.\n\n@codex review\n@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
|
Thanks for checking this edge case. I rechecked the current head ( The regression already includes both synchronous and asynchronous one-shot wrappers whose |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
_RequestContentReplay only checks options.content; multipart options.files is ignored. A non-seekable file upload has content=None, so the client still retries after the first attempt consumes the file and can send an empty/partial upload. Include file parts in replayability/rewind logic, or disable retries when any upload part is non-seekable, and add a multipart regression.
Changes being requested
Raw streaming request bodies are currently reused across automatic retries. A one-shot iterable is exhausted after the first attempt, so a 500, timeout, or connection error can be followed by an empty retry. If that empty retry succeeds, the SDK reports success even though it did not resend the payload. Seekable file-like content has the same issue because its position is not restored.
This change keeps retry behavior correct without eagerly buffering large request bodies:
The regression tests cover retryable status responses, timeouts, and connection errors for sync and async one-shot content, compatibility for exact list/tuple containers, plus original-offset restoration for seekable content.
Additional context & links
The original regression cases fail on main and pass after this change.
Validation: