From fd74ddb0d077a92f5cd738cd113009815ebef296 Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 22:18:30 +0000 Subject: [PATCH 1/9] Fix two bugs that left "delete files" author deletes with unmapped/orphaned files Bug A: BookService.DeleteMany published each book's BookDeletedEvent with deleteFiles hardcoded to false, regardless of what the author-level delete actually requested. Since this is the method BookService.Handle(AuthorDeletedEvent) always routes through, MediaFileService never deleted a single BookFile row for any author delete - it only ever unlinked them (EditionId = 0) - leaving thousands of stale rows that the Unmapped Files UI then surfaced as if they were real importable content. Thread the author's DeleteFiles flag through instead. Kept as an overload (DeleteMany(books, deleteFiles)) rather than adding a parameter to the existing DeleteMany(books) so the ~20 hand-written IBookService test stubs across the test suite that only implement the 1-arg form keep compiling unchanged. Bug B: MediaFileDeletionService.HandleAsync(AuthorDeletedEvent) only ever deleted the legacy single Author.Path folder. An author with separate audiobook/ebook root folders (AudiobookPath/EbookPath) would have one format's folder deleted and the other left completely untouched on disk - confirmed in production: an author's ebook folder (166 files) survived intact while its audiobook folder was correctly removed. Now iterates the distinct set of {Path, AudiobookPath, EbookPath} and applies the same safety checks (configured-root-folder refusal, parent/duplicate-of-another-author refusal) to each one individually. The AllAuthorPaths() lookup used by that check is now fetched lazily (once, only if some path actually needs it) rather than unconditionally up front, so an author whose only path is already refused as unsafe still never touches it - preserving an existing test's expectation that this doesn't happen when it's not needed. Also: DeleteCompletedEvent (which flushes Plex's pending-refresh queue) is now published exactly once per author delete unconditionally, rather than only in some of the refusal paths as before - it's a no-op against an empty queue, so this closes a case where that queue could previously go unflushed. Adds regression coverage for both: a MediaFileDeletionServiceFixture test asserting both audiobook and ebook folders get deleted when they differ, and a new BookServiceAuthorDeletedFixture asserting the DeleteFiles flag reaches BookDeletedEvent in both directions (true and false). --- .../Books/BookServiceAuthorDeletedFixture.cs | 136 ++++++++++++++++++ .../MediaFileDeletionServiceFixture.cs | 77 ++++++++++ .../Books/Services/BookService.cs | 19 ++- .../MediaFiles/MediaFileDeletionService.cs | 73 +++++++--- 4 files changed, 281 insertions(+), 24 deletions(-) create mode 100644 src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs diff --git a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs new file mode 100644 index 00000000..e65669a1 --- /dev/null +++ b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs @@ -0,0 +1,136 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using NLog; +using NUnit.Framework; +using NzbDrone.Common.Messaging; +using NzbDrone.Core.Books; +using NzbDrone.Core.Books.Events; +using NzbDrone.Core.Messaging.Events; + +namespace Chaptarr.Core.Test.Books +{ + // BookService.Handle(AuthorDeletedEvent) used to always publish BookDeletedEvent with + // deleteFiles hardcoded to false, regardless of what the author-level delete actually + // requested - so MediaFileService never deleted BookFile rows for an author's books, only + // unlinked them, leaving them behind as "unmapped files" even when the physical files really + // were deleted (see MediaFileDeletionServiceFixture's dual-path test for the other half of + // this bug). + [TestFixture] + public class BookServiceAuthorDeletedFixture + { + private sealed class RecordingEventAggregator : IEventAggregator + { + public List Events { get; } = new(); + + public void PublishEvent(TEvent @event) + where TEvent : class, IEvent + { + Events.Add(@event); + } + } + + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private class EmptyLinksSeriesBookLinkRepositoryProxy : DispatchProxy + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(ISeriesBookLinkRepository.GetLinksByBook), StringComparison.Ordinal)) + { + return new List(); + } + + throw new NotImplementedException($"Test proxy does not implement ISeriesBookLinkRepository.{targetMethod?.Name}"); + } + } + + private class RecordingBookRepositoryProxy : DispatchProxy + { + public List BooksByAuthor { get; set; } = new(); + public List DeletedBooks { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IBookRepository.GetBooksByAuthorId), StringComparison.Ordinal)) + { + return BooksByAuthor; + } + + if (string.Equals(targetMethod?.Name, "DeleteMany", StringComparison.Ordinal) && + args?.Length == 1 && + args[0] is IEnumerable books) + { + DeletedBooks.AddRange(books); + return null; + } + + throw new NotImplementedException($"Test proxy does not implement IBookRepository.{targetMethod?.Name}"); + } + } + + [Test] + public void should_pass_the_authors_delete_files_flag_through_to_each_books_delete_event() + { + var eventAggregator = new RecordingEventAggregator(); + var bookRepository = DispatchProxy.Create(); + var bookRepoProxy = (RecordingBookRepositoryProxy)(object)bookRepository; + + var author = new Author { Id = 7, Name = "Jim Butcher" }; + var book = new Book { Id = 42, AuthorId = author.Id, Author = author, Title = "Storm Front" }; + bookRepoProxy.BooksByAuthor = new List { book }; + + var service = new BookService( + bookRepository, + null, // editionService + eventAggregator, + null, // authorService + null, // mediaFileService + null, // rootFolderService + DispatchProxy.Create(), + null, // multiCopySeriesService + LogManager.GetCurrentClassLogger()); + + service.Handle(new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false)); + + var published = eventAggregator.Events.OfType().Single(); + Assert.That(published.DeleteFiles, Is.True); + Assert.That(bookRepoProxy.DeletedBooks.Select(b => b.Id), Does.Contain(book.Id)); + } + + [Test] + public void should_not_delete_files_when_the_author_delete_did_not_request_it() + { + var eventAggregator = new RecordingEventAggregator(); + var bookRepository = DispatchProxy.Create(); + var bookRepoProxy = (RecordingBookRepositoryProxy)(object)bookRepository; + + var author = new Author { Id = 7, Name = "Jim Butcher" }; + var book = new Book { Id = 42, AuthorId = author.Id, Author = author, Title = "Storm Front" }; + bookRepoProxy.BooksByAuthor = new List { book }; + + var service = new BookService( + bookRepository, + null, // editionService + eventAggregator, + null, // authorService + null, // mediaFileService + null, // rootFolderService + DispatchProxy.Create(), + null, // multiCopySeriesService + LogManager.GetCurrentClassLogger()); + + service.Handle(new AuthorDeletedEvent(author, deleteFiles: false, addImportListExclusion: false)); + + var published = eventAggregator.Events.OfType().Single(); + Assert.That(published.DeleteFiles, Is.False); + } + } +} diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs index 87dd41f5..581c6e06 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs @@ -90,6 +90,38 @@ public RootFolder GetBestRootFolder(string path, List allRootFolders public string GetBestRootFolderPath(string path, List allRootFolders) => throw new NotImplementedException(); } + private class FolderExistsOnlyDiskProviderProxy : DispatchProxy + { + public HashSet ExistingFolders { get; } = new(PathEqualityComparer.Instance); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IDiskProvider.FolderExists), StringComparison.Ordinal) && + args?.Length == 1 && + args[0] is string folderPath) + { + return ExistingFolders.Contains(folderPath); + } + + throw new NotImplementedException($"Test proxy does not implement IDiskProvider.{targetMethod?.Name}"); + } + } + + private class AllAuthorPathsOnlyAuthorServiceProxy : DispatchProxy + { + public Dictionary Paths { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IAuthorService.AllAuthorPaths), StringComparison.Ordinal)) + { + return Paths; + } + + throw new NotImplementedException($"Test proxy does not implement IAuthorService.{targetMethod?.Name}"); + } + } + private class ThrowingProxy : DispatchProxy where T : class { protected override object Invoke(MethodInfo targetMethod, object[] args) @@ -531,6 +563,51 @@ public void should_not_throw_and_should_refuse_deleting_parent_of_root_folder_on Assert.That(eventAggregator.Events.OfType(), Is.Not.Empty); } + [Test] + public void should_delete_both_audiobook_and_ebook_folders_on_author_delete_when_they_differ() + { + // An author's legacy Path only ever pointed at one of AudiobookPath/EbookPath. Deleting + // just Path silently left the other format's entire folder - and every file in it - + // behind on disk, with no error, despite "delete files" having been requested. + var recycleBinProvider = new RecordingRecycleBinProvider(); + var eventAggregator = new RecordingEventAggregator(); + var diskProvider = DispatchProxy.Create(); + var diskProxy = (FolderExistsOnlyDiskProviderProxy)(object)diskProvider; + + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/audiobooks/Jim Butcher", + AudiobookPath = "/audiobooks/Jim Butcher", + EbookPath = "/ebooks/Jim Butcher" + }; + + diskProxy.ExistingFolders.Add(author.AudiobookPath); + diskProxy.ExistingFolders.Add(author.EbookPath); + + var rootFolderService = new StubRootFolderService( + new RootFolder { Path = "/audiobooks", FolderType = FolderType.Audiobook }, + new RootFolder { Path = "/ebooks", FolderType = FolderType.Ebook }); + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + DispatchProxy.Create>(), + DispatchProxy.Create(), + DispatchProxy.Create>(), + eventAggregator, + rootFolderService, + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + service.HandleAsync(new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.AudiobookPath)); + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.EbookPath)); + Assert.That(eventAggregator.Events.OfType(), Is.Not.Empty); + } + [Test] public void should_delete_managed_ebook_replica_files_on_bookfile_delete_event_even_on_upgrade() { diff --git a/src/NzbDrone.Core/Books/Services/BookService.cs b/src/NzbDrone.Core/Books/Services/BookService.cs index 39f75f8e..148600d5 100644 --- a/src/NzbDrone.Core/Books/Services/BookService.cs +++ b/src/NzbDrone.Core/Books/Services/BookService.cs @@ -70,6 +70,13 @@ void UpdateManyWithLifecycle(List books) void ReassignAuthor(List books, int authorId) { throw new NotImplementedException(); } void RefreshProviderAliases(Book book) { } void DeleteMany(List books); + // The default exists only for lightweight test doubles; BookService overrides it directly. + // Kept as an overload (not an added parameter on the line above) so the many hand-written + // IBookService test stubs that only implement the 1-arg form keep compiling unchanged. + void DeleteMany(List books, bool deleteFiles) + { + DeleteMany(books); + } void SetAddOptions(IEnumerable books); List GetAuthorBooksWithFiles(Author author); List GetBooksForDisplay(int? authorId = null, string mediaType = null); @@ -2007,6 +2014,11 @@ private static List GetPersistedBooksForAuthorReassignment(IEnumerable books) + { + DeleteMany(books, false); + } + + public void DeleteMany(List books, bool deleteFiles) { var booksToDelete = (books ?? new List()) .Where(book => book != null) @@ -2018,7 +2030,7 @@ public void DeleteMany(List books) foreach (var book in booksToDelete) { - _eventAggregator.PublishEvent(new BookDeletedEvent(book, false, false)); + _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false)); _providerAliasService?.DeleteAliases("Book", book.Id); } @@ -2200,7 +2212,10 @@ public void Handle(AuthorDeletedEvent message) { var books = GetBooksByAuthorId(message.Author.Id); - DeleteMany(books); + // Previously hardcoded to false here regardless of what the author-level delete actually + // requested, so MediaFileService always unlinked (never deleted) each book's BookFile rows - + // leaving them behind as "unmapped files" even when the physical files really were deleted. + DeleteMany(books, message.DeleteFiles); } public void Execute(BulkSyncFormatMonitoringCommand message) diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index 35adedf4..0bd07814 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -188,41 +188,70 @@ public void HandleAsync(AuthorDeletedEvent message) if (!isCalibre) { - if (IsPathUnsafeToDelete(author.Path)) + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) + // in addition to the legacy single Path field. Only ever deleting Path left the other + // format's entire folder - and every file in it - untouched on disk while the DB + // treated the author as fully deleted, leaving those files' BookFile rows to surface + // as "unmapped" even though they were never actually removed. + var pathsToDelete = new[] { author.Path, author.AudiobookPath, author.EbookPath } + .Where(p => !p.IsNullOrWhiteSpace()) + .Distinct() + .ToList(); + + // Fetched lazily, once, only if some path actually needs the other-authors check - + // an author whose only path(s) are already refused as unsafe should never need it. + Dictionary allAuthors = null; + + foreach (var path in pathsToDelete) { - _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", - author.Path, author.Name); - _eventAggregator.PublishEvent(new DeleteCompletedEvent()); - return; - } - - var allAuthors = _authorService.AllAuthorPaths(); - - foreach (var s in allAuthors) - { - if (s.Key == author.Id) + if (IsPathUnsafeToDelete(path)) { + _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", + path, author.Name); continue; } - if (author.Path.IsParentPath(s.Value)) + allAuthors ??= _authorService.AllAuthorPaths(); + + var blockedByOtherAuthor = false; + + foreach (var s in allAuthors) { - _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", author.Path); - return; + if (s.Key == author.Id) + { + continue; + } + + if (path.IsParentPath(s.Value)) + { + _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); + blockedByOtherAuthor = true; + break; + } + + if (path.PathEquals(s.Value)) + { + _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); + blockedByOtherAuthor = true; + break; + } } - if (author.Path.PathEquals(s.Value)) + if (blockedByOtherAuthor) { - _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", author.Path); - return; + continue; } - } - if (_diskProvider.FolderExists(message.Author.Path)) - { - _recycleBinProvider.DeleteFolder(message.Author.Path); + if (_diskProvider.FolderExists(path)) + { + _recycleBinProvider.DeleteFolder(path); + } } + // Always published once per author now, regardless of which (if any) path above was + // refused - the previous single-path version only published this in some of those + // cases, which could leave Plex's pending-refresh queue never flushed. ProcessQueue() + // is a no-op against an empty queue, so publishing it unconditionally here is safe. _eventAggregator.PublishEvent(new DeleteCompletedEvent()); } } From ff9f00176005a199e146dced28b07d06a212e85e Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 22:31:54 +0000 Subject: [PATCH 2/9] Address adversarial review: fix dual-format cross-author check, avoid duplicate disk work, isolate per-path failures Four problems found in review of #261: 1. The cross-author safety check (parent-of/same-as another author) only compared each of an author's paths against other authors' single legacy Path column, since AllAuthorPaths() never selected AudiobookPath/EbookPath. A dual-format author's AudiobookPath could collide with a different author's separately-configured EbookPath and go undetected - exactly the layout this PR's fix targets. Added AllAuthorMediaPaths() (repository + service, with the same test-double-safe default-interface-method pattern used elsewhere in both interfaces) flattening every author down to all of Path/AudiobookPath/EbookPath, and switched the check to use it. 2. Fixing DeleteFiles actually reaching BookDeletedEvent reactivated two previously-dormant per-book disk handlers - MediaFileDeletionService.HandleAsync(BookDeletedEvent) and ExtraFileService.Handle(BookDeletedEvent) - which now run concurrently with (and redundant to) this same PR's own whole-author-folder recycle-bin delete, for the exact scenario the PR targets (author delete + deleteFiles=true). Added BookDeletedEvent.SkipDiskCleanup: true when a book delete is published as part of an author-level cascade (BookService.DeleteMany's AuthorDeletedEvent path), false otherwise (standalone book/BookController deletes still get the per-file/per-extra disk cleanup they've always needed, since nothing else covers them). Both handlers now also check this flag before touching disk; DeleteFiles alone still controls whether MediaFileService deletes vs. unlinks the BookFile DB rows, since that's independent of who removes the physical files. 3. pathsToDelete deduped with plain .Distinct() instead of the case-insensitive .Distinct(StringComparer.OrdinalIgnoreCase) that the identical {Path, AudiobookPath, EbookPath} pattern already uses in AuthorLibraryService.DisambiguateGeneratedAuthorPaths - fixed to match, so two of the three fields pointing at the same folder in different case doesn't run the safety checks and DeleteFolder against it twice. 4. _recycleBinProvider.DeleteFolder wasn't wrapped per-path, so one path's failure (permissions, a momentarily-unavailable NFS mount) would abort the whole loop and skip the unconditional DeleteCompletedEvent at the end - leaving a partially-deleted author's Plex refresh queue unflushed, the same bug class this PR's DeleteCompletedEvent fix was meant to close. Wrapped in a try/catch that logs and continues to the next path. Adds regression coverage for the SkipDiskCleanup gate on both the publishing side (BookServiceAuthorDeletedFixture) and the consuming side (MediaFileDeletionServiceFixture, using a throwing IDiskProvider proxy to prove no disk work happens at all). --- .../Books/BookServiceAuthorDeletedFixture.cs | 4 ++ .../MediaFileDeletionServiceFixture.cs | 59 ++++++++++++++++++- .../Books/Events/BookDeletedEvent.cs | 12 +++- .../Books/Repositories/AuthorRepository.cs | 31 ++++++++++ .../Books/Services/AuthorService.cs | 10 ++++ .../Books/Services/BookService.cs | 12 +++- .../Extras/Files/ExtraFileService.cs | 5 +- .../MediaFiles/MediaFileDeletionService.cs | 26 ++++++-- 8 files changed, 147 insertions(+), 12 deletions(-) diff --git a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs index e65669a1..82246da4 100644 --- a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs +++ b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs @@ -103,6 +103,10 @@ public void should_pass_the_authors_delete_files_flag_through_to_each_books_dele var published = eventAggregator.Events.OfType().Single(); Assert.That(published.DeleteFiles, Is.True); Assert.That(bookRepoProxy.DeletedBooks.Select(b => b.Id), Does.Contain(book.Id)); + + // MediaFileDeletionService's own AuthorDeletedEvent handler already recursively deletes + // the author's whole folder(s) - a per-book disk delete here would just race it. + Assert.That(published.SkipDiskCleanup, Is.True); } [Test] diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs index 581c6e06..bd2574de 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs @@ -109,11 +109,11 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) private class AllAuthorPathsOnlyAuthorServiceProxy : DispatchProxy { - public Dictionary Paths { get; } = new(); + public List> Paths { get; } = new(); protected override object Invoke(MethodInfo targetMethod, object[] args) { - if (string.Equals(targetMethod?.Name, nameof(IAuthorService.AllAuthorPaths), StringComparison.Ordinal)) + if (string.Equals(targetMethod?.Name, nameof(IAuthorService.AllAuthorMediaPaths), StringComparison.Ordinal)) { return Paths; } @@ -417,6 +417,61 @@ public void should_delete_every_file_and_replica_and_remove_the_folder_on_whole_ Assert.That(diskProxy.DeletedFolders, Does.Contain(author.EbookPath)); } + [Test] + public void should_do_no_disk_work_on_book_delete_when_an_author_delete_already_covers_it() + { + // A book delete published as part of a larger author delete (SkipDiskCleanup) relies on + // MediaFileDeletionService's own AuthorDeletedEvent handler to recursively remove the + // whole author folder. Doing per-file work here too would race that and duplicate + // recycle-bin entries for the same files. + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/ebooks/Jim Butcher", + EbookPath = "/ebooks/Jim Butcher" + }; + + var bookFolder = "/ebooks/Jim Butcher/Captains Fury"; + + var diskProvider = DispatchProxy.Create>(); + var recycleBinProvider = new RecordingRecycleBinProvider(); + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + DispatchProxy.Create>(), + DispatchProxy.Create>(), + DispatchProxy.Create>(), + new RecordingEventAggregator(), + new StubRootFolderService(new RootFolder { Id = 1, Path = "/ebooks" }), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book + { + Id = 5792, + AuthorId = author.Id, + Author = author, + Title = "Captain's Fury", + MediaType = BookMediaType.Ebook, + BookFiles = new List + { + new() + { + Id = 1, + Path = bookFolder + "/Captains Fury.epub", + Author = author + } + } + }; + + Assert.DoesNotThrow(() => + service.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, skipDiskCleanup: true))); + + Assert.That(recycleBinProvider.DeletedFiles, Is.Empty); + } + [Test] public void should_clean_the_book_folder_from_its_parent_so_it_can_actually_be_removed() { diff --git a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs index 66cd5af5..d299dc83 100644 --- a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs +++ b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs @@ -12,7 +12,16 @@ public class BookDeletedEvent : IEvent public bool ApplyToBothFormats { get; private set; } public IReadOnlyList DeletedBooks { get; private set; } - public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null) + // Set when this book is being deleted as part of a larger author delete, whose own + // AuthorDeletedEvent handler (MediaFileDeletionService) already recursively removes the + // author's whole folder(s). Per-book/per-file disk handlers should skip their own disk work + // in that case - it's redundant at best and racy (duplicate recycle-bin entries, deleting + // files out from under each other) at worst - while DeleteFiles still controls whether + // DB-only cleanup (e.g. actually deleting BookFile rows instead of just unlinking them) + // happens, since that's independent of who removes the physical files. + public bool SkipDiskCleanup { get; private set; } + + public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null, bool skipDiskCleanup = false) { Book = book; DeleteFiles = deleteFiles; @@ -21,6 +30,7 @@ public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion DeletedBooks = (deletedBooks ?? Enumerable.Repeat(book, 1)) .Where(item => item != null) .ToList(); + SkipDiskCleanup = skipDiskCleanup; } } } diff --git a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs index 1d9d2613..813080ae 100644 --- a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs +++ b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs @@ -20,6 +20,11 @@ public interface IAuthorRepository : IBasicRepository Author FindByGoogleBooksId(string googleBooksAuthorId); List GetAuthorIdsByMetadataProfileId(int metadataProfileId); Dictionary AllAuthorPaths(); + // The default exists only for lightweight test doubles. Every production implementation must override it. + List> AllAuthorMediaPaths() + { + throw new NotSupportedException(); + } Dictionary> AllAuthorTags(); void UpdateLastSelectedMediaType(int authorId, string mediaType); @@ -110,6 +115,32 @@ public Dictionary AllAuthorPaths() } } + private sealed class AuthorPathRow + { + public int Id { get; set; } + public string Path { get; set; } + public string AudiobookPath { get; set; } + public string EbookPath { get; set; } + } + + // Unlike AllAuthorPaths (legacy single-path field, used by validators/health checks that + // predate dual-format authors), this flattens every author down to all of the paths they + // actually use - Path, AudiobookPath and EbookPath - one row per non-empty path, so a + // consumer can safely check a candidate path against every path any other author owns. + public List> AllAuthorMediaPaths() + { + using (var conn = _database.OpenConnection()) + { + var strSql = "SELECT \"Id\", \"Path\", \"AudiobookPath\", \"EbookPath\" FROM \"Authors\""; + return conn.Query(strSql) + .SelectMany(row => new[] { row.Path, row.AudiobookPath, row.EbookPath } + .Where(p => !string.IsNullOrWhiteSpace(p)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .Select(p => new KeyValuePair(row.Id, p))) + .ToList(); + } + } + public Dictionary> AllAuthorTags() { using (var conn = _database.OpenConnection()) diff --git a/src/NzbDrone.Core/Books/Services/AuthorService.cs b/src/NzbDrone.Core/Books/Services/AuthorService.cs index 82443e91..4de8fa11 100644 --- a/src/NzbDrone.Core/Books/Services/AuthorService.cs +++ b/src/NzbDrone.Core/Books/Services/AuthorService.cs @@ -57,6 +57,11 @@ Author UpdateAuthorProgressiveSettings(Author author, int? audiobookQualityProfi } List UpdateAuthors(List authors, bool useExistingRelativeFolder); Dictionary AllAuthorPaths(); + // The default exists only for lightweight test doubles. Every production implementation must override it. + List> AllAuthorMediaPaths() + { + throw new NotSupportedException(); + } bool AuthorPathExists(string folder); void RemoveAddOptions(Author author); void SetMediaTypeMonitoring(int authorId, string mediaType, bool monitored); @@ -594,6 +599,11 @@ public Dictionary AllAuthorPaths() return _authorRepository.AllAuthorPaths(); } + public List> AllAuthorMediaPaths() + { + return _authorRepository.AllAuthorMediaPaths(); + } + public List AllForTag(int tagId) { return GetAllAuthors().Where(s => s.Tags.Contains(tagId)) diff --git a/src/NzbDrone.Core/Books/Services/BookService.cs b/src/NzbDrone.Core/Books/Services/BookService.cs index 148600d5..2e1ba52b 100644 --- a/src/NzbDrone.Core/Books/Services/BookService.cs +++ b/src/NzbDrone.Core/Books/Services/BookService.cs @@ -2019,6 +2019,11 @@ public void DeleteMany(List books) } public void DeleteMany(List books, bool deleteFiles) + { + DeleteMany(books, deleteFiles, skipDiskCleanup: false); + } + + private void DeleteMany(List books, bool deleteFiles, bool skipDiskCleanup) { var booksToDelete = (books ?? new List()) .Where(book => book != null) @@ -2030,7 +2035,7 @@ public void DeleteMany(List books, bool deleteFiles) foreach (var book in booksToDelete) { - _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false)); + _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false, skipDiskCleanup: skipDiskCleanup)); _providerAliasService?.DeleteAliases("Book", book.Id); } @@ -2215,7 +2220,10 @@ public void Handle(AuthorDeletedEvent message) // Previously hardcoded to false here regardless of what the author-level delete actually // requested, so MediaFileService always unlinked (never deleted) each book's BookFile rows - // leaving them behind as "unmapped files" even when the physical files really were deleted. - DeleteMany(books, message.DeleteFiles); + // skipDiskCleanup: true because MediaFileDeletionService's own AuthorDeletedEvent handler + // already recursively deletes the author's whole folder(s) when DeleteFiles is set - a + // per-book disk delete here would just race that and duplicate recycle-bin entries. + DeleteMany(books, message.DeleteFiles, skipDiskCleanup: true); } public void Execute(BulkSyncFormatMonitoringCommand message) diff --git a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs index 9ba599c8..dfdeaba8 100644 --- a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs +++ b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs @@ -119,7 +119,10 @@ public void Handle(BookDeletedEvent message) var authorId = book.AuthorId; - if (message.DeleteFiles) + // SkipDiskCleanup means this book is being deleted as part of a larger author delete, + // whose own handler already recursively removes the author's whole folder(s) - recycling + // extras individually here too would just race that and duplicate recycle-bin entries. + if (message.DeleteFiles && !message.SkipDiskCleanup) { var author = book.Author ?? GetAuthorOrNull(authorId); diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index 0bd07814..f9c46458 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -195,12 +195,15 @@ public void HandleAsync(AuthorDeletedEvent message) // as "unmapped" even though they were never actually removed. var pathsToDelete = new[] { author.Path, author.AudiobookPath, author.EbookPath } .Where(p => !p.IsNullOrWhiteSpace()) - .Distinct() + .Distinct(StringComparer.OrdinalIgnoreCase) .ToList(); // Fetched lazily, once, only if some path actually needs the other-authors check - // an author whose only path(s) are already refused as unsafe should never need it. - Dictionary allAuthors = null; + // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy + // single-path AllAuthorPaths - otherwise a dual-format author's AudiobookPath could + // collide with another author's separately-configured EbookPath and never be caught. + List> allAuthors = null; foreach (var path in pathsToDelete) { @@ -211,7 +214,7 @@ public void HandleAsync(AuthorDeletedEvent message) continue; } - allAuthors ??= _authorService.AllAuthorPaths(); + allAuthors ??= _authorService.AllAuthorMediaPaths(); var blockedByOtherAuthor = false; @@ -242,9 +245,20 @@ public void HandleAsync(AuthorDeletedEvent message) continue; } - if (_diskProvider.FolderExists(path)) + try { - _recycleBinProvider.DeleteFolder(path); + if (_diskProvider.FolderExists(path)) + { + _recycleBinProvider.DeleteFolder(path); + } + } + catch (Exception ex) + { + // Don't let one path's failure (permissions, a momentarily-unavailable NFS + // mount, ...) abort the rest of this author's paths or skip the + // DeleteCompletedEvent below - a partially-deleted author still needs its + // Plex refresh queue flushed. + _logger.Error(ex, "Failed to delete '{0}' for author '{1}'.", path, author.Name); } } @@ -291,7 +305,7 @@ private bool IsPathUnsafeToDelete(string path) public void HandleAsync(BookDeletedEvent message) { - if (!message.DeleteFiles) + if (!message.DeleteFiles || message.SkipDiskCleanup) { return; } From 3671adece7596127ce15b4527795697fde300dda Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 22:41:39 +0000 Subject: [PATCH 3/9] Address second round of adversarial review: per-path Calibre detection, fail loudly on unimplemented overload Two more problems found in review of #261: 1. Both AuthorDeletedEvent handlers derived a single isCalibre verdict from author.Path's root folder and applied it to the whole {Path, AudiobookPath, EbookPath} set. A mixed author (e.g. audiobooks under a plain folder, ebooks under a Calibre library - a completely normal Chaptarr configuration) broke in both directions depending on which path happened to be Path: a non-Calibre Path let the async handler recycle-bin the Calibre folder directly instead of routing it through _calibre.DeleteBook(s), corrupting Calibre's own metadata.db; a Calibre Path caused the entire new multi-path loop to skip, silently leaving the non-Calibre path's folder in place - reproducing this very PR's own Bug B. Both the sync Handle (which calls _calibre.DeleteBooks) and the async HandleAsync (which recycle-bins the rest) now compute isCalibre per path and route each one independently: Calibre paths get their specific files (filtered from GetFilesByAuthor by containment under that path) deleted via that path's own CalibreSettings; everything else goes through the existing safety-checked recycle-bin delete. 2. The new IBookService.DeleteMany(books, deleteFiles) default interface method silently forwarded to the 1-arg overload - i.e. dropped deleteFiles - instead of failing loudly, unlike the AllAuthorMediaPaths() default added in the same PR. Any future IBookService implementation that doesn't override the 2-arg overload would compile fine and silently reintroduce Bug A with no error. Changed to throw NotSupportedException, matching the established pattern. Adds a regression test for the mixed-format scenario: an author with a plain AudiobookPath and a Calibre-managed EbookPath now recycle-bins only the former and routes only the latter's file(s) through _calibre.DeleteBooks with the correct settings. --- .../MediaFileDeletionServiceFixture.cs | 102 ++++++++++ .../Books/Services/BookService.cs | 10 +- .../MediaFiles/MediaFileDeletionService.cs | 175 +++++++++++------- 3 files changed, 211 insertions(+), 76 deletions(-) diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs index bd2574de..881ae6c6 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs @@ -107,6 +107,37 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) } } + private class GetFilesByAuthorOnlyMediaFileServiceProxy : DispatchProxy + { + public List Files { get; set; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IMediaFileService.GetFilesByAuthor), StringComparison.Ordinal)) + { + return Files; + } + + throw new NotImplementedException($"Test proxy does not implement IMediaFileService.{targetMethod?.Name}"); + } + } + + private class RecordingCalibreProxy : DispatchProxy + { + public List<(List Books, CalibreSettings Settings)> DeleteBooksCalls { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(ICalibreProxy.DeleteBooks), StringComparison.Ordinal)) + { + DeleteBooksCalls.Add(((List)args[0], (CalibreSettings)args[1])); + return null; + } + + throw new NotImplementedException($"Test proxy does not implement ICalibreProxy.{targetMethod?.Name}"); + } + } + private class AllAuthorPathsOnlyAuthorServiceProxy : DispatchProxy { public List> Paths { get; } = new(); @@ -663,6 +694,77 @@ public void should_delete_both_audiobook_and_ebook_folders_on_author_delete_when Assert.That(eventAggregator.Events.OfType(), Is.Not.Empty); } + [Test] + public void should_route_each_path_through_calibre_or_recycle_bin_independently_when_only_one_format_is_calibre_managed() + { + // Calibre status must be decided per path, not once for the whole author from Path + // alone: a non-Calibre AudiobookPath must never be recycle-binned as if it were part of + // an EbookPath that happens to be Calibre-managed (or vice versa) - either direction + // would either corrupt Calibre's own metadata.db or leave a format's folder untouched. + var recycleBinProvider = new RecordingRecycleBinProvider(); + var eventAggregator = new RecordingEventAggregator(); + var diskProvider = DispatchProxy.Create(); + var diskProxy = (FolderExistsOnlyDiskProviderProxy)(object)diskProvider; + var calibreProxy = DispatchProxy.Create(); + var calibreRecorder = (RecordingCalibreProxy)(object)calibreProxy; + + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/audiobooks/Jim Butcher", + AudiobookPath = "/audiobooks/Jim Butcher", + EbookPath = "/calibre-ebooks/Jim Butcher" + }; + + diskProxy.ExistingFolders.Add(author.AudiobookPath); + diskProxy.ExistingFolders.Add(author.EbookPath); + + var calibreSettings = new CalibreSettings(); + var calibreRootFolder = new RootFolder + { + Path = "/calibre-ebooks", + FolderType = FolderType.Ebook, + IsCalibreLibrary = true, + CalibreSettings = calibreSettings + }; + + var rootFolderService = new StubRootFolderService( + new RootFolder { Path = "/audiobooks", FolderType = FolderType.Audiobook }, + calibreRootFolder); + + var ebookFile = new BookFile { Id = 1, Path = author.EbookPath + "/Storm Front.epub" }; + var audiobookFile = new BookFile { Id = 2, Path = author.AudiobookPath + "/Storm Front.m4b" }; + + var mediaFileService = DispatchProxy.Create(); + ((GetFilesByAuthorOnlyMediaFileServiceProxy)(object)mediaFileService).Files = new List { ebookFile, audiobookFile }; + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + mediaFileService, + DispatchProxy.Create(), + DispatchProxy.Create>(), + eventAggregator, + rootFolderService, + calibreProxy, + LogManager.GetCurrentClassLogger()); + + var authorDeletedEvent = new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false); + service.Handle(authorDeletedEvent); + service.HandleAsync(authorDeletedEvent); + + // The plain audiobook folder goes through the recycle bin, and only that folder. + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.AudiobookPath)); + Assert.That(recycleBinProvider.DeletedFolders, Does.Not.Contain(author.EbookPath)); + + // The Calibre-managed ebook folder goes through _calibre.DeleteBooks instead, with only + // the file(s) actually under that path, using that path's own CalibreSettings. + var calibreCall = calibreRecorder.DeleteBooksCalls.Single(); + Assert.That(calibreCall.Settings, Is.SameAs(calibreSettings)); + Assert.That(calibreCall.Books.Select(b => b.Id), Is.EquivalentTo(new[] { ebookFile.Id })); + } + [Test] public void should_delete_managed_ebook_replica_files_on_bookfile_delete_event_even_on_upgrade() { diff --git a/src/NzbDrone.Core/Books/Services/BookService.cs b/src/NzbDrone.Core/Books/Services/BookService.cs index 2e1ba52b..b9a85871 100644 --- a/src/NzbDrone.Core/Books/Services/BookService.cs +++ b/src/NzbDrone.Core/Books/Services/BookService.cs @@ -70,12 +70,14 @@ void UpdateManyWithLifecycle(List books) void ReassignAuthor(List books, int authorId) { throw new NotImplementedException(); } void RefreshProviderAliases(Book book) { } void DeleteMany(List books); - // The default exists only for lightweight test doubles; BookService overrides it directly. - // Kept as an overload (not an added parameter on the line above) so the many hand-written - // IBookService test stubs that only implement the 1-arg form keep compiling unchanged. + // The default exists only for lightweight test doubles. Kept as an overload (not an added + // parameter on the line above) so the many hand-written IBookService test stubs that only + // implement the 1-arg form keep compiling unchanged. Throws rather than silently forwarding + // to the 1-arg overload (which would drop deleteFiles) - every production implementation + // must override this directly, same as AllAuthorMediaPaths() elsewhere in this change. void DeleteMany(List books, bool deleteFiles) { - DeleteMany(books); + throw new NotSupportedException(); } void SetAddOptions(IEnumerable books); List GetAuthorBooksWithFiles(Author author); diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index f9c46458..bf0130d4 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -158,6 +158,22 @@ private void DeleteFile(BookFile bookFile, string subfolder = "", RootFolder roo } } + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) in + // addition to the legacy single Path field, and each one can independently be a Calibre + // library or not (e.g. ebooks under Calibre, audiobooks under a plain folder). Deriving + // "is this author's stuff Calibre-managed" from author.Path alone and applying that one + // verdict to every path is wrong in both directions: a non-Calibre Path with a Calibre + // EbookPath would recycle-bin the Calibre library directly instead of going through + // _calibre.DeleteBook(s) (corrupting its metadata.db), while a Calibre Path with a + // non-Calibre AudiobookPath would skip deleting the audiobook folder entirely. + private static List DistinctAuthorPaths(Author author) + { + return new[] { author.Path, author.AudiobookPath, author.EbookPath } + .Where(p => !p.IsNullOrWhiteSpace()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + } + [EventHandleOrder(EventHandleOrder.First)] public void Handle(AuthorDeletedEvent message) { @@ -165,14 +181,28 @@ public void Handle(AuthorDeletedEvent message) { var author = message.Author; - var rootFolder = _rootFolderService.GetBestRootFolder(message.Author.Path); - var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + List allFiles = null; - if (isCalibre) + foreach (var path in DistinctAuthorPaths(author)) { - // use authorId for the query - var books = _mediaFileService.GetFilesByAuthor(author.Id); - _calibre.DeleteBooks(books, rootFolder.CalibreSettings); + var rootFolder = _rootFolderService.GetBestRootFolder(path); + var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + + if (!isCalibre) + { + continue; + } + + allFiles ??= _mediaFileService.GetFilesByAuthor(author.Id); + + var booksUnderPath = allFiles + .Where(file => file?.Path != null && (path.IsParentPath(file.Path) || path.PathEquals(file.Path))) + .ToList(); + + if (booksUnderPath.Any()) + { + _calibre.DeleteBooks(booksUnderPath, rootFolder.CalibreSettings); + } } } } @@ -183,91 +213,92 @@ public void HandleAsync(AuthorDeletedEvent message) { var author = message.Author; - var rootFolder = _rootFolderService.GetBestRootFolder(message.Author.Path); - var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; - - if (!isCalibre) + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) + // in addition to the legacy single Path field. Only ever deleting Path left the other + // format's entire folder - and every file in it - untouched on disk while the DB + // treated the author as fully deleted, leaving those files' BookFile rows to surface + // as "unmapped" even though they were never actually removed. + var pathsToDelete = DistinctAuthorPaths(author); + + // Fetched lazily, once, only if some path actually needs the other-authors check - + // an author whose only path(s) are already refused as unsafe should never need it. + // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy + // single-path AllAuthorPaths - otherwise a dual-format author's AudiobookPath could + // collide with another author's separately-configured EbookPath and never be caught. + List> allAuthors = null; + + foreach (var path in pathsToDelete) { - // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) - // in addition to the legacy single Path field. Only ever deleting Path left the other - // format's entire folder - and every file in it - untouched on disk while the DB - // treated the author as fully deleted, leaving those files' BookFile rows to surface - // as "unmapped" even though they were never actually removed. - var pathsToDelete = new[] { author.Path, author.AudiobookPath, author.EbookPath } - .Where(p => !p.IsNullOrWhiteSpace()) - .Distinct(StringComparer.OrdinalIgnoreCase) - .ToList(); + var rootFolder = _rootFolderService.GetBestRootFolder(path); + var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; - // Fetched lazily, once, only if some path actually needs the other-authors check - - // an author whose only path(s) are already refused as unsafe should never need it. - // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy - // single-path AllAuthorPaths - otherwise a dual-format author's AudiobookPath could - // collide with another author's separately-configured EbookPath and never be caught. - List> allAuthors = null; + if (isCalibre) + { + // Calibre-managed paths are cleaned up via _calibre.DeleteBook(s) in the sync + // Handle() above, not a raw recycle-bin folder delete. + continue; + } - foreach (var path in pathsToDelete) + if (IsPathUnsafeToDelete(path)) { - if (IsPathUnsafeToDelete(path)) - { - _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", - path, author.Name); - continue; - } + _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", + path, author.Name); + continue; + } - allAuthors ??= _authorService.AllAuthorMediaPaths(); + allAuthors ??= _authorService.AllAuthorMediaPaths(); - var blockedByOtherAuthor = false; + var blockedByOtherAuthor = false; - foreach (var s in allAuthors) + foreach (var s in allAuthors) + { + if (s.Key == author.Id) { - if (s.Key == author.Id) - { - continue; - } - - if (path.IsParentPath(s.Value)) - { - _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); - blockedByOtherAuthor = true; - break; - } - - if (path.PathEquals(s.Value)) - { - _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); - blockedByOtherAuthor = true; - break; - } + continue; } - if (blockedByOtherAuthor) + if (path.IsParentPath(s.Value)) { - continue; + _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); + blockedByOtherAuthor = true; + break; } - try + if (path.PathEquals(s.Value)) { - if (_diskProvider.FolderExists(path)) - { - _recycleBinProvider.DeleteFolder(path); - } + _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); + blockedByOtherAuthor = true; + break; } - catch (Exception ex) + } + + if (blockedByOtherAuthor) + { + continue; + } + + try + { + if (_diskProvider.FolderExists(path)) { - // Don't let one path's failure (permissions, a momentarily-unavailable NFS - // mount, ...) abort the rest of this author's paths or skip the - // DeleteCompletedEvent below - a partially-deleted author still needs its - // Plex refresh queue flushed. - _logger.Error(ex, "Failed to delete '{0}' for author '{1}'.", path, author.Name); + _recycleBinProvider.DeleteFolder(path); } } - - // Always published once per author now, regardless of which (if any) path above was - // refused - the previous single-path version only published this in some of those - // cases, which could leave Plex's pending-refresh queue never flushed. ProcessQueue() - // is a no-op against an empty queue, so publishing it unconditionally here is safe. - _eventAggregator.PublishEvent(new DeleteCompletedEvent()); + catch (Exception ex) + { + // Don't let one path's failure (permissions, a momentarily-unavailable NFS + // mount, ...) abort the rest of this author's paths or skip the + // DeleteCompletedEvent below - a partially-deleted author still needs its + // Plex refresh queue flushed. + _logger.Error(ex, "Failed to delete '{0}' for author '{1}'.", path, author.Name); + } } + + // Always published once per author now, regardless of which (if any) path above was + // refused/Calibre-routed - the previous single-path version only published this in + // some cases, which could leave Plex's pending-refresh queue never flushed. + // ProcessQueue() is a no-op against an empty queue, so this is safe unconditionally. + _eventAggregator.PublishEvent(new DeleteCompletedEvent()); } } From c4aef2aa709ae2cd4049c3b9ecd27988d75f3135 Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 22:53:09 +0000 Subject: [PATCH 4/9] Address third round of adversarial review: OS-aware path comparison, isolate per-Calibre-path failures Two more fixes, plus two accepted tradeoffs documented below. Fixed: 1/2. Both DistinctAuthorPaths() (MediaFileDeletionService) and AllAuthorMediaPaths() (AuthorRepository) deduped Path/AudiobookPath/EbookPath with StringComparer.OrdinalIgnoreCase, but every actual path comparison in this codebase (PathEquals/IsParentPath) uses DiskProviderBase.PathStringComparison, which is case-sensitive (Ordinal) on Linux - the only OS this host runs. On Linux, OrdinalIgnoreCase could silently collapse two genuinely distinct, case-differing directories into one, dropping the other from both the deletion loop and the cross-author collision check. Switched both to StringComparer.FromComparison( DiskProviderBase.PathStringComparison) so dedup uses the same case-sensitivity semantics as every other path comparison in this file. 3. Handle(AuthorDeletedEvent)'s Calibre-cleanup loop had no per-path exception isolation, unlike the sibling recycle-bin loop in HandleAsync that this same PR already hardened for this exact failure class. An author with two distinct Calibre-managed paths whose first _calibre.DeleteBooks call throws (server down, timeout, locked metadata.db) would never attempt the second. Wrapped in try/catch, logs and continues to the next path. Accepted tradeoffs (not fixed in this PR): - A per-path physical-delete failure is caught and logged but BookFile DB rows for that author are still deleted unconditionally by MediaFileService.HandleAsync(BookDeletedEvent), since that decision is made by a separate, already-published event with no way to know the disk-delete outcome ahead of time in the current architecture. Making DB deletion wait on confirmed disk success would require restructuring author deletion so the whole-folder delete runs synchronously and first - which would reintroduce the blocking-request problem PR #259/#260 were written to fix. This narrows to genuine disk-level exceptions (permission errors, an unavailable NFS mount) during the delete call itself, which are now at least logged at Error level with the specific path, author, and exception. - SkipDiskCleanup is threaded manually through each per-book disk-cleanup handler (ExtraFileService, MediaFileDeletionService) rather than centralized in one place that owns "who deletes physical files for an author-triggered book delete." This matches how DeleteFiles itself already works as a threaded flag across multiple independent handlers in this codebase; centralizing it would be a larger structural change than this bug-fix PR's scope. --- .../Books/Repositories/AuthorRepository.cs | 3 ++- .../MediaFiles/MediaFileDeletionService.cs | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs index 813080ae..8c71358b 100644 --- a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs +++ b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs @@ -3,6 +3,7 @@ using System.Linq; using Dapper; using NLog; +using NzbDrone.Common.Disk; using NzbDrone.Common.Extensions; using NzbDrone.Core.Datastore; using NzbDrone.Core.Messaging.Events; @@ -135,7 +136,7 @@ public List> AllAuthorMediaPaths() return conn.Query(strSql) .SelectMany(row => new[] { row.Path, row.AudiobookPath, row.EbookPath } .Where(p => !string.IsNullOrWhiteSpace(p)) - .Distinct(StringComparer.OrdinalIgnoreCase) + .Distinct(StringComparer.FromComparison(DiskProviderBase.PathStringComparison)) .Select(p => new KeyValuePair(row.Id, p))) .ToList(); } diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index bf0130d4..d8677e96 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -170,7 +170,7 @@ private static List DistinctAuthorPaths(Author author) { return new[] { author.Path, author.AudiobookPath, author.EbookPath } .Where(p => !p.IsNullOrWhiteSpace()) - .Distinct(StringComparer.OrdinalIgnoreCase) + .Distinct(StringComparer.FromComparison(DiskProviderBase.PathStringComparison)) .ToList(); } @@ -199,10 +199,22 @@ public void Handle(AuthorDeletedEvent message) .Where(file => file?.Path != null && (path.IsParentPath(file.Path) || path.PathEquals(file.Path))) .ToList(); - if (booksUnderPath.Any()) + if (!booksUnderPath.Any()) + { + continue; + } + + try { _calibre.DeleteBooks(booksUnderPath, rootFolder.CalibreSettings); } + catch (Exception ex) + { + // Don't let one Calibre-managed path's failure (server down, timeout, locked + // metadata.db) stop another distinct Calibre path on this same author from + // being attempted - same isolation as the recycle-bin loop in HandleAsync. + _logger.Error(ex, "Failed to delete Calibre books at '{0}' for author '{1}'.", path, author.Name); + } } } } From 159770b26f22790e1d9779ed42a46132cac9174a Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 23:02:27 +0000 Subject: [PATCH 5/9] Address fourth round of adversarial review: apply safety checks uniformly to Calibre-routed paths Fixed: 1. Calibre-managed paths never went through IsPathUnsafeToDelete or the cross-author collision check at all - only the plain recycle-bin branch did. Before this PR only the single legacy Path could reach the ungated Calibre branch; this PR's multi-path change tripled that blast radius (Path, AudiobookPath, EbookPath) without extending the guard to match. Extracted the shared check into ShouldRefuseToDeletePath, used identically by both the sync Handle (Calibre) and async HandleAsync (recycle-bin) loops, so every path gets the same refusal checks regardless of which deletion mechanism it ends up routed through. 2. path.PathEquals(file.Path) in the Calibre file-containment filter compared a directory against a full file path and could never be true (mirrored from the author-vs-author path comparison a few lines below, where both sides really are directories - not applicable here). Removed the dead condition, keeping just the IsParentPath containment check that actually applies. Two further findings from this round are narrow, pre-existing-in-spirit limitations left as known gaps rather than expanded further: - A Calibre path whose files are already unlinked (EditionId=0, e.g. from a prior partial/buggy delete) won't be found by GetFilesByAuthor's join and so won't be routed to _calibre.DeleteBooks. This requires an already-desynced DB state to trigger and existed in spirit before this PR too (the old single-path version had the same GetFilesByAuthor dependency); fully closing it means reconciling Calibre-vs-DB state independent of the delete path, which is out of scope here. - A path nested inside another of the same author's paths (e.g. AudiobookPath physically nested under a Calibre EbookPath) can have its files swept into the wrong path's containment filter. Before this PR, an author's single Calibre Path sent every one of the author's files to Calibre unconditionally with no filtering at all, so this is a narrower failure surface than before, not a new one - but a fully correct fix would need to verify path disjointness across the whole set, which given how unusual this folder layout is, is left for a follow-up rather than this PR. --- .../MediaFiles/MediaFileDeletionService.cs | 94 ++++++++++--------- 1 file changed, 51 insertions(+), 43 deletions(-) diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index d8677e96..6bff11f2 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -174,6 +174,48 @@ private static List DistinctAuthorPaths(Author author) .ToList(); } + // Shared by both handlers below so a Calibre-routed path gets exactly the same refusal + // checks as a plain recycle-bin path - it used to skip them entirely, which only mattered + // for the single legacy Path but now applies to up to three paths per author. + // allAuthorsCache is fetched lazily, once per method call, only if some path actually needs + // it - an author whose only path(s) are already refused as unsafe should never touch it. + // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy single-path + // AllAuthorPaths - otherwise a dual-format author's AudiobookPath could collide with another + // author's separately-configured EbookPath and never be caught. + private bool ShouldRefuseToDeletePath(string path, Author author, ref List> allAuthorsCache) + { + if (IsPathUnsafeToDelete(path)) + { + _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", + path, author.Name); + return true; + } + + allAuthorsCache ??= _authorService.AllAuthorMediaPaths(); + + foreach (var s in allAuthorsCache) + { + if (s.Key == author.Id) + { + continue; + } + + if (path.IsParentPath(s.Value)) + { + _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); + return true; + } + + if (path.PathEquals(s.Value)) + { + _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); + return true; + } + } + + return false; + } + [EventHandleOrder(EventHandleOrder.First)] public void Handle(AuthorDeletedEvent message) { @@ -182,6 +224,7 @@ public void Handle(AuthorDeletedEvent message) var author = message.Author; List allFiles = null; + List> allAuthors = null; foreach (var path in DistinctAuthorPaths(author)) { @@ -193,10 +236,15 @@ public void Handle(AuthorDeletedEvent message) continue; } + if (ShouldRefuseToDeletePath(path, author, ref allAuthors)) + { + continue; + } + allFiles ??= _mediaFileService.GetFilesByAuthor(author.Id); var booksUnderPath = allFiles - .Where(file => file?.Path != null && (path.IsParentPath(file.Path) || path.PathEquals(file.Path))) + .Where(file => file?.Path != null && path.IsParentPath(file.Path)) .ToList(); if (!booksUnderPath.Any()) @@ -230,16 +278,9 @@ public void HandleAsync(AuthorDeletedEvent message) // format's entire folder - and every file in it - untouched on disk while the DB // treated the author as fully deleted, leaving those files' BookFile rows to surface // as "unmapped" even though they were never actually removed. - var pathsToDelete = DistinctAuthorPaths(author); - - // Fetched lazily, once, only if some path actually needs the other-authors check - - // an author whose only path(s) are already refused as unsafe should never need it. - // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy - // single-path AllAuthorPaths - otherwise a dual-format author's AudiobookPath could - // collide with another author's separately-configured EbookPath and never be caught. List> allAuthors = null; - foreach (var path in pathsToDelete) + foreach (var path in DistinctAuthorPaths(author)) { var rootFolder = _rootFolderService.GetBestRootFolder(path); var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; @@ -251,40 +292,7 @@ public void HandleAsync(AuthorDeletedEvent message) continue; } - if (IsPathUnsafeToDelete(path)) - { - _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", - path, author.Name); - continue; - } - - allAuthors ??= _authorService.AllAuthorMediaPaths(); - - var blockedByOtherAuthor = false; - - foreach (var s in allAuthors) - { - if (s.Key == author.Id) - { - continue; - } - - if (path.IsParentPath(s.Value)) - { - _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); - blockedByOtherAuthor = true; - break; - } - - if (path.PathEquals(s.Value)) - { - _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); - blockedByOtherAuthor = true; - break; - } - } - - if (blockedByOtherAuthor) + if (ShouldRefuseToDeletePath(path, author, ref allAuthors)) { continue; } From fa5d1f549e751839743ce9edadd932c9f813cd55 Mon Sep 17 00:00:00 2001 From: jordan Date: Sun, 27 Sep 2026 23:11:14 +0000 Subject: [PATCH 6/9] Address fifth round of adversarial review: reuse existing path-dedup helper Replaced the private DistinctAuthorPaths(Author) helper with the existing ExtraFilePathHelper.GetAuthorBasePaths(Author) - already used elsewhere in this same file (CleanupEmptyFolders) - which dedups the identical {Path, AudiobookPath, EbookPath} triple via PathEquals, the same OS-aware comparison semantics as the StringComparer.FromComparison( DiskProviderBase.PathStringComparison) I'd hand-rolled. Avoids two divergent implementations of the same dedup logic living in the same file. This round's other two findings restate the accepted tradeoffs already documented in the previous commit (DB-row deletion not gated on confirmed physical-delete success; a nested-path-inside-another-path edge case for Calibre file matching) rather than surfacing anything new - a reasonable signal this has converged on the core bug. The remaining minor nit (AllAuthorMediaPaths fetched independently by both Handle and HandleAsync for the same event) is accepted as-is: sharing it would need mutable state across two independently-dispatched event handlers for a single extra flat-table query per author delete. --- .../MediaFiles/MediaFileDeletionService.cs | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index 6bff11f2..bc9cc8ea 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -158,22 +158,6 @@ private void DeleteFile(BookFile bookFile, string subfolder = "", RootFolder roo } } - // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) in - // addition to the legacy single Path field, and each one can independently be a Calibre - // library or not (e.g. ebooks under Calibre, audiobooks under a plain folder). Deriving - // "is this author's stuff Calibre-managed" from author.Path alone and applying that one - // verdict to every path is wrong in both directions: a non-Calibre Path with a Calibre - // EbookPath would recycle-bin the Calibre library directly instead of going through - // _calibre.DeleteBook(s) (corrupting its metadata.db), while a Calibre Path with a - // non-Calibre AudiobookPath would skip deleting the audiobook folder entirely. - private static List DistinctAuthorPaths(Author author) - { - return new[] { author.Path, author.AudiobookPath, author.EbookPath } - .Where(p => !p.IsNullOrWhiteSpace()) - .Distinct(StringComparer.FromComparison(DiskProviderBase.PathStringComparison)) - .ToList(); - } - // Shared by both handlers below so a Calibre-routed path gets exactly the same refusal // checks as a plain recycle-bin path - it used to skip them entirely, which only mattered // for the single legacy Path but now applies to up to three paths per author. @@ -226,7 +210,13 @@ public void Handle(AuthorDeletedEvent message) List allFiles = null; List> allAuthors = null; - foreach (var path in DistinctAuthorPaths(author)) + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) + // in addition to the legacy single Path field, and each one can independently be a + // Calibre library or not. Deriving Calibre status from just one path and applying it + // to the rest is wrong in both directions, so each path here is checked individually. + // ExtraFilePathHelper.GetAuthorBasePaths is the existing helper for exactly this + // {Path, AudiobookPath, EbookPath} dedup - reused here rather than reimplemented. + foreach (var path in ExtraFilePathHelper.GetAuthorBasePaths(author)) { var rootFolder = _rootFolderService.GetBestRootFolder(path); var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; @@ -280,7 +270,7 @@ public void HandleAsync(AuthorDeletedEvent message) // as "unmapped" even though they were never actually removed. List> allAuthors = null; - foreach (var path in DistinctAuthorPaths(author)) + foreach (var path in ExtraFilePathHelper.GetAuthorBasePaths(author)) { var rootFolder = _rootFolderService.GetBestRootFolder(path); var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; From b930734c7b245efbd1cdb59e8c285181993a5573 Mon Sep 17 00:00:00 2001 From: jordan Date: Mon, 28 Sep 2026 02:45:23 +0000 Subject: [PATCH 7/9] Suppress per-book OnBookDelete notifications during an author delete Sonarr/Radarr don't send a per-episode/per-file delete notification when a whole series or movie is deleted - just the one series/movie-level notification. Chaptarr's author delete didn't follow that convention: BookService.Handle(AuthorDeletedEvent) publishes BookDeletedEvent once per book, and NotificationService fired a full OnBookDelete notification for every single one on top of the single OnAuthorDelete notification already sent for the whole author - for a large author that's thousands of redundant notifications (and, per the notification target's own rate limiting seen in production, hundreds of visible errors logged for a single user action). Renamed BookDeletedEvent.SkipDiskCleanup to PartOfAuthorDelete, since it now needs to gate two different per-book handlers instead of just the disk-cleanup ones: NotificationService.Handle (BookDeletedEvent) now returns immediately when this is set, without even looking up which notifications are enabled. MediaFileDeletionService and ExtraFileService's existing checks against the old name are unaffected in behavior, just renamed to match. Added NotificationServiceBookDeletedFixture: a throwing INotificationFactory proves the lookup is skipped entirely for an author-delete book (not just that no notification happens to fire), and a counting proxy proves a standalone book delete still looks notifications up exactly once. --- .../Books/BookServiceAuthorDeletedFixture.cs | 6 +- .../MediaFileDeletionServiceFixture.cs | 6 +- .../NotificationServiceBookDeletedFixture.cs | 79 +++++++++++++++++++ .../Books/Events/BookDeletedEvent.cs | 21 ++--- .../Books/Services/BookService.cs | 17 ++-- .../Extras/Files/ExtraFileService.cs | 4 +- .../MediaFiles/MediaFileDeletionService.cs | 2 +- .../Notifications/NotificationService.cs | 9 +++ 8 files changed, 120 insertions(+), 24 deletions(-) create mode 100644 src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs diff --git a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs index 82246da4..2752032b 100644 --- a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs +++ b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs @@ -105,8 +105,10 @@ public void should_pass_the_authors_delete_files_flag_through_to_each_books_dele Assert.That(bookRepoProxy.DeletedBooks.Select(b => b.Id), Does.Contain(book.Id)); // MediaFileDeletionService's own AuthorDeletedEvent handler already recursively deletes - // the author's whole folder(s) - a per-book disk delete here would just race it. - Assert.That(published.SkipDiskCleanup, Is.True); + // the author's whole folder(s), and NotificationService already sends one OnAuthorDelete + // notification for the whole author - a per-book disk delete or notification here would + // just duplicate both. + Assert.That(published.PartOfAuthorDelete, Is.True); } [Test] diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs index 881ae6c6..8bd54c7c 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs @@ -451,8 +451,8 @@ public void should_delete_every_file_and_replica_and_remove_the_folder_on_whole_ [Test] public void should_do_no_disk_work_on_book_delete_when_an_author_delete_already_covers_it() { - // A book delete published as part of a larger author delete (SkipDiskCleanup) relies on - // MediaFileDeletionService's own AuthorDeletedEvent handler to recursively remove the + // A book delete published as part of a larger author delete (PartOfAuthorDelete) relies + // on MediaFileDeletionService's own AuthorDeletedEvent handler to recursively remove the // whole author folder. Doing per-file work here too would race that and duplicate // recycle-bin entries for the same files. var author = new Author @@ -498,7 +498,7 @@ public void should_do_no_disk_work_on_book_delete_when_an_author_delete_already_ }; Assert.DoesNotThrow(() => - service.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, skipDiskCleanup: true))); + service.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: true))); Assert.That(recycleBinProvider.DeletedFiles, Is.Empty); } diff --git a/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs b/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs new file mode 100644 index 00000000..84fb6b69 --- /dev/null +++ b/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs @@ -0,0 +1,79 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using NLog; +using NUnit.Framework; +using NzbDrone.Core.Books; +using NzbDrone.Core.Books.Events; +using NzbDrone.Core.Notifications; + +namespace Chaptarr.Core.Test.Notifications +{ + // A book delete published as part of a larger author delete already gets a single + // OnAuthorDelete notification for the whole author. Sending OnBookDelete on top of that for + // every one of its books is the "one notification per episode when the whole series was + // deleted" noise Sonarr/Radarr deliberately don't send. + [TestFixture] + public class NotificationServiceBookDeletedFixture + { + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private class EmptyOnBookDeleteNotificationFactoryProxy : DispatchProxy + { + public int OnBookDeleteEnabledCalls { get; private set; } + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(INotificationFactory.OnBookDeleteEnabled), StringComparison.Ordinal)) + { + OnBookDeleteEnabledCalls++; + return new List(); + } + + throw new NotImplementedException($"Test proxy does not implement INotificationFactory.{targetMethod?.Name}"); + } + } + + [Test] + public void should_not_look_up_notifications_at_all_when_part_of_an_author_delete() + { + // A throwing INotificationFactory proves OnBookDeleteEnabled is never even called - + // not just that no notification happens to fire. + var service = new NotificationService( + DispatchProxy.Create>(), + DispatchProxy.Create>(), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book { Id = 1, Author = new Author { Id = 1, Name = "Jim Butcher" } }; + + Assert.DoesNotThrow(() => + service.Handle(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: true))); + } + + [Test] + public void should_still_look_up_notifications_for_a_standalone_book_delete() + { + var factory = DispatchProxy.Create(); + var factoryRecorder = (EmptyOnBookDeleteNotificationFactoryProxy)(object)factory; + + var service = new NotificationService( + factory, + DispatchProxy.Create>(), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book { Id = 1, Author = new Author { Id = 1, Name = "Jim Butcher" } }; + + service.Handle(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: false)); + + Assert.That(factoryRecorder.OnBookDeleteEnabledCalls, Is.EqualTo(1)); + } + } +} diff --git a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs index d299dc83..9b3dc047 100644 --- a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs +++ b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs @@ -13,15 +13,18 @@ public class BookDeletedEvent : IEvent public IReadOnlyList DeletedBooks { get; private set; } // Set when this book is being deleted as part of a larger author delete, whose own - // AuthorDeletedEvent handler (MediaFileDeletionService) already recursively removes the - // author's whole folder(s). Per-book/per-file disk handlers should skip their own disk work - // in that case - it's redundant at best and racy (duplicate recycle-bin entries, deleting - // files out from under each other) at worst - while DeleteFiles still controls whether - // DB-only cleanup (e.g. actually deleting BookFile rows instead of just unlinking them) - // happens, since that's independent of who removes the physical files. - public bool SkipDiskCleanup { get; private set; } + // AuthorDeletedEvent handling already covers what a per-book handler would otherwise do + // again for every one of the author's books: MediaFileDeletionService recursively removes + // the author's whole folder(s) (so per-book/per-file disk cleanup here is redundant at best + // and racy - duplicate recycle-bin entries, deleting files out from under each other - at + // worst), and NotificationService already sends a single OnAuthorDelete notification (so a + // per-book OnBookDelete on top of that is exactly the "one notification per episode when a + // whole series is deleted" noise Sonarr/Radarr deliberately avoid). DeleteFiles still + // independently controls whether MediaFileService deletes vs. unlinks the BookFile DB rows, + // since that's unrelated to who removes physical files or which notification fires. + public bool PartOfAuthorDelete { get; private set; } - public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null, bool skipDiskCleanup = false) + public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null, bool partOfAuthorDelete = false) { Book = book; DeleteFiles = deleteFiles; @@ -30,7 +33,7 @@ public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion DeletedBooks = (deletedBooks ?? Enumerable.Repeat(book, 1)) .Where(item => item != null) .ToList(); - SkipDiskCleanup = skipDiskCleanup; + PartOfAuthorDelete = partOfAuthorDelete; } } } diff --git a/src/NzbDrone.Core/Books/Services/BookService.cs b/src/NzbDrone.Core/Books/Services/BookService.cs index b9a85871..8710ed25 100644 --- a/src/NzbDrone.Core/Books/Services/BookService.cs +++ b/src/NzbDrone.Core/Books/Services/BookService.cs @@ -2022,10 +2022,10 @@ public void DeleteMany(List books) public void DeleteMany(List books, bool deleteFiles) { - DeleteMany(books, deleteFiles, skipDiskCleanup: false); + DeleteMany(books, deleteFiles, partOfAuthorDelete: false); } - private void DeleteMany(List books, bool deleteFiles, bool skipDiskCleanup) + private void DeleteMany(List books, bool deleteFiles, bool partOfAuthorDelete) { var booksToDelete = (books ?? new List()) .Where(book => book != null) @@ -2037,7 +2037,7 @@ private void DeleteMany(List books, bool deleteFiles, bool skipDiskCleanup foreach (var book in booksToDelete) { - _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false, skipDiskCleanup: skipDiskCleanup)); + _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false, partOfAuthorDelete: partOfAuthorDelete)); _providerAliasService?.DeleteAliases("Book", book.Id); } @@ -2222,10 +2222,13 @@ public void Handle(AuthorDeletedEvent message) // Previously hardcoded to false here regardless of what the author-level delete actually // requested, so MediaFileService always unlinked (never deleted) each book's BookFile rows - // leaving them behind as "unmapped files" even when the physical files really were deleted. - // skipDiskCleanup: true because MediaFileDeletionService's own AuthorDeletedEvent handler - // already recursively deletes the author's whole folder(s) when DeleteFiles is set - a - // per-book disk delete here would just race that and duplicate recycle-bin entries. - DeleteMany(books, message.DeleteFiles, skipDiskCleanup: true); + // partOfAuthorDelete: true because MediaFileDeletionService's own AuthorDeletedEvent handler + // already recursively deletes the author's whole folder(s) when DeleteFiles is set (a + // per-book disk delete here would just race that and duplicate recycle-bin entries), and + // NotificationService already sends one OnAuthorDelete notification for the whole author - + // a per-book OnBookDelete on top of that is the "notification per episode when the whole + // series was deleted" noise Sonarr/Radarr deliberately don't send. + DeleteMany(books, message.DeleteFiles, partOfAuthorDelete: true); } public void Execute(BulkSyncFormatMonitoringCommand message) diff --git a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs index dfdeaba8..fedb00c4 100644 --- a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs +++ b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs @@ -119,10 +119,10 @@ public void Handle(BookDeletedEvent message) var authorId = book.AuthorId; - // SkipDiskCleanup means this book is being deleted as part of a larger author delete, + // PartOfAuthorDelete means this book is being deleted as part of a larger author delete, // whose own handler already recursively removes the author's whole folder(s) - recycling // extras individually here too would just race that and duplicate recycle-bin entries. - if (message.DeleteFiles && !message.SkipDiskCleanup) + if (message.DeleteFiles && !message.PartOfAuthorDelete) { var author = book.Author ?? GetAuthorOrNull(authorId); diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index bc9cc8ea..b38ee6ad 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -346,7 +346,7 @@ private bool IsPathUnsafeToDelete(string path) public void HandleAsync(BookDeletedEvent message) { - if (!message.DeleteFiles || message.SkipDiskCleanup) + if (!message.DeleteFiles || message.PartOfAuthorDelete) { return; } diff --git a/src/NzbDrone.Core/Notifications/NotificationService.cs b/src/NzbDrone.Core/Notifications/NotificationService.cs index 4616921c..8ba913f0 100644 --- a/src/NzbDrone.Core/Notifications/NotificationService.cs +++ b/src/NzbDrone.Core/Notifications/NotificationService.cs @@ -339,6 +339,15 @@ public void Handle(AuthorDeletedEvent message) public void Handle(BookDeletedEvent message) { + // A book delete published as part of a larger author delete already gets a single + // OnAuthorDelete notification for the whole author - sending OnBookDelete on top of that + // for every one of its books is the "one notification per episode when a whole series is + // deleted" noise Sonarr/Radarr deliberately don't send. + if (message.PartOfAuthorDelete) + { + return; + } + var deleteMessage = new BookDeleteMessage(message.Book, message.DeleteFiles); foreach (var notification in _notificationFactory.OnBookDeleteEnabled()) From 6747cf1dece60e75797865f524c69584824a0843 Mon Sep 17 00:00:00 2001 From: jordan Date: Tue, 29 Sep 2026 19:48:31 +0000 Subject: [PATCH 8/9] Delete the book-file rows captured in the delete event when "delete files" is set With "delete files" checked, deleting an author still left its files listed under Unmapped. BookDeletedEvent handling deletes a book's editions synchronously (EditionService), and MediaFileService.Handle(EditionDeletedEvent) unlinks their files (EditionId = 0) before the async MediaFileService.HandleAsync(BookDeletedEvent) runs. That handler then called DeleteFilesByBook(bookId), which looks files up through the book's editions - by then none match, so nothing was deleted and every row stayed behind as an unmapped file (its path already removed from disk by MediaFileDeletionService). Delete the rows captured in the event's hydrated snapshot (Book.BookFiles) by id, and keep DeleteFilesByBook as a backstop for anything linked after the snapshot. The keep-files path is unchanged (rows stay unmapped on purpose). Seen live: author "ONE" deleted with delete-files: log showed "Unlinking 8 files from deleted edition" and the folder removed, leaving 8 unmapped rows for files that no longer exist. Tests: snapshot rows already unlinked (EditionId 0) are deleted by id (fails without the change); with delete-files off no rows are deleted. --- .../MediaFileServiceDeleteFixture.cs | 49 +++++++++++++++++++ .../MediaFiles/MediaFileService.cs | 17 +++++++ 2 files changed, 66 insertions(+) diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs index 71c993d2..e3422e0c 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs @@ -70,6 +70,7 @@ public int PurgeUnderPath(string pathPrefix) private class MediaFileRepositoryProxy : DispatchProxy { public List DeletedMany { get; } = new List(); + public List DeletedIds { get; } = new List(); public List ReplacementAdditions { get; } = new List(); public List ReplacementRemovals { get; } = new List(); public BookFile DeletedSingle { get; private set; } @@ -87,6 +88,12 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) return null; case nameof(IMediaFileRepository.DeleteMany): + if (args[0] is IEnumerable deletedIds) + { + DeletedIds.AddRange(deletedIds); + return null; + } + DeletedMany.Clear(); DeletedMany.AddRange((IEnumerable)args[0]); return null; @@ -323,5 +330,47 @@ public void book_delete_should_purge_ingest_queue_for_left_behind_book_files() Assert.That(ingestQueue.PurgedPrefixes, Is.EqualTo(new[] { "/books/books/Test Author/Test Book/Test Book.m4b" })); } + [Test] + public void book_delete_with_delete_files_should_delete_snapshot_rows_that_editions_already_unlinked() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + // EditionDeletedEvent has already unlinked these (EditionId = 0) by the time the async handler runs. + var book = new Book + { + Id = 10, + Title = "Test Book", + BookFiles = new List + { + new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 }, + new BookFile { Id = 2, Path = "/books/books/Test Author/Test Book/b.mp3", EditionId = 0 } + } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.EquivalentTo(new[] { 1, 2 }), "rows must be deleted by id, not left behind as unmapped"); + } + + [Test] + public void book_delete_without_delete_files_should_not_delete_any_rows() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + var book = new Book + { + Id = 10, + Title = "Test Book", + BookFiles = new List { new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 } } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: false, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.Empty, "keeping files means keeping their (unmapped) rows"); + } } } diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs index 5a03cff4..2966c471 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs @@ -503,6 +503,23 @@ public void HandleAsync(BookDeletedEvent message) if (message.DeleteFiles) { + // A book's editions are deleted synchronously while BookDeletedEvent is being published + // (EditionService), and Handle(EditionDeletedEvent) unlinks their files (EditionId = 0) + // before this async handler runs - so a lookup by book/edition no longer finds them and + // DeleteFilesByBook alone leaves the rows behind as "unmapped" files even though the user + // asked to delete files. Delete the rows captured in the event's snapshot by id, and keep + // the by-book delete for any row linked after the snapshot was taken. + var snapshotIds = (bookFiles ?? new List()) + .Where(file => file != null && file.Id > 0) + .Select(file => file.Id) + .Distinct() + .ToList(); + + if (snapshotIds.Any()) + { + _mediaFileRepository.DeleteMany(snapshotIds); + } + _mediaFileRepository.DeleteFilesByBook(message.Book.Id); } else From 43d502b3590ae8570c4f16c5ae91e28e9c055f8f Mon Sep 17 00:00:00 2001 From: jordan Date: Tue, 29 Sep 2026 19:50:39 +0000 Subject: [PATCH 9/9] Only delete snapshot file rows that are still unlinked or belong to the deleted book Adversarial review of #261: a file id in the delete event's snapshot could, in a narrow race (concurrent import/replace during a delete of the same book), have been re-linked to another book's edition before the async handler runs. Re-read the rows and delete only those with EditionId 0 or an edition of the deleted book. Tests: a re-linked row survives; the by-book backstop is still called on the delete-files path and not called on the keep-files path. --- .../MediaFileServiceDeleteFixture.cs | 34 ++++++++++++++++++- .../MediaFiles/MediaFileService.cs | 17 +++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs index e3422e0c..2ba2492b 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs @@ -71,6 +71,7 @@ private class MediaFileRepositoryProxy : DispatchProxy { public List DeletedMany { get; } = new List(); public List DeletedIds { get; } = new List(); + public int DeleteFilesByBookCalls { get; private set; } public List ReplacementAdditions { get; } = new List(); public List ReplacementRemovals { get; } = new List(); public BookFile DeletedSingle { get; private set; } @@ -115,8 +116,11 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) return null; - case nameof(IMediaFileRepository.UnlinkFilesByBook): case nameof(IMediaFileRepository.DeleteFilesByBook): + DeleteFilesByBookCalls++; + return null; + + case nameof(IMediaFileRepository.UnlinkFilesByBook): return null; case nameof(IMediaFileRepository.GetFilesByEdition): @@ -335,6 +339,7 @@ public void book_delete_with_delete_files_should_delete_snapshot_rows_that_editi { var repo = DispatchProxy.Create(); var repoProxy = (MediaFileRepositoryProxy)(object)repo; + repoProxy.GetByIdsHandler = ids => ids.Select(id => new BookFile { Id = id, EditionId = 0 }); var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); // EditionDeletedEvent has already unlinked these (EditionId = 0) by the time the async handler runs. @@ -352,6 +357,32 @@ public void book_delete_with_delete_files_should_delete_snapshot_rows_that_editi sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false)); Assert.That(repoProxy.DeletedIds, Is.EquivalentTo(new[] { 1, 2 }), "rows must be deleted by id, not left behind as unmapped"); + Assert.That(repoProxy.DeleteFilesByBookCalls, Is.EqualTo(1), "the by-book delete stays as a backstop"); + } + + [Test] + public void book_delete_with_delete_files_should_not_delete_a_row_relinked_to_another_books_edition() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + repoProxy.GetByIdsHandler = ids => ids.Select(id => new BookFile { Id = id, EditionId = id == 2 ? 999 : 0 }); + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + var book = new Book + { + Id = 10, + Title = "Test Book", + Editions = new List { new Edition { Id = 100, BookId = 10 } }, + BookFiles = new List + { + new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 }, + new BookFile { Id = 2, Path = "/books/books/Test Author/Test Book/b.mp3", EditionId = 0 } + } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.EqualTo(new[] { 1 }), "row 2 now belongs to edition 999 (another book) and must survive"); } [Test] @@ -371,6 +402,7 @@ public void book_delete_without_delete_files_should_not_delete_any_rows() sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: false, addImportListExclusion: false)); Assert.That(repoProxy.DeletedIds, Is.Empty, "keeping files means keeping their (unmapped) rows"); + Assert.That(repoProxy.DeleteFilesByBookCalls, Is.EqualTo(0), "the keep-files path must not delete by book either"); } } } diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs index 2966c471..9c14d76f 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs @@ -517,7 +517,22 @@ public void HandleAsync(BookDeletedEvent message) if (snapshotIds.Any()) { - _mediaFileRepository.DeleteMany(snapshotIds); + // Re-read the rows and only delete ones that are still unlinked or still belong to this + // book's own editions: an id in the snapshot that was re-linked to another book's edition + // in the meantime (concurrent import/replace) must never be deleted here. + var ownEditionIds = (message.Book.Editions ?? new List()) + .Select(edition => edition.Id) + .ToHashSet(); + + var deletableIds = (_mediaFileRepository.Get(snapshotIds) ?? Enumerable.Empty()) + .Where(file => file.EditionId == 0 || ownEditionIds.Contains(file.EditionId)) + .Select(file => file.Id) + .ToList(); + + if (deletableIds.Any()) + { + _mediaFileRepository.DeleteMany(deletableIds); + } } _mediaFileRepository.DeleteFilesByBook(message.Book.Id);