diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 71b63ab..735c77e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,8 +31,9 @@ jobs: - run: pnpm test - # Catches env-parse regressions and enforces the 250KB bundle budget. - # Values are throwaway: nothing here connects to a real service. + # Catches env-parse regressions. Values are throwaway: nothing here + # connects to a real service. The build output is captured so the next + # step can enforce the bundle budget against Next's own figures. - name: Build web working-directory: apps/web env: @@ -42,4 +43,14 @@ jobs: CREDENTIALS_ENCRYPTION_KEY: Y2ktcGxhY2Vob2xkZXItbm90LWEtcmVhbC1zZWNyZXQ= NEXT_PUBLIC_WS_URL: ws://localhost:3001 NEXT_PUBLIC_APP_URL: http://localhost:3000 - run: pnpm build + # `shell: bash` is load-bearing: it runs with `-eo pipefail`, so a failed + # build still fails the step. The default shell would report tee's exit + # code and let a broken build through. + shell: bash + run: pnpm build | tee build.log + + # The 250KB gzipped initial-JS budget (CLAUDE.md §1). Previously the + # comment above claimed the build enforced this; nothing did. + - name: Check bundle budget + working-directory: apps/web + run: node scripts/check-bundle-budget.mjs build.log diff --git a/.gitignore b/.gitignore index a588693..23c1fbb 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,6 @@ blob-report/ .DS_Store *.log .gstack/ + +# Captured build output for the CI bundle-budget gate. +build.log diff --git a/README.md b/README.md index 3273200..20f00d9 100644 --- a/README.md +++ b/README.md @@ -211,11 +211,12 @@ The Playwright suite needs the stack running (`docker compose up -d` and `pnpm d Being honest about what is not done: -- **No rate limiting.** `POST /api/boards` will mint a new guest identity and board on every unauthenticated request. This needs a Redis-backed limiter before any public deployment. -- **The e2e suite has never been executed.** It is written and typechecks, but has not been run against live services. -- **Never deployed.** No production environment has been stood up; the deploy targets in the stack table are intended, not proven. -- **Bundle headroom is thin.** The board route is 245KB first-load JS against a 250KB budget. Run `pnpm analyze` before adding client-side dependencies. -- **The Yjs update log grows unbounded.** Snapshots are written, but the append-only update log has no compaction yet. +- **Never deployed.** No production environment has been stood up; the deploy targets in the stack table are intended, not proven. [`DEPLOYING.md`](DEPLOYING.md) is a runbook that has never been executed end to end, and nothing in it is covered by a test. +- **Bundle headroom is thin.** The board route is 245KB first-load JS against the 250KB gzipped budget — 5KB, or 2%. CI now fails if any route crosses it, but the next client-side dependency is very likely to. Run `pnpm analyze` first. +- **`ws` is a single point of failure.** One Fly machine by design: board state lives in that process's memory, and sharing it across instances needs `@hocuspocus/extension-redis`, which is not installed. A deploy or crash disconnects every board. Clients reconnect and Yjs re-syncs, but in-flight awareness is lost. +- **Rate limits share one bucket without a proxy header.** `clientIpFromRequest` falls back to a literal `"unknown"`, so every caller that arrives without `x-forwarded-for` is counted together. Vercel always sets it, so the deployed web path is fine — but the failure mode is a global lockout rather than blocking one abuser. +- **No Y.Doc schema migration path.** Live documents cannot be recreated from Postgres, and there is no mechanism for evolving their shape. This is the constraint most likely to hurt later. +- **`@quorum/agent-protocol` is not published.** It is dependency-free and meant to be vendored, but there is no npm package and no version field on the wire, so an agent cannot detect contract drift. ## Contributing diff --git a/apps/web/scripts/check-bundle-budget.mjs b/apps/web/scripts/check-bundle-budget.mjs new file mode 100644 index 0000000..b31f172 --- /dev/null +++ b/apps/web/scripts/check-bundle-budget.mjs @@ -0,0 +1,80 @@ +#!/usr/bin/env node +/** + * Enforce the initial-JS budget from CLAUDE.md §1: <250KB gzipped. + * + * Reads the route table Next prints at the end of `next build` and fails if any + * route's First Load JS exceeds the budget. Next's reported figures are + * gzipped (verified: a chunk reported as 54.2 kB measures 53.0 KB under gzip + * and 169 KB raw), so they compare directly against the budget with no + * conversion. + * + * Parsing Next's own output rather than recomputing from .next/ is deliberate: + * the number the team reads in CI is then exactly the number being enforced, + * and there is no second implementation of "which chunks count" to drift. + * + * Usage: next build | tee build.log && node scripts/check-bundle-budget.mjs build.log + */ +import { readFileSync } from "node:fs"; + +const BUDGET_KB = 250; + +const logPath = process.argv[2]; +if (!logPath) { + console.error("usage: check-bundle-budget.mjs "); + process.exit(2); +} + +const text = readFileSync(logPath, "utf8"); + +/** "245 kB" / "1.2 MB" / "149 B" → KB. */ +function toKb(value, unit) { + const n = Number(value); + if (unit === "B") return n / 1024; + if (unit === "kB") return n; + if (unit === "MB") return n * 1024; + return Number.NaN; +} + +// Route rows look like: ├ ƒ /b/[boardId] 107 kB 245 kB +// The LAST size on the row is First Load JS; the first is the route's own JS. +const ROUTE_ROW = /^[┌├└]\s+[^\s]*\s*(\/\S*)\s+.*?([\d.]+)\s*(B|kB|MB)\s*$/; + +const routes = []; +for (const line of text.split("\n")) { + const match = ROUTE_ROW.exec(line.trimEnd()); + if (match) routes.push({ route: match[1], firstLoadKb: toKb(match[2], match[3]) }); +} + +if (routes.length === 0) { + console.error( + "check-bundle-budget: found no route rows in the build output.\n" + + "Did the build succeed, and was its stdout captured to the file passed in?", + ); + process.exit(2); +} + +const over = routes.filter((r) => r.firstLoadKb > BUDGET_KB); +const worst = routes.reduce((a, b) => (b.firstLoadKb > a.firstLoadKb ? b : a)); + +for (const { route, firstLoadKb } of over) { + console.error(`OVER BUDGET ${route} ${firstLoadKb.toFixed(1)} kB > ${BUDGET_KB} kB gzipped`); +} + +if (over.length > 0) { + console.error( + `\n${over.length} route(s) exceed the ${BUDGET_KB}KB initial-JS budget (CLAUDE.md §1).\n` + + "Run `pnpm analyze` to see what landed in the bundle.", + ); + process.exit(1); +} + +const headroom = BUDGET_KB - worst.firstLoadKb; +console.log( + `bundle budget OK — ${routes.length} routes, worst is ${worst.route} at ` + + `${worst.firstLoadKb.toFixed(1)} kB gzipped (${headroom.toFixed(1)} kB headroom).`, +); +if (headroom < 15) { + console.log( + `NOTE: under 15 kB of headroom. Check \`pnpm analyze\` before adding a client dependency.`, + ); +} diff --git a/apps/ws/src/persistence.ts b/apps/ws/src/persistence.ts index 27c4038..3b69389 100644 --- a/apps/ws/src/persistence.ts +++ b/apps/ws/src/persistence.ts @@ -9,6 +9,13 @@ * conservative: Yjs updates are commutative and idempotent, so re-applying * ones already contained in the snapshot is a no-op, and updates appended * after the snapshot was cut are never lost. + * + * Storing a snapshot also COMPACTS: the updates that snapshot subsumes are + * deleted in the same transaction, so the log is bounded by what arrives + * between two debounced snapshots rather than growing for the life of the + * board. Compaction is deliberately conservative in the same direction as + * replay — it prunes only what a snapshot provably contains, so the failure + * mode is keeping a redundant row, never dropping a live one. */ import type { Extension, @@ -71,9 +78,25 @@ export class PostgresPersistence implements Extension { async onStoreDocument({ document, documentName }: onStoreDocumentPayload): Promise { try { + // Read the high-water mark BEFORE encoding. Hocuspocus applies an update + // to the document before onChange writes it, so every row at or below + // this id is already inside the state encoded next. Rows appended while + // we encode land above the mark and are kept — replaying an update the + // snapshot already contains is a no-op, losing one is not. + const compactThrough = await this.snapshots.latestUpdateId(documentName); const state = Y.encodeStateAsUpdate(document); - await this.snapshots.storeSnapshot(documentName, state); - this.log.debug("snapshot stored", { boardId: documentName, bytes: state.byteLength }); + const { prunedUpdates, prunedSnapshots } = await this.snapshots.storeSnapshot( + documentName, + state, + compactThrough, + ); + this.log.debug("snapshot stored", { + boardId: documentName, + bytes: state.byteLength, + compactThrough, + prunedUpdates, + prunedSnapshots, + }); } catch (error) { this.log.error("failed to store yjs snapshot", { boardId: documentName, diff --git a/apps/ws/src/stores.ts b/apps/ws/src/stores.ts index a88d5c1..06d515a 100644 --- a/apps/ws/src/stores.ts +++ b/apps/ws/src/stores.ts @@ -7,7 +7,7 @@ * SECURITY: the agents queries select display-safe columns ONLY. Credential * ciphertext, IVs, tags, and signing secrets are never read by this process. */ -import { and, eq, inArray, sql } from "drizzle-orm"; +import { and, eq, inArray, lte, sql } from "drizzle-orm"; import type { Db } from "@quorum/db/client"; import { @@ -28,6 +28,27 @@ import type { VoteValue } from "@quorum/shared/schemas/vote"; import { serializeError, type Logger } from "./log"; +/** Snapshots kept per board beyond the newest, as a manual recovery fallback. */ +const SNAPSHOT_RETENTION = 3; + +/** + * Rows affected by a DELETE, across driver result shapes. + * + * Order matters: postgres-js returns an Array SUBCLASS that carries the real + * figure on `.count`, and without a RETURNING clause its `.length` is 0. An + * `Array.isArray` check first therefore reports every delete as having removed + * nothing — which is exactly what the compaction log claimed before this. + */ +export function rowsAffected(result: unknown): number { + if (typeof result === "object" && result !== null) { + const record = result as { count?: unknown; rowCount?: unknown }; + if (typeof record.count === "number") return record.count; + if (typeof record.rowCount === "number") return record.rowCount; + } + if (Array.isArray(result)) return result.length; + return 0; +} + // --------------------------------------------------------------------------- // Interfaces // --------------------------------------------------------------------------- @@ -67,13 +88,35 @@ export interface BoardStore { removeAgent(agentId: string): Promise; } +/** What one compacting snapshot write removed. Zeroes when nothing was pruned. */ +export interface SnapshotWriteResult { + prunedUpdates: number; + prunedSnapshots: number; +} + export interface SnapshotStore { loadLatestSnapshot(boardId: string): Promise<{ seq: number; snapshot: Uint8Array } | null>; /** The full raw update log for the board, oldest first. */ listUpdates(boardId: string): Promise; appendUpdate(boardId: string, update: Uint8Array): Promise; - /** Store a snapshot at the next monotonic seq for the board. */ - storeSnapshot(boardId: string, snapshot: Uint8Array): Promise; + /** + * Highest update id currently stored for the board, or null when the log is + * empty. Read BEFORE encoding a snapshot: Hocuspocus applies an update to the + * document before onChange persists it, so a row at or below this id is + * guaranteed to be inside a snapshot encoded afterwards. Rows appended during + * the encode land above the mark and survive. + */ + latestUpdateId(boardId: string): Promise; + /** + * Store a snapshot at the next monotonic seq, and in the SAME transaction + * drop the updates it subsumes plus snapshots older than the retention + * window. Pass `compactThrough: null` to write without compacting. + */ + storeSnapshot( + boardId: string, + snapshot: Uint8Array, + compactThrough: number | null, + ): Promise; } export interface AuditVote { @@ -214,11 +257,50 @@ export function createStores(db: Db, log: Logger): Stores { await db.insert(yjsUpdates).values({ boardId, update: Buffer.from(update) }); }, - async storeSnapshot(boardId, snapshot) { - await db.insert(yjsSnapshots).values({ - boardId, - snapshot: Buffer.from(snapshot), - seq: sql`coalesce((select max(${yjsSnapshots.seq}) from ${yjsSnapshots} where ${yjsSnapshots.boardId} = ${boardId}), 0) + 1`, + async latestUpdateId(boardId) { + const row = await db.query.yjsUpdates.findFirst({ + columns: { id: true }, + where: (table, operators) => operators.eq(table.boardId, boardId), + orderBy: (table, operators) => operators.desc(table.id), + }); + return row?.id ?? null; + }, + + async storeSnapshot(boardId, snapshot, compactThrough) { + // One transaction: a snapshot must never be pruned against unless it is + // itself durable, or a crash between the two statements loses history. + return db.transaction(async (tx) => { + await tx.insert(yjsSnapshots).values({ + boardId, + snapshot: Buffer.from(snapshot), + seq: sql`coalesce((select max(${yjsSnapshots.seq}) from ${yjsSnapshots} where ${yjsSnapshots.boardId} = ${boardId}), 0) + 1`, + }); + // Nothing to prune when the log is empty, but snapshot retention still + // runs below — an idle board keeps writing snapshots on every store, + // and those are the larger rows. + const prunedUpdates = + compactThrough === null + ? 0 + : rowsAffected( + await tx + .delete(yjsUpdates) + .where(and(eq(yjsUpdates.boardId, boardId), lte(yjsUpdates.id, compactThrough))), + ); + // Keep a few older snapshots as a manual fallback if the newest one is + // ever unreadable; without a cap they are the bigger leak of the two, + // since each is the whole document rather than one delta. + const prunedSnapshots = await tx + .delete(yjsSnapshots) + .where( + and( + eq(yjsSnapshots.boardId, boardId), + lte( + yjsSnapshots.seq, + sql`coalesce((select max(${yjsSnapshots.seq}) from ${yjsSnapshots} where ${yjsSnapshots.boardId} = ${boardId}), 0) - ${SNAPSHOT_RETENTION}`, + ), + ), + ); + return { prunedUpdates, prunedSnapshots: rowsAffected(prunedSnapshots) }; }); }, }; diff --git a/apps/ws/test/persistence.test.ts b/apps/ws/test/persistence.test.ts new file mode 100644 index 0000000..f881592 --- /dev/null +++ b/apps/ws/test/persistence.test.ts @@ -0,0 +1,204 @@ +/** + * Yjs persistence: replay correctness and update-log compaction. + * + * Compaction is the risky half — deleting history that a snapshot does not + * actually contain loses board state permanently, and Yjs will not tell you. + * These tests pin the ordering rule that makes it safe. + */ +import { describe, expect, it, vi } from "vitest"; +import * as Y from "yjs"; + +import { PostgresPersistence } from "../src/persistence"; +import type { Logger } from "../src/log"; +import type { SnapshotStore, SnapshotWriteResult } from "../src/stores"; + +const BOARD = "brd_compaction_test"; + +function silentLogger(): Logger { + const logger: Logger = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + child: () => logger, + }; + return logger; +} + +interface UpdateRow { + id: number; + update: Uint8Array; +} + +/** + * In-memory SnapshotStore with the same semantics as the Postgres one: + * monotonic update ids, monotonic snapshot seqs, and a compacting write that + * deletes only rows at or below the mark it is given. + */ +function memoryStore() { + let nextUpdateId = 0; + let nextSeq = 0; + const updates: UpdateRow[] = []; + const snapshots: { seq: number; snapshot: Uint8Array }[] = []; + /** Runs between latestUpdateId() and storeSnapshot(), simulating concurrency. */ + let duringEncode: (() => void) | null = null; + + const store: SnapshotStore = { + async loadLatestSnapshot() { + const latest = snapshots.at(-1); + return latest === undefined ? null : { seq: latest.seq, snapshot: latest.snapshot }; + }, + async listUpdates() { + return updates.map((row) => row.update); + }, + async appendUpdate(_boardId, update) { + nextUpdateId += 1; + updates.push({ id: nextUpdateId, update }); + }, + async latestUpdateId() { + const mark = updates.at(-1)?.id ?? null; + duringEncode?.(); + duringEncode = null; + return mark; + }, + async storeSnapshot(_boardId, snapshot, compactThrough): Promise { + nextSeq += 1; + snapshots.push({ seq: nextSeq, snapshot }); + + let prunedUpdates = 0; + if (compactThrough !== null) { + const keep = updates.filter((row) => row.id > compactThrough); + prunedUpdates = updates.length - keep.length; + updates.splice(0, updates.length, ...keep); + } + + const cutoff = nextSeq - 3; + const keptSnapshots = snapshots.filter((row) => row.seq > cutoff); + const prunedSnapshots = snapshots.length - keptSnapshots.length; + snapshots.splice(0, snapshots.length, ...keptSnapshots); + + return { prunedUpdates, prunedSnapshots }; + }, + }; + + return { + store, + updates, + snapshots, + /** Schedule a write that lands after the mark is read but before the snapshot. */ + onEncode(fn: () => void) { + duringEncode = fn; + }, + }; +} + +/** A doc with `text` typed into it, plus the update that produced it. */ +function docWith(text: string): { doc: Y.Doc; update: Uint8Array } { + const doc = new Y.Doc(); + doc.getText("body").insert(0, text); + return { doc, update: Y.encodeStateAsUpdate(doc) }; +} + +describe("PostgresPersistence compaction", () => { + it("prunes the updates a snapshot already contains", async () => { + const mem = memoryStore(); + const persistence = new PostgresPersistence(mem.store, silentLogger()); + const { doc, update } = docWith("hello"); + + await persistence.onChange({ documentName: BOARD, update } as never); + expect(mem.updates).toHaveLength(1); + + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + + expect(mem.updates).toHaveLength(0); + expect(mem.snapshots).toHaveLength(1); + }); + + it("keeps an update that arrives after the mark is taken", async () => { + const mem = memoryStore(); + const persistence = new PostgresPersistence(mem.store, silentLogger()); + const { doc, update } = docWith("hello"); + await persistence.onChange({ documentName: BOARD, update } as never); + + // A collaborator types while the snapshot is being encoded. That row is + // NOT in the snapshot, so pruning it would lose their edit. + const late = docWith("late edit").update; + mem.onEncode(() => { + void mem.store.appendUpdate(BOARD, late); + }); + + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + + expect(mem.updates).toHaveLength(1); + expect(mem.updates[0]?.update).toBe(late); + }); + + it("survives a round trip: state after compaction equals state before", async () => { + const mem = memoryStore(); + const persistence = new PostgresPersistence(mem.store, silentLogger()); + + const doc = new Y.Doc(); + doc.getText("body").insert(0, "first"); + await persistence.onChange({ + documentName: BOARD, + update: Y.encodeStateAsUpdate(doc), + } as never); + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + + // An edit after the snapshot — this one lives only in the update log. + doc.getText("body").insert(5, " and second"); + await persistence.onChange({ + documentName: BOARD, + update: Y.encodeStateAsUpdate(doc), + } as never); + + const reloaded = await persistence.onLoadDocument({ + document: new Y.Doc(), + documentName: BOARD, + } as never); + + expect(reloaded.getText("body").toString()).toBe("first and second"); + }); + + it("keeps the newest snapshots and drops the rest", async () => { + const mem = memoryStore(); + const persistence = new PostgresPersistence(mem.store, silentLogger()); + const { doc } = docWith("x"); + + for (let i = 0; i < 8; i += 1) { + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + } + + expect(mem.snapshots).toHaveLength(3); + expect(mem.snapshots.at(-1)?.seq).toBe(8); + }); + + it("writes the snapshot but prunes nothing when the log is empty", async () => { + const mem = memoryStore(); + const persistence = new PostgresPersistence(mem.store, silentLogger()); + const { doc } = docWith("x"); + + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + + expect(mem.snapshots).toHaveLength(1); + expect(mem.updates).toHaveLength(0); + }); + + it("leaves the log intact when the snapshot write fails", async () => { + const mem = memoryStore(); + const failing: SnapshotStore = { + ...mem.store, + async storeSnapshot() { + throw new Error("postgres is down"); + }, + }; + const persistence = new PostgresPersistence(failing, silentLogger()); + const { doc, update } = docWith("hello"); + await persistence.onChange({ documentName: BOARD, update } as never); + + // Must not throw — a failed snapshot cannot take the board down. + await persistence.onStoreDocument({ document: doc, documentName: BOARD } as never); + + expect(mem.updates).toHaveLength(1); + }); +}); diff --git a/apps/ws/test/stores.regression-1.test.ts b/apps/ws/test/stores.regression-1.test.ts new file mode 100644 index 0000000..d95e219 --- /dev/null +++ b/apps/ws/test/stores.regression-1.test.ts @@ -0,0 +1,43 @@ +/** + * Regression: compaction reported "prunedUpdates: 0" on every write while + * actually deleting rows, because postgres-js returns an Array SUBCLASS whose + * `.length` is 0 without a RETURNING clause and whose real figure sits on + * `.count`. An Array.isArray branch that ran first swallowed it, so the log + * said nothing was ever pruned. + * Found by /devex-review follow-up on 2026-08-02 by reading live ws output + * against the actual row counts in Postgres. + */ +import { describe, expect, it } from "vitest"; + +import { rowsAffected } from "../src/stores"; + +/** postgres-js: an Array subclass carrying `count`, with no rows in it. */ +class PostgresJsResult extends Array { + count: number; + constructor(count: number) { + super(); + this.count = count; + } +} + +describe("rowsAffected", () => { + it("reads count off a postgres-js result even though it is an empty array", () => { + expect(rowsAffected(new PostgresJsResult(7))).toBe(7); + }); + + it("still reports zero when a postgres-js delete matched nothing", () => { + expect(rowsAffected(new PostgresJsResult(0))).toBe(0); + }); + + it("reads rowCount from a node-postgres style result", () => { + expect(rowsAffected({ rowCount: 4 })).toBe(4); + }); + + it("falls back to array length for drivers that return the deleted rows", () => { + expect(rowsAffected([{ id: 1 }, { id: 2 }])).toBe(2); + }); + + it.each([null, undefined, 0, "3", {}])("returns 0 for the unusable result %j", (value) => { + expect(rowsAffected(value)).toBe(0); + }); +}); diff --git a/eslint.config.mjs b/eslint.config.mjs index 8afd58b..3947950 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -17,6 +17,13 @@ export default tseslint.config( }, js.configs.recommended, ...tseslint.configs.recommended, + { + // Build/CI scripts run under plain Node, so they legitimately use the Node + // globals the browser/TS config knows nothing about. Declared inline rather + // than pulling in the `globals` package for two names. + files: ["**/scripts/**/*.mjs"], + languageOptions: { globals: { process: "readonly", console: "readonly" } }, + }, { rules: { "@typescript-eslint/no-explicit-any": "error",