Fix three bugs that left "delete files" author deletes with unmapped/orphaned files - #261
Open
jordanfelle wants to merge 9 commits into
Open
jordanfelle wants to merge 9 commits into
jordanfelle wants to merge 9 commits into
Conversation
…phaned 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).
… duplicate disk work, isolate per-path failures Four problems found in review of Chaptarr#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).
…n, fail loudly on unimplemented overload Two more problems found in review of Chaptarr#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.
…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 Chaptarr#259/Chaptarr#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.
…rmly 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.
…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.
Author
Live verification on productionDeployed this fix (along with #260) and tested against a real large author: Daniel Defoe, 5420 books, Before: 4,345 pre-existing unlinked After the delete completed:
No leftover author/book rows, no evidence of new "unmapped files" from this delete. Matches the intended fix. |
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.
…iles" 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.
jordanfelle
changed the title
…he deleted book Adversarial review of Chaptarr#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Investigating a report of "a lot of unmapped files" left behind after deleting authors with "delete files" checked turned up two separate bugs:
Bug A - every author delete, universal.
BookService.DeleteManypublished each book'sBookDeletedEventwithdeleteFileshardcoded tofalse, regardless of what the author-level delete actually requested. SinceBookService.Handle(AuthorDeletedEvent)always routes through this method,MediaFileServicenever deleted a singleBookFilerow for any author delete - it only ever unlinked them (EditionId = 0). Those unlinked rows are exactly what the Unmapped Files UI surfaces, so every author delete left its books' file rows behind as apparent "importable" content, even when the files were genuinely gone from disk. Fixed by threading the author'sDeleteFilesflag through. Kept as aDeleteMany(books, deleteFiles)overload rather than adding a parameter to the existingDeleteMany(books), so the ~20 hand-writtenIBookServicetest stubs across the suite that only implement the 1-arg form keep compiling unchanged.Bug B - dual-format authors only.
MediaFileDeletionService.HandleAsync(AuthorDeletedEvent)only ever deleted the legacy singleAuthor.Pathfolder. An author with separate audiobook and ebook root folders (AudiobookPath/EbookPath) would have one format's folder deleted and the other left completely untouched on disk. Confirmed in production: one author's ebook folder (166 files) survived fully intact while their 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 individually. TheAllAuthorPaths()lookup those checks use is now fetched lazily - only if some path actually needs it - so an author whose only path is already refused as unsafe still never touches it (an existing test relies on this).Small related fix:
DeleteCompletedEvent(flushes Plex's pending-refresh queue) is now published exactly once per author delete unconditionally, instead of only in some of the refusal branches as before - it's a no-op against an empty queue, so this just closes a case where the queue could previously go unflushed.Test plan
dotnet testsuite passes (3038/3038)MediaFileDeletionServiceFixturetest: both audiobook and ebook folders get deleted when they differBookServiceAuthorDeletedFixture:DeleteFilesreachesBookDeletedEventcorrectly in both directionsBookFilerows and no surviving folder on either rootUpdate: Bug C - the Bug A fix did not take effect (found live after deploying this PR)
After deploying this branch, deleting an author with "delete files" checked still left its files under Unmapped. Log for the delete:
MediaFileService: Unlinking 8 files from deleted edition ..., then the folder removed from disk, then 8BookFilesrows withEditionId = 0for paths that no longer exist.Cause: threading
deleteFilesintoBookDeletedEvent(Bug A) was not enough because of handler ordering.EditionService.Handle(BookDeletedEvent)deletes the book's editions synchronously, andMediaFileService.Handle(EditionDeletedEvent)unlinks their files (EditionId = 0) right then. ThedeleteFilesbranch lives in the asyncMediaFileService.HandleAsync(BookDeletedEvent), which runs afterwards and callsDeleteFilesByBook(bookId)- a lookup through the book's editions, which no longer match. It deleted nothing.Fix: when
DeleteFilesis set, delete the rows captured in the event's hydrated snapshot (Book.BookFiles) by id, and keepDeleteFilesByBookas a backstop for anything linked after the snapshot. The keep-files path is unchanged (rows stay unmapped on purpose).Tests:
book_delete_with_delete_files_should_delete_snapshot_rows_that_editions_already_unlinked(fails without the change) andbook_delete_without_delete_files_should_not_delete_any_rows. FullChaptarr.Core.Test: 3044 passed.Follow-up from adversarial review: the snapshot rows are re-read and only deleted if still unlinked (
EditionId = 0) or still on one of the deleted book's own editions, so an id re-linked to another book in a narrow import/replace race is never deleted. Tests cover that case, and that the by-book backstop still runs on the delete-files path and not on the keep-files path. Full suite: 3045 passed.