feat(google-api-core): add support for resumable uploads by parthea · Pull Request #18352 · googleapis/google-cloud-python

parthea · 2026-09-11T21:43:39Z

Towards b/457416314, b/556259599

gemini-code-assist

Code Review

This pull request introduces a new resumable transfer library for Google APIs, implementing both synchronous (using requests) and asynchronous (using aiohttp) resumable upload sessions, supported by a sans-I/O protocol state machine and comprehensive tests. The feedback highlights several key areas for improvement: ensuring backward compatibility with Python 3.7/3.8 by replacing asyncio.to_thread with loop.run_in_executor, handling byte-type header keys in the state machine, rejecting unsupported str and dict stream types early, retrying timeouts globally in the synchronous session, and removing redundant deadline checks.

…load_state.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…load_async.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…load.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

daniel-sanche

I'm still digesting this, but giving a quick first round of comments

…er-api-core

daniel-sanche

Its looking really close, but a couple more comments

daniel-sanche · 2026-09-21T19:35:54Z

+            config: Optional upload configuration parameters. Defaults to
+                ``ResumableUploadConfig()`` when ``None``.
+            resumable_url: Pre-existing upload session URL if resuming. When
+                ``None``, a new session is created during ``upload()``.


nit: what happens if both upload_url and resume_url are passed? Should we collapse these into a single argument?

Done in 2223340.

daniel-sanche · 2026-09-21T19:40:06Z

+    @property
+    def upload_url(self) -> Optional[str]:
+        """Optional[str]: The unique upload URL for this session."""
+        return self._state.resumable_url


Maybe it's just me, but I find it confusing that the state holds both an upload_url and a resumable_url, but this upload_url proprety returns the resumable_url

Agreed. Resolved in 2223340. resumable_url was removed.

daniel-sanche · 2026-09-21T19:47:59Z

+                initiate server offset recovery. Terminal errors
+                (``DeadlineExceeded``, ``TransferStalledError``,
+                ``UploadCancelledError``, and ``UnseekableStreamError``) are never
+                retried.


Can the docstring mention that the custom predicate completely repplaced the default transient error logic?

Without reading the internals, I'd be wondering how this is composed with the default logic (i.e. AND/OR/replacement)

Done in e85aa4b

daniel-sanche · 2026-09-21T19:53:33Z

+
+
+class ProtocolState(object):
+    """Encapsulates the state and command formatting for Resumable Upload protocol."""


Does this need to be public?

No, it's not used directly in the generated code. I'll move it to internal

Done in 2736d95

daniel-sanche · 2026-09-21T20:23:09Z

+            UploadProgress snapshots for each progress transition.
+        """
+        if not self._consumed:
+            self._consumed = True


It looks like there's a flaw here: The operation is marked as consumed as soon as the generator is started. But its possible for the user to yield a couple items from the generator, and then await the rest of the operation. That would lead to a broken state.

We should probably only mark this as consumed after the hitting the end of the stream (if I understand correctly)

Fixed 9f0c894

daniel-sanche · 2026-09-21T20:29:11Z

+            req_headers[common.HEADER_CONTENT_LENGTH] = str(size)
+
+        payload = body.encode("utf-8") if isinstance(body, str) else body
+        return "POST", self._initial_url, req_headers, payload


gemini points out a flaw here: This is always called, whether the upload was started with a upload_url, or resume_url. But this code assumes the upload_url is present, and doesn't take the resume_url into account

Maybe the idea is that upload() should never be called with a resume url? But the docstrings don't make that clear, and it doesn't seem like an error would be handled cleanly

I think we should be clearer about how these urls are handled, since we can't change the API in the future

Good catch! I've pushed 2 fixes in e275716:

Removed the separate resumable_url parameter so __init__ only takes upload_url which removes the confusion.

Added validation in .build_start_request() to raise ValueError("upload_url must be provided to start an upload.") when self._initial_url is missing (matching build_chunk_request(), build_query_request(), and
build_cancel_request() which already had this check).

daniel-sanche · 2026-09-21T20:31:52Z

+                request_body=request_body,
+                size=computed_size,
+                progress_queue=progress_queue,
+            )


Personally, I think a lot of complexity would go away if we kept initiate as a required, public call. We could keep all the arguments separate from other methods, and well scoped.

But if this is how other languages handle it, it should be fine

daniel-sanche · 2026-09-21T22:02:21Z

+            return retry_override.with_predicate(wrapped_pred)
+        return google.api_core.retry.StreamingRetry(
+            predicate=self._get_retry_predicate(is_start=False)
+        )


Doesn't this mean we inherit the default deadline of 120 seconds?

Great catch! For uploads taking longer than 120 seconds, any transient error after the 120-second mark would immediately hit RetryError instead of recovering.

I updated _get_streaming_retry in upload.py and _get_async_streaming_retry in upload_async.py to set timeout=None on the default StreamingRetry / AsyncStreamingRetry instances so that overall transfer deadlines and stall detection are controlled by ResumableUploadConfig.deadline and stall_timeout / stall_minimum_rate (along with per-attempt chunk timeouts).

Fixed in b49b94c

daniel-sanche · 2026-09-21T22:09:28Z

+        Raises:
+            ValueError: If required arguments are missing or response not received.
+            GoogleAPICallError: If an unrecoverable API error occurs.
+        """


