From e5dc3b8a54fdad218ba062f8adb58fb5e42d8f9d Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Tue, 1 Sep 2026 21:17:07 +0200 Subject: Bound the directory caches a singleton would otherwise hold for the process lifetime --- .../Providers/DirectoryService.cs | 33 +++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 6060d051a5..43f0f11dfe 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -1,22 +1,32 @@ #pragma warning disable CS1591 using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.IO; using System.Linq; +using BitFaster.Caching.Lru; using MediaBrowser.Model.IO; namespace MediaBrowser.Controller.Providers { public class DirectoryService : IDirectoryService { - // TODO make static and switch to FastConcurrentLru. - private readonly ConcurrentDictionary _cache = new(StringComparer.Ordinal); + private const int DirectoryCacheSize = 2048; + private const int FileCacheSize = 4096; - private readonly ConcurrentDictionary _fileCache = new(StringComparer.Ordinal); + // A bounded LRU sizes its table up front, so it costs several kilobytes while still empty. + // One instance is a DI singleton and lives for the process, but the library code also news + // one up per item in several loops and hands it to QueueRefresh, which holds on to it until + // the refresh runs. Those instances usually ask about a single path, so each cache waits + // until something actually looks in it. + private readonly Lazy> _cache + = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal)); - private readonly ConcurrentDictionary> _filePathCache = new(StringComparer.Ordinal); + private readonly Lazy> _fileCache + = new(static () => new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal)); + + private readonly Lazy>> _filePathCache + = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal)); private readonly IFileSystem _fileSystem; @@ -27,7 +37,7 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata[] GetFileSystemEntries(string path) { - return _cache.GetOrAdd( + return _cache.Value.GetOrAdd( path, static (p, fileSystem) => { @@ -89,13 +99,16 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata? GetFileSystemEntry(string path) { - if (!_fileCache.TryGetValue(path, out var result)) + if (!_fileCache.Value.TryGet(path, out var result)) { var file = _fileSystem.GetFileSystemInfo(path); + + // Only a hit is remembered. A miss is the one answer that changes on its own, when + // the file the path names turns up. if (file?.Exists ?? false) { result = file; - _fileCache.TryAdd(path, result); + _fileCache.Value.AddOrUpdate(path, result); } } @@ -109,10 +122,10 @@ namespace MediaBrowser.Controller.Providers { if (clearCache) { - _filePathCache.TryRemove(path, out _); + _filePathCache.Value.TryRemove(path, out _); } - var filePaths = _filePathCache.GetOrAdd( + var filePaths = _filePathCache.Value.GetOrAdd( path, static (p, fileSystem) => { -- cgit v1.2.3 From d73e3d964e3ce8197ec86bdaad447072305bc0d6 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Wed, 2 Sep 2026 07:06:08 +0200 Subject: Optimize Caches Co-Authored-By: Cody Robibero --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 11 ++ .../Library/LibraryManager.cs | 9 ++ .../Controllers/LibraryStructureController.cs | 12 +- .../Providers/DirectoryService.cs | 73 ++++++++---- .../Providers/IDirectoryService.cs | 6 + MediaBrowser.Providers/Lyric/LyricManager.cs | 13 ++ .../Subtitles/SubtitleManager.cs | 12 ++ .../DirectoryServiceTests.cs | 131 +++++++++++++++++++-- 8 files changed, 236 insertions(+), 31 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index 1bf0f8c76c..bc76c99451 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -8,6 +8,7 @@ using Emby.Server.Implementations.Library; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; using MediaBrowser.Model.IO; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -21,6 +22,7 @@ namespace Emby.Server.Implementations.IO private readonly ILibraryManager _libraryManager; private readonly IServerConfigurationManager _configurationManager; private readonly IFileSystem _fileSystem; + private readonly IDirectoryService _directoryService; private readonly DotIgnoreIgnoreRule _dotIgnoreIgnoreRule; /// @@ -47,6 +49,7 @@ namespace Emby.Server.Implementations.IO /// The library manager. /// The configuration manager. /// The filesystem. + /// The directory service. /// The . /// The .ignore rule handler. public LibraryMonitor( @@ -54,6 +57,7 @@ namespace Emby.Server.Implementations.IO ILibraryManager libraryManager, IServerConfigurationManager configurationManager, IFileSystem fileSystem, + IDirectoryService directoryService, IHostApplicationLifetime appLifetime, DotIgnoreIgnoreRule dotIgnoreIgnoreRule) { @@ -61,6 +65,7 @@ namespace Emby.Server.Implementations.IO _logger = logger; _configurationManager = configurationManager; _fileSystem = fileSystem; + _directoryService = directoryService; _dotIgnoreIgnoreRule = dotIgnoreIgnoreRule; appLifetime.ApplicationStarted.Register(Start); @@ -363,6 +368,12 @@ namespace Emby.Server.Implementations.IO return; } + // Something changed on disk that the server did not necessarily do itself, so whatever is + // cached about that path and its folder is now a guess. This sits above the checks below + // because a change we deliberately do not refresh for still has to be read correctly the + // next time somebody looks at that folder. + _directoryService.Invalidate(path); + // Ignore certain files, If the parent of an ignored path has a change event, ignore that too foreach (var i in _tempIgnoredPaths.Keys) { diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs index 3db8265f6e..92a87d9f50 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -88,6 +88,7 @@ namespace Emby.Server.Implementations.Library private readonly ExtraResolver _extraResolver; private readonly IPathManager _pathManager; private readonly ILocalizationManager _localization; + private readonly IDirectoryService _directoryService; private readonly FastConcurrentLru _cache; private readonly DotIgnoreIgnoreRule _dotIgnoreIgnoreRule; private readonly IMediaStreamRepository _mediaStreamRepository; @@ -189,6 +190,7 @@ namespace Emby.Server.Implementations.Library _pathManager = pathManager; _dotIgnoreIgnoreRule = dotIgnoreIgnoreRule; _localization = localization; + _directoryService = directoryService; _extraResolver = new ExtraResolver(loggerFactory.CreateLogger(), namingOptions, directoryService); _configurationManager.ConfigurationUpdated += ConfigurationUpdated; @@ -3720,6 +3722,10 @@ namespace Emby.Server.Implementations.Library AddMediaPathInternal(name, path, false); } } + + // The directory caches are shared, so the validation below would otherwise resolve + // the libraries root from a listing taken before this folder was created. + _directoryService.Invalidate(virtualFolderPath); } finally { @@ -3920,6 +3926,7 @@ namespace Emby.Server.Implementations.Library try { Directory.Delete(path, true); + _directoryService.Invalidate(path); } finally { @@ -3989,6 +3996,7 @@ namespace Emby.Server.Implementations.Library if (!string.IsNullOrEmpty(shortcut)) { _fileSystem.DeleteFile(shortcut); + _directoryService.Invalidate(shortcut); } var libraryOptions = CollectionFolder.GetLibraryOptions(virtualFolderPath); @@ -4032,6 +4040,7 @@ namespace Emby.Server.Implementations.Library } _fileSystem.CreateShortcut(lnk, _appHost.ReverseVirtualPath(path)); + _directoryService.Invalidate(lnk); RemoveContentTypeOverrides(path); } diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs index 5c596c21b9..5cd6dada19 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -16,6 +16,7 @@ using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.IO; using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; using MediaBrowser.Model.Configuration; using MediaBrowser.Model.Entities; using Microsoft.AspNetCore.Authorization; @@ -34,6 +35,7 @@ public class LibraryStructureController : BaseJellyfinApiController private readonly IServerApplicationPaths _appPaths; private readonly ILibraryManager _libraryManager; private readonly ILibraryMonitor _libraryMonitor; + private readonly IDirectoryService _directoryService; /// /// Initializes a new instance of the class. @@ -41,14 +43,17 @@ public class LibraryStructureController : BaseJellyfinApiController /// Instance of interface. /// Instance of interface. /// Instance of interface. + /// Instance of interface. public LibraryStructureController( IServerConfigurationManager serverConfigurationManager, ILibraryManager libraryManager, - ILibraryMonitor libraryMonitor) + ILibraryMonitor libraryMonitor, + IDirectoryService directoryService) { _appPaths = serverConfigurationManager.ApplicationPaths; _libraryManager = libraryManager; _libraryMonitor = libraryMonitor; + _directoryService = directoryService; } /// @@ -183,6 +188,11 @@ public class LibraryStructureController : BaseJellyfinApiController } Directory.Move(currentPath, newPath); + + // The directory caches are shared, so the validation below would otherwise resolve the + // libraries root from a listing taken before the folder was moved. + _directoryService.Invalidate(currentPath); + _directoryService.Invalidate(newPath); } finally { diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 43f0f11dfe..336e35e293 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.IO; using System.Linq; +using System.Runtime.CompilerServices; using BitFaster.Caching.Lru; using MediaBrowser.Model.IO; @@ -11,33 +12,20 @@ namespace MediaBrowser.Controller.Providers { public class DirectoryService : IDirectoryService { - private const int DirectoryCacheSize = 2048; - private const int FileCacheSize = 4096; - - // A bounded LRU sizes its table up front, so it costs several kilobytes while still empty. - // One instance is a DI singleton and lives for the process, but the library code also news - // one up per item in several loops and hands it to QueueRefresh, which holds on to it until - // the refresh runs. Those instances usually ask about a single path, so each cache waits - // until something actually looks in it. - private readonly Lazy> _cache - = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal)); - - private readonly Lazy> _fileCache - = new(static () => new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal)); - - private readonly Lazy>> _filePathCache - = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal)); + private static readonly ConditionalWeakTable _caches = []; 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.Value.GetOrAdd( + return _cache.Entries.GetOrAdd( path, static (p, fileSystem) => { @@ -99,7 +87,7 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata? GetFileSystemEntry(string path) { - if (!_fileCache.Value.TryGet(path, out var result)) + if (!_cache.Files.TryGet(path, out var result)) { var file = _fileSystem.GetFileSystemInfo(path); @@ -108,7 +96,7 @@ namespace MediaBrowser.Controller.Providers if (file?.Exists ?? false) { result = file; - _fileCache.Value.AddOrUpdate(path, result); + _cache.Files.AddOrUpdate(path, result); } } @@ -122,10 +110,12 @@ namespace MediaBrowser.Controller.Providers { if (clearCache) { - _filePathCache.Value.TryRemove(path, out _); + // Only what is remembered about this directory. Invalidate() also drops the parent, + // which a write needs but which is needless churn on a shared cache here. + Forget(path); } - var filePaths = _filePathCache.Value.GetOrAdd( + return _cache.FilePaths.GetOrAdd( path, static (p, fileSystem) => { @@ -139,13 +129,52 @@ namespace MediaBrowser.Controller.Providers } }, _fileSystem); + } - return filePaths; + public void Invalidate(string path) + { + // Everything remembered about the path itself, and the listing of the directory holding + // it, since writing a file changes what its directory contains. The caches belong to the + // file system rather than to this instance, so this is felt by every reader of it. + Forget(path); + + var parent = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(parent)) + { + Forget(parent); + } } public bool IsAccessible(string path) { return _fileSystem.GetFileSystemEntryPaths(path).Any(); } + + 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; + + // A DirectoryService no longer bounds how long its answers are trusted by dying, so a + // lifetime does. This is a staleness bound, not a snapshot: a long refresh can outlive it + // and re-read a directory partway through. + private static readonly TimeSpan _entryLifetime = TimeSpan.FromMinutes(1); + + public ConcurrentTLru Entries { get; } + = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime); + + public ConcurrentTLru Files { get; } + = new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal, _entryLifetime); + + public ConcurrentTLru> FilePaths { get; } + = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime); + } } } diff --git a/MediaBrowser.Controller/Providers/IDirectoryService.cs b/MediaBrowser.Controller/Providers/IDirectoryService.cs index 8a3fa33da3..609d094254 100644 --- a/MediaBrowser.Controller/Providers/IDirectoryService.cs +++ b/MediaBrowser.Controller/Providers/IDirectoryService.cs @@ -23,6 +23,12 @@ namespace MediaBrowser.Controller.Providers IReadOnlyList GetFilePaths(string path, bool clearCache); + /// + /// Forgets what is cached about a path and about the directory containing it. + /// + /// The file or directory path that changed. + void Invalidate(string path); + bool IsAccessible(string path); } } diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs index af31e373ef..bff076b27b 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -32,6 +32,7 @@ public class LyricManager : ILyricManager private readonly IFileSystem _fileSystem; private readonly ILibraryMonitor _libraryMonitor; private readonly IMediaSourceManager _mediaSourceManager; + private readonly IDirectoryService _directoryService; private readonly ILyricProvider[] _lyricProviders; private readonly ILyricParser[] _lyricParsers; @@ -43,6 +44,7 @@ public class LyricManager : ILyricManager /// Instance of the interface. /// Instance of the interface. /// Instance of the interface. + /// Instance of the interface. /// The list of . /// The list of . public LyricManager( @@ -50,6 +52,7 @@ public class LyricManager : ILyricManager IFileSystem fileSystem, ILibraryMonitor libraryMonitor, IMediaSourceManager mediaSourceManager, + IDirectoryService directoryService, IEnumerable lyricProviders, IEnumerable lyricParsers) { @@ -57,6 +60,7 @@ public class LyricManager : ILyricManager _fileSystem = fileSystem; _libraryMonitor = libraryMonitor; _mediaSourceManager = mediaSourceManager; + _directoryService = directoryService; _lyricProviders = lyricProviders .OrderBy(i => i is IHasOrder hasOrder ? hasOrder.Order : 0) .ToArray(); @@ -250,6 +254,10 @@ public class LyricManager : ILyricManager { _libraryMonitor.ReportFileSystemChangeComplete(path, false); } + + // The refresh below reads the containing folder to find external lyrics, and would find + // the deleted one again in a cached listing. + _directoryService.Invalidate(path); } return audio.RefreshMetadata(CancellationToken.None); @@ -446,6 +454,11 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } + // The directory caches are shared and outlive this call, so the refresh that follows + // would otherwise resolve external lyrics from a listing taken before this file + // landed. + _directoryService.Invalidate(savePath); + return; } catch (Exception ex) diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs index c3458d4b2a..4a2f58d457 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -33,6 +33,7 @@ namespace MediaBrowser.Providers.Subtitles private readonly ILibraryMonitor _monitor; private readonly IMediaSourceManager _mediaSourceManager; private readonly ILocalizationManager _localization; + private readonly IDirectoryService _directoryService; private readonly HashSet _allowedSubtitleFormats; private readonly ISubtitleProvider[] _subtitleProviders; @@ -43,6 +44,7 @@ namespace MediaBrowser.Providers.Subtitles ILibraryMonitor monitor, IMediaSourceManager mediaSourceManager, ILocalizationManager localizationManager, + IDirectoryService directoryService, IEnumerable subtitleProviders, NamingOptions namingOptions) { @@ -51,6 +53,7 @@ namespace MediaBrowser.Providers.Subtitles _monitor = monitor; _mediaSourceManager = mediaSourceManager; _localization = localizationManager; + _directoryService = directoryService; _subtitleProviders = subtitleProviders .OrderBy(i => i is IHasOrder hasOrder ? hasOrder.Order : 0) .ToArray(); @@ -281,6 +284,11 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } + // The directory caches are shared and outlive this call, so the refresh that + // follows would otherwise resolve external subtitles from a listing taken + // before this file landed. + _directoryService.Invalidate(path); + return; } else @@ -395,6 +403,10 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } + // The refresh below reads the containing folder to find external subtitles, and would + // find the deleted one again in a cached listing. + _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 bc03bfc33b..a550828783 100644 --- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs +++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs @@ -268,10 +268,10 @@ namespace Jellyfin.Controller.Tests [Fact] public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing() { - // The service is a singleton, so the cache has to give entries back rather than hold every - // path the server ever saw. Asking for far more paths than it can hold must push the first - // one out, which shows up as the file system being read for it a second time. - const int PathCount = 40000; + // The cache outlives every DirectoryService that reads it, so it has to give entries back + // rather than hold every path the server ever saw. Asking for more paths than it can hold + // must push the first one out, which shows up as the file system being read for it twice. + const int PathCount = 8192; var fileSystemMock = new Mock(); fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny())) @@ -279,17 +279,132 @@ namespace Jellyfin.Controller.Tests var directoryService = new DirectoryService(fileSystemMock.Object); - var firstPath = "/music/artist0"; - directoryService.GetFileSystemEntries(firstPath); + 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); + directoryService.GetFileSystemEntries(FirstPath); - fileSystemMock.Verify(f => f.GetFileSystemEntries(firstPath), Times.Exactly(2)); + fileSystemMock.Verify(f => f.GetFileSystemEntries(FirstPath), Times.Exactly(2)); + } + + [Fact] + public void GetFileSystemEntries_SecondServiceOverSameFileSystem_ReusesTheFirstAnswer() + { + // The library code news up a DirectoryService per item, so what one of them learned about + // a directory has to be worth something to the next one reading the same file system. + var fileSystemMock = new Mock(); + 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(); + firstFileSystem.Setup(f => f.GetFileSystemEntries(LowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata); + var secondFileSystem = new Mock(); + 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() + { + // Clearing only one of the two views of a directory leaves the other one answering from + // before whatever was just written there. + var fileSystemMock = new Mock(); + fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata) + .Returns(_upperCaseFileSystemMetadata); + fileSystemMock.SetupSequence(f => f.GetFilePaths(LowerCasePath, false)) + .Returns(new[] { LowerCasePath + "/Song 2.mp3" }) + .Returns(new[] { LowerCasePath + "/Song 2.mp3", LowerCasePath + "/Song 2.srt" }); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(LowerCasePath); + directoryService.GetFilePaths(LowerCasePath); + + directoryService.Invalidate(LowerCasePath); + + Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(LowerCasePath)); + Assert.Equal(2, directoryService.GetFilePaths(LowerCasePath).Count); + } + + [Fact] + public void Invalidate_GivenAFile_DropsTheListingOfTheDirectoryHoldingIt() + { + // Downloading a subtitle changes what its folder contains, not just the one path. + const string NewFile = LowerCasePath + "/Song 2.srt"; + + var fileSystemMock = new Mock(); + fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata) + .Returns(_upperCaseFileSystemMetadata); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(LowerCasePath); + + directoryService.Invalidate(NewFile); + + Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(LowerCasePath)); + } + + [Fact] + public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem() + { + // Whoever writes the file and whoever refreshes the item hold different services, so + // invalidating has to reach the cache both of them read. + var fileSystemMock = new Mock(); + fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata) + .Returns(_upperCaseFileSystemMetadata); + + new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(LowerCasePath); + new DirectoryService(fileSystemMock.Object).Invalidate(LowerCasePath + "/Song 2.srt"); + + var result = new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(LowerCasePath); + + Assert.Equal(_upperCaseFileSystemMetadata, result); + } + + [Fact] + public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory() + { + // Re-reading one directory is not a reason to make the server list the library folder + // holding it again, which the shared cache would otherwise have to do. + const string ParentPath = "/music"; + + var fileSystemMock = new Mock(); + fileSystemMock.Setup(f => f.GetFilePaths(LowerCasePath)) + .Returns(new[] { LowerCasePath + "/Song 2.mp3" }); + fileSystemMock.Setup(f => f.GetFileSystemEntries(ParentPath)) + .Returns(_lowerCaseFileSystemMetadata); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(ParentPath); + + directoryService.GetFilePaths(LowerCasePath, true); + + directoryService.GetFileSystemEntries(ParentPath); + fileSystemMock.Verify(f => f.GetFileSystemEntries(ParentPath), Times.Once); } [Fact] -- cgit v1.2.3 From b724e57458e60e967bf05e943c5cf6b1a41f4044 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Thu, 3 Sep 2026 11:51:02 +0200 Subject: Reduce comments --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 6 ++---- Emby.Server.Implementations/Library/LibraryManager.cs | 4 ++-- Jellyfin.Api/Controllers/LibraryStructureController.cs | 4 ++-- MediaBrowser.Controller/Providers/DirectoryService.cs | 14 ++++---------- MediaBrowser.Providers/Lyric/LyricManager.cs | 7 ++----- MediaBrowser.Providers/Manager/ProviderManager.cs | 17 +++++++---------- MediaBrowser.Providers/Subtitles/SubtitleManager.cs | 7 ++----- .../Jellyfin.Controller.Tests/DirectoryServiceTests.cs | 14 +------------- .../Manager/ProviderManagerTests.cs | 18 ++++++------------ 9 files changed, 28 insertions(+), 63 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index bc76c99451..e6a33d9dd3 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -368,10 +368,8 @@ namespace Emby.Server.Implementations.IO return; } - // Something changed on disk that the server did not necessarily do itself, so whatever is - // cached about that path and its folder is now a guess. This sits above the checks below - // because a change we deliberately do not refresh for still has to be read correctly the - // next time somebody looks at that folder. + // 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. _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 92a87d9f50..0ced650587 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -3723,8 +3723,8 @@ namespace Emby.Server.Implementations.Library } } - // The directory caches are shared, so the validation below would otherwise resolve - // the libraries root from a listing taken before this folder was created. + // The validation below would otherwise resolve the libraries root from a listing + // taken before this folder was created. _directoryService.Invalidate(virtualFolderPath); } finally diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs index 5cd6dada19..83684f1b91 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -189,8 +189,8 @@ public class LibraryStructureController : BaseJellyfinApiController Directory.Move(currentPath, newPath); - // The directory caches are shared, so the validation below would otherwise resolve the - // libraries root from a listing taken before the folder was moved. + // The validation below would otherwise resolve the libraries root from a listing taken + // before the folder was moved. _directoryService.Invalidate(currentPath); _directoryService.Invalidate(newPath); } diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 336e35e293..613433391b 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -91,8 +91,7 @@ namespace MediaBrowser.Controller.Providers { var file = _fileSystem.GetFileSystemInfo(path); - // Only a hit is remembered. A miss is the one answer that changes on its own, when - // the file the path names turns up. + // Only cache hits: a missing file can turn up later. if (file?.Exists ?? false) { result = file; @@ -110,8 +109,7 @@ namespace MediaBrowser.Controller.Providers { if (clearCache) { - // Only what is remembered about this directory. Invalidate() also drops the parent, - // which a write needs but which is needless churn on a shared cache here. + // Not Invalidate(), which would also drop the parent listing for no reason here. Forget(path); } @@ -133,9 +131,6 @@ namespace MediaBrowser.Controller.Providers public void Invalidate(string path) { - // Everything remembered about the path itself, and the listing of the directory holding - // it, since writing a file changes what its directory contains. The caches belong to the - // file system rather than to this instance, so this is felt by every reader of it. Forget(path); var parent = Path.GetDirectoryName(path); @@ -162,9 +157,8 @@ namespace MediaBrowser.Controller.Providers private const int DirectoryCacheSize = 2048; private const int FileCacheSize = 8192; - // A DirectoryService no longer bounds how long its answers are trusted by dying, so a - // lifetime does. This is a staleness bound, not a snapshot: a long refresh can outlive it - // and re-read a directory partway through. + // 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 Entries { get; } diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs index bff076b27b..57d98af1da 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -255,8 +255,7 @@ public class LyricManager : ILyricManager _libraryMonitor.ReportFileSystemChangeComplete(path, false); } - // The refresh below reads the containing folder to find external lyrics, and would find - // the deleted one again in a cached listing. + // The refresh below would otherwise find the deleted file in a cached listing. _directoryService.Invalidate(path); } @@ -454,9 +453,7 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The directory caches are shared and outlive this call, so the refresh that follows - // would otherwise resolve external lyrics from a listing taken before this file - // landed. + // The refresh that follows would otherwise not see the new file. _directoryService.Invalidate(savePath); return; diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 4ff9ab1a52..e7b15305b3 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -1143,8 +1143,8 @@ namespace MediaBrowser.Providers.Manager return; } - // PriorityQueue is not thread safe, and this runs on whichever thread queued the refresh - // while the processor dequeues on its own, so every touch of the queue takes the lock. + // PriorityQueue is not thread safe and the processor dequeues concurrently, so every + // touch of the queue takes the lock. lock (_refreshQueueLock) { _refreshQueue.Enqueue((itemId, options), priority); @@ -1182,10 +1182,8 @@ namespace MediaBrowser.Providers.Manager { (Guid ItemId, MetadataRefreshOptions RefreshOptions) refreshItem; - // Standing down and taking the next entry happen under one lock, and this is the - // only place the flag is handed back. Releasing it anywhere else would leave a gap - // in which a refresh queued just after the queue ran dry sees a processor that has - // already stopped, and waits forever. + // Dequeueing and standing down happen under one lock, otherwise a refresh queued + // just after the queue ran dry would see a processor that has already stopped. lock (_refreshQueueLock) { if (_disposed @@ -1213,14 +1211,13 @@ namespace MediaBrowser.Providers.Manager } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - // Shutting down. Whatever is still queued keeps its place; the next pass round - // stands the processor down, so the next refresh queued starts one of its own. + // Shutting down: the next pass sees the token and stands the processor down. continue; } catch (Exception ex) { - // A provider that cancelled for its own reasons lands here too, an HTTP timeout - // above all. One unreachable metadata server must not stop the queue draining. + // Includes a provider that cancelled for its own reasons, such as an HTTP + // timeout, which must not stop the queue draining. _logger.LogError(ex, "Error refreshing item"); } } diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs index 4a2f58d457..37d261a95e 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -284,9 +284,7 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The directory caches are shared and outlive this call, so the refresh that - // follows would otherwise resolve external subtitles from a listing taken - // before this file landed. + // The refresh that follows would otherwise not see the new file. _directoryService.Invalidate(path); return; @@ -403,8 +401,7 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } - // The refresh below reads the containing folder to find external subtitles, and would - // find the deleted one again in a cached listing. + // The refresh below would otherwise find the deleted file in a cached listing. _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 a550828783..338ee9c903 100644 --- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs +++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs @@ -268,9 +268,7 @@ namespace Jellyfin.Controller.Tests [Fact] public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing() { - // The cache outlives every DirectoryService that reads it, so it has to give entries back - // rather than hold every path the server ever saw. Asking for more paths than it can hold - // must push the first one out, which shows up as the file system being read for it twice. + // Eviction of the first path shows up as the file system being read for it twice. const int PathCount = 8192; var fileSystemMock = new Mock(); @@ -295,8 +293,6 @@ namespace Jellyfin.Controller.Tests [Fact] public void GetFileSystemEntries_SecondServiceOverSameFileSystem_ReusesTheFirstAnswer() { - // The library code news up a DirectoryService per item, so what one of them learned about - // a directory has to be worth something to the next one reading the same file system. var fileSystemMock = new Mock(); fileSystemMock.Setup(f => f.GetFileSystemEntries(LowerCasePath)) .Returns(_lowerCaseFileSystemMetadata); @@ -328,8 +324,6 @@ namespace Jellyfin.Controller.Tests [Fact] public void Invalidate_GivenADirectory_DropsBothTheListingAndTheFilePaths() { - // Clearing only one of the two views of a directory leaves the other one answering from - // before whatever was just written there. var fileSystemMock = new Mock(); fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath)) .Returns(_lowerCaseFileSystemMetadata) @@ -351,7 +345,6 @@ namespace Jellyfin.Controller.Tests [Fact] public void Invalidate_GivenAFile_DropsTheListingOfTheDirectoryHoldingIt() { - // Downloading a subtitle changes what its folder contains, not just the one path. const string NewFile = LowerCasePath + "/Song 2.srt"; var fileSystemMock = new Mock(); @@ -370,8 +363,6 @@ namespace Jellyfin.Controller.Tests [Fact] public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem() { - // Whoever writes the file and whoever refreshes the item hold different services, so - // invalidating has to reach the cache both of them read. var fileSystemMock = new Mock(); fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath)) .Returns(_lowerCaseFileSystemMetadata) @@ -388,8 +379,6 @@ namespace Jellyfin.Controller.Tests [Fact] public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory() { - // Re-reading one directory is not a reason to make the server list the library folder - // holding it again, which the shared cache would otherwise have to do. const string ParentPath = "/music"; var fileSystemMock = new Mock(); @@ -421,7 +410,6 @@ namespace Jellyfin.Controller.Tests Assert.Null(directoryService.GetFileSystemEntry(MissingPath)); - // The one answer that changes on its own: the file turning up has to be visible. Assert.NotNull(directoryService.GetFileSystemEntry(MissingPath)); } } diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index b3f5af76b7..248b236df8 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -381,10 +381,6 @@ namespace Jellyfin.Providers.Tests.Manager [Fact] public async Task QueueRefresh_ManyItemsQueuedFromManyThreads_ProcessesEveryOne() { - // The queue is filled from whichever thread wants a refresh and drained by a processor of - // its own, so an unsynchronised PriorityQueue can lose entries outright, and a processor - // that stands down before releasing its flag leaves whatever was queued in that gap with - // nobody to drain it. Either way an item silently never gets refreshed. const int ItemCount = 2000; var queued = Enumerable.Range(0, ItemCount).Select(_ => Guid.NewGuid()).ToArray(); @@ -395,7 +391,7 @@ namespace Jellyfin.Providers.Tests.Manager libraryManager.Setup(i => i.GetItemById(It.IsAny())) .Returns((Guid id) => { - // Returning null drains the entry without needing the whole refresh machinery. + // Returning null drains the entry without the whole refresh machinery. processed.Add(id); if (processed.Count == ItemCount) { @@ -425,7 +421,7 @@ namespace Jellyfin.Providers.Tests.Manager } catch (OperationCanceledException) { - // Fall through, so the assertions below name what was lost rather than the wait. + // Fall through so the assertions report what was lost. } Assert.Empty(providerManager.GetRefreshQueue()); @@ -435,9 +431,8 @@ namespace Jellyfin.Providers.Tests.Manager [Fact] public async Task QueueRefresh_RefreshCancelsForItsOwnReasons_KeepsDrainingTheQueue() { - // MetadataService rethrows OperationCanceledException out of a provider, so an HTTP - // timeout against an unreachable metadata server arrives here looking exactly like a - // shutdown. Treating it as one stops the processor and strands the rest of the queue. + // A provider timeout arrives as an OperationCanceledException, indistinguishable from + // a shutdown; treating it as one would strand the rest of the queue. const int ItemCount = 200; var queued = Enumerable.Range(0, ItemCount).Select(_ => Guid.NewGuid()).ToArray(); @@ -454,8 +449,7 @@ namespace Jellyfin.Providers.Tests.Manager { cancelledOnce = true; - // Hold the first entry until the whole batch is queued, so everything that - // follows it is already waiting when the cancellation lands. + // Hold the first entry until the whole batch is queued. allQueued.Wait(TimeSpan.FromSeconds(30)); throw new OperationCanceledException("provider timed out"); } @@ -487,7 +481,7 @@ namespace Jellyfin.Providers.Tests.Manager } catch (OperationCanceledException) { - // Fall through, so the assertions below name what was left stranded. + // Fall through so the assertions report what was stranded. } Assert.Empty(providerManager.GetRefreshQueue()); -- cgit v1.2.3 From 0d9c9c9eccf2f547f824ad47f2038c378930fdd2 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Fri, 4 Sep 2026 19:23:29 +0200 Subject: Fix the library scheduler never retiring its runners --- .../LimitedConcurrencyLibraryScheduler.cs | 148 +++++++++++--- .../LimitedConcurrencyLibrarySchedulerTests.cs | 213 +++++++++++++++++++++ 2 files changed, 332 insertions(+), 29 deletions(-) create mode 100644 tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs (limited to 'MediaBrowser.Controller') diff --git a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs index 6da398129a..3c6ffe3cbd 100644 --- a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs +++ b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs @@ -17,7 +17,8 @@ namespace MediaBrowser.Controller.LibraryTaskScheduler; /// public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibraryScheduler, IAsyncDisposable { - private const int CleanupGracePeriod = 60; + private static readonly TimeSpan _cleanupGracePeriod = TimeSpan.FromSeconds(60); + private readonly IHostApplicationLifetime _hostApplicationLifetime; private readonly ILogger _logger; private readonly IServerConfigurationManager _serverConfigurationManager; @@ -31,6 +32,8 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr private readonly Lock _taskLock = new(); private readonly Channel _tasks = Channel.CreateUnbounded(); + private readonly CancellationTokenSource _disposeTokenSource = new(); + private readonly TimeSpan _gracePeriod; private volatile int _workCounter; private Task? _cleanupTask; @@ -46,10 +49,34 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr IHostApplicationLifetime hostApplicationLifetime, ILogger logger, IServerConfigurationManager serverConfigurationManager) + : this(hostApplicationLifetime, logger, serverConfigurationManager, _cleanupGracePeriod) + { + } + + internal LimitedConcurrencyLibraryScheduler( + IHostApplicationLifetime hostApplicationLifetime, + ILogger logger, + IServerConfigurationManager serverConfigurationManager, + TimeSpan gracePeriod) { _hostApplicationLifetime = hostApplicationLifetime; _logger = logger; _serverConfigurationManager = serverConfigurationManager; + _gracePeriod = gracePeriod; + } + + /// + /// Gets the number of runners the scheduler currently keeps alive. + /// + internal int ActiveRunnerCount + { + get + { + lock (_taskLock) + { + return _taskRunners.Count; + } + } } private void ScheduleTaskCleanup() @@ -68,31 +95,65 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr async Task RunCleanupTask() { - _logger.LogDebug("Schedule cleanup task in {CleanupGracePerioid} sec.", CleanupGracePeriod); - await Task.Delay(TimeSpan.FromSeconds(CleanupGracePeriod)).ConfigureAwait(false); - if (_disposed) + while (true) { - _logger.LogDebug("Abort cleaning up, already disposed."); - return; - } + _logger.LogDebug("Schedule cleanup task in {CleanupGracePeriod}.", _gracePeriod); + try + { + await Task.Delay(_gracePeriod, _disposeTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + _logger.LogDebug("Abort cleaning up, already disposed."); + return; + } - lock (_taskLock) - { - if (_tasks.Reader.Count > 0 || _workCounter > 0) + if (_disposed) { - _logger.LogDebug("Delay cleanup task, operations still running."); - // tasks are still there so its still in use. Reschedule cleanup task. - // we cannot just exit here and rely on the other invoker because there is a considerable timeframe where it could have already ended. - _cleanupTask = RunCleanupTask(); + _logger.LogDebug("Abort cleaning up, already disposed."); return; } + + CancellationTokenSource[] runners; + lock (_taskLock) + { + if (_tasks.Reader.Count > 0 || _workCounter > 0) + { + _logger.LogDebug("Delay cleanup task, operations still running."); + // tasks are still there so its still in use. Wait another grace period. + // we cannot just exit here and rely on the other invoker because there is a considerable timeframe where it could have already ended. + continue; + } + + runners = [.. _taskRunners.Keys]; + + // Retire the runners before they are told to stop: an operation starting while + // they wind down must spawn its own instead of counting these towards the fanout. + _taskRunners.Clear(); + + // Hand the next operation the ability to schedule a cleanup again. Without this + // the very first cleanup would be the only one that ever runs. + _cleanupTask = null; + } + + _logger.LogDebug("Cleanup runners."); + await StopRunners(runners).ConfigureAwait(false); + return; } + } + } - _logger.LogDebug("Cleanup runners."); - foreach (var item in _taskRunners.ToArray()) + private static async Task StopRunners(CancellationTokenSource[] runners) + { + foreach (var runner in runners) + { + try { - await item.Key.CancelAsync().ConfigureAwait(false); - _taskRunners.Remove(item.Key); + await runner.CancelAsync().ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + // The runner already stopped on its own and disposed its stop source. } } } @@ -127,11 +188,14 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr { var stopToken = new CancellationTokenSource(); var combinedSource = CancellationTokenSource.CreateLinkedTokenSource(stopToken.Token, _hostApplicationLifetime.ApplicationStopping); + + // Keyed on its own stop source, because cancelling that is what reaches the linked + // source the runner waits on. Cancellation does not travel the other way. _taskRunners.Add( - combinedSource, + stopToken, Task.Factory.StartNew( ItemWorker, - (combinedSource, stopToken), + (stopToken, combinedSource), combinedSource.Token, TaskCreationOptions.PreferFairness, TaskScheduler.Default)); @@ -145,7 +209,7 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr _deadlockDetector.Value = stopToken.TaskStop; try { - while (!stopToken.GlobalStop.Token.IsCancellationRequested) + while (!stopToken.GlobalStop.IsCancellationRequested) { var item = await _tasks.Reader.ReadAsync(stopToken.GlobalStop.Token).ConfigureAwait(false); try @@ -162,15 +226,24 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr } } } - catch (OperationCanceledException) when (stopToken.TaskStop.IsCancellationRequested) + catch (OperationCanceledException) when (stopToken.GlobalStop.IsCancellationRequested) { // thats how you do it, interupt the waiter thread. There is nothing to do here when it was on purpose. } + catch (ChannelClosedException) + { + // the scheduler was disposed and will not hand out any more work. + } finally { _logger.LogDebug("Cleanup Runner'."); _deadlockDetector.Value = default!; - _taskRunners.Remove(stopToken.TaskStop); + + lock (_taskLock) + { + _taskRunners.Remove(stopToken.TaskStop); + } + stopToken.GlobalStop.Dispose(); stopToken.TaskStop.Dispose(); } @@ -195,7 +268,7 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr finally { item.Progress.Report(100); - item.Done.SetResult(); + item.Done.TrySetResult(); } } @@ -285,16 +358,33 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr _disposed = true; _tasks.Writer.Complete(); - foreach (var item in _taskRunners) + + // Nobody is left to run these, so release whoever is waiting on them. + while (_tasks.Reader.TryRead(out var item)) { - await item.Key.CancelAsync().ConfigureAwait(false); + item.Done.TrySetResult(); } - if (_cleanupTask is not null) + CancellationTokenSource[] runners; + Task? cleanupTask; + lock (_taskLock) { - await _cleanupTask.ConfigureAwait(false); - _cleanupTask?.Dispose(); + runners = [.. _taskRunners.Keys]; + _taskRunners.Clear(); + cleanupTask = _cleanupTask; } + + await StopRunners(runners).ConfigureAwait(false); + + // Cuts the grace period short instead of holding up shutdown for the rest of it. + await _disposeTokenSource.CancelAsync().ConfigureAwait(false); + + if (cleanupTask is not null) + { + await cleanupTask.ConfigureAwait(false); + } + + _disposeTokenSource.Dispose(); } private class TaskQueueItem diff --git a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs new file mode 100644 index 0000000000..21a719a78f --- /dev/null +++ b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs @@ -0,0 +1,213 @@ +using System; +using System.Collections.Concurrent; +using System.Diagnostics; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.LibraryTaskScheduler; +using MediaBrowser.Model.Configuration; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Controller.Tests.LibraryTaskScheduler +{ + public class LimitedConcurrencyLibrarySchedulerTests + { + private static readonly TimeSpan _shortGracePeriod = TimeSpan.FromMilliseconds(50); + + // Generous, because these only ever wait for something that should already have happened. + private static readonly TimeSpan _timeout = TimeSpan.FromSeconds(10); + + [Fact] + public async Task Enqueue_ProcessesEveryItem() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + var data = Enumerable.Range(0, 100).ToArray(); + var processed = new ConcurrentBag(); + + await scheduler.Enqueue( + data, + (item, _) => + { + processed.Add(item); + return Task.CompletedTask; + }, + new Progress(), + CancellationToken.None); + + Assert.Equal(data, processed.Order()); + } + } + + [Fact] + public async Task Enqueue_WithFailingWorker_StillCompletes() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + await scheduler.Enqueue( + Enumerable.Range(0, 20).ToArray(), + (item, _) => item % 2 == 0 ? throw new InvalidOperationException("boom") : Task.CompletedTask, + new Progress(), + CancellationToken.None); + } + } + + /// + /// The runners wait on a source linked to , + /// 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. + /// + [Fact] + public async Task ApplicationStopping_RetiresRunners() + { + using var appStopping = new CancellationTokenSource(); + + // Long enough that the cleanup cannot be what retires them. + var scheduler = CreateScheduler(appStopping, gracePeriod: TimeSpan.FromMinutes(5)); + await using (scheduler) + { + await RunOneOperation(scheduler); + Assert.True(scheduler.ActiveRunnerCount > 0); + + await appStopping.CancelAsync(); + + await WaitForAsync(() => scheduler.ActiveRunnerCount == 0); + } + } + + /// + /// 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. + /// + [Fact] + public async Task Enqueue_RetiresIdleRunnersAfterEveryOperation() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + for (var round = 0; round < 3; round++) + { + await RunOneOperation(scheduler); + Assert.True(scheduler.ActiveRunnerCount > 0, $"no runner spawned in round {round}"); + + await WaitForAsync(() => scheduler.ActiveRunnerCount == 0); + } + } + } + + /// + /// Disposing used to sit out the rest of the cleanup grace period, holding up shutdown for + /// up to a minute. + /// + [Fact] + public async Task DisposeAsync_DoesNotWaitOutTheGracePeriod() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping, gracePeriod: TimeSpan.FromMinutes(5)); + + await RunOneOperation(scheduler); + + var stopwatch = Stopwatch.StartNew(); + await scheduler.DisposeAsync(); + + Assert.True(stopwatch.Elapsed < _timeout, $"disposing took {stopwatch.Elapsed}"); + } + + [Fact] + public async Task Enqueue_AfterDispose_DoesNothing() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await scheduler.DisposeAsync(); + + var processed = 0; + await scheduler.Enqueue( + Enumerable.Range(0, 10).ToArray(), + (_, _) => + { + Interlocked.Increment(ref processed); + return Task.CompletedTask; + }, + new Progress(), + CancellationToken.None); + + Assert.Equal(0, processed); + } + + [Theory] + [InlineData(1)] + [InlineData(4)] + public async Task Enqueue_FromWithinAWorker_DoesNotDeadlock(int fanout) + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping, fanout: fanout); + await using (scheduler) + { + var inner = 0; + + var outer = scheduler.Enqueue( + Enumerable.Range(0, 8).ToArray(), + (_, _) => scheduler.Enqueue( + Enumerable.Range(0, 4).ToArray(), + (_, _) => + { + Interlocked.Increment(ref inner); + return Task.CompletedTask; + }, + new Progress(), + CancellationToken.None), + new Progress(), + CancellationToken.None); + + await outer.WaitAsync(_timeout, TestContext.Current.CancellationToken); + + Assert.Equal(32, inner); + } + } + + private static LimitedConcurrencyLibraryScheduler CreateScheduler( + CancellationTokenSource appStopping, + int fanout = 4, + TimeSpan? gracePeriod = null) + { + var lifetime = new Mock(); + lifetime.SetupGet(x => x.ApplicationStopping).Returns(() => appStopping.Token); + + var configurationManager = new Mock(); + configurationManager.SetupGet(x => x.Configuration) + .Returns(new ServerConfiguration { LibraryScanFanoutConcurrency = fanout }); + + return new LimitedConcurrencyLibraryScheduler( + lifetime.Object, + NullLogger.Instance, + configurationManager.Object, + gracePeriod ?? _shortGracePeriod); + } + + private static Task RunOneOperation(LimitedConcurrencyLibraryScheduler scheduler) + => scheduler.Enqueue( + Enumerable.Range(0, 8).ToArray(), + (_, _) => Task.CompletedTask, + new Progress(), + CancellationToken.None); + + private static async Task WaitForAsync(Func condition) + { + var stopwatch = Stopwatch.StartNew(); + while (!condition()) + { + Assert.True(stopwatch.Elapsed < _timeout, "timed out waiting for the scheduler to settle"); + await Task.Delay(20, TestContext.Current.CancellationToken); + } + } + } +} -- cgit v1.2.3 From 46dd7d8e99ff7167d2d175878896294ed823927f Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Fri, 4 Sep 2026 19:28:59 +0200 Subject: Invalidate the singleton DirectoryService cache on filesystem changes --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 4 +- .../Library/LibraryManager.cs | 3 +- .../Controllers/LibraryStructureController.cs | 3 +- .../MediaBrowser.Controller.csproj | 1 - .../Providers/DirectoryService.cs | 52 +++++---------- MediaBrowser.Providers/Lyric/LyricManager.cs | 4 +- .../Subtitles/SubtitleManager.cs | 4 +- .../DirectoryServiceTests.cs | 73 ---------------------- 8 files changed, 25 insertions(+), 119 deletions(-) (limited to 'MediaBrowser.Controller') 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 @@ - 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 _caches = []; + // TODO make static and switch to FastConcurrentLru. + private readonly ConcurrentDictionary _cache = new(StringComparer.Ordinal); + + private readonly ConcurrentDictionary _fileCache = new(StringComparer.Ordinal); + + private readonly ConcurrentDictionary> _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 Entries { get; } - = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime); - - public ConcurrentTLru Files { get; } - = new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal, _entryLifetime); - - public ConcurrentTLru> 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; @@ -268,62 +267,6 @@ namespace Jellyfin.Controller.Tests fileSystemMock.Verify(f => f.GetFileSystemEntries(_lowerCasePath), Times.Once); } - [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(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny())) - .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(); - 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(); - firstFileSystem.Setup(f => f.GetFileSystemEntries(_lowerCasePath)) - .Returns(_lowerCaseFileSystemMetadata); - var secondFileSystem = new Mock(); - 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() { @@ -363,22 +306,6 @@ namespace Jellyfin.Controller.Tests Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(_lowerCasePath)); } - [Fact] - public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem() - { - var fileSystemMock = new Mock(); - 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() { -- cgit v1.2.3 From e5cd3381acefbe13aa36a26059c28629e23e1944 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sat, 5 Sep 2026 07:08:14 +0200 Subject: Limit cache size again Co-Authored-By: Cody Robibero --- .../Providers/DirectoryService.cs | 91 ++++++++++++++++++---- .../DirectoryServiceTests.cs | 73 +++++++++++++++++ .../LimitedConcurrencyLibrarySchedulerTests.cs | 3 + 3 files changed, 152 insertions(+), 15 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 684e247f86..38872d3cbe 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/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 _cache = new(StringComparer.Ordinal); private readonly ConcurrentDictionary _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 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 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 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)); + } } } } diff --git a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs index 7c275b78cc..e57fbfe473 100644 --- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs +++ b/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(); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny())) + .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(); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny())) + .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() { diff --git a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs index 21a719a78f..f776e893a0 100644 --- a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs +++ b/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. /// + /// A representing the asynchronous unit test. [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. /// + /// A representing the asynchronous unit test. [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. /// + /// A representing the asynchronous unit test. [Fact] public async Task DisposeAsync_DoesNotWaitOutTheGracePeriod() { -- cgit v1.2.3 From f8470630be0f3fa7b8052ebd822f23531d30da2f Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sat, 5 Sep 2026 09:58:04 +0200 Subject: Retire runners that are cancelled before they start --- .../LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs | 4 +++- .../LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs index 3c6ffe3cbd..be75117b6f 100644 --- a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs +++ b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs @@ -191,12 +191,14 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr // Keyed on its own stop source, because cancelling that is what reaches the linked // source the runner waits on. Cancellation does not travel the other way. + // Started without the runner's own token: a task cancelled before it is scheduled + // never runs its body, so it would never take itself out of _taskRunners again. _taskRunners.Add( stopToken, Task.Factory.StartNew( ItemWorker, (stopToken, combinedSource), - combinedSource.Token, + CancellationToken.None, TaskCreationOptions.PreferFairness, TaskScheduler.Default)); } diff --git a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs index f776e893a0..686d839f4f 100644 --- a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs +++ b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs @@ -65,7 +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. /// - /// A representing the asynchronous unit test. + /// A representing the asynchronous unit test. [Fact] public async Task ApplicationStopping_RetiresRunners() { @@ -88,7 +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. /// - /// A representing the asynchronous unit test. + /// A representing the asynchronous unit test. [Fact] public async Task Enqueue_RetiresIdleRunnersAfterEveryOperation() { @@ -110,7 +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. /// - /// A representing the asynchronous unit test. + /// A representing the asynchronous unit test. [Fact] public async Task DisposeAsync_DoesNotWaitOutTheGracePeriod() { -- cgit v1.2.3 From 344a6dcd2c39c6a0a1f34b888f260e297781a2c5 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sun, 6 Sep 2026 08:38:43 +0200 Subject: Apply review suggestions --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 2 -- Emby.Server.Implementations/Library/LibraryManager.cs | 1 - Jellyfin.Api/Controllers/LibraryStructureController.cs | 8 ++------ MediaBrowser.Controller/Providers/DirectoryService.cs | 18 +++++++++++++----- MediaBrowser.Controller/Providers/IDirectoryService.cs | 7 +++++++ MediaBrowser.Providers/Lyric/LyricManager.cs | 2 -- MediaBrowser.Providers/Subtitles/SubtitleManager.cs | 2 -- 7 files changed, 22 insertions(+), 18 deletions(-) (limited to 'MediaBrowser.Controller') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index d5735aed27..0f92e2f03e 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -368,8 +368,6 @@ namespace Emby.Server.Implementations.IO return; } - // 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 80e89b4305..dc76e1183e 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -3723,7 +3723,6 @@ namespace Emby.Server.Implementations.Library } } - // 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 e4833c77dd..65bfe25d21 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -183,15 +183,11 @@ public class LibraryStructureController : BaseJellyfinApiController var tempPath = Path.Combine( rootFolderPath, Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture)); - Directory.Move(currentPath, tempPath); + _directoryService.Move(currentPath, tempPath); currentPath = tempPath; } - Directory.Move(currentPath, newPath); - - // The injected service is a singleton, so its listings of both paths are now stale. - _directoryService.Invalidate(currentPath); - _directoryService.Invalidate(newPath); + _directoryService.Move(currentPath, newPath); } finally { diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 38872d3cbe..f8e0bf4ed9 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -170,6 +170,14 @@ namespace MediaBrowser.Controller.Providers } } + public void Move(string source, string destination) + { + Directory.Move(source, destination); + + Invalidate(source); + Invalidate(destination); + } + public bool IsAccessible(string path) { return _fileSystem.GetFileSystemEntryPaths(path).Any(); @@ -178,21 +186,21 @@ namespace MediaBrowser.Controller.Providers private void DropCacheIfIdleOrFull() { var nowMs = Environment.TickCount64; - var idleMs = nowMs - Volatile.Read(ref _lastAccess); + var idleMs = nowMs - _lastAccess; - if (idleMs >= IdleTimeoutMs || Volatile.Read(ref _recordCount) >= MaxCachedRecords) + if (idleMs >= IdleTimeoutMs || _recordCount >= MaxCachedRecords) { _cache.Clear(); _fileCache.Clear(); _filePathCache.Clear(); - Volatile.Write(ref _recordCount, 0); - Volatile.Write(ref _lastAccess, nowMs); + _recordCount = 0; + _lastAccess = nowMs; return; } if (idleMs >= AccessIntervalMs) { - Volatile.Write(ref _lastAccess, nowMs); + _lastAccess = nowMs; } } diff --git a/MediaBrowser.Controller/Providers/IDirectoryService.cs b/MediaBrowser.Controller/Providers/IDirectoryService.cs index 609d094254..3a943d5f0c 100644 --- a/MediaBrowser.Controller/Providers/IDirectoryService.cs +++ b/MediaBrowser.Controller/Providers/IDirectoryService.cs @@ -29,6 +29,13 @@ namespace MediaBrowser.Controller.Providers /// The file or directory path that changed. void Invalidate(string path); + /// + /// Moves a directory and forgets what is cached about both paths. + /// + /// The directory to move. + /// The path to move the directory to. + void Move(string source, string destination); + bool IsAccessible(string path); } } diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs index dfa7bfde2f..a19262c3a7 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -255,7 +255,6 @@ public class LyricManager : ILyricManager _libraryMonitor.ReportFileSystemChangeComplete(path, false); } - // The injected service is a singleton, so its listing would keep the deleted file. _directoryService.Invalidate(path); } @@ -453,7 +452,6 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } - // 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 aa363c425f..cd9dda21a0 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -284,7 +284,6 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The injected service is a singleton, so its listing of the folder is now stale. _directoryService.Invalidate(path); return; @@ -401,7 +400,6 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } - // The injected service is a singleton, so its listing would keep the deleted file. _directoryService.Invalidate(path); return item.RefreshMetadata(CancellationToken.None); -- cgit v1.2.3