trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns11g rEventsFlows

Preview boots fail on their own schema migration ("table already exists"): 11 of 12 failed deploys #580

Merged masoncrew opened this in answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/558-preview-boots-fail-on-their-own-schema-m → main 3 commits fork

  • Merged. mason/558-preview-boots-fail-on-their-own-schema-m was merged into main as 31c4791.

Review

Sign in to review this change.
8 files changed +503 −27
.gitignore +5
@@ -9,3 +9,8 @@ dist/
99
1010 # a TypeSafe API key, kept beside the forge, never in it
1111 typesafe-api-key
12
13 # databases live under FORGE_APP_DATA or data/, never in the tree (#558)
14 /*.db
15 /*.db-wal
16 /*.db-shm
CLAUDE.md +3 −1
@@ -219,7 +219,9 @@ flag exists only in the stdio dev runner and is never reachable over SSH.
219219 (`notFound`, `noRoute`, `unauthorized`, `forbidden`) or a specific 422 code;
220220 don't invent a new shape.
221221 - **Add a schema change:** append a migration to `core/schema.ts` (they run in
222 order, once). Never edit an existing migration — add a new one.
222 order, once each, recognised by a hash of their text in `schema_migration`
223 — see `kernel/migrate.ts`). Never edit an existing migration — an edited
224 one is a new one and runs again. Add a new one.
223225 - **New git read:** put it in `git/read.ts`; reuse the long-lived `cat-file`
224226 batch reader rather than spawning per object. Run every rev/path through
225227 `assertBatchSafe`.
docs/ARCHITECTURE.md +4 −2
@@ -146,8 +146,10 @@ curate, never implement.
146146
147147 ## Data & state model
148148
149 SQLite (`bun:sqlite`), WAL mode, foreign keys on, numbered migrations tracked
150 in `user_version`. The schema is deliberately small — tables earn their place:
149 SQLite (`bun:sqlite`), WAL mode, foreign keys on, migrations tracked by a
150 hash of their text in `schema_migration` (`kernel/migrate.ts`), so a preview
151 that keeps its data across a rebase never runs one twice; `user_version`
152 still counts them. The schema is deliberately small — tables earn their place:
151153
152154 - `user` (humans and bots; bots are just users with narrower tokens)
153155 - `session` (cookie auth for the web)
forge.db deleted
Binary file not shown.
scripts/gate.ts +1
@@ -56,6 +56,7 @@ export const GATE: readonly string[] = [
5656 "test/helpers.test.ts",
5757 "test/shards.test.ts",
5858 "test/migrate.test.ts",
59 "test/schema-migrations.test.ts",
5960 "test/vitals.test.ts",
6061 "test/guard.test.ts",
6162 "test/asks.test.ts",
src/kernel/db.ts +3 −24
@@ -1,11 +1,12 @@
11 /**
2 * SQLite, opened the one correct way, with linear numbered migrations.
2 * SQLite, opened the one correct way, with its migrations applied
3 * (`kernel/migrate.ts`).
34 * The database is an index over truth that lives in git — losing it should
45 * always be survivable, corrupting it silently should be impossible.
56 */
67
78 import { Database } from "bun:sqlite";
8 import { defect } from "./error.ts";
9 import { migrate } from "./migrate.ts";
910
1011 /**
1112 * How long a writer waits for another writer before giving up. Long by
@@ -119,25 +120,3 @@ export function isBusy(error: unknown): boolean {
119120 /SQLITE_BUSY|database is locked/i.test(String(error))
120121 );
121122 }
122
123 function migrate(db: Database, migrations: readonly string[]): void {
124 const row = db
125 .query<{ user_version: number }, []>("PRAGMA user_version")
126 .get();
127 const applied = row?.user_version ?? 0;
128 if (applied > migrations.length) {
129 defect(
130 "db.future_schema",
131 `database is at schema ${applied}, this build only knows ${migrations.length} — refusing to run`,
132 );
133 }
134 for (let i = applied; i < migrations.length; i++) {
135 const sql = migrations[i];
136 if (!sql) defect("db.migration_missing", `migration ${i + 1} is empty`);
137 db.transaction(() => {
138 db.exec(sql);
139 // PRAGMA can't be parameterized; the value is a loop index, not input.
140 db.exec(`PRAGMA user_version = ${i + 1}`);
141 })();
142 }
143 }
src/kernel/migrate.ts added +268
@@ -0,0 +1,268 @@
1 /**
2 * Which migrations a database has applied — by what each one says, not by
3 * where it sits in the list.
4 *
5 * A position is not an identity. Two pulls that each append a migration
6 * both call theirs N+1; whichever lands second is renumbered N+2 by its
7 * rebase. A preview keeps its data across redeploys, so a database that ran
8 * the branch's migration as N+1 then booted the rebased build, saw
9 * `user_version = N+1`, and ran the same CREATE again as N+2: "table already
10 * exists", and a preview that never came up (#558). So every applied
11 * migration is a row in `schema_migration`, keyed by a hash of its text, and
12 * a boot runs exactly the ones whose text it has not seen. A second boot is
13 * a no-op; a migration that moved is still recognised; a sibling's migration
14 * this build does not know is left alone.
15 *
16 * Each statement is also run so that meeting its own result is a no-op —
17 * `CREATE` gains `IF NOT EXISTS`, an `ADD COLUMN` whose column is there is
18 * skipped — because data written by the positional runner can hold a
19 * branch's table under the wrong number, and a migration recorded nowhere
20 * may still have left its mark.
21 *
22 * `user_version` is still written — the count of applied migrations — so a
23 * build from before this table refuses a database that is ahead of it the
24 * way it always did.
25 */
26
27 import type { Database } from "bun:sqlite";
28 import { defect } from "./error.ts";
29 import { sha256 } from "./hash.ts";
30
31 /** The longest slice of a failing statement a failure message quotes. */
32 const QUOTE_MAX = 80;
33
34 /**
35 * A migration's identity: its text with runs of whitespace collapsed, so
36 * re-indenting a released migration does not make it new.
37 */
38 export function migrationId(sql: string): string {
39 return sha256(sql.replace(/\s+/g, " ").trim());
40 }
41
42 export function migrate(db: Database, migrations: readonly string[]): void {
43 const ids = migrations.map((sql, i) => {
44 if (sql.trim() === "")
45 defect("db.migration_missing", `migration ${i + 1} is empty`);
46 return migrationId(sql);
47 });
48 const seen = new Set<string>();
49 ids.forEach((id, i) => {
50 if (seen.has(id))
51 defect(
52 "db.migration_duplicate",
53 `migration ${i + 1} repeats an earlier one word for word — it would never run`,
54 );
55 seen.add(id);
56 });
57
58 const applied = appliedIds(db, migrations, ids);
59 if (applied.size > migrations.length) {
60 defect(
61 "db.future_schema",
62 `database has applied ${applied.size} migrations, this build only knows ${migrations.length} — refusing to run`,
63 );
64 }
65 for (let i = 0; i < migrations.length; i++) {
66 const id = ids[i]!;
67 if (applied.has(id)) continue;
68 const sql = migrations[i]!;
69 db.transaction(() => {
70 for (const statement of statements(sql)) {
71 try {
72 if (!alreadyAdded(db, statement)) db.exec(idempotent(statement));
73 } catch (error) {
74 defect(
75 "db.migration_failed",
76 `migration ${i + 1} failed at ${quote(statement)}: ${messageOf(error)}`,
77 error,
78 );
79 }
80 }
81 db.query(
82 "INSERT INTO schema_migration (id, position, applied_at) VALUES (?1, ?2, ?3)",
83 ).run(id, i + 1, new Date().toISOString());
84 applied.add(id);
85 // PRAGMA can't be parameterized; the value is a count, not input.
86 db.exec(`PRAGMA user_version = ${applied.size}`);
87 })();
88 }
89 }
90
91 /**
92 * The ids this database has applied. A database from before the table
93 * existed has only `user_version`; its first `user_version` migrations are
94 * taken as applied — what that number always meant — unless one plainly
95 * did not run (a table it creates or a column it adds is missing: a
96 * branch's migration sat at that number instead). They are written down
97 * once, after which positions never matter again.
98 */
99 function appliedIds(
100 db: Database,
101 migrations: readonly string[],
102 ids: readonly string[],
103 ): Set<string> {
104 if (!hasTable(db, "schema_migration")) {
105 const version =
106 db.query<{ user_version: number }, []>("PRAGMA user_version").get()
107 ?.user_version ?? 0;
108 if (version > ids.length) {
109 defect(
110 "db.future_schema",
111 `database is at schema ${version}, this build only knows ${ids.length} — refusing to run`,
112 );
113 }
114 db.transaction(() => {
115 db.exec(`CREATE TABLE schema_migration (
116 id TEXT PRIMARY KEY,
117 position INTEGER NOT NULL,
118 applied_at TEXT NOT NULL
119 )`);
120 const insert = db.query(
121 "INSERT INTO schema_migration (id, position, applied_at) VALUES (?1, ?2, ?3)",
122 );
123 const at = new Date().toISOString();
124 for (let i = 0; i < version; i++)
125 if (leftItsMark(db, migrations.slice(i, version)))
126 insert.run(ids[i]!, i + 1, at);
127 })();
128 }
129 return new Set(
130 db
131 .query<{ id: string }, []>("SELECT id FROM schema_migration")
132 .all()
133 .map((row) => row.id),
134 );
135 }
136
137 const CREATE_RE =
138 /^(CREATE\s+(?:UNIQUE\s+|TEMP\s+|TEMPORARY\s+|VIRTUAL\s+)?(?:TABLE|INDEX|TRIGGER|VIEW))\s+(?!IF\s+NOT\s+EXISTS\b)/i;
139 const CREATE_TABLE_RE =
140 /^CREATE\s+(?:TEMP\s+|TEMPORARY\s+|VIRTUAL\s+)?TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?["`[]?(\w+)/i;
141 const ADD_COLUMN_RE =
142 /^ALTER\s+TABLE\s+["`[]?(\w+)["`\]]?\s+ADD\s+(?:COLUMN\s+)?["`[]?(\w+)/i;
143
144 /** A `CREATE` that meets its own object does nothing instead of failing. */
145 export function idempotent(statement: string): string {
146 return statement.replace(CREATE_RE, "$1 IF NOT EXISTS ");
147 }
148
149 /** An `ADD COLUMN` whose column the table already has. */
150 function alreadyAdded(db: Database, statement: string): boolean {
151 const match = ADD_COLUMN_RE.exec(statement);
152 return match !== null && hasColumn(db, match[1]!, match[2]!);
153 }
154
155 const GONE_RE =
156 /^(?:DROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?["`[]?(\w+)|ALTER\s+TABLE\s+["`[]?(\w+)["`\]]?\s+RENAME\s+TO\b)/i;
157
158 /**
159 * The first of `run` (a migration and the ones applied after it) left its
160 * mark: every table it creates exists and every column it adds is there —
161 * except on tables it or a later one drops or renames away.
162 */
163 function leftItsMark(db: Database, run: readonly string[]): boolean {
164 const gone = new Set(
165 run
166 .flatMap(statements)
167 .map((statement) => {
168 const match = GONE_RE.exec(statement);
169 return (match?.[1] ?? match?.[2])?.toLowerCase();
170 })
171 .filter((name) => name !== undefined),
172 );
173 for (const statement of statements(run[0]!)) {
174 const table = CREATE_TABLE_RE.exec(statement);
175 if (table && !gone.has(table[1]!.toLowerCase()) && !hasTable(db, table[1]!))
176 return false;
177 const column = ADD_COLUMN_RE.exec(statement);
178 if (
179 column &&
180 !gone.has(column[1]!.toLowerCase()) &&
181 !hasColumn(db, column[1]!, column[2]!)
182 )
183 return false;
184 }
185 return true;
186 }
187
188 function hasTable(db: Database, name: string): boolean {
189 return (
190 db
191 .query<{ n: number }, [string]>(
192 "SELECT COUNT(*) AS n FROM sqlite_master WHERE type = 'table' AND name = ?1",
193 )
194 .get(name)!.n === 1
195 );
196 }
197
198 function hasColumn(db: Database, table: string, column: string): boolean {
199 return (
200 db
201 .query<{ n: number }, [string, string]>(
202 "SELECT COUNT(*) AS n FROM pragma_table_info(?1) WHERE name = ?2 COLLATE NOCASE",
203 )
204 .get(table, column)!.n === 1
205 );
206 }
207
208 /**
209 * A migration's statements, one at a time, so a failure can name the one
210 * that failed. Splits on `;` outside quotes and comments; a trigger body
211 * (`CREATE TRIGGER … BEGIN …; END`) stays one statement.
212 */
213 export function statements(sql: string): string[] {
214 const out: string[] = [];
215 let current = "";
216 let i = 0;
217 while (i < sql.length) {
218 const ch = sql[i]!;
219 if (ch === "'" || ch === '"' || ch === "`" || ch === "[") {
220 const close = ch === "[" ? "]" : ch;
221 const end = sql.indexOf(close, i + 1);
222 const stop = end === -1 ? sql.length : end + 1;
223 current += sql.slice(i, stop);
224 i = stop;
225 continue;
226 }
227 if (ch === "-" && sql[i + 1] === "-") {
228 const end = sql.indexOf("\n", i);
229 i = end === -1 ? sql.length : end;
230 continue;
231 }
232 if (ch === "/" && sql[i + 1] === "*") {
233 const end = sql.indexOf("*/", i + 2);
234 i = end === -1 ? sql.length : end + 2;
235 continue;
236 }
237 if (ch === ";") {
238 const trimmed = current.trim();
239 const openTrigger =
240 /^CREATE\s+(TEMP\s+|TEMPORARY\s+)?TRIGGER\b/i.test(trimmed) &&
241 !/\bEND$/i.test(trimmed);
242 if (openTrigger) {
243 current += ch;
244 } else {
245 if (trimmed !== "") out.push(trimmed);
246 current = "";
247 }
248 i++;
249 continue;
250 }
251 current += ch;
252 i++;
253 }
254 if (current.trim() !== "") out.push(current.trim());
255 return out;
256 }
257
258 /** A statement as one short line: whitespace collapsed, long ones cut. */
259 function quote(statement: string): string {
260 const line = statement.replace(/\s+/g, " ").trim();
261 return `"${line.length > QUOTE_MAX ? `${line.slice(0, QUOTE_MAX)}…` : line}"`;
262 }
263
264 function messageOf(error: unknown): string {
265 return (error instanceof Error ? error.message : String(error))
266 .replace(/\s+/g, " ")
267 .trim();
268 }
test/schema-migrations.test.ts added +219
@@ -0,0 +1,219 @@
1 /**
2 * Migrations are tracked by what they say, not where they sit (#558).
3 *
4 * A preview starts from a copy of production's data and keeps it across
5 * redeploys. The shapes that killed previews at boot with "table already
6 * exists" are each a boot here, on a copy of real forge data: the same build
7 * twice, a branch whose migration was renumbered by its rebase, and a branch
8 * that adds a table the data already has. A failure names its statement in
9 * one line, which is what the deploy's summary on the pull shows.
10 */
11
12 import { afterAll, describe, expect, test } from "bun:test";
13 import { copyFileSync, mkdtempSync, rmSync } from "node:fs";
14 import { tmpdir } from "node:os";
15 import { join } from "node:path";
16 import type { Database } from "bun:sqlite";
17 import { openDb } from "../src/kernel/db.ts";
18 import { ForgeError } from "../src/kernel/error.ts";
19 import { statements } from "../src/kernel/migrate.ts";
20 import { migrations } from "../src/core/schema.ts";
21 import { failureReason } from "../src/apps/process.ts";
22
23 const root = mkdtempSync(join(tmpdir(), "forge-schema-"));
24 afterAll(() => rmSync(root, { recursive: true, force: true }));
25
26 let n = 0;
27 const fresh = (): string => join(root, `db-${++n}`);
28
29 /** Production's data, closed and checkpointed, ready to be copied. */
30 function production(list: readonly string[]): string {
31 const path = fresh();
32 const db = openDb(path, list);
33 db.exec("PRAGMA wal_checkpoint(TRUNCATE)");
34 db.close();
35 return path;
36 }
37
38 /** What `apps/data.ts` does for a preview: a copy of production's file. */
39 function copyOf(path: string): string {
40 const target = fresh();
41 copyFileSync(path, target);
42 return target;
43 }
44
45 function boot(path: string, list: readonly string[]): void {
46 openDb(path, list).close();
47 }
48
49 function has(db: Database, table: string): boolean {
50 return (
51 db
52 .query<{ n: number }, [string]>(
53 "SELECT COUNT(*) AS n FROM sqlite_master WHERE type = 'table' AND name = ?1",
54 )
55 .get(table)!.n === 1
56 );
57 }
58
59 const ownTable = "CREATE TABLE branch_note (id TEXT PRIMARY KEY, body TEXT NOT NULL)";
60 const mainTable = "CREATE TABLE main_note (id TEXT PRIMARY KEY)";
61
62 describe("a preview booting on copied data", () => {
63 test("boots, and boots again: the second boot is a no-op", () => {
64 const preview = copyOf(production(migrations));
65 boot(preview, migrations);
66 const db = openDb(preview, migrations);
67 expect(
68 db.query<{ n: number }, []>("SELECT COUNT(*) AS n FROM schema_migration").get()!.n,
69 ).toBe(migrations.length);
70 db.close();
71 });
72
73 test("a branch's migration renumbered by its rebase is not run twice", () => {
74 const prod = production(migrations);
75 // The branch's first push: its own table as the next migration.
76 const preview = copyOf(prod);
77 boot(preview, [...migrations, ownTable]);
78 // Main lands a migration of its own; the branch rebases onto it and
79 // its table is now one further down the list. The preview keeps its
80 // data, so the same CREATE meets a database that already ran it.
81 boot(preview, [...migrations, mainTable, ownTable]);
82 const db = openDb(preview, [...migrations, mainTable, ownTable]);
83 expect(has(db, "branch_note")).toBe(true);
84 expect(has(db, "main_note")).toBe(true);
85 db.close();
86 });
87
88 test("a branch that adds a table on data that already has it boots", () => {
89 // Preview data written by the positional runner: the branch's table
90 // ran as migration N+1 and user_version says N+1. The branch rebases
91 // onto main's own N+1; its table is now N+2 and already exists.
92 const path = copyOf(production(migrations));
93 const { Database } = require("bun:sqlite") as typeof import("bun:sqlite");
94 const old = new Database(path, { strict: true });
95 old.exec("DROP TABLE schema_migration");
96 old.exec(ownTable);
97 old.exec(`PRAGMA user_version = ${migrations.length + 1}`);
98 old.close();
99 const rebased = [...migrations, mainTable, ownTable];
100 boot(path, rebased);
101 const db = openDb(path, rebased);
102 expect(has(db, "branch_note")).toBe(true);
103 expect(has(db, "main_note")).toBe(true);
104 expect(
105 db.query<{ n: number }, []>("SELECT COUNT(*) AS n FROM schema_migration").get()!.n,
106 ).toBe(rebased.length);
107 db.close();
108 });
109
110 test("a column that is already there is not added twice", () => {
111 const path = production(["CREATE TABLE a (x INTEGER)", "ALTER TABLE a ADD COLUMN y TEXT"]);
112 // The same column, under a migration this database has not recorded.
113 boot(path, [
114 "CREATE TABLE a (x INTEGER)",
115 "ALTER TABLE a ADD COLUMN y TEXT",
116 "ALTER TABLE a ADD COLUMN y TEXT NOT NULL DEFAULT ''",
117 ]);
118 });
119
120 test("data from before the tracking table counts its user_version as applied", () => {
121 // A database written by the positional runner: user_version alone.
122 const path = fresh();
123 const { Database } = require("bun:sqlite") as typeof import("bun:sqlite");
124 const old = new Database(path, { create: true, strict: true });
125 old.exec("CREATE TABLE a (x INTEGER)");
126 old.exec("PRAGMA user_version = 1");
127 old.close();
128 const db = openDb(path, ["CREATE TABLE a (x INTEGER)", "CREATE TABLE b (y INTEGER)"]);
129 expect(has(db, "b")).toBe(true);
130 expect(db.query<{ user_version: number }, []>("PRAGMA user_version").get()!.user_version).toBe(2);
131 db.close();
132 });
133
134 test("production's own data from before the tracking table adopts every migration", () => {
135 const path = copyOf(production(migrations));
136 const { Database } = require("bun:sqlite") as typeof import("bun:sqlite");
137 const old = new Database(path, { strict: true });
138 old.exec("DROP TABLE schema_migration");
139 old.close();
140 const db = openDb(path, migrations);
141 const positions = db
142 .query<{ position: number }, []>("SELECT position FROM schema_migration ORDER BY position")
143 .all()
144 .map((row) => row.position);
145 // Every migration recorded once, at its own position.
146 expect(positions).toEqual(migrations.map((_, i) => i + 1));
147 db.close();
148 });
149
150 test("data ahead of this build is still refused", () => {
151 const path = production(["CREATE TABLE a (x INTEGER)", "CREATE TABLE b (y INTEGER)"]);
152 expect(() => openDb(path, ["CREATE TABLE a (x INTEGER)"])).toThrow(ForgeError);
153 });
154 });
155
156 describe("a migration that fails", () => {
157 test("names the statement that failed, in one line, and applies nothing", () => {
158 const path = production(["CREATE TABLE a (x INTEGER)"]);
159 let thrown: unknown;
160 try {
161 openDb(path, [
162 "CREATE TABLE a (x INTEGER)",
163 "CREATE TABLE fine (y INTEGER);\n INSERT INTO\n missing VALUES (1);",
164 ]);
165 } catch (error) {
166 thrown = error;
167 }
168 expect(ForgeError.isInstance(thrown)).toBe(true);
169 const message = (thrown as ForgeError).message;
170 expect(message).toBe(
171 'migration 2 failed at "INSERT INTO missing VALUES (1)": no such table: missing',
172 );
173 const db = openDb(path, ["CREATE TABLE a (x INTEGER)"]);
174 expect(has(db, "fine")).toBe(false);
175 db.close();
176 });
177
178 test("the deploy's summary is that line", async () => {
179 const path = production(["CREATE TABLE a (x INTEGER)"]);
180 const script = join(root, "boot.ts");
181 await Bun.write(
182 script,
183 `import { openDb } from ${JSON.stringify(join(import.meta.dir, "../src/kernel/db.ts"))};
184 openDb(${JSON.stringify(path)}, ["CREATE TABLE a (x INTEGER)", "INSERT INTO missing VALUES (1)"]);`,
185 );
186 const proc = Bun.spawn(["bun", script], { stdout: "pipe", stderr: "pipe" });
187 const output = (await new Response(proc.stderr).text()) + (await new Response(proc.stdout).text());
188 expect(await proc.exited).not.toBe(0);
189 const reason = failureReason(output.split("\n"));
190 expect(reason).toContain(
191 'migration 2 failed at "INSERT INTO missing VALUES (1)": no such table: missing',
192 );
193 });
194 });
195
196 describe("statements", () => {
197 test("split on semicolons outside quotes, comments and trigger bodies", () => {
198 expect(
199 statements(`
200 CREATE TABLE a (x TEXT DEFAULT 'a;b'); -- a comment; with a semicolon
201 /* a block; comment */ INSERT INTO a VALUES (";");
202 CREATE TRIGGER t AFTER INSERT ON a BEGIN
203 DELETE FROM a WHERE x = 'z';
204 DELETE FROM a WHERE x = 'y';
205 END;
206 `),
207 ).toEqual([
208 "CREATE TABLE a (x TEXT DEFAULT 'a;b')",
209 'INSERT INTO a VALUES (";")',
210 expect.stringMatching(/^CREATE TRIGGER t[\s\S]*'y';\s+END$/),
211 ]);
212 });
213
214 test("every forge migration applies on an empty database", () => {
215 const db = openDb(":memory:", migrations);
216 expect(has(db, "usage")).toBe(true);
217 db.close();
218 });
219 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs