diff --git a/frontend/src/Author/Details/AuthorDetails.js b/frontend/src/Author/Details/AuthorDetails.js index cf4a5cf9..cabd3e4c 100644 --- a/frontend/src/Author/Details/AuthorDetails.js +++ b/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 { @@ -412,6 +415,18 @@ class AuthorDetails extends Component { className={styles.contentBody} innerClassName={styles.innerContentBody} > + { + isDeletingAuthor ? + + { + 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.' + } + : + null + } + -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, diff --git a/frontend/src/Commands/commandNames.js b/frontend/src/Commands/commandNames.js index db9a4a0e..5ad1dd36 100644 --- a/frontend/src/Commands/commandNames.js +++ b/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'; diff --git a/frontend/src/Store/Actions/Creators/createRemoveItemHandler.js b/frontend/src/Store/Actions/Creators/createRemoveItemHandler.js index 3e2b925d..ccef6298 100644 --- a/frontend/src/Store/Actions/Creators/createRemoveItemHandler.js +++ b/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, diff --git a/frontend/src/Store/Actions/authorActions.js b/frontend/src/Store/Actions/authorActions.js index 02d7b898..c1f08cfd 100644 --- a/frontend/src/Store/Actions/authorActions.js +++ b/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 { diff --git a/src/Chaptarr.Api.V1/Author/AuthorController.cs b/src/Chaptarr.Api.V1/Author/AuthorController.cs index 2be27c43..53d2b077 100644 --- a/src/Chaptarr.Api.V1/Author/AuthorController.cs +++ b/src/Chaptarr.Api.V1/Author/AuthorController.cs @@ -1235,8 +1235,16 @@ public async Task 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 { id }, deleteFiles, addImportListExclusion); + + return queued ? Accepted() : Ok(); } [HttpPost("{id}/downloadmedia")] diff --git a/src/Chaptarr.Api.V1/Author/AuthorEditorController.cs b/src/Chaptarr.Api.V1/Author/AuthorEditorController.cs index 51b47b74..e510c517 100644 --- a/src/Chaptarr.Api.V1/Author/AuthorEditorController.cs +++ b/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 { }); } } } diff --git a/src/Chaptarr.Core.Test/Books/AuthorServiceDeleteAuthorsSyncOrQueueFixture.cs b/src/Chaptarr.Core.Test/Books/AuthorServiceDeleteAuthorsSyncOrQueueFixture.cs new file mode 100644 index 00000000..14b1e0cf --- /dev/null +++ b/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 : 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 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 PushedCommands { get; } = new(); + public List 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 Authors { get; set; } = new(); + public List 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 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 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 @event) + where TEvent : class, IEvent + { + } + } + + [Test] + public void should_queue_and_return_true_when_book_count_exceeds_the_threshold() + { + var bookRepository = DispatchProxy.Create(); + ((CountOnlyBookRepositoryProxy)(object)bookRepository).Counts = new Dictionary { { 1, 10113 } }; + + var commandQueue = DispatchProxy.Create(); + var commandQueueRecorder = (RecordingCommandQueueManagerProxy)(object)commandQueue; + + var service = new AuthorService( + DispatchProxy.Create>(), + new NoOpEventAggregator(), + null, + null, + commandQueue, + new CacheManager(), + bookRepository, + null, + LogManager.GetCurrentClassLogger()); + + var queued = service.DeleteAuthorsSyncOrQueue(new List { 1 }, deleteFiles: true); + + Assert.That(queued, Is.True); + var command = commandQueueRecorder.PushedCommands.OfType().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(); + ((CountOnlyBookRepositoryProxy)(object)bookRepository).Counts = new Dictionary { { 1, 5 } }; + + var authorRepository = DispatchProxy.Create(); + var authorRepoRecorder = (RecordingAuthorRepositoryProxy)(object)authorRepository; + authorRepoRecorder.Authors = new Dictionary { { author.Id, author } }; + + var service = new AuthorService( + authorRepository, + new NoOpEventAggregator(), + null, + null, + DispatchProxy.Create>(), + new CacheManager(), + bookRepository, + null, + LogManager.GetCurrentClassLogger()); + + var queued = service.DeleteAuthorsSyncOrQueue(new List { author.Id }, deleteFiles: false); + + Assert.That(queued, Is.False); + Assert.That(authorRepoRecorder.DeleteManyCalls, Does.Contain(author.Id)); + } + } +} diff --git a/src/NzbDrone.Core/Books/Commands/DeleteAuthorCommand.cs b/src/NzbDrone.Core/Books/Commands/DeleteAuthorCommand.cs new file mode 100644 index 00000000..28ed66fb --- /dev/null +++ b/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 AuthorIds { get; set; } + public bool DeleteFiles { get; set; } + public bool AddImportListExclusion { get; set; } + + public DeleteAuthorCommand() + { + } + + public DeleteAuthorCommand(List 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; + } +} diff --git a/src/NzbDrone.Core/Books/Repositories/BookRepository.cs b/src/NzbDrone.Core/Books/Repositories/BookRepository.cs index 50726452..869f52eb 100644 --- a/src/NzbDrone.Core/Books/Repositories/BookRepository.cs +++ b/src/NzbDrone.Core/Books/Repositories/BookRepository.cs @@ -26,6 +26,11 @@ public interface IBookRepository : IBasicRepository List GetLastBooks(IEnumerable authorIds); List GetNextBooks(IEnumerable authorIds); List GetBooksByAuthorId(int authorId); + // The default exists only for lightweight test doubles. Every production implementation must override it. + Dictionary CountBooksByAuthorIds(IEnumerable authorIds) + { + throw new NotSupportedException(); + } List GetBooksForRefresh(int authorId, IEnumerable providerIds); List GetBooksByFileIds(IEnumerable fileIds); Book FindByTitle(int authorId, string title); @@ -198,6 +203,55 @@ public List GetBooksByAuthorId(int authorId) return Query(s => s.AuthorId == authorId); } + // A COUNT(*) ... GROUP BY, not one GetBooksByAuthorId(id) per author - that would issue N + // full-row-materializing queries just to size-check a bulk delete before it can even decide + // whether to run it inline or queue it, adding real synchronous DB load on the same request + // path this is meant to keep fast. + public Dictionary CountBooksByAuthorIds(IEnumerable authorIds) + { + var idList = (authorIds ?? Enumerable.Empty()).Distinct().ToList(); + + if (!idList.Any()) + { + return new Dictionary(); + } + + using (var conn = _database.OpenConnection()) + { + var result = new Dictionary(); + + // Dapper's automatic "IN @Ids" list expansion doesn't fire against this connection + // (confirmed live: Postgres received a literal single "$1" placeholder for the whole + // array and rejected it) - build the parameter list by hand instead of relying on it. + // SQLite also has a default ~999 bind-variable limit, so batch there regardless. + var chunkSize = _database.DatabaseType == DatabaseType.SQLite + ? SqliteVariableLimit.MaxParameters + : idList.Count; + + foreach (var batch in idList.Chunk(Math.Max(chunkSize, 1))) + { + var parameters = new DynamicParameters(); + var placeholders = new List(batch.Length); + + for (var i = 0; i < batch.Length; i++) + { + var name = $"Id{i}"; + placeholders.Add("@" + name); + parameters.Add(name, batch[i]); + } + + var sql = $"SELECT \"AuthorId\" AS \"Key\", CAST(COUNT(*) AS INTEGER) AS \"Value\" FROM \"Books\" WHERE \"AuthorId\" IN ({string.Join(",", placeholders)}) GROUP BY \"AuthorId\""; + + foreach (var row in conn.Query>(sql, parameters)) + { + result[row.Key] = row.Value; + } + } + + return result; + } + } + public List GetBooksForRefresh(int authorId, IEnumerable providerIds) { // Refresh must be author-scoped. Provider-ID lookups are handled downstream by the diff --git a/src/NzbDrone.Core/Books/Services/AuthorService.cs b/src/NzbDrone.Core/Books/Services/AuthorService.cs index 82443e91..2dbd1b6c 100644 --- a/src/NzbDrone.Core/Books/Services/AuthorService.cs +++ b/src/NzbDrone.Core/Books/Services/AuthorService.cs @@ -6,6 +6,7 @@ using NLog; using NzbDrone.Common.Cache; using NzbDrone.Common.Extensions; +using NzbDrone.Core.Books.Commands; using NzbDrone.Core.Books.Events; using NzbDrone.Core.MediaCover.Commands; using NzbDrone.Core.MediaFiles; @@ -70,9 +71,14 @@ void EnsureMediaTypeMonitoring(int authorId, string mediaType) List GetAuthorBooksFromCache(int authorId); List GetAuthorIdsByMetadataProfileId(int metadataProfileId); void ClearAuthorCache(); + // The default exists only for lightweight test doubles. Every production implementation must override it. + bool DeleteAuthorsSyncOrQueue(List authorIds, bool deleteFiles, bool addImportListExclusion = false) + { + throw new NotSupportedException(); + } } - public class AuthorService : IAuthorService + public class AuthorService : IAuthorService, IExecute { private readonly IAuthorRepository _authorRepository; private readonly IEventAggregator _eventAggregator; @@ -320,6 +326,50 @@ public void DeleteAuthors(List authorIds, bool deleteFiles, bool addImportL DeleteAuthorsInternal(authorIds, deleteFiles, addImportListExclusion, false); } + // Below the threshold, delete inline and keep the old immediately-consistent contract - a + // caller that deletes then re-adds the same author expects the delete to have already + // happened by the time it gets a response, and that guarantee only breaks down once the + // command queue is actually in the picture. Only defer to the queue once an author (or a + // bulk selection) is large enough that inline deletion is what caused this host to lock up + // in the first place (Charles Dickens, 10113 books) - see PR #260. + private const int AsyncDeleteBookCountThreshold = 200; + + // Returns true if the delete was queued (caller should respond 202 Accepted, work not done + // yet), false if it ran inline before returning (caller should respond 200 OK, already done). + public bool DeleteAuthorsSyncOrQueue(List authorIds, bool deleteFiles, bool addImportListExclusion = false) + { + var distinctIds = (authorIds ?? new List()).Where(id => id > 0).Distinct().ToList(); + if (!distinctIds.Any()) + { + return false; + } + + var totalBooks = _bookRepository.CountBooksByAuthorIds(distinctIds).Values.Sum(); + + if (totalBooks <= AsyncDeleteBookCountThreshold) + { + DeleteAuthorsInternal(distinctIds, deleteFiles, addImportListExclusion, false); + return false; + } + + // High, not Normal: this is an interactive UI action (the user is watching the author page + // wait for it), and at Normal it queues behind every background MissingBookSearch already + // waiting - all command threads can be busy with rate-limited indexer searches for many + // minutes, leaving a delete "stuck" on the author page. Same treatment ManualImportCommand + // gets in CommandController. + _commandQueueManager.Push( + new DeleteAuthorCommand(distinctIds, deleteFiles, addImportListExclusion), + CommandPriority.High, + CommandTrigger.Manual); + + return true; + } + + public void Execute(DeleteAuthorCommand message) + { + DeleteAuthors(message.AuthorIds, message.DeleteFiles, message.AddImportListExclusion); + } + private List DeleteAuthorsInternal( List authorIds, bool deleteFiles,