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>

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 }] }] });

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<BooksQueryResult>(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<BooksQueryResult>(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<BooksQueryResult>(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 };
       }

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<void>((resolve) => {
+        signalFirstMatch = resolve;
+      });
+      const firstMatchBlocked = new Promise<void>((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<void>((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<void>((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);

server/src/modules/hardcover/hardcover-sync.service.ts

@@ -156,6 +156,7 @@ export class HardcoverSyncService {
   private readonly cancelRequests = new Set<number>();
   private readonly syncStatusEvents = new Subject<{ userId: number; status: HardcoverActiveSyncStatus | null }>();
   private readonly activeSyncs = new Map<number, HardcoverActiveSyncStatus>();
+  private readonly bookSyncLocks = new Map<string, Promise<void>>();
   private syncRunCounter = 0;
 
   constructor(
@@ -167,6 +168,10 @@ export class HardcoverSyncService {
   ) {}
 
   async syncBook(userId: number, bookId: number): Promise<HardcoverSyncBookResult> {
+    return this.withBookSyncLock(userId, bookId, () => this.syncBookUnlocked(userId, bookId));
+  }
+
+  private async syncBookUnlocked(userId: number, bookId: number): Promise<HardcoverSyncBookResult> {
     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<T>(userId: number, bookId: number, operation: () => Promise<T>): Promise<T> {
+    const key = `${userId}:${bookId}`;
+    const previous = this.bookSyncLocks.get(key);
+    let release!: () => void;
+    const current = new Promise<void>((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<number> {
     const existing = this.activeSyncs.get(userId);
     if (existing) {
@@ -353,6 +376,10 @@ export class HardcoverSyncService {
   }
 
   async setEdition(userId: number, bookId: number, editionId: number): Promise<SetHardcoverEditionResult> {
+    return this.withBookSyncLock(userId, bookId, () => this.setEditionUnlocked(userId, bookId, editionId));
+  }
+
+  private async setEditionUnlocked(userId: number, bookId: number, editionId: number): Promise<SetHardcoverEditionResult> {
     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++;