Add retry for AccessDeniedException in AbstractFileWatchingService (#128653)

* Unmute testSymlinkUpdateTriggerReload

* Add retry for AccessDeniedException in AbstractFileWatchingService

* Update docs/changelog/128653.yaml

Author: jfreden

docs/changelog/128653.yaml

@@ -0,0 +1,5 @@
+pr: 128653
+summary: Add retry for `AccessDeniedException` in `AbstractFileWatchingService`
+area: Infra/Settings
+type: bug
+issues: []

muted-tests.yml

@@ -486,9 +486,6 @@ tests:
 - class: org.elasticsearch.packaging.test.DockerTests
   method: test124CanRestartContainerWithStackLoggingConfig
   issue: https://github.com/elastic/elasticsearch/issues/128121
-- class: org.elasticsearch.reservedstate.service.FileSettingsServiceIT
-  method: testSymlinkUpdateTriggerReload
-  issue: https://github.com/elastic/elasticsearch/issues/128619
 - class: org.elasticsearch.packaging.test.DockerTests
   method: test085EnvironmentVariablesAreRespectedUnderDockerExec
   issue: https://github.com/elastic/elasticsearch/issues/128115

server/src/main/java/org/elasticsearch/common/file/AbstractFileWatchingService.java

@@ -17,6 +17,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.AccessDeniedException;
 import java.nio.file.ClosedWatchServiceException;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
@@ -56,6 +57,7 @@ public abstract class AbstractFileWatchingService extends AbstractLifecycleCompo
 
     private static final Logger logger = LogManager.getLogger(AbstractFileWatchingService.class);
     private static final int REGISTER_RETRY_COUNT = 5;
+    private static final int ACCESS_DENIED_RETRY_COUNT = 5;
     private final Path settingsDir;
     private final Map<Path, FileUpdateState> fileUpdateState = new HashMap<>();
     private WatchService watchService; // null;
@@ -115,20 +117,33 @@ public final boolean watching() {
         return watcherThread != null;
     }
 
-    private FileUpdateState readFileUpdateState(Path path) throws IOException {
-        try {
-            BasicFileAttributes attr = filesReadAttributes(path, BasicFileAttributes.class);
-            return new FileUpdateState(attr.lastModifiedTime().toMillis(), path.toRealPath().toString(), attr.fileKey());
-        } catch (NoSuchFileException e) {
-            // file doesn't exist anymore
-            return null;
-        }
+    // package private for testing
+    FileUpdateState readFileUpdateState(Path path) throws IOException, InterruptedException {
+        int retryCount = 0;
+        do {
+            try {
+                BasicFileAttributes attr = filesReadAttributes(path, BasicFileAttributes.class);
+                return new FileUpdateState(attr.lastModifiedTime().toMillis(), path.toRealPath().toString(), attr.fileKey());
+            } catch (NoSuchFileException e) {
+                // file doesn't exist anymore
+                return null;
+            } catch (AccessDeniedException e) {
+                // This can happen on Windows when a symlink is deleted for a path while path.toRealPath() is called. In most cases the
+                // symlink is recreated, so retry
+                if (retryCount == ACCESS_DENIED_RETRY_COUNT - 1) {
+                    throw e;
+                }
+                logger.debug("Could not read file state [{}] attempt [{}]", path, retryCount);
+                Thread.sleep(retryDelayMillis(retryCount));
+                retryCount++;
+            }
+        } while (true);
     }
 
     // platform independent way to tell if a file changed
     // we compare the file modified timestamp, the absolute path (symlinks), and file id on the system
     @FixForMultiProject // what do we do when a file is removed?
-    final boolean fileChanged(Path path) throws IOException {
+    final boolean fileChanged(Path path) throws IOException, InterruptedException {
         FileUpdateState newFileState = readFileUpdateState(path);
         if (newFileState == null) {
             fileUpdateState.remove(path);

server/src/test/java/org/elasticsearch/common/file/AbstractFileWatchingServiceTests.java

@@ -25,6 +25,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.AccessDeniedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
@@ -238,6 +239,23 @@ public void testRegisterWatchKeyRetry() throws IOException, InterruptedException
         verify(service, times(2)).retryDelayMillis(anyInt());
     }
 
+    public void testAccessDeniedRetry() throws IOException, InterruptedException {
+        var service = spy(fileWatchingService);
+        doAnswer(i -> 0L).when(service).retryDelayMillis(anyInt());
+
+        Files.createDirectories(service.watchedFileDir());
+        var mockedPath = spy(service.watchedFileDir());
+
+        doThrow(new AccessDeniedException("can't read state")).doThrow(new AccessDeniedException("can't read state - attempt 2"))
+            .doAnswer(i -> mockedPath)
+            .when(mockedPath)
+            .toRealPath();
+
+        var result = service.readFileUpdateState(mockedPath);
+        assertNotNull(result);
+        verify(service, times(2)).retryDelayMillis(anyInt());
+    }
+
     // helpers
     private static void writeTestFile(Path path, String contents) throws IOException {
         Path tempFilePath = createTempFile();