From 5eb7c881dd9a30253ef792a381f8d9951c7ba477 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Mon, 3 Aug 2026 04:49:52 +0300 Subject: [PATCH] fix(db): take the sessions.source rebuild off the open path Widening the sessions.source CHECK is a whole-table rebuild, and its preflight takes a full VACUUM INTO copy of the database before it starts. The cost is proportional to the whole store, not to the change, and it ran unconditionally from initSchema() on every store open. On the 9.16 GB station01 store that never finished inside a command timeout. Every invocation -- including read-only ones like `sessions list --limit 1` -- began the rebuild, was killed, rolled back, and left another multi-GB partial backup behind. Ten days of that accumulated ~25 GB of abandoned backups in migration-backups/ while the constraint was never actually widened, so the next invocation started over. Reads paid a multi-GB write and returned nothing. The rebuild is now opt-in via HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT=1, matching HASNA_SESSIONS_REBUILD_FTS_ON_OPEN, which already gates the other whole-table repair in this file for the same reason. Opening a store is never allowed to cost the size of the store. This only affects stores created before 'codewith' existed: SCHEMA already creates sessions with the wide constraint, so new databases need no migration. A legacy store keeps working for reads and for its existing sources, and rejects codewith rows at the CHECK until an operator migrates deliberately. Measured on the live 9.16 GB store: `sessions list --limit 1` returns in 1168 ms with rc=0 and writes no backup, against a previous rc=124 timeout kill at the 90 s budget with no output. Two db.test.ts cases asserted the old implicit-on-open contract and now set the opt-in explicitly; every assertion they made about migration correctness is preserved. The unknown-source case additionally now proves that opening such a store no longer throws, so one bad row can no longer make every command fail. Agent: Augustus --- src/db/database.source-migration.test.ts | 170 +++++++++++++++++++++++ src/db/database.ts | 25 ++++ test/db.test.ts | 20 ++- 3 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 src/db/database.source-migration.test.ts diff --git a/src/db/database.source-migration.test.ts b/src/db/database.source-migration.test.ts new file mode 100644 index 0000000..1a4b122 --- /dev/null +++ b/src/db/database.source-migration.test.ts @@ -0,0 +1,170 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { initSchema } from "./database.js"; +import { SqliteAdapter } from "./sqlite-adapter.js"; + +/** + * The exact `sessions` DDL carried by stores created before 'codewith' was a + * source — copied verbatim from the live station01 store (9.16 GB) on + * 2026-08-03, whose CHECK reads `source IN ('claude', 'codex', 'gemini')`. + * + * Opening such a store used to rebuild the whole table on the open path, after + * first taking a full `VACUUM INTO` copy of it. On a multi-GB store that never + * finished inside any CLI timeout, so every invocation — including read-only + * ones like `sessions list --limit 1` — restarted the rebuild from scratch and + * left another multi-GB backup behind. + */ +const LEGACY_NARROW_SESSIONS_DDL = `CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + source TEXT NOT NULL CHECK(source IN ('claude', 'codex', 'gemini')), + source_id TEXT NOT NULL, + source_path TEXT, + title TEXT, + project_path TEXT, + project_name TEXT, + model TEXT, + model_provider TEXT, + git_branch TEXT, + git_sha TEXT, + git_origin_url TEXT, + cli_version TEXT, + is_subagent INTEGER NOT NULL DEFAULT 0, + parent_session_id TEXT, + total_input_tokens INTEGER NOT NULL DEFAULT 0, + total_output_tokens INTEGER NOT NULL DEFAULT 0, + total_cache_read_tokens INTEGER NOT NULL DEFAULT 0, + total_cache_write_tokens INTEGER NOT NULL DEFAULT 0, + total_thinking_tokens INTEGER NOT NULL DEFAULT 0, + message_count INTEGER NOT NULL DEFAULT 0, + tool_call_count INTEGER NOT NULL DEFAULT 0, + started_at TEXT, + ended_at TEXT, + duration_seconds REAL, + ingested_at TEXT NOT NULL DEFAULT (datetime('now')), + updated_at TEXT NOT NULL DEFAULT (datetime('now')), + source_modified_at TEXT, + machine TEXT, + metadata TEXT DEFAULT '{}', + UNIQUE(source, source_id) + )`; + +const MIGRATION_BACKUP_DIR = "migration-backups"; +const OPT_IN_ENV = "HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT"; + +let dir: string; +let dbPath: string; +let db: SqliteAdapter | null = null; + +/** Build a scratch store carrying the legacy narrow CHECK, with a row in it. */ +function openLegacyStore(): SqliteAdapter { + const adapter = new SqliteAdapter(dbPath); + adapter.exec(LEGACY_NARROW_SESSIONS_DDL); + adapter.exec( + `INSERT INTO sessions (id, source, source_id, started_at) + VALUES ('legacy-1', 'claude', 'src-1', '2026-07-01T00:00:00Z')`, + ); + return adapter; +} + +function sessionsDdl(adapter: SqliteAdapter): string { + const row = adapter + .prepare("SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'sessions'") + .get() as { sql?: string | null } | undefined; + return row?.sql ?? ""; +} + +function backupFiles(): string[] { + const backupDir = join(dir, MIGRATION_BACKUP_DIR); + if (!existsSync(backupDir)) return []; + return readdirSync(backupDir); +} + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "sessions-source-migration-")); + dbPath = join(dir, "sessions.db"); + delete process.env[OPT_IN_ENV]; +}); + +afterEach(() => { + db?.close(); + db = null; + delete process.env[OPT_IN_ENV]; + rmSync(dir, { recursive: true, force: true }); +}); + +describe("sessions.source constraint migration is off the open path", () => { + test("opening a legacy narrow-constraint store writes NO migration backup", () => { + db = openLegacyStore(); + + // Control: the fixture really does carry the narrow constraint, so this + // test is exercising the case the defect lives in. + expect(sessionsDdl(db)).toContain("'claude', 'codex', 'gemini'"); + expect(sessionsDdl(db)).not.toContain("codewith"); + expect(backupFiles()).toEqual([]); + + initSchema(db); + + // The defect: initSchema ran a full VACUUM INTO copy of the store purely + // because the CHECK was narrow. On the live 9 GB store that never finished. + expect(backupFiles()).toEqual([]); + }); + + test("opening a legacy narrow-constraint store leaves the table in place", () => { + db = openLegacyStore(); + initSchema(db); + + // Reads must still work, and the row must be untouched. + const row = db.prepare("SELECT id, source FROM sessions WHERE id = 'legacy-1'").get() as + | { id: string; source: string } + | undefined; + expect(row).toEqual({ id: "legacy-1", source: "claude" }); + + // No half-built replacement table left behind, and the original survives. + const tables = db + .prepare( + "SELECT COUNT(*) AS c FROM sqlite_master WHERE type = 'table' AND name IN ('sessions', 'sessions_new')", + ) + .get() as { c: number }; + expect(Number(tables.c)).toBe(1); + }); + + test("the migration still runs, and widens the constraint, when opted in", () => { + db = openLegacyStore(); + process.env[OPT_IN_ENV] = "1"; + + initSchema(db); + + // The feature is deferred, not deleted: opting in must actually migrate. + expect(sessionsDdl(db)).toContain("codewith"); + const row = db.prepare("SELECT id, source FROM sessions WHERE id = 'legacy-1'").get() as + | { id: string; source: string } + | undefined; + expect(row).toEqual({ id: "legacy-1", source: "claude" }); + + // And a codewith row becomes insertable, which is the point of the migration. + db.exec( + `INSERT INTO sessions (id, source, source_id, started_at) + VALUES ('cw-1', 'codewith', 'src-cw', '2026-08-01T00:00:00Z')`, + ); + const cw = db.prepare("SELECT source FROM sessions WHERE id = 'cw-1'").get() as + | { source: string } + | undefined; + expect(cw).toEqual({ source: "codewith" }); + }); + + test("an already-migrated store is untouched whether or not the opt-in is set", () => { + db = openLegacyStore(); + process.env[OPT_IN_ENV] = "1"; + initSchema(db); + expect(sessionsDdl(db)).toContain("codewith"); + const backupsAfterMigration = backupFiles().length; + + // Re-opening must be a no-op — no second rebuild, no second backup. + delete process.env[OPT_IN_ENV]; + initSchema(db); + expect(sessionsDdl(db)).toContain("codewith"); + expect(backupFiles().length).toBe(backupsAfterMigration); + }); +}); diff --git a/src/db/database.ts b/src/db/database.ts index 1fe4336..85a1e84 100644 --- a/src/db/database.ts +++ b/src/db/database.ts @@ -431,8 +431,33 @@ function createSessionsReplacementTable(db: SqliteAdapter): void { ); } +/** + * Widening the `sessions.source` CHECK is a whole-table rebuild: SQLite cannot + * alter a CHECK in place, so every row is copied into a replacement table — + * and preflight first takes a full `VACUUM INTO` copy of the database on top of + * that. The cost is proportional to the whole store, not to the change. + * + * That is affordable on a small store and ruinous on a large one. On the 9.16 GB + * station01 store it never finished inside a command timeout, so EVERY + * invocation — including read-only ones like `sessions list --limit 1` — began + * the rebuild, was killed, rolled back, and left another multi-GB partial + * backup behind. Ten days of that accumulated ~25 GB of abandoned backups and + * the constraint was never actually widened, so the next invocation started + * over. Reads paid a multi-GB write and returned nothing. + * + * So the rebuild is opt-in, matching HASNA_SESSIONS_REBUILD_FTS_ON_OPEN, which + * gates the other whole-table repair in this file for exactly this reason. + * Opening a store is never allowed to cost the size of the store. + * + * This only ever affects stores created before 'codewith' existed: SCHEMA + * already creates `sessions` with the wide constraint, so new databases need no + * migration. A legacy store keeps working for reads and for its existing + * sources; it rejects `codewith` rows at the CHECK until an operator runs the + * migration deliberately with HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT=1. + */ function migrateSessionSourceConstraint(db: SqliteAdapter): void { if (sourceCheckAllowsCodewith(tableSql(db, "sessions"))) return; + if (process.env.HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT !== "1") return; preflightSourceConstraintMigration(db); const before = countTables(db); diff --git a/test/db.test.ts b/test/db.test.ts index f0a37b3..eda82c2 100644 --- a/test/db.test.ts +++ b/test/db.test.ts @@ -171,6 +171,9 @@ describe("schema migration", () => { db.exec("INSERT INTO tool_calls_fts_refs(rowid, session_id, tool_call_id) VALUES (1, 's1', 't1')"); db.exec("INSERT INTO tool_calls_fts(rowid, tool_call_id, session_id, tool_name, tool_input, tool_output) VALUES (1, 't1', 's1', 'shell', 'echo ok', 'ok')"); + // The rebuild is opt-in: it copies the whole table and takes a full + // VACUUM INTO backup first, so it is never run implicitly on open. + process.env.HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT = "1"; initSchema(db); const table = db.prepare("SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'sessions'").get() as { sql: string }; @@ -194,6 +197,7 @@ describe("schema migration", () => { ).toThrow(); expect(readdirSync(join(dir, "migration-backups")).some((name) => name.startsWith("sessions-pre-codewith-source-"))).toBe(true); } finally { + delete process.env.HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT; db.close(); rmSync(dir, { recursive: true, force: true }); } @@ -216,7 +220,21 @@ describe("schema migration", () => { ); db.exec("INSERT INTO sessions (id, source, source_id, title) VALUES ('legacy', 'unknown', 'bad', 'still readable')"); - expect(() => initSchema(db)).toThrow(/unknown sources/); + // Opening such a store must NOT throw: the migration is opt-in, so an + // unrecognised source in the data can no longer make every command fail. + initSchema(db); + expect((db.prepare("SELECT title FROM sessions WHERE id = 'legacy'").get() as { title: string }).title).toBe( + "still readable", + ); + + // Opting in still refuses to migrate rather than dropping the unknown rows, + // and still leaves the legacy database readable afterwards. + try { + process.env.HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT = "1"; + expect(() => initSchema(db)).toThrow(/unknown sources/); + } finally { + delete process.env.HASNA_SESSIONS_MIGRATE_SOURCE_CONSTRAINT; + } const row = db.prepare("SELECT title FROM sessions WHERE id = 'legacy'").get() as { title: string }; expect(row.title).toBe("still readable"); db.close();