Caught during the round-4 e2e validation on real PG: every
successful restore landed with `status='completed', was_successful=false`
because the success-branch update only wrote `status` but not
`was_successful` (column default is false). Visible side effect: the
BackupDashboard's "last successful restore" filter would skip the
row + any future audit query gating on was_successful would miss it.
One-line cure: include `was_successful: true` in the success-branch
update payload. Inline comment explains why and references the
review note so future edits keep the two fields together.
Source-inspection test in restoreService.pgBranch.test.js pins the
contract: after `performPostRestoreVerification(...)`, the
`status: 'completed'` update payload must also contain
`was_successful: true`. Future refactors of the success payload that
drop the flag fail the test before merge.
36/36 backup-related integration tests pass.
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.
pg-driver serialises `bigint` (which is what `COUNT(*)` returns) as a
JavaScript STRING to preserve precision for huge counts. The manifest
stores `expected.rowCount` as a JS number (parseInt'd at
databaseBackup.js:118). Strict `!==` in performPostRestoreVerification
flagged every match as a mismatch on PG:
Table activity_logs row count mismatch: expected 16, got 16
Table admin_users row count mismatch: expected 1, got 1
Table app_settings row count mismatch: expected 165, got 165
... (every table, all matching)
Symptom matched the preservedMeta scope leak from round 1: install-
from-backup logged FAILED, trigger file wasn't cleaned, data was
actually intact. Caught on PR #596 e2e re-run.
Cure: coerce both sides with `Number(...)` at the comparison AND in
the interpolated value so the warning text renders `16` not `"16"`.
Pre-emptive: lines 448 + 458-459 had the same string-vs-number issue
masked by `>` (JS coerces operands for `>`), but the warning text
printed `"5"` on PG vs `5` on SQLite, and a future patch changing
`>` to `=== 0` or `!== expectedCount` would silently break on PG.
Coerced at the read site into `eventCountN` / `activeUsersN` locals
+ added a comment block explaining the contract so future edits
don't drop the Number() calls without re-auditing.
New source-inspection test: pins the contract that every `.count`
result in restoreService.js MUST be wrapped in `Number(...)` when
used in a comparison (===/!==/>/</>=/<=). Same source-inspection
pattern as the preservedMeta test added round 1 — pragmatic until
the real-PG integration test follow-up lands.
The maintainer's audit of the rest of the backup/restore surface
(_installFromBackupBoot, _restoreSettingsBoot, _backupPathsBoot,
backupCoverageService, backupIntegrityService, backupService,
databaseBackup) confirmed no other bigint-as-string sites — the
class is now closed in the audited scope.
`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.
Luca