From 1562b117642247c9607b8f4cfd24ab9c342abc54 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:59:48 -0700 Subject: [PATCH] fix(selfhost): translate SQLite AUTOINCREMENT so migrations apply on Postgres Migration 0209 declares `id INTEGER PRIMARY KEY AUTOINCREMENT`. Postgres cannot parse AUTOINCREMENT (SQLSTATE 42601), so runSelfHostMigrations aborted and the process exited: every Orb upgrading past 0208 crash-looped on boot. Reproduced deploying orb-v3.7.0-beta.6 to edge-nl-01; rolled back to beta.5. No data damage -- migrations apply in a transaction, so the table was never created and the ledger still ends at 0208. Migrations are authored in SQLite dialect and translated by translateDdl, which covers SQLite date/json functions and INSERT OR IGNORE. Its doc comment records the assumption behind the gap -- "column types are PG-native" -- which is true of types, but AUTOINCREMENT is a CONSTRAINT. 0209 is the only migration that uses it; every other table uses plain INTEGER PRIMARY KEY, which parses on both dialects, so the gap stayed latent. Stripping the keyword would be worse than leaving it. Bare INTEGER PRIMARY KEY auto-assigns in SQLite but is an ordinary NOT NULL integer in Postgres, so the DDL would parse and every id-omitting insert would fail at runtime instead -- silently, since the writer is best effort and swallows errors. It is rendered as a Postgres identity column instead: BY DEFAULT rather than ALWAYS, because SQLite permits an explicit id, and BIGINT to match the 64-bit rowid. The migration file is deliberately untouched. contentSha256 hashes the raw source, so editing it would throw selfhost_migration_content_drift on every Orb that already applied it -- and on D1 the SQLite syntax is valid. The real-Postgres suite already catches this ("applies every migration") but only runs in selfhost.yml, which is push-to-main only: it went red on the very commit that introduced 0209 and stayed red across five runs while PRs kept merging. The pre-merge counterpart added here applies translateDdl to every migration and fails if a SQLite-only construct survives -- no Postgres, no service container, ~11ms inside the unit run that already happens, because CI runtime is the constraint that moved that workflow post-merge to begin with. Checked post-translation rather than against a banned-string list so it cannot drift from the translator, and comment-stripped so the many migrations that merely DISCUSS these constructs are not flagged. selfhost.yml's header claimed ci.yml validated the pg integration suites pre-merge. It does not and cannot -- there is no service container, so without PG_TEST_URL they skip. Corrected in place. Closes #10138 --- .github/workflows/selfhost.yml | 23 ++- src/selfhost/pg-dialect.ts | 32 ++- .../migration-dialect-portability.test.ts | 185 ++++++++++++++++++ 3 files changed, 233 insertions(+), 7 deletions(-) create mode 100644 test/unit/migration-dialect-portability.test.ts diff --git a/.github/workflows/selfhost.yml b/.github/workflows/selfhost.yml index 22b1a3f58d..11fd188b74 100644 --- a/.github/workflows/selfhost.yml +++ b/.github/workflows/selfhost.yml @@ -6,11 +6,24 @@ # # Push-to-main only since 2026-07-24 (was also per-PR): at this repo's PR volume the ~5-minute # build-boot fired ~100x/week (its path list includes migrations/**, which nearly every backend PR -# touches) and was a top runner-queue consumer. Pre-merge, ci.yml already validates everything except -# the docker boot itself (db:migrations:check, schema drift, the selfhost pg integration suites); a -# boot-breaking merge is caught by this run minutes after landing, and self-host users only ever -# consume tagged releases (release-selfhost.yml), never main -- so the pre-merge boot smoke bought -# almost nothing at real cost. Revisit if a hosted deploy ever tracks main directly. +# touches) and was a top runner-queue consumer. A boot-breaking merge is caught by this run minutes +# after landing, and self-host users only ever consume tagged releases (release-selfhost.yml), never +# main. Revisit if a hosted deploy ever tracks main directly. +# +# #10138 CORRECTION. This header used to claim ci.yml validated "the selfhost pg integration suites" +# pre-merge. It does not, and cannot: those suites require a real Postgres, ci.yml has no service +# container, and without PG_TEST_URL they SKIP. The only place they run is this workflow -- post-merge. +# +# That gap shipped an outage. Migration 0209 used SQLite `AUTOINCREMENT`, which Postgres cannot parse, +# so every Orb upgrading past 0208 crash-looped on boot. The "applies every migration" test here caught +# it correctly on the very first push -- and then this workflow stayed red across five consecutive runs +# while PRs kept merging, because a post-merge failure blocks nothing and pages no one. +# +# The pre-merge counterpart is test/unit/migration-dialect-portability.test.ts: it applies translateDdl +# to every migration and fails if a SQLite-only construct survives. No Postgres, no service container, +# ~11ms inside the unit run that already happens -- deliberately NOT a second PG job, because CI +# runtime is the constraint that moved this workflow post-merge in the first place. It proves parseable, +# not correct; this workflow remains the only real-Postgres coverage, and its red is worth acting on. name: self-host on: diff --git a/src/selfhost/pg-dialect.ts b/src/selfhost/pg-dialect.ts index 4161ea3223..09c4134a8f 100644 --- a/src/selfhost/pg-dialect.ts +++ b/src/selfhost/pg-dialect.ts @@ -246,8 +246,36 @@ export function translateMigrationInserts(sql: string): string { return sql.replace(/INSERT\s+OR\s+IGNORE\s+INTO\b([^;]*);/gi, "INSERT INTO$1 ON CONFLICT DO NOTHING;"); } +/** + * SQLite's auto-assigning surrogate key, rendered as its Postgres equivalent (#10138). + * + * `INTEGER PRIMARY KEY AUTOINCREMENT` is a hard Postgres PARSE error -- `syntax error at or near + * "AUTOINCREMENT"`, SQLSTATE 42601 -- so a migration containing it aborts runSelfHostMigrations and the Orb + * crash-loops on boot. That is what 0209 did: every Orb upgrading past 0208 failed to start. + * + * Stripping the keyword alone would be WORSE, not better. A bare `INTEGER PRIMARY KEY` is auto-assigning in + * SQLite (it aliases the rowid) but is an ordinary NOT NULL integer column in Postgres, so the DDL would + * parse and then every insert that omits the id -- which is every insert, that being the point of a + * surrogate key -- would fail its NOT NULL constraint at RUNTIME instead. On a best-effort writer that + * swallows errors (service_status_samples is exactly that) it would fail silently and simply record + * nothing. Trading a loud boot failure for a silent data-loss failure is not a fix. + * + * `BY DEFAULT` rather than `ALWAYS`: SQLite permits an explicit id on insert, and `ALWAYS` would reject one, + * which would change behaviour for any caller that supplies its own key. `BIGINT` matches SQLite's 64-bit + * rowid rather than narrowing it to int4. + * + * Only the canonical `INTEGER PRIMARY KEY AUTOINCREMENT` spelling is translated. A stray `AUTOINCREMENT` in + * some other shape stays untranslated and surfaces as a clear Postgres syntax error -- the same deliberate + * stance translateMigrationInserts takes with `INSERT OR REPLACE`: fail visibly rather than guess at intent. + */ +export function translateAutoincrementPrimaryKey(sql: string): string { + return sql.replace(/\bINTEGER\s+PRIMARY\s+KEY\s+AUTOINCREMENT\b/gi, "BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY"); +} + /** Translate a DDL statement (migrations). Column types (TEXT/INTEGER/REAL) are PG-native; the SQLite - * default expressions need translating, as does any `INSERT OR IGNORE` seed. No `?` placeholders in DDL. */ + * default expressions need translating, as does any `INSERT OR IGNORE` seed and any AUTOINCREMENT surrogate + * key (#10138 -- a CONSTRAINT rather than a type, which is why the "types are PG-native" note above did not + * cover it). No `?` placeholders in DDL. */ export function translateDdl(sql: string): string { - return translateFunctions(translateMigrationInserts(sql)); + return translateAutoincrementPrimaryKey(translateFunctions(translateMigrationInserts(sql))); } diff --git a/test/unit/migration-dialect-portability.test.ts b/test/unit/migration-dialect-portability.test.ts new file mode 100644 index 0000000000..66f09d55b5 --- /dev/null +++ b/test/unit/migration-dialect-portability.test.ts @@ -0,0 +1,185 @@ +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { translateAutoincrementPrimaryKey, translateDdl } from "../../src/selfhost/pg-dialect"; + +// #10138: migrations are authored in SQLite dialect and translated for Postgres by translateDdl. A construct +// nobody thought to translate is a BOOT failure, not a test failure -- runSelfHostMigrations aborts, the +// process exits, and every Orb upgrading past that migration crash-loops. +// +// That is not hypothetical. 0209 shipped `INTEGER PRIMARY KEY AUTOINCREMENT`, which Postgres cannot parse +// (SQLSTATE 42601), and every Orb past 0208 failed to start until it was translated. +// +// The real-Postgres suite (test/integration/selfhost-pg.test.ts, "applies every migration") DID catch it -- +// but it only runs in selfhost.yml, which is push-to-main only, so the signal arrived AFTER the merge that +// caused it. This file is the pre-merge counterpart: no service container, no Docker, no Postgres, a few +// milliseconds inside the unit run that already happens. It cannot replace the real-PG suite (it proves +// nothing about semantics) and does not try to -- it exists to make THIS failure class impossible to merge. +// +// ── DERIVED FROM THE TRANSLATOR, NOT RESTATED ──────────────────────────────────────────────────────────── +// The assertion runs translateDdl over each migration and checks the OUTPUT. A hand-maintained list of +// "banned strings in migration source" would be a second source of truth that drifts the moment someone adds +// a translation, and would also produce false failures for constructs the translator legitimately handles. +// Checking post-translation means adding a translation automatically satisfies this test, and removing one +// automatically fails it. + +const MIGRATIONS_DIR = join(import.meta.dirname, "..", "..", "migrations"); + +/** SQLite-only constructs that Postgres cannot parse. Each is a hard 42601-class parse error, so surviving + * translation means an Orb that will not boot. Deliberately narrow: only tokens whose presence in a + * Postgres statement is unambiguously fatal, so this can never fail a portable migration. */ +const SQLITE_ONLY_CONSTRUCTS: ReadonlyArray<{ pattern: RegExp; construct: string; fix: string }> = [ + { + pattern: /\bAUTOINCREMENT\b/i, + construct: "AUTOINCREMENT", + fix: "write `INTEGER PRIMARY KEY AUTOINCREMENT` (translateAutoincrementPrimaryKey renders it as a Postgres identity column), or use a non-surrogate key", + }, + { + pattern: /\bINSERT\s+OR\s+REPLACE\b/i, + construct: "INSERT OR REPLACE", + fix: "use `INSERT ... ON CONFLICT () DO UPDATE`; the translator deliberately does not guess a conflict target", + }, + { + pattern: /\bWITHOUT\s+ROWID\b/i, + construct: "WITHOUT ROWID", + fix: "drop it -- it is a SQLite storage hint with no Postgres equivalent and no semantic effect worth porting", + }, + { + pattern: /\bPRAGMA\b/i, + construct: "PRAGMA", + fix: "drop it -- PRAGMA is a SQLite-only statement; per-connection settings belong in the adapter, not a migration", + }, +]; + +function migrationFiles(): string[] { + return readdirSync(MIGRATIONS_DIR) + .filter((name) => name.endsWith(".sql")) + .sort(); +} + +/** + * Strip SQL comments before scanning. Not cosmetic -- without it this check is unusable. + * + * These migrations are heavily commented, and the comments discuss the very constructs being banned: 0054's + * header documents `INSERT OR REPLACE` as the writer's behaviour, 0176 explains why an ingest path used it, + * and 0180 annotates a column with "NOT autoincrement". All three parse and run fine on Postgres; flagging + * them would train the next person to ignore this test, which is worse than not having it. + * + * `--` inside a string literal would be over-stripped. Accepted deliberately: that can only cause a false + * PASS, never a false failure, and the real-Postgres suite is the backstop for anything this misses. A + * checker that cries wolf gets muted; one that occasionally stays quiet does not. + */ +function stripSqlComments(sql: string): string { + return sql.replace(/\/\*[\s\S]*?\*\//g, " ").replace(/--[^\n]*/g, " "); +} + +describe("migration dialect portability (#10138)", () => { + it("sanity: there are migrations to check, so a broken glob cannot pass this file vacuously", () => { + // Without this, a wrong MIGRATIONS_DIR makes every assertion below iterate an empty list and report + // green -- the exact "verifier that passes because it checked nothing" failure the gate exists to catch. + expect(migrationFiles().length).toBeGreaterThan(200); + }); + + it("INVARIANT: no SQLite-only construct survives translateDdl, for every shipped migration", () => { + const offenders: string[] = []; + for (const name of migrationFiles()) { + const translated = stripSqlComments(translateDdl(readFileSync(join(MIGRATIONS_DIR, name), "utf8"))); + for (const { pattern, construct, fix } of SQLITE_ONLY_CONSTRUCTS) { + if (pattern.test(translated)) offenders.push(`${name}: ${construct} survives translation — ${fix}`); + } + } + // Asserted as the full list rather than a count, so the failure message names the file and the remedy + // instead of making the next person bisect the migrations directory. + expect(offenders).toEqual([]); + }); + + it("SELF-TEST: the check still fires on real DDL, and only comments are exempt", () => { + // Comment-stripping is how this check avoids crying wolf, and it is also how it could be silently + // neutered -- a broader strip that swallowed real statements would make the invariant above pass + // unconditionally. Both directions are pinned here rather than trusted. + const detect = (sql: string): boolean => + SQLITE_ONLY_CONSTRUCTS.some(({ pattern }) => pattern.test(stripSqlComments(translateDdl(sql)))); + + // An UNTRANSLATED SQLite-only construct in live DDL is still caught. + expect(detect("CREATE TABLE t (id INTEGER NOT NULL) WITHOUT ROWID;")).toBe(true); + expect(detect("INSERT OR REPLACE INTO t VALUES (1);")).toBe(true); + // The same text inside a comment is not. + expect(detect("-- WITHOUT ROWID is a SQLite thing\nCREATE TABLE t (id INTEGER NOT NULL);")).toBe(false); + expect(detect("/* INSERT OR REPLACE was the old writer */\nCREATE TABLE t (id INTEGER);")).toBe(false); + // And the construct this incident was about is caught when NOT translated, which is what proves the + // green result below comes from the translation rather than from the scan missing it. + expect(SQLITE_ONLY_CONSTRUCTS.some(({ pattern }) => pattern.test("id INTEGER PRIMARY KEY AUTOINCREMENT"))).toBe(true); + }); + + it("REGRESSION (#10138): 0209 is the migration that proved this, and it now translates cleanly", () => { + // Pinned by name. The general invariant above would still pass if 0209 were deleted; this says the + // specific thing that broke production is fixed, and fails loudly if someone reverts the translation. + const sql = readFileSync(join(MIGRATIONS_DIR, "0209_service_status_samples.sql"), "utf8"); + expect(sql).toMatch(/AUTOINCREMENT/i); // the source is still SQLite dialect, by design -- migrations are immutable + expect(translateDdl(sql)).not.toMatch(/AUTOINCREMENT/i); + expect(translateDdl(sql)).toMatch(/BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY/); + }); +}); + +describe("translateAutoincrementPrimaryKey", () => { + it("renders SQLite's auto-assigning surrogate key as a Postgres identity column", () => { + expect(translateAutoincrementPrimaryKey("id INTEGER PRIMARY KEY AUTOINCREMENT,")).toBe( + "id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,", + ); + }); + + it("tolerates the whitespace real migrations are written with (aligned columns, newlines)", () => { + expect(translateAutoincrementPrimaryKey(" id INTEGER PRIMARY KEY AUTOINCREMENT,")).toContain( + "BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY", + ); + expect(translateAutoincrementPrimaryKey("id INTEGER\n PRIMARY KEY\n AUTOINCREMENT,")).toContain( + "BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY", + ); + }); + + it("is case-insensitive, matching how SQL is actually written", () => { + expect(translateAutoincrementPrimaryKey("id integer primary key autoincrement,")).toContain( + "BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY", + ); + }); + + it("BY DEFAULT, never ALWAYS — SQLite allows an explicit id and ALWAYS would reject one", () => { + // A caller supplying its own key is legal in SQLite; GENERATED ALWAYS would turn that into a runtime + // error on Postgres only, which is exactly the kind of dialect-dependent behaviour change this + // translation layer exists to prevent. + const out = translateAutoincrementPrimaryKey("id INTEGER PRIMARY KEY AUTOINCREMENT"); + expect(out).toContain("GENERATED BY DEFAULT AS IDENTITY"); + expect(out).not.toContain("GENERATED ALWAYS"); + }); + + it("BIGINT, not INTEGER — SQLite's rowid is 64-bit and narrowing it would silently cap the table", () => { + expect(translateAutoincrementPrimaryKey("id INTEGER PRIMARY KEY AUTOINCREMENT")).toMatch(/^id BIGINT\b/); + }); + + it("leaves a plain INTEGER PRIMARY KEY alone — it parses on both dialects and is the house pattern", () => { + // Every other table in the repo uses this. Rewriting it would change the meaning of ~200 migrations to + // fix one, and would make previously-explicit ids auto-assign. + expect(translateAutoincrementPrimaryKey("id INTEGER PRIMARY KEY,")).toBe("id INTEGER PRIMARY KEY,"); + }); + + it("does not touch a column merely named like the keyword", () => { + expect(translateAutoincrementPrimaryKey("autoincrement_enabled INTEGER NOT NULL")).toBe( + "autoincrement_enabled INTEGER NOT NULL", + ); + }); + + it("translates every occurrence, not just the first — one file may declare several tables", () => { + const two = "a INTEGER PRIMARY KEY AUTOINCREMENT, b INTEGER PRIMARY KEY AUTOINCREMENT"; + expect(translateAutoincrementPrimaryKey(two).match(/GENERATED BY DEFAULT AS IDENTITY/g)).toHaveLength(2); + }); + + it("is reached through translateDdl, which is what pg-adapter actually calls", () => { + // The unit above proves the rewrite; this proves it is WIRED. A correct translation that the DDL path + // never invokes is the same outage. + expect(translateDdl("CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT, v TEXT);")).toContain( + "BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY", + ); + }); +});