Fix file settings service tests (#115770)

This PR addresses some of the failure causes tracked under
#115280 and
#115725: the latch-await
setup was rather convoluted and the move command not always correctly
invoked in the right order. This PR cleans up latching by separating
awaiting the first processing call (on start) from waiting on the
subsequent call. Also, it makes writing the file more robust w.r.t.
OS'es where `atomic_move` may not be available.

This should address failures around the timeout await, and the assertion
failures around invoked methods tracked here:

#115725 (comment)

But will likely require another round of changes to address the failures
to delete files.

Relates: #115280 
Relates: #115725

Author: n1v0lg

muted-tests.yml

@@ -236,9 +236,6 @@ tests:
 - class: org.elasticsearch.xpack.inference.DefaultEndPointsIT
   method: testInferDeploysDefaultE5
   issue: https://github.com/elastic/elasticsearch/issues/115361
-- class: org.elasticsearch.reservedstate.service.FileSettingsServiceTests
-  method: testProcessFileChanges
-  issue: https://github.com/elastic/elasticsearch/issues/115280
 - class: org.elasticsearch.xpack.security.FileSettingsRoleMappingsRestartIT
   method: testFileSettingsReprocessedOnRestartWithoutVersionChange
   issue: https://github.com/elastic/elasticsearch/issues/115450

server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java

@@ -24,6 +24,8 @@
 import org.elasticsearch.common.component.Lifecycle;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.IOUtils;
+import org.elasticsearch.core.Strings;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.env.BuildVersion;
 import org.elasticsearch.env.Environment;
@@ -39,9 +41,10 @@
 import org.mockito.stubbing.Answer;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.StandardCopyOption;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -50,6 +53,8 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 
+import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 import static org.elasticsearch.node.Node.NODE_NAME_SETTING;
 import static org.hamcrest.Matchers.anEmptyMap;
 import static org.hamcrest.Matchers.hasEntry;
@@ -190,9 +195,7 @@ public void testInitialFileWorks() throws Exception {
             return null;
         }).when(controller).process(any(), any(XContentParser.class), any(), any());
 
-        CountDownLatch latch = new CountDownLatch(1);
-
-        fileSettingsService.addFileChangedListener(latch::countDown);
+        CountDownLatch fileProcessingLatch = new CountDownLatch(1);
 
         Files.createDirectories(fileSettingsService.watchedFileDir());
         // contents of the JSON don't matter, we just need a file to exist
@@ -202,15 +205,14 @@ public void testInitialFileWorks() throws Exception {
             try {
                 return invocation.callRealMethod();
             } finally {
-                latch.countDown();
+                fileProcessingLatch.countDown();
             }
         }).when(fileSettingsService).processFileOnServiceStart();
 
         fileSettingsService.start();
         fileSettingsService.clusterChanged(new ClusterChangedEvent("test", clusterService.state(), ClusterState.EMPTY_STATE));
 
-        // wait for listener to be called
-        assertTrue(latch.await(20, TimeUnit.SECONDS));
+        longAwait(fileProcessingLatch);
 
         verify(fileSettingsService, times(1)).processFileOnServiceStart();
         verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION), any());
@@ -223,40 +225,40 @@ public void testProcessFileChanges() throws Exception {
             return null;
         }).when(controller).process(any(), any(XContentParser.class), any(), any());
 
-        // we get three events: initial clusterChanged event, first write, second write
-        CountDownLatch latch = new CountDownLatch(3);
-
-        fileSettingsService.addFileChangedListener(latch::countDown);
-
-        Files.createDirectories(fileSettingsService.watchedFileDir());
-        // contents of the JSON don't matter, we just need a file to exist
-        writeTestFile(fileSettingsService.watchedFile(), "{}");
-
+        CountDownLatch changesOnStartLatch = new CountDownLatch(1);
         doAnswer((Answer<?>) invocation -> {
             try {
                 return invocation.callRealMethod();
             } finally {
-                latch.countDown();
+                changesOnStartLatch.countDown();
             }
         }).when(fileSettingsService).processFileOnServiceStart();
