fix(client): retry only replayable request content - #3771
Hughhhhcoder wants to merge 6 commits into
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
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.
|
Addressed in commit |
|
@codex 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. Hooray! 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 the follow-up. Both cases are covered on the current head: commit Validation on the current head: the focused |
|
Synced this branch with the latest upstream Validation after the sync:
|
|
@codex 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. Can't wait for the next one! 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". |
![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: b5876c89cb
ℹ️ 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".
| request_body_replays = [ | ||
| _RequestContentReplay(input_options.content), | ||
| *(_RequestContentReplay(content) for content in _iter_file_contents(input_options.files)), | ||
| ] |
There was a problem hiding this comment.
When a subclass or provider's _prepare_options() replaces a replayable input body with a cached one-shot iterator or non-seekable file, these trackers still describe the original input_options; because preparation happens later, a retryable failure can trigger a second attempt using the exhausted transformed body and even report success after sending an empty payload. The async path has the same ordering. Derive the replay state from the prepared options actually used for the first attempt, or otherwise ensure preparation cannot invalidate it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 50fb1224. The replay trackers are now created after _prepare_options() for each attempt, so they describe the content and multipart files actually passed to _build_request() rather than the original input options. I added sync and async regressions where the preparation hook replaces replayable bytes with the same cached one-shot iterable; both now stop after the first failed attempt instead of reusing the exhausted transformed body.
Validation after the change:
PYTHONPATH=src ./scripts/test tests/test_client.py: 275 passed, 2 skipped in each Pydantic lanePYTHONPATH=src ./scripts/lint: Ruff, Pyright, mypy, and import checks passedgit diff --check: passed
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed current 50fb1224. The remaining preparation-order issue is resolved: replay state is now derived from the prepared options immediately before _build_request(), so a provider hook that substitutes a one-shot body is treated as non-replayable on the actual attempt. The added sync and async regressions cover that transformation directly. Together with the existing multipart rewind/non-seekable coverage, I do not see a remaining replay-safety blocker.
2 participants
Changes being requested
Automatic retries can reuse a consumed request body. A one-shot iterable or non-seekable multipart file part may then send an empty body on the retry; if that retry succeeds, the SDK reports success without resending the original payload. Seekable streams also need their original offset restored. The expected behavior is to retry only when every body part can be replayed.
The retry paths currently decide whether to retry based on response status or request failure, but do not check the replayability of
options.contentandoptions.filesor rewind seekable parts. This patch adds one replayability guard for both synchronous and asynchronous clients:The patch does not buffer request bodies or change the public API. #3484 proposes spooling non-seekable multipart streams to preserve retries; this PR takes the smaller, conservative approach of not retrying a consumed stream. The multipart checks here were added in response to review feedback, so they are no longer out of scope.
Additional context & links
The one-shot-content and multipart regression cases fail on the earlier base and pass with this change. The branch has been normally merged with current
main, retaining the separate retry-limit and application-error changes from #3867.Validation after that merge:
PYTHONPATH=src ./scripts/test tests/test_client.pywith the repository's pinned Steady mock server: 275 passed, 2 skipped in each Pydantic lane;PYTHONPATH=src ./scripts/lint: Ruff, Pyright, mypy, and import checks passed using the documented Node.js and pnpm versions;git diff --check: passed.