Skip to content

fix(s3): preserve original cause in ParallelMultipartDownloaderSubscriber.onError#7175

Open
rishabhjainps wants to merge 1 commit into
aws:masterfrom
rishabhjainps:fix/multipart-download-onerror-cause-ordering
Open

fix(s3): preserve original cause in ParallelMultipartDownloaderSubscriber.onError#7175
rishabhjainps wants to merge 1 commit into
aws:masterfrom
rishabhjainps:fix/multipart-download-onerror-cause-ordering

Conversation

@rishabhjainps

@rishabhjainps rishabhjainps commented Jul 21, 2026

Copy link
Copy Markdown

Motivation and Context

Fixes #7174.

When a multipart download (S3AsyncClient with multipartEnabled(true), e.g. via S3TransferManager.downloadFile) fails, the caller's completion future can complete with a bare java.util.concurrent.CancellationException that has no cause attached, and the real trigger is never logged.

The cause is the ordering in ParallelMultipartDownloaderSubscriber.onError:

public void onError(Throwable t) {
    inFlightRequests.values().forEach(future -> future.cancel(true)); // cancel first
    inFlightRequests.clear();
    resultFuture.completeExceptionally(t);                            // real cause last
}

Each part future is wired to resultFuture with CompletableFutureUtils.forwardExceptionTo(resultFuture, response) in sendNextRequest/sendFirstRequest. Cancelling those part futures therefore forwards a CancellationException onto resultFuture before resultFuture.completeExceptionally(t) runs, so the original t loses the race and is discarded. The method also does not log t, so the trigger is invisible even at DEBUG.

The sibling class ParallelPresignedUrlMultipartDownloaderSubscriber.onError already does this correctly — it completes resultFuture before cancelling and logs the cause. This change brings ParallelMultipartDownloaderSubscriber in line with it.

Modifications

In ParallelMultipartDownloaderSubscriber.onError:

  • Complete resultFuture with the original Throwable before cancelling the in-flight part futures, so the real cause wins the race instead of a CancellationException.
  • Log the cause at debug level, matching the sibling class.

Testing

  • Added ParallelMultipartDownloaderSubscriberTest, which verifies that after onError the caller-facing resultFuture completes with the original throwable (not a CancellationException), that this holds both with and without in-flight part requests, and that in-flight part futures are still cancelled. The test fails against the previous cancel-before-complete ordering and passes with this change.
  • Built and tested locally on the services/s3 module with Corretto 21: ./mvnw -pl services/s3 -am install succeeds; -Dtest=ParallelMultipartDownloaderSubscriberTest runs 3/3 green; checkstyle:check on the module is clean.

Types of changes

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

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@rishabhjainps
rishabhjainps requested a review from a team as a code owner July 21, 2026 19:52
…iber.onError

onError cancelled the in-flight part requests before completing resultFuture
with the failure cause. Each part future is wired to resultFuture via
CompletableFutureUtils.forwardExceptionTo, so cancelling first raced a
CancellationException onto resultFuture and discarded the original cause;
callers observing the download's completion future saw a bare
CancellationException with no root cause, and the trigger was never logged.

Complete resultFuture with the original throwable first, then cancel the
in-flight parts, and log the cause at debug level, matching the sibling
ParallelPresignedUrlMultipartDownloaderSubscriber. Add a unit test covering the
onError cause-preservation ordering.
@rishabhjainps
rishabhjainps force-pushed the fix/multipart-download-onerror-cause-ordering branch from 6d63b16 to d4efe6e Compare July 21, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ParallelMultipartDownloaderSubscriber.onError cancels part futures before completing resultFuture, swallowing the original error

1 participant