+
+        CountDownLatch changesLatch = new CountDownLatch(1);
         doAnswer((Answer<?>) invocation -> {
             try {
                 return invocation.callRealMethod();
             } finally {
-                latch.countDown();
+                changesLatch.countDown();
             }
         }).when(fileSettingsService).processFileChanges();
 
+        Files.createDirectories(fileSettingsService.watchedFileDir());
+        // contents of the JSON don't matter, we just need a file to exist
+        writeTestFile(fileSettingsService.watchedFile(), "{}");
+
         fileSettingsService.start();
         fileSettingsService.clusterChanged(new ClusterChangedEvent("test", clusterService.state(), ClusterState.EMPTY_STATE));
-        // second file change; contents still don't matter
-        overwriteTestFile(fileSettingsService.watchedFile(), "{}");
 
-        // wait for listener to be called (once for initial processing, once for subsequent update)
-        assertTrue(latch.await(20, TimeUnit.SECONDS));
+        longAwait(changesOnStartLatch);
 
         verify(fileSettingsService, times(1)).processFileOnServiceStart();
         verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION), any());
+
+        // second file change; contents still don't matter
+        writeTestFile(fileSettingsService.watchedFile(), "[]");
+        longAwait(changesLatch);
+
         verify(fileSettingsService, times(1)).processFileChanges();
         verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_VERSION_ONLY), any());
     }
@@ -295,9 +297,7 @@ public void testStopWorksInMiddleOfProcessing() throws Exception {
         // Make some fake settings file to cause the file settings service to process it
         writeTestFile(fileSettingsService.watchedFile(), "{}");
 
-        // we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file,
-        // on Linux is instantaneous. Windows is instantaneous too.
-        assertTrue(processFileLatch.await(30, TimeUnit.SECONDS));
+        longAwait(processFileLatch);
 
         // Stopping the service should interrupt the watcher thread, we should be able to stop
         fileSettingsService.stop();
@@ -352,15 +352,34 @@ public void testHandleSnapshotRestoreResetsMetadata() throws Exception {
     }
 
     // helpers
-    private void writeTestFile(Path path, String contents) throws IOException {
-        Path tempFilePath = createTempFile();
-        Files.writeString(tempFilePath, contents);
-        Files.move(tempFilePath, path, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+    private static void writeTestFile(Path path, String contents) {
+        Path tempFile = null;
+        try {
+            tempFile = Files.createTempFile(path.getParent(), path.getFileName().toString(), "tmp");
+            Files.writeString(tempFile, contents);
+
+            try {
+                Files.move(tempFile, path, REPLACE_EXISTING, ATOMIC_MOVE);
+            } catch (AtomicMoveNotSupportedException e) {
+                Files.move(tempFile, path, REPLACE_EXISTING);
+            }
+        } catch (final IOException e) {
+            throw new UncheckedIOException(Strings.format("could not write file [%s]", path.toAbsolutePath()), e);
+        } finally {
+            // we are ignoring exceptions here, so we do not need handle whether or not tempFile was initialized nor if the file exists
+            IOUtils.deleteFilesIgnoringExceptions(tempFile);
+        }
     }
 
-    private void overwriteTestFile(Path path, String contents) throws IOException {
-        Path tempFilePath = createTempFile();
-        Files.writeString(tempFilePath, contents);
-        Files.move(tempFilePath, path, StandardCopyOption.REPLACE_EXISTING);
+    // this waits for up to 20 seconds to account for watcher service differences between OSes
+    // on MacOS it may take up to 10 seconds for the Java watcher service to notice the file,
+    // on Linux is instantaneous. Windows is instantaneous too.
+    private static void longAwait(CountDownLatch latch) {
+        try {
+            assertTrue("longAwait: CountDownLatch did not reach zero within the timeout", latch.await(20, TimeUnit.SECONDS));
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            fail(e, "longAwait: interrupted waiting for CountDownLatch to reach zero");
+        }
     }
 }