Skip to content

fix(storage): retry retryable HTTP status codes (408/429/5xx) for XML multipart upload operations #14802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chandra-siri wants to merge 1 commit into
base: main
Choose a base branch
from
+1,187 −2
Open

fix(storage): retry retryable HTTP status codes (408/429/5xx) for XML multipart upload operations #14802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ...gle-cloud-storage/src/main/java/com/google/cloud/storage/DefaultStorageRetryStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,13 @@ private RetryResult deepShouldRetry(BaseServiceException baseServiceException) {
if (baseServiceException.getCode() == BaseServiceException.UNKNOWN_CODE
&& baseServiceException.getReason() == null) {
final Throwable cause = baseServiceException.getCause();
if (cause instanceof IOException) {
if (cause instanceof HttpResponseException) {
int code = ((HttpResponseException) cause).getStatusCode();
return shouldRetryCodeReason(code, null);
} else if (cause instanceof Retryable) {
Retryable retryable = (Retryable) cause;
return (idempotent && retryable.isRetryable()) ? RetryResult.RETRY : RetryResult.NO_RETRY;
} else if (cause instanceof IOException) {
IOException ioException = (IOException) cause;
return shouldRetryIOException(ioException);
}
Expand Down
5 changes: 4 additions & 1 deletion ...torage/google-cloud-storage/src/main/java/com/google/cloud/storage/RewindableContent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,10 @@ public long getLength() {
@Override
void rewindTo(long offset) {
Preconditions.checkArgument(
offset < size, "provided offset must be less than size (%d < %d)", offset, size);
0 <= offset && offset <= size,
"provided offset must be within [0, size] (0 <= %s <= %s)",
offset,
size);
this.readOffset = offset;
}

Expand Down
16 changes: 16 additions & 0 deletions ...storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.storage;

import com.google.api.client.googleapis.json.GoogleJsonError;
import com.google.api.client.http.HttpResponseException;
import com.google.api.core.ApiFuture;
import com.google.api.core.ApiFutures;
import com.google.api.core.InternalApi;
Expand Down Expand Up @@ -128,9 +129,24 @@ static BaseServiceException coalesce(Throwable t) {
if (t.getCause() instanceof ApiException) {
return asStorageException((ApiException) t.getCause());
}
if (t instanceof HttpResponseException) {
return fromHttpResponseException((HttpResponseException) t);
}
if (t.getCause() instanceof HttpResponseException) {
return fromHttpResponseException((HttpResponseException) t.getCause());
}
return getStorageException(t);
}

/**
* Create a {@link StorageException} from an {@link HttpResponseException} preserving its HTTP
* status code (and, for a {@code GoogleJsonResponseException}, its JSON error code and reason),
* its message and the original exception as the cause.
*/
private static StorageException fromHttpResponseException(HttpResponseException e) {
return new StorageException((IOException) e);
}

static <T> ApiFuture<T> coalesceAsync(ApiFuture<T> originalFuture) {
return ApiFutures.catchingAsync(
originalFuture,
Expand Down
231 changes: 231 additions & 0 deletions ...loud-storage/src/test/java/com/google/cloud/storage/DefaultRetryHandlingBehaviorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,37 @@ enum ThrowableCategory {
AUTH_RETRYABLE_TRUE(new RetryableException(true)),
AUTH_RETRYABLE_FALSE(new RetryableException(false)),
UNKNOWN_HOST_EXCEPTION(C.UNKNOWN_HOST_EXCEPTION),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_401(
new StorageException(0, "code 0 wrapping http 401", C.HTTP_401)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_403(
new StorageException(0, "code 0 wrapping http 403", C.HTTP_403)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_404(
new StorageException(0, "code 0 wrapping http 404", C.HTTP_404)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408(
new StorageException(0, "code 0 wrapping http 408", C.HTTP_409)),
Comment on lines +356 to +357

@gemini-code-assist gemini-code-assist Bot Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a discrepancy here where STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408 is wrapping C.HTTP_409 instead of C.HTTP_408. This appears to be a copy-paste typo. Please use C.HTTP_408 to ensure the test correctly validates behavior for HTTP 408 (Request Timeout).

Suggested change
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408(
new StorageException(0, "code 0 wrapping http 408", C.HTTP_409)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408(
new StorageException(0, "code 0 wrapping http 408", C.HTTP_408)),
References
  1. Keep duplicated or similar text/exception messages in sync across the codebase. If a change is made to one instance, ensure it is applied to all other locations to maintain consistency.

STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_429(
new StorageException(0, "code 0 wrapping http 429", C.HTTP_429)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_500(
new StorageException(0, "code 0 wrapping http 500", C.HTTP_500)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_502(
new StorageException(0, "code 0 wrapping http 502", C.HTTP_502)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_503(
new StorageException(0, "code 0 wrapping http 503", C.HTTP_503)),
STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_504(
new StorageException(0, "code 0 wrapping http 504", C.HTTP_504)),
COALESCED_HTTP_RESPONSE_EXCEPTION_401(StorageException.coalesce(C.HTTP_401)),
COALESCED_HTTP_RESPONSE_EXCEPTION_403(StorageException.coalesce(C.HTTP_403)),
COALESCED_HTTP_RESPONSE_EXCEPTION_404(StorageException.coalesce(C.HTTP_404)),
COALESCED_HTTP_RESPONSE_EXCEPTION_408(StorageException.coalesce(C.HTTP_409)),

@gemini-code-assist gemini-code-assist Bot Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similarly, COALESCED_HTTP_RESPONSE_EXCEPTION_408 is using C.HTTP_409 instead of C.HTTP_408. Please update this to use C.HTTP_408.

Suggested change
COALESCED_HTTP_RESPONSE_EXCEPTION_408(StorageException.coalesce(C.HTTP_409)),
COALESCED_HTTP_RESPONSE_EXCEPTION_408(StorageException.coalesce(C.HTTP_408)),
References
  1. Keep duplicated or similar text/exception messages in sync across the codebase. If a change is made to one instance, ensure it is applied to all other locations to maintain consistency.

COALESCED_HTTP_RESPONSE_EXCEPTION_429(StorageException.coalesce(C.HTTP_429)),
COALESCED_HTTP_RESPONSE_EXCEPTION_500(StorageException.coalesce(C.HTTP_500)),
COALESCED_HTTP_RESPONSE_EXCEPTION_502(StorageException.coalesce(C.HTTP_502)),
COALESCED_HTTP_RESPONSE_EXCEPTION_503(StorageException.coalesce(C.HTTP_503)),
COALESCED_HTTP_RESPONSE_EXCEPTION_504(StorageException.coalesce(C.HTTP_504)),
STORAGE_EXCEPTION_0_AUTH_RETRYABLE_TRUE(
new StorageException(0, "code 0 wrapping auth retryable", new RetryableException(true))),
STORAGE_EXCEPTION_0_AUTH_RETRYABLE_FALSE(
new StorageException(0, "code 0 wrapping auth retryable", new RetryableException(false))),
;

private final Throwable throwable;
Expand Down Expand Up @@ -1082,6 +1113,206 @@ private static ImmutableList<Case> getAllCases() {
ThrowableCategory.UNKNOWN_HOST_EXCEPTION,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_401,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_401,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_403,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_403,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_404,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_404,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_408,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_429,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_429,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_500,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_500,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_502,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_502,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_503,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_503,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_504,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_504,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_401,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_401,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_403,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_403,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_404,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_404,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_408,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_408,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_429,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_429,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_500,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_500,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_502,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_502,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_503,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_503,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_504,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.SAME),
new Case(
ThrowableCategory.COALESCED_HTTP_RESPONSE_EXCEPTION_504,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.DEFAULT_MORE_STRICT),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_AUTH_RETRYABLE_TRUE,
HandlerCategory.IDEMPOTENT,
ExpectRetry.YES,
Behavior.DEFAULT_MORE_PERMISSIBLE),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_AUTH_RETRYABLE_TRUE,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_AUTH_RETRYABLE_FALSE,
HandlerCategory.IDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME),
new Case(
ThrowableCategory.STORAGE_EXCEPTION_0_AUTH_RETRYABLE_FALSE,
HandlerCategory.NONIDEMPOTENT,
ExpectRetry.NO,
Behavior.SAME))
.build();
}
Expand Down
Loading
Loading