fix(restore): hoist preservedMeta above SQLite/PG split (PR #596 blocker)

`preservedMeta` was declared with `let` INSIDE the PostgreSQL else
branch of performDatabaseRestore (~L850), then read AFTER the else
block closed at the shared replay site (~L1030). On every real PG
restore, this threw:

  ReferenceError: preservedMeta is not defined

after psql had already loaded the data successfully. Knock-on
effects per the maintainer's review:

  - Loud `Install-from-backup: FAILED` line in combined.log even
    though the data restored cleanly
  - Trigger file in `_installFromBackupBoot.js` was left in place
    because the success branch never ran — admin had to manually
    rm it before the next boot
  - The operator-meta replay (restore_allow_force,
    restore_allow_force_auto_upgraded) silently dropped, exactly
    the chicken-and-egg the snapshot was added to close.
    `restore_allow_force` reverted to the backup's value on every
    PG restore.

CI missed it because integration tests around `performFullRestore`
only exercise the SQLite branch (`this.dbType === 'sqlite'`). The PG
branch requires a real psql binary + cluster, which lives in the
"real-PG integration test in CI" follow-up.

Cure: hoist the `const PRESERVED_META_KEYS = [...]` + `let
preservedMeta = []` declarations above the SQLite/PG split. SQLite
leaves them empty; PG branch fills them; replay block at the bottom
reads them on both paths (no-op on SQLite).

New test: `restoreService.pgBranch.test.js` pins the scope contract
via source inspection. Two assertions:
  1. Exactly one `let preservedMeta = []` declaration in the file,
     positioned before the SQLite/PG branch split
  2. The replay block `if (preservedMeta.length > 0)` sits outside
     the else block (closing `      }` exists between the branch
     opener and the replay site)
Source-inspection beats a runtime test here because (a) it doesn't
need a real PG cluster + psql binary, (b) it pins the EXACT property
that broke, more directly than a runtime test would.

Closes PR #596 review blocker.
This commit is contained in:
Luca
2026-06-01 21:51:49 +02:00
parent 205802fb9d
commit a23fa3bb12
2 changed files with 192 additions and 5 deletions
@@ -0,0 +1,167 @@
/**
* Pins the fix for the PR #596 review blocker.
*
* **The bug**
*
* `preservedMeta` was declared with `let` INSIDE the PostgreSQL
* `else` branch of `performDatabaseRestore`, then read AFTER the
* `else` block closed at the shared replay site (~L1030). On every
* real PG restore:
*
* ReferenceError: preservedMeta is not defined
*
* would fire — psql had already completed the data restore, but
* the operator-meta replay never ran, the trigger file was left
* in place by `_installFromBackupBoot.js` because the restore
* "failed", and `combined.log` got a loud FAILED line even though
* the data was back. Caught on PR #596 review by the maintainer.
*
* **Why CI missed it**
*
* The integration tests around `performFullRestore` only exercise
* the SQLite branch via `this.dbType === 'sqlite'`. The PG branch
* (~L827-984) requires a real PG connection + real `psql` binary,
* neither of which are in the test environment. So the scope leak
* sat untested until the maintainer ran a real DR cycle.
*
* **What this test does**
*
* Reads the source of `restoreService.js` and asserts the scope
* contract: the `preservedMeta` declaration sits ABOVE the
* SQLite/PG branch split, so the replay block at the bottom of the
* try{} can read it on either branch.
*
* Source-inspection is uglier than a runtime test but it has two
* advantages here: (a) it doesn't require a real PG cluster + psql
* binary in CI, (b) it pins the EXACT contract — "the declaration
* must be visible to the replay block" — which is the property
* that broke, more directly than a runtime test would.
*
* The follow-up "real-PG integration test in CI" (separate task)
* would replace this with an end-to-end exercise, at which point
* this can be deleted.
*/
const fs = require('fs');
const path = require('path');
describe('restoreService — PG branch scope contract (PR #596 review)', () => {
let src;
let lines;
beforeAll(() => {
src = fs.readFileSync(
path.join(__dirname, '..', '..', 'src', 'services', 'restoreService.js'),
'utf8',
);
lines = src.split(/\r?\n/);
});
/** Return the 1-based line number of the FIRST line matching `re`. */
function findFirst(re) {
const idx = lines.findIndex((l) => re.test(l));
return idx >= 0 ? idx + 1 : -1;
}
/** Return the 1-based line number of the LAST line matching `re`. */
function findLast(re) {
let last = -1;
lines.forEach((l, i) => { if (re.test(l)) last = i + 1; });
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:
//
// 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)
);
expect(splitLine).toBeGreaterThan(-1);
const splitLineOneBased = splitLine + 1;
// 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);
});
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*\)/);
expect(replayLine).toBeGreaterThan(0);
// 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;
break;
}
}
expect(foundElseClose).toBe(true);
});
});
+25 -5
View File
@@ -792,6 +792,26 @@ 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.
const PRESERVED_META_KEYS = [
'restore_allow_force',
'restore_allow_force_auto_upgraded',
];
let preservedMeta = [];
try {
if (this.dbType === 'sqlite') {
// SQLite restore
@@ -843,11 +863,11 @@ class RestoreService {
// back to whatever was in the backup. Two consecutive restores
// needed the SQL workaround again. With this snapshot/replay,
// the operator's policy persists across restores.
const PRESERVED_META_KEYS = [
'restore_allow_force',
'restore_allow_force_auto_upgraded',
];
let preservedMeta = [];
//
// 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).
try {
preservedMeta = await db('app_settings')
.whereIn('setting_key', PRESERVED_META_KEYS)