Merged PR 706846: Add retries when opening a file fails with UnauthorizedAccessException

Add retries when opening a file fails with UnauthorizedAccessException.

The PR also changes the exception type thrown due to sharing violation on linux. Before the code was failing with `IOException` on Linux and `UnauthoziedAccessException` on Windows. Now the behavior is consistent.

Related work items: #2039887

Public/Src/Cache/ContentStore/Interfaces/FileSystem/AbsFileSystemExtension.cs

@@ -6,6 +6,7 @@ using System.Diagnostics.ContractsLight;
 using System.IO;
 using System.Threading.Tasks;
 using BuildXL.Cache.ContentStore.Hashing;
+using BuildXL.Cache.ContentStore.Interfaces.Tracing;
 using BuildXL.Cache.ContentStore.UtilitiesCore;
 
 namespace BuildXL.Cache.ContentStore.Interfaces.FileSystem
@@ -109,6 +110,28 @@ namespace BuildXL.Cache.ContentStore.Interfaces.FileSystem
             return fileSystem.TryOpen(path, fileAccess, fileMode, share, FileOptions.None, DefaultFileStreamBufferSize);
         }
 
+        /// <summary>
+        /// Tries opening a given <paramref name="path"/> and retries if <see cref="UnauthorizedAccessException"/> is happening.
+        /// </summary>
+        public static async Task<StreamWithLength?> TryOpenWithRetriesAsync(this IAbsFileSystem fileSystem, AbsolutePath path, FileAccess fileAccess, FileMode fileMode, FileShare share, int retryCount, TimeSpan retryDelay, Action<UnauthorizedAccessException> onException)
+        {
+            Contract.Requires(retryCount > 0);
+            for (int i = 0; i < retryCount; i++)
+            {
+                try
+                {
+                    return fileSystem.TryOpen(path, fileAccess, fileMode, share, FileOptions.None, DefaultFileStreamBufferSize);
+                }
+                catch (UnauthorizedAccessException e) when (i < retryCount - 1)
+                {
+                    onException(e);
+                    await Task.Delay(retryDelay);
+                }
+            }
+
+            throw Contract.AssertFailure("Not reachable");
+        }
+
         /// <summary>
         /// Open the named file asynchronously for reading.
         /// </summary>

Public/Src/Cache/ContentStore/Interfaces/FileSystem/IAbsFileSystem.cs

@@ -46,6 +46,11 @@ namespace BuildXL.Cache.ContentStore.Interfaces.FileSystem
         /// Unlike System.IO.FileStream, this provides a way to atomically check for the existence of a file and open it.
         /// This method throws the same set of exceptions that <see cref="FileStream"/> constructor does.
         /// </remarks>
+        /// <exception cref="IOException">An I/O error occurred.</exception>
+        /// <exception cref="UnauthorizedAccessException">
+        /// Unlike <see cref="FileStream"/>'s ctor, this method fails with <see cref="UnauthorizedAccessException"/> in case of
+        /// sharing violation and not with <see cref="IOException"/>. Plus it tries to find an active process that owns the handle and add such information into the error's text message.
+        /// </exception>
         StreamWithLength? TryOpen(AbsolutePath path, FileAccess fileAccess, FileMode fileMode, FileShare share, FileOptions options, int bufferSize);
 
         /// <summary>

Public/Src/Cache/ContentStore/Library/FileSystem/PassThroughFileSystem.cs

@@ -549,7 +549,15 @@ namespace BuildXL.Cache.ContentStore.FileSystem
                 return null;
             }
 
-            return new FileStream(path.Path, mode, accessMode, share, bufferSize, options);
+            try
+            {
+                return new FileStream(path.Path, mode, accessMode, share, bufferSize, options);
+            }
+            catch (IOException e) when(e.Message.Contains("The process cannot access the file"))
+            {
+                // Finding open handles is not supported for Unix.
+                throw CreateUnauthorizedAccessException(e.Message, path.ToString(), tryFindOpenHandles: false);
+            }
         }
 
         /// <summary>
@@ -1204,23 +1212,27 @@ namespace BuildXL.Cache.ContentStore.FileSystem
                         throw new DirectoryNotFoundException(message);
                     case ERROR_ACCESS_DENIED:
                     case ERROR_SHARING_VIOLATION:
