Repository navigation
feat(storage): stream resumable file uploads - #59
Conversation
|
@codex review |
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. |
![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: 75b6a915c8
ℹ️ 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".
| session_id=session.session_id, | ||
| part_number=part_index + 1, | ||
| data=data[offset : offset + session.part_size], | ||
| data=source.read(session.part_size), |
There was a problem hiding this comment.
When a seekable binary stream returns fewer than the requested bytes before EOF—which is valid for file-like objects—this advances to the next part number after that short read. The resulting non-final part can violate the API's minimum part-size requirement, and the fixed total_parts loop may stop while unread data remains, causing the upload to fail or assemble incomplete content. Accumulate reads until session.part_size bytes have been collected or true EOF is reached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd90802. Part reads now accumulate until the server-selected size is filled or the source reaches EOF, with regression coverage for legal short reads.
| while chunk := source.read(_UPLOAD_SPOOL_READ_SIZE): | ||
| target.write(chunk) |
There was a problem hiding this comment.
For a non-seekable source backed by a nonblocking binary stream, read(size) may return None when no data is currently available, but this truthiness loop treats that result as EOF. If it occurs after reading a prefix, the SDK creates and completes a session containing only that prefix even though more bytes may arrive, silently truncating the upload; distinguish b"" from None and either wait, retry, or raise instead of finalizing the spool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd90802. A None read now raises BlockingIOError instead of being treated as EOF, preventing silent truncation; the behavior has focused regression coverage.
| raise | ||
| return self.complete_upload_session(path, session_id=session.session_id) | ||
| """Upload bytes or a binary stream through a resumable session.""" | ||
| with _resumable_upload_source(data) as (source, total_size): |
There was a problem hiding this comment.
For a non-seekable input, entering this context drains the entire source into a temporary file before create_upload_session() validates the path or checks for an active session. Consequently, an invalid path or unauthenticated call with a large or live stream can consume unbounded disk space or block indefinitely before raising an error that was knowable immediately; validate those local preconditions before entering the spooling context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed the valid local portion in bd90802. Path and active-session authentication are validated before a non-seekable source is spooled. The server-side active-upload check remains in createUploadSession because total_size is required by that wire operation and is unknowable until spooling completes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd90802cee
ℹ️ 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".
| ) | ||
| try: | ||
| self._upload_session_parts(path, source, session) | ||
| except (OSError, TypeError, ValueError, VolcanoError): |
There was a problem hiding this comment.
For a custom BinaryIO whose read() raises an exception outside this allowlist (for example, RuntimeError) after the upload session has been created, upload_resumable() propagates the error without calling _abort_failed_upload(). This leaves the server-side session and any uploaded parts active even though the convenience operation has failed; catch arbitrary Exception around the part-upload loop while still re-raising the original error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6d2f4b8. A try/finally success guard now aborts the session for any exception raised while reading or uploading parts, without catching or replacing the original exception.
There was a problem hiding this comment.
Correction: the fix is in 5ef28e9 (the prior reply contained a mistyped short SHA).
| if not source.seekable(): | ||
| return None |
There was a problem hiding this comment.
seekable() as non-seekable
For a non-seekable binary file-like object that exposes bounded read() but does not inherit IOBase and therefore has no seekable() method, this unconditional probe raises AttributeError instead of using the advertised spooling path. This affects common read-only stream wrappers such as HTTP response bodies; treat an absent seekability probe as non-seekable so these sources can be spooled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6d2f4b8. Missing seekability probes are treated as non-seekable and use the bounded spooling path, with regression coverage for a duck-typed read-only stream.
There was a problem hiding this comment.
Correction: the fix is in 5ef28e9 (the prior reply contained a mistyped short SHA).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ef28e9b1b
ℹ️ 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".
| upload_succeeded = True | ||
| finally: | ||
| if not upload_succeeded: | ||
| self._abort_failed_upload(path, session.session_id) |
There was a problem hiding this comment.
When a stream read fails and the best-effort abort raises something other than VolcanoError—for example, _session_token() raises RuntimeError after a concurrent sign-out—this finally block replaces the original reader error with the cleanup error because _abort_failed_upload() suppresses only VolcanoError. The fresh evidence in this revision is the new unconditional cleanup path for arbitrary reader exceptions; suppress ordinary abort-time exceptions so the original upload failure remains observable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11bf76a. Best-effort abort now suppresses ordinary cleanup exceptions, and regression coverage proves the original reader failure remains observable when abort itself raises.
There was a problem hiding this comment.
Correction: the fix is in 4f8f7d2 (the prior reply contained a mistyped short SHA).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f8f7d26b1
ℹ️ 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".
| position = source.tell() | ||
| try: | ||
| source.seek(0, SEEK_END) | ||
| return source.tell() - position |
There was a problem hiding this comment.
When a valid seekable stream is positioned beyond EOF (for example, BytesIO(b"a").seek(2)), the end position is smaller than the current position, so this returns a negative size. upload_resumable() then sends that negative total_size when creating the session instead of treating the stream's readable remainder as an empty upload; clamp the difference to zero.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2eb8e5f. Remaining readable size is now clamped to zero when the cursor is beyond EOF, with focused regression coverage.
| finally: | ||
| source.seek(position) | ||
| except (AttributeError, OSError, ValueError): | ||
| return None |
There was a problem hiding this comment.
When an ostensibly seekable stream reaches EOF during the size probe but seek(position) then raises OSError or ValueError—for example, a remote range-backed stream whose restore operation fails—the outer handler converts that restoration failure into the non-seekable fallback even though the cursor is still at EOF. The subsequent spool can therefore create and complete an empty upload instead of surfacing the I/O failure; only fall back when the original position is known to be intact, and otherwise propagate the restoration error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2eb8e5f. The size probe now restores the original cursor in a finally block; restoration failures propagate, and fallback spooling occurs only after a successful restore.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
1 participant
Summary:
Scope:
Python facade-only input handling. No OpenAPI or Hosting behavior changes.
Verification: