From df34de1c38b2f7aadd2f7c6c284918e33b1d4fbb Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Thu, 2 Jul 2026 23:28:42 +0900 Subject: [PATCH] fix(sync): WebDAV upload-after-enable and deletion propagation (#4856, #4860) (#4892) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): upload book files when Upload Book Files is enabled after first sync (#4856) Incremental sync decided what to push purely from `isLocalNewer` (`book.updatedAt` vs the shared index). A book's config/cover change over time, but its FILE is immutable per hash and only needs uploading once. After a first sync with "Upload Book Files" off, toggling it on never bumped `book.updatedAt`, so the book was skipped and its file never reached the remote. Record which book FILES are already on the remote in library.json (`uploadedHashes`) and split the push decision: config/cover stay gated on the incremental "changed locally" cursor, while a file is (re)uploaded only when syncBooks is on and its hash isn't recorded yet. This keeps an incremental "Sync now" O(changed) — once a file is recorded, later syncs skip it with no per-book HEAD probe, so large libraries don't pay an O(library) cost on every sync. Full Sync bypasses the record as an escape hatch for out-of-band drift. The record is additive and optional, so an old client that rewrites the index just drops it and the next new-client sync re-verifies each file once and re-records it. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sync): propagate WebDAV book deletions to peers and the server (#4860) A book deleted on one device only tombstoned itself in library.json — the deletion never reached other devices or the server: - peers kept the book: the reconcile pass skipped deletedAt entries, so a tombstone never removed the local copy; - the server kept the files: the per-hash directory was never GC'd; - the tombstone could vanish entirely: a device that had never seen the book rebuilt the index purely from its own library, dropping the tombstone and silently reviving the book for everyone. Fixes all three in engine.syncLibrary: - apply a peer's tombstone locally (LocalStore.deleteBookLocally removes the app-managed copy and persists the tombstone), with edit-wins-over-delete LWW so a book still being read isn't yanked; - GC the remote per-hash directory of tombstoned books, scoped to the dirs the discovery scan saw so removed dirs are never re-DELETEd; - union remote-only entries (chiefly tombstones) into the re-pushed index so a deletion can't be dropped by a device that never had the book. Books tombstoned mid-run are excluded from the push pass via the merged state so a just-deleted book isn't re-uploaded right before it is GC'd. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../services/sync/file/appLocalStore.test.ts | 18 ++ .../sync/file/engine-deletion-sync.test.ts | 225 ++++++++++++++++++ .../sync/file/engine-metadata-sync.test.ts | 1 + .../sync/file/engine-sync-paths.test.ts | 148 +++++++++++- .../__tests__/services/sync/file/wire.test.ts | 12 + .../src/services/sync/file/appLocalStore.ts | 18 ++ .../src/services/sync/file/engine.ts | 191 +++++++++++---- .../src/services/sync/file/localStore.ts | 7 + .../src/services/sync/file/wire.ts | 12 + 9 files changed, 590 insertions(+), 42 deletions(-) create mode 100644 apps/readest-app/src/__tests__/services/sync/file/engine-deletion-sync.test.ts diff --git a/apps/readest-app/src/__tests__/services/sync/file/appLocalStore.test.ts b/apps/readest-app/src/__tests__/services/sync/file/appLocalStore.test.ts index fd2226f5..3adb1d45 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/appLocalStore.test.ts +++ b/apps/readest-app/src/__tests__/services/sync/file/appLocalStore.test.ts @@ -44,6 +44,7 @@ beforeEach(() => { savedLibrary = books; }), generateCoverImageUrl: vi.fn(async () => 'blob:cover'), + deleteBook: vi.fn(async () => {}), } as unknown as AppService; envConfig = { getAppService: async () => appService } as unknown as EnvConfigType; }); @@ -84,4 +85,21 @@ describe('createAppLocalStore — library hydration (data-loss guard)', () => { expect(appService.loadLibraryBooks).not.toHaveBeenCalled(); expect(savedLibrary!.map((b) => b.hash).sort()).toEqual(['a', 'b', 'c']); }); + + test('deleteBookLocally removes the managed copy and persists the tombstone (#4860)', async () => { + useLibraryStore.getState().setLibrary([makeBook('a'), makeBook('b')]); + + await makeStore().deleteBookLocally(makeBook('a', { deletedAt: 500 })); + + // The managed local copy is removed via the 'local' delete action. + expect(appService.deleteBook).toHaveBeenCalledWith( + expect.objectContaining({ hash: 'a' }), + 'local', + ); + // The tombstone is persisted and the other book survives. + expect(savedLibrary!.map((b) => b.hash).sort()).toEqual(['a', 'b']); + expect(savedLibrary!.find((b) => b.hash === 'a')!.deletedAt).toBe(500); + // The deleted book drops off the visible shelf. + expect(useLibraryStore.getState().visibleLibrary.map((b) => b.hash)).toEqual(['b']); + }); }); diff --git a/apps/readest-app/src/__tests__/services/sync/file/engine-deletion-sync.test.ts b/apps/readest-app/src/__tests__/services/sync/file/engine-deletion-sync.test.ts new file mode 100644 index 00000000..9526047f --- /dev/null +++ b/apps/readest-app/src/__tests__/services/sync/file/engine-deletion-sync.test.ts @@ -0,0 +1,225 @@ +import { describe, expect, test, vi } from 'vitest'; + +import type { Book } from '@/types/book'; +import { FileSyncEngine } from '@/services/sync/file/engine'; +import type { FileSyncProvider } from '@/services/sync/file/provider'; +import type { LocalStore } from '@/services/sync/file/localStore'; +import type { RemoteLibraryIndex } from '@/services/sync/file/wire'; + +/** + * #4860: WebDAV deletions must propagate. Three behaviours are exercised here: + * 1. a peer's tombstone deletes the book locally (edit-wins-over-delete guard), + * 2. the deleted book's remote hash directory is GC'd off the server, + * 3. a tombstone for a book this device never had survives the index re-push + * (otherwise the deletion would silently vanish from library.json). + */ + +const makeBook = (hash: string, overrides: Partial = {}): Book => ({ + hash, + format: 'EPUB', + title: `Book ${hash}`, + sourceTitle: `Book ${hash}`, + author: 'A', + createdAt: 1, + updatedAt: 1, + ...overrides, +}); + +type Captured = { writes: { path: string; body: string }[]; deletedDirs: string[] }; + +const fakeProvider = ( + opts: Partial & { captured?: Captured } = {}, +): FileSyncProvider => ({ + rootPath: '/', + readText: opts.readText ?? (async () => null), + readBinary: opts.readBinary ?? (async () => null), + head: opts.head ?? (async () => null), + list: opts.list ?? (async () => []), + writeText: + opts.writeText ?? + (async (path: string, body: string) => { + opts.captured?.writes.push({ path, body }); + }), + writeBinary: opts.writeBinary ?? (async () => {}), + ensureDir: opts.ensureDir ?? (async () => {}), + deleteDir: + opts.deleteDir ?? + (async (path: string) => { + opts.captured?.deletedDirs.push(path); + }), + uploadStream: opts.uploadStream, + downloadStream: opts.downloadStream, +}); + +const fakeStore = (opts: Partial = {}): LocalStore => ({ + loadConfig: opts.loadConfig ?? (async () => null), + saveBookConfig: opts.saveBookConfig ?? (async () => {}), + loadBookFile: opts.loadBookFile ?? (async () => null), + resolveLocalBookPath: opts.resolveLocalBookPath ?? (async () => null), + saveBookFile: opts.saveBookFile ?? (async () => {}), + prepareLocalBookPath: opts.prepareLocalBookPath ?? (async () => '/local/dst'), + loadBookCover: opts.loadBookCover ?? (async () => null), + saveBookCover: opts.saveBookCover ?? (async () => {}), + addBookToLibrary: opts.addBookToLibrary ?? (async () => {}), + updateBookMetadata: opts.updateBookMetadata ?? (async () => {}), + deleteBookLocally: opts.deleteBookLocally ?? (async () => {}), +}); + +const makeIndex = (books: Book[]): RemoteLibraryIndex => ({ + schemaVersion: 1, + updatedAt: 1, + books, +}); + +const libraryWrite = (captured: Captured) => + captured.writes.find((w) => w.path.endsWith('library.json')); + +describe('FileSyncEngine.syncLibrary — deletion propagation (#4860)', () => { + test('deletes a book locally when a peer tombstoned it', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100, deletedAt: 200 })])) + : null, + captured, + }); + const deleteBookLocally = vi.fn<(b: Book) => Promise>(async () => {}); + const store = fakeStore({ deleteBookLocally }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100 })], + { strategy: 'silent', syncBooks: false, deviceId: 'd' }, + ); + + expect(deleteBookLocally).toHaveBeenCalledTimes(1); + expect(deleteBookLocally.mock.calls[0]![0].hash).toBe('h1'); + expect(deleteBookLocally.mock.calls[0]![0].deletedAt).toBe(200); + expect(res.booksDeleted).toBe(1); + + // The tombstone must be carried into the re-pushed index. + const idx = libraryWrite(captured); + expect(idx).toBeDefined(); + const parsed = JSON.parse(idx!.body) as RemoteLibraryIndex; + const h1 = parsed.books.find((b) => b.hash === 'h1'); + expect(h1?.deletedAt).toBe(200); + }); + + test('does not re-push the config of a book it just peer-deleted', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100, deletedAt: 200 })])) + : p.endsWith('config.json') + ? JSON.stringify({ schemaVersion: 1, bookHash: 'h1', config: {}, booknotes: [] }) + : null, + captured, + }); + // The book file is still on disk after a 'local' delete removes the copy, so + // loadConfig returns a real config — the push loop must still skip it. + const store = fakeStore({ + loadConfig: async () => ({ updatedAt: 1, booknotes: [] }), + loadBookFile: async () => ({ bytes: new ArrayBuffer(8), size: 8 }), + }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100 })], + { strategy: 'silent', syncBooks: true, deviceId: 'd' }, + ); + + expect(res.booksDeleted).toBe(1); + expect(res.configsUploaded).toBe(0); + expect(res.filesUploaded).toBe(0); + expect(captured.writes.filter((w) => w.path.endsWith('config.json'))).toHaveLength(0); + }); + + test('does not delete locally when the local copy was edited after the deletion', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 50, deletedAt: 100 })])) + : null, + captured, + }); + const deleteBookLocally = vi.fn<(b: Book) => Promise>(async () => {}); + const store = fakeStore({ deleteBookLocally }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + // Local edit (updatedAt 200) is newer than the remote deletion (100). + [makeBook('h1', { updatedAt: 200 })], + { strategy: 'silent', syncBooks: false, deviceId: 'd' }, + ); + + expect(deleteBookLocally).not.toHaveBeenCalled(); + expect(res.booksDeleted).toBe(0); + }); + + test('GCs the remote hash directory of a locally-deleted book', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async () => null, // fresh remote index + list: async (path: string) => + path.endsWith('/books') + ? [{ name: 'h1', path: '/Readest/books/h1', isDirectory: true }] + : [], + captured, + }); + const store = fakeStore(); + + await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100, deletedAt: 100 })], + { strategy: 'silent', syncBooks: true, deviceId: 'd' }, + ); + + expect(captured.deletedDirs).toContain('/Readest/books/h1'); + }); + + test('does not GC a hash dir that is no longer on the server', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async () => null, + list: async () => [], // the books dir is empty — nothing to GC + captured, + }); + const store = fakeStore(); + + await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100, deletedAt: 100 })], + { strategy: 'silent', syncBooks: true, deviceId: 'd' }, + ); + + expect(captured.deletedDirs).toHaveLength(0); + }); + + test('preserves a remote tombstone for a book this device never had', async () => { + const captured: Captured = { writes: [], deletedDirs: [] }; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify( + makeIndex([ + makeBook('h1', { updatedAt: 100 }), + makeBook('h2', { updatedAt: 100, deletedAt: 300 }), + ]), + ) + : null, + captured, + }); + const store = fakeStore(); + + // Local library only has h1; it has never seen h2. + await new FileSyncEngine(provider, store).syncLibrary([makeBook('h1', { updatedAt: 100 })], { + strategy: 'silent', + syncBooks: false, + deviceId: 'd', + }); + + const idx = libraryWrite(captured); + expect(idx).toBeDefined(); + const parsed = JSON.parse(idx!.body) as RemoteLibraryIndex; + const h2 = parsed.books.find((b) => b.hash === 'h2'); + expect(h2?.deletedAt).toBe(300); + }); +}); 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 98e0a300..1b2cc7f5 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 @@ -92,6 +92,7 @@ const makeStore = (overrides: Partial = {}): LocalStore => ({ saveBookCover: async () => {}, addBookToLibrary: async () => {}, updateBookMetadata: async () => {}, + deleteBookLocally: async () => {}, ...overrides, }); diff --git a/apps/readest-app/src/__tests__/services/sync/file/engine-sync-paths.test.ts b/apps/readest-app/src/__tests__/services/sync/file/engine-sync-paths.test.ts index 0f151720..725b0515 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/engine-sync-paths.test.ts +++ b/apps/readest-app/src/__tests__/services/sync/file/engine-sync-paths.test.ts @@ -58,6 +58,7 @@ const fakeStore = (opts: Partial = {}): LocalStore => ({ saveBookCover: opts.saveBookCover ?? (async () => {}), addBookToLibrary: opts.addBookToLibrary ?? (async () => {}), updateBookMetadata: opts.updateBookMetadata ?? (async () => {}), + deleteBookLocally: opts.deleteBookLocally ?? (async () => {}), }); describe('FileSyncEngine.pushBookFile — streaming upload', () => { @@ -163,10 +164,11 @@ describe('FileSyncEngine.syncLibrary — receive strategy is pull-only', () => { }); }); -const makeIndex = (books: Book[]): RemoteLibraryIndex => ({ +const makeIndex = (books: Book[], uploadedHashes?: string[]): RemoteLibraryIndex => ({ schemaVersion: 1, updatedAt: 1, books, + ...(uploadedHashes ? { uploadedHashes } : {}), }); const makeEnvelope = (over: Partial = {}): RemoteBookConfig => ({ @@ -273,6 +275,150 @@ describe('FileSyncEngine.syncLibrary — incremental diff (default)', () => { expect(configWrites(captured)).toHaveLength(0); }); + // #4856: enabling "Upload Book Files" AFTER the first (config-only) sync must + // upload the file even though the book's config is already in sync with the + // index (its updatedAt is unchanged, so the incremental cursor would skip it). + test('uploads the file when syncBooks is enabled after a config-only first sync', async () => { + const captured: Captured = { writes: [] }; + const binaryWrites: string[] = []; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100 })])) + : null, + head: async () => null, // no book file on the remote yet + writeBinary: async (path: string) => { + binaryWrites.push(path); + }, + captured, + }); + const store = fakeStore({ + loadConfig: async () => ({ updatedAt: 1, booknotes: [] }), + loadBookFile: async () => ({ bytes: new ArrayBuffer(10), size: 10 }), + }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100 })], + { + strategy: 'silent', + syncBooks: true, + deviceId: 'd', + }, + ); + + expect(res.filesUploaded).toBe(1); + expect(binaryWrites.some((p) => p.includes('/Readest/books/h1/'))).toBe(true); + // The config is already in sync, so it must NOT be re-pushed. + expect(configWrites(captured)).toHaveLength(0); + // The upload is recorded so the next incremental sync skips the probe. + const idx = JSON.parse( + captured.writes.find((w) => w.path.endsWith('library.json'))!.body, + ) as RemoteLibraryIndex; + expect(idx.uploadedHashes).toContain('h1'); + }); + + test('skips the file when the remote already has a same-size copy (syncBooks on)', async () => { + const captured: Captured = { writes: [] }; + const binaryWrites: string[] = []; + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100 })])) + : null, + head: async () => ({ size: 10 }), // remote already has the file, same size + writeBinary: async (path: string) => { + binaryWrites.push(path); + }, + captured, + }); + const store = fakeStore({ + loadConfig: async () => ({ updatedAt: 1, booknotes: [] }), + loadBookFile: async () => ({ bytes: new ArrayBuffer(10), size: 10 }), + }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100 })], + { + strategy: 'silent', + syncBooks: true, + deviceId: 'd', + }, + ); + + expect(res.filesUploaded).toBe(0); + expect(res.filesAlreadyInSync).toBe(1); + expect(binaryWrites).toHaveLength(0); + expect(configWrites(captured)).toHaveLength(0); + // The freshly-verified file is now recorded so the next sync skips it. + const idx = JSON.parse( + captured.writes.find((w) => w.path.endsWith('library.json'))!.body, + ) as RemoteLibraryIndex; + expect(idx.uploadedHashes).toContain('h1'); + }); + + // #4856 perf: once a file is recorded in the index, an incremental sync must + // NOT HEAD-probe it again — the steady state stays O(changed), not O(library). + test('does not probe an already-recorded file (stays O(changed))', async () => { + const captured: Captured = { writes: [] }; + const head = vi.fn(async () => null); + const loadBookFile = vi.fn(async () => ({ bytes: new ArrayBuffer(10), size: 10 })); + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100 })], ['h1'])) + : null, + head, + captured, + }); + const store = fakeStore({ + loadConfig: async () => ({ updatedAt: 1, booknotes: [] }), + loadBookFile, + }); + + const res = await new FileSyncEngine(provider, store).syncLibrary( + [makeBook('h1', { updatedAt: 100 })], + { strategy: 'silent', syncBooks: true, deviceId: 'd' }, + ); + + // No file work at all: no HEAD probe, no bytes read, no upload. + expect(head).not.toHaveBeenCalled(); + expect(loadBookFile).not.toHaveBeenCalled(); + expect(res.filesUploaded).toBe(0); + expect(res.filesAlreadyInSync).toBe(0); + expect(res.booksSynced).toBe(0); + // The recorded hash is preserved across the re-push. + const idx = JSON.parse( + captured.writes.find((w) => w.path.endsWith('library.json'))!.body, + ) as RemoteLibraryIndex; + expect(idx.uploadedHashes).toContain('h1'); + }); + + test('fullSync re-probes a recorded file (drift escape hatch)', async () => { + const head = vi.fn(async () => ({ size: 10 })); + const provider = fakeProvider({ + readText: async (p) => + p.endsWith('library.json') + ? JSON.stringify(makeIndex([makeBook('h1', { updatedAt: 100 })], ['h1'])) + : null, + head, + captured: { writes: [] }, + }); + const store = fakeStore({ + loadConfig: async () => ({ updatedAt: 1, booknotes: [] }), + loadBookFile: async () => ({ bytes: new ArrayBuffer(10), size: 10 }), + }); + + await new FileSyncEngine(provider, store).syncLibrary([makeBook('h1', { updatedAt: 100 })], { + strategy: 'silent', + syncBooks: true, + deviceId: 'd', + fullSync: true, + }); + + // Full Sync bypasses the record and re-verifies the file. + expect(head).toHaveBeenCalled(); + }); + test('fullSync re-pushes an in-sync book', async () => { const captured: Captured = { writes: [] }; const provider = fakeProvider({ diff --git a/apps/readest-app/src/__tests__/services/sync/file/wire.test.ts b/apps/readest-app/src/__tests__/services/sync/file/wire.test.ts index 2f2999ad..15c05819 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/wire.test.ts +++ b/apps/readest-app/src/__tests__/services/sync/file/wire.test.ts @@ -59,4 +59,16 @@ describe('wire envelope (frozen)', () => { expect(ok?.books).toHaveLength(1); expect(ok?.updatedAt).toBe(5); }); + + test('parseRemoteLibraryIndex preserves the optional uploadedHashes record (#4856)', () => { + const parsed = parseRemoteLibraryIndex( + JSON.stringify({ schemaVersion: 1, books: [book], updatedAt: 5, uploadedHashes: ['h1'] }), + ); + expect(parsed?.uploadedHashes).toEqual(['h1']); + // Legacy index without the field parses fine (treated as empty by the engine). + const legacy = parseRemoteLibraryIndex( + JSON.stringify({ schemaVersion: 1, books: [book], updatedAt: 5 }), + ); + expect(legacy?.uploadedHashes).toBeUndefined(); + }); }); diff --git a/apps/readest-app/src/services/sync/file/appLocalStore.ts b/apps/readest-app/src/services/sync/file/appLocalStore.ts index 54388cf1..94c48cab 100644 --- a/apps/readest-app/src/services/sync/file/appLocalStore.ts +++ b/apps/readest-app/src/services/sync/file/appLocalStore.ts @@ -130,4 +130,22 @@ export const createAppLocalStore = ({ // new title / author / cover show up without a reload. await useLibraryStore.getState().updateBook(envConfig, book); }, + + deleteBookLocally: async (book) => { + // Remove this device's managed copy of the book file (cloudService.deleteBook + // with 'local' only ever touches app-managed Books// sources; an + // in-place / external original is left untouched). The tombstone itself is + // set by the engine before this call — we just persist it. + try { + await appService.deleteBook(book, 'local'); + } catch (e) { + console.warn('createAppLocalStore: local book delete failed', book.hash, e); + } + book.coverImageUrl = null; + book.syncedAt = Date.now(); + if (!useLibraryStore.getState().libraryLoaded) { + useLibraryStore.getState().setLibrary(await appService.loadLibraryBooks()); + } + await useLibraryStore.getState().updateBook(envConfig, book); + }, }); diff --git a/apps/readest-app/src/services/sync/file/engine.ts b/apps/readest-app/src/services/sync/file/engine.ts index 74ddb486..0210d1c9 100644 --- a/apps/readest-app/src/services/sync/file/engine.ts +++ b/apps/readest-app/src/services/sync/file/engine.ts @@ -68,6 +68,8 @@ export interface SyncLibraryResult { filesAlreadyInSync: number; coversUploaded: number; booksDownloaded: number; + /** Local books removed because a peer's tombstone propagated to this device (#4860). */ + booksDeleted: number; /** Already-local books whose metadata was refreshed from a newer index copy (#4756). */ metadataUpdated: number; /** Distinct books that had any sync activity (pushed, downloaded, or reconciled). */ @@ -363,6 +365,7 @@ export class FileSyncEngine { filesAlreadyInSync: 0, coversUploaded: 0, booksDownloaded: 0, + booksDeleted: 0, metadataUpdated: 0, booksSynced: 0, failures: 0, @@ -413,6 +416,20 @@ export class FileSyncEngine { return (book.updatedAt ?? 0) > (remote.updatedAt ?? 0); }; + // File-upload cursor (#4856): the index records which book FILES already + // live on the remote. A book's file is immutable per hash, so once recorded + // it never needs re-checking — this keeps an incremental sync O(changed) + // by skipping the per-book HEAD probe for already-mirrored files instead of + // probing every book each run. Seeded from the pulled index and carried + // forward (plus this run's uploads) into the re-pushed index. Empty in send + // mode / on a fresh remote, so the first sync verifies every file once. + const uploadedHashes = new Set(remoteIndex?.uploadedHashes ?? []); + // A file needs (re)uploading only when syncBooks is on and the remote copy + // isn't recorded yet. Full Sync bypasses the record as an escape hatch for + // drift (e.g. a file deleted out-of-band via the browse pane). + const needsFilePush = (book: Book): boolean => + options.syncBooks && (fullSync || !uploadedHashes.has(book.hash)); + const remoteBooksToDownload: Book[] = []; // The remote source of truth for a book's on-disk filename is the per-hash // directory listing — NOT the book's title (which may be stale). We always @@ -474,6 +491,43 @@ export class FileSyncEngine { }); } + // Deletion propagation (#4860): a book a peer tombstoned in the shared index + // must be removed from this device too, not just hidden on the origin. Apply + // the deletion with edit-wins-over-delete semantics — only when it is newer + // than any local change, so a device that kept reading a book after another + // device deleted it keeps its copy (and the live row re-revives the tombstone + // on the next push). + if (canPull && remoteIndex && remoteIndex.books) { + const remoteDeletions = remoteIndex.books.filter((rb) => { + if (!rb.deletedAt) return false; + const local = allBooksMap.get(rb.hash); + return !!local && !local.deletedAt && (rb.deletedAt ?? 0) > (local.updatedAt ?? 0); + }); + await runPool(remoteDeletions, concurrency, async (rb) => { + const local = allBooksMap.get(rb.hash)!; + const deleted: Book = { + ...local, + deletedAt: rb.deletedAt, + downloadedAt: null, + coverDownloadedAt: null, + updatedAt: Math.max(local.updatedAt ?? 0, rb.updatedAt ?? 0), + }; + try { + await this.store.deleteBookLocally(deleted); + // Keep the tombstone in allBooksMap so the index re-push carries it. + allBooksMap.set(rb.hash, deleted); + result.booksDeleted += 1; + syncedHashes.add(rb.hash); + } catch (e) { + console.warn('file sync: local delete failed', rb.hash, e); + } + }); + } + + // Hash directories that still exist on the remote. Populated by the discovery + // scan below and reused by the deleted-book GC before the index re-push. + const remoteHashDirs = new Set(); + if (canPull) { const candidateHashes = new Set(); @@ -495,7 +549,9 @@ export class FileSyncEngine { const booksDirPath = `${buildBasePath(this.provider.rootPath)}/${SYNC_BOOKS_DIR}`; const dirEntries = await this.provider.list(booksDirPath); for (const entry of dirEntries) { - if (entry.isDirectory && !allBooksMap.has(entry.name)) { + if (!entry.isDirectory) continue; + remoteHashDirs.add(entry.name); + if (!allBooksMap.has(entry.name)) { candidateHashes.add(entry.name); } } @@ -608,6 +664,9 @@ export class FileSyncEngine { await this.store.addBookToLibrary(rb); result.booksDownloaded += 1; syncedHashes.add(rb.hash); + // We just pulled its bytes, so the file is on the remote — record it + // so a later push-side sync doesn't HEAD-probe it back. + uploadedHashes.add(rb.hash); } else { // No bytes returned (typically a 404 we couldn't resolve). result.failures += 1; @@ -635,10 +694,23 @@ export class FileSyncEngine { // Books we just downloaded already exist on the remote — don't re-push // them. Only push books already present in the caller-supplied library. const downloadedHashes = new Set(remoteBooksToDownload.map((b) => b.hash)); - // Incremental (default): push only books that changed locally since the - // last index push. Full-sync re-checks everything. + // A book's config/cover only need pushing when it changed locally since the + // last index push (incremental; full-sync re-checks everything). Its FILE, + // by contrast, is immutable per hash and only needs uploading when the + // remote copy is missing per the index's uploaded-file record (`needsFilePush`) + // — which catches the user enabling "Upload Book Files" only after the first + // (config-only) sync (#4856) without a per-book probe once files are recorded. + const configChanged = (b: Book): boolean => fullSync || isLocalNewer(b); + // Consult the merged state, not the caller's raw book: a book a peer just + // tombstoned in this same run is now deletedAt in allBooksMap even though + // the caller's array copy isn't — pushing it would re-upload a book we are + // about to GC (#4860). + const isEffectivelyDeleted = (b: Book): boolean => !!(allBooksMap.get(b.hash) ?? b).deletedAt; const booksToPush = books.filter( - (b) => !b.deletedAt && !downloadedHashes.has(b.hash) && (fullSync || isLocalNewer(b)), + (b) => + !isEffectivelyDeleted(b) && + !downloadedHashes.has(b.hash) && + (configChanged(b) || needsFilePush(b)), ); result.totalBooks = booksToPush.length; @@ -654,51 +726,57 @@ export class FileSyncEngine { pushStarted += 1; let phase: SyncFailureEntry['phase'] = 'upload-config'; try { - const config = await this.store.loadConfig(book); - if (config) { - // Mirror the reader hook's pull-merge-push discipline so a manual - // "Sync now" can't blind-overwrite state this device hasn't pulled - // yet. Only in two-way ('silent') mode — 'send' keeps the blind - // push. A failed pull-merge falls back to the local config. - let configToPush = config; - if (canPull) { - try { - const pull = await this.pullBookConfig(book, config); - if (pull.applied && pull.mergedConfig) { - configToPush = pull.mergedConfig; - // Persist the merged superset locally so this device - // converges too, not just the remote. - await this.store.saveBookConfig(book, pull.mergedConfig); + if (configChanged(book)) { + const config = await this.store.loadConfig(book); + if (config) { + // Mirror the reader hook's pull-merge-push discipline so a manual + // "Sync now" can't blind-overwrite state this device hasn't pulled + // yet. Only in two-way ('silent') mode — 'send' keeps the blind + // push. A failed pull-merge falls back to the local config. + let configToPush = config; + if (canPull) { + try { + const pull = await this.pullBookConfig(book, config); + if (pull.applied && pull.mergedConfig) { + configToPush = pull.mergedConfig; + // Persist the merged superset locally so this device + // converges too, not just the remote. + await this.store.saveBookConfig(book, pull.mergedConfig); + } + } catch (e) { + console.warn('file sync: config pull-merge failed', book.hash, e); } - } catch (e) { - console.warn('file sync: config pull-merge failed', book.hash, e); } - } - await this.pushBookConfig(book, configToPush, options.deviceId); - result.configsUploaded += 1; - syncedHashes.add(book.hash); - } - // Covers ride along with the config-level sync, NOT with syncBooks: - // the receiving device can't regenerate them without the book bytes. - // Failures here are warnings, not hard failures. - try { - const coverResult = await this.pushBookCover(book); - if (coverResult.uploaded) { - result.coversUploaded += 1; + await this.pushBookConfig(book, configToPush, options.deviceId); + result.configsUploaded += 1; syncedHashes.add(book.hash); } - } catch (e) { - console.warn('file sync: cover failed', book.hash, e); + // Covers ride along with the config-level sync, NOT with syncBooks: + // the receiving device can't regenerate them without the book bytes. + // Failures here are warnings, not hard failures. + try { + const coverResult = await this.pushBookCover(book); + if (coverResult.uploaded) { + result.coversUploaded += 1; + syncedHashes.add(book.hash); + } + } catch (e) { + console.warn('file sync: cover failed', book.hash, e); + } } - if (options.syncBooks) { + if (needsFilePush(book)) { phase = 'upload-file'; const fileResult = await this.pushBookFile(book); if (fileResult.uploaded) { result.filesUploaded += 1; syncedHashes.add(book.hash); + uploadedHashes.add(book.hash); } else if (fileResult.reason === 'remote-matches') { result.filesAlreadyInSync += 1; + uploadedHashes.add(book.hash); } + // 'no-source' → the file isn't on this device; leave it unrecorded + // so a device that does have it can upload and record it later. } } catch (e) { result.failures += 1; @@ -713,16 +791,47 @@ export class FileSyncEngine { }); } - // Push the merged index whenever we're allowed to write, even if no - // binaries moved this turn (keeps library.json authoritative). Soft-deleted - // books' per-hash dirs are intentionally NOT GC'd here — that's the manual - // cleanup sweep's job (see deleteRemoteBookDir). + // The final index whenever we're allowed to write, even if no binaries + // moved this turn (keeps library.json authoritative). Union in any remote + // entries this device never materialised (chiefly peers' tombstones): + // rebuilding purely from allBooksMap would drop a deletion for a book we + // never had, silently reviving it for every other device (#4860). if (canPush) { + const indexByHash = new Map(allBooksMap); + if (remoteIndex?.books) { + for (const rb of remoteIndex.books) { + if (!indexByHash.has(rb.hash)) indexByHash.set(rb.hash, rb); + } + } + + // GC the remote per-hash directory of every tombstoned book whose files + // still linger on the server (#4860). Scoped to dirs the discovery scan + // actually saw, so a dir removed on a previous sync is never re-DELETEd + // and 'send' mode (which never lists) is a safe no-op. This is what makes + // a deletion reclaim server space instead of leaving orphaned book files. + const dirsToGc = Array.from(remoteHashDirs).filter( + (hash) => indexByHash.get(hash)?.deletedAt, + ); + await runPool(dirsToGc, concurrency, async (hash) => { + try { + await deleteRemoteBookDir(this.provider, hash); + } catch (e) { + console.warn('file sync: failed to GC deleted book dir', hash, e); + } + }); + try { const newIndex: RemoteLibraryIndex = { schemaVersion: 1, - books: Array.from(allBooksMap.values()), + books: Array.from(indexByHash.values()), updatedAt: Date.now(), + // Carry the uploaded-file record forward so the next incremental sync + // stays O(changed). Keep only hashes that still map to a live indexed + // book so the set can't grow unbounded with tombstoned / evicted books. + uploadedHashes: Array.from(uploadedHashes).filter((hash) => { + const b = indexByHash.get(hash); + return !!b && !b.deletedAt; + }), }; await this.pushLibraryIndex(newIndex); } catch (e) { diff --git a/apps/readest-app/src/services/sync/file/localStore.ts b/apps/readest-app/src/services/sync/file/localStore.ts index 80f9165c..32c44a61 100644 --- a/apps/readest-app/src/services/sync/file/localStore.ts +++ b/apps/readest-app/src/services/sync/file/localStore.ts @@ -50,4 +50,11 @@ export interface LocalStore { addBookToLibrary(book: Book): Promise; /** Persist refreshed metadata for a book already in the local library. */ updateBookMetadata(book: Book): Promise; + /** + * Apply a peer's deletion locally: remove this device's managed copy of the + * book file and persist the tombstone (`book.deletedAt`) so the book drops + * off the shelf and stops being re-uploaded. External / in-place sources are + * never touched — only the app-managed `Books//` copy is removed. + */ + deleteBookLocally(book: Book): Promise; } diff --git a/apps/readest-app/src/services/sync/file/wire.ts b/apps/readest-app/src/services/sync/file/wire.ts index c44d3f7e..d76d2408 100644 --- a/apps/readest-app/src/services/sync/file/wire.ts +++ b/apps/readest-app/src/services/sync/file/wire.ts @@ -96,6 +96,18 @@ export interface RemoteLibraryIndex { schemaVersion: 1; books: Book[]; updatedAt: number; + /** + * Hashes whose book FILE (not just config/cover) is confirmed present on the + * remote. A book's file is immutable per hash, so once uploaded it never + * needs re-checking — recording it here lets an incremental "Sync now" skip + * the per-book HEAD probe for already-mirrored files and stay O(changed) + * instead of O(library) when "Upload Book Files" is on (#4856). + * + * Optional + additive: a legacy/absent value is treated as empty, so an old + * client that rewrites the index simply drops it and the next new-client sync + * re-verifies each file once (a bounded, self-healing HEAD) and re-records it. + */ + uploadedHashes?: string[]; } export const parseRemoteLibraryIndex = (raw: string | null): RemoteLibraryIndex | null => {