Fix bug in MultipartS3AsyncClient GetObject Retryable Errors #6309

davidh44 · 2025-07-31T06:42:11Z

Motivation and Context

This PR fixes a bug in the Java-based multipart S3 client (MultipartS3AsyncClient) that causes incorrect retry behavior and duplicate request logging during GetObject operations.

Currently, when a retryable error, e.g., 503 Slowdown, is returned by S3, the SDK may do one of two things:

Fail the request right away instead of retrying
Retry the error, but incorrectly process subsequent responses. A successful 200 response returned by the sever will be ignored. Instead, the SDK will log the initial error, and continue to retry until retry attempts are exhausted. This may happen instead of scenario 1 (failing right away), if many concurrent requests are in progress and a race condition occurs.

Modifications

SplittingTransformer

Retryable errors on the first part will be retried. Errors on subsequent parts will not be retried
- Part numbers will be tracked in IndividualTransformer
- Update to only call publisherToUpstream.error() in IndividualTransformer.exceptionOccurred() for part numbers greater than 1
Retry attempt responses will be properly processed
- Remove invocation of individualFuture.completeExceptionally() in IndividualTransformer.prepare() when the resultFuture completes

DownloadObjectHelper

Forward exceptions from the subscriber future to the result future

MultipartDownloaderSubscriber

Keep track of each part GET request and cancel all in-flight ones if onError() is invoked

Testing

Added mock tests
Integ tests passed

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

.../amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java

.changes/next-release/bugfix-AmazonS3-263fed5.json

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

services/s3/pom.xml

...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java

...are/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java

Copilot

Pull Request Overview

This PR fixes a bug in the MultipartS3AsyncClient where retryable errors during GetObject operations were failing immediately or causing the initial error to be propagated even after successful retries. The fix improves error handling logic in the SplittingTransformer to properly handle retries for the first part of multipart downloads.

Fixed error propagation logic to only retry errors for the first part of multipart downloads
Added proper future forwarding to ensure upstream errors are handled correctly
Enhanced test coverage with comprehensive WireMock tests for retry scenarios

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
SplittingTransformer.java	Core fix - updated error handling logic to properly distinguish first part from subsequent parts and handle retries correctly
MultipartDownloaderSubscriber.java	Added future tracking and cleanup for proper error handling
DownloadObjectHelper.java	Added future forwarding to ensure exceptions are properly propagated
S3MultipartClientGetObjectWiremockTest.java	Comprehensive new test suite covering retry scenarios and error handling
MultipartDownloaderSubscriberWiremockTest.java	Removed obsolete test that was failing on first request errors
MultipartS3AsyncClient.java	Updated javadoc to reflect GET support
pom.xml	Added test dependency for retries module
IndividualPartSubscriberTckTest.java	Updated constructor call to match new signature
bugfix changelog	Documentation of the fix

...are/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java

dagnir · 2025-08-01T21:56:11Z

Please add more context and details in the PR description.

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

.changes/next-release/bugfix-AmazonS3-263fed5.json

sonarqubecloud · 2025-08-05T03:38:18Z

Quality Gate failed

Failed conditions
78.4% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

...ava/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java

...are/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

zoewangg · 2025-08-05T20:16:02Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -314,14 +317,26 @@ public void onStream(SdkPublisher<ByteBuffer> publisher) {
                    );
                }
            }
-            publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response));
+
+            CompletableFutureUtils.forwardResultTo(upstreamFuture, resultFuture);


Why do we forward result here again and do it for every part?

I guess we just need to do it for the first part. When this is removed, an error on a subsequent part won't get propagated.

Specifically, in the new test I added, if we don't call this at all, instead of S3Exception being thrown, we get:

SdkClientException: Unable to execute HTTP request: onError() was already invoked. (SDK Attempt Count: 2)

aws-sdk-java-v2/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java

Line 317 in 14cccda

assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY),

Hmm, we should probably figure out why onError was invoked multiple times.

This is the flow of events:

Error response is returned

IndividualTransformer.exceptionOccurred() is invoked, which calls publisherToUpstream.error(error)

