diff --git a/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts b/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts index 078bb8f1..5eb3c1d4 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts +++ b/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 }); diff --git a/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts b/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts index 32dcfac8..ca90a398 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts +++ b/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', () => { diff --git a/apps/readest-app/src/services/sync/file/engine.ts b/apps/readest-app/src/services/sync/file/engine.ts index 0210d1c9..227bcba4 100644 --- a/apps/readest-app/src/services/sync/file/engine.ts +++ b/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)!; diff --git a/apps/readest-app/src/services/sync/file/merge.ts b/apps/readest-app/src/services/sync/file/merge.ts index 23d0a6aa..e0d4d592 100644 --- a/apps/readest-app/src/services/sync/file/merge.ts +++ b/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));