feat(sync): decouple the incremental-pull cursor from updated_at via server synced_at (#4678) (#4712)

* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-22 03:24:01 +08:00
parent a9c0f3d46d
commit 9155ae627c
7 changed files with 275 additions and 15 deletions
@@ -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);
+  });
+});
@@ -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');
+  });
+});
@@ -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;
@@ -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, 'reading_status' | 'reading_status_updated_at'>,
+): 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);
              }
@@ -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.
@@ -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);
@@ -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;