From 5ef3f3e11e69a77d3bdcd5941c61b2798f0c0713 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 24 Aug 2026 14:21:58 +0000 Subject: [PATCH] fix: a directory that stops being readable no longer throws at whoever lists it SaferEnumerateFiles returned a lazy sequence, so an I/O error was raised where the sequence was walked rather than where it was created - past the try/catch callers had wrapped around it. IgnoreInaccessible did not help either: it only forgives permissions, not a volume that has stopped answering. Walk the enumerator defensively instead, keeping what was read and reporting the reason, and let a caller ask whether a directory can be read at all. See issue #1984. Co-authored-by: rmcrackan --- Source/AppScaffolding/LibationScaffolding.cs | 11 +- Source/FileManager/BackgroundFileSystem.cs | 36 ++- Source/FileManager/FileUtility.cs | 100 ++++++- .../BackgroundFileSystemTests.cs | 77 ++++++ .../SaferEnumerateFilesTests.cs | 254 ++++++++++++++++++ 5 files changed, 464 insertions(+), 14 deletions(-) create mode 100644 Source/_Tests/FileManager.Tests/SaferEnumerateFilesTests.cs diff --git a/Source/AppScaffolding/LibationScaffolding.cs b/Source/AppScaffolding/LibationScaffolding.cs index dcd749f6..9c953ca0 100644 --- a/Source/AppScaffolding/LibationScaffolding.cs +++ b/Source/AppScaffolding/LibationScaffolding.cs @@ -289,11 +289,20 @@ public static class LibationScaffolding // begin logging session with a form feed Log.Logger.Information("\r\n\f"); + // -1 means the count could not be taken. Listing a directory no longer throws when it stops being + // readable partway through, so an incomplete walk has to be asked about rather than caught: a count + // that silently means "as many as I managed to read" is worse than no count in a bug report. static int fileCount(FileManager.LongPath? longPath) { if (longPath is null) return -1; - try { return FileManager.FileUtility.SaferEnumerateFiles(longPath).Count(); } + + var complete = true; + try + { + var count = FileManager.FileUtility.SaferEnumerateFiles(longPath, onIncomplete: _ => complete = false).Count(); + return complete ? count : -1; + } catch { return -1; } } diff --git a/Source/FileManager/BackgroundFileSystem.cs b/Source/FileManager/BackgroundFileSystem.cs index b1518d93..f66a5ce5 100644 --- a/Source/FileManager/BackgroundFileSystem.cs +++ b/Source/FileManager/BackgroundFileSystem.cs @@ -69,19 +69,33 @@ public class BackgroundFileSystem : IDisposable fsCache.AddRange(SafestEnumerateFiles(RootDirectory)); } - directoryChangesEvents = new BlockingCollection(); - fileSystemWatcher = new FileSystemWatcher(RootDirectory) + try { - IncludeSubdirectories = true, - EnableRaisingEvents = true - }; - fileSystemWatcher.Created += FileSystemWatcher_Changed; - fileSystemWatcher.Deleted += FileSystemWatcher_Changed; - fileSystemWatcher.Renamed += FileSystemWatcher_Changed; - fileSystemWatcher.Error += FileSystemWatcher_Error; + directoryChangesEvents = new BlockingCollection(); + fileSystemWatcher = new FileSystemWatcher(RootDirectory) + { + IncludeSubdirectories = true, + EnableRaisingEvents = true + }; + fileSystemWatcher.Created += FileSystemWatcher_Changed; + fileSystemWatcher.Deleted += FileSystemWatcher_Changed; + fileSystemWatcher.Renamed += FileSystemWatcher_Changed; + fileSystemWatcher.Error += FileSystemWatcher_Error; - backgroundScanner = new Task(BackgroundScanner); - backgroundScanner.Start(); + backgroundScanner = new Task(BackgroundScanner); + backgroundScanner.Start(); + } + // Watching a directory fails for the same reasons reading one does, and a removable drive can be pulled + // between the listing above and this. Constructing this type happens inside the static initializer that + // builds the Books file cache, and the runtime caches a failed initializer for the life of the process: + // one throw here would be rethrown at every later reader of the Books directory. Give up the live + // updates instead. Dropping RootDirectory has the owner rebuild this once the directory works again. + catch (Exception ex) + { + Serilog.Log.Logger.Error(ex, "Could not watch a directory for changes, so its file cache was abandoned: {@DebugText}", new { path = (string?)RootDirectory }); + Stop(); + RootDirectory = null; + } } private void Stop() { diff --git a/Source/FileManager/FileUtility.cs b/Source/FileManager/FileUtility.cs index 7575ad7a..6972b27b 100644 --- a/Source/FileManager/FileUtility.cs +++ b/Source/FileManager/FileUtility.cs @@ -263,8 +263,10 @@ public static class FileUtility /// Starting directory /// Filename pattern match /// Search subdirectories or only top level directory for files + /// Called with the reason when the walk ends early, so a caller that must not + /// mistake a truncated list for an empty directory can tell the difference. /// List of files - public static IEnumerable SaferEnumerateFiles(LongPath path, string searchPattern = "*", SearchOption searchOption = SearchOption.TopDirectoryOnly) + public static IEnumerable SaferEnumerateFiles(LongPath path, string searchPattern = "*", SearchOption searchOption = SearchOption.TopDirectoryOnly, Action? onIncomplete = null) { var enumOptions = new EnumerationOptions { @@ -273,7 +275,101 @@ public static class FileUtility ReturnSpecialDirectories = false, MatchType = MatchType.Simple }; - return Directory.EnumerateFiles(path.Path, searchPattern, enumOptions).Select(p => (LongPath)p); + return IterateSafely( + () => Directory.EnumerateFiles(path.Path, searchPattern, enumOptions).Select(p => (LongPath)p), + path, + onIncomplete); + } + + /// + /// Walks a file system sequence so that a directory which stops being readable partway through ends the walk + /// instead of throwing at whoever is consuming it. + /// + /// only forgives permissions. A disconnected or failing + /// volume raises an I/O error from the enumerator itself, and because enumeration is lazy that error is + /// raised wherever the sequence is finally walked - which is past any try/catch the caller wrapped around + /// the call that produced it. Libation lost a whole session that way: a Books folder on a USB drive that + /// started returning I/O errors took down the file cache, the type initializer that builds it, and with it + /// every subsequent launch. See issue #1984. + /// + /// + internal static IEnumerable IterateSafely(Func> getSequence, LongPath path, Action? onIncomplete = null) + { + IEnumerator enumerator; + try + { + //Opening the directory is itself a read, and fails the same way. + enumerator = getSequence().GetEnumerator(); + } + catch (Exception ex) when (IsUnreadable(ex)) + { + ReportIncomplete(ex, path, onIncomplete); + yield break; + } + + try + { + while (true) + { + LongPath current; + try + { + if (!enumerator.MoveNext()) + break; + current = enumerator.Current; + } + catch (Exception ex) when (IsUnreadable(ex)) + { + //Whatever has already been read is still good and still worth returning. + ReportIncomplete(ex, path, onIncomplete); + break; + } + + yield return current; + } + } + finally + { + enumerator.Dispose(); + } + } + + private static bool IsUnreadable(Exception ex) + => ex is IOException or UnauthorizedAccessException or System.Security.SecurityException; + + private static void ReportIncomplete(Exception ex, LongPath path, Action? onIncomplete) + { + try + { + //A directory that has simply gone is routine: temp folders are created and cleaned up under a scan + //all the time. A directory that is there and cannot be read is worth seeing in a bug report. + if (ex is DirectoryNotFoundException) + Serilog.Log.Logger.Debug(ex, "Stopped listing files in a directory that is no longer there: {@DebugText}", new { path = (string)path }); + else + Serilog.Log.Logger.Warning(ex, "Could not finish listing files. The results are incomplete: {@DebugText}", new { path = (string)path }); + } + catch { /* logging must not be the thing that breaks a file listing */ } + + onIncomplete?.Invoke(ex); + } + + /// + /// Whether a directory can actually be read, as opposed to merely existing. A removable drive that has been + /// pulled, and a failing one, can both still answer that they are a directory while every read of them fails. + /// + public static bool CanEnumerate(LongPath path) + { + var readable = true; + + //The first entry is enough. A volume that cannot be read fails on the first attempt, and this must not + //pay for walking a whole library to answer the question. + foreach (var _ in IterateSafely( + () => Directory.EnumerateFileSystemEntries(path.Path).Select(p => (LongPath)p), + path, + _ => readable = false)) + break; + + return readable; } /// diff --git a/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs b/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs index 178b2cf1..a8bf5f8f 100644 --- a/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs +++ b/Source/_Tests/FileManager.Tests/BackgroundFileSystemTests.cs @@ -2,6 +2,7 @@ using FileManager; using Microsoft.VisualStudio.TestTools.UnitTesting; using System; using System.IO; +using System.Runtime.Versioning; using System.Text.RegularExpressions; using System.Threading; @@ -104,6 +105,82 @@ public class DisposeWhileEventsAreArriving } } +/// +/// From issue #1984, where a Books folder on a failing USB drive closed Libation on every launch. This type is +/// constructed from the static initializer of AudibleFileStorage, and the runtime caches a failed static +/// initializer for the life of the process: anything that escapes here is rethrown at every later caller that +/// so much as asks where the Books folder is, including the startup logging that runs before the window opens. +/// So construction has to survive a root directory it cannot read, however it cannot read it. +/// +[TestClass] +[DoNotParallelize] +public class WhenTheRootDirectoryCannotBeRead +{ + private string tempDir = string.Empty; + + [TestInitialize] + public void Initialize() + { + tempDir = Path.Combine(Path.GetTempPath(), $"libation-bfs-unreadable-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + } + + [TestCleanup] + public void Cleanup() + { + try + { + if (!OperatingSystem.IsWindows() && Directory.Exists(tempDir)) + File.SetUnixFileMode(tempDir, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + Directory.Delete(tempDir, recursive: true); + } + catch (IOException) + { + // A leftover temp directory is not worth failing a test over. + } + } + + [TestMethod] + public void a_root_that_is_not_there_is_reported_rather_than_thrown() + { + using var sut = new BackgroundFileSystem(Path.Combine(tempDir, "no-such-folder"), "*.*", SearchOption.AllDirectories); + + Assert.IsNull(sut.RootDirectory, "a root that cannot be used is dropped, so the owner rebuilds this when it can"); + Assert.IsNull(sut.FindFile(new Regex(".*"))); + } + + [TestMethod] + public void a_root_that_refuses_to_be_read_costs_the_cache_and_nothing_else() + { + // Assert.Inconclusive is not [DoesNotReturn], so return explicitly or the body below still + // looks reachable on Windows to the platform compatibility analyzer + if (!OperatingSystem.IsLinux() && !OperatingSystem.IsMacOS()) + { + Assert.Inconclusive("Skipped because revoking directory read permission needs unix file modes."); + return; + } + if (Environment.IsPrivilegedProcess) + { + Assert.Inconclusive("Skipped because root may read a directory with no permissions, so there is nothing to refuse."); + return; + } + + aRootThatRefusesToBeRead(tempDir); + } + + [SupportedOSPlatform("linux")] + [SupportedOSPlatform("macos")] + private static void aRootThatRefusesToBeRead(string directory) + { + File.WriteAllText(Path.Combine(directory, "book.m4b"), "audio"); + File.SetUnixFileMode(directory, UnixFileMode.None); + + using var sut = new BackgroundFileSystem(directory, "*.*", SearchOption.AllDirectories); + + Assert.IsNull(sut.FindFile(new Regex(@"book\.m4b$")), "nothing can be read, so nothing is found"); + } +} + /// /// 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 diff --git a/Source/_Tests/FileManager.Tests/SaferEnumerateFilesTests.cs b/Source/_Tests/FileManager.Tests/SaferEnumerateFilesTests.cs new file mode 100644 index 00000000..ce4ed6c1 --- /dev/null +++ b/Source/_Tests/FileManager.Tests/SaferEnumerateFilesTests.cs @@ -0,0 +1,254 @@ +using FileManager; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.Versioning; + +namespace SaferEnumerateFilesTests; + +/// +/// From issue #1984: a Books folder on a USB drive started returning I/O errors partway through a library scan, +/// and Libation closed with "Libation encountered a fatal error". It then closed the same way on every launch +/// afterwards, before the window appeared. +/// +/// Two guards were supposed to prevent that and neither did. +/// only forgives permissions, so an I/O error still came out of the enumerator. And the try/catch the caller had +/// wrapped around the call never saw it: listing files is lazy, so the error was raised where the sequence was +/// walked - inside the caller's AddRange, long after the try/catch had exited. +/// +/// +[TestClass] +public class WhenADirectoryStopsBeingReadablePartwayThrough +{ + private static IEnumerable TwoFilesThenFails(Exception failure) + { + yield return (LongPath)"/books/first.m4b"; + yield return (LongPath)"/books/second.m4b"; + throw failure; + } + + /// The shape of the crash: the walk is consumed by an AddRange, exactly as the file cache does. + [TestMethod] + public void the_walk_ends_early_instead_of_throwing_at_whoever_is_consuming_it() + { + var found = new List(); + + found.AddRange(FileUtility.IterateSafely( + () => TwoFilesThenFails(new IOException("Input/output error : '/Volumes/NO NAME/Audible audiobooks'")), + (LongPath)"/Volumes/NO NAME/Audible audiobooks")); + + CollectionAssert.AreEqual( + new[] { "/books/first.m4b", "/books/second.m4b" }, + found.Select(f => (string)f).ToArray(), + "everything read before the failure is still good, and still worth returning"); + } + + [TestMethod] + public void the_caller_can_tell_a_truncated_walk_from_an_empty_directory() + { + Exception? reported = null; + var failure = new IOException("Input/output error"); + + FileUtility.IterateSafely(() => TwoFilesThenFails(failure), (LongPath)"/books", ex => reported = ex).ToList(); + + Assert.AreSame(failure, reported); + } + + [TestMethod] + public void nothing_is_reported_when_the_whole_directory_was_read() + { + var reported = false; + + FileUtility.IterateSafely(() => new[] { (LongPath)"/books/only.m4b" }, (LongPath)"/books", _ => reported = true).ToList(); + + Assert.IsFalse(reported); + } + + /// Opening the directory is itself a read, and fails the same way. + [TestMethod] + public void a_failure_before_the_first_entry_lists_nothing_rather_than_throwing() + { + Exception? reported = null; + + var found = FileUtility.IterateSafely( + () => throw new IOException("Input/output error"), + (LongPath)"/books", + ex => reported = ex).ToList(); + + Assert.AreEqual(0, found.Count); + Assert.IsInstanceOfType(reported); + } + + [TestMethod] + public void a_directory_that_has_gone_is_forgiven_too() + { + var found = FileUtility.IterateSafely( + () => TwoFilesThenFails(new DirectoryNotFoundException()), + (LongPath)"/books").ToList(); + + Assert.AreEqual(2, found.Count); + } + + [TestMethod] + public void so_is_a_directory_the_user_is_not_allowed_to_read() + { + var found = FileUtility.IterateSafely( + () => TwoFilesThenFails(new UnauthorizedAccessException()), + (LongPath)"/books").ToList(); + + Assert.AreEqual(2, found.Count); + } + + /// + /// The guard is for a file system that will not answer, not for bugs. Widening it to everything would hide + /// the next defect in here behind a short list of files. + /// + [TestMethod] + public void a_failure_that_is_not_the_file_system_is_still_raised() + { + var found = FileUtility.IterateSafely( + () => TwoFilesThenFails(new InvalidOperationException("a real bug")), + (LongPath)"/books"); + + Assert.ThrowsExactly(() => found.ToList()); + } +} + +[TestClass] +public class SaferEnumerateFilesAgainstARealDirectory +{ + private string tempDir = string.Empty; + + [TestInitialize] + public void Initialize() + { + tempDir = Path.Combine(Path.GetTempPath(), $"libation-enumerate-tests-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + } + + [TestCleanup] + public void Cleanup() + { + try + { + if (!OperatingSystem.IsWindows() && Directory.Exists(tempDir)) + File.SetUnixFileMode(tempDir, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + Directory.Delete(tempDir, recursive: true); + } + catch (IOException) + { + // A leftover temp directory is not worth failing a test over. + } + } + + [TestMethod] + public void the_files_that_are_there_are_listed() + { + File.WriteAllText(Path.Combine(tempDir, "book.m4b"), "audio"); + Directory.CreateDirectory(Path.Combine(tempDir, "nested")); + File.WriteAllText(Path.Combine(tempDir, "nested", "another.m4b"), "audio"); + + var topOnly = FileUtility.SaferEnumerateFiles(tempDir).Select(f => Path.GetFileName((string)f)).ToArray(); + var everything = FileUtility.SaferEnumerateFiles(tempDir, "*", SearchOption.AllDirectories).Select(f => Path.GetFileName((string)f)).Order().ToArray(); + + CollectionAssert.AreEqual(new[] { "book.m4b" }, topOnly); + CollectionAssert.AreEqual(new[] { "another.m4b", "book.m4b" }, everything); + } + + /// + /// This used to throw, and lazily, so it landed on whoever walked the sequence rather than whoever asked for + /// it. A Books folder that is not there is a settings problem to report, never a crash. + /// + [TestMethod] + public void a_directory_that_is_not_there_lists_nothing_and_says_why() + { + Exception? reported = null; + var missing = Path.Combine(tempDir, "no-such-folder"); + + var found = FileUtility.SaferEnumerateFiles(missing, onIncomplete: ex => reported = ex).ToList(); + + Assert.AreEqual(0, found.Count); + Assert.IsInstanceOfType(reported); + } +} + +[TestClass] +public class CanEnumerate +{ + private string tempDir = string.Empty; + + [TestInitialize] + public void Initialize() + { + tempDir = Path.Combine(Path.GetTempPath(), $"libation-can-enumerate-tests-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + } + + [TestCleanup] + public void Cleanup() + { + try + { + if (!OperatingSystem.IsWindows() && Directory.Exists(tempDir)) + File.SetUnixFileMode(tempDir, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + Directory.Delete(tempDir, recursive: true); + } + catch (IOException) + { + // A leftover temp directory is not worth failing a test over. + } + } + + [TestMethod] + public void a_readable_directory_can_be_read() + { + Assert.IsTrue(FileUtility.CanEnumerate(tempDir), "an empty directory is readable, it is just empty"); + + File.WriteAllText(Path.Combine(tempDir, "book.m4b"), "audio"); + + Assert.IsTrue(FileUtility.CanEnumerate(tempDir)); + } + + [TestMethod] + public void a_directory_that_is_not_there_cannot_be_read() + => Assert.IsFalse(FileUtility.CanEnumerate(Path.Combine(tempDir, "no-such-folder"))); + + /// + /// Existing is not the same as usable, which is the whole point of asking. A pulled or failing drive still + /// answers that it is a directory, and a recursive listing of it comes back empty rather than refusing - + /// IgnoreInaccessible sees to that - so without this check Libation reports a full library as downloading + /// nothing instead of saying the drive is unreadable. + /// + [TestMethod] + public void a_directory_that_exists_but_refuses_to_be_read_cannot_be_read() + { + // Assert.Inconclusive is not [DoesNotReturn], so return explicitly or the body below still + // looks reachable on Windows to the platform compatibility analyzer + if (!OperatingSystem.IsLinux() && !OperatingSystem.IsMacOS()) + { + Assert.Inconclusive("Skipped because revoking directory read permission needs unix file modes."); + return; + } + if (Environment.IsPrivilegedProcess) + { + Assert.Inconclusive("Skipped because root may read a directory with no permissions, so there is nothing to refuse."); + return; + } + + aDirectoryThatExistsButRefusesToBeRead(tempDir); + } + + [SupportedOSPlatform("linux")] + [SupportedOSPlatform("macos")] + private static void aDirectoryThatExistsButRefusesToBeRead(string directory) + { + File.WriteAllText(Path.Combine(directory, "book.m4b"), "audio"); + File.SetUnixFileMode(directory, UnixFileMode.None); + + Assert.IsTrue(Directory.Exists(directory), "the directory is still there; it just cannot be read"); + Assert.AreEqual(0, FileUtility.SaferEnumerateFiles(directory, "*", SearchOption.AllDirectories).Count(), "a listing comes back empty rather than refusing"); + Assert.IsFalse(FileUtility.CanEnumerate(directory)); + } +}