From c369723af02d991f436522e64f811ecd0d32a2e4 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Thu, 19 Dec 2024 10:12:07 +0100
Subject: [PATCH] Simplify AtomicFileWriter and use clearer temporary file
 names (#10058)

* Simplify AtomicFileWriter and use clearer temporary file names

* Uses a more readable name for the temporary file, derived from the target path
* Streamline the fallback logic for atomic move, and just let the error bubble up otherwise rather than logging then throwing which was leading to duplicate stacktraces in the system logs

Related to https://github.com/jenkinsci/jenkins/pull/2548#discussion_r79500936

* Persist fallback to non-atomic move if filesystem throws AtomicMoveNotSupportedException

* Extract to a static method

* Fix reviews
---
 .../java/hudson/util/AtomicFileWriter.java    | 60 +++++++++----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java
index fabe15c5a6b6..252605c06de7 100644
--- a/core/src/main/java/hudson/util/AtomicFileWriter.java
+++ b/core/src/main/java/hudson/util/AtomicFileWriter.java
@@ -67,6 +67,11 @@ public class AtomicFileWriter extends Writer {
     private static /* final */ boolean REQUIRES_DIR_FSYNC = SystemProperties.getBoolean(
             AtomicFileWriter.class.getName() + ".REQUIRES_DIR_FSYNC", !Functions.isWindows());
 
+    /**
+     * Whether the platform supports atomic move.
+     */
+    private static boolean atomicMoveSupported = true;
+
     static {
         if (DISABLE_FORCED_FLUSH) {
             LOGGER.log(Level.WARNING, "DISABLE_FORCED_FLUSH flag used, this could result in dataloss if failures happen in your storage subsystem.");
@@ -149,7 +154,7 @@ public AtomicFileWriter(@NonNull Path destinationPath, @NonNull Charset charset,
 
         try {
             // JENKINS-48407: NIO's createTempFile creates file with 0600 permissions, so we use pre-NIO for this...
-            tmpPath = File.createTempFile("atomic", "tmp", dir.toFile()).toPath();
+            tmpPath = File.createTempFile(destPath.getFileName() + "-atomic", "tmp", dir.toFile()).toPath();
         } catch (IOException e) {
             throw new IOException("Failed to create a temporary file in " + dir, e);
         }
@@ -207,36 +212,14 @@ public void abort() throws IOException {
     public void commit() throws IOException {
         close();
         try {
-            // Try to make an atomic move.
-            Files.move(tmpPath, destPath, StandardCopyOption.ATOMIC_MOVE);
-        } catch (IOException moveFailed) {
-            // If it falls here that can mean many things. Either that the atomic move is not supported,
-            // or something wrong happened. Anyway, let's try to be over-diagnosing
-            if (moveFailed instanceof AtomicMoveNotSupportedException) {
-                LOGGER.log(Level.WARNING, "Atomic move not supported. falling back to non-atomic move.", moveFailed);
-            } else {
-                LOGGER.log(Level.WARNING, "Unable to move atomically, falling back to non-atomic move.", moveFailed);
-            }
-
-            if (destPath.toFile().exists()) {
-                LOGGER.log(Level.INFO, "The target file {0} was already existing", destPath);
-            }
-
+            move(tmpPath, destPath);
+        } finally {
             try {
-                Files.move(tmpPath, destPath, StandardCopyOption.REPLACE_EXISTING);
-            } catch (IOException replaceFailed) {
-                replaceFailed.addSuppressed(moveFailed);
-                LOGGER.log(Level.WARNING, "Unable to move {0} to {1}. Attempting to delete {0} and abandoning.",
-                           new Path[]{tmpPath, destPath});
-                try {
-                    Files.deleteIfExists(tmpPath);
-                } catch (IOException deleteFailed) {
-                    replaceFailed.addSuppressed(deleteFailed);
-                    LOGGER.log(Level.WARNING, "Unable to delete {0}, good bye then!", tmpPath);
-                    throw replaceFailed;
-                }
-
-                throw replaceFailed;
+                // In case of prior failure, the temporary file should be deleted.
+                // If the operation succeeded, the tmpPath is already deleted.
+                Files.deleteIfExists(tmpPath);
+            } catch (IOException e) {
+                LOGGER.log(Level.WARNING, e, () -> "Failed to delete temporary file " + tmpPath + " for destination file " + destPath);
             }
         }
 
@@ -253,6 +236,23 @@ public void commit() throws IOException {
         }
     }
 
+    private static void move(Path source, Path destination) throws IOException {
+        if (atomicMoveSupported) {
+            try {
+                // Try to make an atomic move.
+                Files.move(source, destination, StandardCopyOption.ATOMIC_MOVE);
+                return;
+            } catch (AtomicMoveNotSupportedException e) {
+                // Both files are on the same filesystem, so this should not happen.
+                LOGGER.log(Level.WARNING, e, () -> "Atomic move " + source + " → " + destination + " not supported. Falling back to non-atomic move.");
+                atomicMoveSupported = false;
+            }
+        }
+        if (!atomicMoveSupported) {
+            Files.move(source, destination, StandardCopyOption.REPLACE_EXISTING);
+        }
+    }
+
     private static final class CleanupChecker implements Runnable {
         private final FileChannelWriter core;
         private final Path tmpPath;