From 0aeadc420335e6859d20ddd2c26891c1fb2af532 Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:06:09 -0700 Subject: [PATCH 1/2] fix(test): make the reconcile-lease tests deterministic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/store/gmail-watch-state.test.ts` failed on roughly half of all runs of the file alone (observed 4/6 on clean main), on either or both of the two assertions that compare one lease token against another: AssertionError: expected '2026-08-02 11:48:36.747-08' not to be '2026-08-02 11:48:36.747-08' ## Root cause The lease token IS `claimed_until` — `now() + leaseMs` — and PGlite's `now()` advances only in whole MILLISECONDS (verified: its `::text` rendering never carries sub-millisecond digits). Two claims of the same `leaseMs` landing in the same millisecond therefore render the identical token. The `expireReconcileLease` helper rewound `claimed_until` in SQL, which faked the expiry WITHOUT the elapsed time that expiry implies — so the successor's claim ran in the same millisecond as its predecessor's and the two tokens came back equal. In production the successor can only claim once the prior lease has actually expired, i.e. at least `leaseMs` later, so its `claimed_until` is necessarily later and the tokens necessarily differ regardless of clock precision. The SQL shortcut was the only thing removing that guarantee. ## Fix Expire the lease the way production does: advance the clock. PGlite reads the JS system clock for `now()` (verified), so the lease `describe` block fakes `Date` only — `vi.useFakeTimers({ toFake: ['Date'] })`, leaving `setTimeout` real so nothing PGlite relies on stalls — and the helper moves the clock past `leaseMs`. No retries, no widened tolerances, no sleeps; the tests assert exactly what they asserted before. ## Verification - Reproduced first on unmodified main: 4/6 runs failed. - 12/12 consecutive green runs of the file alone after the fix. - Not vacuous: reverting the store's `AND claimed_until = $2::timestamptz` release guard still fails the stale-holder test. - Full suite 1340 passed / 0 failed; `tsc` clean for this file; Biome clean. Test-only change — `src/store/gmail-watch-state.ts` is untouched. Co-Authored-By: Claude Opus 5 --- src/store/gmail-watch-state.test.ts | 44 +++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/store/gmail-watch-state.test.ts b/src/store/gmail-watch-state.test.ts index b7b4c52..3bceb0a 100644 --- a/src/store/gmail-watch-state.test.ts +++ b/src/store/gmail-watch-state.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createPgliteDb, type Db } from '../db/client.js' import { migrate } from '../db/migrate.js' import { createGmailWatchStateStore } from './gmail-watch-state.js' @@ -349,15 +349,41 @@ describe('createGmailWatchStateStore', () => { // --- claimReconcileLease / releaseReconcileLease (HT-48, gmail-push.md §6) --- - /** Directly rewinds a mailbox's claimed_until into the past — mirrors conversations.test.ts's expireLease for the outbound lease, exercising expiry without a real sleep. */ - async function expireReconcileLease(db: Db, mailboxId: string) { - await db.query( - "UPDATE gmail_watch_state SET claimed_until = now() - interval '1 second' WHERE mailbox_id = $1", - [mailboxId], - ) + /** + * Expire a lease the way production does — by moving the clock past it, not + * by rewinding `claimed_until` with SQL. + * + * ## Why not the SQL rewind these tests used to do + * + * The lease token IS `claimed_until` (`now() + leaseMs`), so two claims + * whose `now()` lands on the same instant with the same `leaseMs` render the + * IDENTICAL token — and PGlite's `now()` only advances in whole + * MILLISECONDS, so back-to-back claims collide routinely. Rewinding + * `claimed_until` in SQL faked the expiry WITHOUT the elapsed time it + * implies, so the successor's claim ran in the same millisecond as its + * predecessor's and the two tokens came back equal — a real-clock race that + * failed roughly half of all runs. + * + * In production the successor can only claim once the prior lease has + * actually expired, i.e. at least `leaseMs` later, so its `claimed_until` is + * necessarily later too and the tokens necessarily differ. Advancing the + * (faked) system clock — which PGlite reads for `now()` — reproduces that + * guarantee exactly, and deterministically. + */ + function expireReconcileLease(leaseMs: number) { + vi.setSystemTime(Date.now() + leaseMs + 1_000) } describe('claimReconcileLease / releaseReconcileLease', () => { + // Only `Date` is faked: PGlite reads the system clock for `now()`, but + // faking timers wholesale would stall any `setTimeout` it relies on. + beforeEach(() => { + vi.useFakeTimers({ toFake: ['Date'] }) + }) + afterEach(() => { + vi.useRealTimers() + }) + it('claims an unclaimed mailbox, setting claimed_until in the future and returning it as the lease token', async () => { const { db, store } = await freshStore() const mailboxId = await insertMailbox(db) @@ -403,7 +429,7 @@ describe('createGmailWatchStateStore', () => { const first = await store.claimReconcileLease(mailboxId, 30_000) expect(first).not.toBeNull() - await expireReconcileLease(db, mailboxId) + expireReconcileLease(30_000) const second = await store.claimReconcileLease(mailboxId, 30_000) expect(second).not.toBeNull() @@ -469,7 +495,7 @@ describe('createGmailWatchStateStore', () => { // its lease before ever calling release. const tokenA = await store.claimReconcileLease(mailboxId, 30_000) expect(tokenA).not.toBeNull() - await expireReconcileLease(db, mailboxId) + expireReconcileLease(30_000) // Holder B — a legitimate successor — claims the now-expired lease and // is actively working. From f9ab2fef4520c2ea1c80ba30af550a19ee4b3c10 Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:26:29 -0700 Subject: [PATCH 2/2] fix(test): pin the lease tests to a fixed clock, tighten the expiry step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review follow-ups (Codex substitute, CodeRabbit rate limited): - Pin the faked clock to a constant instant rather than inheriting the wall clock at hook time. The assertions were all relative so this was not a flake source, but the point of the change is to remove real-clock dependence from the block, and inheriting `Date.now()` left some. - Advance by `leaseMs + 1` ms rather than `+ 1s`. One millisecond is the smallest step PGlite's `now()` can resolve, so the claim guard's strict `claimed_until < now()` boundary stays under test instead of being cleared by a wide margin. - Move `expireReconcileLease` inside the describe block that installs the fake timers — it calls `vi.setSystemTime` unconditionally, so at file scope a future caller outside that block would hit a non-obvious throw. - Tighten the helper's doc: it models the expiry, not the passage of time — the clock stays frozen at its new instant until moved again. Re-verified: 12/12 consecutive green runs; the stale-holder test still fails when the store's `AND claimed_until = $2::timestamptz` release guard is reverted, so it remains non-vacuous. Co-Authored-By: Claude Opus 5 --- src/store/gmail-watch-state.test.ts | 68 +++++++++++++++++------------ 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/src/store/gmail-watch-state.test.ts b/src/store/gmail-watch-state.test.ts index 3bceb0a..cd297db 100644 --- a/src/store/gmail-watch-state.test.ts +++ b/src/store/gmail-watch-state.test.ts @@ -349,41 +349,55 @@ describe('createGmailWatchStateStore', () => { // --- claimReconcileLease / releaseReconcileLease (HT-48, gmail-push.md §6) --- - /** - * Expire a lease the way production does — by moving the clock past it, not - * by rewinding `claimed_until` with SQL. - * - * ## Why not the SQL rewind these tests used to do - * - * The lease token IS `claimed_until` (`now() + leaseMs`), so two claims - * whose `now()` lands on the same instant with the same `leaseMs` render the - * IDENTICAL token — and PGlite's `now()` only advances in whole - * MILLISECONDS, so back-to-back claims collide routinely. Rewinding - * `claimed_until` in SQL faked the expiry WITHOUT the elapsed time it - * implies, so the successor's claim ran in the same millisecond as its - * predecessor's and the two tokens came back equal — a real-clock race that - * failed roughly half of all runs. - * - * In production the successor can only claim once the prior lease has - * actually expired, i.e. at least `leaseMs` later, so its `claimed_until` is - * necessarily later too and the tokens necessarily differ. Advancing the - * (faked) system clock — which PGlite reads for `now()` — reproduces that - * guarantee exactly, and deterministically. - */ - function expireReconcileLease(leaseMs: number) { - vi.setSystemTime(Date.now() + leaseMs + 1_000) - } - describe('claimReconcileLease / releaseReconcileLease', () => { - // Only `Date` is faked: PGlite reads the system clock for `now()`, but - // faking timers wholesale would stall any `setTimeout` it relies on. + /** + * These tests run on a FROZEN, fixed clock. Only `Date` is faked — PGlite + * reads the system clock for `now()`, so faking it moves the database's + * clock too, while leaving `setTimeout` real so nothing PGlite relies on + * stalls. The clock is pinned to a constant instant rather than whatever + * `useFakeTimers` inherited from the wall clock, so no assertion in this + * block can depend on when it happened to run; time moves only when + * {@link expireReconcileLease} moves it. + */ + const FROZEN_NOW = new Date('2026-01-01T00:00:00.000Z') + beforeEach(() => { vi.useFakeTimers({ toFake: ['Date'] }) + vi.setSystemTime(FROZEN_NOW) }) afterEach(() => { vi.useRealTimers() }) + /** + * Expire a lease the way production does — by moving the clock past it, + * not by rewinding `claimed_until` with SQL. Advances to ONE MILLISECOND + * past expiry (the smallest step PGlite's `now()` can resolve), so the + * claim guard's strict `claimed_until < now()` boundary stays under test + * rather than being cleared by a wide margin. + * + * ## Why not the SQL rewind these tests used to do + * + * The lease token IS `claimed_until` (`now() + leaseMs`), so two claims + * whose `now()` lands on the same instant with the same `leaseMs` render + * the IDENTICAL token — and PGlite's `now()` only advances in whole + * MILLISECONDS, so back-to-back claims collide routinely. Rewinding + * `claimed_until` in SQL faked the expiry WITHOUT the elapsed time it + * implies, so the successor's claim ran in the same millisecond as its + * predecessor's and the two tokens came back equal — a real-clock race + * that failed roughly half of all runs. + * + * In production the successor can only claim once the prior lease has + * actually expired, i.e. at least `leaseMs` later, so its `claimed_until` + * is necessarily later too and the tokens necessarily differ. Jumping the + * clock past the lease reproduces that ORDERING guarantee deterministically + * — it models the expiry, not the passage of time itself: the clock stays + * frozen at its new instant until moved again. + */ + function expireReconcileLease(leaseMs: number) { + vi.setSystemTime(Date.now() + leaseMs + 1) + } + it('claims an unclaimed mailbox, setting claimed_until in the future and returning it as the lease token', async () => { const { db, store } = await freshStore() const mailboxId = await insertMailbox(db)