From 413f2c6f5ec48de221c38526725ef1ceb13b6a1e Mon Sep 17 00:00:00 2001 From: RanVaknin <> Date: Tue, 19 May 2026 21:04:19 +0000 Subject: [PATCH 1/2] Skip file downloads in DownloadDirectoryHelper after the operation is cancelled to prevent directory recreation --- .../bugfix-S3TransferManager-8858.json | 6 +++ .../s3/internal/DownloadDirectoryHelper.java | 21 +++++--- .../internal/DownloadDirectoryHelperTest.java | 51 ++++++++++++++++++- 3 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 .changes/next-release/bugfix-S3TransferManager-8858.json diff --git a/.changes/next-release/bugfix-S3TransferManager-8858.json b/.changes/next-release/bugfix-S3TransferManager-8858.json new file mode 100644 index 000000000000..6b9c171aa60c --- /dev/null +++ b/.changes/next-release/bugfix-S3TransferManager-8858.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "S3 Transfer Manager", + "contributor": "", + "description": "Fix a resource leak in `downloadDirectory` where cancelling the download could still allow late-arriving file transfers to recreate the destination directory. Added a guard in `DownloadDirectoryHelper` to skip file downloads after the operation is cancelled." +} diff --git a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java index 69f59473d060..3d22103c4a7c 100644 --- a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java +++ b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java @@ -106,7 +106,7 @@ private void doDownloadDirectory(CompletableFuture r CompletableFuture allOfFutures = new CompletableFuture<>(); AsyncBufferingSubscriber asyncBufferingSubscriber = - new AsyncBufferingSubscriber<>(downloadSingleFile(downloadDirectoryRequest, request, + new AsyncBufferingSubscriber<>(downloadSingleFile(returnFuture, downloadDirectoryRequest, request, failedFileDownloads), allOfFutures, transferConfiguration.option( @@ -129,11 +129,13 @@ private void doDownloadDirectory(CompletableFuture r } private Function> downloadSingleFile( + CompletableFuture returnFuture, DownloadDirectoryRequest downloadDirectoryRequest, ListObjectsV2Request listRequest, Queue failedFileDownloads) { - return s3Object -> doDownloadSingleFile(downloadDirectoryRequest, + return s3Object -> doDownloadSingleFile(returnFuture, + downloadDirectoryRequest, failedFileDownloads, listRequest, s3Object); @@ -158,10 +160,17 @@ private void validatePath(Path destinationDirectory, Path targetPath, String key } } - private CompletableFuture doDownloadSingleFile(DownloadDirectoryRequest downloadDirectoryRequest, - Collection failedFileDownloads, - ListObjectsV2Request listRequest, - S3Object s3Object) { + private CompletableFuture doDownloadSingleFile( + CompletableFuture returnFuture, + DownloadDirectoryRequest downloadDirectoryRequest, + Collection failedFileDownloads, + ListObjectsV2Request listRequest, + S3Object s3Object) { + + if (returnFuture.isDone()) { + return CompletableFutureUtils.failedFuture( + SdkClientException.create("Download was cancelled before file could be started")); + } Path destinationPath = determineDestinationPath(downloadDirectoryRequest, listRequest, s3Object); diff --git a/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java index dc8536e8fc6c..1474f073a340 100644 --- a/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java +++ b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java @@ -39,6 +39,7 @@ import java.util.UUID; import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.Phaser; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -55,6 +56,7 @@ import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; +import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.services.s3.model.EncodingType; import software.amazon.awssdk.services.s3.model.GetObjectRequest; @@ -72,6 +74,7 @@ import software.amazon.awssdk.transfer.s3.model.FileDownload; import software.amazon.awssdk.transfer.s3.progress.LoggingTransferListener; import software.amazon.awssdk.transfer.s3.progress.TransferListener; +import software.amazon.awssdk.utils.async.SimplePublisher; public class DownloadDirectoryHelperTest { private static final String DIRECTORY_NAME = "test"; @@ -197,13 +200,22 @@ void downloadDirectory_cancel_shouldCancelAllFutures() throws Exception { CompletableFuture future2 = new CompletableFuture<>(); FileDownload fileDownload2 = newDownload(future2); - when(singleDownloadFunction.apply(any(DownloadFileRequest.class))).thenReturn(fileDownload, fileDownload2); + AtomicInteger callCount = new AtomicInteger(0); + CountDownLatch bothStarted = new CountDownLatch(2); + when(singleDownloadFunction.apply(any(DownloadFileRequest.class))).thenAnswer(invocation -> { + bothStarted.countDown(); + return callCount.incrementAndGet() == 1 ? fileDownload : fileDownload2; + }); DirectoryDownload downloadDirectory = downloadDirectoryHelper.downloadDirectory(DownloadDirectoryRequest.builder() .destination(directory) .bucket("bucket") .build()); + + // Wait for both downloads to have started before cancelling + assertThat(bothStarted.await(5, TimeUnit.SECONDS)).isTrue(); + downloadDirectory.completionFuture().cancel(true); assertThatThrownBy(() -> future.get(1, TimeUnit.SECONDS)) @@ -567,6 +579,43 @@ private FileDownload newFailedDownload(SdkClientException exception) { return fileDownload2; } + /** + * Verifies that after a directory download is cancelled, subsequent S3 objects do not cause + * directories to be created on the filesystem. This reproduces the bug where a late-arriving + * S3Object would call createParentDirectoriesIfNeeded() and recreate a directory that was + * already cleaned up. + */ + @Test + void downloadDirectory_cancelledFuture_shouldNotCreateDirectories() throws Exception { + SimplePublisher publisher = new SimplePublisher<>(); + when(listObjectsHelper.listS3ObjectsRecursively(any(ListObjectsV2Request.class))) + .thenReturn(SdkPublisher.adapt(publisher)); + + DownloadDirectoryHelper helper = new DownloadDirectoryHelper( + TransferManagerConfiguration.builder().build(), + listObjectsHelper, + singleDownloadFunction); + + DirectoryDownload directoryDownload = helper.downloadDirectory( + DownloadDirectoryRequest.builder() + .destination(directory) + .bucket("bucket") + .build()); + + // Cancel before any items are delivered + directoryDownload.completionFuture().cancel(true); + + // Now deliver an item — this simulates a late-arriving S3Object after cancel + publisher.send(S3Object.builder().key("subdir/file.txt").size(100L).build()); + publisher.complete(); + + Thread.sleep(200); + + // The subdirectory should NOT have been created + Path subdir = directory.resolve("subdir"); + assertThat(Files.exists(subdir)).isFalse(); + } + private FileDownload newDownload(CompletableFuture future) { return new DefaultFileDownload(future, new DefaultTransferProgress(DefaultTransferProgressSnapshot.builder() From 2c2dc4f0d1e8c6f76af5ab2540e88d2e4f0c88a0 Mon Sep 17 00:00:00 2001 From: RanVaknin <> Date: Tue, 19 May 2026 21:07:15 +0000 Subject: [PATCH 2/2] fix formatting --- .../next-release/bugfix-S3TransferManager-8858.json | 2 +- .../s3/internal/DownloadDirectoryHelperTest.java | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/.changes/next-release/bugfix-S3TransferManager-8858.json b/.changes/next-release/bugfix-S3TransferManager-8858.json index 6b9c171aa60c..2c77bf9c132a 100644 --- a/.changes/next-release/bugfix-S3TransferManager-8858.json +++ b/.changes/next-release/bugfix-S3TransferManager-8858.json @@ -2,5 +2,5 @@ "type": "bugfix", "category": "S3 Transfer Manager", "contributor": "", - "description": "Fix a resource leak in `downloadDirectory` where cancelling the download could still allow late-arriving file transfers to recreate the destination directory. Added a guard in `DownloadDirectoryHelper` to skip file downloads after the operation is cancelled." + "description": "Fix a resource leak in `downloadDirectory` where cancelling the download could still allow file transfers that arrive late to recreate the destination directory. Added a guard in `DownloadDirectoryHelper` to skip file downloads after the operation is cancelled." } diff --git a/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java index 1474f073a340..ac3bc291af10 100644 --- a/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java +++ b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java @@ -213,7 +213,6 @@ void downloadDirectory_cancel_shouldCancelAllFutures() throws Exception { .bucket("bucket") .build()); - // Wait for both downloads to have started before cancelling assertThat(bothStarted.await(5, TimeUnit.SECONDS)).isTrue(); downloadDirectory.completionFuture().cancel(true); @@ -579,12 +578,6 @@ private FileDownload newFailedDownload(SdkClientException exception) { return fileDownload2; } - /** - * Verifies that after a directory download is cancelled, subsequent S3 objects do not cause - * directories to be created on the filesystem. This reproduces the bug where a late-arriving - * S3Object would call createParentDirectoriesIfNeeded() and recreate a directory that was - * already cleaned up. - */ @Test void downloadDirectory_cancelledFuture_shouldNotCreateDirectories() throws Exception { SimplePublisher publisher = new SimplePublisher<>(); @@ -602,16 +595,13 @@ void downloadDirectory_cancelledFuture_shouldNotCreateDirectories() throws Excep .bucket("bucket") .build()); - // Cancel before any items are delivered directoryDownload.completionFuture().cancel(true); - // Now deliver an item — this simulates a late-arriving S3Object after cancel publisher.send(S3Object.builder().key("subdir/file.txt").size(100L).build()); publisher.complete(); Thread.sleep(200); - // The subdirectory should NOT have been created Path subdir = directory.resolve("subdir"); assertThat(Files.exists(subdir)).isFalse(); }