Node should start up despite of a lingering `.es_temp_file` (#21210)

When ES starts up we verify we can write to all data folders and that they support atomic moves. We do so by creating and deleting temp files. If for some reason the files was successfully created but not successfully deleted, we still shut down correctly but subsequent start attempts will fail with a file already exists exception.

This commit makes sure to first clean any existing temporary files.

Superseeds #21007

core/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -901,11 +901,12 @@ public final class NodeEnvironment  implements Closeable {
         final NodePath[] nodePaths = nodePaths();
         for (NodePath nodePath : nodePaths) {
             assert Files.isDirectory(nodePath.path) : nodePath.path + " is not a directory";
-            final Path src = nodePath.path.resolve("__es__.tmp");
-            final Path target = nodePath.path.resolve("__es__.final");
+            final Path src = nodePath.path.resolve(TEMP_FILE_NAME + ".tmp");
+            final Path target = nodePath.path.resolve(TEMP_FILE_NAME + ".final");
             try {
+                Files.deleteIfExists(src);
                 Files.createFile(src);
-                Files.move(src, target, StandardCopyOption.ATOMIC_MOVE);
+                Files.move(src, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
             } catch (AtomicMoveNotSupportedException ex) {
                 throw new IllegalStateException("atomic_move is not supported by the filesystem on path ["
                         + nodePath.path
@@ -1005,19 +1006,19 @@ public final class NodeEnvironment  implements Closeable {
         }
     }
 
+    // package private for testing
+    static final String TEMP_FILE_NAME = ".es_temp_file";
+
     private static void tryWriteTempFile(Path path) throws IOException {
         if (Files.exists(path)) {
-            Path resolve = path.resolve(".es_temp_file");
-            boolean tempFileCreated = false;
+            Path resolve = path.resolve(TEMP_FILE_NAME);
             try {
-                Files.createFile(resolve);
-                tempFileCreated = true;
-            } catch (IOException ex) {
-                throw new IOException("failed to write in data directory [" + path + "] write permission is required", ex);
-            } finally {
-                if (tempFileCreated) {
+                // delete any lingering file from a previous failure
                 Files.deleteIfExists(resolve);
-                }
+                Files.createFile(resolve);
+                Files.delete(resolve);
+            } catch (IOException ex) {
+                throw new IOException("failed to test writes in data directory [" + path + "] write permission is required", ex);
             }
         }
     }

core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java

@@ -34,6 +34,7 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.IndexSettingsModule;
 
 import java.io.IOException;
+import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -416,6 +417,39 @@ public class NodeEnvironmentTests extends ESTestCase {
         env.close();
     }
 
+    public void testExistingTempFiles() throws IOException {
+        String[] paths = tmpPaths();
+        // simulate some previous left over temp files
+        for (String path : randomSubsetOf(randomIntBetween(1, paths.length), paths)) {
+            final Path nodePath = NodeEnvironment.resolveNodePath(PathUtils.get(path), 0);
+            Files.createDirectories(nodePath);
+            Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME));
+            if (randomBoolean()) {
+                Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".tmp"));
+            }
+            if (randomBoolean()) {
+                Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".final"));
+            }
+        }
+        NodeEnvironment env = newNodeEnvironment(paths, Settings.EMPTY);
+        try {
+            env.ensureAtomicMoveSupported();
+        } catch (AtomicMoveNotSupportedException e) {
+            // that's OK :)
+        }
+        env.close();
+        // check we clean up
+        for (String path: paths) {
+            final Path nodePath = NodeEnvironment.resolveNodePath(PathUtils.get(path), 0);
+            final Path tempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME);
+            assertFalse(tempFile + " should have been cleaned", Files.exists(tempFile));
+            final Path srcTempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".src");
+            assertFalse(srcTempFile + " should have been cleaned", Files.exists(srcTempFile));
+            final Path targetTempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".target");
+            assertFalse(targetTempFile + " should have been cleaned", Files.exists(targetTempFile));
+        }
+    }
+
     /** Converts an array of Strings to an array of Paths, adding an additional child if specified */
     private Path[] stringsToPaths(String[] strings, String additional) {
         Path[] locations = new Path[strings.length];

qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java

@@ -30,7 +30,6 @@ import java.nio.file.Path;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashSet;
 
 public class NodeEnvironmentEvilTests extends ESTestCase {
@@ -75,7 +74,7 @@ public class NodeEnvironmentEvilTests extends ESTestCase {
             IOException ioException = expectThrows(IOException.class, () -> {
                 new NodeEnvironment(build, new Environment(build));
             });
-            assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to write in data directory"));
+            assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to test writes in data directory"));
         }
     }
 
@@ -100,7 +99,7 @@ public class NodeEnvironmentEvilTests extends ESTestCase {
             IOException ioException = expectThrows(IOException.class, () -> {
                 new NodeEnvironment(build, new Environment(build));
             });
-            assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to write in data directory"));
+            assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to test writes in data directory"));
         }
     }
 }