forked from akai/readest
fix(library): never let a routine save shrink library.json (cold-start "Open with" wipe) (#4627)
Opening a book via Android "Open with" on a cold start could clear the entire library. `openTransient` built an ephemeral entry on top of the not-yet-loaded (empty) store; the library page's `length > 0` cached-skip then skipped loading the real library from disk; and a later `saveLibraryBooks` persisted the empty/partial set — overwriting both library.json and its .bak. Introduced by #4407 (transient "Open with"), made reliably reproducible by #4527 (reliable cold-start delivery) — so released v0.11.4, which lacks #4527, does not reproduce it. Two layers of defense: - saveLibraryBooks now merges with the on-disk library (union by hash, incoming wins), so a routine save is monotonic: it can add or modify rows (including `deletedAt` tombstones) but can never drop a book. Deliberate, authoritative rewrites (restore, tombstone GC, account reset) opt in via the new `{ replace: true }`. This layer alone makes the wipe impossible. - openTransient loads the real library from disk before importing a transient book — also fixing the cold-start hash-match miss that re-imported already-imported books — and the library page's load-skip now gates on the store's `libraryLoaded` flag instead of `length > 0`. Tests cover the merge floor (no-drop / no-wipe-on-empty / tombstone preserved / incoming-wins) and both `{ replace: true }` paths. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -90,12 +90,12 @@ export function libraryTests(getService: () => AppService) {
|
||||
expect(loaded).toEqual([]);
|
||||
});
|
||||
|
||||
it('should overwrite previous library on save', async () => {
|
||||
it('should overwrite (and shrink) the library only when replace is set', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'old' })]);
|
||||
|
||||
const newBooks = [makeBook({ hash: 'new1' }), makeBook({ hash: 'new2' })];
|
||||
await service.saveLibraryBooks(newBooks);
|
||||
await service.saveLibraryBooks(newBooks, { replace: true });
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
expect(loaded).toHaveLength(2);
|
||||
@@ -143,13 +143,70 @@ export function libraryTests(getService: () => AppService) {
|
||||
expect(loaded[0]!.updatedAt).toBe(5000);
|
||||
});
|
||||
|
||||
it('should save empty array', async () => {
|
||||
it('should clear the library when saving an empty array with replace', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([makeBook()]);
|
||||
await service.saveLibraryBooks([]);
|
||||
await service.saveLibraryBooks([], { replace: true });
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
expect(loaded).toEqual([]);
|
||||
});
|
||||
|
||||
// Merge-floor safebelt: a routine save may ADD or MODIFY rows (including
|
||||
// setting `deletedAt` tombstones) but must never silently DROP a book that
|
||||
// exists on disk. Guards against a stale or partially-loaded in-memory
|
||||
// library wiping library.json (the cold-start "Open with" race).
|
||||
it('should not drop on-disk books absent from the saved set (merge floor)', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([
|
||||
makeBook({ hash: 'a', title: 'A' }),
|
||||
makeBook({ hash: 'b', title: 'B' }),
|
||||
makeBook({ hash: 'c', title: 'C' }),
|
||||
]);
|
||||
|
||||
// A later save that only knows about 'a' must not erase 'b' and 'c'.
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'a', title: 'A2' })]);
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
const byHash = Object.fromEntries(loaded.map((b) => [b.hash, b.title]));
|
||||
expect(loaded).toHaveLength(3);
|
||||
expect(byHash).toEqual({ a: 'A2', b: 'B', c: 'C' });
|
||||
});
|
||||
|
||||
it('should preserve the whole library when an empty set is saved (no wipe)', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'a' }), makeBook({ hash: 'b' })]);
|
||||
|
||||
await service.saveLibraryBooks([]);
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
expect(loaded.map((b) => b.hash).sort()).toEqual(['a', 'b']);
|
||||
});
|
||||
|
||||
it('should keep tombstones (deletedAt) not present in the incoming set', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([
|
||||
makeBook({ hash: 'live' }),
|
||||
makeBook({ hash: 'gone', deletedAt: 1234 }),
|
||||
]);
|
||||
|
||||
// A save that omits the tombstone must neither lose nor resurrect it.
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'live' })]);
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
const tomb = loaded.find((b) => b.hash === 'gone');
|
||||
expect(loaded).toHaveLength(2);
|
||||
expect(tomb?.deletedAt).toBe(1234);
|
||||
});
|
||||
|
||||
it('should let the incoming row win on a hash conflict (modify in place)', async () => {
|
||||
const service = getService();
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'a', title: 'old' })]);
|
||||
await service.saveLibraryBooks([makeBook({ hash: 'a', title: 'new' })]);
|
||||
|
||||
const loaded = await service.loadLibraryBooks();
|
||||
expect(loaded).toHaveLength(1);
|
||||
expect(loaded[0]!.title).toBe('new');
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
@@ -143,6 +143,7 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP
|
||||
const { token, user } = useAuth();
|
||||
const {
|
||||
library: libraryBooks,
|
||||
libraryLoaded: libraryLoadedFromDisk,
|
||||
isSyncing,
|
||||
syncProgress,
|
||||
updateBook,
|
||||
@@ -545,7 +546,11 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP
|
||||
}
|
||||
};
|
||||
|
||||
const hasCachedLibrary = libraryBooks.length > 0;
|
||||
// Reuse the in-store library only when it was actually loaded from disk.
|
||||
// Gating on `length > 0` was unsafe: a transient "Open with" entry made the
|
||||
// store non-empty before any disk load, so this skipped loadLibraryBooks and
|
||||
// a later save persisted the partial library (wiping library.json).
|
||||
const hasCachedLibrary = libraryLoadedFromDisk;
|
||||
const loadingTimeout = hasCachedLibrary ? null : setTimeout(() => setLoading(true), 500);
|
||||
const initLibrary = async () => {
|
||||
const appService = await envConfig.getAppService();
|
||||
|
||||
@@ -83,7 +83,18 @@ export function useOpenWithBooks() {
|
||||
// or uploading to the cloud. We also setLibrary so the reader's
|
||||
// initViewState (which looks the book up via getBookByHash) can find
|
||||
// the ephemeral entry.
|
||||
const { library, setLibrary, getBookByHash } = useLibraryStore.getState();
|
||||
const { setLibrary, getBookByHash, libraryLoaded } = useLibraryStore.getState();
|
||||
// Load the real library from disk before building any transient entry.
|
||||
// On a cold-start "Open with" the store may not be populated yet; importing
|
||||
// onto an empty in-memory library would (a) miss the hash-match for an
|
||||
// already-imported book and (b) let the library page's cached-skip persist
|
||||
// an empty library, wiping library.json. Loading first makes the store
|
||||
// reflect disk (and marks it loaded) so neither happens.
|
||||
let library = useLibraryStore.getState().library;
|
||||
if (!libraryLoaded) {
|
||||
library = await appService.loadLibraryBooks();
|
||||
setLibrary(library);
|
||||
}
|
||||
const bookIds: string[] = [];
|
||||
let libraryMutated = false;
|
||||
|
||||
|
||||
@@ -9,6 +9,7 @@ import {
|
||||
FileSystem,
|
||||
OsPlatform,
|
||||
ResolvedPath,
|
||||
SaveLibraryBooksOptions,
|
||||
SelectDirectoryMode,
|
||||
} from '@/types/system';
|
||||
import { DatabaseOpts, DatabaseService } from '@/types/database';
|
||||
@@ -421,7 +422,7 @@ export abstract class BaseAppService implements AppService {
|
||||
return LibrarySvc.loadLibraryBooks(this.fs, this.generateCoverImageUrl.bind(this));
|
||||
}
|
||||
|
||||
async saveLibraryBooks(books: Book[]): Promise<void> {
|
||||
return LibrarySvc.saveLibraryBooks(this.fs, books);
|
||||
async saveLibraryBooks(books: Book[], options?: SaveLibraryBooksOptions): Promise<void> {
|
||||
return LibrarySvc.saveLibraryBooks(this.fs, books, options);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { FileSystem } from '@/types/system';
|
||||
import { FileSystem, SaveLibraryBooksOptions } from '@/types/system';
|
||||
import { Book } from '@/types/book';
|
||||
import { getLibraryFilename } from '@/utils/book';
|
||||
import { safeLoadJSON, safeSaveJSON } from './persistence';
|
||||
@@ -35,8 +35,28 @@ export async function loadLibraryBooks(
|
||||
return books;
|
||||
}
|
||||
|
||||
export async function saveLibraryBooks(fs: FileSystem, books: Book[]): Promise<void> {
|
||||
export async function saveLibraryBooks(
|
||||
fs: FileSystem,
|
||||
books: Book[],
|
||||
options?: SaveLibraryBooksOptions,
|
||||
): Promise<void> {
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
const libraryBooks = books.map(({ coverImageUrl, ...rest }) => rest);
|
||||
await safeSaveJSON(fs, getLibraryFilename(), 'Books', libraryBooks);
|
||||
const incoming = books.map(({ coverImageUrl, ...rest }) => rest);
|
||||
|
||||
if (options?.replace) {
|
||||
await safeSaveJSON(fs, getLibraryFilename(), 'Books', incoming);
|
||||
return;
|
||||
}
|
||||
|
||||
// Merge-floor: treat the on-disk library as a floor. A routine save may add
|
||||
// new books or modify existing rows (including setting `deletedAt`
|
||||
// tombstones), but it must never silently drop a book that exists on disk.
|
||||
// This stops a stale or partially-loaded in-memory library (e.g. the
|
||||
// cold-start "Open with" race) from wiping library.json. Deliberate removals
|
||||
// must go through `{ replace: true }`.
|
||||
const existing = await safeLoadJSON<Book[]>(fs, getLibraryFilename(), 'Books', []);
|
||||
const merged = new Map<string, Book>();
|
||||
for (const book of existing) merged.set(book.hash, book);
|
||||
for (const book of incoming) merged.set(book.hash, book); // incoming wins per hash
|
||||
await safeSaveJSON(fs, getLibraryFilename(), 'Books', Array.from(merged.values()));
|
||||
}
|
||||
|
||||
@@ -68,6 +68,16 @@ export interface FileSystem {
|
||||
getPrefix(base: BaseDir): Promise<string>;
|
||||
}
|
||||
|
||||
export interface SaveLibraryBooksOptions {
|
||||
/**
|
||||
* Overwrite `library.json` with exactly the given set, allowing it to shrink.
|
||||
* Reserved for deliberate, authoritative rewrites (tombstone GC, explicit
|
||||
* "clear library", account reset). Routine saves must NOT set this — the
|
||||
* default merge-floor protects against silently dropping books on disk.
|
||||
*/
|
||||
replace?: boolean;
|
||||
}
|
||||
|
||||
export interface AppService {
|
||||
osPlatform: OsPlatform;
|
||||
appPlatform: AppPlatform;
|
||||
@@ -205,7 +215,7 @@ export interface AppService {
|
||||
loadBookContent(book: Book): Promise<BookContent>;
|
||||
resolveNativeBookFilePath(book: Book): Promise<string | null>;
|
||||
loadLibraryBooks(): Promise<Book[]>;
|
||||
saveLibraryBooks(books: Book[]): Promise<void>;
|
||||
saveLibraryBooks(books: Book[], options?: SaveLibraryBooksOptions): Promise<void>;
|
||||
getCoverImageUrl(book: Book): string;
|
||||
getCoverImageBlobUrl(book: Book): Promise<string>;
|
||||
generateCoverImageUrl(book: Book): Promise<string>;
|
||||
|
||||
Reference in New Issue
Block a user