From 04ba2ffbcfa58218855a69700c8ebc736b4e0256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois-Xavier=20Payet?= Date: Sat, 26 Sep 2026 17:46:03 +0200 Subject: [PATCH] fix(metadata): keep a pinned hardcover edition past the edition cap (#1549) * fix(metadata): keep a pinned hardcover edition past the edition cap resolveCachedMatch looked the cached edition up in the capped 50-edition list of the work, so on works with more editions (Harry Potter, Le Petit Prince, Dune) a manually chosen edition past the cap was never found and pickBestEdition silently re-pointed the book on the very sync that setEdition triggers. Resolve the cached edition by id, scoped to the book, and only fall back to the capped list when Hardcover no longer has that edition on the book. Fixes #1252 * fix(metadata): serialize Hardcover edition changes with sync Keep a manual edition choice from being overwritten by an older in-flight sync. Queue sync and edition operations per user and book so the manual choice runs after earlier work. Ref #1252 --------- Co-authored-by: neon <18559658+neonsolstice@users.noreply.github.com> --- .../hardcover-book-match.service.test.ts | 76 +++++++++++++++++ .../hardcover/hardcover-book-match.service.ts | 44 +++++++--- .../hardcover/hardcover-sync.service.test.ts | 83 +++++++++++++++++++ .../hardcover/hardcover-sync.service.ts | 42 ++++++++-- 4 files changed, 224 insertions(+), 21 deletions(-) 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++;