Fix multipart uploads on retry#3484
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c842c4440
ℹ️ 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".
|
|
||
| buffered = cast(IO[bytes], tempfile.SpooledTemporaryFile(max_size=_SPOOLED_FILE_MAX_SIZE)) | ||
| try: | ||
| shutil.copyfileobj(file, buffered) |
There was a problem hiding this comment.
Avoid spooling when retries are disabled
Because ReplayableFiles is constructed unconditionally before _request_with_retries reads max_retries, this copy now drains every non-seekable multipart upload into a temporary file even when the caller set max_retries=0 and is using normal API-key auth, where there is no retry path to replay. Large pipe/socket/generated uploads that previously streamed directly can now block until EOF or fill temporary storage; please only spool non-seekable streams when a retry or auth-retry can actually occur.
Useful? React with 👍 / 👎.
Changes being requested
Multipart requests currently reuse file objects across retries without rewinding them. A non-seekable stream is therefore empty on the second attempt, and a successful retry can make the upload appear to have succeeded with an empty file.
This change makes multipart bodies replayable for both sync and async clients:
Regression coverage exercises HTTP 500 responses, connection errors, timeouts, seekable streams, disabled retries, and workload identity 401 refreshes in both sync and async paths.
Additional context & links
Validated with the full client test module (187 tests), Ruff, Pyright, and mypy.