Do we need to clear self._buffered_chunk or any other state data? (_stall_timeout_started? _aggregate_lag)

Fixed in 369a790

daniel-sanche · 2026-09-21T22:10:23Z

+            1.0,
+            expected_sec - self._aggregate_lag + self._config.stall_timeout,
+        )
+        per_attempt_timeout = max(5.0, min(next_chunk_timeout, 2.0 * expected_sec))


Why is this being clamped to 5 seconds? Doesn't this kind of remove the benefit of timeout_override, if it can only be 5 seconds max?

daniel-sanche · 2026-09-21T22:14:47Z

+
+        if self._aggregate_lag > 0.0:
+            if self._stall_timeout_started is None:
+                self._stall_timeout_started = t_start


I added this as a child comment in a different thread, but to make sure it's not lost: shouldn't we only count current_lag as stall time? This seems to treat the entire operation as time spent stalled, even if it's just slightly over

This was a bug. I responded in #18352 (comment) and fixed the issue in e29f1cb

[DRAFT] feat: add support for resumable uploads

7492d88

gemini-code-assist Bot reviewed Sep 11, 2026

View reviewed changes

parthea changed the title ~~[DRAFT] feat: add support for resumable uploads~~ [DRAFT] feat(google-api-core): add support for resumable uploads Sep 11, 2026

parthea and others added 9 commits September 11, 2026 21:58

lint

326ae78

lint

9936e0d

mypy

9e689b8

lint

9c49a81

address review feedback

b0de580

address review feedback

ae11c27

Update packages/google-api-core/google/api_core/resumable_transfer/up…

f57c598

…load_state.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/google-api-core/google/api_core/resumable_transfer/up…

efbc623

…load_async.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Update packages/google-api-core/google/api_core/resumable_transfer/up…

41a3759

…load.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

parthea changed the title ~~[DRAFT] feat(google-api-core): add support for resumable uploads~~ feat(google-api-core): add support for resumable uploads Sep 14, 2026

parthea added 10 commits September 14, 2026 17:35

lint

405396e

additional headers->headers

5409b00

lint

0e0ad52

cover

3936458

cover

8c9c900

cover

96f3114

lint

59de7db

cover

e13ff31

lint

c9fbb46

mypy

5bd4bec

parthea marked this pull request as ready for review September 14, 2026 19:39

parthea requested a review from a team as a code owner September 14, 2026 19:39

daniel-sanche requested changes Sep 14, 2026

View reviewed changes

daniel-sanche reviewed Sep 14, 2026

View reviewed changes

Comment thread packages/google-api-core/google/api_core/resumable_transfer/upload.py Outdated

daniel-sanche reviewed Sep 14, 2026

View reviewed changes

Comment thread packages/google-api-core/google/api_core/resumable_transfer/upload_async.py Outdated

daniel-sanche reviewed Sep 15, 2026

View reviewed changes

Comment thread packages/google-api-core/google/api_core/resumable_transfer/upload.py Outdated

daniel-sanche reviewed Sep 15, 2026

View reviewed changes

Comment thread packages/google-api-core/google/api_core/resumable_transfer/upload.py Outdated

parthea added 15 commits September 18, 2026 14:16

fix build

7aeef14

remove duplicate arguments in initiate

8322a1e

address review feedback

546f206

address review feedback

c6289da

adjust formatting

7824062

address review feedback

c17a7d5

Merge remote-tracking branch 'origin/main' into feat/resumable-transf…

0b1960a

…er-api-core

address review feedback

4dd0b3e

address review feedback

a3c1433

address review feedback

512d44a

address review feedback

61853c9

address review feedback

fcb7990

address review feedback

f31e693

address review feedback

3a74227

address review feedback

c76964d

daniel-sanche reviewed Sep 21, 2026

View reviewed changes

parthea added 10 commits September 21, 2026 23:45

address review feedback

2223340

update docstring

e85aa4b

add underscore prefix to classes

2736d95

address review feedback

9f0c894

clean up

512e282

address review feedback

e275716

revert

2bebc58

address review feedback

e29f1cb

address review feedback

b49b94c

address review feedback

369a790



		class ProtocolState(object):
		"""Encapsulates the state and command formatting for Resumable Upload protocol."""

Conversation

parthea commented Sep 11, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

daniel-sanche Sep 21, 2026 •

edited

Loading

daniel-sanche Sep 21, 2026 •

edited

Loading