From b807f7b5a7f80552367da85f11e083eff2480183 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Thu, 3 Sep 2026 09:57:43 +0530 Subject: [PATCH] fix(db): refuse to roll back a migration that defines no down section rollback ran a migration's down section when it had one and then deleted its tracking row either way. A migration that changed the schema without a `-- +down` section was therefore recorded as rolled back while its change stayed in the database, and the next migrate replayed statements such as ALTER TABLE ADD COLUMN that cannot run twice, leaving the database stuck between the two states with no way forward or back. It now refuses that case with WRN-DB-MIGRATION-IRREVERSIBLE and leaves the tracking row alone, so the database stays consistent and the operator chooses: add a down section, or write a new migration that reverses the change. The check runs before the lock is taken, so a dry run reports the same refusal rather than promising a rollback that would fail. A migration whose up is empty still rolls back by dropping its tracking row, since there is nothing to undo. That keeps placeholder migrations working, and `wrnexus db rollback` prints the refusal as a message rather than a stack trace. Co-Authored-By: Claude Opus 5 --- packages/cli/src/db.ts | 13 +++++++- packages/db/src/migrate.ts | 24 +++++++++++++-- packages/db/test/db.test.ts | 61 +++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 6ef48254..9392187c 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -313,7 +313,18 @@ export async function runDbCommand( break; } case "rollback": { - const name = await rollback(db, migrationsDir); + let name: string | null; + try { + name = await rollback(db, migrationsDir); + } catch (error) { + // A migration with no down section is refused rather than half-undone. + // That is an operator decision to make, not a stack trace to read. + const message = error instanceof Error ? error.message : String(error); + if (!message.startsWith("WRN-DB-MIGRATION-IRREVERSIBLE")) throw error; + console.error(`✗ ${message.replace("WRN-DB-MIGRATION-IRREVERSIBLE: ", "")}`); + process.exitCode = 1; + break; + } console.log(name ? `✓ Rolled back ${name}${label}` : `Nothing to roll back${label}.`); break; } diff --git a/packages/db/src/migrate.ts b/packages/db/src/migrate.ts index aff7c0ee..4bff8ef6 100644 --- a/packages/db/src/migrate.ts +++ b/packages/db/src/migrate.ts @@ -231,7 +231,18 @@ export async function migrate( return applyMigrations(db, loadMigrations(dir), options); } -/** Roll back the most recently applied migration. Returns its name, or null. */ +/** + * Roll back the most recently applied migration. Returns its name, or null when + * nothing has been applied. + * + * Refuses a migration that changed the schema but defines no `-- +down` section. + * Removing its tracking row would leave the change in place and unrecorded, and + * the next `migrate` would replay statements such as `ALTER TABLE ... ADD COLUMN` + * that cannot run twice, stranding the database between the two states. + * + * A migration whose `up` is empty — a placeholder, or comments only — has + * nothing to undo, so it rolls back by dropping its tracking row alone. + */ export async function rollback( db: Db, dir: string, @@ -241,12 +252,21 @@ export async function rollback( const applied = await appliedMigrations(db); const last = applied[applied.length - 1]; if (!last) return null; + const migration = loadMigrations(dir).find((m) => m.name === last); + const reversible = + !migration || !hasExecutableSql(migration.up) || hasExecutableSql(migration.down); + if (!reversible) + throw new Error( + `WRN-DB-MIGRATION-IRREVERSIBLE: ${last} changes the schema but defines no "-- +down" section, ` + + "so rolling it back would drop its record while leaving the change applied. " + + "Add a `-- +down` section to that migration, or write a new migration that reverses it.", + ); + // Checked before the lock so a dry run reports the same refusal. if (options.dryRun) return last; const release = options.lock === false ? undefined : await acquireMigrationLock(db, options.lockTimeoutMs ?? 300_000); - const migration = loadMigrations(dir).find((m) => m.name === last); try { throwIfAborted(options.signal); await db.tx(async (tx) => { diff --git a/packages/db/test/db.test.ts b/packages/db/test/db.test.ts index 5ea5ae2c..51338118 100644 --- a/packages/db/test/db.test.ts +++ b/packages/db/test/db.test.ts @@ -145,6 +145,67 @@ test("an empty migration set does not touch the database", async () => { expect(await applyMigrations(db, [])).toEqual([]); }); +test("rollback refuses a schema change that defines no down section", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-nodown-")); + writeFileSync(join(dir, "0001_add.sql"), "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);"); + + const db = createDb(sqlite()); + expect(await migrate(db, dir)).toEqual(["0001_add"]); + + // Rolling this back would drop the record and leave the table, and the next + // migrate would replay a CREATE that cannot run twice. + await expect(rollback(db, dir)).rejects.toThrow(/WRN-DB-MIGRATION-IRREVERSIBLE/); + + // The refusal leaves the record alone, so the database stays consistent. + expect(await appliedMigrations(db)).toEqual(["0001_add"]); + expect((await status(db, dir))[0]!.applied).toBe(true); + db.close(); +}); + +test("a dry run reports the same refusal instead of promising a rollback", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-dry-")); + writeFileSync(join(dir, "0001_add.sql"), "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);"); + + const db = createDb(sqlite()); + await migrate(db, dir); + await expect(rollback(db, dir, { dryRun: true })).rejects.toThrow( + /WRN-DB-MIGRATION-IRREVERSIBLE/, + ); + db.close(); +}); + +test("rollback still undoes a migration that defines a down section", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-down-")); + writeFileSync( + join(dir, "0001_add.sql"), + "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);\n-- +down\nDROP TABLE t;", + ); + + const db = createDb(sqlite()); + await migrate(db, dir); + expect(await rollback(db, dir)).toBe("0001_add"); + expect(await appliedMigrations(db)).toEqual([]); + const remaining = await db.all<{ name: string }>( + "SELECT name FROM sqlite_master WHERE type='table' AND name='t'", + ); + expect(remaining.length).toBe(0); + db.close(); +}); + +test("a migration whose up is only comments has nothing to undo and rolls back", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-noop-")); + writeFileSync( + join(dir, "0001_placeholder.sql"), + "-- +up\n-- Nothing yet.\n-- +down\n-- Nothing to undo.", + ); + + const db = createDb(sqlite()); + await migrate(db, dir); + expect(await rollback(db, dir)).toBe("0001_placeholder"); + expect(await appliedMigrations(db)).toEqual([]); + db.close(); +}); + test("comment-only migrations are recorded without executing empty SQL", async () => { const db = createDb(sqlite()); const migrations = [