Limit cache size again

Co-Authored-By: Cody Robibero <cody@robibe.ro>

MediaBrowser.Controller/Providers/DirectoryService.cs

@@ -5,13 +5,19 @@ using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Threading;
 using MediaBrowser.Model.IO;
 
 namespace MediaBrowser.Controller.Providers
 {
     public class DirectoryService : IDirectoryService
     {
-        // TODO make static and switch to FastConcurrentLru.
+        // TODO replace with one shared bounded cache.
+        private const int MaxCachedRecords = 100_000;
+        private const int AccessIntervalMs = 1_000;
+        // Timeout cache if no access for 5 minutes.
+        private const int IdleTimeoutMs = 5 * 60 * 1_000;
+
         private readonly ConcurrentDictionary<string, FileSystemMetadata[]> _cache = new(StringComparer.Ordinal);
 
         private readonly ConcurrentDictionary<string, FileSystemMetadata> _fileCache = new(StringComparer.Ordinal);
@@ -20,6 +26,12 @@ namespace MediaBrowser.Controller.Providers
 
         private readonly IFileSystem _fileSystem;
 
+        // ConcurrentDictionary.Count locks the dictionary, so keep an estimated counter.
+        // Concurrent factory runs can overcount and a clear racing an add can undercount,
+        // it only has to be roughly right.
+        private int _recordCount;
+        private long _lastAccess = Environment.TickCount64;
+
         public DirectoryService(IFileSystem fileSystem)
         {
             _fileSystem = fileSystem;
@@ -27,20 +39,26 @@ namespace MediaBrowser.Controller.Providers
 
         public FileSystemMetadata[] GetFileSystemEntries(string path)
         {
+            DropCacheIfIdleOrFull();
+
             return _cache.GetOrAdd(
                 path,
-                static (p, fileSystem) =>
+                static (p, state) =>
                 {
+                    FileSystemMetadata[] entries;
                     try
                     {
-                        return fileSystem.GetFileSystemEntries(p).ToArray();
+                        entries = state.FileSystem.GetFileSystemEntries(p).ToArray();
                     }
                     catch (DirectoryNotFoundException)
                     {
-                        return [];
+                        entries = [];
                     }
+
+                    Interlocked.Add(ref state.Service._recordCount, entries.Length + 1);
+                    return entries;
                 },
-                _fileSystem);
+                (FileSystem: _fileSystem, Service: this));
         }
 
         public List<FileSystemMetadata> GetDirectories(string path)
@@ -89,13 +107,18 @@ namespace MediaBrowser.Controller.Providers
 
         public FileSystemMetadata? GetFileSystemEntry(string path)
         {
+            DropCacheIfIdleOrFull();
+
             if (!_fileCache.TryGetValue(path, out var result))
             {
                 var file = _fileSystem.GetFileSystemInfo(path);
                 if (file?.Exists ?? false)
                 {
                     result = file;
-                    _fileCache.TryAdd(path, result);
+                    if (_fileCache.TryAdd(path, result))
+                    {
+                        Interlocked.Increment(ref _recordCount);
+                    }
                 }
             }
 
@@ -107,25 +130,31 @@ namespace MediaBrowser.Controller.Providers
 
         public IReadOnlyList<string> GetFilePaths(string path, bool clearCache)
         {
-            if (clearCache)
+            if (clearCache && _filePathCache.TryRemove(path, out var cached))
             {
-                _filePathCache.TryRemove(path, out _);
+                Interlocked.Add(ref _recordCount, -(cached.Count + 1));
             }
 
+            DropCacheIfIdleOrFull();
+
             var filePaths = _filePathCache.GetOrAdd(
                 path,
-                static (p, fileSystem) =>
+                static (p, state) =>
                 {
+                    List<string> filePaths;
                     try
                     {
-                        return fileSystem.GetFilePaths(p).OrderBy(x => x).ToList();
+                        filePaths = state.FileSystem.GetFilePaths(p).OrderBy(x => x).ToList();
                     }
                     catch (DirectoryNotFoundException)
                     {
-                        return [];
+                        filePaths = [];
                     }
+
+                    Interlocked.Add(ref state.Service._recordCount, filePaths.Count + 1);
+                    return filePaths;
                 },
-                _fileSystem);
+                (FileSystem: _fileSystem, Service: this));
 
             return filePaths;
         }
@@ -146,11 +175,43 @@ namespace MediaBrowser.Controller.Providers
             return _fileSystem.GetFileSystemEntryPaths(path).Any();
         }
 
+        private void DropCacheIfIdleOrFull()
+        {
+            var nowMs = Environment.TickCount64;
+            var idleMs = nowMs - Volatile.Read(ref _lastAccess);
+
+            if (idleMs >= IdleTimeoutMs || Volatile.Read(ref _recordCount) >= MaxCachedRecords)
+            {
+                _cache.Clear();
+                _fileCache.Clear();
+                _filePathCache.Clear();
+                Volatile.Write(ref _recordCount, 0);
+                Volatile.Write(ref _lastAccess, nowMs);
+                return;
+            }
+
+            if (idleMs >= AccessIntervalMs)
+            {
+                Volatile.Write(ref _lastAccess, nowMs);
+            }
+        }
+
         private void Forget(string path)
         {
-            _cache.TryRemove(path, out _);
-            _fileCache.TryRemove(path, out _);
-            _filePathCache.TryRemove(path, out _);
+            if (_cache.TryRemove(path, out var entries))
+            {
+                Interlocked.Add(ref _recordCount, -(entries.Length + 1));
+            }
+
+            if (_fileCache.TryRemove(path, out _))
+            {
+                Interlocked.Decrement(ref _recordCount);
+            }
+
+            if (_filePathCache.TryRemove(path, out var filePaths))
+            {
+                Interlocked.Add(ref _recordCount, -(filePaths.Count + 1));
+            }
         }
     }
 }

tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs

@@ -1,3 +1,4 @@
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using MediaBrowser.Controller.Providers;
@@ -326,6 +327,78 @@ namespace Jellyfin.Controller.Tests
             fileSystemMock.Verify(f => f.GetFileSystemEntries(parentPath), Times.Once);
         }
 
+        [Fact]
+        public void GetFileSystemEntries_MoreRecordsThanTheCeiling_DropsCache()
+        {
+            // Charged by the files in a listing, not the number of listings, so a few big folders
+            // reach the limit where a lot of small ones would not.
+            const int FolderCount = 60;
+            var bigListing = new FileSystemMetadata[5000];
+            for (var i = 0; i < bigListing.Length; i++)
+            {
+                bigListing[i] = new FileSystemMetadata
+                {
+                    FullName = "/music/track" + i.ToString(CultureInfo.InvariantCulture),
+                    IsDirectory = false
+                };
+            }
+
+            var fileSystemMock = new Mock<IFileSystem>();
+            fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>()))
+                .Returns(bigListing);
+
+            var directoryService = new DirectoryService(fileSystemMock.Object);
+
+            const string FirstPath = "/music/artist0";
+            directoryService.GetFileSystemEntries(FirstPath);
+
+            for (var i = 1; i < FolderCount; 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_RepeatedlyInvalidatedFolder_KeepsUnrelatedEntriesCached()
+        {
+            // Invalidating gives the records back, so churning one folder must not add up to the
+            // ceiling and drop everything else with it.
+            const int ChurnCount = 50;
+            var bigListing = new FileSystemMetadata[5000];
+            for (var i = 0; i < bigListing.Length; i++)
+            {
+                bigListing[i] = new FileSystemMetadata
+                {
+                    FullName = "/music/track" + i.ToString(CultureInfo.InvariantCulture),
+                    IsDirectory = false
+                };
+            }
+
+            var fileSystemMock = new Mock<IFileSystem>();
+            fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>()))
+                .Returns(bigListing);
+
+            var directoryService = new DirectoryService(fileSystemMock.Object);
+
+            const string ChurnedPath = "/music/watched";
+            const string StablePath = "/music/untouched";
+            directoryService.GetFileSystemEntries(StablePath);
+
+            for (var i = 0; i < ChurnCount; i++)
+            {
+                directoryService.GetFileSystemEntries(ChurnedPath);
+                directoryService.Invalidate(ChurnedPath);
+            }
+
+            directoryService.GetFileSystemEntries(StablePath);
+
+            fileSystemMock.Verify(f => f.GetFileSystemEntries(StablePath), Times.Once);
+        }
+
         [Fact]
         public void GetFileSystemEntry_MissingPath_IsNotRemembered()
         {

tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs

@@ -65,6 +65,7 @@ namespace Jellyfin.Controller.Tests.LibraryTaskScheduler
         /// so a shutdown has to reach them. It does not travel from the linked source back to the one
         /// the cleanup cancels, which is what made them immortal.
         /// </summary>
+        /// <returns><placeholder>A <see cref="Task"/> representing the asynchronous unit test.</placeholder></returns>
         [Fact]
         public async Task ApplicationStopping_RetiresRunners()
         {
@@ -87,6 +88,7 @@ namespace Jellyfin.Controller.Tests.LibraryTaskScheduler
         /// The cleanup used to be a one shot: it never released the scheduling slot it took, so
         /// every runner spawned after the first pass stayed around for the lifetime of the server.
         /// </summary>
+        /// <returns><placeholder>A <see cref="Task"/> representing the asynchronous unit test.</placeholder></returns>
         [Fact]
         public async Task Enqueue_RetiresIdleRunnersAfterEveryOperation()
         {
@@ -108,6 +110,7 @@ namespace Jellyfin.Controller.Tests.LibraryTaskScheduler
         /// Disposing used to sit out the rest of the cleanup grace period, holding up shutdown for
         /// up to a minute.
         /// </summary>
+        /// <returns><placeholder>A <see cref="Task"/> representing the asynchronous unit test.</placeholder></returns>
         [Fact]
         public async Task DisposeAsync_DoesNotWaitOutTheGracePeriod()
         {