Invalidate the singleton DirectoryService cache on filesystem changes

8 files changed, 25 insertions, 119 deletions

diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs
index e6a33d9dd3..d5735aed27 100644
--- a/Emby.Server.Implementations/IO/LibraryMonitor.cs
+++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs
@@ -368,8 +368,8 @@ namespace Emby.Server.Implementations.IO
                 return;
             }
 
-            // Invalidate before the checks below: a change we deliberately do not refresh for still
-            // has to be read correctly the next time somebody looks at that folder.
+            // The injected service is a singleton, so drop the path before the checks below:
+            // a change we deliberately do not refresh for still has to read correctly later.
             _directoryService.Invalidate(path);
 
             // Ignore certain files, If the parent of an ignored path has a change event, ignore that too
diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs
index 0ced650587..80e89b4305 100644
--- a/Emby.Server.Implementations/Library/LibraryManager.cs
+++ b/Emby.Server.Implementations/Library/LibraryManager.cs
@@ -3723,8 +3723,7 @@ namespace Emby.Server.Implementations.Library
                     }
                 }
 
-                // The validation below would otherwise resolve the libraries root from a listing
-                // taken before this folder was created.
+                // The injected service is a singleton, so its listing predates this folder.
                 _directoryService.Invalidate(virtualFolderPath);
             }
             finally
diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs
index 83684f1b91..e4833c77dd 100644
--- a/Jellyfin.Api/Controllers/LibraryStructureController.cs
+++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs
@@ -189,8 +189,7 @@ public class LibraryStructureController : BaseJellyfinApiController
 
             Directory.Move(currentPath, newPath);
 
-            // The validation below would otherwise resolve the libraries root from a listing taken
-            // before the folder was moved.
+            // The injected service is a singleton, so its listings of both paths are now stale.
             _directoryService.Invalidate(currentPath);
             _directoryService.Invalidate(newPath);
         }
diff --git a/MediaBrowser.Controller/MediaBrowser.Controller.csproj b/MediaBrowser.Controller/MediaBrowser.Controller.csproj
index 06188ad511..73cdf18e91 100644
--- a/MediaBrowser.Controller/MediaBrowser.Controller.csproj
+++ b/MediaBrowser.Controller/MediaBrowser.Controller.csproj
@@ -18,7 +18,6 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="BitFaster.Caching" />
     <PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
   </ItemGroup>
 
diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs
index 613433391b..684e247f86 100644
--- a/MediaBrowser.Controller/Providers/DirectoryService.cs
+++ b/MediaBrowser.Controller/Providers/DirectoryService.cs
@@ -1,31 +1,33 @@
 #pragma warning disable CS1591
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
-using System.Runtime.CompilerServices;
-using BitFaster.Caching.Lru;
 using MediaBrowser.Model.IO;
 
 namespace MediaBrowser.Controller.Providers
 {
     public class DirectoryService : IDirectoryService
     {
-        private static readonly ConditionalWeakTable<IFileSystem, DirectoryCache> _caches = [];
+        // TODO make static and switch to FastConcurrentLru.
+        private readonly ConcurrentDictionary<string, FileSystemMetadata[]> _cache = new(StringComparer.Ordinal);
+
+        private readonly ConcurrentDictionary<string, FileSystemMetadata> _fileCache = new(StringComparer.Ordinal);
+
+        private readonly ConcurrentDictionary<string, List<string>> _filePathCache = new(StringComparer.Ordinal);
 
         private readonly IFileSystem _fileSystem;
-        private readonly DirectoryCache _cache;
 
         public DirectoryService(IFileSystem fileSystem)
         {
             _fileSystem = fileSystem;
-            _cache = _caches.GetValue(fileSystem, static _ => new DirectoryCache());
         }
 
         public FileSystemMetadata[] GetFileSystemEntries(string path)
         {
-            return _cache.Entries.GetOrAdd(
+            return _cache.GetOrAdd(
                 path,
                 static (p, fileSystem) =>
                 {
@@ -87,15 +89,13 @@ namespace MediaBrowser.Controller.Providers
 
         public FileSystemMetadata? GetFileSystemEntry(string path)
         {
-            if (!_cache.Files.TryGet(path, out var result))
+            if (!_fileCache.TryGetValue(path, out var result))
             {
                 var file = _fileSystem.GetFileSystemInfo(path);
-
-                // Only cache hits: a missing file can turn up later.
                 if (file?.Exists ?? false)
                 {
                     result = file;
-                    _cache.Files.AddOrUpdate(path, result);
+                    _fileCache.TryAdd(path, result);
                 }
             }
 
@@ -109,11 +109,10 @@ namespace MediaBrowser.Controller.Providers
         {
             if (clearCache)
             {
-                // Not Invalidate(), which would also drop the parent listing for no reason here.
-                Forget(path);
+                _filePathCache.TryRemove(path, out _);
             }
 
-            return _cache.FilePaths.GetOrAdd(
+            var filePaths = _filePathCache.GetOrAdd(
                 path,
                 static (p, fileSystem) =>
                 {
@@ -127,6 +126,8 @@ namespace MediaBrowser.Controller.Providers
                     }
                 },
                 _fileSystem);
+
+            return filePaths;
         }
 
         public void Invalidate(string path)
@@ -147,28 +148,9 @@ namespace MediaBrowser.Controller.Providers
 
         private void Forget(string path)
         {
-            _cache.Entries.TryRemove(path, out _);
-            _cache.Files.TryRemove(path, out _);
-            _cache.FilePaths.TryRemove(path, out _);
-        }
-
-        private sealed class DirectoryCache
-        {
-            private const int DirectoryCacheSize = 2048;
-            private const int FileCacheSize = 8192;
-
-            // The cache outlives the DirectoryService instances reading it, so entries need their
-            // own staleness bound. A long refresh can outlive it and re-read a directory partway.
-            private static readonly TimeSpan _entryLifetime = TimeSpan.FromMinutes(1);
-
-            public ConcurrentTLru<string, FileSystemMetadata[]> Entries { get; }
-                = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime);
-
-            public ConcurrentTLru<string, FileSystemMetadata> Files { get; }
-                = new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal, _entryLifetime);
-
-            public ConcurrentTLru<string, List<string>> FilePaths { get; }
-                = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime);
+            _cache.TryRemove(path, out _);
+            _fileCache.TryRemove(path, out _);
+            _filePathCache.TryRemove(path, out _);
         }
     }
 }
diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs
index 57d98af1da..dfa7bfde2f 100644
--- a/MediaBrowser.Providers/Lyric/LyricManager.cs
+++ b/MediaBrowser.Providers/Lyric/LyricManager.cs
@@ -255,7 +255,7 @@ public class LyricManager : ILyricManager
                 _libraryMonitor.ReportFileSystemChangeComplete(path, false);
             }
 
-            // The refresh below would otherwise find the deleted file in a cached listing.
+            // The injected service is a singleton, so its listing would keep the deleted file.
             _directoryService.Invalidate(path);
         }
 
@@ -453,7 +453,7 @@ public class LyricManager : ILyricManager
                     await stream.CopyToAsync(fs).ConfigureAwait(false);
                 }
 
-                // The refresh that follows would otherwise not see the new file.
+                // The injected service is a singleton, so its listing of the folder is now stale.
                 _directoryService.Invalidate(savePath);
 
                 return;
diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
index 37d261a95e..aa363c425f 100644
--- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
+++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
@@ -284,7 +284,7 @@ namespace MediaBrowser.Providers.Subtitles
                             await stream.CopyToAsync(fs).ConfigureAwait(false);
                         }
 
-                        // The refresh that follows would otherwise not see the new file.
+                        // The injected service is a singleton, so its listing of the folder is now stale.
                         _directoryService.Invalidate(path);
 
                         return;
@@ -401,7 +401,7 @@ namespace MediaBrowser.Providers.Subtitles
                 _monitor.ReportFileSystemChangeComplete(path, false);
             }
 
