From a23fa3bb12cd05a922c0fb860ae97ffd3fe2baff Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Mon, 1 Jun 2026 21:51:49 +0200 Subject: [PATCH] fix(restore): hoist preservedMeta above SQLite/PG split (PR #596 blocker) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../restoreService.pgBranch.test.js | 167 ++++++++++++++++++ backend/src/services/restoreService.js | 30 +++- 2 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 backend/__tests__/integration/restoreService.pgBranch.test.js diff --git a/backend/__tests__/integration/restoreService.pgBranch.test.js b/backend/__tests__/integration/restoreService.pgBranch.test.js new file mode 100644 index 00000000..6b0342e9 --- /dev/null +++ b/backend/__tests__/integration/restoreService.pgBranch.test.js @@ -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); + }); +}); diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index 7096d73b..bba57295 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -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)