fix(sync): prevent cross-device progress overwrite; retry first pull on flaky networks (#4341)

Closes #4222.

Three changes that together stabilize Readest sync across devices:

**Stop the artificial updatedAt bump in useProgressAutoSave**

saveConfig unconditionally bumps config.updatedAt = Date.now(), and
useProgressAutoSave used to fire saveConfig on the very first relocate
after book open — even though that relocate just reflects the position
loaded from disk, not user action. The stale local config then looked
"newer" than a fresher server-side push, so the next auto-push
overwrote the other device's real progress via last-writer-wins. The
hook now snapshots the loaded location and skips saveConfig when the
in-memory location still matches it.

**Retry the first config pull with backoff + release the gate**

useProgressSync gates pushes behind a successful pull (so a brand-new
import can't clobber the server's real progress). But handleAutoSync
only re-arms on progress.location changes, so a single failed pull
(Android cold-start contention, Wi-Fi/LTE handoff, captive portal)
used to block every push for the whole reader session. The new
pullWithRetry retries on backoff (1500/4000/10000 ms) and releases
the gate after exhaustion — server-side last-writer-wins still
protects the cross-device case (a stale local push with an older
updated_at loses to a fresher server record). sync-book-progress
events reset the chain so manual pull-to-refresh recovers cleanly.
Fetch timeout bumped from 8s to 15s to better tolerate slow networks
in that cold-start window.

**Server piggybacks books.progress off configs push**

/api/sync POST now updates books.progress + books.updated_at for each
upserted config, gated by .lt('updated_at') so a concurrent newer
books push is never downgraded and a missing row is a silent no-op
(useBooksSync still seeds new rows from the library page). The
in-reader syncBooks round-trip is dropped — the reader now sends one
POST per auto-save instead of two, and the books row stays consistent
with config pushes even while a reader stays open (#4198).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Huang Xin
2026-05-28 21:55:07 +08:00
committed by GitHub
parent 48d52ea898
commit e29331bea9
6 changed files with 415 additions and 28 deletions
@@ -0,0 +1,126 @@
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
import { act, cleanup, renderHook } from '@testing-library/react';
type Progress = { location: string } | null;
const h = vi.hoisted(() => {
const makeStore = <T,>(state: T) => {
const fn = () => state;
(fn as unknown as { getState: () => T }).getState = () => state;
return fn as (() => T) & { getState: () => T };
};
const state = {
config: { location: 'cfi-loc', updatedAt: 1000 } as { location: string; updatedAt: number },
progress: { location: 'cfi-loc' } as Progress,
previewMode: false,
};
return {
makeStore,
state,
saveConfigMock: vi.fn(async () => {}),
};
});
vi.mock('@/context/EnvContext', () => ({
useEnv: () => ({ envConfig: { name: 'env' } }),
}));
vi.mock('@/store/bookDataStore', () => ({
useBookDataStore: h.makeStore({
getConfig: () => h.state.config,
saveConfig: h.saveConfigMock,
}),
}));
vi.mock('@/store/readerStore', () => ({
useReaderStore: h.makeStore({
getProgress: () => h.state.progress,
getViewState: () => ({ previewMode: h.state.previewMode }),
}),
}));
vi.mock('@/store/settingsStore', () => ({
useSettingsStore: h.makeStore({ settings: { version: 1 } }),
}));
import { useProgressAutoSave } from '@/app/reader/hooks/useProgressAutoSave';
const flushDebouncedSave = async () => {
await act(async () => {
// debounce 1000ms + inner setTimeout 500ms + slack
vi.advanceTimersByTime(2000);
for (let i = 0; i < 10; i++) await Promise.resolve();
});
};
beforeEach(() => {
vi.useFakeTimers();
h.saveConfigMock.mockClear();
h.state.config = { location: 'cfi-loc', updatedAt: 1000 };
h.state.progress = { location: 'cfi-loc' };
h.state.previewMode = false;
});
afterEach(() => {
vi.useRealTimers();
cleanup();
});
describe('useProgressAutoSave', () => {
test('skips save on book open when location matches the loaded config', async () => {
// Bug from issue #4222: opening a book where the in-memory location still
// matches what was loaded from disk should not call saveConfig — doing so
// would artificially bump config.updatedAt and let a stale local push
// overwrite a newer server-side config (the progress someone else just
// read on another device).
renderHook(() => useProgressAutoSave('h1-view1'));
await flushDebouncedSave();
expect(h.saveConfigMock).not.toHaveBeenCalled();
});
test('saves once the location advances past the loaded position', async () => {
const { rerender } = renderHook(() => useProgressAutoSave('h1-view1'));
await flushDebouncedSave();
expect(h.saveConfigMock).not.toHaveBeenCalled();
// Simulate the reader advancing to a new location (either user pagination
// or applyRemoteProgress.view.goTo). The config's location is what
// setProgress would have updated, and progress reference changes too.
h.state.config = { location: 'cfi-loc-next', updatedAt: 1000 };
h.state.progress = { location: 'cfi-loc-next' };
rerender();
await flushDebouncedSave();
expect(h.saveConfigMock).toHaveBeenCalledTimes(1);
});
test('saves when applyRemoteProgress moves the view to a newer remote location', async () => {
// Mirrors the cross-device sync case: device opens at the local position
// (loaded from disk), then the pull lands a newer remote position and
// view.goTo(remote) fires. Auto-save must persist the new location so the
// outgoing push carries the remote-applied progress, not the stale local.
const { rerender } = renderHook(() => useProgressAutoSave('h1-view1'));
await flushDebouncedSave();
expect(h.saveConfigMock).not.toHaveBeenCalled();
h.state.config = { location: 'cfi-remote', updatedAt: 1000 };
h.state.progress = { location: 'cfi-remote' };
rerender();
await flushDebouncedSave();
expect(h.saveConfigMock).toHaveBeenCalledTimes(1);
});
test('skips save while in preview mode', async () => {
h.state.previewMode = true;
h.state.config = { location: 'cfi-different', updatedAt: 1000 };
h.state.progress = { location: 'cfi-different' };
renderHook(() => useProgressAutoSave('h1-view1'));
await flushDebouncedSave();
expect(h.saveConfigMock).not.toHaveBeenCalled();
});
});
@@ -31,7 +31,11 @@ const h = vi.hoisted(() => {
user: { id: 'u1' },
syncConfigsMock: vi.fn(async () => {}),
syncBooksMock: vi.fn(async () => {}),
state: { syncedConfigs: [] as unknown[] | null },
state: {
syncedConfigs: [] as unknown[] | null,
progress: { location: 'cfi-loc' } as { location: string } | null,
},
eventListeners: new Map<string, Set<(e: CustomEvent) => void>>(),
};
});
@@ -65,7 +69,7 @@ vi.mock('@/store/readerStore', () => ({
renderer: { getContents: () => [], primaryIndex: 0 },
goTo: vi.fn(),
}),
getProgress: () => ({ location: 'cfi-loc' }),
getProgress: () => h.state.progress,
setHoveredBookKey: vi.fn(),
getViewState: () => ({ previewMode: false }),
}),
@@ -92,6 +96,25 @@ vi.mock('@/libs/document', () => ({
CFI: { compare: () => 0 },
}));
vi.mock('@/utils/event', () => ({
eventDispatcher: {
on: (name: string, fn: (e: CustomEvent) => void) => {
const set = h.eventListeners.get(name) ?? new Set();
set.add(fn);
h.eventListeners.set(name, set);
},
off: (name: string, fn: (e: CustomEvent) => void) => {
h.eventListeners.get(name)?.delete(fn);
},
dispatch: (name: string, detail: unknown) => {
const listeners = h.eventListeners.get(name);
if (!listeners) return;
const event = new CustomEvent(name, { detail });
for (const fn of [...listeners]) fn(event);
},
},
}));
import { useProgressSync } from '@/app/reader/hooks/useProgressSync';
import { SYNC_PROGRESS_INTERVAL_SEC } from '@/services/constants';
@@ -107,6 +130,8 @@ beforeEach(() => {
h.syncConfigsMock.mockClear();
h.syncBooksMock.mockClear();
h.state.syncedConfigs = [];
h.state.progress = { location: 'cfi-loc' };
h.eventListeners.clear();
});
afterEach(() => {
@@ -114,14 +139,109 @@ afterEach(() => {
cleanup();
});
const flushMicrotasks = async () => {
for (let i = 0; i < 20; i++) await Promise.resolve();
};
const advance = async (ms: number) => {
await act(async () => {
vi.advanceTimersByTime(ms);
await flushMicrotasks();
});
};
const pullCallCount = () =>
h.syncConfigsMock.mock.calls.filter((c) => (c as unknown[])[3] === 'pull').length;
const pushCallCount = () =>
h.syncConfigsMock.mock.calls.filter((c) => (c as unknown[])[3] === 'push').length;
describe('useProgressSync', () => {
test('auto-sync push also pushes the books row so other devices see fresh progress', async () => {
test('auto-sync push only hits the configs lane; the server piggybacks books.progress', async () => {
// Issue #4198 used to be fixed by a second syncBooks call from the
// reader so that other devices' library pull-to-refresh would see fresh
// progress while a reader stayed open. The /api/sync POST handler now
// updates books.progress + books.updated_at off the same configs push,
// so the reader-side syncBooks round-trip is gone.
renderHook(() => useProgressSync('h1-view1'));
await flushAutoSync();
// configs lane still pushed
expect(h.syncConfigsMock).toHaveBeenCalledWith(expect.any(Array), 'h1', 'm1', 'push');
// books lane also pushed with the in-memory library Book
expect(h.syncBooksMock).toHaveBeenCalledWith([h.libraryBook], 'push');
expect(h.syncBooksMock).not.toHaveBeenCalled();
});
test('retries the first pull on failure with backoff, then releases the gate', async () => {
// Pull failure is simulated by a mock that resolves without ever flipping
// h.state.syncedConfigs to a non-null array — the same observable state
// as a real pullChanges that threw and skipped setSyncResult. Without
// retries the configs sync would be stuck on this single failed attempt
// for the whole reader session (handleAutoSync only re-arms on
// progress.location changes), so the user's progress never reaches the
// server until they reopen the book.
h.state.syncedConfigs = null;
const { rerender } = renderHook(() => useProgressSync('h1-view1'));
// Initial attempt fires from the [progress] effect on mount.
await advance(0);
expect(pullCallCount()).toBe(1);
// First backoff = 1500ms.
await advance(1500);
expect(pullCallCount()).toBe(2);
// Second backoff = 4000ms.
await advance(4000);
expect(pullCallCount()).toBe(3);
// Third backoff = 10000ms.
await advance(10000);
expect(pullCallCount()).toBe(4);
// Gate released after exhausted retries — a subsequent location change
// takes the push branch instead of queueing another pull. Simulate the
// user paginating: mutate the shared progress state and force a render
// so the [progress?.location] effect re-arms handleAutoSync.
h.state.progress = { location: 'cfi-loc-next' };
rerender();
await advance(SYNC_PROGRESS_INTERVAL_SEC * 1000 + 100);
expect(pushCallCount()).toBeGreaterThanOrEqual(1);
});
test('a successful pull cancels the pending retry chain', async () => {
// Render with the default mock (syncedConfigs = [], which the [syncedConfigs]
// effect treats as a successful empty pull → configPulled flips on mount).
renderHook(() => useProgressSync('h1-view1'));
await advance(0);
const initialPulls = pullCallCount();
// Wait past every retry window — nothing should fire because the gate
// is already open and the retry timer was cancelled.
await advance(20000);
expect(pullCallCount()).toBe(initialPulls);
});
test('sync-book-progress event resets and re-runs the pull chain', async () => {
h.state.syncedConfigs = null;
renderHook(() => useProgressSync('h1-view1'));
await advance(0);
// Let one backoff fire, then user invokes a manual refresh.
await advance(1500);
const callsBeforeRefresh = pullCallCount();
expect(callsBeforeRefresh).toBe(2);
act(() => {
const listeners = h.eventListeners.get('sync-book-progress');
listeners?.forEach((fn) =>
fn(new CustomEvent('sync-book-progress', { detail: { bookKey: 'h1-view1' } })),
);
});
await flushMicrotasks();
// The refresh issues a fresh pull immediately.
expect(pullCallCount()).toBe(callsBeforeRefresh + 1);
// And the retry chain restarts from delay[0].
await advance(1500);
expect(pullCallCount()).toBe(callsBeforeRefresh + 2);
});
});
@@ -1,4 +1,4 @@
import { useCallback, useEffect } from 'react';
import { useCallback, useEffect, useRef } from 'react';
import { useEnv } from '@/context/EnvContext';
import { useBookDataStore } from '@/store/bookDataStore';
import { useReaderStore } from '@/store/readerStore';
@@ -11,6 +11,15 @@ export const useProgressAutoSave = (bookKey: string) => {
const { getProgress } = useReaderStore();
const progress = getProgress(bookKey);
// Tracks the location we last persisted (or, before the first save, the
// location loaded from disk at book open). We skip saveConfig when the
// in-memory location matches — saveConfig unconditionally bumps
// config.updatedAt, and a bump on the initial relocate makes the local
// record look newer than a fresher server-side push, so the next sync
// overwrites the server's progress with the stale local one (issue #4222).
const lastSavedLocationRef = useRef<string | null>(null);
const initializedRef = useRef(false);
// eslint-disable-next-line react-hooks/exhaustive-deps
const saveBookConfig = useCallback(
debounce(() => {
@@ -20,13 +29,34 @@ export const useProgressAutoSave = (bookKey: string) => {
if (useReaderStore.getState().getViewState(bookKey)?.previewMode) return;
const config = getConfig(bookKey);
if (!config) return;
const currentLocation = config.location ?? null;
if (!initializedRef.current) {
initializedRef.current = true;
lastSavedLocationRef.current = currentLocation;
return;
}
if (currentLocation === lastSavedLocationRef.current) return;
const settings = useSettingsStore.getState().settings;
await saveConfig(envConfig, bookKey, config, settings);
lastSavedLocationRef.current = currentLocation;
}, 500);
}, 1000),
[],
);
useEffect(() => {
// Snapshot the loaded-from-disk location before any progress events fire,
// so we don't treat the initial relocate as a user-driven change.
if (!initializedRef.current) {
const config = getConfig(bookKey);
if (config) {
initializedRef.current = true;
lastSavedLocationRef.current = config.location ?? null;
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [bookKey]);
useEffect(() => {
saveBookConfig();
// eslint-disable-next-line react-hooks/exhaustive-deps
@@ -3,7 +3,6 @@ import { useAuth } from '@/context/AuthContext';
import { useSync } from '@/hooks/useSync';
import { BookConfig, FIXED_LAYOUT_FORMATS } from '@/types/book';
import { useBookDataStore } from '@/store/bookDataStore';
import { useLibraryStore } from '@/store/libraryStore';
import { useReaderStore } from '@/store/readerStore';
import { useSettingsStore } from '@/store/settingsStore';
import { useTranslation } from '@/hooks/useTranslation';
@@ -14,41 +13,50 @@ import { eventDispatcher } from '@/utils/event';
import { DEFAULT_BOOK_SEARCH_CONFIG, SYNC_PROGRESS_INTERVAL_SEC } from '@/services/constants';
import { getCFIFromXPointer, getXPointerFromCFI } from '@/utils/xcfi';
// Backoff schedule for the first-pull retry on book open. After these
// attempts the gate releases unconditionally so the user's progress can
// still sync out even if the server keeps timing out (high Android network
// concurrency, captive portal, transient 5xx). Total window ≈ 15.5s.
const PULL_RETRY_DELAYS_MS = [1500, 4000, 10000];
export const useProgressSync = (bookKey: string) => {
const _ = useTranslation();
const { getConfig, setConfig, getBookData } = useBookDataStore();
const { getView, getProgress, setHoveredBookKey } = useReaderStore();
const { settings } = useSettingsStore();
const { syncedConfigs, syncConfigs, syncBooks } = useSync(bookKey);
const { syncedConfigs, syncConfigs } = useSync(bookKey);
const { user } = useAuth();
const progress = getProgress(bookKey);
const configPulled = useRef(false);
const hasPulledConfigOnce = useRef(false);
const pullAttempt = useRef(0);
const pullInFlight = useRef(false);
const pullRetryTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
const clearPendingPullRetry = () => {
if (pullRetryTimer.current !== null) {
clearTimeout(pullRetryTimer.current);
pullRetryTimer.current = null;
}
};
const pushConfig = async (bookKey: string, config: BookConfig | null) => {
const book = getBookData(bookKey)?.book;
if (!config || !book || !user) return;
const bookHash = bookKey.split('-')[0]!;
const bookHash = book.hash;
const metaHash = book.metaHash;
const newConfig = { ...config, bookHash, metaHash };
const compressedConfig = JSON.parse(
serializeConfig(newConfig, settings.globalViewSettings, DEFAULT_BOOK_SEARCH_CONFIG),
);
delete compressedConfig.booknotes;
// The /api/sync POST handler piggybacks books.progress + books.updated_at
// off this configs push (saves the separate syncBooks round-trip that
// used to keep the library record fresh while a reader stayed open —
// see issue #4198). useBooksSync still seeds new books rows when the
// user is on the library page.
await syncConfigs([compressedConfig], bookHash, metaHash, 'push');
// Also push the corresponding `books` row. The library sync lane
// (useBooksSync) only runs while the library page is mounted, so while a
// reader stays open the server's `books` record is never re-pushed and
// other devices' library pull-to-refresh keeps showing stale progress
// (issue #4198). useProgressAutoSave has already merged config.progress
// into the in-memory library Book via saveConfig, so we just forward
// that book through the books lane.
const libraryBook = useLibraryStore.getState().library.find((b) => b.hash === bookHash);
if (libraryBook && !libraryBook.deletedAt) {
await syncBooks([libraryBook], 'push');
}
};
const pullConfig = async (bookKey: string) => {
@@ -59,9 +67,43 @@ export const useProgressSync = (bookKey: string) => {
await syncConfigs([], bookHash, metaHash, 'pull');
};
// Drives the pull on book open. A successful pull is signalled by the
// [syncedConfigs] effect below flipping `configPulled.current` to true and
// clearing the retry state — so this function just kicks off the next
// pull and (re)schedules a retry. If the gate is still closed after
// PULL_RETRY_DELAYS_MS is exhausted, release it unconditionally so the
// user's auto-push isn't blocked by a server outage. Re-entry while a
// pull is in flight or a retry timer is pending is a no-op.
const pullWithRetry = useCallback(async () => {
if (configPulled.current) return;
if (pullInFlight.current) return;
if (pullRetryTimer.current !== null) return;
pullInFlight.current = true;
try {
await pullConfig(bookKey);
} finally {
pullInFlight.current = false;
}
if (configPulled.current) return;
if (pullAttempt.current >= PULL_RETRY_DELAYS_MS.length) {
// Best-effort release. The server-side last-writer-wins compare still
// protects the cross-device case (a stale local push with an older
// updated_at will lose to a fresher server record).
configPulled.current = true;
return;
}
const delay = PULL_RETRY_DELAYS_MS[pullAttempt.current]!;
pullAttempt.current += 1;
pullRetryTimer.current = setTimeout(() => {
pullRetryTimer.current = null;
if (!configPulled.current) pullWithRetry();
}, delay);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [bookKey]);
const syncConfig = async () => {
if (!configPulled.current) {
pullConfig(bookKey);
pullWithRetry();
} else {
// Skip pushes while previewing a deep-link target — the position in
// memory reflects the annotation, not what the user is actually reading.
@@ -90,8 +132,13 @@ export const useProgressSync = (bookKey: string) => {
const handleSyncBookProgress = async (event: CustomEvent) => {
const { bookKey: syncBookKey } = event.detail;
if (syncBookKey === bookKey) {
// Manual pull-to-refresh: tear down any prior retry chain so the new
// attempt starts fresh, rather than being short-circuited by the
// "retry already pending" guard in pullWithRetry.
configPulled.current = false;
await pullConfig(bookKey);
pullAttempt.current = 0;
clearPendingPullRetry();
await pullWithRetry();
}
};
@@ -119,14 +166,20 @@ export const useProgressSync = (bookKey: string) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [progress?.location]);
// Pull: pull progress once when the book is opened
// Pull: pull progress once when the book is opened, with retry on failure
useEffect(() => {
if (!progress || hasPulledConfigOnce.current) return;
hasPulledConfigOnce.current = true;
pullConfig(bookKey);
pullWithRetry();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [progress]);
// Clean up any pending retry timer on unmount so it doesn't fire after the
// reader has been torn down.
useEffect(() => {
return () => clearPendingPullRetry();
}, []);
const applyRemoteProgress = async (syncedConfigs: BookConfig[]) => {
const config = getConfig(bookKey);
const book = getBookData(bookKey)?.book;
@@ -189,6 +242,10 @@ export const useProgressSync = (bookKey: string) => {
useEffect(() => {
if (!configPulled.current && syncedConfigs) {
configPulled.current = true;
// Pull succeeded — cancel any in-flight retry chain and reset the
// attempt counter so a future sync-book-progress event starts clean.
pullAttempt.current = 0;
clearPendingPullRetry();
applyRemoteProgress(syncedConfigs).catch((error) => {
console.error('Failed to apply remote progress', error);
});
+2 -2
View File
@@ -48,7 +48,7 @@ export class SyncClient {
Authorization: `Bearer ${token}`,
},
},
8000,
15000,
);
if (!res.ok) {
@@ -77,7 +77,7 @@ export class SyncClient {
},
body: JSON.stringify(payload),
},
8000,
15000,
);
if (!res.ok) {
+54
View File
@@ -310,6 +310,60 @@ export async function POST(req: NextRequest) {
if (configsResult?.error) throw new Error(configsResult.error);
if (notesResult?.error) throw new Error(notesResult.error);
// Piggyback the per-book reading progress from the configs push onto the
// matching `books` row. Other devices' library pull-to-refresh reads
// books.progress + books.updated_at, so without this the row would stay
// stale until the user navigates back to the library and useBooksSync
// re-pushes. The .lt('updated_at') predicate keeps last-writer-wins —
// a concurrent newer books push is never downgraded — and a missing
// row is a silent no-op (useBooksSync will insert it later).
type BookProgressUpdate = {
book_hash: string;
progress: [number, number];
updated_at: string;
};
const bookProgressUpdates: BookProgressUpdate[] = [];
for (const rec of (configsResult.data ?? []) as unknown as DBBookConfig[]) {
if (!rec.book_hash || !rec.updated_at || rec.progress == null) continue;
let parsed: unknown;
try {
parsed = typeof rec.progress === 'string' ? JSON.parse(rec.progress) : rec.progress;
} catch {
continue;
}
if (
!Array.isArray(parsed) ||
parsed.length !== 2 ||
typeof parsed[0] !== 'number' ||
typeof parsed[1] !== 'number'
) {
continue;
}
bookProgressUpdates.push({
book_hash: rec.book_hash,
progress: [parsed[0], parsed[1]],
updated_at: rec.updated_at,
});
}
if (bookProgressUpdates.length > 0) {
await Promise.all(
bookProgressUpdates.map(async (u) => {
const { error } = await supabase
.from('books')
.update({ progress: u.progress, updated_at: u.updated_at })
.eq('user_id', user.id)
.eq('book_hash', u.book_hash)
.lt('updated_at', u.updated_at);
if (error) {
// Best-effort: never fail the configs push because of this side
// effect — useBooksSync will reconcile the row later.
console.warn('books.progress piggyback failed for', u.book_hash, error.message);
}
}),
);
}
return NextResponse.json(
{
books: booksResult?.data || [],