diff --git a/backend/__tests__/integration/restoreService.pgBranch.test.js b/backend/__tests__/integration/restoreService.pgBranch.test.js index e91ab75d..b8943b8a 100644 --- a/backend/__tests__/integration/restoreService.pgBranch.test.js +++ b/backend/__tests__/integration/restoreService.pgBranch.test.js @@ -70,43 +70,40 @@ describe('restoreService — PG branch scope contract (PR #596 review)', () => { return last; } - it('declares preservedMeta above the SQLite/PG branch split in performDatabaseRestore', () => { - // The function spans from `async performDatabaseRestore(` to the - // matching `}`. We don't need the closing brace — just need to - // verify the order of three landmarks: + it('preservedMetaSnapshot lives on `this` and is initialised in the constructor', () => { + // PR #596 round 3 moved the snapshot from a block-scoped local to + // an instance variable so the replay can happen in `restore()` + // AFTER post-restore verification — preventing the replay row + // from inflating the row-count check. // - // 1. `async performDatabaseRestore(` opens the function - // 2. `let preservedMeta = []` (the declaration) must come - // BEFORE... - // 3. `if (this.dbType === 'sqlite')` (the branch split) - const functionStart = findFirst(/async\s+performDatabaseRestore\s*\(/); - expect(functionStart).toBeGreaterThan(0); - - const declarations = lines - .map((l, i) => ({ line: i + 1, text: l })) - .filter(({ text }) => /let\s+preservedMeta\s*=\s*\[\s*\]/.test(text)); - - // The fix removed the in-branch duplicate, so there should be - // EXACTLY ONE declaration of `let preservedMeta` in the file. - // If a reviewer accidentally re-introduces the block-scoped - // duplicate, this catches it. - expect(declarations).toHaveLength(1); - - const declarationLine = declarations[0].line; - expect(declarationLine).toBeGreaterThan(functionStart); - - // The SQLite/PG split is the first `if (this.dbType === 'sqlite')` - // after the function opener. - const splitLine = lines.findIndex((l, i) => - i + 1 > functionStart && /if\s*\(\s*this\.dbType\s*===\s*['"]sqlite['"]\s*\)/.test(l) + // Contract: + // 1. The constructor initialises `this.preservedMetaSnapshot = []` + // 2. The `restore()` entry point resets it per call (no leak + // across consecutive runs in the singleton service instance) + // 3. `performDatabaseRestore` assigns to `this.preservedMetaSnapshot` + // inside the PG branch (must run before DROP) + // 4. The replay reads `this.preservedMetaSnapshot` — NOT a bare + // `preservedMeta` local — so a future refactor can't + // accidentally drop the snapshot half on the floor again. + const constructorInit = lines.some((l) => + /this\.preservedMetaSnapshot\s*=\s*\[\s*\]/.test(l) ); - expect(splitLine).toBeGreaterThan(-1); - const splitLineOneBased = splitLine + 1; + expect(constructorInit).toBe(true); - // The actual contract: declaration line MUST come before the - // split line. If a future edit puts the declaration inside the - // else block again, this assertion fails with a clear message. - expect(declarationLine).toBeLessThan(splitLineOneBased); + const assignmentSites = lines.filter((l) => + /this\.preservedMetaSnapshot\s*=\s*(\[\s*\]|await\s+db)/.test(l) + ); + // Constructor init + restore() per-run reset + the PG-branch + // assignment from db query. Three writes. + expect(assignmentSites.length).toBeGreaterThanOrEqual(3); + + // No stray bare `preservedMeta` local-scoped declaration in + // performDatabaseRestore — would indicate someone re-introduced + // the round-1 footgun. + const dangerousLocalDecl = lines.filter((l) => + /^\s*(let|const)\s+preservedMeta\s*=/.test(l) + ); + expect(dangerousLocalDecl).toEqual([]); }); it('every .count() result is coerced to Number before comparison', () => { @@ -151,59 +148,77 @@ describe('restoreService — PG branch scope contract (PR #596 review)', () => { expect(dangerousLines).toEqual([]); }); - it('the replay block reads preservedMeta outside the SQLite/PG branch', () => { - // The replay block lives near the end of performDatabaseRestore. - // It must NOT be guarded by `this.dbType === 'postgresql'` — the - // intent of hoisting the declaration is that SQLite ALSO runs - // through the replay block (it just no-ops because the snapshot - // wasn't taken on the SQLite branch). The test catches a regression - // where a refactor moves the replay back inside the PG branch. - const replayLine = findLast(/if\s*\(\s*preservedMeta\.length\s*>\s*0\s*\)/); + it('npm run migrate:safe is invoked after the replay in restore()', () => { + // Contract from PR #596 round 4: backups taken on older picpeak + // versions must restore COMPLETELY on a newer image — even if new + // migrations have been added since the backup was taken. The + // restore() flow shells out to `npm run migrate:safe` AFTER the + // operator-meta replay so the schema catches up to the running + // code WITHIN the restore boundary (not on the next container + // restart). + // + // Contract: + // 1. A `migrate:safe` shell-out exists somewhere in restoreService + // 2. It sits AFTER the replay drain — verification → replay → + // migrations is the documented order + // 3. It does NOT sit inside performDatabaseRestore (must run + // against the reinit'd pool from the parent restore()) + const migrateLine = findFirst(/['"]migrate:safe['"]/); + expect(migrateLine).toBeGreaterThan(0); + + const replayLine = findLast(/this\.preservedMetaSnapshot\.length\s*>\s*0/); expect(replayLine).toBeGreaterThan(0); + expect(migrateLine).toBeGreaterThan(replayLine); - // Walk backwards from the replay line and look for the nearest - // `} else {` opener. If the nearest is the PG `else`, it would - // mean we're inside that branch. If it's null OR points at a - // different else (one further out), we're at the right scope. - let nearestElseLine = -1; - for (let i = replayLine - 2; i >= 0; i--) { - if (/^\s*}\s*else\s*\{\s*$/.test(lines[i]) || /^\s*else\s*\{\s*$/.test(lines[i])) { - nearestElseLine = i + 1; - break; - } - if (/^\s*\}\s*$/.test(lines[i]) && i > 0) { - // Closing brace before an else opener — keep walking - } - } - - // The nearest `else {` opener BACKWARDS from the replay site - // should be either nothing (replay sits at function scope) or - // an else from a *different* outer construct. Either way, the - // replay must NOT be lexically inside the dbType === 'sqlite' - // / else split. We assert this by checking that the SQLite/PG - // split line is BEFORE the replay, AND that there's a closing - // brace `}` between them at column-0 indentation depth that - // matches the split's depth. - const sqliteSplitLine = lines.findIndex((l) => - /if\s*\(\s*this\.dbType\s*===\s*['"]sqlite['"]\s*\)/.test(l) - ) + 1; - - expect(sqliteSplitLine).toBeGreaterThan(0); - expect(replayLine).toBeGreaterThan(sqliteSplitLine); - - // Between the split and the replay, there should be a line that - // closes the else block. We look for ` }` (six-space indent - // matching the else opener's depth) before the replay site. - let foundElseClose = false; - for (let i = sqliteSplitLine; i < replayLine; i++) { - // The else block closes with ` }` at the same indent as - // the `} else {` opener. Look for any line that's exactly - // six-space indent + `}` to find the closing brace. - if (/^ \}\s*$/.test(lines[i])) { - foundElseClose = true; + // Must NOT live inside performDatabaseRestore (same scope as the + // replay check above). + const dbRestoreStart = findFirst(/async\s+performDatabaseRestore\s*\(/); + let dbRestoreEnd = -1; + for (let i = dbRestoreStart; i < lines.length; i++) { + if (/^ \}\s*$/.test(lines[i])) { + dbRestoreEnd = i + 1; break; } } - expect(foundElseClose).toBe(true); + expect(migrateLine < dbRestoreStart || migrateLine > dbRestoreEnd).toBe(true); + }); + + it('the replay site lives in restore() AFTER performPostRestoreVerification', () => { + // PR #596 round 3 moved the replay out of performDatabaseRestore + // and into the parent restore() method, sequenced AFTER the + // post-restore verification. Otherwise the replay's upserted row + // count was being flagged as a verification mismatch (e.g. + // "expected 190, got 191" because the fresh-install seeded + // `restore_allow_force_auto_upgraded` that wasn't in the backup). + // + // Contract: the line that drains `this.preservedMetaSnapshot` + // must come AFTER `performPostRestoreVerification` AND must NOT + // sit inside `performDatabaseRestore`. + const verificationLine = findFirst(/performPostRestoreVerification\s*\(/); + expect(verificationLine).toBeGreaterThan(0); + + const replayLine = findLast(/this\.preservedMetaSnapshot\.length\s*>\s*0/); + expect(replayLine).toBeGreaterThan(0); + expect(replayLine).toBeGreaterThan(verificationLine); + + // `performDatabaseRestore` must not contain the replay drain. + // Find the function bounds + assert no drain line falls inside. + const dbRestoreStart = findFirst(/async\s+performDatabaseRestore\s*\(/); + expect(dbRestoreStart).toBeGreaterThan(0); + + // Find the closing brace of performDatabaseRestore. Lazy heuristic: + // the first `^ \}\s*$` (two-space indent + }) after the function + // start. Brittle to indent changes but unambiguous in this codebase. + let dbRestoreEnd = -1; + for (let i = dbRestoreStart; i < lines.length; i++) { + if (/^ \}\s*$/.test(lines[i])) { + dbRestoreEnd = i + 1; + break; + } + } + expect(dbRestoreEnd).toBeGreaterThan(dbRestoreStart); + + // The replay drain line must be OUTSIDE [dbRestoreStart, dbRestoreEnd]. + expect(replayLine < dbRestoreStart || replayLine > dbRestoreEnd).toBe(true); }); }); diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index 8c87b6f2..16a55f40 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -39,6 +39,12 @@ class RestoreService { this.currentProgress = null; this.restoreLog = []; this.preRestoreBackupPath = null; + // Snapshot of operator-meta settings (e.g. `restore_allow_force`) + // captured by performDatabaseRestore BEFORE the DROP DATABASE. + // Drained by restore() AFTER post-restore verification passes so + // the replay doesn't inflate the row-count check. Reset per run + // via beforeRestore() to keep state from leaking across calls. + this.preservedMetaSnapshot = []; this.dbType = knexConfig.client === 'pg' ? 'postgresql' : 'sqlite'; this.tempDir = path.join(os.tmpdir(), 'picpeak-restore'); } @@ -64,6 +70,7 @@ class RestoreService { this.isRunning = true; this.restoreLog = []; + this.preservedMetaSnapshot = []; // reset per run const startTime = new Date(); let restoreRun = null; @@ -221,6 +228,89 @@ class RestoreService { throw new Error(`Post-restore verification failed: ${verification.errors.join(', ')}`); } + // Step 7b: Replay operator-meta settings AFTER verification. + // + // `performDatabaseRestore` stashed the pre-DROP snapshot of + // operator-meta keys on `this.preservedMetaSnapshot`. We drain + // it here, AFTER verification has already confirmed the + // restored DB matches the backup's row counts. Running this + // upsert sequence here instead of inside performDatabaseRestore + // (where it used to live) prevents the replay from inflating + // the post-restore row count and tripping the verification + // check — see the round-3 PR #596 notes for the full story. + // + // UPSERT by setting_key: if the backup had the same key with a + // different value, we overwrite; if the row doesn't exist in + // the backup, we insert. Either way the operator's pre-restore + // policy survives. SQLite branch leaves the snapshot empty so + // this block is a no-op there. + if (this.preservedMetaSnapshot && this.preservedMetaSnapshot.length > 0) { + try { + for (const row of this.preservedMetaSnapshot) { + await db('app_settings') + .insert({ + setting_key: row.setting_key, + setting_value: row.setting_value, + setting_type: row.setting_type || 'restore', + updated_at: new Date(), + }) + .onConflict('setting_key') + .merge({ + setting_value: row.setting_value, + updated_at: new Date(), + }); + } + this.log('info', `Replayed ${this.preservedMetaSnapshot.length} restore-meta setting(s) post-verification`); + } catch (err) { + this.log('warn', `Could not replay restore-meta settings (admin may need to re-set them): ${err.message}`); + } + } + + // Step 7c: Apply any post-backup migrations to the restored DB. + // + // The backup carries the schema state of whatever migrations had + // been applied at backup time. If the running image is NEWER — + // because the admin upgraded picpeak between when the backup + // was taken and when they restored — the restored DB ends up + // mismatched against the running code: queries fail, new + // columns are missing, new tables don't exist. + // + // Previously the comment said "deferred to next container + // restart" — but that left the running process serving a + // mismatched schema until the operator manually restarted. + // Not acceptable per the "backup must restore completely even + // when new features have been added in the meantime" contract. + // + // Implementation: shell out to `npm run migrate:safe`, which is + // the EXACT script wait-for-db.sh runs on boot. Running it as a + // subprocess means no risk to our reinit'd pool (subprocess gets + // its own knex instance, destroys it on exit; our parent pool + // is untouched). Idempotent — migrations already applied are + // tracked in the restored `migrations` table and get skipped. + // + // Failure is non-fatal: the restore data itself is in place, + // and the next container restart's wait-for-db.sh will retry. + // Surfacing the error gives the operator a chance to investigate + // proactively rather than discovering it on the next 500 from + // a missing column. + try { + this.log('info', 'Applying post-restore migrations to restored database...'); + this.updateProgress('Applying any post-backup migrations...'); + const backendRoot = path.join(__dirname, '..', '..'); + const { stderr } = await spawnAsync('npm', ['run', 'migrate:safe'], { + cwd: backendRoot, + env: { ...process.env }, + }); + if (stderr && stderr.trim()) { + this.log('info', `Post-restore migrate:safe stderr: ${stderr.slice(0, 500)}`); + } + this.log('info', 'Post-restore migrations applied'); + } catch (migErr) { + this.log('warn', + `Post-restore migrate:safe failed — restore data is in place but the schema may lag the running image. ` + + `A container restart will retry via wait-for-db.sh. Error: ${migErr.message}`); + } + // Step 8: Clean up temporary files if (localBackupPath !== options.source) { await fs.unlink(localBackupPath).catch(err => @@ -803,25 +893,30 @@ class RestoreService { restoreFile = decompressedPath; } - // Hoisted above the SQLite/PG split so the post-restore replay - // block at the bottom (~L1030) can read them even when execution - // takes the SQLite path. Without this hoist, the PG branch - // populated `preservedMeta` in a block-scoped `let` and then the - // shared replay code below tried to read the same name, throwing - // `ReferenceError: preservedMeta is not defined` — which caused - // every PG restore to "succeed at the data layer" while emitting - // a loud FAILED line, skipping the trigger cleanup in - // _installFromBackupBoot.js, and silently dropping the - // operator-meta replay that was the whole reason this snapshot - // existed. The maintainer caught this on PR #596 review. - // SQLite branch leaves these as the empty defaults — the - // replay block at the bottom is a no-op when `preservedMeta` is - // empty, so behaviour is unchanged for SQLite. + // Snapshot of operator-meta keys captured BEFORE the DROP. + // Stashed onto `this.preservedMetaSnapshot` so the parent + // `restore()` method can drain + apply it AFTER post-restore + // verification passes. Order matters here: + // + // - PR #596 round 1: lifted the declaration above the + // SQLite/PG split to fix a ReferenceError when the replay + // was inline at the bottom of this method. + // - PR #596 round 3: moved the REPLAY itself out of here and + // into restore(), because the round-1 in-method replay ran + // BEFORE post-restore verification — which then counted the + // replayed row and flagged + // Table app_settings row count mismatch: expected 190, got 191 + // as a verification failure even though both Stage A and + // the replay had succeeded. Verification now sees the + // as-restored DB (matches the backup exactly), replay layers + // on top after verification has signed off. + // + // SQLite branch leaves `preservedMetaSnapshot` empty — verification + // and replay both no-op for it, unchanged behaviour. const PRESERVED_META_KEYS = [ 'restore_allow_force', 'restore_allow_force_auto_upgraded', ]; - let preservedMeta = []; try { if (this.dbType === 'sqlite') { @@ -875,16 +970,18 @@ class RestoreService { // needed the SQL workaround again. With this snapshot/replay, // the operator's policy persists across restores. // - // PRESERVED_META_KEYS + `preservedMeta` are declared above the - // SQLite/PG split (~L795) so the replay block at the bottom - // can read them on both branches. Only the snapshot READ - // needs to happen here in the PG branch (must run before DROP). + // PRESERVED_META_KEYS is declared above the SQLite/PG split + // (~L820). The snapshot READ happens here in the PG branch + // (must run before DROP), but is stashed on + // `this.preservedMetaSnapshot` for the parent `restore()` + // method to consume AFTER verification — see the round-3 + // notes there. try { - preservedMeta = await db('app_settings') + this.preservedMetaSnapshot = await db('app_settings') .whereIn('setting_key', PRESERVED_META_KEYS) .select('setting_key', 'setting_value', 'setting_type'); - this.log('info', `Snapshotted ${preservedMeta.length} restore-meta setting(s) for post-restore replay`, { - keys: preservedMeta.map(r => r.setting_key), + this.log('info', `Snapshotted ${this.preservedMetaSnapshot.length} restore-meta setting(s) for post-restore replay`, { + keys: this.preservedMetaSnapshot.map(r => r.setting_key), }); } catch (err) { this.log('warn', `Could not snapshot restore-meta settings (continuing): ${err.message}`); @@ -1043,42 +1140,24 @@ END $$;` // uses `npm run migrate:safe` (run-migrations-safe.js) which // knows to skip helpers.js + walks core/ explicitly. // - // For restore: the dump we just loaded already contains the - // schema state of whatever migrations had been applied at - // backup time. If the running image has NEWER migrations that - // need to run on top of the restored DB, those will be applied - // on the NEXT container start by wait-for-db.sh + the safe - // runner. That's a one-restart penalty in the unusual case of - // restoring from a backup older than the current image, and - // matches what picpeak does on every other boot already. - this.log('info', 'Skipping in-process migrate (deferred to next boot via safe runner)'); + // The safe runner gets invoked AFTER verification in restore() + // (see step 7c) to apply any post-backup migrations to the + // restored DB. This closes the contract "backup must restore + // completely even when new features have been added in the + // meantime" — without this step, restoring an old backup on a + // newer image would leave the running process serving a + // mismatched schema until the next container restart. + this.log('info', 'Schema migrations deferred to restore() step 7c (npm run migrate:safe subprocess)'); - // Replay the snapshotted operator-meta settings on top of the - // restored DB. UPSERT by setting_key — if the backup had the - // same key with a different value, we overwrite it; if the row - // doesn't exist in the backup, we insert it. Either way the - // operator's pre-restore policy survives. - if (preservedMeta.length > 0) { - try { - for (const row of preservedMeta) { - await db('app_settings') - .insert({ - setting_key: row.setting_key, - setting_value: row.setting_value, - setting_type: row.setting_type || 'restore', - updated_at: new Date(), - }) - .onConflict('setting_key') - .merge({ - setting_value: row.setting_value, - updated_at: new Date(), - }); - } - this.log('info', `Replayed ${preservedMeta.length} restore-meta setting(s) post-restore`); - } catch (err) { - this.log('warn', `Could not replay restore-meta settings (admin may need to re-set them): ${err.message}`); - } - } + // NOTE: operator-meta REPLAY does NOT happen here any more. + // PR #596 round 3: if the replay runs inside performDatabaseRestore, + // it lands BEFORE post-restore verification — and verification + // then counts the replayed row as a mismatch (e.g. "expected 190, + // got 191" because the fresh-install seeded + // `restore_allow_force_auto_upgraded` that wasn't in the backup). + // Replay is now drained by the parent `restore()` method AFTER + // verification passes. Snapshot lives on + // `this.preservedMetaSnapshot` for that drain. return { success: true }; diff --git a/docs/backup-restore.md b/docs/backup-restore.md index ed54207d..f659f1b1 100644 --- a/docs/backup-restore.md +++ b/docs/backup-restore.md @@ -120,6 +120,14 @@ Use this when picpeak is running and you want to roll back to a specific backup Failures during restore trigger an automatic rollback from the pre-restore safety snapshot. The destination ends up either as the restored state OR as the original pre-restore state — never as a half-clobbered mix. +### Restoring an older backup on a newer image + +picpeak's restore path is forward-compatible: a backup taken on an older version restores cleanly onto a newer image without any manual schema work. After loading the dump, the restore service runs the same `npm run migrate:safe` script that `wait-for-db.sh` uses on every container boot. Any migrations that have been added between the backup's snapshot and the current image are applied inline, against the freshly-restored DB, before the restore is reported as complete. + +Net effect: even if `bugfix/cool-new-feature` shipped a migration that adds a `widgets` table and your backup predates that branch, after restore your install has the `widgets` table (empty), the right indexes, and any seed rows the migration emits. No "you'll need to restart the container once" footnote. + +The same applies to the install-from-backup trigger — migrations land inside the restore boundary, so the moment the server prints `Server running on port 3000`, the schema matches the running image. Log in and use the install immediately. + ## Disaster recovery (install from a backup) For full DR after `docker compose down -v`, host migration, drive replacement, or moving an install between hosts. picpeak detects a trigger file on first boot and runs the restore before the admin UI surfaces. You open the browser, log in with your original credentials, and the install is fully populated.