From 008577a7784c2c65166c2a136669d8f5a91577e2 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 17 Jun 2026 07:14:58 -0700 Subject: [PATCH] =?UTF-8?q?fix(sync):=20pre-landing=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20monotonic=20clock=20seed,=20scoped=20CHECK=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial (codex) review of the implementation diff caught three: - P1 (correctness): the fresh-schema setval was not monotonic. initSchema replays the schema blob, and the unconditional setval(MAX(generation)) could move page_generation_clock_seq.last_value BACKWARD on an already-upgraded brain, letting a stored query_cache bookmark serve stale rows. Seed via GREATEST over the sequence's OWN last_value (+ old table value + MAX(generation)) in all 3 fresh schemas and migration v118, so a replay is idempotent — mirrors the old table's ON CONFLICT DO NOTHING. Pinned by a new monotonic regression test. - P2: v119's CHECK-exists guard keyed on conname only (not globally unique). Scope it to conrelid = 'op_checkpoints'::regclass. - P3: in-progress note ran into the prior sentence in fail/warn doctor messages; separate it with '. '. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/doctor.ts | 2 +- src/core/migrate.ts | 5 ++++- src/core/pglite-schema.ts | 11 +++++++++-- src/core/schema-embedded.ts | 12 ++++++++++-- src/schema.sql | 12 ++++++++++-- test/page-generation-counter.test.ts | 17 +++++++++++++++++ 6 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 57d90ddb8..f931b5486 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -3531,7 +3531,7 @@ export async function checkSyncFreshness( const details = { unchanged_count, synced_recently_count, stale_count }; // BUG 4: append in-progress context when any source is actively syncing. // Empty otherwise, so steady-state messages are byte-for-byte unchanged. - const inProgressNote = inProgress.length ? ` ${inProgress.join('; ')}.` : ''; + const inProgressNote = inProgress.length ? `. ${inProgress.join('; ')}` : ''; if (hasFailures) { return { diff --git a/src/core/migrate.ts b/src/core/migrate.ts index fceb81bd7..acc57aec2 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -5306,6 +5306,7 @@ export const MIGRATIONS: Migration[] = [ SELECT setval('page_generation_clock_seq', GREATEST( 1, + COALESCE((SELECT last_value FROM page_generation_clock_seq), 0), COALESCE((SELECT value FROM page_generation_clock WHERE id = 1), 0), COALESCE((SELECT MAX(generation) FROM pages), 0) )); @@ -5355,7 +5356,9 @@ export const MIGRATIONS: Migration[] = [ DO $$ BEGIN IF NOT EXISTS ( - SELECT 1 FROM pg_constraint WHERE conname = 'op_checkpoints_completed_keys_array' + SELECT 1 FROM pg_constraint + WHERE conname = 'op_checkpoints_completed_keys_array' + AND conrelid = 'op_checkpoints'::regclass ) THEN ALTER TABLE op_checkpoints ADD CONSTRAINT op_checkpoints_completed_keys_array diff --git a/src/core/pglite-schema.ts b/src/core/pglite-schema.ts index 1279ce910..1a0bc6c19 100644 --- a/src/core/pglite-schema.ts +++ b/src/core/pglite-schema.ts @@ -169,8 +169,15 @@ INSERT INTO page_generation_clock (id, value) -- the first write strictly exceeds the seed; floor 1 (sequence MINVALUE). -- Layer-1 reads last_value. Table + trigger names retained. CREATE SEQUENCE IF NOT EXISTS page_generation_clock_seq; -SELECT setval('page_generation_clock_seq', - GREATEST(1, COALESCE((SELECT MAX(generation) FROM pages), 0))); +-- Monotonic seed: GREATEST over the sequence's OWN last_value too, so replaying +-- this blob on an already-upgraded brain (initSchema is re-runnable) can never +-- move last_value BACKWARD below a stored query_cache bookmark. +SELECT setval('page_generation_clock_seq', GREATEST( + 1, + COALESCE((SELECT last_value FROM page_generation_clock_seq), 0), + COALESCE((SELECT value FROM page_generation_clock WHERE id = 1), 0), + COALESCE((SELECT MAX(generation) FROM pages), 0) +)); CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS $func$ BEGIN diff --git a/src/core/schema-embedded.ts b/src/core/schema-embedded.ts index d0e2c51ef..32beb37d0 100644 --- a/src/core/schema-embedded.ts +++ b/src/core/schema-embedded.ts @@ -233,8 +233,16 @@ INSERT INTO page_generation_clock (id, value) -- first write strictly exceeds the seed; floor 1 (sequence MINVALUE). Layer-1 -- reads last_value. The table + trigger names are retained. CREATE SEQUENCE IF NOT EXISTS page_generation_clock_seq; -SELECT setval('page_generation_clock_seq', - GREATEST(1, COALESCE((SELECT MAX(generation) FROM pages), 0))); +-- Monotonic seed: GREATEST over the sequence's OWN last_value too, so replaying +-- this blob on an already-upgraded brain (initSchema is re-runnable) can never +-- move last_value BACKWARD below a stored query_cache bookmark (which would let +-- Layer 1 serve stale rows). Mirrors the old table's ON CONFLICT DO NOTHING. +SELECT setval('page_generation_clock_seq', GREATEST( + 1, + COALESCE((SELECT last_value FROM page_generation_clock_seq), 0), + COALESCE((SELECT value FROM page_generation_clock WHERE id = 1), 0), + COALESCE((SELECT MAX(generation) FROM pages), 0) +)); CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS \$func\$ BEGIN diff --git a/src/schema.sql b/src/schema.sql index e02bc7f02..dcf9455e1 100644 --- a/src/schema.sql +++ b/src/schema.sql @@ -229,8 +229,16 @@ INSERT INTO page_generation_clock (id, value) -- first write strictly exceeds the seed; floor 1 (sequence MINVALUE). Layer-1 -- reads last_value. The table + trigger names are retained. CREATE SEQUENCE IF NOT EXISTS page_generation_clock_seq; -SELECT setval('page_generation_clock_seq', - GREATEST(1, COALESCE((SELECT MAX(generation) FROM pages), 0))); +-- Monotonic seed: GREATEST over the sequence's OWN last_value too, so replaying +-- this blob on an already-upgraded brain (initSchema is re-runnable) can never +-- move last_value BACKWARD below a stored query_cache bookmark (which would let +-- Layer 1 serve stale rows). Mirrors the old table's ON CONFLICT DO NOTHING. +SELECT setval('page_generation_clock_seq', GREATEST( + 1, + COALESCE((SELECT last_value FROM page_generation_clock_seq), 0), + COALESCE((SELECT value FROM page_generation_clock WHERE id = 1), 0), + COALESCE((SELECT MAX(generation) FROM pages), 0) +)); CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS $func$ BEGIN diff --git a/test/page-generation-counter.test.ts b/test/page-generation-counter.test.ts index 989455ee5..25f57f94b 100644 --- a/test/page-generation-counter.test.ts +++ b/test/page-generation-counter.test.ts @@ -295,4 +295,21 @@ describe('v0.42.x sequence-backed clock (BUG 1: contention removal)', () => { expect(Number(n3[0].v)).toBe(101); await engine.executeRaw(`DROP SEQUENCE test_probe_seq`); }); + + test('re-seeding is monotonic: the GREATEST guard never moves last_value backward', async () => { + // Regression for the codex P1: initSchema replays the schema blob, whose + // setval must NOT reset the clock below its current value — a backward move + // would let a stored query_cache bookmark serve stale rows. Push the + // sequence high, then run the EXACT monotonic seed the blob + v118 use. + await engine.executeRaw(`SELECT setval('page_generation_clock_seq', 999999)`); + await engine.executeRaw( + `SELECT setval('page_generation_clock_seq', GREATEST( + 1, + COALESCE((SELECT last_value FROM page_generation_clock_seq), 0), + COALESCE((SELECT value FROM page_generation_clock WHERE id = 1), 0), + COALESCE((SELECT MAX(generation) FROM pages), 0) + ))`, + ); + expect(await clockValue()).toBeGreaterThanOrEqual(999999); + }); });