From 167617cce00a9f9fd5d3d2c3e50742fc66ecf01d Mon Sep 17 00:00:00 2001 From: Nicholas Wallace Date: Sat, 8 Mar 2025 10:43:27 -0700 Subject: [PATCH 1/3] Add: transaction to empty author remove --- server/routers/ApiRouter.js | 65 ++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/server/routers/ApiRouter.js b/server/routers/ApiRouter.js index 5d706e65..ea22ea69 100644 --- a/server/routers/ApiRouter.js +++ b/server/routers/ApiRouter.js @@ -420,40 +420,53 @@ class ApiRouter { async checkRemoveAuthorsWithNoBooks(authorIds) { if (!authorIds?.length) return - const bookAuthorsToRemove = ( - await Database.authorModel.findAll({ - where: [ - { - id: authorIds, - asin: { - [sequelize.Op.or]: [null, ''] + const transaction = await Database.sequelize.transaction() + try { + // Select authors with locking to prevent concurrent updates + const bookAuthorsToRemove = ( + await Database.authorModel.findAll({ + where: [ + { + id: authorIds, + asin: { + [sequelize.Op.or]: [null, ''] + }, + description: { + [sequelize.Op.or]: [null, ''] + }, + imagePath: { + [sequelize.Op.or]: [null, ''] + } }, - description: { - [sequelize.Op.or]: [null, ''] - }, - imagePath: { - [sequelize.Op.or]: [null, ''] - } - }, - sequelize.where(sequelize.literal('(SELECT count(*) FROM bookAuthors ba WHERE ba.authorId = author.id)'), 0) - ], - attributes: ['id', 'name', 'libraryId'], - raw: true - }) - ).map((au) => ({ id: au.id, name: au.name, libraryId: au.libraryId })) + sequelize.where(sequelize.literal('(SELECT count(*) FROM bookAuthors ba WHERE ba.authorId = author.id)'), 0) + ], + attributes: ['id', 'name', 'libraryId'], + raw: true, + transaction + }) + ).map((au) => ({ id: au.id, name: au.name, libraryId: au.libraryId })) - if (bookAuthorsToRemove.length) { - await Database.authorModel.destroy({ - where: { - id: bookAuthorsToRemove.map((au) => au.id) - } - }) + if (bookAuthorsToRemove.length) { + await Database.authorModel.destroy({ + where: { + id: bookAuthorsToRemove.map((au) => au.id) + }, + transaction + }) + } + + await transaction.commit() + + // Remove all book authors after completing remove from database bookAuthorsToRemove.forEach(({ id, name, libraryId }) => { Database.removeAuthorFromFilterData(libraryId, id) // TODO: Clients were expecting full author in payload but its unnecessary SocketAuthority.emitter('author_removed', { id, libraryId }) Logger.info(`[ApiRouter] Removed author "${name}" with no books`) }) + } catch (error) { + await transaction.rollback() + Logger.error(`[ApiRouter] Error removing authors: ${error.message}`) } } From 84e20e041c97271f7f3117c171d5980720665822 Mon Sep 17 00:00:00 2001 From: Nicholas Wallace Date: Sat, 8 Mar 2025 11:16:34 -0700 Subject: [PATCH 2/3] Fix: empty series delete flakiness --- server/routers/ApiRouter.js | 70 +++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 31 deletions(-) diff --git a/server/routers/ApiRouter.js b/server/routers/ApiRouter.js index ea22ea69..564b5cbc 100644 --- a/server/routers/ApiRouter.js +++ b/server/routers/ApiRouter.js @@ -392,21 +392,47 @@ class ApiRouter { async checkRemoveEmptySeries(seriesIds) { if (!seriesIds?.length) return - const series = await Database.seriesModel.findAll({ - where: { - id: seriesIds - }, - attributes: ['id', 'name', 'libraryId'], - include: { - model: Database.bookModel, - attributes: ['id'] - } - }) + const transaction = await Database.sequelize.transaction() + try { + const seriesBooksToRemove = ( + await Database.seriesModel.findAll({ + where: [ + { + id: seriesIds + }, + sequelize.where(sequelize.literal('(SELECT count(*) FROM bookSeries bs WHERE bs.seriesId = series.id)'), 0) + ], + attributes: ['id', 'name', 'libraryId'], + include: { + model: Database.bookModel, + attributes: ['id'], + required: false // Ensure it includes series even if no books exist + }, + transaction + }) + ).map((s) => ({ id: s.id, name: s.name, libraryId: s.libraryId })) - for (const s of series) { - if (!s.books.length) { - await this.removeEmptySeries(s) + if (seriesBooksToRemove.length) { + await Database.seriesModel.destroy({ + where: { + id: seriesBooksToRemove.map((s) => s.id) + }, + transaction + }) } + + await transaction.commit() + + seriesBooksToRemove.forEach((id, name, libraryId) => { + Logger.info(`[ApiRouter] Series "${name}" is now empty. Removing series`) + + // Remove series from library filter data + Database.removeSeriesFromFilterData(libraryId, id) + SocketAuthority.emitter('series_removed', { id: id, libraryId: libraryId }) + }) + } catch (error) { + await transaction.rollback() + Logger.error(`[ApiRouter] Error removing empty series: ${error.message}`) } } @@ -470,24 +496,6 @@ class ApiRouter { } } - /** - * Remove an empty series & close an open RSS feed - * @param {import('../models/Series')} series - */ - async removeEmptySeries(series) { - await RssFeedManager.closeFeedForEntityId(series.id) - Logger.info(`[ApiRouter] Series "${series.name}" is now empty. Removing series`) - - // Remove series from library filter data - Database.removeSeriesFromFilterData(series.libraryId, series.id) - SocketAuthority.emitter('series_removed', { - id: series.id, - libraryId: series.libraryId - }) - - await series.destroy() - } - async getUserListeningSessionsHelper(userId) { const userSessions = await Database.getPlaybackSessions({ userId }) return userSessions.sort((a, b) => b.updatedAt - a.updatedAt) From 8f308c6180cd7bbe742a2cfd8bcbedc558b558ac Mon Sep 17 00:00:00 2001 From: advplyr Date: Sat, 8 Mar 2025 17:47:47 -0600 Subject: [PATCH 3/3] Close RSS feeds after removing empty series --- server/routers/ApiRouter.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/server/routers/ApiRouter.js b/server/routers/ApiRouter.js index 564b5cbc..6779d0af 100644 --- a/server/routers/ApiRouter.js +++ b/server/routers/ApiRouter.js @@ -394,7 +394,7 @@ class ApiRouter { const transaction = await Database.sequelize.transaction() try { - const seriesBooksToRemove = ( + const seriesToRemove = ( await Database.seriesModel.findAll({ where: [ { @@ -412,10 +412,10 @@ class ApiRouter { }) ).map((s) => ({ id: s.id, name: s.name, libraryId: s.libraryId })) - if (seriesBooksToRemove.length) { + if (seriesToRemove.length) { await Database.seriesModel.destroy({ where: { - id: seriesBooksToRemove.map((s) => s.id) + id: seriesToRemove.map((s) => s.id) }, transaction }) @@ -423,13 +423,17 @@ class ApiRouter { await transaction.commit() - seriesBooksToRemove.forEach((id, name, libraryId) => { + seriesToRemove.forEach(({ id, name, libraryId }) => { Logger.info(`[ApiRouter] Series "${name}" is now empty. Removing series`) // Remove series from library filter data Database.removeSeriesFromFilterData(libraryId, id) SocketAuthority.emitter('series_removed', { id: id, libraryId: libraryId }) }) + // Close rss feeds - remove from db and emit socket event + if (seriesToRemove.length) { + await RssFeedManager.closeFeedsForEntityIds(seriesToRemove.map((s) => s.id)) + } } catch (error) { await transaction.rollback() Logger.error(`[ApiRouter] Error removing empty series: ${error.message}`)