From 37f086cd4f316126429fa9754b388aafcd652d50 Mon Sep 17 00:00:00 2001 From: Ivan Cheung Date: Wed, 2 Sep 2026 09:12:25 -0400 Subject: [PATCH] fix(lifeboard): harden state persistence An audit of the persistence mechanism turned up five defects. Each is confirmed by a test that fails without the fix. A SECOND TAB LOST ITS WRITES. Sequence numbers were minted from the in-memory tail, so two connections to the same database produced the same key, IndexedDB rejected the duplicate, and the write vanished -- a tick in the second tab reverted itself with no explanation. Reading and appending now happen in one transaction, and a tab absorbs whatever another wrote before deciding what its own change means, so an update against a record it had never seen is applied rather than dropped. A NEW BUILD NEVER REACHED ANYONE. app.js is not content-hashed and the service worker was cache-first, so a deploy was invisible until someone bumped the cache name by hand -- which nothing enforced. The shell is network-first with a cache fallback now: online you get the current build, offline you get the last one that worked. Content-hashed chunks stay cache-first, where that is free and correct. The e2e serves a changed build mid-run and asserts it arrives. THE APP DID NOT START WITHOUT STORAGE. A private window rejects the open, and the rejection was `undefined` because request errors were passed through unchecked. The result was a blank screen with nothing in the console. There is a backend seam now, with an in-memory fallback and a `durable` flag; the app boots and says plainly that it is not saving anything. TRANSCRIPT WRITES COULD REJECT WITH NOBODY LISTENING. Three console writes were fire-and-forget, so a full disk became an unhandled rejection. Losing a line of narration is survivable; losing it silently while the page logs an uncaught error is not. THE COST OF A WRITE GREW WITH THE AGE OF THE HOUSEHOLD. Every append re-folded the entire log and structuredClone'd every record: 0.4 ms per write at 200 entries, 4.8 ms at 3,000, and every tick writes twice. Entries now fold into the existing view incrementally. Also handles onblocked and onversionchange, which could otherwise leave the open promise neither resolved nor rejected -- an app hanging on a blank screen with no error at all. Co-Authored-By: Claude Opus 5 (1M context) --- packages/lifeboard/e2e/offline.e2e.mjs | 18 ++- packages/lifeboard/src/app.ts | 12 +- packages/lifeboard/src/log.ts | 10 +- packages/lifeboard/src/store.ts | 147 ++++++++++++++++++++----- packages/lifeboard/src/sw.js | 45 +++++--- packages/lifeboard/src/transcript.ts | 21 +++- packages/lifeboard/test/store.test.ts | 91 ++++++++++++++- 7 files changed, 294 insertions(+), 50 deletions(-) diff --git a/packages/lifeboard/e2e/offline.e2e.mjs b/packages/lifeboard/e2e/offline.e2e.mjs index 7ab3896..f08eeb3 100644 --- a/packages/lifeboard/e2e/offline.e2e.mjs +++ b/packages/lifeboard/e2e/offline.e2e.mjs @@ -17,10 +17,17 @@ const TYPES = { ".html": "text/html", ".js": "text/javascript", ".css": "text/cs const results = []; const ok = (n, c) => { results.push(c); console.log(`${c ? "PASS" : "FAIL"} ${n}`); }; +// Bumped mid-run to stand in for a deploy: the point of the shell being network-first is that a new +// build reaches a device that already has the old one cached. +let build = "build-1"; + const server = createServer(async (req, res) => { const path = req.url === "/" ? "/index.html" : req.url.split("?")[0]; try { - const body = await readFile(join(DIST, path)); + let body = await readFile(join(DIST, path)); + if (path === "/index.html") { + body = Buffer.from(body.toString().replace("", ``)); + } // Long-lived caching is what lets an offline reload be served from disk — the same header the // hashed bundle wants in production. res.writeHead(200, { "content-type": TYPES[extname(path)] ?? "application/octet-stream", "cache-control": "max-age=3600" }); @@ -102,6 +109,15 @@ await page.waitForTimeout(300); ok("Connections opens offline and offers a way in", await page.locator(".lb-add-assistant").isVisible()); ok("Connections says plainly that nothing is connected", /nothing connected yet/i.test((await page.locator("#board").textContent()) ?? "")); +// A deploy has to reach a device that already has the old build cached. app.js is not +// content-hashed, so a cache-first shell would serve the old one forever. +await page.context().setOffline(false); +build = "build-2"; +await page.reload(); +await page.waitForTimeout(300); +const served = await page.evaluate(() => document.querySelector('meta[name="lb-build"]')?.getAttribute("content")); +ok(`a new build reaches a device that cached the old one (served ${served})`, served === "build-2"); + ok(`no console errors (saw ${errors.length}${errors.length ? `: ${errors[0]}` : ""})`, errors.length === 0); // The only thing the app reaches for off its own origin is the local bridge, and it works without diff --git a/packages/lifeboard/src/app.ts b/packages/lifeboard/src/app.ts index 84afcab..4b014c0 100644 --- a/packages/lifeboard/src/app.ts +++ b/packages/lifeboard/src/app.ts @@ -5,7 +5,7 @@ import { syncGoogle } from "./google/sync.js"; import { localToday } from "./shell.js"; import { BridgeClient, bridgeToken, discoverBridge, hasUsedBridge, type BridgeInfo } from "./bridge/client.js"; import { browserPrefs, type Prefs } from "./prefs.js"; -import { openStore, type Store } from "./store.js"; +import { openStoreOrMemory, type Store } from "./store.js"; import { startShell, type Regions, type Shell } from "./shell.js"; import { add } from "./types.js"; @@ -17,7 +17,7 @@ export interface AppDeps { } export async function startApp(deps: AppDeps): Promise { - const store = deps.store ?? (await openStore()); + const store = deps.store ?? (await openStoreOrMemory()); const prefs = deps.prefs ?? browserPrefs(); const now = deps.now ?? (() => new Date()); await seedIfEmpty(store, localToday(now())); @@ -41,6 +41,14 @@ export async function startApp(deps: AppDeps): Promise { }, }); + // Said once, plainly, rather than letting someone tick things all evening and lose them. + if (!store.durable) { + shell.console.post({ + kind: "error", + text: "This device will not let lifeboard save anything — changes will be lost when you close the tab. Private browsing usually causes this.", + }); + } + // The catch-up pass, in the background. It must never delay the board coming up: the family's own // records are already on screen, and Google is the slow, failable part. const clientId = prefs.get("lifeboard.googleClientId"); diff --git a/packages/lifeboard/src/log.ts b/packages/lifeboard/src/log.ts index 5bf83e2..509b781 100644 --- a/packages/lifeboard/src/log.ts +++ b/packages/lifeboard/src/log.ts @@ -28,7 +28,15 @@ export interface EntryContext { * replaces a record in place; a retraction drops it. Both stay in the log. */ export function materialise(entries: readonly Entry[]): Map { - const view = new Map(); + return foldInto(new Map(), entries); +} + +/** + * Fold entries into an existing view, in place. Appending one entry should cost one operation, not + * a rebuild of the whole log: re-folding on every write makes the cost of a tick grow with the age + * of the household (measured at 0.4 ms per write at 200 entries and 4.8 ms at 3,000). + */ +export function foldInto(view: Map, entries: readonly Entry[]): Map { for (const e of entries) { if (e.op === "retract") view.delete(e.ref); else view.set(e.ref, { ...structuredClone(e.value), collection: e.collection, seq: e.seq } as Row); diff --git a/packages/lifeboard/src/store.ts b/packages/lifeboard/src/store.ts index 7a67fb7..f2a4de5 100644 --- a/packages/lifeboard/src/store.ts +++ b/packages/lifeboard/src/store.ts @@ -2,11 +2,24 @@ // and handed to subscribers. The whole log lives in memory too — at family scale that is thousands // of entries, not millions, and keeping it there is what makes history and re-derivation cheap. -import { entriesFor, materialise, type Row } from "./log.js"; +import { entriesFor, foldInto, materialise, type Row } from "./log.js"; import type { By, CollectionName, Entry, Mutation, Value } from "./types.js"; import { refOf } from "./types.js"; const STORE = "entries"; + +/** + * Where the log physically lives. The one operation that matters is `commit`: read whatever landed + * after `after`, decide what to append given that, and append it — atomically. Splitting those into + * separate reads and writes is what let a second tab mint a colliding sequence number. + */ +export interface Backend { + load(): Promise; + commit(after: number, make: (behind: readonly Entry[]) => Entry[]): Promise<{ behind: Entry[]; fresh: Entry[] }>; + close(): void; + /** False for the in-memory fallback, so the UI can say that nothing is being saved. */ + readonly durable: boolean; +} export type Listener = (changed: Set) => void; export interface ListFilter { @@ -27,18 +40,35 @@ export class Store { private queue: Promise = Promise.resolve(); private constructor( - private db: IDBDatabase, + private backend: Backend, private newId: () => string, ) {} + /** False when the log is being kept in memory only — the app must say so rather than imply a save. */ + get durable(): boolean { + return this.backend.durable; + } + static async open(name = "lifeboard", newId: () => string = defaultId): Promise { - const db = await request(indexedDB.open(name, 1), (r) => { - const d = r.result as IDBDatabase; - if (!d.objectStoreNames.contains(STORE)) d.createObjectStore(STORE, { keyPath: "seq" }); - }); - const s = new Store(db, newId); - s.entries = await request(db.transaction(STORE, "readonly").objectStore(STORE).getAll()); - s.entries.sort((a, b) => a.seq - b.seq); + return Store.on(await indexedDbBackend(name), newId); + } + + /** + * Open the durable log, or fall back to memory when this device will not store anything — private + * windows, storage disabled, a full disk. A blank screen is a worse answer than a working app that + * admits it is forgetting; the caller checks `durable` and says so. + */ + static async openOrMemory(name = "lifeboard", newId: () => string = defaultId): Promise { + try { + return await Store.open(name, newId); + } catch { + return Store.on(memoryBackend(), newId); + } + } + + static async on(backend: Backend, newId: () => string = defaultId): Promise { + const s = new Store(backend, newId); + s.entries = (await backend.load()).sort((a, b) => a.seq - b.seq); s.view = materialise(s.entries); return s; } @@ -79,25 +109,38 @@ export class Store { } private async write(mutations: readonly Mutation[], by: By): Promise { - const fresh = entriesFor(mutations, { - by, - now: new Date(), - newId: this.newId, - fromSeq: this.entries.length === 0 ? 0 : this.entries[this.entries.length - 1].seq, - existing: this.view, + // One atomic step: read what another tab may have written, then decide what to append given + // that. Deriving the next sequence number from this instance's memory instead is what made a + // second tab collide on the key and lose its write silently. + const { behind, fresh } = await this.backend.commit(this.tail(), (newer) => { + if (newer.length > 0) this.absorb(newer); + return entriesFor(mutations, { + by, + now: new Date(), + newId: this.newId, + fromSeq: this.tail(), + existing: this.view, + }); }); - if (fresh.length === 0) return []; - const tx = this.db.transaction(STORE, "readwrite"); - const os = tx.objectStore(STORE); - for (const e of fresh) os.add(e); - await done(tx); + if (fresh.length > 0) this.absorb(fresh); + this.announce(new Set([...behind, ...fresh].map((e) => e.collection))); + return fresh; + } + + private tail(): number { + return this.entries.length === 0 ? 0 : this.entries[this.entries.length - 1].seq; + } + + /** Append entries to the log and fold them into the view — one operation each, not a rebuild. */ + private absorb(entries: readonly Entry[]): void { + this.entries.push(...entries); + foldInto(this.view, entries); + } - this.entries.push(...fresh); - this.view = materialise(this.entries); - const changed = new Set(fresh.map((e) => e.collection)); + private announce(changed: Set): void { + if (changed.size === 0) return; for (const fn of this.listeners) fn(changed); - return fresh; } /** Resolves once every write queued so far has landed. Tests await this instead of guessing at a @@ -113,11 +156,59 @@ export class Store { close(): void { this.listeners.clear(); - this.db.close(); + this.backend.close(); } } export const openStore = Store.open; +export const openStoreOrMemory = Store.openOrMemory; + +/** IndexedDB, the durable home. */ +export async function indexedDbBackend(name: string): Promise { + const db = await request(indexedDB.open(name, 1), (r) => { + const d = r.result as IDBDatabase; + if (!d.objectStoreNames.contains(STORE)) d.createObjectStore(STORE, { keyPath: "seq" }); + }); + // An old tab holding the database open would otherwise block a future upgrade forever, and the + // open promise would neither resolve nor reject — hanging at a blank screen with no error is + // worse than any failure it could report. + db.onversionchange = () => db.close(); + + return { + durable: true, + load: () => request(db.transaction(STORE, "readonly").objectStore(STORE).getAll()), + async commit(after, make) { + const tx = db.transaction(STORE, "readwrite"); + const os = tx.objectStore(STORE); + const behind = await request(os.getAll(IDBKeyRange.lowerBound(after, true))); + const fresh = make(behind); + if (fresh.length === 0) { + tx.abort(); + return { behind, fresh }; + } + for (const e of fresh) os.add(e); + await done(tx); + return { behind, fresh }; + }, + close: () => db.close(), + }; +} + +/** The fallback when a device will not store anything. Forgets on reload, and says so. */ +export function memoryBackend(seed: Entry[] = []): Backend { + const entries = [...seed]; + return { + durable: false, + load: async () => [...entries], + async commit(after, make) { + const behind = entries.filter((e) => e.seq > after); + const fresh = make(behind); + entries.push(...fresh); + return { behind, fresh }; + }, + close: () => {}, + }; +} export type { Row }; const defaultId = (): string => crypto.randomUUID(); @@ -126,7 +217,11 @@ function request(req: IDBRequest, onUpgrade?: (r: IDBRequest) => void): Promi return new Promise((resolve, reject) => { if (onUpgrade) (req as IDBOpenDBRequest).onupgradeneeded = () => onUpgrade(req); req.onsuccess = () => resolve(req.result as T); - req.onerror = () => reject(req.error); + // `req.error` is null on several failure paths; rejecting with null surfaces as "undefined" and + // says nothing about what went wrong. + req.onerror = () => reject(req.error ?? new Error("IndexedDB request failed")); + (req as IDBOpenDBRequest).onblocked = () => + reject(new Error("Another tab is holding this device's data open. Close it and reload.")); }); } diff --git a/packages/lifeboard/src/sw.js b/packages/lifeboard/src/sw.js index e4bfcbe..5f4148d 100644 --- a/packages/lifeboard/src/sw.js +++ b/packages/lifeboard/src/sw.js @@ -6,8 +6,11 @@ // - the renderer's lazily-imported chunks are cached the first time they are fetched, because which // ones are needed depends on what the user looks at and their names are content-hashed. // -// Cache-first for both. This bundle is versioned by CACHE name: bumping it on deploy is what -// evicts the old one, so a stale chunk can never outlive the app.js that asked for it. +// The two are cached differently, and the difference matters. Content-hashed chunks are immutable, +// so cache-first is right and free. The SHELL is not hashed -- app.js is always called app.js -- so +// cache-first there would serve the old build forever, and a deploy would only reach anyone who +// happened to bump the cache name by hand. The shell is network-first with a cache fallback +// instead: online, you get the current build; offline, you get the last one that worked. const CACHE = "lifeboard-v1"; const SHELL = ["./", "./index.html", "./style.css", "./app.js"]; @@ -25,26 +28,38 @@ self.addEventListener("activate", (e) => { ); }); +const isShell = (url) => { + const path = new URL(url).pathname; + return path === "/" || /\/(index\.html|app\.js|style\.css)$/.test(path); +}; + +const remember = (req, res) => { + if (res.ok) { + const copy = res.clone(); + void caches.open(CACHE).then((c) => c.put(req, copy)); + } + return res; +}; + self.addEventListener("fetch", (e) => { const req = e.request; if (req.method !== "GET" || new URL(req.url).origin !== self.location.origin) return; + if (req.mode === "navigate" || isShell(req.url)) { + // Network-first: a deploy reaches people. The cached copy is what makes it work on a plane. + e.respondWith( + fetch(req) + .then((res) => remember(req, res)) + .catch(() => caches.match(req).then((hit) => hit ?? caches.match("./index.html"))), + ); + return; + } + + // Content-hashed chunks never change under a given name, so the cache is always right. e.respondWith( caches.match(req).then((hit) => { if (hit) return hit; - return fetch(req) - .then((res) => { - if (res.ok) { - const copy = res.clone(); - void caches.open(CACHE).then((c) => c.put(req, copy)); - } - return res; - }) - .catch(() => { - // A navigation with nothing cached for this exact URL still has a shell to fall back to. - if (req.mode === "navigate") return caches.match("./index.html"); - throw new Error("offline and not cached"); - }); + return fetch(req).then((res) => remember(req, res)); }), ); }); diff --git a/packages/lifeboard/src/transcript.ts b/packages/lifeboard/src/transcript.ts index 3859b48..fca64c3 100644 --- a/packages/lifeboard/src/transcript.ts +++ b/packages/lifeboard/src/transcript.ts @@ -25,7 +25,7 @@ export interface MessageRec { /** Persist one line. Attributed to `code`: the transcript records what happened, it is not itself * something a person or a model asserted. */ export function persistLine(store: Store, space: string, line: Line): void { - void store.apply( + swallow(store.apply( [add("message", { id: line.id, space, @@ -34,16 +34,29 @@ export function persistLine(store: Store, space: string, line: Line): void { ...(line.undoSeqs ? { undoSeqs: line.undoSeqs } : {}), })], { kind: "code", fn: "console" }, - ); + )); } /** Update a line already in the log — a pending line that has become the answer. */ export function updateLine(store: Store, line: Line): void { - void store.apply([update("message", line.id, { kind: line.kind, text: line.text })], { kind: "code", fn: "console" }); + swallow(store.apply([update("message", line.id, { kind: line.kind, text: line.text })], { kind: "code", fn: "console" })); } export function markUndone(store: Store, line: Line): void { - void store.apply([update("message", line.id, { undone: true })], { kind: "code", fn: "console" }); + swallow(store.apply([update("message", line.id, { undone: true })], { kind: "code", fn: "console" })); +} + +/** + * The transcript is written without anyone awaiting it — the console must not block on a disk write. + * That makes a failure (a full disk, a device that will not store anything) an unhandled rejection, + * which the browser reports as an uncaught error and nobody acts on. Losing a line of narration is + * survivable; losing it silently while the page logs an error is not. + */ +function swallow(p: Promise): void { + void p.catch((e: unknown) => { + // eslint-disable-next-line no-console + console.warn("lifeboard: a transcript line was not saved —", e instanceof Error ? e.message : e); + }); } /** Rebuild a profile's transcript from the log, undo buttons and all. */ diff --git a/packages/lifeboard/test/store.test.ts b/packages/lifeboard/test/store.test.ts index 7323890..94378ec 100644 --- a/packages/lifeboard/test/store.test.ts +++ b/packages/lifeboard/test/store.test.ts @@ -1,6 +1,6 @@ import "fake-indexeddb/auto"; import { describe, it, expect } from "vitest"; -import { openStore } from "../src/store.js"; +import { memoryBackend, openStore, openStoreOrMemory, Store } from "../src/store.js"; import { add, update, del, type By, type Item } from "../src/types.js"; const you: By = { kind: "human", profile: "ivan" }; @@ -124,3 +124,92 @@ describe("concurrent writes", () => { expect(s.list("item")).toHaveLength(1); }); }); + +describe("a second tab on the same device", () => { + it("does not lose a write to a sequence-number collision", async () => { + const name = `lb-tabs-${++n}`; + const tabA = await openStore(name); + const tabB = await openStore(name); + + await tabA.apply([add("item", { space: "shared", text: "from A" })], you); + await tabB.apply([add("item", { space: "shared", text: "from B" })], you); + + const reopened = await openStore(name); + expect(reopened.list("item").map((i) => i.text).sort()).toEqual(["from A", "from B"]); + expect(reopened.history().map((e) => e.seq)).toEqual([1, 2]); + }); + + it("picks up what the other tab wrote before computing its own change", async () => { + const name = `lb-tabs-${++n}`; + const tabA = await openStore(name); + const tabB = await openStore(name); + await tabA.apply([add("item", { id: "milk", space: "shared", text: "Milk", done: false })], you); + + // Tab B has never seen "milk", so without absorbing first this update would be dropped as a + // mutation against a record that does not exist. + await tabB.apply([update("item", "milk", { done: true })], you); + expect(tabB.get("item", "milk")).toMatchObject({ done: true }); + + const reopened = await openStore(name); + expect(reopened.get("item", "milk")).toMatchObject({ done: true }); + }); + + it("tells its own subscribers about the other tab's writes", async () => { + const name = `lb-tabs-${++n}`; + const tabA = await openStore(name); + const tabB = await openStore(name); + const seen: string[][] = []; + tabB.subscribe((c) => seen.push([...c].sort())); + + await tabA.apply([add("event", { space: "shared", title: "Swimming" })], you); + // A no-op of tab B's own is enough to notice: it reads the log before deciding it has nothing. + await tabB.apply([update("item", "ghost", { done: true })], you); + expect(seen).toEqual([["event"]]); + expect(tabB.list("event")).toHaveLength(1); + }); +}); + +describe("when the device will not store anything", () => { + it("reports a real error rather than rejecting with undefined", async () => { + const saved = globalThis.indexedDB; + (globalThis as { indexedDB?: unknown }).indexedDB = { + open() { + const req: Record = {}; + setTimeout(() => (req.onerror as (() => void) | undefined)?.(), 0); + return req; + }, + }; + await expect(openStore("nope")).rejects.toThrow(/IndexedDB request failed/); + (globalThis as { indexedDB?: unknown }).indexedDB = saved; + }); +}); + +describe("when the device will not store anything", () => { + it("falls back to memory rather than failing to start", async () => { + const saved = globalThis.indexedDB; + (globalThis as { indexedDB?: unknown }).indexedDB = { + open() { + const req: Record = {}; + setTimeout(() => (req.onerror as (() => void) | undefined)?.(), 0); + return req; + }, + }; + const s = await openStoreOrMemory("nope"); + expect(s.durable).toBe(false); + await s.apply([add("item", { space: "shared", text: "Milk" })], you); + expect(s.list("item")).toHaveLength(1); + (globalThis as { indexedDB?: unknown }).indexedDB = saved; + }); + + it("reports itself as durable when storage does work", async () => { + expect((await fresh()).durable).toBe(true); + }); + + it("the memory backend behaves like the real one, minus the durability", async () => { + const s = await Store.on(memoryBackend()); + await s.apply([add("item", { id: "a", space: "shared", text: "A", done: false })], you); + await s.apply([update("item", "a", { done: true })], you); + expect(s.get("item", "a")).toMatchObject({ done: true }); + expect(s.history("item/a").map((e) => e.op)).toEqual(["assert", "correct"]); + }); +});