fix(sqlite): tag per-migration database backups with version and prevent collisions

server/setup/migrationsSqlite.backup.test.ts

@@ -40,11 +40,11 @@ function seedDatabase(dbPath: string) {
     }
 }
 
-function siteColumns(dbPath: string): string[] {
+function tableColumns(dbPath: string, tableName: string): string[] {
     const db = new Database(dbPath, { readonly: true });
     try {
         return (
-            db.prepare(`PRAGMA table_info(sites)`).all() as Array<{
+            db.prepare(`PRAGMA table_info(${tableName})`).all() as Array<{
                 name: unknown;
             }>
         ).map((row) => String(row.name));
@@ -53,7 +53,23 @@ function siteColumns(dbPath: string): string[] {
     }
 }
 
-function runMigrations(workdir: string): {
+function executedMigrationVersions(dbPath: string): string[] {
+    const db = new Database(dbPath, { readonly: true });
+    try {
+        return (
+            db.prepare(`SELECT version FROM versionMigrations`).all() as Array<{
+                version: unknown;
+            }>
+        ).map((row) => String(row.version));
+    } finally {
+        db.close();
+    }
+}
+
+function runMigrations(
+    workdir: string,
+    env: Record<string, string> = {}
+): {
     exitCode: number;
     output: string;
 } {
@@ -77,7 +93,12 @@ function runMigrations(workdir: string): {
         const output = execFileSync(
             process.execPath,
             [tsxCli, "--tsconfig", tsconfig, migrationsScript],
-            { cwd: workdir, timeout: 120000, encoding: "utf8" }
+            {
+                cwd: workdir,
+                timeout: 120000,
+                encoding: "utf8",
+                env: { ...process.env, NODE_ENV: "test", ...env }
+            }
         );
         return { exitCode: 0, output };
     } catch (error) {
@@ -89,8 +110,7 @@ function runMigrations(workdir: string): {
     }
 }
 
-function testSingleBackupPerUpgrade() {
-    console.log("Running single backup per upgrade test...");
+function createTestEnvironment(): string {
     for (const generated of ["server/build.ts", "server/db/index.ts"]) {
         if (!fs.existsSync(path.join(repoRoot, generated))) {
             throw new Error(
@@ -101,17 +121,29 @@ function testSingleBackupPerUpgrade() {
     const workdir = fs.mkdtempSync(
         path.join(os.tmpdir(), "pangolin-backup-test-")
     );
+    fs.mkdirSync(path.join(workdir, "config", "db"), { recursive: true });
+    fs.copyFileSync(
+        path.join(repoRoot, "config", "config.example.yml"),
+        path.join(workdir, "config", "config.yml")
+    );
+    const traefikSrc = path.join(repoRoot, "config", "traefik");
+    if (fs.existsSync(traefikSrc)) {
+        fs.cpSync(traefikSrc, path.join(workdir, "config", "traefik"), {
+            recursive: true
+        });
+    }
+    fs.symlinkSync(
+        path.join(repoRoot, "server"),
+        path.join(workdir, "server"),
+        process.platform === "win32" ? "junction" : "dir"
+    );
+    return workdir;
+}
+
+function testMultipleSequentialMigrations() {
+    console.log("Running multiple sequential migrations test...");
+    const workdir = createTestEnvironment();
     try {
-        fs.mkdirSync(path.join(workdir, "config", "db"), { recursive: true });
-        fs.copyFileSync(
-            path.join(repoRoot, "config", "config.example.yml"),
-            path.join(workdir, "config", "config.yml")
-        );
-        fs.symlinkSync(
-            path.join(repoRoot, "server"),
-            path.join(workdir, "server"),
-            process.platform === "win32" ? "junction" : "dir"
-        );
         seedDatabase(path.join(workdir, "config", "db", "db.sqlite"));
         const result = runMigrations(workdir);
         assertEquals(result.exitCode, 0, "Seeded migrations must run cleanly");
@@ -120,6 +152,169 @@ function testSingleBackupPerUpgrade() {
                 "Seeded migrations did not complete; the backup assertions below would be vacuous"
             );
         }
+
+        const backupsDir = path.join(workdir, "config", "db", "backups");
+        const backups = fs.existsSync(backupsDir)
+            ? fs
+                  .readdirSync(backupsDir)
+                  .filter((file) => file.endsWith(".sqlite"))
+            : [];
+
+        // Upgrading from 1.21.0 runs 1.22.0 and 1.23.0 -> produces 2 distinct backups
+        assertEquals(
+            backups.length,
+            2,
+            "Each migration must have its own distinct backup snapshot"
+        );
+
+        const v122Backup = backups.find((file) =>
+            file.includes("_v1.22.0.sqlite")
+        );
+        const v123Backup = backups.find((file) =>
+            file.includes("_v1.23.0.sqlite")
+        );
+
+        if (!v122Backup || !v123Backup) {
+            throw new Error(
+                `Expected backups for v1.22.0 and v1.23.0, found: ${backups.join(", ")}`
+            );
+        }
+
+        // Verify pre-1.22.0 snapshot state: sites has 'subnet' (not exitNodeSubnet), versions = [1.21.0]
+        const v122Columns = tableColumns(
+            path.join(backupsDir, v122Backup),
+            "sites"
+        );
+        assertEquals(
+            v122Columns.includes("subnet") &&
+                !v122Columns.includes("exitNodeSubnet"),
+            true,
+            "Backup before 1.22.0 must retain pre-1.22.0 schema (sites.subnet)"
+        );
+        const v122Versions = executedMigrationVersions(
+            path.join(backupsDir, v122Backup)
+        );
+        assertEquals(
+            v122Versions.includes("1.21.0") && !v122Versions.includes("1.22.0"),
+            true,
+            "Backup before 1.22.0 must only record version 1.21.0"
+        );
+
+        // Verify pre-1.23.0 snapshot state: sites has 'exitNodeSubnet' (1.22.0 applied), newt has no agent
+        const v123Columns = tableColumns(
+            path.join(backupsDir, v123Backup),
+            "sites"
+        );
+        assertEquals(
+            v123Columns.includes("exitNodeSubnet"),
+            true,
+            "Backup before 1.23.0 must contain successfully applied 1.22.0 schema (sites.exitNodeSubnet)"
+        );
+        const v123NewtCols = tableColumns(
+            path.join(backupsDir, v123Backup),
+            "newt"
+        );
+        assertEquals(
+            !v123NewtCols.includes("agent"),
+            true,
+            "Backup before 1.23.0 must not contain 1.23.0 schema changes yet"
+        );
+        const v123Versions = executedMigrationVersions(
+            path.join(backupsDir, v123Backup)
+        );
+        assertEquals(
+            v123Versions.includes("1.21.0") && v123Versions.includes("1.22.0"),
+            true,
+            "Backup before 1.23.0 must record both 1.21.0 and 1.22.0"
+        );
+    } finally {
+        fs.rmSync(workdir, { recursive: true, force: true });
+    }
+}
+
+function testFailureInLaterMigrationPreservesRestorePoints() {
+    console.log("Running failure in later migration test...");
+    const workdir = createTestEnvironment();
+    try {
+        const dbPath = path.join(workdir, "config", "db", "db.sqlite");
+        seedDatabase(dbPath);
+
+        // Intentionally drop table 'newt' so migration 1.23.0 fails on ALTER TABLE newt ADD COLUMN agent
+        const db = new Database(dbPath);
+        db.exec("DROP TABLE newt;");
+        db.close();
+
+        const result = runMigrations(workdir);
+        assertEquals(
+            result.exitCode,
+            1,
+            "Migration suite must fail when 1.23.0 errors"
+        );
+
+        const backupsDir = path.join(workdir, "config", "db", "backups");
+        const backups = fs.existsSync(backupsDir)
+            ? fs
+                  .readdirSync(backupsDir)
+                  .filter((file) => file.endsWith(".sqlite"))
+            : [];
+
+        // Both pre-1.22.0 and pre-1.23.0 backups must exist
+        assertEquals(
+            backups.length,
+            2,
+            "Backups for earlier successful migration and the failed migration must both exist"
+        );
+
+        const v122Backup = backups.find((file) =>
+            file.includes("_v1.22.0.sqlite")
+        );
+        const v123Backup = backups.find((file) =>
+            file.includes("_v1.23.0.sqlite")
+        );
+
+        if (!v122Backup || !v123Backup) {
+            throw new Error(
+                `Expected restore points for v1.22.0 and v1.23.0, found: ${backups.join(", ")}`
+            );
+        }
+
+        // Verify pre-1.23.0 backup is a valid restore point with 1.22.0 changes applied
+        const v123SitesCols = tableColumns(
+            path.join(backupsDir, v123Backup),
+            "sites"
+        );
+        assertEquals(
+            v123SitesCols.includes("exitNodeSubnet"),
+            true,
+            "Pre-failure restore point must have 1.22.0 changes intact"
+        );
+        const v123Versions = executedMigrationVersions(
+            path.join(backupsDir, v123Backup)
+        );
+        assertEquals(
+            v123Versions.includes("1.22.0"),
+            true,
+            "Pre-failure restore point must record successful 1.22.0 migration"
+        );
+    } finally {
+        fs.rmSync(workdir, { recursive: true, force: true });
+    }
+}
+
+function testDisableBackupOnMigration() {
+    console.log("Running DISABLE_BACKUP_ON_MIGRATION test...");
+    const workdir = createTestEnvironment();
+    try {
+        seedDatabase(path.join(workdir, "config", "db", "db.sqlite"));
+        const result = runMigrations(workdir, {
+            DISABLE_BACKUP_ON_MIGRATION: "1"
+        });
+        assertEquals(
+            result.exitCode,
+            0,
+            "Migrations must succeed with backups disabled"
+        );
+
         const backupsDir = path.join(workdir, "config", "db", "backups");
         const backups = fs.existsSync(backupsDir)
             ? fs
@@ -128,23 +323,19 @@ function testSingleBackupPerUpgrade() {
             : [];
         assertEquals(
             backups.length,
-            1,
-            "One upgrade must produce exactly one database backup even with several pending migrations"
+            0,
+            "No backup files should be created when DISABLE_BACKUP_ON_MIGRATION is set"
         );
-        const columns = siteColumns(path.join(backupsDir, backups[0]));
-        if (!columns.includes("subnet")) {
-            throw new Error(
-                `The single backup must be the pre-upgrade snapshot (sites.subnet), got sites(${columns.join(",")})`
-            );
-        }
     } finally {
         fs.rmSync(workdir, { recursive: true, force: true });
     }
 }
 
 try {
-    testSingleBackupPerUpgrade();
-    console.log("All tests passed successfully!");
+    testMultipleSequentialMigrations();
+    testFailureInLaterMigrationPreservesRestorePoints();
+    testDisableBackupOnMigration();
+    console.log("All backup migration regression tests passed successfully!");
 } catch (error) {
     console.error("Test failed:", error);
     process.exit(1);

server/setup/migrationsSqlite.ts

@@ -5,7 +5,7 @@ import path from "path";
 import semver from "semver";
 import { versionMigrations } from "../db/sqlite";
 import { __DIRNAME, APP_PATH, APP_VERSION } from "@server/lib/consts";
-import { formatBackupTimestamp } from "@server/lib/backupFileName";
+import { formatBackupFileName } from "@server/lib/backupFileName";
 import { SqliteError } from "better-sqlite3";
 import fs from "fs";
 import { build } from "@server/build";
@@ -107,7 +107,7 @@ async function run() {
     await runMigrations();
 }
 
-function backupDb() {
+function backupDb(version?: string) {
     // make dir config/db/backups
     const appPath = APP_PATH;
     const dbDir = path.join(appPath, "db");
@@ -120,11 +120,10 @@ function backupDb() {
     }
 
     // copy the db.sqlite file to backups
-    // add the date to the filename
-    const date = new Date();
-    const dateString = formatBackupTimestamp(date);
+    // add the date and migration version to the filename
+    const fileName = formatBackupFileName(version);
     const dbPath = path.join(dbDir, "db.sqlite");
-    const backupPath = path.join(backupsDir, `db_${dateString}.sqlite`);
+    const backupPath = path.join(backupsDir, fileName);
     fs.copyFileSync(dbPath, backupPath);
 }
 
@@ -163,6 +162,12 @@ export async function runMigrations() {
         }
     } catch (e) {
         console.error("Error running migrations:", e);
+        if (
+            process.env.NODE_ENV === "test" ||
+            process.env.ENVIRONMENT === "test"
+        ) {
+            throw e;
+        }
         await new Promise((resolve) =>
             setTimeout(resolve, 1000 * 60 * 60 * 24 * 1)
         );
@@ -191,18 +196,15 @@ async function executeScripts() {
         );
 
         // Run migrations in order
-        // Take a single backup before any migration runs, so one upgrade
-        // produces one restore point instead of one backup per migration.
-        if (
-            migrationsToRun.length > 0 &&
-            !process.env.DISABLE_BACKUP_ON_MIGRATION
-        ) {
-            backupDb();
-        }
         for (const migration of migrationsToRun) {
             console.log(`Running migration ${migration.version}`);
 
             try {
+                if (!process.env.DISABLE_BACKUP_ON_MIGRATION) {
+                    // Backup the database before running the migration
+                    backupDb(migration.version);
+                }
+
                 await migration.run();
 
                 // Update version in database

server/setup/scriptsSqlite/1.0.0-beta9.ts

@@ -10,7 +10,6 @@ import {
     users
 } from "../../db/sqlite";
 import { APP_PATH, configFilePath1, configFilePath2 } from "@server/lib/consts";
-import { formatBackupTimestamp } from "@server/lib/backupFileName";
 import { eq, sql } from "drizzle-orm";
 import fs from "fs";
 import * as yaml from "js-yaml";
@@ -21,25 +20,6 @@ import { fromZodError } from "zod-validation-error";
 export default async function migration() {
     console.log("Running setup script 1.0.0-beta.9...");
 
-    // make dir config/db/backups
-    const appPath = APP_PATH;
-    const dbDir = path.join(appPath, "db");
-
-    const backupsDir = path.join(dbDir, "backups");
-
-    // check if the backups directory exists and create it if it doesn't
-    if (!fs.existsSync(backupsDir)) {
-        fs.mkdirSync(backupsDir, { recursive: true });
-    }
-
-    // copy the db.sqlite file to backups
-    // add the date to the filename
-    const date = new Date();
-    const dateString = formatBackupTimestamp(date);
-    const dbPath = path.join(dbDir, "db.sqlite");
-    const backupPath = path.join(backupsDir, `db_${dateString}.sqlite`);
-    fs.copyFileSync(dbPath, backupPath);
-
     await db.transaction(async (trx) => {
         try {
             // Determine which config file exists