From 5e4be8464fc8f107b0e901bc684ecf8d02da679b Mon Sep 17 00:00:00 2001 From: David Ho Date: Fri, 4 Sep 2026 11:40:51 -0700 Subject: [PATCH] Fix async getObject(request, Path) deleting a pre-existing file when the download fails before writing --- .../bugfix-AWSSDKforJavav2-7c3e9d1.json | 6 ++ .../async/FileAsyncResponseTransformer.java | 9 +- .../FileAsyncResponseTransformerTest.java | 82 ++++++++++++++++++- 3 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-7c3e9d1.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-7c3e9d1.json b/.changes/next-release/bugfix-AWSSDKforJavav2-7c3e9d1.json new file mode 100644 index 000000000000..8f61af10391c --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-7c3e9d1.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "Prevent `AsyncResponseTransformer.toFile` from deleting a pre-existing destination when a request fails before the SDK opens the file. Calls using `CREATE_NEW` now preserve the existing file and surface `FileAlreadyExistsException` instead of allowing a retry to overwrite it." +} diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java index c35d28c53601..752af9ea4c28 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java @@ -139,6 +139,7 @@ private AsynchronousFileChannel createChannel(Path path) throws IOException { @Override public CompletableFuture prepare() { + fileChannel = null; cf = new CompletableFuture<>(); cf.whenComplete((r, t) -> { if (t != null && fileChannel != null) { @@ -169,14 +170,16 @@ public void onStream(SdkPublisher publisher) { @Override public void exceptionOccurred(Throwable throwable) { + AsynchronousFileChannel currentFileChannel = fileChannel; + fileChannel = null; try { - if (fileChannel != null) { + if (currentFileChannel != null) { runAndLogError(log.logger(), String.format("Failed to close the file %s, resource may be leaked", path), - () -> fileChannel.close()); + currentFileChannel::close); } } finally { - if (configuration.failureBehavior() == FailureBehavior.DELETE) { + if (currentFileChannel != null && configuration.failureBehavior() == FailureBehavior.DELETE) { runAndLogError(log.logger(), String.format("Failed to delete the file %s", path), () -> Files.deleteIfExists(path)); diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java index 0464489b081f..81a8fa87de9e 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java @@ -121,7 +121,8 @@ public void synchronousPublisher_shouldNotHang() throws Exception { @Test void noConfiguration_fileAlreadyExists_shouldThrowException() throws Exception { Path testPath = testFs.getPath("test_file.txt"); - Files.write(testPath, RandomStringUtils.random(1000).getBytes(StandardCharsets.UTF_8)); + String existingContent = RandomStringUtils.randomAlphanumeric(1000); + Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8)); assertThat(testPath).exists(); String content = RandomStringUtils.randomAlphanumeric(30000); @@ -131,6 +132,7 @@ void noConfiguration_fileAlreadyExists_shouldThrowException() throws Exception { transformer.onResponse("foobar"); transformer.onStream(testPublisher(content)); assertThatThrownBy(() -> future.join()).hasRootCauseInstanceOf(FileAlreadyExistsException.class); + assertThat(testPath).hasContent(existingContent); } @Test @@ -186,6 +188,73 @@ void createOrAppendExisting_fileExists_shouldAppend() throws Exception { assertThat(testPath).hasContent(existingString + content); } + @ParameterizedTest + @MethodSource("deleteConfigurations") + void exceptionOccurred_beforeFileOpened_shouldPreserveExistingFile(FileTransformerConfiguration configuration) + throws Exception { + Path testPath = testFs.getPath("test_file.txt"); + String existingContent = RandomStringUtils.randomAlphanumeric(1000); + Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8)); + + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath, configuration); + CompletableFuture future = transformer.prepare(); + RuntimeException exception = new RuntimeException("oops"); + transformer.exceptionOccurred(exception); + + assertThat(future).failsWithin(1, TimeUnit.SECONDS) + .withThrowableOfType(ExecutionException.class) + .withCause(exception); + assertThat(testPath).hasContent(existingContent); + } + + @Test + void exceptionOccurred_beforeFileOpenedOnRetry_shouldPreserveExistingFile() throws Exception { + Path testPath = testFs.getPath("test_file.txt"); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath); + + stubException(transformer); + assertThat(testPath).doesNotExist(); + + String existingContent = RandomStringUtils.randomAlphanumeric(1000); + Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8)); + CompletableFuture future = transformer.prepare(); + RuntimeException exception = new RuntimeException("oops"); + transformer.exceptionOccurred(exception); + + assertThat(future).failsWithin(1, TimeUnit.SECONDS) + .withThrowableOfType(ExecutionException.class) + .withCause(exception); + assertThat(testPath).hasContent(existingContent); + } + + @Test + void exceptionOccurred_afterFileOpened_shouldAllowRetry() throws Exception { + Path testPath = testFs.getPath("test_file.txt"); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath); + + stubException(transformer); + assertThat(testPath).doesNotExist(); + + String content = RandomStringUtils.randomAlphanumeric(1000); + stubSuccessfulStreaming(content, transformer); + assertThat(testPath).hasContent(content); + } + + @Test + void exceptionOccurred_calledTwice_shouldNotDeleteReplacement() throws Exception { + Path testPath = testFs.getPath("test_file.txt"); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath); + + stubException(transformer); + assertThat(testPath).doesNotExist(); + + String replacementContent = RandomStringUtils.randomAlphanumeric(1000); + Files.write(testPath, replacementContent.getBytes(StandardCharsets.UTF_8)); + transformer.exceptionOccurred(new RuntimeException("second callback")); + + assertThat(testPath).hasContent(replacementContent); + } + @ParameterizedTest @MethodSource("configurations") void exceptionOccurred_deleteFileBehavior(FileTransformerConfiguration configuration) throws Exception { @@ -203,6 +272,17 @@ void exceptionOccurred_deleteFileBehavior(FileTransformerConfiguration configura } } + private static List deleteConfigurations() { + List conf = new ArrayList<>(); + for (FileWriteOption fileWriteOption : FileWriteOption.values()) { + conf.add(FileTransformerConfiguration.builder() + .fileWriteOption(fileWriteOption) + .failureBehavior(DELETE) + .build()); + } + return conf; + } + private static List configurations() { List conf = new ArrayList<>(); conf.add(FileTransformerConfiguration.defaultCreateNew());