diff --git a/server/src/modules/hardcover/hardcover-book-match.service.test.ts b/server/src/modules/hardcover/hardcover-book-match.service.test.ts index 0361c13e1..c58df8f58 100644 --- a/server/src/modules/hardcover/hardcover-book-match.service.test.ts +++ b/server/src/modules/hardcover/hardcover-book-match.service.test.ts @@ -129,6 +129,82 @@ describe('HardcoverBookMatchService', () => { ); }); + it('keeps a cached edition that sits past the selection cap instead of re-pointing it', async () => { + mockRepo.findBookState.mockResolvedValue({ + hardcoverBookId: 100, + hardcoverEditionId: 9999, + matchError: null, + }); + const cappedWindow = Array.from({ length: 50 }, (_, index) => ({ + id: index + 1, + pages: 410, + isbn_13: index === 0 ? '9781234567890' : null, + isbn_10: null, + audio_seconds: null, + })); + mockClient.query.mockImplementation((_userId, _token, query: string) => { + if (query.includes('FindBookEditionMetricsById')) { + return { books: [{ id: 100, editions: [{ id: 9999, pages: 250, isbn_13: null, isbn_10: null, audio_seconds: null }] }] }; + } + return { books: [{ id: 100, editions: cappedWindow }] }; + }); + + const result = await makeService().matchBook(1, 'tok', { ...baseBook, isbn13: '9781234567890', pageCount: 410 }); + + expect(result).toEqual({ + hardcoverBookId: 100, + hardcoverEditionId: 9999, + editionPages: 250, + editionAudioSeconds: null, + matchMethod: 'cached', + }); + expect(mockClient.query).toHaveBeenCalledTimes(1); + expect(mockClient.query).toHaveBeenCalledWith(1, 'tok', expect.stringContaining('FindBookEditionMetricsById'), { id: 100, editionId: 9999 }); + expect(mockRepo.upsertBookState).not.toHaveBeenCalled(); + }); + + it('falls back to the capped list only after the cached edition is missing from the book', async () => { + mockRepo.findBookState.mockResolvedValue({ + hardcoverBookId: 100, + hardcoverEditionId: 200, + matchError: null, + }); + mockClient.query.mockImplementation((_userId, _token, query: string) => { + if (query.includes('FindBookEditionMetricsById')) { + return { books: [{ id: 100, editions: [] }] }; + } + return { books: [{ id: 100, editions: [{ id: 301, pages: 410, isbn_13: null, isbn_10: null, audio_seconds: null }] }] }; + }); + + const result = await makeService().matchBook(1, 'tok', { ...baseBook, pageCount: 410 }); + + expect(result?.hardcoverEditionId).toBe(301); + const queries = mockClient.query.mock.calls.map((call) => call[2] as string); + expect(queries).toHaveLength(2); + expect(queries[0]).toContain('FindBookEditionMetricsById'); + expect(queries[1]).toContain('FindBookEditionsById'); + expect(mockRepo.upsertBookState).toHaveBeenCalledWith( + expect.objectContaining({ hardcoverBookId: 100, hardcoverEditionId: 301, matchMethod: 'cached' }), + ); + }); + + it('skips the by-id lookup when no edition is cached', async () => { + mockRepo.findBookState.mockResolvedValue({ + hardcoverBookId: 100, + hardcoverEditionId: null, + matchError: null, + }); + mockClient.query.mockResolvedValue({ + books: [{ id: 100, editions: [{ id: 301, pages: 410, isbn_13: null, isbn_10: null, audio_seconds: null }] }], + }); + + const result = await makeService().matchBook(1, 'tok', baseBook); + + expect(result?.hardcoverEditionId).toBe(301); + expect(mockClient.query).toHaveBeenCalledTimes(1); + expect(mockClient.query).toHaveBeenCalledWith(1, 'tok', expect.stringContaining('FindBookEditionsById'), { id: 100 }); + }); + it('re-queries when cached state has match error', async () => { mockRepo.findBookState.mockResolvedValue({ hardcoverBookId: null, matchError: 'no_match' }); mockClient.query.mockResolvedValue({ books: [{ id: 99, editions: [{ id: 55 }] }] }); diff --git a/server/src/modules/hardcover/hardcover-book-match.service.ts b/server/src/modules/hardcover/hardcover-book-match.service.ts index 2424c2d90..375d5f53f 100644 --- a/server/src/modules/hardcover/hardcover-book-match.service.ts +++ b/server/src/modules/hardcover/hardcover-book-match.service.ts @@ -97,6 +97,18 @@ query FindBookEditionsById($id: Int!) { } }`; +// Resolves the cached edition by id, scoped to the book, instead of checking membership in the +// capped list above: popular works have hundreds of editions, and a pick past the cap is still +// the user's edition. Without this, the next sync would silently replace it via pickBestEdition. +const FIND_BOOK_EDITION_METRICS_BY_ID_QUERY = ` +query FindBookEditionMetricsById($id: Int!, $editionId: Int!) { + books(where: { id: { _eq: $id } }, limit: 1) { + id + editions(where: { id: { _eq: $editionId } }, limit: 1) {${EDITION_FIELDS} + } + } +}`; + const AUDIO_FORMATS = new Set(['m4b', 'm4a', 'mp3', 'aax', 'aacx', 'aac', 'flac', 'ogg', 'opus', 'wma', 'mka']); interface HardcoverEdition { @@ -448,19 +460,18 @@ export class HardcoverBookMatchService { cachedEditionId: number | null, ): Promise<{ hardcoverEditionId: number | null; editionPages: number | null; editionAudioSeconds: number | null }> { try { - const data = await this.client.query(userId, token, FIND_BOOK_EDITIONS_BY_HARDCOVER_ID_QUERY, { - id: hardcoverBookId, - }); - const hardcoverBook = data.books?.[0]; - if (!hardcoverBook) { - return { hardcoverEditionId: cachedEditionId, editionPages: null, editionAudioSeconds: null }; - } - - const editions = hardcoverBook.editions ?? []; - // A missing progress metric is not a reason to silently re-point the user's edition. if (cachedEditionId != null) { - const cachedEdition = editions.find((edition) => edition.id === cachedEditionId); + const data = await this.client.query(userId, token, FIND_BOOK_EDITION_METRICS_BY_ID_QUERY, { + id: hardcoverBookId, + editionId: cachedEditionId, + }); + const hardcoverBook = data.books?.[0]; + if (!hardcoverBook) { + return { hardcoverEditionId: cachedEditionId, editionPages: null, editionAudioSeconds: null }; + } + + const cachedEdition = hardcoverBook.editions?.find((edition) => edition.id === cachedEditionId); if (cachedEdition) { return { hardcoverEditionId: cachedEditionId, @@ -470,7 +481,16 @@ export class HardcoverBookMatchService { } } - const edition = this.pickBestEdition(editions, book); + // No usable cached edition on this book: fall back to the best edition among the capped list. + const data = await this.client.query(userId, token, FIND_BOOK_EDITIONS_BY_HARDCOVER_ID_QUERY, { + id: hardcoverBookId, + }); + const hardcoverBook = data.books?.[0]; + if (!hardcoverBook) { + return { hardcoverEditionId: cachedEditionId, editionPages: null, editionAudioSeconds: null }; + } + + const edition = this.pickBestEdition(hardcoverBook.editions ?? [], book); if (!edition) { return { hardcoverEditionId: cachedEditionId, editionPages: null, editionAudioSeconds: null }; } diff --git a/server/src/modules/hardcover/hardcover-sync.service.test.ts b/server/src/modules/hardcover/hardcover-sync.service.test.ts index 9cbd3a267..055fa3a83 100644 --- a/server/src/modules/hardcover/hardcover-sync.service.test.ts +++ b/server/src/modules/hardcover/hardcover-sync.service.test.ts @@ -1,6 +1,12 @@ import { Logger } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { BookService } from '../book/book.service'; +import { HardcoverBookMatchService } from './hardcover-book-match.service'; +import { HardcoverClientService } from './hardcover-client.service'; +import { HardcoverRepository } from './hardcover.repository'; +import { HardcoverSettingsService } from './hardcover-settings.service'; import { HardcoverSyncService } from './hardcover-sync.service'; const mockRepo = { @@ -41,6 +47,20 @@ function makeService() { return new HardcoverSyncService(mockRepo as any, mockClient as any, mockMatchService as any, mockSettingsService as any, mockBookService as any); } +async function makeTestingService() { + const module = await Test.createTestingModule({ + providers: [ + HardcoverSyncService, + { provide: HardcoverRepository, useValue: mockRepo }, + { provide: HardcoverClientService, useValue: mockClient }, + { provide: HardcoverBookMatchService, useValue: mockMatchService }, + { provide: HardcoverSettingsService, useValue: mockSettingsService }, + { provide: BookService, useValue: mockBookService }, + ], + }).compile(); + return module.get(HardcoverSyncService); +} + const defaultSettings = { tokenConfigured: true, enabled: true, @@ -898,6 +918,69 @@ describe('HardcoverSyncService', () => { }); describe('setEdition', () => { + it.each(['single book', 'sync all'])('keeps a new manual edition after an older %s sync finishes', async (source) => { + let state = { userId: 1, bookId: 1, hardcoverBookId: 10, hardcoverEditionId: 20, matchError: null, lastSyncedAt: null as Date | null }; + let releaseFirstMatch!: () => void; + let signalFirstMatch!: () => void; + const firstMatchStarted = new Promise((resolve) => { + signalFirstMatch = resolve; + }); + const firstMatchBlocked = new Promise((resolve) => { + releaseFirstMatch = resolve; + }); + + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockRepo.findBookSyncData.mockResolvedValue({ ...readingBook, startedAt: null, progress: null }); + mockRepo.findSyncableBooks.mockResolvedValue([{ ...readingBook, startedAt: null, progress: null }]); + mockRepo.findBookState.mockImplementation(() => Promise.resolve(state)); + mockRepo.upsertBookState.mockImplementation((data) => { + state = { ...state, ...data }; + return Promise.resolve(state); + }); + mockMatchService.findEditionForBook.mockResolvedValue({ id: 30, format: 'Physical Book' }); + mockMatchService.matchBook.mockImplementationOnce(async () => { + signalFirstMatch(); + await firstMatchBlocked; + return { hardcoverBookId: 10, hardcoverEditionId: 20, editionPages: 300, matchMethod: 'cached' }; + }); + mockMatchService.matchBook.mockImplementationOnce(() => + Promise.resolve({ + hardcoverBookId: 10, + hardcoverEditionId: state.hardcoverEditionId, + editionPages: 300, + matchMethod: 'cached', + }), + ); + mockClient.query.mockResolvedValue({ insert_user_book: { user_book: { id: 55 } }, update_user_book: { error: null } }); + + const service = await makeTestingService(); + if (source === 'single book') { + const earlierSync = service.syncBook(1, 1); + await firstMatchStarted; + const choice = service.setEdition(1, 1, 30); + await new Promise((resolve) => setImmediate(resolve)); + expect(mockMatchService.findEditionForBook).not.toHaveBeenCalled(); + releaseFirstMatch(); + await expect(earlierSync).resolves.toBe('synced'); + await expect(choice).resolves.toEqual({ success: true }); + } else { + await service.syncAll(1); + await firstMatchStarted; + const choice = service.setEdition(1, 1, 30); + await new Promise((resolve) => setImmediate(resolve)); + expect(mockMatchService.findEditionForBook).not.toHaveBeenCalled(); + releaseFirstMatch(); + await expect(choice).resolves.toEqual({ success: true }); + } + + expect(state.hardcoverEditionId).toBe(30); + expect(mockMatchService.matchBook).toHaveBeenCalledTimes(2); + const syncedEditions = mockClient.query.mock.calls + .filter((call) => (call[2] as string).includes('mutation UpdateUserBook')) + .map((call) => call[3].object.edition_id); + expect(syncedEditions).toEqual([20, 30]); + }); + it('throws when the book has no matched hardcoverBookId', async () => { mockSettingsService.getTokenForUser.mockResolvedValue('tok'); mockRepo.findBookState.mockResolvedValue(null); diff --git a/server/src/modules/hardcover/hardcover-sync.service.ts b/server/src/modules/hardcover/hardcover-sync.service.ts index b4a211b09..ff833f665 100644 --- a/server/src/modules/hardcover/hardcover-sync.service.ts +++ b/server/src/modules/hardcover/hardcover-sync.service.ts @@ -156,6 +156,7 @@ export class HardcoverSyncService { private readonly cancelRequests = new Set(); private readonly syncStatusEvents = new Subject<{ userId: number; status: HardcoverActiveSyncStatus | null }>(); private readonly activeSyncs = new Map(); + private readonly bookSyncLocks = new Map>(); private syncRunCounter = 0; constructor( @@ -167,6 +168,10 @@ export class HardcoverSyncService { ) {} async syncBook(userId: number, bookId: number): Promise { + return this.withBookSyncLock(userId, bookId, () => this.syncBookUnlocked(userId, bookId)); + } + + private async syncBookUnlocked(userId: number, bookId: number): Promise { const token = await this.settingsService.getTokenForUser(userId); if (!token) return 'skipped'; @@ -180,6 +185,24 @@ export class HardcoverSyncService { return this.syncSingleBook(userId, token, book, state); } + private async withBookSyncLock(userId: number, bookId: number, operation: () => Promise): Promise { + const key = `${userId}:${bookId}`; + const previous = this.bookSyncLocks.get(key); + let release!: () => void; + const current = new Promise((resolve) => { + release = resolve; + }); + this.bookSyncLocks.set(key, current); + + if (previous) await previous; + try { + return await operation(); + } finally { + if (this.bookSyncLocks.get(key) === current) this.bookSyncLocks.delete(key); + release(); + } + } + async syncAll(userId: number): Promise { const existing = this.activeSyncs.get(userId); if (existing) { @@ -353,6 +376,10 @@ export class HardcoverSyncService { } async setEdition(userId: number, bookId: number, editionId: number): Promise { + return this.withBookSyncLock(userId, bookId, () => this.setEditionUnlocked(userId, bookId, editionId)); + } + + private async setEditionUnlocked(userId: number, bookId: number, editionId: number): Promise { const { token, hardcoverBookId } = await this.requireMatchedBook(userId, bookId); const edition = await this.matchService.findEditionForBook(userId, token, hardcoverBookId, editionId); @@ -370,7 +397,7 @@ export class HardcoverSyncService { lastSyncedAt: null, }); - await this.syncBook(userId, bookId); + await this.syncBookUnlocked(userId, bookId); await this.bookService.setHardcoverEditionIdIfEmpty(bookId, String(editionId)).catch((err) => { this.logger.warn( @@ -406,14 +433,11 @@ export class HardcoverSyncService { return; } - const state = await this.repo.findBookState(userId, book.bookId); - if (!this.isBookInSyncScope(settings, book, state)) { - skipped++; - this.emitProgress(userId, synced); - continue; - } - - const result = await this.syncSingleBook(userId, token, book, state); + const result = await this.withBookSyncLock(userId, book.bookId, async () => { + const state = await this.repo.findBookState(userId, book.bookId); + if (!this.isBookInSyncScope(settings, book, state)) return 'skipped'; + return this.syncSingleBook(userId, token, book, state); + }); if (result === 'synced') synced++; else if (result === 'skipped') skipped++; else failed++;