Fix dead temporary files

Pushed-by: k8s-git-ops<admin@cloudogu.com>
Co-authored-by: René Pfeuffer<rene.pfeuffer@cloudogu.com>
Committed-by: René Pfeuffer<rene.pfeuffer@cloudogu.com>

gradle/changelog/temporary-files.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Deletion of temporary files on exceptions

scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java

@@ -62,7 +62,13 @@ public final class CopyOnWrite {
     validateInput(targetFile);
     execute(() -> {
       Path temporaryFile = createTemporaryFile(targetFile);
-      executeCallback(writer, targetFile, temporaryFile);
+      try {
+        executeCallback(writer, targetFile, temporaryFile);
+      } catch (Exception e) {
+        LOG.warn("error writing temporary file");
+        deleteTemporaryFile(temporaryFile);
+        throw e;
+      }
       replaceOriginalFile(targetFile, temporaryFile);
       onSuccess.run();
     }).withLockedFileForWrite(targetFile);
@@ -170,7 +176,7 @@ public final class CopyOnWrite {
       restoreBackup(targetFile, backupFile);
       throw new StoreException("could rename temporary file to target file", e);
     }
-    deleteBackupFile(backupFile);
+    deleteTemporaryFile(backupFile);
   }
 
   @SuppressWarnings("squid:S3725") // performance of Files#exists
@@ -190,7 +196,7 @@ public final class CopyOnWrite {
     }
   }
 
-  private static void deleteBackupFile(Path backupFile) {
+  private static void deleteTemporaryFile(Path backupFile) {
     if (backupFile != null) {
       try {
         Files.delete(backupFile);

scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java

@@ -24,7 +24,6 @@
 
 package sonia.scm.store;
 
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -35,6 +34,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static sonia.scm.store.CopyOnWrite.withTemporaryFile;
 
@@ -50,7 +50,7 @@ class CopyOnWriteTest {
       }
     }, expectedFile);
 
-    Assertions.assertThat(expectedFile).hasContent("great success");
+    assertThat(expectedFile).hasContent("great success");
   }
 
   @Test
@@ -64,7 +64,7 @@ class CopyOnWriteTest {
       }
     }, expectedFile);
 
-    Assertions.assertThat(expectedFile).hasContent("great success");
+    assertThat(expectedFile).hasContent("great success");
   }
 
   @Test
@@ -101,7 +101,22 @@ class CopyOnWriteTest {
         },
         unchangedOriginalFile));
 
-    Assertions.assertThat(unchangedOriginalFile).hasContent("this should be kept");
+    assertThat(unchangedOriginalFile).hasContent("this should be kept");
+  }
+
+  @Test
+  void shouldDeleteTemporaryFileIfFileCouldNotBeWritten(@TempDir Path tempDir) throws IOException {
+    Path unchangedOriginalFile = tempDir.resolve("target.txt");
+
+    assertThrows(
+      StoreException.class,
+      () -> withTemporaryFile(
+        file -> {
+          throw new IOException("test");
+        },
+        unchangedOriginalFile));
+
+    assertThat(tempDir).isEmptyDirectory();
   }
 
   @Test
@@ -131,7 +146,7 @@ class CopyOnWriteTest {
         Files::delete,
         backedUpFile));
 
-    Assertions.assertThat(backedUpFile).hasContent("this should be kept");
+    assertThat(backedUpFile).hasContent("this should be kept");
   }
 
   @Test
@@ -148,6 +163,6 @@ class CopyOnWriteTest {
       }
     }, expectedFile);
 
-    Assertions.assertThat(Files.list(tempDir)).hasSize(1);
+    assertThat(Files.list(tempDir)).hasSize(1);
   }
 }