Fix async getObject(request, Path) deleting a pre-existing file when the download fails before writing (#7353)

Author: davidh44

.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."
+}

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<ResponseT> prepare() {
+        fileChannel = null;
         cf = new CompletableFuture<>();
         cf.whenComplete((r, t) -> {
             if (t != null && fileChannel != null) {
@@ -169,14 +170,16 @@ public void onStream(SdkPublisher<ByteBuffer> 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));

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<String> transformer = new FileAsyncResponseTransformer<>(testPath, configuration);
+        CompletableFuture<String> 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<String> transformer = new FileAsyncResponseTransformer<>(testPath);
+
+        stubException(transformer);
+        assertThat(testPath).doesNotExist();
+
+        String existingContent = RandomStringUtils.randomAlphanumeric(1000);
+        Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8));
+        CompletableFuture<String> 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<String> 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<String> 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<FileTransformerConfiguration> deleteConfigurations() {
+        List<FileTransformerConfiguration> conf = new ArrayList<>();
+        for (FileWriteOption fileWriteOption : FileWriteOption.values()) {
+            conf.add(FileTransformerConfiguration.builder()
+                                                 .fileWriteOption(fileWriteOption)
+                                                 .failureBehavior(DELETE)
+                                                 .build());
+        }
+        return conf;
+    }
+
     private static List<FileTransformerConfiguration> configurations() {
         List<FileTransformerConfiguration> conf = new ArrayList<>();
         conf.add(FileTransformerConfiguration.defaultCreateNew());