diff --git a/web/package-lock.json b/web/package-lock.json index 035e3b542..e6a9c6976 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -50,6 +50,7 @@ "@testing-library/svelte": "^5.2.8", "@types/d3-scale": "4.0.9", "@types/qrcode": "^1.5.6", + "fake-indexeddb": "6.2.5", "jsdom": "^26.1.0", "marked": "^18.0.9", "svelte": "^5.56.8", @@ -2886,6 +2887,16 @@ "node": ">=12.0.0" } }, + "node_modules/fake-indexeddb": { + "version": "6.2.5", + "resolved": "https://registry.npmjs.org/fake-indexeddb/-/fake-indexeddb-6.2.5.tgz", + "integrity": "sha512-CGnyrvbhPlWYMngksqrSSUT1BAVP49dZocrHuK0SvtR0D5TMs5wP0o3j7jexDJW01KSadjBp1M/71o/KR3nD1w==", + "dev": true, + "license": "Apache-2.0", + "engines": { + "node": ">=18" + } + }, "node_modules/fdir": { "version": "6.5.0", "resolved": "https://registry.npmjs.org/fdir/-/fdir-6.5.0.tgz", diff --git a/web/package.json b/web/package.json index f96b8dcf4..e30a8285f 100644 --- a/web/package.json +++ b/web/package.json @@ -28,6 +28,7 @@ "@testing-library/svelte": "^5.2.8", "@types/d3-scale": "4.0.9", "@types/qrcode": "^1.5.6", + "fake-indexeddb": "6.2.5", "jsdom": "^26.1.0", "marked": "^18.0.9", "svelte": "^5.56.8", diff --git a/web/src/lib/stores/localIndexPersistence.idb.test.ts b/web/src/lib/stores/localIndexPersistence.idb.test.ts new file mode 100644 index 000000000..a1bd1a079 --- /dev/null +++ b/web/src/lib/stores/localIndexPersistence.idb.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect } from 'vitest'; +import type { ItemIndexRow } from '$lib/types'; +import { loadPersistence, rawItem, openSecondConnection, harnessDbName } from '../../test/idbHarness'; + +/** + * PLAN-2636 unit 1 — end-to-end IDB coverage for the localIndex persistence + * layer, which ran as a no-op under vitest until this project existed (no + * IndexedDB in the node env — Wren's #1148 finding). The sibling + * `localIndexPersistence.test.ts` pins the PURE decision (`shouldWriteRow`) + * and explicitly says it does NOT cover the interleaving outcome. This file + * covers exactly that outcome through a real (fake-indexeddb) database. + * + * The ruling on the PLAN-2636 trail: the family's races are SNAPSHOT- + * staleness, not read-staleness — the stale thing is the RAM snapshot handed + * to persistUpserts, never its `get`, which reads committed state. IDB + * serializes overlapping readwrite transactions, so the race BUG-2609 fixed + * reproduces with plain SEQUENTIAL calls: a newer delta commits its atomic + * rows+cursor transaction, then the older fire-and-forget snapshot lands last. + * No interleaving hook — none is needed, and an awaited barrier between a get + * and a put would fail on a real browser (TransactionInactiveError) anyway. + */ + +function row(id: string, seq: number | undefined, extra: Record = {}): ItemIndexRow { + return { id, seq, ...extra } as unknown as ItemIndexRow; +} + +describe('BUG-2609 end-to-end: stale snapshot does not clobber a newer persisted row', () => { + it('a delayed older-seq upsert landing after a newer delta is refused, and hydrate agrees', async () => { + const { persistDelta, persistUpserts, hydrate } = await loadPersistence(); + const U = null; + const WS = 'ws-2609'; + + // 1) The SSE delta lands first — newer-seq row + cursor advance, atomic. + await persistDelta(U, WS, [row('x', 7)], 'cursor-7', false); + // 2) The stale RAM snapshot (older seq) lands LAST — the fire-and-forget + // upsert that BUG-2609's evidence run captured mid-flight. + await persistUpserts(U, WS, [row('x', 5)]); + + // The seq guard refused the stale write: IDB still holds seq 7... + expect((await rawItem(U, WS, 'x'))?.seq).toBe(7); + // ...and a warm hydrate reflects that, with the cursor still past the delta. + const h = await hydrate(U, WS); + expect(h.items.find((i) => i.id === 'x')?.seq).toBe(7); + expect(h.cursor).toBe('cursor-7'); + }); + + it('a seq-less optimistic snapshot does not overwrite a stamped persisted row', async () => { + // The asymmetry leg, end-to-end: the optimistic-reorder path clears seq + // (TASK-1357); persisting it must not strand the cache with an + // unorderable row while the cursor sits past the correcting delta. + const { persistDelta, persistUpserts, hydrate } = await loadPersistence(); + const U = null; + const WS = 'ws-2609-asym'; + + await persistDelta(U, WS, [row('x', 7)], 'cursor-7', false); + await persistUpserts(U, WS, [row('x', undefined)]); // seq-less snapshot lands last + + expect((await rawItem(U, WS, 'x'))?.seq).toBe(7); + expect((await hydrate(U, WS)).items.find((i) => i.id === 'x')?.seq).toBe(7); + }); + + it('a genuinely newer upsert still wins (the guard refuses only STALE writes)', async () => { + const { persistUpserts, hydrate } = await loadPersistence(); + const U = null; + const WS = 'ws-2609-fwd'; + + await persistUpserts(U, WS, [row('x', 5)]); + await persistUpserts(U, WS, [row('x', 8)]); // newer — must land + + expect((await rawItem(U, WS, 'x'))?.seq).toBe(8); + expect((await hydrate(U, WS)).items.find((i) => i.id === 'x')?.seq).toBe(8); + }); +}); + +describe('IDB transaction serialization (platform contract this layer leans on)', () => { + // Optional characterization the lead invited: the persistence write path's + // correctness rests on IDB serializing overlapping readwrite transactions, + // so the get inside persistUpserts always reads committed state. This pins + // the guarantee at the raw-store level — a fake-indexeddb regression that + // broke it would surface here rather than as a mysterious flake upstream. + it('two overlapping readwrite transactions on the same store do not interleave', async () => { + const db = await openSecondConnection(null, 'ws-serialize'); + try { + const order: string[] = []; + + // Tx A: read, then (after a microtask) write — started first. + const txA = db.transaction('items', 'readwrite'); + const pA = (async () => { + await txA.objectStore('items').get('k'); + order.push('A:read'); + txA.objectStore('items').put({ id: 'k', seq: 1, who: 'A' }); + order.push('A:write'); + await txA.done; + order.push('A:done'); + })(); + + // Tx B: started while A is open; must not commit before A finishes. + const txB = db.transaction('items', 'readwrite'); + const pB = (async () => { + const seen = (await txB.objectStore('items').get('k')) as { who?: string } | undefined; + order.push(`B:read(${seen?.who ?? 'none'})`); + txB.objectStore('items').put({ id: 'k', seq: 2, who: 'B' }); + await txB.done; + order.push('B:done'); + })(); + + await Promise.all([pA, pB]); + + // A's transaction commits entirely before B's read runs — B sees A's + // committed write, never an interleaved half-state. (The exact order + // of the two read/write log lines within A is not asserted; the + // contract is that B:read observes A's value and B:done follows + // A:done.) + expect(order.indexOf('A:done')).toBeLessThan(order.indexOf('B:done')); + expect(order).toContain('B:read(A)'); + const final = (await rawViaDb(db)) ?? {}; + expect((final as { who?: string }).who).toBe('B'); + } finally { + db.close(); + } + }); +}); + +async function rawViaDb(db: Awaited>): Promise { + return db.get('items', 'k'); +} + +describe('database naming', () => { + it('the harness addresses the exact database the module writes to', async () => { + // Guards the vacuous-test failure mode: if harnessDbName drifted from + // the module's dbName, rawItem would read an empty sibling DB and every + // assertion above would pass for the wrong reason. Persist through the + // module, then read the raw store under the harness name directly. + const { persistUpserts } = await loadPersistence(); + await persistUpserts('user-42', 'My WS', [row('y', 1)]); + + const dbNameShouldBe = harnessDbName('user-42', 'My WS'); + expect(dbNameShouldBe).toBe( + `pad-local-index-${encodeURIComponent('user-42')}-${encodeURIComponent('My WS')}`, + ); + expect((await rawItem('user-42', 'My WS', 'y'))?.seq).toBe(1); + }); +}); diff --git a/web/src/test/idbHarness.idb.test.ts b/web/src/test/idbHarness.idb.test.ts new file mode 100644 index 000000000..dcee63a76 --- /dev/null +++ b/web/src/test/idbHarness.idb.test.ts @@ -0,0 +1,92 @@ +import { describe, it, expect } from 'vitest'; +import type { ItemIndexRow } from '$lib/types'; +import { + openSecondConnection, + seedV1Database, + openWithMigration, + versionErrorOnDowngradeOpen, + rawItem, +} from './idbHarness'; + +/** + * PLAN-2636 unit 1 — capability pins for the two things the lead named as + * blocking a harness shape (unit-2 test matrix prerequisites). These are + * living characterizations, not just a one-off spike: if a fake-indexeddb + * bump ever regressed either capability, unit 2's matrix would fail + * mysteriously; here it fails at the harness with a clear message. + */ + +function row(id: string, seq: number): ItemIndexRow { + return { id, seq } as unknown as ItemIndexRow; +} + +describe('cross-tab: two connections share one in-memory database (2635 prerequisite)', () => { + it('a write through one connection is visible through a second connection', async () => { + const U = null; + const WS = 'ws-crosstab'; + const tabA = await openSecondConnection(U, WS); + const tabB = await openSecondConnection(U, WS); + try { + await tabA.put('items', row('shared', 1)); + // tabB is a distinct handle; it must see tabA's committed write, the + // way two browser tabs on one origin share the database. + expect(((await tabB.get('items', 'shared')) as ItemIndexRow | undefined)?.seq).toBe(1); + + await tabB.put('items', row('shared', 2)); + expect(((await tabA.get('items', 'shared')) as ItemIndexRow | undefined)?.seq).toBe(2); + } finally { + tabA.close(); + tabB.close(); + } + }); +}); + +describe('format-version migration (unit-2 v1→v2 prerequisite)', () => { + it('a v1 database reopens under a higher format version with items retained + new store created', async () => { + const U = null; + const WS = 'ws-migrate'; + await seedV1Database(U, WS, [row('keep', 3)], { + cursor: 'c1', + schemaVersion: 3, + includesUnparentedMetadata: true, + }); + + const { oldVersion, db } = await openWithMigration(U, WS, 2, 'tombstones'); + try { + // The upgrade actually ran from v1 (not a fresh create). + expect(oldVersion).toBe(1); + // Pre-existing items survived the format bump. + expect(((await db.get('items', 'keep')) as ItemIndexRow | undefined)?.seq).toBe(3); + // The new store exists. + expect(db.objectStoreNames.contains('tombstones')).toBe(true); + // And the old meta row is intact. + expect((await db.get('meta', 'sync')) as { cursor?: string }).toMatchObject({ cursor: 'c1' }); + } finally { + db.close(); + } + }); + + it('an OLD build opening a since-upgraded database gets a VersionError (fail-safe degrade)', async () => { + const U = null; + const WS = 'ws-downgrade'; + await seedV1Database(U, WS, [row('x', 1)]); + // Newer build upgrades it to v2. + const { db } = await openWithMigration(U, WS, 2, 'tombstones'); + db.close(); + + // The old build still calls openDB(name, 1) → VersionError, which + // localIndexPersistence.open() catches and returns null for, degrading + // that tab to memory-only. Transient and acceptable during a deploy. + const errName = await versionErrorOnDowngradeOpen(U, WS, 1); + expect(errName).toBe('VersionError'); + }); +}); + +describe('per-test isolation', () => { + it('databases do not leak across tests (fresh IDBFactory per test)', async () => { + // If setup-idb's per-test factory reset regressed, a row written by an + // earlier test in this file would still be readable here. It must not be. + expect(await rawItem(null, 'ws-crosstab', 'shared')).toBeUndefined(); + expect(await rawItem(null, 'ws-migrate', 'keep')).toBeUndefined(); + }); +}); diff --git a/web/src/test/idbHarness.ts b/web/src/test/idbHarness.ts new file mode 100644 index 000000000..da3828213 --- /dev/null +++ b/web/src/test/idbHarness.ts @@ -0,0 +1,173 @@ +// idb test harness (PLAN-2636 unit 1). Helpers the localIndex persistence +// regression tests build on. All of these run under the `idb` vitest project, +// whose setup (`setup-idb.ts`) installs a fresh fake-indexeddb per test. +// +// Design note (lead ruling on the PLAN-2636 trail): the localIndex cache +// races are SNAPSHOT-staleness, not read-staleness — the stale thing is the +// RAM snapshot handed to persistUpserts, never its own `get`, which always +// reads committed state. IDB serializes overlapping readwrite transactions, +// so those races reproduce with plain SEQUENTIAL calls; there is deliberately +// NO interleaving hook, and none is needed. (An awaited barrier between a +// get and a put would also fail on a real browser with +// TransactionInactiveError — a fake-indexeddb-only seam pinning behavior +// production can't have.) These helpers therefore give cross-tab CONNECTIONS +// and version-MIGRATION control, not scheduling control. + +import { openDB, type IDBPDatabase } from 'idb'; +import { vi } from 'vitest'; +import type { ItemIndexRow } from '$lib/types'; + +/** + * Mirrors localIndexPersistence.dbName EXACTLY so the harness addresses the + * same database the module writes to — `pad-local-index-{ns}-{ws}`, where the + * user namespace and workspace are `encodeURIComponent`-encoded and a null + * user is the literal `anon` namespace. A drift here would silently point the + * raw readers / second connection at a different database and make every + * assertion vacuous, so it is kept byte-identical to the source. + */ +export function harnessDbName(userId: string | null, ws: string): string { + const ns = userId ? encodeURIComponent(userId) : 'anon'; + return `pad-local-index-${ns}-${encodeURIComponent(ws)}`; +} + +/** + * Load a FRESH instance of the persistence module — `vi.resetModules()` + * clears its module-level connection cache (`dbs`) so a previous test's + * cached handle (pointing at the prior test's now-discarded IDBFactory) + * can't leak in. Pair with the per-test fresh factory in setup-idb.ts for + * full isolation. Returns the module's public surface. + */ +export async function loadPersistence(): Promise< + typeof import('$lib/stores/localIndexPersistence') +> { + vi.resetModules(); + return import('$lib/stores/localIndexPersistence'); +} + +/** + * A SECOND raw connection to the same (user, workspace) database — the + * "other tab". Used by the 2635 cross-tab pins: one connection persists an + * unmerged snapshot while another holds the merged row. fake-indexeddb backs + * both handles with one in-memory store within a single test context + * (verified in the unit-1 spike), so writes through one are visible to the + * other, exactly like two browser tabs. + * + * Opens at IDB format version 1 with the same stores localIndexPersistence + * creates, so it never triggers an unexpected upgrade. + */ +export async function openSecondConnection( + userId: string | null, + ws: string, +): Promise { + return openDB(harnessDbName(userId, ws), 1, { + upgrade(db) { + if (!db.objectStoreNames.contains('items')) { + db.createObjectStore('items', { keyPath: 'id' }); + } + if (!db.objectStoreNames.contains('meta')) { + db.createObjectStore('meta', { keyPath: 'key' }); + } + }, + }); +} + +/** + * Create a v1-SHAPED database (the pre-unit-2 layout: `items` + `meta`, IDB + * format version 1) and seed it, so unit 2's v1→v2 migration exercise can + * reopen it under the new upgrade path and assert the old data survives. The + * connection is closed before returning so the migration open isn't blocked. + */ +export async function seedV1Database( + userId: string | null, + ws: string, + items: ItemIndexRow[] = [], + meta?: { cursor: string; schemaVersion: number; includesUnparentedMetadata: boolean }, +): Promise { + const db = await openDB(harnessDbName(userId, ws), 1, { + upgrade(db) { + db.createObjectStore('items', { keyPath: 'id' }); + db.createObjectStore('meta', { keyPath: 'key' }); + }, + }); + try { + const tx = db.transaction(['items', 'meta'], 'readwrite'); + for (const row of items) tx.objectStore('items').put(row); + if (meta) { + tx.objectStore('meta').put({ key: 'sync', ...meta }); + } + await tx.done; + } finally { + db.close(); + } +} + +/** + * Read the raw `items` store for a database, bypassing localIndexPersistence + * entirely — the ground-truth assertion for what is actually persisted. + */ +export async function rawItems(userId: string | null, ws: string): Promise { + const db = await openSecondConnection(userId, ws); + try { + return (await db.getAll('items')) as ItemIndexRow[]; + } finally { + db.close(); + } +} + +/** Raw read of a single persisted item by id (or undefined). */ +export async function rawItem( + userId: string | null, + ws: string, + id: string, +): Promise { + const db = await openSecondConnection(userId, ws); + try { + return (await db.get('items', id)) as ItemIndexRow | undefined; + } finally { + db.close(); + } +} + +/** + * Open a database at a HIGHER IDB format version to create a new object + * store, mirroring what unit 2's v1→v2 bump will do, and assert the pre- + * existing data survived the upgrade. Returns the observed `oldVersion` so a + * test can pin that the upgrade actually ran (vs a clean create). + */ +export async function openWithMigration( + userId: string | null, + ws: string, + toVersion: number, + newStore: string, +): Promise<{ oldVersion: number; db: IDBPDatabase }> { + let oldVersion = -1; + const db = await openDB(harnessDbName(userId, ws), toVersion, { + upgrade(db, from) { + oldVersion = from; + if (!db.objectStoreNames.contains(newStore)) { + db.createObjectStore(newStore, { keyPath: 'id' }); + } + }, + }); + return { oldVersion, db }; +} + +/** + * Assert the fail-safe degrade: an OLD build opening a database at format + * version `oldVersion` after a newer build already upgraded it to a higher + * version gets a VersionError (which localIndexPersistence catches → returns + * null → that tab degrades to memory-only). Returns the caught error name. + */ +export async function versionErrorOnDowngradeOpen( + userId: string | null, + ws: string, + oldVersion: number, +): Promise { + try { + const db = await openDB(harnessDbName(userId, ws), oldVersion); + db.close(); + return null; + } catch (e) { + return (e as Error)?.name ?? String(e); + } +} diff --git a/web/src/test/setup-idb.ts b/web/src/test/setup-idb.ts new file mode 100644 index 000000000..a095af948 --- /dev/null +++ b/web/src/test/setup-idb.ts @@ -0,0 +1,26 @@ +// Setup for the `idb` vitest project (PLAN-2636 unit 1). Installs +// fake-indexeddb's in-memory IndexedDB as the global `indexedDB` (and the full +// set of `IDB*` interface globals — `IDBRequest`, `IDBKeyRange`, `IDBDatabase` +// … — which the `idb` wrapper references) so `localIndexPersistence` — which +// no-ops whenever `typeof indexedDB === 'undefined'` — actually exercises its +// write/read path under test. Before this project existed the entire +// persistence layer shipped on live-browser evidence runs only (Wren's #1148 +// finding). +// +// `fake-indexeddb/auto` installs every IDB global once at import. A FRESH +// `IDBFactory` per test is the isolation boundary: every test starts with zero +// databases, so no cross-test state leaks through the shared in-memory store. +// The interface classes are stateless and stay put; only the factory (which +// owns the databases) is swapped. Tests that also need localIndexPersistence's +// own module-level connection cache reset pair this with `loadPersistence()` +// (vi.resetModules) from the idb harness. + +import 'fake-indexeddb/auto'; +import { IDBFactory } from 'fake-indexeddb'; +import { beforeEach } from 'vitest'; + +beforeEach(() => { + // A brand-new factory drops every database created by the previous test. + // The IDB* interface globals installed by `/auto` above are unaffected. + globalThis.indexedDB = new IDBFactory(); +}); diff --git a/web/vitest.config.ts b/web/vitest.config.ts index 23555dbb2..41d840d0b 100644 --- a/web/vitest.config.ts +++ b/web/vitest.config.ts @@ -69,7 +69,19 @@ const browserTestDepsInstalled = canResolve('@testing-library/jest-dom') && canResolve('@sveltejs/vite-plugin-svelte'); +// The `idb` project needs fake-indexeddb (PLAN-2636 unit 1). Like the jsdom +// deps it's declared in package.json but absent in a symlinked worktree until +// npm install runs; when it can't be resolved we register only the other +// projects so `npm run test` stays green, and the idb suite activates +// automatically once the dep is present (mirrors the jsdom self-disable). +const idbTestDepInstalled = canResolve('fake-indexeddb'); + const BROWSER_TEST_GLOB = 'src/**/*.svelte.test.ts'; +// IDB-backed persistence tests. The `.idb.test.ts` suffix routes them to the +// dedicated `idb` project; they must be excluded from the node project (which +// has no indexedDB — the persistence layer would silently no-op there and the +// test would pass vacuously) the same way the svelte glob is. +const IDB_TEST_GLOB = 'src/**/*.idb.test.ts'; const nodeProject = { resolve: { alias: { $lib } }, @@ -77,14 +89,34 @@ const nodeProject = { name: 'node', environment: 'node', include: ['src/**/*.test.ts'], - // The jsdom project owns these; they'd blow up in the plain node env. - exclude: [BROWSER_TEST_GLOB], + // The jsdom / idb projects own these; they'd blow up or no-op in the + // plain node env. + exclude: [BROWSER_TEST_GLOB, IDB_TEST_GLOB], + }, +}; + +const idbProject = { + resolve: { alias: { $lib } }, + server: nodeModulesRealPath + ? { fs: { allow: [projectRoot, nodeModulesRealPath] } } + : undefined, + test: { + name: 'idb', + // fake-indexeddb runs in plain node — no DOM needed. Its setup installs + // a fresh in-memory IndexedDB per test. + environment: 'node', + include: [IDB_TEST_GLOB], + setupFiles: ['./src/test/setup-idb.ts'], }, }; export default defineConfig(async () => { const projects: Record[] = [nodeProject]; + if (idbTestDepInstalled) { + projects.push(idbProject); + } + if (browserTestDepsInstalled) { // Dynamic import so a missing plugin can never crash config loading. const { svelte } = await import('@sveltejs/vite-plugin-svelte');