From c6ba29abaf373b413f338a680d546ee94ba6cb2f Mon Sep 17 00:00:00 2001 From: jordan Date: Tue, 29 Sep 2026 22:35:43 +0000 Subject: [PATCH 1/2] perf(extras): batch the post-scan metadata-file cleanup and probe the right base path first CleanExtraFileService.Clean ran after every author scan and, for each MetadataFiles row, probed the author's base paths in a fixed audiobook -> ebook -> legacy order and deleted missing rows one by one. DiskProviderBase.FileExists falls back to listing directories when the exact path misses, so an ebook sidecar always paid for a miss under the audiobook folder before its hit, and every missing row cost its own DELETE. - Probe the base path the row's book file lives under first. Sidecars are written under ExtraFilePathHelper.GetPreferredBasePath(author, bookFile), so that is where they are. Rows with no book file keep the existing order, and every base path is still tried before a row counts as missing. New ExtraFilePathHelper.GetAuthorBasePaths(author, preferredBasePath) overload; one IMediaFileService.GetFilesByAuthor query per author (optional constructor dependency, no effect when absent). - Delete all missing rows with one DeleteMany instead of one Delete per row. - Skip rows with a blank RelativePath instead of throwing from Path.Combine. - Return early for an author with no metadata files. Tests: one batched delete with the exact ids; nothing deleted when all files exist; a linked ebook sidecar is found on the first probe and the audiobook folder is never touched; a file that exists only under the other base path is kept; default order for rows without a book file; blank paths ignored; no work for an author with no rows; works without the media file service. The batching and preferred-base tests fail if either change is reverted. Refs #271 --- .../Extras/CleanExtraFileServiceFixture.cs | 204 ++++++++++++++++++ .../Extras/ExtraFilePathHelper.cs | 11 + .../Files/CleanMetadataFileService.cs | 54 ++++- 3 files changed, 265 insertions(+), 4 deletions(-) create mode 100644 src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs diff --git a/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs b/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs new file mode 100644 index 00000000..9bcbba19 --- /dev/null +++ b/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs @@ -0,0 +1,204 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using NLog; +using NUnit.Framework; +using NzbDrone.Common.Disk; +using NzbDrone.Core.Books; +using NzbDrone.Core.Extras.Metadata.Files; +using NzbDrone.Core.MediaFiles; + +namespace Chaptarr.Core.Test.Extras +{ + [TestFixture] + public class CleanExtraFileServiceFixture + { + private const string AudioBase = "/mnt/Media/Audio Books/Some Author"; + private const string EbookBase = "/mnt/Media/Books/Some Author"; + + private class DiskProxy : DispatchProxy + { + public HashSet ExistingFiles { get; } = new HashSet(); + public List Probes { get; } = new List(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (targetMethod?.Name == nameof(IDiskProvider.FileExists) && args?.Length == 1 && args[0] is string path) + { + Probes.Add(path); + return ExistingFiles.Contains(path); + } + + throw new NotImplementedException($"Test proxy does not implement IDiskProvider.{targetMethod?.Name}"); + } + } + + private class MetadataFileServiceProxy : DispatchProxy + { + public List Files { get; set; } = new List(); + public List> DeleteManyCalls { get; } = new List>(); + public List DeleteSingleCalls { get; } = new List(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + switch (targetMethod?.Name) + { + case nameof(IMetadataFileService.GetFilesByAuthor): + return Files; + case nameof(IMetadataFileService.DeleteMany): + DeleteManyCalls.Add(((IEnumerable)args[0]).ToList()); + return null; + case nameof(IMetadataFileService.Delete): + DeleteSingleCalls.Add((int)args[0]); + return null; + } + + throw new NotImplementedException($"Test proxy does not implement IMetadataFileService.{targetMethod?.Name}"); + } + } + + private class MediaFileServiceProxy : DispatchProxy + { + public List Files { get; set; } = new List(); + public int GetFilesByAuthorCalls { get; private set; } + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (targetMethod?.Name == nameof(IMediaFileService.GetFilesByAuthor) && args?.Length == 1 && args[0] is int) + { + GetFilesByAuthorCalls++; + return Files; + } + + throw new NotImplementedException($"Test proxy does not implement IMediaFileService.{targetMethod?.Name}"); + } + } + + private DiskProxy _disk; + private MetadataFileServiceProxy _metadata; + private MediaFileServiceProxy _media; + private CleanExtraFileService _sut; + private Author _author; + + [SetUp] + public void SetUp() + { + var disk = DispatchProxy.Create(); + var metadata = DispatchProxy.Create(); + var media = DispatchProxy.Create(); + _disk = (DiskProxy)(object)disk; + _metadata = (MetadataFileServiceProxy)(object)metadata; + _media = (MediaFileServiceProxy)(object)media; + _sut = new CleanExtraFileService(metadata, disk, LogManager.GetCurrentClassLogger(), media); + _author = new Author { Id = 1, Name = "Some Author", AudiobookPath = AudioBase, EbookPath = EbookBase, Path = AudioBase }; + } + + private static MetadataFile Row(int id, string relative, int? bookFileId = null) => + new MetadataFile { Id = id, AuthorId = 1, RelativePath = relative, BookFileId = bookFileId }; + + [Test] + public void should_delete_every_missing_row_in_a_single_batch_not_one_delete_per_row() + { + _metadata.Files = new List { Row(1, "A/cover.jpg"), Row(2, "B/cover.jpg"), Row(3, "C/cover.jpg") }; + _disk.ExistingFiles.Add(EbookBase + "/B/cover.jpg"); + + _sut.Clean(_author); + + Assert.That(_metadata.DeleteManyCalls, Has.Count.EqualTo(1), "one batched delete"); + Assert.That(_metadata.DeleteManyCalls[0], Is.EquivalentTo(new[] { 1, 3 })); + Assert.That(_metadata.DeleteSingleCalls, Is.Empty, "no per-row deletes"); + } + + [Test] + public void should_not_delete_anything_when_every_file_exists() + { + _metadata.Files = new List { Row(1, "A/cover.jpg"), Row(2, "B/cover.jpg") }; + _disk.ExistingFiles.Add(AudioBase + "/A/cover.jpg"); + _disk.ExistingFiles.Add(EbookBase + "/B/cover.jpg"); + + _sut.Clean(_author); + + Assert.That(_metadata.DeleteManyCalls, Is.Empty); + Assert.That(_metadata.DeleteSingleCalls, Is.Empty); + } + + [Test] + public void should_probe_the_base_path_the_linked_book_file_lives_under_first() + { + // An ebook sidecar: the ebook folder is the right place, the audiobook folder would be a guaranteed miss. + _media.Files = new List { new BookFile { Id = 50, Path = EbookBase + "/Book/Book.epub", MediaType = "ebook" } }; + _metadata.Files = new List { Row(1, "Book/cover.jpg", 50) }; + _disk.ExistingFiles.Add(EbookBase + "/Book/cover.jpg"); + + _sut.Clean(_author); + + Assert.That(_disk.Probes, Is.EqualTo(new[] { EbookBase + "/Book/cover.jpg" }), "found on the first probe, the audiobook folder is never touched"); + Assert.That(_metadata.DeleteManyCalls, Is.Empty); + } + + [Test] + public void should_still_find_a_file_under_another_base_path_when_the_preferred_one_misses() + { + _media.Files = new List { new BookFile { Id = 50, Path = EbookBase + "/Book/Book.epub", MediaType = "ebook" } }; + _metadata.Files = new List { Row(1, "Book/cover.jpg", 50) }; + _disk.ExistingFiles.Add(AudioBase + "/Book/cover.jpg"); // exists only under the other format's folder + + _sut.Clean(_author); + + Assert.That(_metadata.DeleteManyCalls, Is.Empty, "a file that exists somewhere must be kept"); + Assert.That(_disk.Probes.First(), Is.EqualTo(EbookBase + "/Book/cover.jpg"), "preferred base is tried first"); + Assert.That(_disk.Probes, Does.Contain(AudioBase + "/Book/cover.jpg")); + } + + [Test] + public void should_keep_the_default_probe_order_for_rows_that_have_no_book_file() + { + _metadata.Files = new List { Row(1, "poster.jpg") }; + _disk.ExistingFiles.Add(EbookBase + "/poster.jpg"); + + _sut.Clean(_author); + + Assert.That(_disk.Probes, Is.EqualTo(new[] { AudioBase + "/poster.jpg", EbookBase + "/poster.jpg" }), "audiobook, then ebook (legacy path equals audiobook and is de-duplicated)"); + Assert.That(_metadata.DeleteManyCalls, Is.Empty); + } + + [Test] + public void should_ignore_rows_without_a_relative_path_instead_of_throwing() + { + _metadata.Files = new List { Row(1, null), Row(2, " "), Row(3, "gone.jpg") }; + + Assert.DoesNotThrow(() => _sut.Clean(_author)); + + Assert.That(_metadata.DeleteManyCalls.Single(), Is.EqualTo(new[] { 3 }), "only the row that really is missing is deleted"); + } + + [Test] + public void should_do_no_disk_or_media_file_work_for_an_author_with_no_metadata_files() + { + _metadata.Files = new List(); + + _sut.Clean(_author); + + Assert.That(_disk.Probes, Is.Empty); + Assert.That(_media.GetFilesByAuthorCalls, Is.EqualTo(0)); + Assert.That(_metadata.DeleteManyCalls, Is.Empty); + } + + [Test] + public void should_work_without_a_media_file_service_using_the_default_probe_order() + { + var disk = DispatchProxy.Create(); + var metadata = DispatchProxy.Create(); + var diskProxy = (DiskProxy)(object)disk; + var metadataProxy = (MetadataFileServiceProxy)(object)metadata; + metadataProxy.Files = new List { Row(1, "Book/cover.jpg", 50), Row(2, "gone.jpg") }; + diskProxy.ExistingFiles.Add(EbookBase + "/Book/cover.jpg"); + var sut = new CleanExtraFileService(metadata, disk, LogManager.GetCurrentClassLogger()); + + Assert.DoesNotThrow(() => sut.Clean(_author)); + + Assert.That(metadataProxy.DeleteManyCalls.Single(), Is.EqualTo(new[] { 2 }), "the existing file is kept, the missing one is deleted"); + } + } +} diff --git a/src/NzbDrone.Core/Extras/ExtraFilePathHelper.cs b/src/NzbDrone.Core/Extras/ExtraFilePathHelper.cs index 43b08d0d..4393e925 100644 --- a/src/NzbDrone.Core/Extras/ExtraFilePathHelper.cs +++ b/src/NzbDrone.Core/Extras/ExtraFilePathHelper.cs @@ -26,6 +26,17 @@ public static List GetAuthorBasePaths(Author author) return paths; } + // The author's base paths with `preferredBasePath` (if any) probed first. Extras are written under + // GetPreferredBasePath(author, bookFile), so looking there first finds an existing file in one + // probe instead of missing under the other formats' folders first. + public static List GetAuthorBasePaths(Author author, string preferredBasePath) + { + var paths = GetAuthorBasePaths(author); + PreferBase(paths, preferredBasePath); + + return paths; + } + public static string GetPreferredBasePath(Author author, BookFile bookFile) { if (author == null) diff --git a/src/NzbDrone.Core/Extras/Metadata/Files/CleanMetadataFileService.cs b/src/NzbDrone.Core/Extras/Metadata/Files/CleanMetadataFileService.cs index c4a7818d..d0d7ce78 100644 --- a/src/NzbDrone.Core/Extras/Metadata/Files/CleanMetadataFileService.cs +++ b/src/NzbDrone.Core/Extras/Metadata/Files/CleanMetadataFileService.cs @@ -1,9 +1,11 @@ +using System.Collections.Generic; using System.IO; using System.Linq; using NLog; using NzbDrone.Common.Disk; +using NzbDrone.Common.Extensions; using NzbDrone.Core.Books; -using NzbDrone.Core.Extras; +using NzbDrone.Core.MediaFiles; namespace NzbDrone.Core.Extras.Metadata.Files { @@ -17,14 +19,17 @@ public class CleanExtraFileService : ICleanMetadataService private readonly IMetadataFileService _metadataFileService; private readonly IDiskProvider _diskProvider; private readonly Logger _logger; + private readonly IMediaFileService _mediaFileService; public CleanExtraFileService(IMetadataFileService metadataFileService, IDiskProvider diskProvider, - Logger logger) + Logger logger, + IMediaFileService mediaFileService = null) { _metadataFileService = metadataFileService; _diskProvider = diskProvider; _logger = logger; + _mediaFileService = mediaFileService; } public void Clean(Author author) @@ -32,19 +37,60 @@ public void Clean(Author author) _logger.Debug("Cleaning missing metadata files for author: {0}", author.Name); var metadataFiles = _metadataFileService.GetFilesByAuthor(author.Id); + if (metadataFiles.Count == 0) + { + return; + } + + // Sidecar files are written under ExtraFilePathHelper.GetPreferredBasePath(author, bookFile), so + // for rows that point at a book file, probe that base path first. Probing a fixed audiobook -> + // ebook -> legacy order made every ebook sidecar miss under the audiobook folder first, and a + // miss is the expensive case (FileExists falls back to listing directories). + var bookFilesById = _mediaFileService == null + ? new Dictionary() + : (_mediaFileService.GetFilesByAuthor(author.Id) ?? new List()) + .GroupBy(file => file.Id) + .ToDictionary(group => group.Key, group => group.First()); + + var basePathsByPreferred = new Dictionary>(); + var missingIds = new List(); foreach (var metadataFile in metadataFiles) { - var exists = ExtraFilePathHelper.GetAuthorBasePaths(author) + if (metadataFile.RelativePath.IsNullOrWhiteSpace()) + { + continue; + } + + string preferredBasePath = null; + if (metadataFile.BookFileId.HasValue && bookFilesById.TryGetValue(metadataFile.BookFileId.Value, out var bookFile)) + { + preferredBasePath = ExtraFilePathHelper.GetPreferredBasePath(author, bookFile); + } + + var cacheKey = preferredBasePath ?? string.Empty; + if (!basePathsByPreferred.TryGetValue(cacheKey, out var basePaths)) + { + basePaths = ExtraFilePathHelper.GetAuthorBasePaths(author, preferredBasePath); + basePathsByPreferred[cacheKey] = basePaths; + } + + var exists = basePaths .Select(p => Path.Combine(p, metadataFile.RelativePath)) .Any(_diskProvider.FileExists); if (!exists) { _logger.Debug("Deleting metadata file from database: {0}", metadataFile.RelativePath); - _metadataFileService.Delete(metadataFile.Id); + missingIds.Add(metadataFile.Id); } } + + // One statement for all missing rows instead of one DELETE per row. + if (missingIds.Count > 0) + { + _metadataFileService.DeleteMany(missingIds); + } } } } From e9eb2b39d695aa61dfae613c6f14fd5858b3b9aa Mon Sep 17 00:00:00 2001 From: jordan Date: Tue, 29 Sep 2026 22:37:19 +0000 Subject: [PATCH 2/2] Test the fallback for a metadata row whose linked book file no longer exists Adversarial review of #273: a BookFileId that is not among the author's book files (deleted, or never mapped) must fall back to the default probe order and still keep a file that exists under another base path. --- .../Extras/CleanExtraFileServiceFixture.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs b/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs index 9bcbba19..cb9c84d7 100644 --- a/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs +++ b/src/Chaptarr.Core.Test/Extras/CleanExtraFileServiceFixture.cs @@ -163,6 +163,20 @@ public void should_keep_the_default_probe_order_for_rows_that_have_no_book_file( Assert.That(_metadata.DeleteManyCalls, Is.Empty); } + [Test] + public void should_fall_back_to_the_default_order_when_the_linked_book_file_no_longer_exists() + { + // BookFileId 99 is not among the author's book files (deleted, or never mapped). + _media.Files = new List { new BookFile { Id = 50, Path = EbookBase + "/Other/Other.epub", MediaType = "ebook" } }; + _metadata.Files = new List { Row(1, "Book/cover.jpg", 99) }; + _disk.ExistingFiles.Add(EbookBase + "/Book/cover.jpg"); + + _sut.Clean(_author); + + Assert.That(_disk.Probes, Is.EqualTo(new[] { AudioBase + "/Book/cover.jpg", EbookBase + "/Book/cover.jpg" }), "default audiobook -> ebook order"); + Assert.That(_metadata.DeleteManyCalls, Is.Empty, "the file exists under the ebook folder, so the row is kept"); + } + [Test] public void should_ignore_rows_without_a_relative_path_instead_of_throwing() {