-
-                        string extraMessage = string.Empty;
-
-                        if (path != null)
-                        {
-                            extraMessage = " " + (FileUtilities.TryFindOpenHandlesToFile(path, out var info, printCurrentFilePath: false)
-                                ? info
-                                : "Attempt to find processes with open handles to the file failed.");
-                        }
-
-                        throw new UnauthorizedAccessException($"{message}.{extraMessage}");
+                        throw CreateUnauthorizedAccessException(message, path, tryFindOpenHandles: true);
                     default:
                         throw new IOException(message, ExceptionUtilities.HResultFromWin32(lastError));
                 }
             }
         }
 
+        private static UnauthorizedAccessException CreateUnauthorizedAccessException(string message, string? path, bool tryFindOpenHandles)
+        {
+            string extraMessage = string.Empty;
+
+            if (path != null && tryFindOpenHandles)
+            {
+                extraMessage = " " + (FileUtilities.TryFindOpenHandlesToFile(path, out var info, printCurrentFilePath: false)
+                    ? info
+                    : "Attempt to find processes with open handles to the file failed.");
+            }
+
+            throw new UnauthorizedAccessException($"{message}.{extraMessage}");
+        }
+
         /// <inheritdoc />
         public DateTime GetDirectoryCreationTimeUtc(AbsolutePath path)
         {

Public/Src/Cache/ContentStore/Library/Stores/ContentStoreSettings.cs

@@ -74,6 +74,16 @@ namespace BuildXL.Cache.ContentStore.Stores
         public bool AssumeCallerCreatesDirectoryForPlace { get; set; } = false;
 
         public bool RemoveAuditRuleInheritance { get; set; } = false;
+
+        /// <summary>
+        /// A number of retries used for opening a file for hashing.
+        /// </summary>
+        public int? RetryCountForFileHashing { get; set; }
+
+        /// <summary>
+        /// A delay between retrying opening a file for hashing.
+        /// </summary>
+        public TimeSpan RetryDelayForFileHashing { get; set; } = TimeSpan.FromMilliseconds(100);
     }
 
     /// <nodoc />

Public/Src/Cache/ContentStore/Library/Stores/FileSystemContentStoreInternal.cs

