From 9155ae627ccb74e22c43489d2e4459059a212c32 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 22 Jun 2026 03:24:01 +0800 Subject: [PATCH] feat(sync): decouple the incremental-pull cursor from updated_at via server synced_at (#4678) (#4712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(sync): decouple the incremental-pull cursor from updated_at (#4678) `books.updated_at` was overloaded as both the incremental-pull cursor (`GET /api/sync?since=…` filters `updated_at > since`, devices keep one global `max(updated_at)` watermark) and the library "date read" sort key. A server-resolved reading-status merge had to be written with a timestamp greater than every peer's global cursor to propagate, which forced `updated_at = now()` and reordered the date-read library by sync-processing time (the #4677 symptom). Introduce a server-assigned `synced_at` column on `books`, stamped by a `BEFORE INSERT OR UPDATE` trigger on every write, used only as the pull cursor. `updated_at` stays pure client event time used only for sorting. - Migration 016 + baseline schema: add `synced_at` (NOT NULL DEFAULT now()), index `(user_id, synced_at)`, trigger `set_books_synced_at`. Backfill `synced_at = updated_at` before creating the trigger so existing devices' cursors hand over without a re-sync storm. - GET: books filters/orders on `synced_at > since` (a delete bumps synced_at, so the deleted_at clause is dropped); configs/notes stay on updated_at. - POST: extract `buildStatusPropagationRow` and drop the `updated_at = now()` bump — the trigger advances synced_at so peers re-pull the status change while updated_at (the sort key) stays put. - Client `computeMaxTimestamp` keys on synced_at, falling back to updated_at/deleted_at for pre-migration servers and configs/notes. Backward-compatible: `synced_at >= updated_at` always, so `synced_at > since` is a strict superset of `updated_at > since` — old web clients and the koplugin keep working with no data loss (at worst a redundant idempotent re-pull of rare server-merged rows). The koplugin's shared pull/push cursor is left untouched; a proper split is a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sync): make the books synced_at backfill safe for large live tables (#4678) The single `UPDATE … WHERE synced_at IS NULL` deadlocked on a 3.8M-row production `books` table: it rewrites every row in one transaction while the live /api/sync push path upserts books rows, and the two lock rows in opposite orders. `ALTER COLUMN … SET NOT NULL` (full-table ACCESS EXCLUSIVE scan) and a plain CREATE INDEX (write-blocking SHARE lock) compounded it. Rework migration 016 as an online migration (run via psql, not in a wrapping transaction): - backfill in small autocommitted batches via a procedure, using FOR UPDATE SKIP LOCKED so it never waits on an app-locked row; - CREATE INDEX CONCURRENTLY instead of a blocking build; - install the trigger last (so it can't clobber the updated_at backfill); - drop the hard SET NOT NULL (the default + trigger + backfill keep the column populated and the client falls back to updated_at); a NOT VALID CHECK + VALIDATE alternative is included, commented, for operators who want it. The baseline schema.sql (fresh, empty installs) keeps the simple inline NOT NULL DEFAULT now() + trigger. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/hooks/useSync-cursor.test.ts | 31 +++++ .../pages/api/sync-synced-at-cursor.test.ts | 46 +++++++ apps/readest-app/src/hooks/useSync.ts | 18 ++- apps/readest-app/src/pages/api/sync.ts | 49 ++++++-- apps/readest-app/src/types/book.ts | 6 + docker/volumes/db/init/schema.sql | 22 ++++ .../db/migrations/016_add_books_synced_at.sql | 118 ++++++++++++++++++ 7 files changed, 275 insertions(+), 15 deletions(-) create mode 100644 apps/readest-app/src/__tests__/hooks/useSync-cursor.test.ts create mode 100644 apps/readest-app/src/__tests__/pages/api/sync-synced-at-cursor.test.ts create mode 100644 docker/volumes/db/migrations/016_add_books_synced_at.sql diff --git a/apps/readest-app/src/__tests__/hooks/useSync-cursor.test.ts b/apps/readest-app/src/__tests__/hooks/useSync-cursor.test.ts new file mode 100644 index 00000000..8da72617 --- /dev/null +++ b/apps/readest-app/src/__tests__/hooks/useSync-cursor.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { computeMaxTimestamp } from '@/hooks/useSync'; +import type { BookDataRecord } from '@/types/book'; + +const iso = (ms: number) => new Date(ms).toISOString(); + +// Issue #4678: the incremental-pull cursor is decoupled from updated_at. The +// server stamps a `synced_at` on every books write, so the client watermark +// keys on synced_at and ignores updated_at (the client event time / sort key) +// and deleted_at (a delete bumps synced_at too). +describe('computeMaxTimestamp (synced_at pull cursor)', () => { + it('keys on synced_at, ignoring updated_at and deleted_at', () => { + const max = computeMaxTimestamp([ + { synced_at: iso(5000), updated_at: iso(1000), deleted_at: null }, + { synced_at: iso(3000), updated_at: iso(9999), deleted_at: iso(8000) }, + ] as unknown as BookDataRecord[]); + expect(max).toBe(5000); + }); + + it('falls back to updated_at/deleted_at when synced_at is absent (old server)', () => { + const max = computeMaxTimestamp([ + { updated_at: iso(1000), deleted_at: iso(4000) }, + { updated_at: iso(2000), deleted_at: null }, + ] as unknown as BookDataRecord[]); + expect(max).toBe(4000); + }); + + it('returns 0 for empty input', () => { + expect(computeMaxTimestamp([])).toBe(0); + }); +}); diff --git a/apps/readest-app/src/__tests__/pages/api/sync-synced-at-cursor.test.ts b/apps/readest-app/src/__tests__/pages/api/sync-synced-at-cursor.test.ts new file mode 100644 index 00000000..aab7294f --- /dev/null +++ b/apps/readest-app/src/__tests__/pages/api/sync-synced-at-cursor.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest'; +import { buildStatusPropagationRow } from '@/pages/api/sync'; +import type { DBBook } from '@/types/records'; + +const iso = (ms: number) => new Date(ms).toISOString(); + +// Issue #4678: when the server wins a books row but the client's reading_status +// is newer, the change must still reach every peer. It used to be propagated by +// rewriting `updated_at = now()`, which reordered the date-read library by +// sync-processing time (#4677). Now the `synced_at` trigger advances the pull +// cursor on the write, so the propagation row keeps `updated_at` untouched. +describe('buildStatusPropagationRow', () => { + const serverBook = { + user_id: 'u', + book_hash: 'h', + format: 'EPUB', + title: 'T', + author: 'A', + updated_at: iso(1000), + reading_status: 'reading', + reading_status_updated_at: iso(500), + } as unknown as DBBook; + + const fresherStatus = { + reading_status: 'finished', + reading_status_updated_at: iso(2000), + }; + + it('grafts the fresher status onto the server row', () => { + const row = buildStatusPropagationRow(serverBook, fresherStatus); + expect(row.reading_status).toBe('finished'); + expect(row.reading_status_updated_at).toBe(iso(2000)); + }); + + it('leaves updated_at untouched so the date-read sort never jumps to sync time', () => { + const row = buildStatusPropagationRow(serverBook, fresherStatus); + expect(row.updated_at).toBe(serverBook.updated_at); + }); + + it('preserves the rest of the server row', () => { + const row = buildStatusPropagationRow(serverBook, fresherStatus); + expect(row.book_hash).toBe('h'); + expect(row.title).toBe('T'); + expect(row.format).toBe('EPUB'); + }); +}); diff --git a/apps/readest-app/src/hooks/useSync.ts b/apps/readest-app/src/hooks/useSync.ts index 19f40a0d..35db314d 100644 --- a/apps/readest-app/src/hooks/useSync.ts +++ b/apps/readest-app/src/hooks/useSync.ts @@ -20,16 +20,24 @@ const transformsFromDB = { configs: transformBookConfigFromDB, }; -const computeMaxTimestamp = (records: BookDataRecord[]): number => { +// The incremental-pull watermark. The server stamps a `synced_at` on every +// books write (issue #4678), so it — not updated_at — is the monotonic cursor: +// keying on it lets a server-resolved merge propagate without the date-read +// library jumping to sync-processing time. Rows without synced_at (configs, +// notes, or a pre-migration server) fall back to max(updated_at, deleted_at); +// for books a delete bumps synced_at too, so deleted_at need not be consulted. +export const computeMaxTimestamp = (records: BookDataRecord[]): number => { let maxTime = 0; for (const rec of records) { + if (rec.synced_at) { + maxTime = Math.max(maxTime, new Date(rec.synced_at).getTime()); + continue; + } if (rec.updated_at) { - const updatedTime = new Date(rec.updated_at).getTime(); - maxTime = Math.max(maxTime, updatedTime); + maxTime = Math.max(maxTime, new Date(rec.updated_at).getTime()); } if (rec.deleted_at) { - const deletedTime = new Date(rec.deleted_at).getTime(); - maxTime = Math.max(maxTime, deletedTime); + maxTime = Math.max(maxTime, new Date(rec.deleted_at).getTime()); } } return maxTime; diff --git a/apps/readest-app/src/pages/api/sync.ts b/apps/readest-app/src/pages/api/sync.ts index a80fcb7c..7a1fbb11 100644 --- a/apps/readest-app/src/pages/api/sync.ts +++ b/apps/readest-app/src/pages/api/sync.ts @@ -73,6 +73,28 @@ export function resolveReadingStatusMerge( }; } +/** + * Build the row written when the server wins a books row by `updated_at` but + * the client's reading_status is the fresher one: graft the status onto the + * server row and leave everything else — crucially `updated_at` — untouched. + * + * The `books_set_synced_at` trigger stamps `synced_at = now()` on this write, + * so peers re-pull the status change via the synced_at cursor without the + * date-read library (sorted by updated_at) jumping to sync-processing time. + * Previously this rewrote `updated_at = now()` to force propagation, which was + * the #4677 reorder symptom. See issue #4678. + */ +export function buildStatusPropagationRow( + serverBook: DBBook, + status: Pick, +): DBBook { + return { + ...serverBook, + reading_status: status.reading_status, + reading_status_updated_at: status.reading_status_updated_at, + }; +} + const transformsToDB = { books: transformBookToDB, book_notes: transformBookNoteToDB, @@ -131,6 +153,13 @@ export async function GET(req: NextRequest) { let offset = 0; let hasMore = true; + // books keys the pull on the server-assigned `synced_at` cursor, which a + // trigger bumps on every write — including deletes — so a server-resolved + // merge propagates without touching updated_at (the date-read sort key). + // configs/notes have no server-side merge, so they stay on updated_at and + // still need the explicit deleted_at clause. See issue #4678. + const cursorColumn = table === 'books' ? 'synced_at' : 'updated_at'; + while (hasMore) { let query = supabase .from(table) @@ -146,8 +175,12 @@ export async function GET(req: NextRequest) { query = query.eq('meta_hash', metaHashParam); } - query = query.or(`updated_at.gt.${sinceIso},deleted_at.gt.${sinceIso}`); - query = query.order('updated_at', { ascending: false }); + if (cursorColumn === 'synced_at') { + query = query.gt('synced_at', sinceIso); + } else { + query = query.or(`updated_at.gt.${sinceIso},deleted_at.gt.${sinceIso}`); + } + query = query.order(cursorColumn, { ascending: false }); console.log('Querying table:', table, 'since:', sinceIso, 'offset:', offset); @@ -438,17 +471,13 @@ export async function POST(req: NextRequest) { ); if (statusChanged) { // Server wins the row, but the client's status is newer. Write - // server's row with the fresher status and bump updated_at so - // peers re-pull the status change. + // server's row with the fresher status; the books_set_synced_at + // trigger advances synced_at so peers re-pull the change, while + // updated_at (the date-read sort key) stays put. See #4678. // The runtime DB row carries all DBBook columns; the static type // of `serverBook` is a narrower intersection so `unknown` is // required to bridge the gap at this one construction site. - toUpdate.push({ - ...serverBook, - reading_status: status.reading_status, - reading_status_updated_at: status.reading_status_updated_at, - updated_at: new Date().toISOString(), - } as unknown as DBBook); + toUpdate.push(buildStatusPropagationRow(serverBook as unknown as DBBook, status)); } else { batchAuthoritativeRecords.push(serverData); } diff --git a/apps/readest-app/src/types/book.ts b/apps/readest-app/src/types/book.ts index 5ef86215..40a16725 100644 --- a/apps/readest-app/src/types/book.ts +++ b/apps/readest-app/src/types/book.ts @@ -470,6 +470,12 @@ export interface BookDataRecord { user_id: string; updated_at: number | null; deleted_at: number | null; + // Server-assigned incremental-pull cursor, decoupled from updated_at (the + // client event time / sort key). Present on books rows from a server that + // ran migration 016; absent (fall back to updated_at) on older servers and + // on config/note records. Carried over the wire as an ISO-8601 string. + // See issue #4678. + synced_at?: string | null; // Only book records carry an upload state: a book is indexed in the cloud // as soon as its metadata syncs, but is unavailable to peers until its file // blob is uploaded. Absent on config/note records. diff --git a/docker/volumes/db/init/schema.sql b/docker/volumes/db/init/schema.sql index 5b817b3c..81613496 100644 --- a/docker/volumes/db/init/schema.sql +++ b/docker/volumes/db/init/schema.sql @@ -15,6 +15,7 @@ CREATE TABLE public.books ( updated_at timestamp with time zone NULL DEFAULT now(), deleted_at timestamp with time zone NULL, uploaded_at timestamp with time zone NULL, + synced_at timestamp with time zone NOT NULL DEFAULT now(), progress integer[] NULL, reading_status text NULL, reading_status_updated_at timestamp with time zone NULL, @@ -25,6 +26,27 @@ CREATE TABLE public.books ( CONSTRAINT books_user_id_fkey FOREIGN KEY (user_id) REFERENCES auth.users (id) ON DELETE CASCADE ); +-- Server-assigned incremental-pull cursor, decoupled from updated_at (the +-- client event time / sort key). A trigger stamps it on every write so a +-- server-resolved merge propagates without reordering the date-read library. +-- See migration 016_add_books_synced_at.sql (issue #4678). +CREATE INDEX idx_books_user_synced ON public.books (user_id, synced_at); + +CREATE OR REPLACE FUNCTION public.set_books_synced_at() +RETURNS trigger +LANGUAGE plpgsql +AS $$ +BEGIN + NEW.synced_at := now(); + RETURN NEW; +END; +$$; + +CREATE TRIGGER books_set_synced_at + BEFORE INSERT OR UPDATE ON public.books + FOR EACH ROW + EXECUTE FUNCTION public.set_books_synced_at(); + ALTER TABLE public.books ENABLE ROW LEVEL SECURITY; CREATE POLICY select_books ON public.books FOR SELECT TO authenticated USING ((SELECT auth.uid()) = user_id); CREATE POLICY insert_books ON public.books FOR INSERT TO authenticated WITH CHECK ((SELECT auth.uid()) = user_id); diff --git a/docker/volumes/db/migrations/016_add_books_synced_at.sql b/docker/volumes/db/migrations/016_add_books_synced_at.sql new file mode 100644 index 00000000..2d1085ca --- /dev/null +++ b/docker/volumes/db/migrations/016_add_books_synced_at.sql @@ -0,0 +1,118 @@ +-- Migration 016: Add a server-assigned `synced_at` cursor to books (issue #4678) +-- +-- `books.updated_at` was overloaded as two things with conflicting needs: +-- 1. the incremental-pull cursor (GET /api/sync?since=… filters updated_at > +-- since, and each device keeps a single global max(updated_at) watermark); +-- 2. the library "date read" sort key (wants the client event time). +-- +-- A server-resolved merge (e.g. the reading_status field-level LWW in #4634) +-- has to be written with a timestamp greater than every peer's global cursor to +-- propagate, which forced updated_at = now() and reordered the date-read library +-- by sync-processing time (the #4677 symptom). +-- +-- Decouple the two: `synced_at` is a monotonic, server-stamped cursor used ONLY +-- by the incremental pull, while `updated_at` stays pure client event time used +-- ONLY for sorting. A BEFORE INSERT/UPDATE trigger forces synced_at = now() on +-- every server write (clients never send it), so a status merge propagates by +-- bumping synced_at without touching updated_at. +-- +-- Backfill synced_at = updated_at so existing devices' updated_at-based cursors +-- hand over seamlessly: `synced_at > since` returns the same rows as before +-- (synced_at == updated_at) plus, going forward, server-resolved merges. +-- +-- ┌───────────────────────────────────────────────────────────────────────────┐ +-- │ RUN ONLINE, NOT INSIDE A TRANSACTION. │ +-- │ │ +-- │ Apply with psql against a live, large `books` table (millions of rows): │ +-- │ psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -f 016_add_books_synced_at.sql │ +-- │ │ +-- │ Do NOT paste it into a wrapping BEGIN/COMMIT or the Supabase dashboard SQL │ +-- │ editor: it uses CREATE INDEX CONCURRENTLY and a CALL into a procedure that │ +-- │ COMMITs each backfill batch — both are rejected inside a transaction. │ +-- │ │ +-- │ A single bulk `UPDATE … WHERE synced_at IS NULL` deadlocks against the │ +-- │ live /api/sync upserts (both lock books rows, in opposite orders). The │ +-- │ backfill below instead walks the table in small autocommitted batches and │ +-- │ uses FOR UPDATE SKIP LOCKED so it never waits on a row the app holds. │ +-- └───────────────────────────────────────────────────────────────────────────┘ + +-- 1. Add the column (nullable, no default → metadata-only, instant) and set the +-- default up front so rows INSERTed during the backfill already get now(). +ALTER TABLE public.books + ADD COLUMN IF NOT EXISTS synced_at timestamp with time zone NULL; +ALTER TABLE public.books + ALTER COLUMN synced_at SET DEFAULT now(); + +-- 2. Backfill in small, individually-committed batches. SKIP LOCKED steps over +-- rows currently locked by a concurrent push (they fall to a later pass), so +-- the backfill never deadlocks with live traffic. The trigger is installed +-- only AFTER this completes, so it can't clobber the updated_at backfill. +CREATE OR REPLACE PROCEDURE public.backfill_books_synced_at(batch_size int DEFAULT 10000) +LANGUAGE plpgsql +AS $$ +DECLARE + n int; +BEGIN + LOOP + WITH todo AS ( + SELECT ctid + FROM public.books + WHERE synced_at IS NULL + LIMIT batch_size + FOR UPDATE SKIP LOCKED + ) + UPDATE public.books b + SET synced_at = COALESCE(b.updated_at, b.created_at, now()) + FROM todo + WHERE b.ctid = todo.ctid; + + GET DIAGNOSTICS n = ROW_COUNT; + COMMIT; + + IF n = 0 THEN + -- A pass updated nothing: either we're done, or the only rows left are + -- momentarily app-locked. Stop when truly none remain, else briefly wait. + EXIT WHEN NOT EXISTS (SELECT 1 FROM public.books WHERE synced_at IS NULL); + PERFORM pg_sleep(0.1); + END IF; + END LOOP; +END; +$$; + +CALL public.backfill_books_synced_at(10000); +DROP PROCEDURE public.backfill_books_synced_at(int); + +-- 3. Index the cursor without blocking writes (CONCURRENTLY → no SHARE lock). +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_books_user_synced + ON public.books (user_id, synced_at); + +-- 4. Install the trigger LAST, so from here every write is server-stamped. +CREATE OR REPLACE FUNCTION public.set_books_synced_at() +RETURNS trigger +LANGUAGE plpgsql +AS $$ +BEGIN + -- Server-authoritative: ignore any client-supplied value and stamp the + -- transaction time. transaction_timestamp() (= now()) is stable within a + -- batch upsert, which is fine — a batch is a single pull delta. + NEW.synced_at := now(); + RETURN NEW; +END; +$$; + +DROP TRIGGER IF EXISTS books_set_synced_at ON public.books; +CREATE TRIGGER books_set_synced_at + BEFORE INSERT OR UPDATE ON public.books + FOR EACH ROW + EXECUTE FUNCTION public.set_books_synced_at(); + +-- 5. (Optional) Enforce NOT NULL without the full-table ACCESS EXCLUSIVE scan +-- that `ALTER COLUMN … SET NOT NULL` takes. A NOT VALID check skips existing +-- rows; VALIDATE then scans under a lighter SHARE UPDATE EXCLUSIVE lock that +-- still allows concurrent reads and writes. Safe to omit — the default, the +-- trigger and the backfill already keep the column populated, and the client +-- falls back to updated_at when synced_at is absent. +-- ALTER TABLE public.books +-- ADD CONSTRAINT books_synced_at_not_null CHECK (synced_at IS NOT NULL) NOT VALID; +-- ALTER TABLE public.books +-- VALIDATE CONSTRAINT books_synced_at_not_null;