-            // The refresh below would otherwise find the deleted file in a cached listing.
+            // The injected service is a singleton, so its listing would keep the deleted file.
             _directoryService.Invalidate(path);
 
             return item.RefreshMetadata(CancellationToken.None);
diff --git a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
index 14ef604d34..7c275b78cc 100644
--- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
+++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
@@ -1,4 +1,3 @@
-using System.Globalization;
 using System.IO;
 using System.Linq;
 using MediaBrowser.Controller.Providers;
@@ -269,62 +268,6 @@ namespace Jellyfin.Controller.Tests
         }
 
         [Fact]
-        public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing()
-        {
-            // Eviction of the first path shows up as the file system being read for it twice.
-            const int PathCount = 8192;
-
-            var fileSystemMock = new Mock<IFileSystem>();
-            fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>()))
-                .Returns(_lowerCaseFileSystemMetadata);
-
-            var directoryService = new DirectoryService(fileSystemMock.Object);
-
-            const string FirstPath = "/music/artist0";
-            directoryService.GetFileSystemEntries(FirstPath);
-
-            for (var i = 1; i < PathCount; i++)
-            {
-                directoryService.GetFileSystemEntries("/music/artist" + i.ToString(CultureInfo.InvariantCulture));
-            }
-
-            directoryService.GetFileSystemEntries(FirstPath);
-
-            fileSystemMock.Verify(f => f.GetFileSystemEntries(FirstPath), Times.Exactly(2));
-        }
-
-        [Fact]
-        public void GetFileSystemEntries_SecondServiceOverSameFileSystem_ReusesTheFirstAnswer()
-        {
-            var fileSystemMock = new Mock<IFileSystem>();
-            fileSystemMock.Setup(f => f.GetFileSystemEntries(_lowerCasePath))
-                .Returns(_lowerCaseFileSystemMetadata);
-
-            new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath);
-            var result = new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath);
-
-            Assert.Equal(_lowerCaseFileSystemMetadata, result);
-            fileSystemMock.Verify(f => f.GetFileSystemEntries(_lowerCasePath), Times.Once);
-        }
-
-        [Fact]
-        public void GetFileSystemEntries_SeparateFileSystems_DoNotShareAnswers()
-        {
-            var firstFileSystem = new Mock<IFileSystem>();
-            firstFileSystem.Setup(f => f.GetFileSystemEntries(_lowerCasePath))
-                .Returns(_lowerCaseFileSystemMetadata);
-            var secondFileSystem = new Mock<IFileSystem>();
-            secondFileSystem.Setup(f => f.GetFileSystemEntries(_lowerCasePath))
-                .Returns(_upperCaseFileSystemMetadata);
-
-            var firstResult = new DirectoryService(firstFileSystem.Object).GetFileSystemEntries(_lowerCasePath);
-            var secondResult = new DirectoryService(secondFileSystem.Object).GetFileSystemEntries(_lowerCasePath);
-
-            Assert.Equal(_lowerCaseFileSystemMetadata, firstResult);
-            Assert.Equal(_upperCaseFileSystemMetadata, secondResult);
-        }
-
-        [Fact]
         public void Invalidate_GivenADirectory_DropsBothTheListingAndTheFilePaths()
         {
             var fileSystemMock = new Mock<IFileSystem>();
@@ -364,22 +307,6 @@ namespace Jellyfin.Controller.Tests
         }
 
         [Fact]
-        public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem()
-        {
-            var fileSystemMock = new Mock<IFileSystem>();
-            fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(_lowerCasePath))
-                .Returns(_lowerCaseFileSystemMetadata)
-                .Returns(_upperCaseFileSystemMetadata);
-
-            new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath);
-            new DirectoryService(fileSystemMock.Object).Invalidate(Path.Combine(_lowerCasePath, "Song 2.srt"));
-
-            var result = new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath);
-
-            Assert.Equal(_upperCaseFileSystemMetadata, result);
-        }
-
-        [Fact]
         public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory()
         {
             var parentPath = LocalPath("/music");