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

daniel-sanche · 2026-09-14T22:15:51Z

+            data = self._buffered_chunk
+            data_len = len(data)
+
+            is_last = data_len < chunk_size


Gemini says there is a data-loss issue here. It says there are cases where data_len < chuck size without it being the last chunk, like if it retries after a partial upload. Can you verify that?

daniel-sanche · 2026-09-14T22:24:49Z

+            if captured:
+                while captured:
+                    yield captured.pop(0)
+


I find the scope of captured very confusing. The same reference is used as a class attribute, context manager, and local variable

I was very confused reading this methodf for the first time, because I wasn't sure how captured gets populated. But after a while, I realized it's another way of accessing of self._captured_progress. I think we should avoid that

daniel-sanche · 2026-09-14T22:26:55Z

+            if computed_size is None:
+                computed_size = len(stream)
+        elif not hasattr(stream, "read") and isinstance(stream, Iterable):
+            stream_obj = io.BytesIO(b"".join(stream))


This seems to load the whole stream into memory, right? I thought the purpose of using an iterator here would be to avoid that

daniel-sanche · 2026-09-14T22:35:32Z

+        recovery_loop = google.api_core.retry.Retry(
+            predicate=lambda e: isinstance(e, _RecoveryRetransmit)
+        )
+        return recovery_loop(do_transmit)()


All these nested retries make me nervous. Especially since some of these retries are managed by the user, and some are hard-coded. It looks like the request is set up like this:

iter_upload() / upload() │ ├── 1️⃣ [LAYER 1: Start Request Retry] (upload.py:L463) │ initiate() │ └── _get_retry(is_start=True)(do_initiate) <-- Retries initial POST /start │ └── 5️⃣ [LAYER 5: Manual Chunk Loop] (upload.py:L658) _transmit_all_chunks() └── while not finished: _transmit_chunk() │ └── 4️⃣ [LAYER 4: Recovery Retransmit Loop] (upload.py:L579-L582) recovery_loop = Retry(predicate=_RecoveryRetransmit)(do_transmit) └── do_transmit() │ ├── 2️⃣ [LAYER 2: Chunk HTTP Retry] (upload.py:L534) │ _get_retry()(do_http) <-- Retries POST chunk on 503/timeout │ └── on 400/409/412 or missing header: _recover() │ └── 3️⃣ [LAYER 3: Query HTTP Retry] (upload.py:L609) _get_retry()(do_query) <-- Retries POST query on 503 # Then raises synthetic exception: raise _RecoveryRetransmit() <-- Bounces back to Layer 4!

Can we try to flatten this? This seems like a place to use StreamingRetry, instead of trying to wrap each request

daniel-sanche · 2026-09-14T22:37:25Z

+                    raise
+
+            await asyncio.sleep(delay)
+            delay = min(delay * multiplier, max_delay)


Why are you re-implementing async_retry here? Why not use the one in api_core?

daniel-sanche · 2026-09-14T22:40:39Z

+        Raises:
+            Exception: Re-raises any exception encountered during the background transfer.
+        """
+        while True:


this looks risky. If the _progress_queue isn't updated properly, this method will hang

Gemini points out this can happen if the task is ended with a BaseException, like asyncio.CancelledError. Those are never inserted into the queue

daniel-sanche · 2026-09-14T23:48:22Z

+    retry: Optional[google.api_core.retry.Retry] = None
+    on_progress: Optional[Callable[[common.UploadProgress], None]] = None
+    response_type: Optional[Any] = None
+    content_type: Optional[str] = None


Do we have to store all of this on a shared config class?

It seems like some of these would be better off as method arguments (eg start_retry vs retry, on_progress, etc). They don't all apply equally to sync vs async, and initialize vs upload

daniel-sanche · 2026-09-14T23:50:55Z

+                progress_queue.put_nowait(exc)
+                raise
+
+        task = asyncio.create_task(_run())


This seems surprising to me. upload is a sync method, that starts a background task?

I'd expect upload to return the awaitable, and then the user can choose to await it directly, or create a background task

daniel-sanche · 2026-09-15T00:16:21Z

+        stall_minimum_rate: Minimum transfer rate in bytes per second. Defaults to 64 KiB/s.
+        stall_timeout: Stall duration threshold in seconds. Defaults to 120s.
+        headers: Additional HTTP headers dispatched exclusively with start request.
+        deadline: Overall global deadline for the upload process.


It seems like the deadline isn't strictly enforced, right? We only check the deadline after a failure, but let healthy streams keep going?

We should make sure that's what we want, and then document it

daniel-sanche · 2026-09-15T00:20:37Z

+    Attributes:
+        chunk_size: Size in bytes for each uploaded data chunk. Defaults to 10 MiB.
+        start_timeout: Local per-request timeout in seconds for start request.
+        start_retry: Custom retry policy for the start request.


I wonder if we actually want to expose the full retry object to end-users? Is it ok if they use custom predicates, or could that break stuff? (Gemini seems to think this could break some internal logic)

Maybe we should encapsulate some of the retry details? Or raise an exception if they change non-backoff fields? (Maybe we should make a simplified version of the retry config?)

daniel-sanche · 2026-09-15T01:20:35Z

+    headers: Optional[Union[Mapping[str, str], Sequence[Tuple[str, str]]]] = None
+    deadline: Optional[datetime.datetime] = None
+    timeout: Optional[float] = None
+    retry: Optional[google.api_core.retry.Retry] = None


Is it fully safe to accept user-specified retry configs here? what if they choose different retryable errors than we expect?

daniel-sanche · 2026-09-15T02:10:56Z

+                >= self._config.stall_timeout
+            ):
+                self._get_deadline_remaining()
+                raise exceptions.TransferStalledError(


Are you sure this is right? It looks like any stall will be raised as a terminal error, and not be retried. Is that part of the spec?

[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

daniel-sanche reviewed Sep 15, 2026

View reviewed changes

Uh oh!

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!

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 14, 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!

daniel-sanche Sep 15, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniel-sanche Sep 14, 2026 •

edited

Loading

daniel-sanche Sep 15, 2026 •

edited

Loading