feat(sync): propagate tags and reading status through third-party file sync (#4973)

The library.json index already carries full Book objects, but the
metadata merge overlay dropped tags and readingStatus on apply, so
tagging or marking a book Finished never reached peers syncing via
WebDAV or Google Drive (the same overlay gap that hit group membership
in #4942):

- mergeBookMetadata carries tags with the metadata LWW subset (raw
  assignment, so tag removal clears on peers) and merges readingStatus
  on its own readingStatusUpdatedAt clock, the client-side mirror of the
  native field-level server merge. This survives the asymmetric race
  where one device edits metadata after a peer changes the status;
  whole-book LWW alone would drop the status change.
- New shouldApplyRemoteBookMetadata reconciliation predicate triggers on
  either clock; the engine's index reconcile uses it so a status-only
  change propagates without a metadata edit.
- Merge-law tests (direction, removal, idempotence, asymmetric races)
  plus engine-level propagation tests for both fields.

Prerequisite for gating native sync when a third-party provider is
selected (#4380): third-party sync should reach metadata parity before
it becomes the only channel.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts

@@ -157,6 +157,69 @@ describe('FileSyncEngine metadata reconciliation (#4756)', () => {
     expect(indexedBook.groupName).toBe('Sci-Fi');
   });
 
+  test('pulls a peer readingStatus change even when local metadata is newer (#4634 semantics)', async () => {
+    // Peer marked the book Finished (status clock 250), then this device
+    // edited the title (book clock 300 > remote 200). Whole-book LWW alone
+    // would never apply the status change.
+    const local = makeLocalBook({
+      title: 'Edited Locally',
+      readingStatus: 'reading',
+      readingStatusUpdatedAt: 100,
+      updatedAt: 300,
+    });
+    const remote = makeLocalBook({
+      title: 'Stale Remote Title',
+      readingStatus: 'finished',
+      readingStatusUpdatedAt: 250,
+      updatedAt: 200,
+    });
+    const capture: { index?: RemoteLibraryIndex | null } = {};
+    const provider = makeProvider(makeRemoteIndex(remote, 200), null, capture);
+
+    const updateBookMetadata = vi.fn(async (_book: Book) => {});
+    const store = makeStore({ updateBookMetadata });
+
+    const engine = new FileSyncEngine(provider, store);
+    const result = await engine.syncLibrary([local], {
+      strategy: 'silent',
+      syncBooks: false,
+      deviceId: 'pc-device',
+    });
+
+    expect(updateBookMetadata).toHaveBeenCalledTimes(1);
+    const merged = updateBookMetadata.mock.calls[0]![0];
+    expect(merged.readingStatus).toBe('finished');
+    expect(merged.readingStatusUpdatedAt).toBe(250);
+    expect(merged.title).toBe('Edited Locally');
+    expect(result.metadataUpdated).toBe(1);
+
+    const indexedBook = capture.index!.books.find((b) => b.hash === 'h1')!;
+    expect(indexedBook.readingStatus).toBe('finished');
+    expect(indexedBook.title).toBe('Edited Locally');
+  });
+
+  test('pulls newer remote tags for a book the device already has', async () => {
+    const local = makeLocalBook({ tags: ['old'], updatedAt: 100 });
+    const remote = makeLocalBook({ tags: ['sf', 'fav'], updatedAt: 200 });
+    const capture: { index?: RemoteLibraryIndex | null } = {};
+    const provider = makeProvider(makeRemoteIndex(remote, 200), null, capture);
+
+    const updateBookMetadata = vi.fn(async (_book: Book) => {});
+    const store = makeStore({ updateBookMetadata });
+
+    const engine = new FileSyncEngine(provider, store);
+    await engine.syncLibrary([local], {
+      strategy: 'silent',
+      syncBooks: false,
+      deviceId: 'pc-device',
+    });
+
+    expect(updateBookMetadata).toHaveBeenCalledTimes(1);
+    expect(updateBookMetadata.mock.calls[0]![0].tags).toEqual(['sf', 'fav']);
+    const indexedBook = capture.index!.books.find((b) => b.hash === 'h1')!;
+    expect(indexedBook.tags).toEqual(['sf', 'fav']);
+  });
+
   test('does not overwrite local metadata when the local copy is newer', async () => {
     const local = makeLocalBook({ title: 'Local Newer', updatedAt: 300 });
     const remote = makeLocalBook({ title: 'Remote Older', updatedAt: 200 });

apps/readest-app/src/__tests__/services/sync/file/merge.test.ts

@@ -4,6 +4,7 @@ import {
   mergeBookConfig,
   mergeBookMetadata,
   isRemoteBookMetadataNewer,
+  shouldApplyRemoteBookMetadata,
 } from '@/services/sync/file/merge';
 import type { Book, BookConfig, BookNote } from '@/types/book';
 import type { RemoteBookConfig } from '@/services/sync/file/wire';
@@ -159,6 +160,138 @@ describe('mergeBookMetadata (LWW field subset)', () => {
     expect(m.groupId).toBeUndefined();
     expect(m.groupName).toBeUndefined();
   });
+
+  test('carries remote tags when remote is newer; tag removal propagates', () => {
+    const local = { hash: 'h', title: 'T', author: 'A', tags: ['old'], updatedAt: 1 } as Book;
+    const tagged = {
+      hash: 'h',
+      title: 'T',
+      author: 'A',
+      tags: ['sf', 'fav'],
+      updatedAt: 9,
+    } as Book;
+    expect(mergeBookMetadata(local, tagged).tags).toEqual(['sf', 'fav']);
+
+    const cleared = { hash: 'h', title: 'T', author: 'A', updatedAt: 9 } as Book;
+    expect(mergeBookMetadata(local, cleared).tags).toBeUndefined();
+  });
+
+  test('keeps every local metadata field (incl. tags) when local is newer', () => {
+    const local = { hash: 'h', title: 'L', author: 'L', tags: ['mine'], updatedAt: 9 } as Book;
+    const remote = { hash: 'h', title: 'R', author: 'R', tags: ['theirs'], updatedAt: 1 } as Book;
+    const m = mergeBookMetadata(local, remote);
+    expect(m.title).toBe('L');
+    expect(m.tags).toEqual(['mine']);
+    expect(m.updatedAt).toBe(9);
+  });
+
+  test('readingStatus merges on its own timestamp even when local metadata is newer (#4634 semantics)', () => {
+    // Asymmetric case: this device edited the title AFTER the peer marked
+    // the book Finished. Whole-book LWW would drop the status change.
+    const local = {
+      hash: 'h',
+      title: 'Edited locally',
+      author: 'A',
+      readingStatus: 'reading',
+      readingStatusUpdatedAt: 5,
+      updatedAt: 20,
+    } as Book;
+    const remote = {
+      hash: 'h',
+      title: 'Old title',
+      author: 'A',
+      readingStatus: 'finished',
+      readingStatusUpdatedAt: 15,
+      updatedAt: 10,
+    } as Book;
+    const m = mergeBookMetadata(local, remote);
+    expect(m.readingStatus).toBe('finished');
+    expect(m.readingStatusUpdatedAt).toBe(15);
+    expect(m.title).toBe('Edited locally');
+    expect(m.updatedAt).toBe(20);
+  });
+
+  test('keeps the local readingStatus when it is newer, even as remote metadata wins', () => {
+    const local = {
+      hash: 'h',
+      title: 'L',
+      author: 'A',
+      readingStatus: 'finished',
+      readingStatusUpdatedAt: 15,
+      updatedAt: 1,
+    } as Book;
+    const remote = {
+      hash: 'h',
+      title: 'R',
+      author: 'A',
+      readingStatus: 'reading',
+      readingStatusUpdatedAt: 5,
+      updatedAt: 9,
+    } as Book;
+    const m = mergeBookMetadata(local, remote);
+    expect(m.title).toBe('R');
+    expect(m.readingStatus).toBe('finished');
+    expect(m.readingStatusUpdatedAt).toBe(15);
+  });
+
+  test('merge is idempotent: re-merging the same remote is a no-op', () => {
+    const local = {
+      hash: 'h',
+      title: 'L',
+      author: 'A',
+      readingStatus: 'reading',
+      readingStatusUpdatedAt: 5,
+      tags: ['a'],
+      updatedAt: 1,
+    } as Book;
+    const remote = {
+      hash: 'h',
+      title: 'R',
+      author: 'A',
+      readingStatus: 'finished',
+      readingStatusUpdatedAt: 15,
+      tags: ['b'],
+      updatedAt: 9,
+    } as Book;
+    const once = mergeBookMetadata(local, remote);
+    const twice = mergeBookMetadata(once, remote);
+    expect(twice).toEqual(once);
+  });
+});
+
+describe('shouldApplyRemoteBookMetadata', () => {
+  test('true when only the readingStatus timestamp is newer', () => {
+    const local = { updatedAt: 20, readingStatusUpdatedAt: 5 } as Book;
+    const remote = { updatedAt: 10, readingStatusUpdatedAt: 15 } as Book;
+    expect(shouldApplyRemoteBookMetadata(local, remote)).toBe(true);
+  });
+
+  test('true when book metadata is newer, false when neither is', () => {
+    expect(shouldApplyRemoteBookMetadata({ updatedAt: 1 } as Book, { updatedAt: 2 } as Book)).toBe(
+      true,
+    );
+    expect(
+      shouldApplyRemoteBookMetadata(
+        { updatedAt: 2, readingStatusUpdatedAt: 2 } as Book,
+        { updatedAt: 2, readingStatusUpdatedAt: 2 } as Book,
+      ),
+    ).toBe(false);
+  });
+
+  test('tombstone on either side disqualifies', () => {
+    expect(
+      shouldApplyRemoteBookMetadata(
+        { updatedAt: 1 } as Book,
+        { updatedAt: 9, readingStatusUpdatedAt: 9, deletedAt: 9 } as Book,
+      ),
+    ).toBe(false);
+    expect(
+      shouldApplyRemoteBookMetadata(
+        { updatedAt: 1, deletedAt: 1 } as Book,
+        { updatedAt: 9 } as Book,
+      ),
+    ).toBe(false);
+  });
 });
 
 describe('isRemoteBookMetadataNewer', () => {

apps/readest-app/src/services/sync/file/engine.ts

@@ -19,7 +19,7 @@ import {
   parseRemoteLibraryIndex,
   RemoteLibraryIndex,
 } from './wire';
-import { isRemoteBookMetadataNewer, mergeBookConfig, mergeBookMetadata } from './merge';
+import { mergeBookConfig, mergeBookMetadata, shouldApplyRemoteBookMetadata } from './merge';
 
 export type SyncStrategy = 'silent' | 'send' | 'receive';
 
@@ -438,7 +438,9 @@ export class FileSyncEngine {
 
     // Metadata reconciliation for books present BOTH locally and in the shared
     // library.json (#4756). Last-writer-wins on `book.updatedAt`: when a peer's
-    // indexed copy is strictly newer, pull its title / author / cover down.
+    // indexed copy is strictly newer, pull its title / author / tags / cover
+    // down; readingStatus rides its own readingStatusUpdatedAt clock so a
+    // status-only change also triggers (see shouldApplyRemoteBookMetadata).
     // Updating allBooksMap with the merged copy also stops the final index
     // re-push from clobbering the peer's newer metadata with this device's
     // stale copy.
@@ -446,7 +448,7 @@ export class FileSyncEngine {
       const remoteNewer = remoteIndex.books.filter((rb) => {
         if (rb.deletedAt) return false;
         const local = allBooksMap.get(rb.hash);
-        return !!local && !local.deletedAt && isRemoteBookMetadataNewer(local, rb);
+        return !!local && !local.deletedAt && shouldApplyRemoteBookMetadata(local, rb);
       });
       await runPool(remoteNewer, concurrency, async (rb) => {
         const local = allBooksMap.get(rb.hash)!;

apps/readest-app/src/services/sync/file/merge.ts

@@ -87,32 +87,48 @@ export const mergeBookConfig = (
  * Overlay the user-facing metadata of `remote` onto `local`, preserving every
  * device-local / file-system field: `filePath`, `sourceTitle` (which names the
  * on-disk file), `coverImageUrl` (a device-local blob URL the caller
- * regenerates), reading progress, reading status, `hash`, `format`,
- * `createdAt`, etc.
+ * regenerates), reading progress, `hash`, `format`, `createdAt`, etc.
  *
- * Group membership (`groupId` / `groupName`) is user-facing metadata and travels
- * too, matching the native cloud sync (`transform.ts` maps `group_id` /
- * `group_name`). Without this, re-grouping a book already present on both
- * devices bumps `updatedAt` and wins LWW, yet the group change is dropped by the
- * overlay — so it never propagated for already-synced books (#4942). Assigning
- * the raw remote values (not `?? local`) lets a group removal (undefined) clear
- * membership on peers too.
+ * Two independent merge clocks, mirroring the native cloud sync:
+ *   - The metadata field subset applies only when `remote.updatedAt` is
+ *     strictly newer (whole-subset LWW). Group membership (`groupId` /
+ *     `groupName`) and `tags` travel with it — without them a re-group or
+ *     re-tag of an already-synced book bumps `updatedAt`, wins LWW, yet the
+ *     change is dropped by the overlay and never propagates (#4942).
+ *     Assigning the raw remote values (not `?? local`) lets removals
+ *     (undefined) clear on peers too.
+ *   - `readingStatus` merges on its own `readingStatusUpdatedAt` clock
+ *     (field-level LWW, the client-side mirror of the native server merge,
+ *     #4634). This survives the asymmetric race where this device edited
+ *     metadata AFTER a peer changed the status: whole-book LWW alone would
+ *     silently drop the status change.
  *
- * The scalar fields a metadata edit changes mirror `getBookWithUpdatedMetadata`
- * in `utils/book.ts`, which is the local side of the same operation. The cover
- * image is replicated separately as cover.png bytes (see the reconciliation pass
- * in the engine), so it is intentionally absent here.
+ * The metadata subset mirrors `getBookWithUpdatedMetadata` in `utils/book.ts`,
+ * the local side of the same operation. The cover image is replicated
+ * separately as cover.png bytes (see the reconciliation pass in the engine),
+ * so it is intentionally absent here.
  */
-export const mergeBookMetadata = (local: Book, remote: Book): Book => ({
-  ...local,
-  title: remote.title,
-  author: remote.author,
-  metadata: remote.metadata ?? local.metadata,
-  primaryLanguage: remote.primaryLanguage ?? local.primaryLanguage,
-  groupId: remote.groupId,
-  groupName: remote.groupName,
-  updatedAt: remote.updatedAt,
-});
+export const mergeBookMetadata = (local: Book, remote: Book): Book => {
+  const remoteMetaNewer = (remote.updatedAt ?? 0) > (local.updatedAt ?? 0);
+  const merged: Book = remoteMetaNewer
+    ? {
+        ...local,
+        title: remote.title,
+        author: remote.author,
+        metadata: remote.metadata ?? local.metadata,
+        primaryLanguage: remote.primaryLanguage ?? local.primaryLanguage,
+        groupId: remote.groupId,
+        groupName: remote.groupName,
+        tags: remote.tags,
+        updatedAt: remote.updatedAt,
+      }
+    : { ...local };
+  if ((remote.readingStatusUpdatedAt ?? 0) > (local.readingStatusUpdatedAt ?? 0)) {
+    merged.readingStatus = remote.readingStatus;
+    merged.readingStatusUpdatedAt = remote.readingStatusUpdatedAt;
+  }
+  return merged;
+};
 
 /**
  * LWW predicate for the library-index metadata reconciliation: true when the
@@ -122,3 +138,16 @@ export const mergeBookMetadata = (local: Book, remote: Book): Book => ({
  */
 export const isRemoteBookMetadataNewer = (local: Book, remote: Book): boolean =>
   !remote.deletedAt && !local.deletedAt && (remote.updatedAt ?? 0) > (local.updatedAt ?? 0);
+
+/**
+ * Reconciliation trigger: apply `mergeBookMetadata` when the remote copy is
+ * newer on EITHER clock — book metadata (`updatedAt`) or reading status
+ * (`readingStatusUpdatedAt`). Checking only `updatedAt` would skip the
+ * status-only-newer case entirely, so a peer's Finished mark could never
+ * reach a device that edited the book's metadata afterwards.
+ */
+export const shouldApplyRemoteBookMetadata = (local: Book, remote: Book): boolean =>
+  !remote.deletedAt &&
+  !local.deletedAt &&
+  ((remote.updatedAt ?? 0) > (local.updatedAt ?? 0) ||
+    (remote.readingStatusUpdatedAt ?? 0) > (local.readingStatusUpdatedAt ?? 0));