In SimplePublisher.doProcessQueue(), in the ON_ERROR case failureMessage is set

IndividualPartSubscriber.onNext() is invoked, because there is still outstanding demand. Here, publisherToUpstream.send(byteBuffer) is called

In SimplePublisher.doProcessQueue(), the entry is onNext, but because the failureMessage is set, entry.resultFuture.completeExceptionally(failureMessage.get()) is invoked

https://github.com/aws/aws-sdk-java-v2/blob/master/utils/src/main/java/software/amazon/awssdk/utils/async/SimplePublisher.java#L260

Not sure how we can prevent 4) from happening, is it necessary?

We could always prevent sending bytes to the publisherToUpstream after IndividualTransformer received an error, would that be a solution?

L-Applin · 2025-08-06T15:28:07Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

+    /**
+     * Tracks the part number. Errors will only be retried for the first part.
+     */
+    private final AtomicInteger partNumber = new AtomicInteger(0);


NIT (non-blocking): I think we should avoid referring to 'parts' in the SplittingTranformer, that's a s3 concept, when this class is more abstract, in a core package. Maybe rename to something like 'onNextSignalsSent' or onNextNumber which would track the total amount of 'onNext' signals sent to the downstreamSubscriber

L-Applin · 2025-08-06T15:29:06Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -259,28 +262,27 @@ private void handleFutureCancel(Throwable e) {
     * body publisher.
     */
    private class IndividualTransformer implements AsyncResponseTransformer<ResponseT, ResponseT> {
+        private final int partNumber;


NIT: same here, avoid referring to s3 related terms

L-Applin · 2025-08-06T15:52:15Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -314,14 +317,26 @@ public void onStream(SdkPublisher<ByteBuffer> publisher) {
                    );
                }
            }
-            publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response));
+
+            CompletableFutureUtils.forwardResultTo(upstreamFuture, resultFuture);


We could always prevent sending bytes to the publisherToUpstream after IndividualTransformer received an error, would that be a solution?

davidh44 added 4 commits July 30, 2025 23:40

Fix bug in MultipartS3AsyncClient GetObject retryable errors

3ceee7f

Merge branch 'master' into hdavidh/multipart-download-retryable-errors

0381604

Remove retry logic from SplittingTransformer

4cdded1

Update mock error test

54457df

davidh44 marked this pull request as ready for review July 31, 2025 19:12

davidh44 requested a review from a team as a code owner July 31, 2025 19:12

davidh44 commented Jul 31, 2025

View reviewed changes

.../amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java Show resolved Hide resolved

zoewangg requested changes Aug 1, 2025

View reviewed changes

zoewangg requested a review from Copilot August 1, 2025 17:38

Copilot AI reviewed Aug 1, 2025

View reviewed changes

...are/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java Outdated Show resolved Hide resolved

dagnir requested changes Aug 1, 2025

View reviewed changes

davidh44 added 2 commits August 1, 2025 17:34

Address comments pt 1

7f8803f

Address comments pt2

22b0a0b

RanVaknin requested changes Aug 4, 2025

View reviewed changes

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java Show resolved Hide resolved

dagnir requested changes Aug 4, 2025

View reviewed changes

.changes/next-release/bugfix-AmazonS3-263fed5.json Outdated Show resolved Hide resolved

Add test case and update changelog

05ea027

RanVaknin approved these changes Aug 5, 2025

View reviewed changes

dagnir approved these changes Aug 5, 2025

View reviewed changes

zoewangg reviewed Aug 5, 2025

View reviewed changes

davidh44 added 2 commits August 5, 2025 14:50

Address comments

14cccda

Address comments

9749657

L-Applin reviewed Aug 6, 2025

View reviewed changes

Fix bug in MultipartS3AsyncClient GetObject Retryable Errors #6309

Are you sure you want to change the base?

Fix bug in MultipartS3AsyncClient GetObject Retryable Errors #6309

Uh oh!

Conversation

davidh44 commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

dagnir commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Aug 5, 2025

Quality Gate failed

Uh oh!

Uh oh!

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!

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!

Uh oh!

davidh44 commented Jul 31, 2025 •

edited

Loading