[chandra-siri]

Summary

XML Multipart Upload operations in MultipartUploadClient (such as uploadPart, completeMultipartUpload, abortMultipartUpload, listParts, and listMultipartUploads) execute raw com.google.api.client.http.HttpRequest calls that throw a plain com.google.api.client.http.HttpResponseException on non-2xx responses.

When Retrying.DefaultRetrier.run(...) caught those exceptions, StorageException.coalesce(Throwable) fell through to getStorageException(t) -> new StorageException(UNKNOWN_CODE, t.getMessage(), t) (code = 0, reason = null, retryable = false), and DefaultStorageRetryStrategy.InterceptorImpl.deepShouldRetry(BaseServiceException) routed the HttpResponseException cause (since HttpResponseException extends IOException) into shouldRetryIOException(IOException), classifying retryable HTTP status codes (408, 429, 500, 502, 503, 504) as unretryable on Attempt #1.

Changes

1. StorageException.coalesce(Throwable):
   • When t or t.getCause() is an HttpResponseException, delegate to new StorageException((IOException) e) so BaseHttpServiceException preserves the HTTP status code (getCode()), message (including XML <Error> bodies), cause, and retryability.
2. DefaultStorageRetryStrategy.InterceptorImpl.deepShouldRetry(BaseServiceException):
   • When code == UNKNOWN_CODE && reason == null, evaluate cause instanceof HttpResponseException (shouldRetryCodeReason(statusCode, null)) and cause instanceof Retryable ((idempotent && isRetryable()) ? RETRY : NO_RETRY) before cause instanceof IOException, matching beforeEval(Exception).
3. RewindableContent.PathRewindableContent.rewindTo(long):
   • Update the offset precondition from offset < size to 0 <= offset && offset <= size so retrying a 0-byte Path payload in uploadPart can rewind to offset 0 without throwing IllegalArgumentException.
4. Tests:
   • StorageExceptionHttpResponseExceptionTest: Unit tests verifying StorageException.coalesce preserves HTTP status codes, messages, causes, and retryability for HttpResponseException.
   • DefaultRetryHandlingBehaviorTest: Permutation matrix cases for STORAGE_EXCEPTION_0_HTTP_RESPONSE_EXCEPTION_*, COALESCED_HTTP_RESPONSE_EXCEPTION_*, and STORAGE_EXCEPTION_0_AUTH_RETRYABLE_*.
   • RetryingTest: End-to-end DefaultRetrier + DefaultStorageRetryStrategy retry tests with raw HttpResponseException.
   • MultipartUploadClientImplRetryTest: Hermetic MockHttpTransport retry tests across all MultipartUploadClientImpl operations, status codes (408, 429, 500, 502, 503, 504, 400, 403, 404), and RequestBody payload rewinding (ByteBuffer, multi-ByteBuffer, Path, and 0-byte Path).
   • RewindablePathContentTest & ITMultipartUploadClientRetryTest: Unit tests for PathRewindableContent.rewindTo bounds and FakeHttpServer integration tests via MultipartUploadClient.create(...).

[gemini-code-assist]

Code Review

This pull request enhances Google Cloud Storage retry handling by integrating HttpResponseException and Retryable exceptions into the retry strategy, adjusting boundary checks in RewindableContent to allow rewinding to the end of the content, and adding comprehensive tests for multipart upload client retries. The review feedback correctly identifies copy-paste typos in DefaultRetryHandlingBehaviorTest.java where HTTP 408 test cases incorrectly wrapped C.HTTP_409 instead of C.HTTP_408.

[gemini-code-assist]

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.

[gemini-code-assist]

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.