@@ -270,7 +270,7 @@ namespace BuildXL.Cache.ContentStore.Stores
         public async Task<ContentHashWithSize?> TryHashFileAsync(Context context, AbsolutePath path, HashType hashType, Func<Stream, Stream>? wrapStream = null)
         {
             // We only hash the file if a trusted hash is not supplied
-            using var stream = FileSystem.TryOpen(path, FileAccess.Read, FileMode.Open, FileShare.Read | FileShare.Delete);
+            using var stream = await tryOpenFileAsync();
             if (stream == null)
             {
                 return null;
@@ -284,8 +284,30 @@ namespace BuildXL.Cache.ContentStore.Stores
 
             // Hash the file in  place
             return await HashContentAsync(context, wrappedStream.AssertHasLength(), hashType);
-        }
 
+            Task<StreamWithLength?> tryOpenFileAsync()
+            {
+                if (_settings.RetryCountForFileHashing is { } retryCount)
+                {
+                    return FileSystem.TryOpenWithRetriesAsync(
+                        path,
+                        FileAccess.Read,
+                        FileMode.Open,
+                        FileShare.Read | FileShare.Delete,
+                        retryCount: retryCount,
+                        retryDelay: _settings.RetryDelayForFileHashing,
+                        onException:
+                        ex =>
+                        {
+                            Tracer.Warning(context, ex, $"Transient failure during opening file for hashing. Retrying in '{_settings.RetryDelayForFileHashing}'");
+                        }
+                    );
+                }
+
+                return Task.FromResult(FileSystem.TryOpen(path, FileAccess.Read, FileMode.Open, FileShare.Read | FileShare.Delete));
+            }
+        }
+        
         private void DeleteTempFolder()
         {
             if (FileSystem.DirectoryExists(_tempFolder))

Public/Src/Cache/ContentStore/Test/FileSystem/PassThroughFileSystemTests.cs

@@ -18,7 +18,6 @@ using BuildXL.Cache.ContentStore.Hashing;
 using BuildXL.Cache.ContentStore.Interfaces.Tracing;
 using BuildXL.Cache.ContentStore.Tracing.Internal;
 using BuildXL.Utilities.ParallelAlgorithms;
-using BuildXL.Utilities.Tracing;
 using BuildXL.Utilities.Core.Tracing;
 using Xunit.Abstractions;
 
@@ -32,6 +31,75 @@ namespace ContentStoreTest.FileSystem
         {
         }
 
+        [Fact]
+        public void SharingViolationFailsWithUnauthorizedAccessException()
+        {
+            using (var testDirectory = new DisposableDirectory(FileSystem))
+            {
+                var filePath = testDirectory.Path / "file";
+                FileSystem.WriteAllBytes(filePath, new byte[] {1, 2, 3});
+                using var file = FileSystem.OpenForWrite(filePath, expectingLength: 42, FileMode.Open, FileShare.None);
+
+                Assert.Throws<UnauthorizedAccessException>(
+                    () => FileSystem.OpenForWrite(filePath, expectingLength: 42, FileMode.Open, FileShare.None));
+            }
+        }
+
+        [Fact]
+        public async Task TryOpenWithRetriesAsyncRetriesMultipleTimes()
+        {
+            using (var testDirectory = new DisposableDirectory(FileSystem))
+            {
+                var filePath = testDirectory.Path / "file";
+                FileSystem.WriteAllBytes(filePath, new byte[] { 1, 2, 3 });
+                int callbackCount = 0;
+                const int retryCount = 4;
+                using var file = FileSystem.OpenForWrite(filePath, expectingLength: 42, FileMode.Open, FileShare.None);
+                try
+                {
+                    using var file2 = await FileSystem.TryOpenWithRetriesAsync(
+                        filePath,
+                        FileAccess.Read,
+                        FileMode.Open,
+                        FileShare.None,
+                        retryCount: retryCount,
+                        retryDelay: TimeSpan.FromMilliseconds(10),
+                        onException: _ =>
+                        {
+                            callbackCount++;
+                        });
+                    Assert.True(false, "TryOpenWithRetriesAsync should fail.");
+                }
+                catch (UnauthorizedAccessException)
+                { }
+
+                Assert.Equal(retryCount - 1, callbackCount);
+            }
+        }
+
+        [Fact]
+        public async Task TryOpenWithRetriesAsyncRecovers()
+        {
+            using (var testDirectory = new DisposableDirectory(FileSystem))
+            {
+                var filePath = testDirectory.Path / "file";
+                FileSystem.WriteAllBytes(filePath, new byte[] { 1, 2, 3 });
+                var file = FileSystem.OpenForWrite(filePath, expectingLength: 42, FileMode.Open, FileShare.Write);
+                using var file2 = await FileSystem.TryOpenWithRetriesAsync(
+                    filePath,
+                    FileAccess.Read,
+                    FileMode.Open,
+                    FileShare.None,
+                    retryCount: 4,
+                    retryDelay: TimeSpan.FromMilliseconds(10),
+                    onException: _ =>
+                    {
+                        // Disposing the file should allow us to open it.
+                        file.Dispose();
+                    });
+            }
+        }
+
         [Fact]
         public void OpenFileFromAbsentDirectoryShouldThrowDirectoryNotFoundException()
         {

Public/Src/Cache/DistributedCache.Host/Configuration/DistributedContentSettings.cs

@@ -796,6 +796,12 @@ namespace BuildXL.Cache.Host.Configuration
         [DataMember]
         public bool TraceFileSystemContentStoreDiagnosticMessages { get; set; } = false;
 
+        [DataMember]
+        public int? RetryCountForFileHashing { get; set; }
+
+        [DataMember]
+        public TimeSpan? RetryDelayForFileHashing { get; set; }
+
         [DataMember]
         [Validation.Range(1, int.MaxValue)]
         public int? SilentOperationDurationThreshold { get; set; }

Public/Src/Cache/DistributedCache.Host/Service/Internal/DistributedContentStoreFactory.cs

@@ -448,6 +448,8 @@ namespace BuildXL.Cache.Host.Service.Internal
             ApplyIfNotNull(settings.ReserveSpaceTimeoutInMinutes, v => result.ReserveTimeout = TimeSpan.FromMinutes(v));
 
             ApplyIfNotNull(settings.UseHierarchicalTraceIds, v => Context.UseHierarchicalIds = v);
+            ApplyIfNotNull(settings.RetryCountForFileHashing, v => result.RetryCountForFileHashing = v);
+            ApplyIfNotNull(settings.RetryDelayForFileHashing, v => result.RetryDelayForFileHashing = v);
 
             return result;
         }