feat(storage): support retries with sink uploads by brianquinlan · Pull Request #236 · googleapis/google-cloud-dart

brianquinlan · 2026-04-14T18:29:31Z

Fixes #218

Adds ifGenerationMatch and retry parameters to uploadObjectFromSink.

Part of the complexity of the retry strategy is that it is possible for Google Cloud Storage to only acknowledge part of the upload and uploads must be done in 256KB chunks.

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

gemini-code-assist

Code Review

This pull request significantly improves the resumable upload implementation for Google Cloud Storage by introducing a robust chunking mechanism that correctly handles 308 status codes, server-side range tracking, and automated retries. It also adds support for idempotency through generation matching and refactors internal buffering for better efficiency. Review feedback identifies a logic error in the finalization of uploads when receiving a 308 response, suggests using memory-efficient views instead of copying data during chunking, and points out a bug in the manual calculation of the Content-Length header for metadata.

gemini-code-assist · 2026-04-14T18:31:55Z

+          if (isClose && parsed != null && parsed == newEnd) {
+            throw StateError(
+              'Server returned 308 but all bytes were sent and acknowledged.',
+            );
+          }
+
+          if (parsed != null) {
+            loopExpectedByte = parsed;
+            currentExpectedByte = parsed;
+            final expectedEnd = _nextExpectedByte + chunk.length;
+            if (loopExpectedByte >= expectedEnd) {
+              return res;
+            }


The current logic throws a StateError or returns a 308 response when a chunk upload is completed but not yet finalized by the server (i.e., when isClose is true). For resumable uploads, the final request must return a 2xx status code containing the object metadata. If the server returns a 308 even after all bytes are acknowledged, the client should proceed to send a finalization request (using bytes */total) instead of throwing an error or returning the 308 response.

if (parsed != null) { loopExpectedByte = parsed; currentExpectedByte = parsed; final expectedEnd = _nextExpectedByte + chunk.length; if (!isClose && loopExpectedByte >= expectedEnd) { return res; } }

natebosch · 2026-04-14T21:25:32Z

+/// > not assume that the server received all bytes sent in any given request.
+int? _parseRange(String? rangeHeader) {
+  if (rangeHeader == null) return null;
+  final match = RegExp(r'bytes=0-(\d+)').firstMatch(rangeHeader);


[optional] Gemini tells me that GCS does always use 0- ranges (I didn't look it up to confirm), but I wonder it it makes sense to be target the more general spec anyway?

Suggested change

final match = RegExp(r'bytes=0-(\d+)').firstMatch(rangeHeader);

final match = RegExp(r'bytes=\d+-(\d+)').firstMatch(rangeHeader);

This header is GCS' acknowledgment of the bytes that it has received. The documentation doesn't describe how to interpret the lower value

Repeat the above steps for each remaining chunk of data that you want to upload, using the upper value contained in the Range header of each response to determine where to start each successive chunk; you should not assume that the server received all bytes sent in any given request.

If the lower value is non-zero then I treat it like the "Range" header was absent, which means that:

in a check request, assume that all bytes must be resent

in an upload request, assume that the state of the upload is unknown and do a check request

natebosch · 2026-04-14T21:47:08Z

+          if (statusRes.statusCode == 308) {
+            loopExpectedByte = _parseRange(statusRes.headers['range']) ?? 0;
+          } else if (statusRes.statusCode == 200 ||
+              statusRes.statusCode == 201) {
+            return statusRes;
+          } else {
+            throw ServiceException.fromHttpResponse(statusRes, statusRes.body);
+          }


[optional] Since 2 of 3 conditions in the chain leave the function I think it's bit easer to read as guard clauses.

Suggested change

if (statusRes.statusCode == 308) {

loopExpectedByte = _parseRange(statusRes.headers['range']) ?? 0;

} else if (statusRes.statusCode == 200 ||

statusRes.statusCode == 201) {

return statusRes;

} else {

throw ServiceException.fromHttpResponse(statusRes, statusRes.body);

}

if (statusRes.statusCode == 200 || statusRes.statusCode == 201) {

return statusRes;

}

if (statusRes.statusCOde != 308) {

throw ServiceException.fromHttpResponse(statusRes, statusRes.body);

}

loopExpectedByte = _parseRange(statusRes.headers['range']) ?? 0;

I didn't follow your exact advice but I restructured this a bit to be more (IMO) readable.

natebosch · 2026-04-14T21:48:52Z

-        newSize = requiredCapacity;
+  Future<http.Response> _uploadChunk(
+    Uint8List chunk,
+    bool isClose,


[nit] What is the specific meaning of "close" here? isClosed? isClosingChunk? isNearby?

I changed this to isLast with a note that this is the final chunk to upload.

natebosch · 2026-04-14T21:54:48Z

  FutureOr<http.Client> client,
  Uri url, {
  ObjectMetadata? metadata,
+  bool isIdempotent = false,


[nit] Can you add a doc comment on the method that describes how to use this argument?

natebosch · 2026-04-14T21:58:03Z

+    if (res.statusCode < 200 || res.statusCode >= 300) {
+      throw ServiceException.fromHttpResponse(res, res.body);
+    }


[optional] I like how patterns read here.

Suggested change

if (res.statusCode < 200 || res.statusCode >= 300) {

throw ServiceException.fromHttpResponse(res, res.body);

}

if (res.statusCode case < 200 || >= 300) {

throw ServiceException.fromHttpResponse(res, res.body);

}

Minor disagree on that one ;-)

natebosch · 2026-04-14T22:08:46Z

+      final mockClient = MockClient((request) async {
+        count++;
+        if (count == 1 && request.method == 'POST') {
+          // Start resumable upload.
+          return http.Response(
+            '',
+            200,
+            headers: {'location': 'http://example.com/location'},
+          );
+        } else if (count == 2 && request.method == 'PUT') {
+          // First chunk upload works.
+          return http.Response('', 308, headers: {'range': 'bytes=0-262143'});
+        } else if (count == 3 && request.method == 'PUT') {
+          // Second chunk upload fails.
+          throw http.ClientException('message');
+        } else if (count == 4 && request.method == 'PUT') {
+          // Status check returns a range smaller than what was acknowledged
+          // previously.
+          return http.Response('', 308, headers: {'range': 'bytes=0-1000'});
+        } else {
+          throw StateError(
+            'Unexpected call (count: $count, method: ${request.method}, '
+            'uri: ${request.url})',
+          );
+        }
+      });


[optional] The count behavior, especially in the second tests where it's used in an expectation, feels a little more indirect than I like for tests.

What about something like a List<(String, http.Response Function()> like

final responses = <(String, http.Response Function())>[ ('POST', () => http.Response('', 200, headers: {'location': 'http://example.com/location'})), ('PUT', () => http.Response('', 308, headers: {'range': 'bytes=0-262143'})), ('PUT', () => throw http.ClientException('message')), ('PUT', () => http.Response('', 308, headers: {'range': 'bytes=0-1000'})), ];

Use responses.removeAt(0) to know what method to check for, and use the callback to respond. The expect(count, 6) can become something like expect(responses, isEmpty);, which doesn't carry all the semantics of the test in the expectation, but I think will be a little more clear than a failure like "expected: 6, actual: 5"

Done. I'm neutral on whether this is better.

Co-authored-by: Nate Bosch <nbosch1@gmail.com>

…quinlan/google-cloud-dart into upload_testbench_plus_test

brianquinlan and others added 22 commits April 9, 2026 14:47

test(storage): run sink tests using testbench

00bb956

Update upload_object_from_sink_test.dart

839b5fa

Update upload_object_from_sink_test.dart

4c2800a

Update pkgs/google_cloud_storage/test/upload_object_from_sink_test.dart

7ae6c9f

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

Update upload_object_from_sink_test.dart

ad85244

Update upload_object_from_sink_test.dart

a281db1

Update test_utils.dart

e3e70db

Failing tests

fe3b834

Fix #1

3220ff8

Tests

8c0a93d

something

b5d7c8c

Handle range.

5118ee0

Update resumeable_upload.dart

72e0893

Fix

df0e354

Update upload_object_from_sink_test.dart

06ca04f

partial write, block rewrite

ce07498

meg to k

b72649f

Restore size

8d45abc

Fix

12e44cc

Cleanup

e1c8391

Fix

8033a76

Simplify

feb6e5a

gemini-code-assist bot reviewed Apr 14, 2026

View reviewed changes

brianquinlan added 6 commits April 14, 2026 11:46

Simplify

72ac6dd

Fix

fc09bac

Merge branch 'main' into upload_testbench_plus_test

3712718

Fix

db46f97

Remove length

29f7f6f

Fix

36d2e82

brianquinlan requested a review from natebosch April 14, 2026 21:11

brianquinlan assigned natebosch Apr 14, 2026

natebosch approved these changes Apr 14, 2026

View reviewed changes

brianquinlan and others added 10 commits April 16, 2026 09:44

Update pkgs/google_cloud_storage/lib/src/client.dart

7f00703

Co-authored-by: Nate Bosch <nbosch1@gmail.com>

Update pkgs/google_cloud_storage/lib/src/client.dart

ee990f6

Co-authored-by: Nate Bosch <nbosch1@gmail.com>

Apply suggestion from @natebosch

7fd6cfb

Co-authored-by: Nate Bosch <nbosch1@gmail.com>

Apply suggestion from @natebosch

c8c2ebe

Co-authored-by: Nate Bosch <nbosch1@gmail.com>

review feedback

f82f4ac

Merge branch 'upload_testbench_plus_test' of https://github.com/brian…

5754d80

…quinlan/google-cloud-dart into upload_testbench_plus_test

no future

47b3ba4

test fix

735cfae

Merge branch 'main' into upload_testbench_plus_test

55fbd9d

Fixes

a40622d

	final match = RegExp(r'bytes=0-(\d+)').firstMatch(rangeHeader);
	final match = RegExp(r'bytes=\d+-(\d+)').firstMatch(rangeHeader);

Conversation

brianquinlan commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brianquinlan commented Apr 14, 2026 •

edited

Loading