fix(restore): move operator-meta replay after post-restore verification (PR #596 round 3)

End-to-end DR cycle surfaced one more PG-only landmine — and it
turned out to be a side-effect of the round-1 replay placement, not
a new bug. Round 2 fixed the comparison logic; round 3 fixes the
ordering.

Symptom on real PG install:

  [install-from-backup] FAILED — Post-restore verification failed:
    Table app_settings row count mismatch: expected 190, got 191.
    Trigger file left in place for retry.

Root cause: the operator-meta replay (introduced in round 1) ran
INSIDE performDatabaseRestore, lined up BEFORE the post-restore
verification step in the parent restore() method. So:

  1. psql restores app_settings → 190 rows (matches backup)
  2. Replay upserts `restore_allow_force_auto_upgraded` (which the
     fresh-install seeded but the backup didn't have) → 191 rows
  3. performPostRestoreVerification counts 191, manifest says 190,
     verification fails the row-count check.

Replay is doing the right thing (preserving operator policy). The
verification is doing the right thing (counts must match). They
disagree because the replay landed in the wrong sequence relative
to verification.

Cure: move the replay out of performDatabaseRestore and into
restore() AFTER `performPostRestoreVerification` passes.
Verification now sees the as-restored DB (matches the backup
exactly), replay layers on top once verification has signed off.

Mechanism: snapshot stashed on `this.preservedMetaSnapshot`
(initialised in constructor, reset per run at the top of restore()).
performDatabaseRestore writes it in the PG branch before DROP;
restore() drains it after verification. SQLite leaves it empty,
both steps no-op there.

Tests:
  - Updated `restoreService.pgBranch.test.js` to pin the new shape:
    * `this.preservedMetaSnapshot` is initialised in the constructor
    * No stray `let preservedMeta = []` local declarations anywhere
    * Replay drain (`this.preservedMetaSnapshot.length > 0`) sits in
      restore() AFTER `performPostRestoreVerification(...)` and is
      lexically OUTSIDE `performDatabaseRestore`.
  - The bigint-as-string contract from round 2 still holds.

34/34 backup-related integration tests pass.
This commit is contained in:
Luca
2026-06-01 22:44:51 +02:00
parent 354fbed182
commit 20e3092c14
3 changed files with 242 additions and 140 deletions
@@ -70,43 +70,40 @@ describe('restoreService — PG branch scope contract (PR #596 review)', () => {
return last; return last;
} }
it('declares preservedMeta above the SQLite/PG branch split in performDatabaseRestore', () => { it('preservedMetaSnapshot lives on `this` and is initialised in the constructor', () => {
// The function spans from `async performDatabaseRestore(` to the // PR #596 round 3 moved the snapshot from a block-scoped local to
// matching `}`. We don't need the closing brace — just need to // an instance variable so the replay can happen in `restore()`
// verify the order of three landmarks: // AFTER post-restore verification — preventing the replay row
// from inflating the row-count check.
// //
// 1. `async performDatabaseRestore(` opens the function // Contract:
// 2. `let preservedMeta = []` (the declaration) must come // 1. The constructor initialises `this.preservedMetaSnapshot = []`
// BEFORE... // 2. The `restore()` entry point resets it per call (no leak
// 3. `if (this.dbType === 'sqlite')` (the branch split) // across consecutive runs in the singleton service instance)
const functionStart = findFirst(/async\s+performDatabaseRestore\s*\(/); // 3. `performDatabaseRestore` assigns to `this.preservedMetaSnapshot`
expect(functionStart).toBeGreaterThan(0); // inside the PG branch (must run before DROP)
// 4. The replay reads `this.preservedMetaSnapshot` — NOT a bare
const declarations = lines // `preservedMeta` local — so a future refactor can't
.map((l, i) => ({ line: i + 1, text: l })) // accidentally drop the snapshot half on the floor again.
.filter(({ text }) => /let\s+preservedMeta\s*=\s*\[\s*\]/.test(text)); const constructorInit = lines.some((l) =>
/this\.preservedMetaSnapshot\s*=\s*\[\s*\]/.test(l)
// 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)
); );
expect(splitLine).toBeGreaterThan(-1); expect(constructorInit).toBe(true);
const splitLineOneBased = splitLine + 1;
// The actual contract: declaration line MUST come before the const assignmentSites = lines.filter((l) =>
// split line. If a future edit puts the declaration inside the /this\.preservedMetaSnapshot\s*=\s*(\[\s*\]|await\s+db)/.test(l)
// else block again, this assertion fails with a clear message. );
expect(declarationLine).toBeLessThan(splitLineOneBased); // 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', () => { 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([]); expect(dangerousLines).toEqual([]);
}); });
it('the replay block reads preservedMeta outside the SQLite/PG branch', () => { it('npm run migrate:safe is invoked after the replay in restore()', () => {
// The replay block lives near the end of performDatabaseRestore. // Contract from PR #596 round 4: backups taken on older picpeak
// It must NOT be guarded by `this.dbType === 'postgresql'` — the // versions must restore COMPLETELY on a newer image — even if new
// intent of hoisting the declaration is that SQLite ALSO runs // migrations have been added since the backup was taken. The
// through the replay block (it just no-ops because the snapshot // restore() flow shells out to `npm run migrate:safe` AFTER the
// wasn't taken on the SQLite branch). The test catches a regression // operator-meta replay so the schema catches up to the running
// where a refactor moves the replay back inside the PG branch. // code WITHIN the restore boundary (not on the next container
const replayLine = findLast(/if\s*\(\s*preservedMeta\.length\s*>\s*0\s*\)/); // 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(replayLine).toBeGreaterThan(0);
expect(migrateLine).toBeGreaterThan(replayLine);
// Walk backwards from the replay line and look for the nearest // Must NOT live inside performDatabaseRestore (same scope as the
// `} else {` opener. If the nearest is the PG `else`, it would // replay check above).
// mean we're inside that branch. If it's null OR points at a const dbRestoreStart = findFirst(/async\s+performDatabaseRestore\s*\(/);
// different else (one further out), we're at the right scope. let dbRestoreEnd = -1;
let nearestElseLine = -1; for (let i = dbRestoreStart; i < lines.length; i++) {
for (let i = replayLine - 2; i >= 0; i--) { if (/^ \}\s*$/.test(lines[i])) {
if (/^\s*}\s*else\s*\{\s*$/.test(lines[i]) || /^\s*else\s*\{\s*$/.test(lines[i])) { dbRestoreEnd = i + 1;
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;
break; 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);
}); });
}); });
+136 -57
View File
@@ -39,6 +39,12 @@ class RestoreService {
this.currentProgress = null; this.currentProgress = null;
this.restoreLog = []; this.restoreLog = [];
this.preRestoreBackupPath = null; 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.dbType = knexConfig.client === 'pg' ? 'postgresql' : 'sqlite';
this.tempDir = path.join(os.tmpdir(), 'picpeak-restore'); this.tempDir = path.join(os.tmpdir(), 'picpeak-restore');
} }
@@ -64,6 +70,7 @@ class RestoreService {
this.isRunning = true; this.isRunning = true;
this.restoreLog = []; this.restoreLog = [];
this.preservedMetaSnapshot = []; // reset per run
const startTime = new Date(); const startTime = new Date();
let restoreRun = null; let restoreRun = null;
@@ -221,6 +228,89 @@ class RestoreService {
throw new Error(`Post-restore verification failed: ${verification.errors.join(', ')}`); 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 // Step 8: Clean up temporary files
if (localBackupPath !== options.source) { if (localBackupPath !== options.source) {
await fs.unlink(localBackupPath).catch(err => await fs.unlink(localBackupPath).catch(err =>
@@ -803,25 +893,30 @@ class RestoreService {
restoreFile = decompressedPath; restoreFile = decompressedPath;
} }
// Hoisted above the SQLite/PG split so the post-restore replay // Snapshot of operator-meta keys captured BEFORE the DROP.
// block at the bottom (~L1030) can read them even when execution // Stashed onto `this.preservedMetaSnapshot` so the parent
// takes the SQLite path. Without this hoist, the PG branch // `restore()` method can drain + apply it AFTER post-restore
// populated `preservedMeta` in a block-scoped `let` and then the // verification passes. Order matters here:
// shared replay code below tried to read the same name, throwing //
// `ReferenceError: preservedMeta is not defined` — which caused // - PR #596 round 1: lifted the declaration above the
// every PG restore to "succeed at the data layer" while emitting // SQLite/PG split to fix a ReferenceError when the replay
// a loud FAILED line, skipping the trigger cleanup in // was inline at the bottom of this method.
// _installFromBackupBoot.js, and silently dropping the // - PR #596 round 3: moved the REPLAY itself out of here and
// operator-meta replay that was the whole reason this snapshot // into restore(), because the round-1 in-method replay ran
// existed. The maintainer caught this on PR #596 review. // BEFORE post-restore verification — which then counted the
// SQLite branch leaves these as the empty defaults — the // replayed row and flagged
// replay block at the bottom is a no-op when `preservedMeta` is // Table app_settings row count mismatch: expected 190, got 191
// empty, so behaviour is unchanged for SQLite. // 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 = [ const PRESERVED_META_KEYS = [
'restore_allow_force', 'restore_allow_force',
'restore_allow_force_auto_upgraded', 'restore_allow_force_auto_upgraded',
]; ];
let preservedMeta = [];
try { try {
if (this.dbType === 'sqlite') { if (this.dbType === 'sqlite') {
@@ -875,16 +970,18 @@ class RestoreService {
// needed the SQL workaround again. With this snapshot/replay, // needed the SQL workaround again. With this snapshot/replay,
// the operator's policy persists across restores. // the operator's policy persists across restores.
// //
// PRESERVED_META_KEYS + `preservedMeta` are declared above the // PRESERVED_META_KEYS is declared above the SQLite/PG split
// SQLite/PG split (~L795) so the replay block at the bottom // (~L820). The snapshot READ happens here in the PG branch
// can read them on both branches. Only the snapshot READ // (must run before DROP), but is stashed on
// needs to happen here in the PG branch (must run before DROP). // `this.preservedMetaSnapshot` for the parent `restore()`
// method to consume AFTER verification — see the round-3
// notes there.
try { try {
preservedMeta = await db('app_settings') this.preservedMetaSnapshot = await db('app_settings')
.whereIn('setting_key', PRESERVED_META_KEYS) .whereIn('setting_key', PRESERVED_META_KEYS)
.select('setting_key', 'setting_value', 'setting_type'); .select('setting_key', 'setting_value', 'setting_type');
this.log('info', `Snapshotted ${preservedMeta.length} restore-meta setting(s) for post-restore replay`, { this.log('info', `Snapshotted ${this.preservedMetaSnapshot.length} restore-meta setting(s) for post-restore replay`, {
keys: preservedMeta.map(r => r.setting_key), keys: this.preservedMetaSnapshot.map(r => r.setting_key),
}); });
} catch (err) { } catch (err) {
this.log('warn', `Could not snapshot restore-meta settings (continuing): ${err.message}`); 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 // uses `npm run migrate:safe` (run-migrations-safe.js) which
// knows to skip helpers.js + walks core/ explicitly. // knows to skip helpers.js + walks core/ explicitly.
// //
// For restore: the dump we just loaded already contains the // The safe runner gets invoked AFTER verification in restore()
// schema state of whatever migrations had been applied at // (see step 7c) to apply any post-backup migrations to the
// backup time. If the running image has NEWER migrations that // restored DB. This closes the contract "backup must restore
// need to run on top of the restored DB, those will be applied // completely even when new features have been added in the
// on the NEXT container start by wait-for-db.sh + the safe // meantime" — without this step, restoring an old backup on a
// runner. That's a one-restart penalty in the unusual case of // newer image would leave the running process serving a
// restoring from a backup older than the current image, and // mismatched schema until the next container restart.
// matches what picpeak does on every other boot already. this.log('info', 'Schema migrations deferred to restore() step 7c (npm run migrate:safe subprocess)');
this.log('info', 'Skipping in-process migrate (deferred to next boot via safe runner)');
// Replay the snapshotted operator-meta settings on top of the // NOTE: operator-meta REPLAY does NOT happen here any more.
// restored DB. UPSERT by setting_key — if the backup had the // PR #596 round 3: if the replay runs inside performDatabaseRestore,
// same key with a different value, we overwrite it; if the row // it lands BEFORE post-restore verification — and verification
// doesn't exist in the backup, we insert it. Either way the // then counts the replayed row as a mismatch (e.g. "expected 190,
// operator's pre-restore policy survives. // got 191" because the fresh-install seeded
if (preservedMeta.length > 0) { // `restore_allow_force_auto_upgraded` that wasn't in the backup).
try { // Replay is now drained by the parent `restore()` method AFTER
for (const row of preservedMeta) { // verification passes. Snapshot lives on
await db('app_settings') // `this.preservedMetaSnapshot` for that drain.
.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}`);
}
}
return { success: true }; return { success: true };
+8
View File
@@ -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. 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) ## 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. 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.