fix(restore): set was_successful=true on the completed update
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.
This commit is contained in:
@@ -148,6 +148,41 @@ describe('restoreService — PG branch scope contract (PR #596 review)', () => {
|
||||
expect(dangerousLines).toEqual([]);
|
||||
});
|
||||
|
||||
it('the completed-restore update sets was_successful=true', () => {
|
||||
// Without this, every successful restore ends up with
|
||||
// status='completed', was_successful=false — the dashboard's
|
||||
// "last successful restore" widget then filters out the row +
|
||||
// any future audit query gating on was_successful misses it.
|
||||
// Caught locally + maintainer PR #596 review.
|
||||
//
|
||||
// Contract: the update payload that writes status='completed' on
|
||||
// the SUCCESS branch ALSO includes was_successful: true. We pin
|
||||
// it by source inspection so any future refactor of the success
|
||||
// payload keeps both fields together.
|
||||
// The success-branch update lives AFTER performPostRestoreVerification.
|
||||
// There's also a `status: 'completed'` in the dry-run / early-return
|
||||
// path (failure handling has its own block too) — we want the
|
||||
// SUCCESS-branch one specifically.
|
||||
const verifyLine = findFirst(/performPostRestoreVerification\s*\(/);
|
||||
expect(verifyLine).toBeGreaterThan(0);
|
||||
|
||||
const completedStatusLineIdx = lines
|
||||
.map((l, i) => ({ line: i + 1, text: l }))
|
||||
.find(({ line, text }) =>
|
||||
line > verifyLine && /status:\s*['"]completed['"]/.test(text)
|
||||
);
|
||||
expect(completedStatusLineIdx).toBeDefined();
|
||||
|
||||
// Look in the next ~10 lines for was_successful: true. The actual
|
||||
// payload is small (no nested objects between status and the
|
||||
// closing })), so a fixed-window search is reliable.
|
||||
const window = lines.slice(
|
||||
completedStatusLineIdx.line - 1,
|
||||
completedStatusLineIdx.line + 10,
|
||||
).join('\n');
|
||||
expect(window).toMatch(/was_successful:\s*true/);
|
||||
});
|
||||
|
||||
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
|
||||
|
||||
@@ -326,6 +326,15 @@ class RestoreService {
|
||||
await db('restore_runs').where('id', runId).update({
|
||||
completed_at: endTime,
|
||||
status: 'completed',
|
||||
// Default for the column is `false`. Without this line, every
|
||||
// SUCCESSFUL restore ends up with `status='completed',
|
||||
// was_successful=false` — which the BackupDashboard "last
|
||||
// successful restore" widget then filters out, and any future
|
||||
// audit query that gates on was_successful misses the row
|
||||
// entirely. Cosmetic but enough to mislead an operator
|
||||
// scanning restore history. Catches Ralf 2026-06-01 + maintainer
|
||||
// PR #596 review note about the cosmetic.
|
||||
was_successful: true,
|
||||
duration_seconds: durationSeconds,
|
||||
pre_restore_backup_path: this.preRestoreBackupPath,
|
||||
statistics: JSON.stringify({
|
||||
|
||||
Reference in New Issue
Block a user