Run author delete as a background command instead of inline in the HTTP request

frontend/src/Author/Details/AuthorDetails.js

@@ -259,6 +259,8 @@ class AuthorDetails extends Component {
       saveError,
       isDeleting,
       deleteError,
+      isDeletingAuthor,
+      isDeleteAuthorQueued,
       statistics = {},
       selectedMediaType,
       onMediaTypeChange,
@@ -368,6 +370,7 @@ class AuthorDetails extends Component {
             <PageToolbarButton
               label={translate('Delete')}
               iconName={icons.DELETE}
+              isDisabled={!!isDeletingAuthor}
               onPress={this.onDeleteAuthorPress}
             />
 
@@ -412,6 +415,18 @@ class AuthorDetails extends Component {
           className={styles.contentBody}
           innerClassName={styles.innerContentBody}
         >
+          {
+            isDeletingAuthor ?
+              <Alert kind={kinds.INFO}>
+                {
+                  isDeleteAuthorQueued ?
+                    'This author is queued for deletion. It will be deleted in the background once a worker is free, and it will disappear from the library when it finishes. You can leave this page.' :
+                    'This author is being deleted in the background. It will disappear from the library when it finishes. You can leave this page.'
+                }
+              </Alert> :
+              null
+          }
+
           <SwipeHeaderConnector
             className={styles.header}
             nextLink={`/author/${nextAuthor.id}`}
@@ -737,6 +752,8 @@ AuthorDetails.propTypes = {
   saveError: PropTypes.object,
   isDeleting: PropTypes.bool.isRequired,
   deleteError: PropTypes.object,
+  isDeletingAuthor: PropTypes.bool,
+  isDeleteAuthorQueued: PropTypes.bool,
   onSaveSelected: PropTypes.func.isRequired
 };
 

frontend/src/Author/Details/AuthorDetailsConnector.js

@@ -187,6 +187,18 @@ function createMapStateToProps() {
         isRenamingAuthorCommand.body.authorIds.indexOf(author.id) > -1
         );
 
+        // A large author delete runs as a background command and can sit queued behind other work for a
+        // while; surface that (and the running state) so the page is not silently unchanged.
+        const deleteAuthorCommand = _.find(commands, (command) => (
+          command.body &&
+          command.body.name === commandNames.DELETE_AUTHOR &&
+          isCommandExecuting(command) &&
+          Array.isArray(command.body.authorIds) &&
+          command.body.authorIds.indexOf(author.id) > -1
+        ));
+        const isDeletingAuthor = !!deleteAuthorCommand;
+        const isDeleteAuthorQueued = !!deleteAuthorCommand && deleteAuthorCommand.status === 'queued';
+
         const isFetching = isBooksFetching || isSeriesFetching || isBookFilesFetching;
         const isPopulated = isBooksPopulated && isSeriesPopulated && isBookFilesPopulated;
 
@@ -404,6 +416,8 @@ function createMapStateToProps() {
           isSearching,
           isRenamingFiles,
           isRenamingAuthor,
+          isDeletingAuthor,
+          isDeleteAuthorQueued,
           isFetching,
           isPopulated,
           booksError,

frontend/src/Commands/commandNames.js

@@ -6,6 +6,7 @@ export const CLEAR_BLOCKLIST = 'ClearBlocklist';
 export const CHECK_HEALTH = 'CheckHealth';
 export const CLEAR_LOGS = 'ClearLog';
 export const CUTOFF_UNMET_BOOK_SEARCH = 'CutoffUnmetBookSearch';
+export const DELETE_AUTHOR = 'DeleteAuthor';
 export const DELETE_LOG_FILES = 'DeleteLogFiles';
 export const DELETE_UPDATE_LOG_FILES = 'DeleteUpdateLogFiles';
 export const DOWNLOADED_BOOKS_SCAN = 'DownloadedBooksScan';

frontend/src/Store/Actions/Creators/createRemoveItemHandler.js

@@ -3,7 +3,14 @@ import { batchActions } from 'redux-batched-actions';
 import createAjaxRequest from 'Utilities/createAjaxRequest';
 import { removeItem, set } from '../baseActions';
 
-function createRemoveItemHandler(section, url) {
+// allowQueuedResponse: opt-in for endpoints that can respond 202 (queued, not done yet) instead
+// of always meaning "already deleted" - e.g. an author delete large enough to run as a background
+// command. Left off (default) for every other consumer of this shared factory, whose endpoints
+// have never returned anything but a completed 2xx for a delete.
+//
+// onQueued(dispatch, getState, payload): optional feedback hook for that 202 case, so the user is told the
+// delete is happening in the background instead of the page silently staying as it was.
+function createRemoveItemHandler(section, url, { allowQueuedResponse = false, onQueued = null } = {}) {
   return function(getState, payload, dispatch) {
     const {
       id,
@@ -28,7 +35,25 @@ function createRemoveItemHandler(section, url) {
 
     const promise = createAjaxRequest(ajaxOptions).request;
 
-    promise.done((data) => {
+    promise.done((data, textStatus, jqXHR) => {
+      // 202 means the delete was only queued (e.g. a large author delete run as a background
+      // command instead of inline) - the row hasn't actually been removed yet, so pulling it out
+      // of the UI now would show it as gone while it's still fully present in the database.
+      // Leave it in place; it'll disappear once the command finishes and the list next refreshes.
+      if (allowQueuedResponse && jqXHR.status === 202) {
+        dispatch(set({
+          section,
+          isDeleting: false,
+          deleteError: null
+        }));
+
+        if (onQueued) {
+          onQueued(dispatch, getState, payload);
+        }
+
+        return;
+      }
+
       dispatch(batchActions([
         set({
           section,

frontend/src/Store/Actions/authorActions.js

@@ -12,6 +12,7 @@ import translate from 'Utilities/String/translate';
 import { showMessage } from './appActions';
 import { set, update, updateItem } from './baseActions';
 import { fetchBooks } from './bookActions';
+import { fetchCommands } from './commandActions';
 import createHandleActions from './Creators/createHandleActions';
 import createRemoveItemHandler from './Creators/createRemoveItemHandler';
 import createSaveProviderHandler from './Creators/createSaveProviderHandler';
@@ -341,7 +342,28 @@ export const actionHandlers = handleThunks({
     return abortRequest;
   },
   [SAVE_AUTHOR]: createSaveProviderHandler(section, '/author', { getAjaxOptions: getSaveAjaxOptions }),
-  [DELETE_AUTHOR]: createRemoveItemHandler(section, '/author'),
+  // A large author delete runs as a background command and responds 202 (queued, not done yet)
+  // instead of a completed 2xx - see AuthorService.DeleteAuthorsSyncOrQueue.
+  [DELETE_AUTHOR]: createRemoveItemHandler(section, '/author', {
+    allowQueuedResponse: true,
+    onQueued: (dispatch, getState, payload) => {
+      const author = (getState().authors.items || []).find((item) => item.id === payload.id);
+      const name = author ? author.authorName : 'the author';
+
+      dispatch(showMessage({
+        id: `author-delete-queued-${payload.id}`,
+        name: 'AuthorDeleteQueued',
+        message: `Deleting ${name} in the background. It will be removed when a worker is free - you can leave this page.`,
+        type: 'info',
+        hideAfter: 15
+      }));
+
+      // The server publishes no update when a command is only queued (the first push to the client is
+      // when it starts), and the 202 response has no body, so pull the command list once now: that puts
+      // the queued DeleteAuthor command in the store and lets the author page show its "queued" banner.
+      dispatch(fetchCommands());
+    }
+  }),
 
   [TOGGLE_AUTHOR_MONITORED]: (getState, payload, dispatch) => {
     const {

src/Chaptarr.Api.V1/Author/AuthorController.cs

@@ -1235,8 +1235,16 @@ public async Task<ActionResult> DeleteAuthor(int id, bool deleteFiles = false, b
                 return Ok();
             }
 
-            _authorService.DeleteAuthor(id, deleteFiles, addImportListExclusion);
-            return Ok();
+            // Deleting a large author (thousands of books) inline here would block this request for
+            // as long as every synchronous BookDeletedEvent subscriber (file unlink, history, extras,
+            // ...) takes to run against all of them. AuthorService routes it through the command
+            // queue once it's big enough that inline deletion is what caused this host to lock up in
+            // the first place, and keeps everything else on the old, immediately-consistent path. See
+            // backlog: "Chaptarr: run author delete as a background Command, not inline in the HTTP
+            // request".
+            var queued = _authorService.DeleteAuthorsSyncOrQueue(new List<int> { id }, deleteFiles, addImportListExclusion);
+
+            return queued ? Accepted() : Ok();
         }
 
         [HttpPost("{id}/downloadmedia")]

src/Chaptarr.Api.V1/Author/AuthorEditorController.cs

@@ -338,11 +338,19 @@ private static bool HasCompatibleRootFolder(NzbDrone.Core.Books.Author author, L
         }
 
         [HttpDelete]
-        public object DeleteAuthor([FromBody] AuthorEditorResource resource)
+        public IActionResult DeleteAuthor([FromBody] AuthorEditorResource resource)
         {
-            _authorService.DeleteAuthors(resource.AuthorIds, false);
+            // See AuthorController.DeleteAuthor - a bulk selection can add up to just as many books
+            // as one huge author, so this goes through the same size-gated path instead of always
+            // blocking the request on every synchronous BookDeletedEvent subscriber.
+            var queued = _authorService.DeleteAuthorsSyncOrQueue(resource.AuthorIds, false);
 
-            return new { };
+            if (queued)
+            {
+                return Accepted(new { });
+            }
+
+            return Ok(new { });
         }
     }
 }

src/Chaptarr.Core.Test/Books/AuthorServiceDeleteAuthorsSyncOrQueueFixture.cs

@@ -0,0 +1,156 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using NLog;
+using NUnit.Framework;
+using NzbDrone.Common.Cache;
+using NzbDrone.Common.Messaging;
+using NzbDrone.Core.Books;
+using NzbDrone.Core.Books.Commands;
+using NzbDrone.Core.Messaging.Commands;
+using NzbDrone.Core.Messaging.Events;
+
+namespace Chaptarr.Core.Test.Books
+{
+    // AuthorService.DeleteAuthorsSyncOrQueue has already needed two rounds of behavioral fixes
+    // (the 200-book threshold, the queued-vs-inline return value driving the controllers' 202 vs
+    // 200 response) with no test coverage catching either round - covering the core decision here.
+    [TestFixture]
+    public class AuthorServiceDeleteAuthorsSyncOrQueueFixture
+    {
+        private class ThrowingProxy<T> : 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 CountOnlyBookRepositoryProxy : DispatchProxy
+        {
+            public Dictionary<int, int> Counts { get; set; } = new();
+
+            protected override object Invoke(MethodInfo targetMethod, object[] args)
+            {
+                if (string.Equals(targetMethod?.Name, nameof(IBookRepository.CountBooksByAuthorIds), StringComparison.Ordinal))
+                {
+                    return Counts;
+                }
+
+                throw new NotImplementedException($"Test proxy does not implement IBookRepository.{targetMethod?.Name}");
+            }
+        }
+
+        private class RecordingCommandQueueManagerProxy : DispatchProxy
+        {
+            public List<Command> PushedCommands { get; } = new();
+            public List<CommandPriority> PushedPriorities { get; } = new();
+
+            protected override object Invoke(MethodInfo targetMethod, object[] args)
+            {
+                if (string.Equals(targetMethod?.Name, "Push", StringComparison.Ordinal) &&
+                    args?.Length >= 1 && args[0] is Command command)
+                {
+                    PushedCommands.Add(command);
+                    PushedPriorities.Add(args.Length >= 2 && args[1] is CommandPriority priority ? priority : CommandPriority.Normal);
+                    return null;
+                }
+
+                throw new NotImplementedException($"Test proxy does not implement IManageCommandQueue.{targetMethod?.Name}");
+            }
+        }
+
+        private class RecordingAuthorRepositoryProxy : DispatchProxy
+        {
+            public Dictionary<int, Author> Authors { get; set; } = new();
+            public List<int> DeleteManyCalls { get; } = new();
+
+            protected override object Invoke(MethodInfo targetMethod, object[] args)
+            {
+                if (string.Equals(targetMethod?.Name, "Get", StringComparison.Ordinal) &&
+                    args?.Length == 1 && args[0] is IEnumerable<int> getIds)
+                {
+                    return getIds.Select(id => Authors.TryGetValue(id, out var author) ? author : null)
+                        .Where(author => author != null)
+                        .ToList();
+                }
+
+                if (string.Equals(targetMethod?.Name, "DeleteMany", StringComparison.Ordinal) &&
+                    args?.Length == 1 && args[0] is IEnumerable<int> deleteIds)
+                {
+                    DeleteManyCalls.AddRange(deleteIds);
+                    return null;
+                }
+
+                throw new NotImplementedException($"Test proxy does not implement IAuthorRepository.{targetMethod?.Name}");
+            }
+        }
+
+        private sealed class NoOpEventAggregator : IEventAggregator
+        {
+            public void PublishEvent<TEvent>(TEvent @event)
+                where TEvent : class, IEvent
+            {
+            }
+        }
+
+        [Test]
+        public void should_queue_and_return_true_when_book_count_exceeds_the_threshold()
+        {
+            var bookRepository = DispatchProxy.Create<IBookRepository, CountOnlyBookRepositoryProxy>();
+            ((CountOnlyBookRepositoryProxy)(object)bookRepository).Counts = new Dictionary<int, int> { { 1, 10113 } };
+
+            var commandQueue = DispatchProxy.Create<IManageCommandQueue, RecordingCommandQueueManagerProxy>();
+            var commandQueueRecorder = (RecordingCommandQueueManagerProxy)(object)commandQueue;
+
+            var service = new AuthorService(
+                DispatchProxy.Create<IAuthorRepository, ThrowingProxy<IAuthorRepository>>(),
+                new NoOpEventAggregator(),
+                null,
+                null,
+                commandQueue,
+                new CacheManager(),
+                bookRepository,
+                null,
+                LogManager.GetCurrentClassLogger());
+
+            var queued = service.DeleteAuthorsSyncOrQueue(new List<int> { 1 }, deleteFiles: true);
+
+            Assert.That(queued, Is.True);
+            var command = commandQueueRecorder.PushedCommands.OfType<DeleteAuthorCommand>().Single();
+            Assert.That(command.AuthorIds, Is.EquivalentTo(new[] { 1 }));
+            Assert.That(command.DeleteFiles, Is.True);
+            Assert.That(commandQueueRecorder.PushedPriorities, Is.EqualTo(new[] { CommandPriority.High }), "an interactive delete must not wait behind background searches at Normal priority");
+        }
+
+        [Test]
+        public void should_delete_inline_and_return_false_when_book_count_is_at_or_under_the_threshold()
+        {
+            var author = new Author { Id = 1, Name = "Small Author" };
+
+            var bookRepository = DispatchProxy.Create<IBookRepository, CountOnlyBookRepositoryProxy>();
+            ((CountOnlyBookRepositoryProxy)(object)bookRepository).Counts = new Dictionary<int, int> { { 1, 5 } };
+
+            var authorRepository = DispatchProxy.Create<IAuthorRepository, RecordingAuthorRepositoryProxy>();
+            var authorRepoRecorder = (RecordingAuthorRepositoryProxy)(object)authorRepository;
+            authorRepoRecorder.Authors = new Dictionary<int, Author> { { author.Id, author } };
+
+            var service = new AuthorService(
+                authorRepository,
+                new NoOpEventAggregator(),
+                null,
+                null,
+                DispatchProxy.Create<IManageCommandQueue, ThrowingProxy<IManageCommandQueue>>(),
+                new CacheManager(),
+                bookRepository,
+                null,
+                LogManager.GetCurrentClassLogger());
+
+            var queued = service.DeleteAuthorsSyncOrQueue(new List<int> { author.Id }, deleteFiles: false);
+
+            Assert.That(queued, Is.False);
+            Assert.That(authorRepoRecorder.DeleteManyCalls, Does.Contain(author.Id));
+        }
+    }
+}

src/NzbDrone.Core/Books/Commands/DeleteAuthorCommand.cs

@@ -0,0 +1,34 @@
+using System.Collections.Generic;
+using NzbDrone.Core.Messaging.Commands;
+
+namespace NzbDrone.Core.Books.Commands
+{
+    public class DeleteAuthorCommand : Command
+    {
+        public List<int> AuthorIds { get; set; }
+        public bool DeleteFiles { get; set; }
+        public bool AddImportListExclusion { get; set; }
+
+        public DeleteAuthorCommand()
+        {
+        }
+
+        public DeleteAuthorCommand(List<int> authorIds, bool deleteFiles, bool addImportListExclusion = false)
+        {
+            AuthorIds = authorIds;
+            DeleteFiles = deleteFiles;
+            AddImportListExclusion = addImportListExclusion;
+        }
+
+        public override bool SendUpdatesToClient => true;
+        public override bool IsLongRunning => true;
+
+        // Scoped to DeleteFiles so a metadata-only delete isn't lumped into the "default" disk-access
+        // group at all. When it does apply, it lands in the same "default" group every other
+        // RequiresDiskAccess command uses via Command's own default DiskAccessGroup (MoveAuthorCommand,
+        // RenameAuthorCommand, BulkMoveAuthorCommand, RescanFoldersCommand, ManualImportCommand, ...),
+        // so CommandQueue's disk-access serialization (see PR #188) already keeps this from running
+        // concurrently with a move/rename touching the same author's files.
+        public override bool RequiresDiskAccess => DeleteFiles;
+    }
+}