From acbd82af7f42fc6db8d25e7f641c34562c5cfcb5 Mon Sep 17 00:00:00 2001 From: Dhriti Chopra Date: Wed, 28 Oct 2026 08:31:37 +0000 Subject: [PATCH 1/2] fix(storage): preserve HTTP status for XML multipart upload errors so retries work MultipartUploadClientImpl sends raw XML API requests that throw a plain HttpResponseException on error. These were not translated, so the retrier's catch-all coalesced them into a StorageException with code 0, and the retry strategy could not classify them. As a result, retryable responses (408, 429, 5xx) were never retried, even for idempotent operations such as uploadPart, and callers saw getCode() == 0. Translate IOExceptions via StorageException.translate inside each MultipartUploadClient operation, matching HttpStorageRpc, so the HTTP status code is preserved and retries follow the standard rules. createMultipartUpload remains non-idempotent and is not retried. --- .../storage/MultipartUploadClientImpl.java | 42 +- .../ITMultipartUploadClientRetryTest.java | 373 ++++++++++++++++++ 2 files changed, 404 insertions(+), 11 deletions(-) create mode 100644 java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClientImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClientImpl.java index 2aecba6232b1..cc4a0b77593b 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClientImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClientImpl.java @@ -17,6 +17,7 @@ import com.google.cloud.storage.Conversions.Decoder; import com.google.cloud.storage.Retrying.Retrier; +import com.google.cloud.storage.StorageException.IOExceptionCallable; import com.google.cloud.storage.multipartupload.model.AbortMultipartUploadRequest; import com.google.cloud.storage.multipartupload.model.AbortMultipartUploadResponse; import com.google.cloud.storage.multipartupload.model.CompleteMultipartUploadRequest; @@ -29,6 +30,8 @@ import com.google.cloud.storage.multipartupload.model.ListPartsResponse; import com.google.cloud.storage.multipartupload.model.UploadPartRequest; import com.google.cloud.storage.multipartupload.model.UploadPartResponse; +import java.io.IOException; +import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -54,7 +57,7 @@ final class MultipartUploadClientImpl extends MultipartUploadClient { public CreateMultipartUploadResponse createMultipartUpload(CreateMultipartUploadRequest request) { return retrier.run( retryAlgorithmManager.nonIdempotent(), - () -> httpRequestManager.sendCreateMultipartUploadRequest(request), + translateIOException(() -> httpRequestManager.sendCreateMultipartUploadRequest(request)), Decoder.identity()); } @@ -63,7 +66,7 @@ public ListPartsResponse listParts(ListPartsRequest request) { return retrier.run( retryAlgorithmManager.idempotent(), - () -> httpRequestManager.sendListPartsRequest(request), + translateIOException(() -> httpRequestManager.sendListPartsRequest(request)), Decoder.identity()); } @@ -72,7 +75,7 @@ public AbortMultipartUploadResponse abortMultipartUpload(AbortMultipartUploadReq return retrier.run( retryAlgorithmManager.idempotent(), - () -> httpRequestManager.sendAbortMultipartUploadRequest(request), + translateIOException(() -> httpRequestManager.sendAbortMultipartUploadRequest(request)), Decoder.identity()); } @@ -81,7 +84,7 @@ public CompleteMultipartUploadResponse completeMultipartUpload( CompleteMultipartUploadRequest request) { return retrier.run( retryAlgorithmManager.idempotent(), - () -> httpRequestManager.sendCompleteMultipartUploadRequest(request), + translateIOException(() -> httpRequestManager.sendCompleteMultipartUploadRequest(request)), Decoder.identity()); } @@ -90,12 +93,13 @@ public UploadPartResponse uploadPart(UploadPartRequest request, RequestBody requ AtomicBoolean dirty = new AtomicBoolean(false); return retrier.run( retryAlgorithmManager.idempotent(), - () -> { - if (dirty.getAndSet(true)) { - requestBody.getContent().rewindTo(0); - } - return httpRequestManager.sendUploadPartRequest(request, requestBody.getContent()); - }, + translateIOException( + () -> { + if (dirty.getAndSet(true)) { + requestBody.getContent().rewindTo(0); + } + return httpRequestManager.sendUploadPartRequest(request, requestBody.getContent()); + }), Decoder.identity()); } @@ -103,7 +107,23 @@ public UploadPartResponse uploadPart(UploadPartRequest request, RequestBody requ public ListMultipartUploadsResponse listMultipartUploads(ListMultipartUploadsRequest request) { return retrier.run( retryAlgorithmManager.idempotent(), - () -> httpRequestManager.sendListMultipartUploadsRequest(request), + translateIOException(() -> httpRequestManager.sendListMultipartUploadsRequest(request)), Decoder.identity()); } + + /** + * XML API requests surface error responses as a plain {@link + * com.google.api.client.http.HttpResponseException}. Translate any {@link IOException} into a + * {@link StorageException} before it reaches the retrier so the HTTP status code is preserved and + * the retry algorithm can classify it (e.g. 503 as retryable). + */ + private static Callable translateIOException(IOExceptionCallable c) { + return () -> { + try { + return c.call(); + } catch (IOException e) { + throw StorageException.translate(e); + } + }; + } } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java new file mode 100644 index 000000000000..9fe804fce342 --- /dev/null +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java @@ -0,0 +1,373 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.storage; + +import static com.google.common.truth.Truth.assertThat; +import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE; +import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST; +import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.NO_CONTENT; +import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.OK; +import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.SERVICE_UNAVAILABLE; +import static org.junit.Assert.assertThrows; + +import com.google.cloud.storage.FakeHttpServer.HttpRequestHandler; +import com.google.cloud.storage.it.runner.StorageITRunner; +import com.google.cloud.storage.it.runner.annotations.Backend; +import com.google.cloud.storage.it.runner.annotations.ParallelFriendly; +import com.google.cloud.storage.it.runner.annotations.SingleBackend; +import com.google.cloud.storage.multipartupload.model.AbortMultipartUploadRequest; +import com.google.cloud.storage.multipartupload.model.CompleteMultipartUploadRequest; +import com.google.cloud.storage.multipartupload.model.CompleteMultipartUploadResponse; +import com.google.cloud.storage.multipartupload.model.CompletedMultipartUpload; +import com.google.cloud.storage.multipartupload.model.CompletedPart; +import com.google.cloud.storage.multipartupload.model.CreateMultipartUploadRequest; +import com.google.cloud.storage.multipartupload.model.ListMultipartUploadsRequest; +import com.google.cloud.storage.multipartupload.model.ListMultipartUploadsResponse; +import com.google.cloud.storage.multipartupload.model.ListPartsRequest; +import com.google.cloud.storage.multipartupload.model.ListPartsResponse; +import com.google.cloud.storage.multipartupload.model.UploadPartRequest; +import com.google.cloud.storage.multipartupload.model.UploadPartResponse; +import com.google.common.collect.ImmutableList; +import io.grpc.netty.shaded.io.netty.buffer.ByteBuf; +import io.grpc.netty.shaded.io.netty.buffer.Unpooled; +import io.grpc.netty.shaded.io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.grpc.netty.shaded.io.netty.handler.codec.http.FullHttpRequest; +import io.grpc.netty.shaded.io.netty.handler.codec.http.FullHttpResponse; +import io.grpc.netty.shaded.io.netty.handler.codec.http.HttpRequest; +import io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * Verifies that {@link MultipartUploadClient} operations classify XML API error responses by their + * HTTP status code, retrying transient errors for idempotent operations. + */ +@RunWith(StorageITRunner.class) +@SingleBackend(Backend.PROD) +@ParallelFriendly +public final class ITMultipartUploadClientRetryTest { + + private static final byte[] PART_CONTENT = "hello world".getBytes(StandardCharsets.UTF_8); + private static final String ETAG = "\"af1ed31420542285653c803a34aa839a\""; + + @Test + public void uploadPart_retriesOn503_andResendsFullBody() throws Exception { + List receivedBodies = new CopyOnWriteArrayList<>(); + FailFirst handler = + new FailFirst( + 2, + SERVICE_UNAVAILABLE, + req -> { + DefaultFullHttpResponse resp = new DefaultFullHttpResponse(req.protocolVersion(), OK); + resp.headers().set("ETag", ETAG); + return resp; + }, + receivedBodies); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + UploadPartResponse response = + client.uploadPart(uploadPartRequest(), RequestBody.of(ByteBuffer.wrap(PART_CONTENT))); + + assertThat(response.eTag()).isEqualTo(ETAG); + assertThat(handler.attempts()).isEqualTo(3); + // every attempt, including the retries, must send the complete part content + assertThat(receivedBodies).hasSize(3); + for (byte[] body : receivedBodies) { + assertThat(body).isEqualTo(PART_CONTENT); + } + } + } + + @Test + public void uploadPart_retriesAllRetryableStatusCodes() throws Exception { + ImmutableList retryable = + ImmutableList.of( + HttpResponseStatus.REQUEST_TIMEOUT, + HttpResponseStatus.TOO_MANY_REQUESTS, + HttpResponseStatus.INTERNAL_SERVER_ERROR, + HttpResponseStatus.BAD_GATEWAY, + SERVICE_UNAVAILABLE, + HttpResponseStatus.GATEWAY_TIMEOUT); + for (HttpResponseStatus status : retryable) { + FailFirst handler = + new FailFirst( + 1, + status, + req -> { + DefaultFullHttpResponse resp = + new DefaultFullHttpResponse(req.protocolVersion(), OK); + resp.headers().set("ETag", ETAG); + return resp; + }); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + UploadPartResponse response = + client.uploadPart(uploadPartRequest(), RequestBody.of(ByteBuffer.wrap(PART_CONTENT))); + + assertThat(response.eTag()).isEqualTo(ETAG); + assertThat(handler.attempts()).isEqualTo(2); + } + } + } + + @Test + public void uploadPart_exhaustsRetries_surfacesHttpStatusCode() throws Exception { + FailFirst handler = new FailFirst(Integer.MAX_VALUE, SERVICE_UNAVAILABLE, null); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + StorageException e = + assertThrows( + StorageException.class, + () -> + client.uploadPart( + uploadPartRequest(), RequestBody.of(ByteBuffer.wrap(PART_CONTENT)))); + + assertThat(e.getCode()).isEqualTo(503); + // FakeHttpServer configures maxAttempts = 6 + assertThat(handler.attempts()).isEqualTo(6); + } + } + + @Test + public void uploadPart_doesNotRetryClientError_surfacesHttpStatusCode() throws Exception { + FailFirst handler = new FailFirst(Integer.MAX_VALUE, BAD_REQUEST, null); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + StorageException e = + assertThrows( + StorageException.class, + () -> + client.uploadPart( + uploadPartRequest(), RequestBody.of(ByteBuffer.wrap(PART_CONTENT)))); + + assertThat(e.getCode()).isEqualTo(400); + assertThat(handler.attempts()).isEqualTo(1); + } + } + + @Test + public void listParts_retriesOn503() throws Exception { + FailFirst handler = + new FailFirst( + 2, + SERVICE_UNAVAILABLE, + xml( + "test-buckettest-key" + + "test-upload-id")); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + ListPartsResponse response = + client.listParts( + ListPartsRequest.builder() + .bucket("test-bucket") + .key("test-key") + .uploadId("test-upload-id") + .build()); + + assertThat(response.bucket()).isEqualTo("test-bucket"); + assertThat(handler.attempts()).isEqualTo(3); + } + } + + @Test + public void abortMultipartUpload_retriesOn503() throws Exception { + FailFirst handler = + new FailFirst( + 2, + SERVICE_UNAVAILABLE, + req -> new DefaultFullHttpResponse(req.protocolVersion(), NO_CONTENT)); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + client.abortMultipartUpload( + AbortMultipartUploadRequest.builder() + .bucket("test-bucket") + .key("test-key") + .uploadId("test-upload-id") + .build()); + + assertThat(handler.attempts()).isEqualTo(3); + } + } + + @Test + public void completeMultipartUpload_retriesOn503() throws Exception { + FailFirst handler = + new FailFirst( + 2, + SERVICE_UNAVAILABLE, + xml( + "test-buckettest-key" + + "" + + ETAG + + "")); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + CompleteMultipartUploadResponse response = + client.completeMultipartUpload( + CompleteMultipartUploadRequest.builder() + .bucket("test-bucket") + .key("test-key") + .uploadId("test-upload-id") + .multipartUpload( + CompletedMultipartUpload.builder() + .parts( + ImmutableList.of( + CompletedPart.builder().partNumber(1).eTag(ETAG).build())) + .build()) + .build()); + + assertThat(response.etag()).isEqualTo(ETAG); + assertThat(handler.attempts()).isEqualTo(3); + } + } + + @Test + public void listMultipartUploads_retriesOn503() throws Exception { + FailFirst handler = + new FailFirst( + 2, + SERVICE_UNAVAILABLE, + xml( + "test-bucket" + + "")); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + ListMultipartUploadsResponse response = + client.listMultipartUploads( + ListMultipartUploadsRequest.builder().bucket("test-bucket").build()); + + assertThat(response.bucket()).isEqualTo("test-bucket"); + assertThat(handler.attempts()).isEqualTo(3); + } + } + + @Test + public void createMultipartUpload_isNonIdempotent_doesNotRetry_surfacesHttpStatusCode() + throws Exception { + FailFirst handler = new FailFirst(Integer.MAX_VALUE, SERVICE_UNAVAILABLE, null); + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + + StorageException e = + assertThrows( + StorageException.class, + () -> + client.createMultipartUpload( + CreateMultipartUploadRequest.builder() + .bucket("test-bucket") + .key("test-key") + .build())); + + assertThat(e.getCode()).isEqualTo(503); + assertThat(handler.attempts()).isEqualTo(1); + } + } + + private static MultipartUploadClient newClient(FakeHttpServer server) { + return MultipartUploadClient.create(MultipartUploadSettings.of(server.getHttpStorageOptions())); + } + + private static UploadPartRequest uploadPartRequest() { + return UploadPartRequest.builder() + .bucket("test-bucket") + .key("test-key") + .uploadId("test-upload-id") + .partNumber(1) + .build(); + } + + private static HttpRequestHandler xml(String body) { + return req -> { + ByteBuf buf = Unpooled.wrappedBuffer(body.getBytes(StandardCharsets.UTF_8)); + DefaultFullHttpResponse resp = new DefaultFullHttpResponse(req.protocolVersion(), OK, buf); + resp.headers().set(CONTENT_TYPE, "application/xml; charset=utf-8"); + return resp; + }; + } + + /** + * Responds to the first {@code failures} requests with {@code status}, then delegates to {@code + * success}. Optionally records the body of every request received. + */ + private static final class FailFirst implements HttpRequestHandler { + private final int failures; + private final HttpResponseStatus status; + private final HttpRequestHandler success; + private final List receivedBodies; + private final AtomicInteger attempts = new AtomicInteger(); + + private FailFirst(int failures, HttpResponseStatus status, HttpRequestHandler success) { + this(failures, status, success, new CopyOnWriteArrayList<>()); + } + + private FailFirst( + int failures, + HttpResponseStatus status, + HttpRequestHandler success, + List receivedBodies) { + this.failures = failures; + this.status = status; + this.success = success; + this.receivedBodies = receivedBodies; + } + + @Override + public FullHttpResponse apply(HttpRequest req) throws Exception { + int attempt = attempts.incrementAndGet(); + if (req instanceof FullHttpRequest) { + ByteBuf content = ((FullHttpRequest) req).content(); + byte[] body = new byte[content.readableBytes()]; + content.getBytes(content.readerIndex(), body); + receivedBodies.add(body); + } + if (attempt <= failures) { + ByteBuf buf = + Unpooled.wrappedBuffer(status.reasonPhrase().getBytes(StandardCharsets.UTF_8)); + FullHttpResponse resp = new DefaultFullHttpResponse(req.protocolVersion(), status, buf); + resp.headers().set(CONTENT_TYPE, "text/plain; charset=utf-8"); + return resp; + } + return success.apply(req); + } + + private int attempts() { + return attempts.get(); + } + } +} From 8e3dd664aa58b2b913c1fbcbd0ebe6151628a2bf Mon Sep 17 00:00:00 2001 From: Dhriti Chopra Date: Thu, 29 Oct 2026 05:42:45 +0000 Subject: [PATCH 2/2] test(storage): reuse a single FakeHttpServer across retryable status codes --- .../ITMultipartUploadClientRetryTest.java | 44 ++++++++++++------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java index 9fe804fce342..84a9be76beef 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadClientRetryTest.java @@ -17,6 +17,7 @@ package com.google.cloud.storage; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE; import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST; import static io.grpc.netty.shaded.io.netty.handler.codec.http.HttpResponseStatus.NO_CONTENT; @@ -54,6 +55,7 @@ import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.runner.RunWith; @@ -109,26 +111,36 @@ public void uploadPart_retriesAllRetryableStatusCodes() throws Exception { HttpResponseStatus.BAD_GATEWAY, SERVICE_UNAVAILABLE, HttpResponseStatus.GATEWAY_TIMEOUT); - for (HttpResponseStatus status : retryable) { - FailFirst handler = - new FailFirst( - 1, - status, - req -> { - DefaultFullHttpResponse resp = - new DefaultFullHttpResponse(req.protocolVersion(), OK); - resp.headers().set("ETag", ETAG); - return resp; - }); - - try (FakeHttpServer server = FakeHttpServer.of(handler)) { - MultipartUploadClient client = newClient(server); + // Share a single server across all status codes; the handler reads the status to fail with + // from currentStatus, and attempts is reset before each iteration. + AtomicReference currentStatus = new AtomicReference<>(); + AtomicInteger attempts = new AtomicInteger(); + HttpRequestHandler handler = + req -> { + if (attempts.incrementAndGet() == 1) { + HttpResponseStatus status = currentStatus.get(); + ByteBuf buf = + Unpooled.wrappedBuffer(status.reasonPhrase().getBytes(StandardCharsets.UTF_8)); + FullHttpResponse resp = new DefaultFullHttpResponse(req.protocolVersion(), status, buf); + resp.headers().set(CONTENT_TYPE, "text/plain; charset=utf-8"); + return resp; + } + DefaultFullHttpResponse resp = new DefaultFullHttpResponse(req.protocolVersion(), OK); + resp.headers().set("ETag", ETAG); + return resp; + }; + + try (FakeHttpServer server = FakeHttpServer.of(handler)) { + MultipartUploadClient client = newClient(server); + for (HttpResponseStatus status : retryable) { + currentStatus.set(status); + attempts.set(0); UploadPartResponse response = client.uploadPart(uploadPartRequest(), RequestBody.of(ByteBuffer.wrap(PART_CONTENT))); - assertThat(response.eTag()).isEqualTo(ETAG); - assertThat(handler.attempts()).isEqualTo(2); + assertWithMessage("status %s", status).that(response.eTag()).isEqualTo(ETAG); + assertWithMessage("status %s", status).that(attempts.get()).isEqualTo(2); } } }