From b74ded3a9c823fc9aa33264be595a6ea7e7f37af Mon Sep 17 00:00:00 2001 From: Tyagiquamar Date: Fri, 18 Sep 2026 21:31:00 +0530 Subject: [PATCH] fix(sqlite): tag per-migration database backups with version and prevent collisions --- server/lib/backupFileName.test.ts | 47 +++- server/lib/backupFileName.ts | 23 ++ server/setup/migrationsSqlite.backup.test.ts | 243 +++++++++++++++++-- server/setup/migrationsSqlite.ts | 30 +-- server/setup/scriptsSqlite/1.0.0-beta9.ts | 20 -- 5 files changed, 298 insertions(+), 65 deletions(-) diff --git a/server/lib/backupFileName.test.ts b/server/lib/backupFileName.test.ts index 01fc4796a..fd8a047eb 100644 --- a/server/lib/backupFileName.test.ts +++ b/server/lib/backupFileName.test.ts @@ -1,4 +1,4 @@ -import { formatBackupTimestamp } from "./backupFileName"; +import { formatBackupFileName, formatBackupTimestamp } from "./backupFileName"; import { assertEquals } from "@test/assert"; // Local-time constructors are used throughout, matching formatBackupTimestamp, @@ -29,7 +29,9 @@ function testMonthIsOneIndexed() { } { - const result = formatBackupTimestamp(new Date(2026, 11, 31, 23, 59, 59)); + const result = formatBackupTimestamp( + new Date(2026, 11, 31, 23, 59, 59) + ); assertEquals( result, "2026-12-31_23-59-59", @@ -73,9 +75,7 @@ function testNamesSortChronologically() { new Date(2026, 11, 31, 23, 59, 59) ]; - const sorted = taken - .map((date) => formatBackupTimestamp(date)) - .sort(); + const sorted = taken.map((date) => formatBackupTimestamp(date)).sort(); assertEquals( sorted.join(","), @@ -89,11 +89,48 @@ function testNamesSortChronologically() { ); } +function testFormatBackupFileName() { + console.log("Running backup file name formatting tests..."); + + const date = new Date(2026, 8, 12, 20, 35, 56); + + // With semver version string without leading 'v' + assertEquals( + formatBackupFileName("1.22.0", date), + "db_2026-09-12_20-35-56_v1.22.0.sqlite", + "Filename must include timestamp and prefixed version tag" + ); + + // With version string already containing 'v' + assertEquals( + formatBackupFileName("v1.22.0", date), + "db_2026-09-12_20-35-56_v1.22.0.sqlite", + "Filename must not duplicate 'v' prefix if already present" + ); + + // Without version (fallback/default) + assertEquals( + formatBackupFileName(undefined, date), + "db_2026-09-12_20-35-56.sqlite", + "Filename without version must match default timestamped format" + ); + + // Distinct versions within the exact same second do not collide + const sameSecondFile1 = formatBackupFileName("1.21.0", date); + const sameSecondFile2 = formatBackupFileName("1.22.0", date); + if (sameSecondFile1 === sameSecondFile2) { + throw new Error( + "Backup file names for different versions in the same second must not collide" + ); + } +} + // Run all tests try { testMonthIsOneIndexed(); testEveryFieldIsZeroPadded(); testNamesSortChronologically(); + testFormatBackupFileName(); console.log("All tests passed successfully!"); } catch (error) { console.error("Test failed:", error); diff --git a/server/lib/backupFileName.ts b/server/lib/backupFileName.ts index a5888450a..9d2c1bd62 100644 --- a/server/lib/backupFileName.ts +++ b/server/lib/backupFileName.ts @@ -26,3 +26,26 @@ export function formatBackupTimestamp(date: Date = new Date()): string { return `${datePart}_${timePart}`; } + +/** + * Builds the full database backup file name, including timestamp and optional version tag. + * + * When a migration version is provided, the filename includes `_v`, + * preventing collisions between multiple migrations running in the same second and making it easy + * to identify the migration state contained in the backup. + * + * @param version Optional migration version being run. + * @param date The moment the backup is being taken. Defaults to now. + * @returns A filename of the form `db_YYYY-MM-DD_HH-MM-SS_v.sqlite` or `db_YYYY-MM-DD_HH-MM-SS.sqlite`. + */ +export function formatBackupFileName( + version?: string, + date: Date = new Date() +): string { + const timestamp = formatBackupTimestamp(date); + if (version) { + const versionTag = version.startsWith("v") ? version : `v${version}`; + return `db_${timestamp}_${versionTag}.sqlite`; + } + return `db_${timestamp}.sqlite`; +} diff --git a/server/setup/migrationsSqlite.backup.test.ts b/server/setup/migrationsSqlite.backup.test.ts index e450155ff..616cf303d 100644 --- a/server/setup/migrationsSqlite.backup.test.ts +++ b/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 = {} +): { 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); diff --git a/server/setup/migrationsSqlite.ts b/server/setup/migrationsSqlite.ts index 07b38e52b..eb2846610 100644 --- a/server/setup/migrationsSqlite.ts +++ b/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 diff --git a/server/setup/scriptsSqlite/1.0.0-beta9.ts b/server/setup/scriptsSqlite/1.0.0-beta9.ts index 6766c0756..2494556ce 100644 --- a/server/setup/scriptsSqlite/1.0.0-beta9.ts +++ b/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