Run author delete as a background command instead of inline in the HTTP request - #260
Open
jordanfelle wants to merge 8 commits into
Open
jordanfelle wants to merge 8 commits into
jordanfelle wants to merge 8 commits into
Conversation
…TP request AuthorController.DeleteAuthor and AuthorEditorController.DeleteAuthor (bulk) called AuthorService.DeleteAuthor(s) directly inside the request handler. That method loops every book belonging to the author(s) and publishes BookDeletedEvent synchronously per book, which still has several fully-synchronous IHandle<BookDeletedEvent> subscribers doing real work per book: MediaFileService unlinking files (over this host's NFS-mounted media), ExtraFileService, DeferredCoverDownloadService, TrackedDownloadService, HistoryService. PR Chaptarr#259 already moved the Discord/webhook notification half of this off the sync path, but for a small author the rest was never noticeable. For a massive author (Charles Dickens, 10113 books) it's a different story: one HTTP request doing 10k+ sequential rounds of disk I/O and DB writes with nothing yielding control back, which hangs the request long enough to look like the whole instance froze. Add DeleteAuthorCommand and have AuthorService execute it the same way every other multi-book operation already runs (BulkMoveAuthorCommand, BulkRefreshAuthorCommand, ...): both delete endpoints now push the command and return immediately, and the actual deletion happens on the command queue where it shows up in Activity like any other long-running job instead of hanging the request. RequiresDiskAccess is instance-scoped to DeleteFiles so a metadata-only delete isn't serialized behind unrelated disk work in the "default" disk-access group (see PR Chaptarr#188). The readdAuthor branch of AuthorController.DeleteAuthor is untouched - it calls the separate DeleteAuthorForReadd path and depends on that finishing synchronously before the subsequent AddAuthorAsync call in the same request, per the existing comment there.
…de, correct disk-access claim Three problems found in review of Chaptarr#260: 1. Queuing every delete unconditionally broke the old immediately-consistent contract: a client that deletes an author then re-adds it before the queued DeleteAuthorCommand actually runs would have the re-add silently wiped out once the stale delete finally executes, since AuthorLibraryService.AddAuthorAsync reuses the still-present row rather than treating it as new. Add AuthorService.DeleteAuthorsSyncOrQueue: below a 200-book threshold, delete inline (same as before this PR, so the old guarantee holds for the overwhelming majority of authors); only defer to the command queue once a delete is big enough that inline deletion is what caused the lockup in the first place. Both controllers now call this instead of always pushing the command. 2. The DeleteAuthorCommand.RequiresDiskAccess comment claimed it would keep this delete from overlapping concurrent moves/renames, but no move/rename command in the codebase opts into the disk-access group - so no such exclusion actually happens yet. Corrected the comment to state what's actually enforced today instead of overclaiming. 3. DeleteAuthor kept returning 200 OK even when the work was only queued, unlike the sibling DownloadAuthorMedia endpoint which returns 202 for the same push-then-return shape. DeleteAuthorsSyncOrQueue now reports whether it queued or ran inline, and both controllers return 202 Accepted when queued, 200 OK when actually done before responding.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
…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.
…ize-gate count query Three problems found in review of Chaptarr#260, one filed as a follow-up ticket: 1. createRemoveItemHandler (single-author delete) optimistically dispatched removeItem on any 2xx response, but DeleteAuthorsSyncOrQueue now returns 202 when the delete is only queued - the row is still fully present in the DB at that point. The author would vanish from the UI immediately while its 10k+ book rows were still being processed in the background; refreshing before the command finished would bring it back, and an immediate re-add by the same provider ID could hit a duplicate/conflict error against the not-yet-deleted row. Now checks jqXHR.status === 202 and skips the optimistic removeItem in that case - the item disappears naturally once the command finishes and the list next refreshes. 2. DeleteAuthorsSyncOrQueue's size check ran distinctIds.Sum(id => _bookRepository.GetBooksByAuthorId(id).Count) - one full-row-materializing query per author ID just to decide whether to run the delete inline or queue it, adding real synchronous DB load (and full Book row payloads, not just counts) to the same request path this exists to keep fast. Added IBookRepository.CountBooksByAuthorIds, a single grouped COUNT(*) query (chunked for SQLite's bind-variable limit), and switched the size check to use it. 3. Bulk delete's authorIndexActions handler unconditionally clears deleteError on any 2xx and relies on SignalR for actual list removal - already correct for the "don't remove until it's really gone" concern the single-delete handler had, so left as-is. A queued bulk delete that later fails asynchronously surfaces that failure via Activity/command history rather than the delete modal, same as every other command-based operation in this app (BulkRefreshAuthor, etc.) - not a new inconsistency this PR introduces. Filed as a backlog ticket rather than fixed here: the readdAuthor (purge & re-add) branch of AuthorController.DeleteAuthor still calls DeleteAuthorForReadd synchronously with no size gate, so purging and re-adding a massive author still reproduces the original freeze via that action. Not a quick fix - DeleteAuthorForReadd's synchronous completion is a hard dependency of the same-request AddAuthorAsync call that follows it, so queuing it requires restructuring that hand-off, not just copying DeleteAuthorsSyncOrQueue's threshold check.
…he 202 UI fix, add coverage
Fixed:
1. DeleteAuthorCommand.RequiresDiskAccess's comment claimed no other command shared its
disk-access group - wrong. Command's own base class default (DiskAccessGroup =>
RequiresDiskAccess ? "default" : null) already puts MoveAuthorCommand, RenameAuthorCommand,
BulkMoveAuthorCommand, RescanFoldersCommand, ManualImportCommand, and others in the same
"default" group this command uses, so CommandQueue's disk-access serialization already
protects against a concurrent move/rename. Corrected the comment.
2. The 202-handling added to createRemoveItemHandler last round was author-delete-specific
behavior baked into a generic factory shared by ~20 unrelated delete flows (books, download
clients, tags, cancel-command, ...) - any of them starting to legitimately return 202 for an
unrelated reason would silently inherit "leave it in the list" behavior with no guarantee a
SignalR removal broadcast exists for that resource. Made it opt-in via an
{ allowQueuedResponse: true } option, and only authorActions' DELETE_AUTHOR registration
passes it - every other consumer is completely unaffected.
3. Added AuthorServiceDeleteAuthorsSyncOrQueueFixture covering the core decision this method
makes (queue + return true above the threshold, delete inline + return false at/below it) -
this exact logic had already needed two rounds of behavioral fixes (the threshold check itself,
then the queued-vs-inline return value driving the controllers' 202/200 split) with no test
catching either.
Accepted as a documented tradeoff, not fixed: the size-gate's book count and the delete that
follows it aren't in one atomic transaction, so a concurrent import adding books to a selected
author between the count and the delete could in theory push it over the threshold without the
gate seeing it. Worst case this falls back to the original synchronous behavior for that one
request - not data loss, just the pre-PR behavior in a narrow race window. Closing it fully would
mean locking rows across the decision and the delete, which is a larger change than this size
gate calls for.
…use to Postgres Confirmed live on this host: deleting author 2049 (5420 books) threw Npgsql.PostgresException: 42601 syntax error at or near "$1" - Dapper's automatic "IN @ids" list-expansion (relied on to turn one array parameter into IN (@ids1,@ids2,...)) did not fire against this connection, so Postgres received a literal single positional parameter directly after IN, which is invalid syntax. This broke every author delete, not just large ones, since DeleteAuthorsSyncOrQueue calls this method first regardless of author size. Stopped relying on Dapper's magic list expansion and build the parameterized IN clause by hand (named @id0, @id1, ... parameters via DynamicParameters, joined into the SQL text directly). Verified the resulting placeholder shape against the live Postgres database directly via a PREPARE/EXECUTE before deploying. The existing unit test for this code path uses a mocked repository and doesn't touch a real database, so it passed before this fix and continues to pass after - it was never going to catch a real-SQL-dialect bug like this one. Noted as a gap: this class of bug (works against every in-process test double, breaks against the actual Postgres driver) isn't something the current test suite can catch at all.
3 of 4 tasks
Author
Live verification on production, plus a bug found and fixedFirst deploy attempt hit a real production bug: Fixed in 07afe0e by building the parameterized After the fix, retried against Daniel Defoe, 5420 books, deleteFiles=true:
Noted in the commit: the existing unit test for this method uses a mocked repository and never touches a real database, so it passed before and after this fix - it couldn't have caught a real-SQL-dialect bug like this one. Worth keeping in mind for future changes to raw-SQL repository methods. |
…ind background searches DeleteAuthorsSyncOrQueue pushed DeleteAuthorCommand at Normal priority. A delete is an interactive UI action (the user is watching the author page wait for it), but at Normal it queues behind every background MissingBookSearch already waiting, and all command threads can be busy with rate-limited indexer searches for many minutes. Seen live: deleting a 208-book author sat at "queued" on the author page for 10+ minutes with 10 MissingBookSearch commands holding every thread and 6 more queued ahead of it. Push it at High, the same treatment ManualImportCommand gets in CommandController. It still needs a free thread, so it starts as soon as one search finishes rather than after the whole backlog. Test: the queueing test now also asserts the command is pushed at High (fails at Normal).
A large author delete runs as a durable background command and can sit queued behind other work,
but the UI gave no sign of it: after confirming Delete the author page just stayed as it was.
- createRemoveItemHandler: an onQueued(dispatch, getState, payload) hook, called on the 202
"queued" response (opt-in alongside allowQueuedResponse; other consumers unchanged).
- authorActions: on a queued author delete, show an info toast ("Deleting <name> in the
background. It will be removed when a worker is free - you can leave this page.").
- AuthorDetailsConnector: detect a queued/started DeleteAuthor command whose authorIds include
this author (mirrors the existing RenameAuthor tracking), passed as isDeletingAuthor /
isDeleteAuthorQueued.
- AuthorDetails: while it is queued or running, show an info banner (queued vs running wording)
and disable the toolbar Delete button so it cannot be submitted twice.
- commandNames: DELETE_AUTHOR.
…ally shows Adversarial review of the delete-feedback commit: CommandQueueManager.Push publishes no CommandUpdatedEvent (the first push to the client is when the command starts) and the 202 response has no body, so the queued DeleteAuthor command never reached the UI store and the 'queued' banner state could only appear after a reload. Dispatch fetchCommands() from the onQueued hook so the queued command is in the store right away; SignalR then carries queued -> started -> completed.
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
AuthorController.DeleteAuthorandAuthorEditorController.DeleteAuthor(bulk) both calledAuthorService.DeleteAuthor(s)directly inside the HTTP request handler.BookDeletedEventsynchronously per book. SeveralIHandle<BookDeletedEvent>subscribers still do real synchronous work per book:MediaFileService(NFS file unlink),ExtraFileService,DeferredCoverDownloadService,TrackedDownloadService,HistoryService. Dispatch OnBookDelete/OnAuthorDelete notifications off the delete path #259 already moved the Discord/webhook notification piece off this path, but that's invisible for a small author.DeleteAuthorCommandand hasAuthorServiceexecute it the same way every other multi-book operation already runs (BulkMoveAuthorCommand,BulkRefreshAuthorCommand, ...). Both delete endpoints now push the command and return immediately; the deletion runs on the command queue and shows up in Activity like any other long-running job.RequiresDiskAccessis scoped to the command instance'sDeleteFilesflag, so a metadata-only delete isn't serialized behind unrelated disk-access-group work (fix(commands): stop disk-access groups from globally blocking each other #188).readdAuthorbranch ofAuthorController.DeleteAuthoris untouched - it uses the separateDeleteAuthorForReaddpath, which needs to finish synchronously before the subsequentAddAuthorAsynccall in that same request.Test plan
dotnet buildofChaptarr.Api.V1(and its dependency chain) succeedsUpdate: run the queued delete at High priority
Found live after deploying: deleting a 208-book author sat at "queued" on the author page for 10+ minutes. All 10 command threads were held by rate-limited
MissingBookSearchcommands (6 more queued ahead), and the delete was pushed atNormal, so it waited behind the whole backlog. A delete is an interactive UI action, so it is now pushed atHigh(same treatmentManualImportCommandgets inCommandController). It still needs a free thread, so it starts as soon as one search finishes rather than after the backlog. The queueing test now assertsHigh(it fails atNormal). Full suite: 3037 passed.Update: tell the user it is happening in the background
Queueing the delete is durable (it survived a restart and ran), but the UI gave no sign of it - after confirming Delete the author page just stayed as it was, which looks like a failed delete. Now:
createRemoveItemHandlergains an opt-inonQueuedhook; other consumers unchanged).DeleteAuthorcommand for that author is queued or running, the author page shows an info banner (queued vs running wording) and the toolbar Delete button is disabled, so it cannot be submitted twice. Tracking mirrors the existingRenameAuthorhandling.Testing: syntax-checked with esbuild only; this repo has no frontend tests and I have not run the webpack build in isolation or checked it in a browser yet.
Follow-up from adversarial review of the feedback change: the server publishes no update when a command is merely queued (
CommandQueueManager.Push-> nothing; the first client push isCommandExecutorafter start) and the 202 has no body, so the queued command never reached the UI store and the "queued" banner could only appear after a reload. TheonQueuedhook now also dispatchesfetchCommands(), so the queued command is in the store right away and SignalR carries queued -> started -> completed. Still frontend-only and syntax/webpack-checked, not verified in a browser.