diff --git a/Source/FileManager/BackgroundFileSystem.cs b/Source/FileManager/BackgroundFileSystem.cs index 4a319084..b1518d93 100644 --- a/Source/FileManager/BackgroundFileSystem.cs +++ b/Source/FileManager/BackgroundFileSystem.cs @@ -107,7 +107,17 @@ private void Stop() catch (ObjectDisposedException) { } //Wait for background scanner to terminate before reinitializing. - backgroundScanner?.Wait(); + try + { + backgroundScanner?.Wait(); + } + // A scanner that died still has to be waited on, but its failure is not this caller's to raise: Stop() is + // reached from Refresh() and from Dispose(), neither of which has anything to do with whatever went wrong, + // and both of which are about to replace the scanner anyway. + catch (AggregateException ex) + { + Serilog.Log.Logger.Error(ex, "The file cache's background scanner had already stopped on an error."); + } //Dispose of directoryChangesEvents after backgroundScanner exists. Clear the field first so a late event //has nothing to add to. CompleteAdding has to have happened while it was still set, or the scanner would @@ -146,8 +156,19 @@ private void BackgroundScanner() { while (directoryChangesEvents?.TryTake(out var change, -1) is true) { - lock (fsCacheLocker) - UpdateLocalCache(change); + try + { + lock (fsCacheLocker) + UpdateLocalCache(change); + } + // One path this cannot read must not end the scan. An exception here used to be terminal twice over: + // it stopped the cache tracking anything further, and it was stored on the task, so the next Stop() + // rethrew it as an AggregateException at whoever had called Refresh() - a caller with nothing to do + // with the file that went missing. + catch (Exception ex) + { + Serilog.Log.Logger.Debug(ex, "Could not apply a file system change to the file cache: {@DebugText}", new { change.ChangeType, change.FullPath }); + } } } @@ -181,14 +202,36 @@ private void AddPath(LongPath path) { path = path.LongPathName; //Temporary files created when updating the db will disappear before their attributes can be read. - if (Path.GetFileName(path).Contains("LibationContext.db") || !File.Exists(path) && !Directory.Exists(path)) + if (Path.GetFileName(path).Contains("LibationContext.db")) + return; + + // Whether it exists and what it is were two questions, and the answer to the first could stop being true + // before the second was asked: a download's temp file, or a folder the user has just moved or deleted, is + // gone by the time its attributes are read. One question instead, and its failure means the same thing + // the existence check meant - there is nothing here to add. + if (TryGetAttributes(path) is not FileAttributes attributes) return; - if (File.GetAttributes(path).HasFlag(FileAttributes.Directory)) + + if (attributes.HasFlag(FileAttributes.Directory)) AddUniqueFiles(SafestEnumerateFiles(path)); else AddUniqueFile(path); } + /// What the path is, or null when it cannot be read and so has nothing to add. + internal static FileAttributes? TryGetAttributes(LongPath path) + { + try + { + return File.GetAttributes(path); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or ArgumentException or NotSupportedException) + { + Serilog.Log.Logger.Debug(ex, "Nothing to add to the file cache for {@DebugText}", new { path = (string)path }); + return null; + } + } + private IEnumerable SafestEnumerateFiles(string path) { try diff --git a/Source/FileManager/_InternalsVisible.cs b/Source/FileManager/_InternalsVisible.cs new file mode 100644 index 00000000..47db7673 --- /dev/null +++ b/Source/FileManager/_InternalsVisible.cs @@ -0,0 +1 @@ +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo(nameof(FileManager) + ".Tests")] diff --git a/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs b/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs index 4565db85..178b2cf1 100644 --- a/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs +++ b/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs @@ -103,3 +103,132 @@ private static bool WaitFor(Func condition, int timeoutMs = 5000) return false; } } + +/// +/// A second failure mode in the same class, from a CI run where all three Windows legs failed and the other six +/// passed: every test in FileLiberator.Tests' PDF path suite failed in TestInitialize with an AggregateException +/// wrapping FileNotFoundException: Could not find file ..., naming a path none of those tests had +/// anything to do with. The watcher had raised Created for a folder an earlier test's cleanup then deleted; the +/// scanner asked whether it existed, was told yes, asked what it was, and got an exception. That killed the +/// scanner and was stored on its task, and the next Stop() - reached from AudibleFileStorage.Audio.Refresh() by +/// way of Dispose() - rethrew it at that caller. +/// +/// The trigger is a disagreement between Exists and GetAttributes over a long \\?\ path, which cannot be +/// staged on the platform these tests usually run on. So the guard itself is asserted directly, and the rest +/// covers what the death cost: a cache that stops tracking, and an exception handed to an unrelated caller. +/// +/// +[TestClass] +[DoNotParallelize] +public class PathsThatVanishBeforeTheyAreRead +{ + private string tempDir = string.Empty; + + [TestInitialize] + public void Initialize() + { + tempDir = Path.Combine(Path.GetTempPath(), $"libation-bfs-vanishing-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + } + + [TestCleanup] + public void Cleanup() + { + try + { + Directory.Delete(tempDir, recursive: true); + } + catch (IOException) + { + // A leftover temp directory is not worth failing a test over. + } + } + + /// Creates a batch of files and deletes the tree while their events are still queued. + private void ChurnVanishingFiles() + { + for (var batch = 0; batch < 5; batch++) + { + var directory = Path.Combine(tempDir, $"vanishing-{batch}"); + Directory.CreateDirectory(directory); + + for (var i = 0; i < 40; i++) + File.WriteAllText(Path.Combine(directory, $"book-{i}.m4b"), "audio"); + + // No wait: the scanner should reach these paths after they have gone. + Directory.Delete(directory, recursive: true); + } + } + + [TestMethod] + public void a_path_that_is_not_there_reads_as_nothing_rather_than_throwing() + { + // The fix, on its own terms. Asking what a path is used to be allowed to throw, on the strength of having + // asked a moment earlier whether it was there. + Assert.IsNull(BackgroundFileSystem.TryGetAttributes(Path.Combine(tempDir, "never-existed.m4b"))); + Assert.IsNull(BackgroundFileSystem.TryGetAttributes(Path.Combine(tempDir, "no", "such", "folder", "book.m4b"))); + } + + [TestMethod] + public void a_path_that_is_there_still_reads_as_what_it_is() + { + var file = Path.Combine(tempDir, "real.m4b"); + File.WriteAllText(file, "audio"); + + Assert.IsFalse(BackgroundFileSystem.TryGetAttributes(file)!.Value.HasFlag(FileAttributes.Directory)); + Assert.IsTrue(BackgroundFileSystem.TryGetAttributes(tempDir)!.Value.HasFlag(FileAttributes.Directory)); + } + + [TestMethod] + public void the_scanner_keeps_going_after_a_path_it_cannot_read() + { + using var sut = new BackgroundFileSystem(tempDir, "*.*", SearchOption.AllDirectories); + + ChurnVanishingFiles(); + + // The scanner is still doing its job, which is what dying used to cost: the first unreadable path ended + // the loop, and every later change to the Books directory went unnoticed for the rest of the session. + File.WriteAllText(Path.Combine(tempDir, "survivor.m4b"), "audio"); + + var found = WaitFor(() => sut.FindFile(new Regex(@"survivor\.m4b$")) is not null); + Assert.IsTrue(found, "the scanner stopped tracking changes after a path it could not read"); + } + + [TestMethod] + public void disposing_afterwards_does_not_hand_the_failure_to_the_caller() + { + // This is the CI stack: Refresh() found the Books directory changed, disposed the old file system, and + // Stop() waited on a scanner that had already faulted. + var sut = new BackgroundFileSystem(tempDir, "*.*", SearchOption.AllDirectories); + + ChurnVanishingFiles(); + + sut.Dispose(); + } + + [TestMethod] + public void a_file_that_outlives_its_event_is_still_tracked() + { + // The guard must not have been widened into ignoring everything: what is really there still lands. + using var sut = new BackgroundFileSystem(tempDir, "*.*", SearchOption.AllDirectories); + + var directory = Path.Combine(tempDir, "kept"); + Directory.CreateDirectory(directory); + File.WriteAllText(Path.Combine(directory, "kept.m4b"), "audio"); + + var found = WaitFor(() => sut.FindFile(new Regex(@"kept\.m4b$")) is not null); + Assert.IsTrue(found, "a file that was never deleted did not reach the cache"); + } + + private static bool WaitFor(Func condition, int timeoutMs = 5000) + { + for (var waited = 0; waited < timeoutMs; waited += 50) + { + if (condition()) + return true; + Thread.Sleep(50); + } + + return false; + } +}