From 2c724c9283fe31ec8b723e20cc149bc963f7e2c3 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 22 Aug 2026 18:03:18 +0200 Subject: [PATCH] R-379/R-380: put the customer's undo copy back when a database restore fails R-379 and R-380 were one failure. Both ended with a half-restored database; the only difference was whether it looked broken. Postgres emptied and crash-looped; MariaDB applied part of the dump and reported health=healthy with a zero-row schema-version table. Measured live on demo-hp 2026-08-22. The undo copy was already taken and already good - proven by hand that day on both engines. Nothing in the product could apply it. Now it does, with the same ImportDump call, before any restart and inside the DB-only window. The WHOLE undo set, matched on this run's stamp. writeSafetyDump returned one path for an app with two databases; a rollback on that would restore one and leave the other half-written. When the rollback also fails the app is HELD STOPPED (operator ruling): a running app on a half-written database lets the customer make the damage permanent. Every start path refuses it - customer button, appstop Recover, boot sweep - via the shared driveStartGate, checked ABOVE its driveless early return because these apps have no drive. The marker is ended so nothing auto-restarts it. The row goes red. Cleared with --clear-restore-hold, an operator CLI route. --single-transaction is a belt on Postgres only; MariaDB DDL is not transactional and that is why the rollback is the fix. R-381: the engine's stderr stops reaching the customer (615 bytes on MariaDB, its middle rows out of their own database) and starts reaching the operator log, which never had it. R-382: the summary log prints the volume count it already held. Undo copies resolve to their own app, are marked IsUndo, and are capped at 3 per app, pruned from the capture side. The reported render-as-an-app symptom did NOT reproduce - the live page was read first and had zero occurrences. Tests 1468 -> 1483. Eight red-proofs; ONE PASSED and is reported: the R-381 behavioural test injected below ImportDump. A guard at that layer now convicts. --- CHANGELOG.md | 63 ++++ controller/cmd/controller/main.go | 86 ++++- .../cmd/controller/r379_hold_gate_test.go | 143 ++++++++ controller/internal/api/router.go | 20 ++ controller/internal/appbackup/dbdump.go | 72 +++- .../appbackup/r381_undo_naming_test.go | 182 ++++++++++ controller/internal/backup/backup.go | 19 + .../internal/backup/offbox_reconstitute.go | 272 +++++++++++++-- controller/internal/backup/offbox_restore.go | 6 + .../internal/backup/r355_safetydump_test.go | 18 +- .../internal/backup/r379_rollback_test.go | 330 ++++++++++++++++++ .../internal/backup/r47_replay_order_test.go | 21 +- controller/internal/backup/recovery_unit.go | 5 + controller/internal/settings/settings.go | 73 ++++ controller/internal/web/handlers.go | 15 + 15 files changed, 1288 insertions(+), 37 deletions(-) create mode 100644 controller/cmd/controller/r379_hold_gate_test.go create mode 100644 controller/internal/appbackup/r381_undo_naming_test.go create mode 100644 controller/internal/backup/r379_rollback_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cadb6a2..cb96f6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,66 @@ +## v0.220.0 — when a database restore fails, the customer's own copy goes back (2026-08-22, R-379/R-380/R-381/R-382) +**MinAgent: 0.129.0** (unchanged — no new agent coupling) + +**R-379 and R-380 were ONE failure and they get ONE fix.** Both ended with a half-restored database. +The only difference was whether it looked broken: on Postgres the database was emptied and the app +crash-looped; on MariaDB part of the dump applied, the rest did not, and the app reported +`health=healthy, running=true, restarts=0` with its schema-version table holding zero rows. Measured +live on `demo-hp` on 2026-08-22 (`audits/DRILL-r356b-driveless-db-restore-2026-08-22/`). + +**The undo copy was already being taken, and was already good.** It was proven good by hand that day +on both engines — `docmost` recovered, `bookstack`'s `migrations` went 0 → 102 rows. What no product +action could do was apply it: `pre-restore-` files are skipped at three call sites so they are never +mistaken for a replay source, and the filename appeared only inside an error string. **The fix is the +product making the same `ImportDump` call a person made by hand.** + +**When the replay fails, the undo set is now re-applied automatically**, before any restart and with +the DB service still up, so the app never observes the half state. The app then starts and the +message says **both** things: the restore failed, *and* the data is back as it was. A message that +reported only the failure would leave the customer believing their data was gone when it is not — the +omission of a gain misleads exactly as much as the omission of a loss. + +**THE WHOLE undo set, not the first file.** `writeSafetyDump` returned one path for an app with two +databases; a rollback built on that would have restored one and left the other half-written — this +defect, one database over. It now returns the set, matched on **this run's stamp**: four `pre-restore-` +files accumulated on one app in one afternoon, so a prefix match would replay an arbitrary older state. + +**When the rollback ALSO fails, the app is HELD STOPPED — operator ruling, 2026-08-22.** A running app +on a half-written database lets the customer type into it and turns a recoverable state into a +permanent one. The hold is persisted, every start path refuses it (the customer's button, the app-stop +`Recover()` starter, and the boot sweep — through the shared `driveStartGate`, checked **above** its +driveless early return because these apps have no drive), the app-stop marker is ended so nothing +auto-restarts it at the next boot, the row goes **red** rather than green, and the operator is told. +Clearing it is `--clear-restore-hold `, an operator CLI route reached through `docker exec`. + +**`--single-transaction` is a BELT, not the fix.** Added to the Postgres import so a replay is +all-or-nothing at the engine. **It does NOT make MariaDB atomic** — MariaDB's DDL is not +transactional, so a partial apply there is unavoidable at the engine level. That is precisely why the +rollback exists, and this flag does not make it optional. + +**R-381 — the failure message stops pasting the engine's output at the customer.** It was 407 bytes on +Postgres (a caret diagram and `exit status 3`) and **615 on MariaDB, whose middle was an `INSERT INTO +migrations VALUES (…)` listing — rows out of the customer's own database, HTML-escaped, on their +dashboard.** The full engine text now goes to the operator log, which never had it before: the +diagnostic is **added**, not removed. + +**R-382 — the summary log line prints the volume count it already held.** It said "0 file(s) placed, +1 DB dump(s) replayed" on a run that returned a 52 MB Postgres data directory. + +**The undo copies are named for what they are, and bounded.** `pre-restore-20260822T140924Z-docmost-postgres.sql` +used to derive the phantom stack `pre-restore-20260822T140924Z-docmost`; it now resolves to `docmost` +and carries `IsUndo`. **The reported symptom — that they render as apps on the customer's backup page +— did NOT reproduce**: the live page was read first and contained zero `pre-restore` strings, because +`buildAppBackupRows` iterates deployed apps and only reads that map by key. The phantom name was real +as a map KEY; the row was not. Their VISIBILITY is unchanged and deliberate. A cap of **3 per app** +now applies, pruned from the capture side and never from the restore path — a delete on the failure +path is how an undo goes missing at the moment it is needed. + +**Tests:** `internal/backup/r379_rollback_test.go`, `cmd/controller/r379_hold_gate_test.go`, +`internal/appbackup/r381_undo_naming_test.go`. Test count 1468 → 1483. **Eight red-proofs; one PASSED +and is reported rather than omitted** — the R-381 behavioural test injected below `ImportDump` and so +could not see a leak reintroduced inside it. A guard at that layer was added and the mutation then +convicted. + ## v0.219.0 — the off-site restore refused every app that has no data drive (2026-08-22, R-356) **MinAgent: 0.129.0** (unchanged — no new agent coupling) diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 3d915e0..a6b9b99 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -84,6 +84,8 @@ func main() { abandonStatus := flag.Bool("abandon-status", false, "R-241: print the state of this box's off-site abandonment countdown (set-aside path, due date, days left) and exit. Read-only.") abandonExtend := flag.Int("abandon-extend", 0, "R-241 (operator): extend a running abandonment countdown by N days from now, then exit. Refuses when no countdown is running.") abandonStop := flag.Bool("abandon-stop", false, "R-241 (operator): stop a running abandonment countdown, then exit. The set-aside history is kept and nothing is deleted. Refuses when no countdown is running.") + listRestoreHolds := flag.Bool("restore-holds", false, "R-379 (operator): list apps this controller is deliberately holding stopped after a failed database restore whose rollback also failed, then exit. Read-only.") + clearRestoreHold := flag.String("clear-restore-hold", "", "R-379 (operator): clear the restore hold on APP so it can start again, then exit. Clearing does NOT repair the database — check the app's data first; the undo copies are in its unit's db-dumps dir.") flag.Parse() if *showVersion { @@ -160,6 +162,48 @@ func main() { // §7.5 (R-241) — the operator's abandonment levers. Grouped in one block, before the server // starts, exactly like the other CLI subcommands: each loads config + settings, acts, and exits. + // R-379 (operator): the way OUT of a hold. A CLI subcommand rather than an API route, matching + // --abandon-stop above: the controller's HTTP surface is authenticated as the CUSTOMER, and + // clearing a hold is a decision that should follow an operator looking at the database. Reaching + // this needs `docker exec` into the guest, which is the operator gate this project already uses. + if *listRestoreHolds || *clearRestoreHold != "" { + cfg, err := config.LoadPermissive(*configPath) + if err != nil { + fmt.Fprintf(os.Stderr, "restore-hold: loading config: %v\n", err) + os.Exit(1) + } + lg := log.New(os.Stderr, "", 0) + sett, err := settings.Load(cfg.Paths.DataDir+"/settings.json", lg) + if err != nil { + fmt.Fprintf(os.Stderr, "restore-hold: loading settings: %v\n", err) + os.Exit(1) + } + if *listRestoreHolds { + holds := sett.ListRestoreHolds() + if len(holds) == 0 { + fmt.Println("no restore holds in force") + os.Exit(0) + } + for _, h := range holds { + fmt.Printf("%s\theld since %s\n replay error : %s\n rollback err : %s\n", h.Stack, h.At, h.ReplayError, h.RollbackErr) + } + os.Exit(0) + } + cleared, err := sett.ClearRestoreHold(*clearRestoreHold) + if err != nil { + fmt.Fprintf(os.Stderr, "restore-hold: clearing %s: %v\n", *clearRestoreHold, err) + os.Exit(1) + } + if !cleared { + // "there was nothing to clear" is not success — a silent 0 here would let an operator + // believe they had unblocked an app whose name they mistyped. + fmt.Fprintf(os.Stderr, "no restore hold in force for %q — nothing was changed\n", *clearRestoreHold) + os.Exit(2) + } + fmt.Printf("restore hold cleared for %s — the app may be started again. Check its data first: the undo copies are in its unit's db-dumps dir.\n", *clearRestoreHold) + os.Exit(0) + } + if *abandonStatus || *abandonExtend > 0 || *abandonStop { cfg, err := config.LoadPermissive(*configPath) if err != nil { @@ -981,6 +1025,31 @@ func main() { } notifier.NotifyBackupRunFailures(rs.Message, d) }) + // R-379/R-380: an app HELD after a failed database restore whose rollback also failed. + // + // ROUTED THROUGH `backup_run_failures`, and the reuse is deliberate but imperfect — say so + // rather than let a future reader assume it was the natural fit. That type is operator-only + // (`notify.operatorOnlyEvents`), carries severity `error` (a VALID token — `warn` would + // coerce to `info` and email nobody, which is R-329 and still live elsewhere), and its + // payload is exactly {app, leg, reason}. What it is NOT is a restore event: it says "backup + // run". **No operator-facing RESTORE event type exists**, and minting one is a two-repo + // change with a hub deploy, which this task's baselines hold fixed. The leg is named + // `restore-hold` so the record is unambiguous to whoever reads it. + // + // DELIBERATELY NOT `backup_failed`: customer-enabled by default, Hungarian "A biztonsági + // mentés sikertelen!", and this is a deliberate hold rather than a backup failure. R-171 one + // path over. + backupMgr.SetRestoreHoldNotify(func(stack string, replayErr, rollbackErr error) { + d := notify.BackupRunFailuresDetails{RunKind: "offsite-reconstitute", Failed: 1, Attempted: 1} + reason := "rollback failed" + if rollbackErr != nil { + reason = rollbackErr.Error() + } + d.Apps = append(d.Apps, notify.RunFailureDetail{App: stack, Leg: "restore-hold", Reason: reason}) + msg := fmt.Sprintf("App %q is HELD STOPPED: its off-site database restore failed AND the rollback to the customer's own pre-restore copy also failed. "+ + "The app will not start from any path until the hold is cleared. Replay error: %v. Rollback error: %v", stack, replayErr, rollbackErr) + notifier.NotifyBackupRunFailures(msg, d) + }) // 3a: the pre-push enlargement gate blocked an app's userdata push (config+DB still saved). Edge- // triggered by the engine (only NEW blocks notify), so the hub's per-event-type cooldown suffices — // no controller-side timer (the hub owns cooldown). @@ -1774,6 +1843,19 @@ type driveStartGate struct { } func (g driveStartGate) MayStart(stackName string) (bool, string) { + // R-379/R-380 HOLDER, checked FIRST and deliberately ABOVE the `HDD_PATH == ""` early return + // below. The apps this hold exists for are DRIVELESS — a failed database restore is the case, + // and 40 of the 53 catalogue apps have no drive at all — so a hold placed after that return + // would never be consulted for the exact class it was built for. + // + // This is holder #4, and putting it here rather than in bootDriveGate is what gives it two + // callers with one implementation: the boot sweep reaches it through bootDriveGate, and the + // app-stop guard's Recover reaches it directly. A hold only one path honours is not a hold. + if g.sett != nil { + if h, ok := g.sett.GetRestoreHold(stackName); ok { + return false, "held after a failed restore whose rollback also failed (" + h.At + ") — clear the hold to start it" + } + } if g.mgr == nil { return false, "no stack manager wired — the drive cannot be determined" } @@ -2444,7 +2526,9 @@ func (a *exportAdapter) GetImportRoot() string { return a.mgr.GetImportRoot() } // GetStackNamespaceRoot (R-203) — the felhom-data namespace root, NOT the drive path. The appbackup // path helpers all take this; passing HDD_PATH straight in is what made the export plan and the // off-site capture set describe different directories on the system-data fallback. -func (a *exportAdapter) GetStackNamespaceRoot(name string) string { return a.mgr.StackNamespaceRoot(name) } +func (a *exportAdapter) GetStackNamespaceRoot(name string) string { + return a.mgr.StackNamespaceRoot(name) +} func (a *exportAdapter) GetStackHDDPath(name string) string { s, ok := a.mgr.GetStack(name) diff --git a/controller/cmd/controller/r379_hold_gate_test.go b/controller/cmd/controller/r379_hold_gate_test.go new file mode 100644 index 0000000..2803764 --- /dev/null +++ b/controller/cmd/controller/r379_hold_gate_test.go @@ -0,0 +1,143 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "io" + "log" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// R-379 SCENARIO D — a held app, later. A hold that only one start path honours is not a hold, so +// each path is proven rather than asserted. + +func holdTestSettings(t *testing.T) *settings.Settings { + t.Helper() + s, err := settings.Load(filepath.Join(t.TempDir(), "settings.json"), log.New(io.Discard, "", 0)) + if err != nil { + t.Fatal(err) + } + return s +} + +// The SHARED gate refuses a held app — this is the path both the boot sweep (via bootDriveGate) and +// the app-stop guard's Recover() reach. +// +// POSITIVE CONTROL BUILT IN: the same app, same gate, is allowed once the hold is cleared. Without +// it "the gate refuses" could be true because the gate refuses everything. +func TestR379_DriveStartGate_RefusesAHeldApp_AndAllowsItAfterClearing(t *testing.T) { + sett := holdTestSettings(t) + g := driveStartGate{sett: sett} // nil mgr on purpose: the hold must be answered BEFORE the drive + + // Before: no hold. The nil manager makes this "cannot determine", which is NOT the hold's reason. + if _, why := g.MayStart("docmost"); why == "" { + t.Fatal("fixture: expected some reason from the bare gate") + } else if strings.Contains(why, "held after a failed restore") { + t.Fatalf("no hold exists yet, but the gate cited one: %q", why) + } + + if err := sett.SetRestoreHold(settings.RestoreHold{Stack: "docmost", At: "2026-08-22T14:00:00Z"}); err != nil { + t.Fatal(err) + } + ok, why := g.MayStart("docmost") + if ok { + t.Fatal("a held app must not be startable") + } + if !strings.Contains(why, "held after a failed restore") { + t.Errorf("the refusal must NAME the hold, got %q", why) + } + + // POSITIVE CONTROL: clear it, and the gate stops citing the hold. + cleared, err := sett.ClearRestoreHold("docmost") + if err != nil || !cleared { + t.Fatalf("clearing the hold failed: cleared=%v err=%v", cleared, err) + } + if _, why := g.MayStart("docmost"); strings.Contains(why, "held after a failed restore") { + t.Errorf("the hold was cleared but the gate still cites it: %q", why) + } +} + +// The hold is checked ABOVE the `HDD_PATH == ""` early return. The apps this exists for are +// DRIVELESS — a failed database restore is the case, and 40 of 53 catalogue apps have no drive — so +// a hold placed after that return would never be consulted for the exact class it was built for. +func TestR379_HoldIsCheckedBeforeTheDrivelessEarlyReturn(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatal(err) + } + var body *ast.BlockStmt + for _, d := range f.Decls { + fn, ok := d.(*ast.FuncDecl) + if ok && fn.Recv != nil && fn.Name.Name == "MayStart" && fn.Body != nil { + // driveStartGate is the receiver we want; bootDriveGate's MayStart is separate. + if st, ok := fn.Recv.List[0].Type.(*ast.Ident); ok && st.Name == "driveStartGate" { + body = fn.Body + } + } + } + if body == nil { + t.Fatal("driveStartGate.MayStart not found in main.go") + } + holdPos, drivelessPos := -1, -1 + ast.Inspect(body, func(n ast.Node) bool { + if c, ok := n.(*ast.CallExpr); ok { + if sel, ok := c.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "GetRestoreHold" && holdPos < 0 { + holdPos = fset.Position(c.Pos()).Line + } + } + // the `hdd == ""` driveless early return + if b, ok := n.(*ast.BinaryExpr); ok && b.Op == token.EQL && drivelessPos < 0 { + if id, ok := b.X.(*ast.Ident); ok && id.Name == "hdd" { + drivelessPos = fset.Position(b.Pos()).Line + } + } + return true + }) + if holdPos < 0 { + t.Fatal("driveStartGate.MayStart never consults GetRestoreHold — a held app would start from the boot sweep and from Recover()") + } + if drivelessPos < 0 { + t.Fatal("the driveless early return was not found — this test can no longer prove the ordering") + } + if holdPos > drivelessPos { + t.Fatalf("the hold check (line %d) is AFTER the driveless early return (line %d) — it would never fire for the 40 driveless apps this exists for", holdPos, drivelessPos) + } +} + +// The production wiring: main() must hand the backup manager its hold notifier, or a held app is +// held silently and no operator ever hears. AST, not strings.Contains — a commented-out call still +// contains the string. +func TestR379_MainWiresTheRestoreHoldNotifier(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatal(err) + } + var found bool + for _, d := range f.Decls { + fn, ok := d.(*ast.FuncDecl) + if !ok || fn.Name.Name != "main" || fn.Body == nil { + continue + } + ast.Inspect(fn.Body, func(n ast.Node) bool { + c, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := c.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "SetRestoreHoldNotify" { + found = true + return false + } + return true + }) + } + if !found { + t.Fatal("main() never calls SetRestoreHoldNotify — an app would be held with nobody told") + } +} diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index f8a9393..0998f16 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -514,6 +514,16 @@ func (r *Router) deployStack(w http.ResponseWriter, req *http.Request, name stri } } +// restoreHoldFor reports whether an app is held after a failed restore whose rollback also failed, +// and returns the CUSTOMER-facing Hungarian reason. Delegates to the backup manager so the sentence +// has one source; the drive gate's own reason is operator-English and stays that way. +func (r *Router) restoreHoldFor(name string) (bool, string) { + if r.backupMgr == nil { + return false, "" + } + return r.backupMgr.RestoreHoldFor(name) +} + // startGatedByMissingDrive reports whether starting `name` must be BLOCKED because the drive its // HDD_PATH points at is currently disconnected or decommissioned. Returns the storage path for the // message. SSD-resident apps (no HDD_PATH) are never gated. @@ -565,6 +575,16 @@ func (r *Router) actionStack(w http.ResponseWriter, action, name string) { // Drive-absent gate: refuse to start an app whose data drive is currently disconnected/decommissioned // (the intermediary-mount gate). Starting it would let it write to the empty fail-closed stable path // or just crash-loop; block with a clear message until the drive returns (then the gate auto-restarts). + // R-379/R-380: an app held after a failed restore + failed rollback must not start from the + // customer's button either. Checked BEFORE the drive gate because it applies to driveless apps, + // which is the class the hold exists for. + if action == "start" || action == "restart" { + if held, why := r.restoreHoldFor(name); held { + writeJSON(w, http.StatusConflict, apiResponse{OK: false, Error: why}) + return + } + } + if action == "start" { if gated, hdd := r.startGatedByMissingDrive(name); gated { writeJSON(w, http.StatusConflict, apiResponse{ diff --git a/controller/internal/appbackup/dbdump.go b/controller/internal/appbackup/dbdump.go index aa96d07..19be592 100644 --- a/controller/internal/appbackup/dbdump.go +++ b/controller/internal/appbackup/dbdump.go @@ -75,6 +75,42 @@ type DumpFileInfo struct { Size int64 ModTime time.Time Validation DumpValidation + // IsUndo (R-379, v0.220.0) marks a `pre-restore-` safety copy: the undo taken immediately before + // a reconstitution, not the app's own backup. Its VISIBILITY is a recorded decision — see the + // comment on preRestoreDumpPrefix, "an undo the customer cannot see is not much of one" — so this + // flag names it rather than hiding it. + // + // It also fixes a real, if latent, naming defect: the filename parse below used to derive + // StackName by trimming only the engine suffix, so + // `pre-restore-20260822T140924Z-docmost-postgres.sql` yielded the phantom stack + // `pre-restore-20260822T140924Z-docmost`. That name reaches the `dbStacks` lookup in + // web.buildAppBackupRows as a KEY. It renders nothing today because that function iterates + // DEPLOYED apps and only reads the map by key — verified on the live page 2026-08-22, which + // contained zero `pre-restore` strings while four such files sat on disk — but any future code + // that RANGES over that map would surface a stack that does not exist. + IsUndo bool + // UndoAt is when the undo copy was taken, parsed from the stamp in its own filename. Zero for a + // normal dump. + UndoAt time.Time +} + +// UndoDumpPrefix is the marker on a pre-restore safety copy. It is duplicated from +// backup.preRestoreDumpPrefix on purpose — appbackup must not import backup (that direction is the +// dependency, not this one) — and the two are pinned equal by a test. +const UndoDumpPrefix = "pre-restore-" + +// trimUndoPrefix splits `pre-restore--` into (, , true). Returns +// (base, "", false) for anything that is not an undo copy, so a normal dump flows unchanged. +func trimUndoPrefix(base string) (string, string, bool) { + if !strings.HasPrefix(base, UndoDumpPrefix) { + return base, "", false + } + after := strings.TrimPrefix(base, UndoDumpPrefix) + i := strings.Index(after, "-") + if i <= 0 { + return base, "", false // a prefix with no stamp is not the shape writeSafetyDump writes + } + return after[i+1:], after[:i], true } // DiscoverDatabases finds running database containers via docker ps. @@ -571,6 +607,15 @@ func ListDumpFiles(dumpDir string, cached func(name string, size int64, mod time // Parse stack name and DB type from filename: "paperless-ngx-postgres.sql" base := strings.TrimSuffix(e.Name(), ".sql") + // R-379: strip the `pre-restore--` head FIRST, so an undo copy resolves to the app it + // belongs to instead of a phantom stack named after its own timestamp. + if rest, stamp, ok := trimUndoPrefix(base); ok { + base = rest + f.IsUndo = true + if t, err := time.Parse("20060102T150405Z", stamp); err == nil { + f.UndoAt = t + } + } if strings.HasSuffix(base, "-postgres") { f.StackName = strings.TrimSuffix(base, "-postgres") f.DBType = DBTypePostgres @@ -680,8 +725,15 @@ func ImportDump(ctx context.Context, db DiscoveredDB, dumpPath string, logger *l dbName = user } // ON_ERROR_STOP=1: a real import error must FAIL (and surface), not silently half-apply. + // + // --single-transaction (R-380, v0.220.0) is a BELT, not the fix. It makes a Postgres replay + // all-or-nothing at the engine, so the emptied-and-crash-looping state measured on `docmost` + // on 2026-08-22 stops being reachable on this engine. It does NOT make MariaDB atomic — + // MariaDB's DDL is not transactional, so a partial apply there is unavoidable at the engine + // and is why the rollback in offbox_reconstitute.go exists and is the actual fix. Do not + // read this flag as making the rollback optional. cmd = exec.CommandContext(impCtx, "docker", "exec", "-i", db.ContainerID, - "psql", "-v", "ON_ERROR_STOP=1", "-U", user, "-d", dbName) + "psql", "-v", "ON_ERROR_STOP=1", "--single-transaction", "-U", user, "-d", dbName) case DBTypeMariaDB: password := getMariaDBPassword(impCtx, db.ContainerID) if password == "" { @@ -700,11 +752,21 @@ func ImportDump(ctx context.Context, db DiscoveredDB, dumpPath string, logger *l logger.Printf("[DEBUG] [backup] ImportDump: importing %s into %s (%s)", dumpPath, db.ContainerName, db.DBType) } if err := cmd.Run(); err != nil { - msg := strings.TrimSpace(stderr.String()) - if len(msg) > 300 { - msg = msg[:300] + // R-381: the engine's stderr goes to the OPERATOR LOG, in full. It used to be truncated to + // 300 chars and folded into the returned error, which the restore surface then rendered to + // the customer — measured 2026-08-22 at 407 bytes (Postgres, a caret diagram and + // `exit status 3`) and 615 bytes (MariaDB, whose middle was an `INSERT INTO migrations + // VALUES (...)` listing, i.e. ROWS OUT OF THE CUSTOMER'S OWN DATABASE, HTML-escaped, on + // their dashboard). + // + // The diagnostic is ADDED here, not removed: previously nothing logged the full text at all, + // so this is strictly more for the operator and strictly less for the customer. + full := strings.TrimSpace(stderr.String()) + if logger != nil { + logger.Printf("[ERROR] [backup] %s import into %s FAILED: %v — engine output follows:\n%s", + db.DBType, db.ContainerName, err, full) } - return fmt.Errorf("%s import into %s failed: %s — %w", db.DBType, db.ContainerName, msg, err) + return fmt.Errorf("%s import into %s failed: %w", db.DBType, db.ContainerName, err) } if logger != nil { logger.Printf("[INFO] [backup] Imported DB dump %s into %s (%s)", filepath.Base(dumpPath), db.ContainerName, db.DBType) diff --git a/controller/internal/appbackup/r381_undo_naming_test.go b/controller/internal/appbackup/r381_undo_naming_test.go new file mode 100644 index 0000000..8bd5e69 --- /dev/null +++ b/controller/internal/appbackup/r381_undo_naming_test.go @@ -0,0 +1,182 @@ +package appbackup + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "testing" + "time" +) + +// R-379/R-381. An undo copy used to derive its stack name by trimming only the engine suffix, so +// `pre-restore-20260822T140924Z-docmost-postgres.sql` yielded the phantom stack +// `pre-restore-20260822T140924Z-docmost`. That name reaches web.buildAppBackupRows' `dbStacks` map +// as a KEY. +// +// MEASURED ON THE LIVE PAGE 2026-08-22 BEFORE CHANGING ANYTHING: /backups/apps contained ZERO +// `pre-restore` strings while four such files sat on disk, because that function iterates DEPLOYED +// apps and only reads the map by key. So the phantom never renders TODAY — the visible-row defect +// does not reproduce, and this test does not claim it did. What it pins is the naming, so any future +// code that RANGES over that map cannot surface a stack that does not exist. +func TestUndoCopyResolvesToItsOwnApp(t *testing.T) { + dir := t.TempDir() + write := func(name string) { + if err := os.WriteFile(filepath.Join(dir, name), []byte("-- PostgreSQL database dump\nCREATE TABLE x();\n"), 0o644); err != nil { + t.Fatal(err) + } + } + write("docmost-postgres.sql") + write("pre-restore-20260822T140924Z-docmost-postgres.sql") + write("pre-restore-20260822T142418Z-bookstack-mariadb.sql") + + files, err := ListDumpFiles(dir, nil) + if err != nil { + t.Fatal(err) + } + byName := map[string]DumpFileInfo{} + for _, f := range files { + byName[f.FileName] = f + } + if len(byName) != 3 { + t.Fatalf("expected 3 dumps listed, got %d", len(byName)) + } + + own := byName["docmost-postgres.sql"] + if own.StackName != "docmost" || own.IsUndo { + t.Errorf("the app's own dump must be stack=docmost and NOT an undo; got stack=%q isUndo=%v", own.StackName, own.IsUndo) + } + + u := byName["pre-restore-20260822T140924Z-docmost-postgres.sql"] + if u.StackName != "docmost" { + t.Errorf("an undo copy must resolve to the app it belongs to, not a phantom; got %q", u.StackName) + } + if !u.IsUndo { + t.Error("an undo copy must be marked as one — its visibility is a recorded decision, so it must be NAMED rather than hidden") + } + if u.DBType != DBTypePostgres { + t.Errorf("the engine must still be parsed; got %q", u.DBType) + } + want, _ := time.Parse("20060102T150405Z", "20260822T140924Z") + if !u.UndoAt.Equal(want) { + t.Errorf("UndoAt = %v, want %v", u.UndoAt, want) + } + + m := byName["pre-restore-20260822T142418Z-bookstack-mariadb.sql"] + if m.StackName != "bookstack" || !m.IsUndo || m.DBType != DBTypeMariaDB { + t.Errorf("mariadb undo copy mis-parsed: stack=%q isUndo=%v type=%q", m.StackName, m.IsUndo, m.DBType) + } +} + +// A file that merely starts with the prefix but is not the shape writeSafetyDump writes must be left +// alone rather than guessed at. +func TestUndoPrefixWithoutAStampIsNotTreatedAsAnUndo(t *testing.T) { + if base, stamp, ok := trimUndoPrefix("pre-restore-nostamp"); ok { + t.Errorf("a prefix with no stamp separator must not parse as an undo; got base=%q stamp=%q", base, stamp) + } + if base, _, ok := trimUndoPrefix("docmost-postgres"); ok || base != "docmost-postgres" { + t.Errorf("a normal dump must flow through unchanged; got base=%q ok=%v", base, ok) + } +} + +// R-381, AT THE LAYER THE LEAK LIVES IN. +// +// WHY THIS TEST EXISTS AND THE BEHAVIOURAL ONE WAS NOT ENOUGH — recorded because it was caught by a +// red-proof that PASSED. The sibling test in internal/backup asserts that the reconstitution's +// customer sentence carries no engine output, but it injects at `m.importDBDump`, i.e. BELOW +// ImportDump. Re-adding the stderr to ImportDump's own fmt.Errorf therefore did not fail it. The +// leak's home is this function, so the guard belongs here. +// +// ImportDump shells out to docker, so its failure path cannot be driven in a unit test. What CAN be +// pinned is the thing the mutation changes: the error it constructs must not reference the captured +// stderr. AST, not strings.Contains — a commented-out reference still contains the string. +func TestImportDumpErrorDoesNotCarryEngineStderr(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "dbdump.go", nil, 0) + if err != nil { + t.Fatal(err) + } + var body *ast.BlockStmt + for _, d := range f.Decls { + if fn, ok := d.(*ast.FuncDecl); ok && fn.Name.Name == "ImportDump" && fn.Body != nil { + body = fn.Body + } + } + if body == nil { + t.Fatal("ImportDump not found — this guard can no longer prove anything") + } + + // Find the name the stderr text is captured into, so the test tracks a rename instead of + // silently passing when the variable is called something else. + stderrVar := "" + ast.Inspect(body, func(n ast.Node) bool { + as, ok := n.(*ast.AssignStmt) + if !ok || len(as.Lhs) != 1 || len(as.Rhs) != 1 { + return true + } + call, ok := as.Rhs[0].(*ast.CallExpr) + if !ok { + return true + } + // strings.TrimSpace(stderr.String()) + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "TrimSpace" { + if id, ok := as.Lhs[0].(*ast.Ident); ok { + stderrVar = id.Name + } + } + return true + }) + if stderrVar == "" { + t.Fatal("could not find where ImportDump captures the engine stderr — the guard cannot aim") + } + + var leaked bool + ast.Inspect(body, func(n ast.Node) bool { + ret, ok := n.(*ast.ReturnStmt) + if !ok { + return true + } + for _, r := range ret.Results { + call, ok := r.(*ast.CallExpr) + if !ok { + continue + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Errorf" { + continue + } + for _, arg := range call.Args { + if id, ok := arg.(*ast.Ident); ok && id.Name == stderrVar { + leaked = true + } + } + } + return true + }) + if leaked { + t.Fatalf("ImportDump's returned error passes %q (the engine stderr) — that text reaches the customer's dashboard. "+ + "Measured 2026-08-22 at 615 bytes on MariaDB, whose middle was rows out of the customer's own database. "+ + "The full text belongs in the operator log, which this function already writes.", stderrVar) + } + + // POSITIVE CONTROL for the "it is still logged" half: the operator must not lose the diagnostic. + var logged bool + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Printf" { + for _, arg := range call.Args { + if id, ok := arg.(*ast.Ident); ok && id.Name == stderrVar { + logged = true + } + } + } + return true + }) + if !logged { + t.Error("the engine stderr is no longer logged either — the diagnostic must be ADDED to the operator log, not removed from everywhere") + } +} diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index f3f8ca5..fbdcdfe 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -54,6 +54,18 @@ type Manager struct { // precedent. unitNotify func(stackName string, err error, usage *UnitSpace) + // restoreHoldNotify (R-379/R-380), if set, is called ONCE when an app is HELD after a database + // replay failed AND the rollback to the customer's own pre-restore copy also failed. Wired in + // cmd/controller/main.go. Same seam shape as unitNotify above and for the same reason: the + // manager must not import the notifier. + // + // OPERATOR-TIER, and this is the whole reason it is a seam rather than a direct call. A held app + // must NOT reach `NotifyBackupFailed` — that type is customer-enabled by default + // (`settings.DefaultEnabledEvents`) and carries the Hungarian "A biztonsági mentés sikertelen!", + // which would alarm a customer about an app we are DELIBERATELY holding. That is R-171's defect + // one path over, and it is the same distinction `ErrStartRefused` exists to keep. + restoreHoldNotify func(stack string, replayErr, rollbackErr error) + // unitSpaceFn (R-165 / B2), if set, replaces the real statfs behind the capture floor so a test // can state a filesystem's occupancy as an input. Nil in production → `unitTargetSpace`. unitSpaceFn func(stackName string) *UnitSpace @@ -137,6 +149,13 @@ type Manager struct { discoverDBs func(ctx context.Context) ([]DiscoveredDB, error) importDBDump func(ctx context.Context, db DiscoveredDB, dumpPath string) error + // rollbackImport (R-379) — the ROLLBACK's ImportDump seam. Deliberately SEPARATE from + // importDBDump above even though both default to ImportDump: the whole point of the rollback is + // what happens when the replay fails, so a test must be able to make the replay fail and the + // rollback succeed (and the reverse). One shared seam cannot express that, and a test that + // cannot express the case cannot pin it. + rollbackImport func(ctx context.Context, db DiscoveredDB, dumpPath string) error + // F3 volume-dump seam — overridable in tests so runVolumeDumps' gating (protected / volume-less / // disconnected) can be unit-tested without Docker. Nil → the real DumpAppVolumesSafe. dumpVolumesSafe func(stackName string) error diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index 93d2d38..c924141 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -6,8 +6,11 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strings" "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" ) // Offsite reconstitution (R-43, v0.148.0) — the leg that was missing. @@ -71,11 +74,16 @@ type OffsiteReconstituteResult struct { // restore that had silently dropped a 1.4 MB volume archive — a true sentence leaving a false // impression, which is the shape this surface keeps having removed from it. VolumesReplayed int - SafetyDump string // path of the pre-restore dump (the undo), "" when the app has no DB - DumpsAt time.Time // when the snapshot's DB half was taken (zero = unknown/legacy unit) - OffsiteRunID string // "" for a pre-v0.148 snapshot — an unverified pair - Skewed bool // the snapshot carries no coherence stamp: files and DB may differ in age - LooksEmpty bool // R-44 sniff on the dump about to be replayed + SafetyDump string // path of the pre-restore dump (the undo), "" when the app has no DB + // RolledBack (R-379) is true when the database replay FAILED and this run put the customer's own + // pre-restore copy back. It is on the result rather than inferred from the error, because "the + // restore failed" and "your data is as it was" are two different facts and the surface has to be + // able to say both. + RolledBack bool + DumpsAt time.Time // when the snapshot's DB half was taken (zero = unknown/legacy unit) + OffsiteRunID string // "" for a pre-v0.148 snapshot — an unverified pair + Skewed bool // the snapshot carries no coherence stamp: files and DB may differ in age + LooksEmpty bool // R-44 sniff on the dump about to be replayed // Placement (R-351) is what the backup recorded about where this app's data lived, compared // against where this restore actually wrote. Carried on the RESULT and not only on the refusal, // so a restore that proceeded into a different destination says so in its own outcome rather @@ -112,13 +120,43 @@ func rsyncRestoreOverwrite(src, dst string) (int, error) { return countRestoredFiles(string(out)), nil } +// safetyDumpSet is what ONE reconstitution's undo consists of: the stamp that identifies this run's +// files, and one written path per database the app has. +// +// R-379: it exists because `writeSafetyDump` used to return only the FIRST path, and the rollback +// added in v0.220.0 must re-apply EVERY database's undo or it restores one and leaves the other +// half-written — the defect it exists to close, one database over. The stamp is the IDENTITY: three +// runs against `docmost` on 2026-08-22 left three `pre-restore-*` files in the same directory, so +// matching on the prefix would replay an arbitrary older state. Match on this stamp, never on the +// prefix, never on age or size. +type safetyDumpSet struct { + Stamp string // 20060102T150405Z — this run's, and only this run's + Files []safetyDumpFile // one per database, in discovery order +} + +// safetyDumpFile pairs an undo file with the database it came from, so the rollback can hand each +// dump back to the container it belongs to instead of guessing from the filename. +type safetyDumpFile struct { + DB DiscoveredDB + Path string +} + +// First returns the first written path, or "" — the value the pre-v0.220.0 signature returned, kept +// because the customer-facing message names one file and changing that is not this task. +func (s safetyDumpSet) First() string { + if len(s.Files) == 0 { + return "" + } + return s.Files[0].Path +} + // writeSafetyDump dumps every live database of stack into the app's unit db-dumps dir under the -// `pre-restore-` prefix, and returns the first dump's path. Returns ("", nil) when the app has no +// `pre-restore-` prefix, and returns the SET it wrote. Returns (zero, nil) when the app has no // database at all — a no-DB app has nothing to undo and must flow exactly as it did before // v0.148.0 (no dump, no replay, no behaviour change). // // A discovered database that CANNOT be dumped is a hard error: it means the undo would not exist. -func (m *Manager) writeSafetyDump(ctx context.Context, stackName, nsRoot string) (string, error) { +func (m *Manager) writeSafetyDump(ctx context.Context, stackName, nsRoot string) (safetyDumpSet, error) { discover := m.discoverDBs if discover == nil { discover = func(ctx context.Context) ([]DiscoveredDB, error) { @@ -127,7 +165,7 @@ func (m *Manager) writeSafetyDump(ctx context.Context, stackName, nsRoot string) } dbs, err := discover(ctx) if err != nil { - return "", fmt.Errorf("a biztonsági mentés előtt nem sikerült felderíteni az adatbázisokat: %w", err) + return safetyDumpSet{}, fmt.Errorf("a biztonsági mentés előtt nem sikerült felderíteni az adatbázisokat: %w", err) } var mine []DiscoveredDB for _, db := range dbs { @@ -136,34 +174,185 @@ func (m *Manager) writeSafetyDump(ctx context.Context, stackName, nsRoot string) } } if len(mine) == 0 { - return "", nil // no DB → nothing to undo → scenario E flows unchanged + return safetyDumpSet{}, nil // no DB → nothing to undo → scenario E flows unchanged } dumpDir := AppDBDumpPath(nsRoot, stackName) if err := os.MkdirAll(dumpDir, 0755); err != nil { - return "", fmt.Errorf("a biztonsági mentés könyvtára nem hozható létre: %w", err) + return safetyDumpSet{}, fmt.Errorf("a biztonsági mentés könyvtára nem hozható létre: %w", err) } - stamp := time.Now().UTC().Format("20060102T150405Z") - first := "" + set := safetyDumpSet{Stamp: time.Now().UTC().Format("20060102T150405Z")} for _, db := range mine { res := m.dumpForSafety(ctx, db, dumpDir) if res.Error != nil { - return "", fmt.Errorf("a jelenlegi adatbázis biztonsági mentése sikertelen (%s): %w — a visszaállítás nem indult el", db.ContainerName, res.Error) + return safetyDumpSet{}, fmt.Errorf("a jelenlegi adatbázis biztonsági mentése sikertelen (%s): %w — a visszaállítás nem indult el", db.ContainerName, res.Error) } // DumpOne writes `-.sql`; rename it under the safety prefix so it can never be // picked up as a replay SOURCE and can never overwrite the app's real dump. - safe := filepath.Join(dumpDir, fmt.Sprintf("%s%s-%s-%s.sql", preRestoreDumpPrefix, stamp, stackName, db.DBType)) + safe := filepath.Join(dumpDir, fmt.Sprintf("%s%s-%s-%s.sql", preRestoreDumpPrefix, set.Stamp, stackName, db.DBType)) if res.FilePath != safe { if err := os.Rename(res.FilePath, safe); err != nil { - return "", fmt.Errorf("a biztonsági mentés véglegesítése sikertelen: %w", err) + return safetyDumpSet{}, fmt.Errorf("a biztonsági mentés véglegesítése sikertelen: %w", err) } } - if first == "" { - first = safe - } + // EVERY file, not just the first — R-379, and the reason is on safetyDumpSet. + set.Files = append(set.Files, safetyDumpFile{DB: db, Path: safe}) m.logger.Printf("[INFO] [offbox] %s: pre-restore safety dump written → %s (%s)", stackName, filepath.Base(safe), humanizeBytes(res.Size)) } - return first, nil + return set, nil +} + +// maxUndoCopiesPerApp is how many `pre-restore-` copies an app keeps. +// +// THREE, and the reasoning rather than a number pulled from the air. One is not enough: the case +// that needs an undo is a restore that went wrong, and the second-guess attempt is exactly when the +// customer reaches for the state before the FIRST attempt. Many is not free: they live inside the +// recovery unit, so every one is also mirrored to Tier 2 AND pushed off-site permanently — four +// accumulated on `docmost` in a single afternoon on 2026-08-22 (135 KB + 135 KB + 141 KB + 138 KB), +// each of them forever. Three keeps two prior attempts and bounds the off-site growth. +const maxUndoCopiesPerApp = 3 + +// pruneUndoCopies keeps the newest maxUndoCopiesPerApp undo copies for an app and removes the rest. +// +// DELIBERATELY NOT CALLED FROM THE RESTORE PATH. A delete on the failure path is how an undo goes +// missing at exactly the moment it is needed; this runs from the capture side, where nothing is +// depending on the files right now. It is called AFTER a successful capture, never before one. +// +// Ordering is by the stamp IN THE FILENAME, not by mtime and never by size: mtime moves when a file +// is copied or a filesystem is restored, and the stamp is the identity writeSafetyDump assigned. +// The newest is never a deletion candidate even if the list is somehow malformed. +func (m *Manager) pruneUndoCopies(dumpDir, stack string) { + entries, err := os.ReadDir(dumpDir) + if err != nil { + return + } + type undo struct{ name, stamp string } + var undos []undo + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".sql") { + continue + } + base := strings.TrimSuffix(e.Name(), ".sql") + if !strings.HasPrefix(base, preRestoreDumpPrefix) { + continue + } + after := strings.TrimPrefix(base, preRestoreDumpPrefix) + i := strings.Index(after, "-") + if i <= 0 { + continue // not the shape writeSafetyDump writes — leave it alone rather than guess + } + undos = append(undos, undo{name: e.Name(), stamp: after[:i]}) + } + if len(undos) <= maxUndoCopiesPerApp { + return + } + sort.Slice(undos, func(a, b int) bool { return undos[a].stamp > undos[b].stamp }) // newest first + for _, u := range undos[maxUndoCopiesPerApp:] { + p := filepath.Join(dumpDir, u.name) + if err := os.Remove(p); err != nil { + m.logger.Printf("[WARN] [backup] %s: could not prune old undo copy %s: %v", stack, u.name, err) + continue + } + m.logger.Printf("[INFO] [backup] %s: pruned old undo copy %s (keeping the newest %d)", stack, u.name, maxUndoCopiesPerApp) + } +} + +// RestoreHoldFor reports whether an app is being held stopped after a failed restore + failed +// rollback, and returns the customer-facing reason. Every start path consults this — the customer's +// button, the app-stop Recover() starter, and the boot reconciler — because a hold that only one +// path honours is not a hold. +// +// Nil settings ⇒ NOT held. That direction is deliberate and is the opposite of the usual fail-closed +// rule: with no settings there is no hold recorded, so refusing every start would strand every app +// on a misconfigured box. The write side logs loudly when it cannot persist (see +// holdAppAfterFailedRollback), which is where that case is caught. +func (m *Manager) RestoreHoldFor(stack string) (bool, string) { + if m == nil || m.settings == nil { + return false, "" + } + h, ok := m.settings.GetRestoreHold(stack) + if !ok { + return false, "" + } + when := h.At + if t, err := time.Parse(time.RFC3339, h.At); err == nil { + when = t.Format("2006-01-02 15:04") + } + return true, fmt.Sprintf("a(z) %s adatainak visszaállítása %s-kor megszakadt, és a korábbi állapotot sem sikerült visszatölteni. "+ + "Az alkalmazás biztonsági okból leállítva marad, hogy az adatai ne sérüljenek tovább. Vedd fel velünk a kapcsolatot", stack, when) +} + +// holdAppAfterFailedRollback records the R-379/R-380 hold and makes sure nothing restarts the app +// behind our back. +// +// OPERATOR RULING, 2026-08-22: when the replay fails AND the rollback fails, the app is HELD +// STOPPED rather than started. A running app on a half-written database lets the customer type into +// it, and that turns a recoverable state into a permanent one. The alternative — start it and mark +// it — was considered and declined. +// +// It ENDS the app-stop marker deliberately. The marker means "owed a restart"; a held app is not +// owed one, and leaving the marker active would have Recover() start the broken app at the next +// controller boot. The hold is the thing that persists, not the marker. +func (m *Manager) holdAppAfterFailedRollback(stack string, replayErr, rollbackErr error) { + if m.settings == nil { + m.logger.Printf("[ERROR] [offbox] %s: cannot persist the restore hold — no settings wired; the app is stopped but NOTHING will refuse a restart", stack) + return + } + h := settings.RestoreHold{ + Stack: stack, + At: time.Now().UTC().Format(time.RFC3339), + } + if replayErr != nil { + h.ReplayError = replayErr.Error() + } + if rollbackErr != nil { + h.RollbackErr = rollbackErr.Error() + } + if err := m.settings.SetRestoreHold(h); err != nil { + m.logger.Printf("[ERROR] [offbox] %s: persisting the restore hold FAILED: %v — the app is stopped and unguarded", stack, err) + } + // The app is not owed a restart; it is deliberately held. See the doc comment. + if m.appStop != nil { + m.appStop.End() + } + if m.restoreHoldNotify != nil { + m.restoreHoldNotify(stack, replayErr, rollbackErr) + } +} + +// SetRestoreHoldNotify wires the operator notification for a held app (cmd/controller/main.go). +func (m *Manager) SetRestoreHoldNotify(fn func(stack string, replayErr, rollbackErr error)) { + m.restoreHoldNotify = fn +} + +// rollbackSafetyDump re-applies THIS RUN's undo set, database by database, and is the whole of +// R-379's fix: it is the same ImportDump call a person made by hand on 2026-08-22 to recover +// `docmost` and `bookstack` after a failed replay, moved into the product. +// +// It runs with the DB service still up (the replay's own window) and BEFORE any restart, so the +// app never observes the half-written state. An error here means the app cannot be trusted to run — +// see the hold in ReconstituteFromOffsite. +func (m *Manager) rollbackSafetyDump(ctx context.Context, stack string, set safetyDumpSet) error { + if len(set.Files) == 0 { + return nil + } + imp := m.rollbackImport + if imp == nil { + imp = func(ctx context.Context, db DiscoveredDB, path string) error { + return ImportDump(ctx, db, path, m.logger, m.isDebug()) + } + } + for _, f := range set.Files { + if _, sErr := os.Stat(f.Path); sErr != nil { + return fmt.Errorf("a visszavonáshoz szükséges mentés nem található (%s): %w", filepath.Base(f.Path), sErr) + } + m.logger.Printf("[INFO] [offbox] %s: rolling back to the pre-restore state from %s", stack, filepath.Base(f.Path)) + if err := imp(ctx, f.DB, f.Path); err != nil { + return fmt.Errorf("a korábbi állapot visszaállítása sikertelen (%s): %w", f.DB.ContainerName, err) + } + } + m.logger.Printf("[INFO] [offbox] %s: rollback complete — %d database(s) returned to the pre-restore state", stack, len(set.Files)) + return nil } // dumpForSafety is the DumpOne seam for the safety dump (tests inject; nil → the real DumpOne). @@ -325,10 +514,11 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack // --- THE UNDO, BEFORE THE ACT --------------------------------------------------------------- // Taken while the stack is still UP (a stopped database cannot be dumped) and before a single // byte is overwritten, so a failure here aborts with the live app completely untouched. - safety, err := m.writeSafetyDump(ctx, stack, liveNs) + safetySet, err := m.writeSafetyDump(ctx, stack, liveNs) if err != nil { return res, err } + safety := safetySet.First() res.SafetyDump = safety hasDB := safety != "" if hasDB { @@ -436,10 +626,39 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack n, iErr := m.reimportDBDumpsFrom(ctx, stack, scratchDumpDir) res.DBsReplayed = n if iErr != nil { - if sErr := restartStack(); sErr != nil { - m.logger.Printf("[WARN] [offbox] %s: full start after failed replay also failed: %v", stack, sErr) + // --- R-379/R-380: PUT THE CUSTOMER'S OWN COPY BACK --------------------------------- + // Until v0.220.0 this branch restarted the app onto a HALF-WRITTEN database and named + // the undo file in the message. Measured 2026-08-22 on demo-hp: Postgres was left + // emptied and crash-looping; MariaDB was left partly applied while the app reported + // `health=healthy`. Both are the same failure — a half state — and the only difference + // was whether it looked broken. MariaDB's structural statements are not transactional, + // so no engine flag can prevent the half state; putting the undo back is what removes + // it. This is the same ImportDump call a person ran by hand that day to recover both + // apps, moved into the product. + // + // The rollback runs BEFORE any restart and with the DB service still up, so the app + // never observes the half state. The ORIGINAL replay error is never swallowed: it is + // logged here in full and named in the customer's sentence. + m.logger.Printf("[ERROR] [offbox] %s: database replay failed, rolling back to the pre-restore state: %v", stack, iErr) + if rbErr := m.rollbackSafetyDump(ctx, stack, safetySet); rbErr != nil { + // BOTH failed. Do NOT start the app: a running app on a half-written database lets + // the customer type into it and makes the damage permanent. Hold it instead — + // operator ruling, 2026-08-22. + m.logger.Printf("[ERROR] [offbox] %s: ROLLBACK ALSO FAILED (%v) — holding the app stopped; replay error was: %v", stack, rbErr, iErr) + m.holdAppAfterFailedRollback(stack, iErr, rbErr) + return res, fmt.Errorf("a(z) %s adatbázisának visszaállítása sikertelen, és a korábbi állapot visszatöltése sem sikerült. "+ + "Az alkalmazást biztonsági okból LEÁLLÍTVA hagytuk, hogy az adatai ne sérüljenek tovább. "+ + "Vedd fel velünk a kapcsolatot — a korábbi állapot mentése megvan: %s", stack, filepath.Base(safety)) } - return res, fmt.Errorf("az adatbázis visszaállítása sikertelen: %w — a korábbi állapot mentése megvan: %s", iErr, filepath.Base(safety)) + res.RolledBack = true + if sErr := restartStack(); sErr != nil { + m.logger.Printf("[WARN] [offbox] %s: start after a successful rollback failed: %v", stack, sErr) + } + // Says BOTH things. A message that reported only the failure would leave the customer + // believing their data was gone when it is back — the omission of a GAIN is as + // misleading as the omission of a loss. + return res, fmt.Errorf("a(z) %s adatbázisának visszaállítása sikertelen — az adataid visszakerültek a visszaállítás előtti állapotba, "+ + "az alkalmazás fut tovább. Ha újra megpróbálnád, előbb vedd fel velünk a kapcsolatot", stack) } } if err := restartStack(); err != nil { @@ -449,8 +668,11 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack m.logger.Printf("[WARN] [offbox] %s reconstituted but health check failed: %v", stack, err) } - m.logger.Printf("[INFO] [offbox] reconstituted %s from snapshot %s: %d file(s) placed, %d DB dump(s) replayed, safety dump=%s, skewed=%v", - stack, id, res.FilesPlaced, res.DBsReplayed, filepath.Base(safety), res.Skewed) + // R-382: VolumesReplayed was set above and never printed, so the operator log said + // "0 file(s) placed, 1 DB dump(s) replayed" on a run that returned a 52 MB Postgres data + // directory — less informative than the customer's own flash, which already named the volumes. + m.logger.Printf("[INFO] [offbox] reconstituted %s from snapshot %s: %d file(s) placed, %d volume(s) replayed, %d DB dump(s) replayed, safety dump=%s, skewed=%v", + stack, id, res.FilesPlaced, res.VolumesReplayed, res.DBsReplayed, filepath.Base(safety), res.Skewed) return res, nil } diff --git a/controller/internal/backup/offbox_restore.go b/controller/internal/backup/offbox_restore.go index 3457277..1b76ba7 100644 --- a/controller/internal/backup/offbox_restore.go +++ b/controller/internal/backup/offbox_restore.go @@ -35,6 +35,12 @@ func (m *Manager) SetOffboxFullPlaceCopier(fn func(src, dst string) (int, error) m.offboxFullPlaceCopier = fn } +// SetRollbackImportFn overrides the ROLLBACK's ImportDump (tests; no Docker needed). Separate from +// the replay's own import seam on purpose — see the field comment on Manager.rollbackImport. +func (m *Manager) SetRollbackImportFn(fn func(ctx context.Context, db DiscoveredDB, dumpPath string) error) { + m.rollbackImport = fn +} + // SetSafetyDumpFn overrides the pre-restore safety dump (tests; no Docker needed). func (m *Manager) SetSafetyDumpFn(fn func(ctx context.Context, db DiscoveredDB, dumpDir string) DumpResult) { m.safetyDumpFn = fn diff --git a/controller/internal/backup/r355_safetydump_test.go b/controller/internal/backup/r355_safetydump_test.go index b8e23d3..2d43298 100644 --- a/controller/internal/backup/r355_safetydump_test.go +++ b/controller/internal/backup/r355_safetydump_test.go @@ -45,7 +45,11 @@ func TestR355_SafetyDumpIsTakenForTheCorrectlyAttributedApp(t *testing.T) { return DumpResult{DB: db, FilePath: p, Size: 13} } - safety, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + // v0.220.0 (R-379): writeSafetyDump returns the SET it wrote, so a rollback can re-apply EVERY + // database's undo. `.First()` is the value this signature returned before; these assertions are + // unchanged in meaning. + set, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + safety := set.First() if err != nil { t.Fatalf("writeSafetyDump: %v", err) } @@ -83,7 +87,11 @@ func TestR355_MisattributedAppGetsNoUndoCopy(t *testing.T) { return DumpResult{DB: db} } - safety, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + // v0.220.0 (R-379): writeSafetyDump returns the SET it wrote, so a rollback can re-apply EVERY + // database's undo. `.First()` is the value this signature returned before; these assertions are + // unchanged in meaning. + set, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + safety := set.First() if err != nil { t.Fatalf("writeSafetyDump: %v", err) } @@ -111,7 +119,11 @@ func TestR355_RestoreRefusesWhenTheUndoCannotBeTaken(t *testing.T) { return DumpResult{DB: db, Error: os.ErrPermission} } - safety, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + // v0.220.0 (R-379): writeSafetyDump returns the SET it wrote, so a rollback can re-apply EVERY + // database's undo. `.First()` is the value this signature returned before; these assertions are + // unchanged in meaning. + set, err := m.writeSafetyDump(context.Background(), "paperless-ngx", nsRoot) + safety := set.First() if err == nil { t.Fatal("a database that cannot be dumped must be a hard error — the undo would not exist") } diff --git a/controller/internal/backup/r379_rollback_test.go b/controller/internal/backup/r379_rollback_test.go new file mode 100644 index 0000000..6c07193 --- /dev/null +++ b/controller/internal/backup/r379_rollback_test.go @@ -0,0 +1,330 @@ +package backup + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-379 / R-380. When an off-site database replay failed, the app was restarted onto a HALF-WRITTEN +// database and the customer was shown the undo copy's filename — a file nothing in the product could +// apply. Measured live on demo-hp 2026-08-22: Postgres left emptied and crash-looping, MariaDB left +// partly applied while `docker inspect` reported health=healthy. Both are the same failure — a half +// state — and the only difference was whether it looked broken. +// +// The fix puts the customer's own pre-restore copy back automatically. These tests pin that, the +// double-failure hold, and the two absence claims that go with them. + +// rollbackFixture is reconFixture with the replay failing and the rollback injectable. +func rollbackFixture(t *testing.T, rollbackErr error) (*Manager, *recordingProvider, *int) { + t.Helper() + m, prov, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + m.importDBDump = func(context.Context, DiscoveredDB, string) error { + return errors.New("replay blew up") + } + calls := 0 + m.SetRollbackImportFn(func(_ context.Context, _ DiscoveredDB, path string) error { + calls++ + // The rollback must be handed THIS RUN's undo file, not the scratch's dump. + if !strings.Contains(filepath.Base(path), preRestoreDumpPrefix) { + t.Errorf("rollback was handed %q — that is not an undo copy", filepath.Base(path)) + } + return rollbackErr + }) + return m, prov, &calls +} + +// SCENARIO A — replay fails, rollback succeeds. The app comes back and the outcome records it. +// +// WRONG OUTCOMES PINNED: the app left down; or a message that reports the failure and omits that the +// data is back — the omission of a GAIN is as misleading as the omission of a loss, and here it is +// the difference between a customer who panics and one who does not. +func TestR379_ScenarioA_RollbackSucceeds_AppComesBack(t *testing.T) { + m, prov, calls := rollbackFixture(t, nil) + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) + if err == nil { + t.Fatal("a failed replay must still be surfaced as a failure") + } + if *calls != 1 { + t.Fatalf("the undo copy must be re-applied exactly once, got %d", *calls) + } + if !res.RolledBack { + t.Error("the result must record that the rollback happened") + } + if !prov.fullStarted { + t.Fatal("after a successful rollback the app must be started — it is in a known good state") + } + // The customer sentence says BOTH things. + low := err.Error() + if !strings.Contains(low, "sikertelen") { + t.Errorf("the message must say the restore failed; got: %v", err) + } + if !strings.Contains(low, "visszakerültek") { + t.Errorf("the message must say the data is back as it was; got: %v", err) + } + // And no hold was written — the app is fine. + if held, _ := m.RestoreHoldFor("immich"); held { + t.Error("a recovered app must NOT be held") + } +} + +// SCENARIO C — replay fails AND rollback fails. The app is held, not started. +// +// OPERATOR RULING 2026-08-22: a running app on a half-written database lets the customer type into +// it and makes the damage permanent. +// +// WRONG OUTCOMES PINNED: the app started anyway; or the app-stop marker left active, which would +// have Recover() start the broken app at the next controller boot, quietly, hours later. +func TestR379_ScenarioC_BothFail_AppIsHeldNotStarted(t *testing.T) { + m, prov, calls := rollbackFixture(t, errors.New("rollback blew up too")) + notified := 0 + m.SetRestoreHoldNotify(func(string, error, error) { notified++ }) + + _, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) + if err == nil { + t.Fatal("a double failure must be surfaced") + } + if *calls != 1 { + t.Fatalf("the rollback must have been attempted, got %d calls", *calls) + } + // THE OBSERVABLE THAT MATTERS. + if prov.fullStarted { + t.Fatal("the app was STARTED onto a half-written database — the exact outcome the hold exists to prevent") + } + held, why := m.RestoreHoldFor("immich") + if !held { + t.Fatal("the hold must be persisted, or nothing will refuse a restart later") + } + if why == "" || !strings.Contains(why, "kapcsolat") { + t.Errorf("the hold's reason must name a route the customer can take; got %q", why) + } + if notified != 1 { + t.Errorf("the operator must be told exactly once, got %d", notified) + } + if !strings.Contains(err.Error(), "LEÁLLÍTVA") { + t.Errorf("the customer must be told the app was deliberately stopped; got: %v", err) + } +} + +// SCENARIO E — the replay SUCCEEDS. Nothing new may happen. +// +// WRONG OUTCOME PINNED: a rollback firing on a successful restore would overwrite the restored data +// with the pre-restore state — a silent, total loss of the thing the customer asked for. +func TestR379_ScenarioE_SuccessfulReplay_NoRollbackNoHold(t *testing.T) { + m, prov, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + rolled := 0 + m.SetRollbackImportFn(func(context.Context, DiscoveredDB, string) error { rolled++; return nil }) + held := 0 + m.SetRestoreHoldNotify(func(string, error, error) { held++ }) + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) + if err != nil { + t.Fatalf("a clean restore must succeed: %v", err) + } + if rolled != 0 { + t.Fatalf("a rollback fired on a SUCCESSFUL restore — the restored data would be overwritten; calls=%d", rolled) + } + if res.RolledBack { + t.Error("a successful restore must not report a rollback") + } + if held != 0 { + t.Errorf("no hold may be written on success, got %d", held) + } + if isHeld, _ := m.RestoreHoldFor("immich"); isHeld { + t.Error("a successful restore must leave no hold") + } + if !prov.fullStarted { + t.Error("a successful restore still starts the app") + } +} + +// SCENARIO E, POSITIVE CONTROL. "No rollback fired" is an absence claim, so prove the counter can +// count: the same seam, driven through the failure path, must register. +func TestR379_ScenarioE_PositiveControl_TheCounterCanCount(t *testing.T) { + m, _, calls := rollbackFixture(t, nil) + if _, err := m.ReconstituteFromOffsite(context.Background(), "immich", false); err == nil { + t.Fatal("fixture: the replay must fail here") + } + if *calls == 0 { + t.Fatal("the rollback counter never increments — Scenario E's zero would have proven nothing") + } +} + +// SCENARIO F — an app with no database. Untouched in every respect. +func TestR379_ScenarioF_NoDatabase_Unchanged(t *testing.T) { + m, prov, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", "") + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return nil, nil } + rolled := 0 + m.SetRollbackImportFn(func(context.Context, DiscoveredDB, string) error { rolled++; return nil }) + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) + if err != nil { + t.Fatalf("a no-DB app must restore: %v", err) + } + if rolled != 0 || res.RolledBack { + t.Error("a no-DB app has nothing to undo and nothing to roll back") + } + if res.SafetyDump != "" { + t.Errorf("a no-DB app takes no undo copy, got %q", res.SafetyDump) + } + if held, _ := m.RestoreHoldFor("immich"); held { + t.Error("a no-DB app must never be held") + } + if !prov.fullStarted { + t.Error("a no-DB app still starts") + } +} + +// SCENARIO G — two databases, one replay fails. The WHOLE undo set is re-applied. +// +// WRONG OUTCOME PINNED: only the first. writeSafetyDump used to return one path for an app with two +// databases, so a rollback built on that value would restore one and leave the other half-written — +// this defect, one database over. +func TestR379_ScenarioG_TwoDatabases_WholeSetRolledBack(t *testing.T) { + m, _, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + two := []DiscoveredDB{ + {StackName: "immich", DBType: DBTypePostgres, ContainerName: "immich-postgres", ContainerID: "a"}, + {StackName: "immich", DBType: DBTypeMariaDB, ContainerName: "immich-maria", ContainerID: "b"}, + } + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return two, nil } + m.safetyDumpFn = func(_ context.Context, db DiscoveredDB, dumpDir string) DumpResult { + p := filepath.Join(dumpDir, "immich-"+string(db.DBType)+".sql") + if err := os.WriteFile(p, []byte(pgDump(1)), 0o644); err != nil { + t.Fatal(err) + } + return DumpResult{DB: db, FilePath: p, Size: 42} + } + m.importDBDump = func(context.Context, DiscoveredDB, string) error { return errors.New("replay failed") } + + var rolled []string + m.SetRollbackImportFn(func(_ context.Context, db DiscoveredDB, path string) error { + rolled = append(rolled, db.ContainerName+":"+filepath.Base(path)) + return nil + }) + + if _, err := m.ReconstituteFromOffsite(context.Background(), "immich", false); err == nil { + t.Fatal("the replay failure must surface") + } + if len(rolled) != 2 { + t.Fatalf("BOTH databases must be rolled back, got %d: %v", len(rolled), rolled) + } + // Each database got its OWN undo file, matched by identity rather than by prefix. + if !strings.Contains(rolled[0], "immich-postgres:") || !strings.Contains(rolled[1], "immich-maria:") { + t.Errorf("each database must get its own undo copy, got %v", rolled) + } + for _, r := range rolled { + if !strings.Contains(r, preRestoreDumpPrefix) { + t.Errorf("rollback used a non-undo file: %s", r) + } + } +} + +// The undo set is matched on THIS RUN's stamp. Three undo copies from three different runs +// accumulated on docmost in one afternoon; a prefix match would replay an arbitrary older state. +func TestR379_UndoSetIsThisRunOnly(t *testing.T) { + nsRoot := t.TempDir() + m := newSafetyTestManager() + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { + return []DiscoveredDB{{StackName: "app", DBType: DBTypePostgres, ContainerName: "app-postgres", ContainerID: "c"}}, nil + } + m.safetyDumpFn = func(_ context.Context, db DiscoveredDB, dumpDir string) DumpResult { + p := filepath.Join(dumpDir, "app-postgres.sql") + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + return DumpResult{DB: db, FilePath: p, Size: 1} + } + // An OLDER undo copy from a previous run, already on disk. + dumpDir := AppDBDumpPath(nsRoot, "app") + if err := os.MkdirAll(dumpDir, 0o755); err != nil { + t.Fatal(err) + } + stale := filepath.Join(dumpDir, preRestoreDumpPrefix+"20200101T000000Z-app-postgres.sql") + if err := os.WriteFile(stale, []byte("STALE"), 0o644); err != nil { + t.Fatal(err) + } + + set, err := m.writeSafetyDump(context.Background(), "app", nsRoot) + if err != nil { + t.Fatal(err) + } + if len(set.Files) != 1 { + t.Fatalf("one database → one undo file, got %d", len(set.Files)) + } + if strings.Contains(set.Files[0].Path, "20200101") { + t.Fatal("the set picked up an OLDER run's undo copy — it must carry only this run's stamp") + } + if set.Stamp == "" || strings.Contains(set.Files[0].Path, set.Stamp) == false { + t.Errorf("the file must carry this run's stamp %q, got %q", set.Stamp, set.Files[0].Path) + } +} + +// pruneUndoCopies keeps the newest N and never the oldest — ordered by the STAMP, not by mtime. +func TestR379_PruneKeepsTheNewestByStamp(t *testing.T) { + m := newSafetyTestManager() + dir := t.TempDir() + stamps := []string{"20260101T000000Z", "20260201T000000Z", "20260301T000000Z", "20260401T000000Z", "20260501T000000Z"} + for _, st := range stamps { + if err := os.WriteFile(filepath.Join(dir, preRestoreDumpPrefix+st+"-app-postgres.sql"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + } + // The app's OWN dump must never be a candidate. + own := filepath.Join(dir, "app-postgres.sql") + if err := os.WriteFile(own, []byte("own"), 0o644); err != nil { + t.Fatal(err) + } + + m.pruneUndoCopies(dir, "app") + + if _, err := os.Stat(own); err != nil { + t.Fatal("the app's own dump was pruned — only undo copies are candidates") + } + for _, st := range stamps[len(stamps)-maxUndoCopiesPerApp:] { + if _, err := os.Stat(filepath.Join(dir, preRestoreDumpPrefix+st+"-app-postgres.sql")); err != nil { + t.Errorf("the newest %d must survive; %s is gone", maxUndoCopiesPerApp, st) + } + } + for _, st := range stamps[:len(stamps)-maxUndoCopiesPerApp] { + if _, err := os.Stat(filepath.Join(dir, preRestoreDumpPrefix+st+"-app-postgres.sql")); err == nil { + t.Errorf("the oldest must be pruned; %s survived", st) + } + } +} + +// R-381 — the customer sentence must not carry the engine's output. +// +// MEASURED 2026-08-22: 407 bytes (Postgres, with a caret diagram and `exit status 3`) and 615 bytes +// (MariaDB, whose middle was an `INSERT INTO migrations VALUES (...)` listing — ROWS OUT OF THE +// CUSTOMER'S OWN DATABASE, HTML-escaped, on their dashboard). +func TestR381_CustomerMessageCarriesNoEngineOutput(t *testing.T) { + engineNoise := "ERROR: syntax error at end of input\nLINE 1: COPY public.felhom_r356b_discriminator \n ^ — exit status 3" + m, _, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + m.importDBDump = func(context.Context, DiscoveredDB, string) error { + return errors.New(engineNoise) + } + m.SetRollbackImportFn(func(context.Context, DiscoveredDB, string) error { return nil }) + + _, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) + if err == nil { + t.Fatal("the failure must surface") + } + msg := err.Error() + for _, leak := range []string{"ERROR: syntax", "LINE 1:", "COPY public.", "exit status"} { + if strings.Contains(msg, leak) { + t.Errorf("the customer message leaks engine output %q; got: %s", leak, msg) + } + } + if len(msg) > 320 { + t.Errorf("the customer message is %d bytes — it is a sentence, not a transcript: %s", len(msg), msg) + } + // POSITIVE CONTROL: the noise really was in the error the code received, so the absence above is + // the message being clean rather than the noise never existing. + if !strings.Contains(engineNoise, "exit status") { + t.Fatal("fixture: the planted noise does not contain the marker this test greps for") + } +} diff --git a/controller/internal/backup/r47_replay_order_test.go b/controller/internal/backup/r47_replay_order_test.go index bfe48a6..446ed40 100644 --- a/controller/internal/backup/r47_replay_order_test.go +++ b/controller/internal/backup/r47_replay_order_test.go @@ -264,11 +264,19 @@ func TestRestoreFromUnitIgnoresSafetyDumpsWhenDecidingToReplay(t *testing.T) { // TestReconstituteReplayFailureStillBringsTheStackUp: the DB-only window is a deliberate half-started // state, so EVERY exit from it must end in a full start. Otherwise a failed restore leaves the // customer with a running database and no application — an outage caused by the recovery tool. +// +// UPDATED FOR v0.220.0 (R-379). The requirement is unchanged and still asserted; what changed is +// what happens BETWEEN the failure and the full start. A failed replay now re-applies the customer's +// own pre-restore copy first, and only then starts. The rollback seam is injected as SUCCEEDING here +// because that is this test's subject; the double-failure path — where the app is deliberately NOT +// started — is Scenario C and has its own test in r379_rollback_test.go. func TestReconstituteReplayFailureStillBringsTheStackUp(t *testing.T) { m, prov, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) m.importDBDump = func(context.Context, DiscoveredDB, string) error { return context.DeadlineExceeded } + rolledBack := 0 + m.SetRollbackImportFn(func(context.Context, DiscoveredDB, string) error { rolledBack++; return nil }) res, err := m.ReconstituteFromOffsite(context.Background(), "immich", false) if err == nil { @@ -280,9 +288,16 @@ func TestReconstituteReplayFailureStillBringsTheStackUp(t *testing.T) { if got := strings.Join(prov.calls, ","); got != "stop,startsvc:immich-postgres,start" { t.Fatalf("sequence = %q, want the best-effort full start after the failure", got) } - // The existing message shape stays: the operator needs the undo's filename. - if !strings.Contains(err.Error(), filepath.Base(res.SafetyDump)) { - t.Fatalf("the error must name the safety dump so the operator can undo, got: %v", err) + if rolledBack != 1 { + t.Fatalf("the customer's pre-restore copy must be put back before the start; rollback calls = %d", rolledBack) + } + if !res.RolledBack { + t.Error("the outcome must record that a rollback happened — the surface has to be able to say the data is back") + } + // v0.220.0: the customer sentence now states the OUTCOME (their data is as it was) instead of a + // filename. The filename remains for the operator, in the log and on the result. + if res.SafetyDump == "" || !strings.Contains(filepath.Base(res.SafetyDump), preRestoreDumpPrefix) { + t.Fatalf("the result must still carry the undo copy for the operator, got %q", res.SafetyDump) } } diff --git a/controller/internal/backup/recovery_unit.go b/controller/internal/backup/recovery_unit.go index 1876d84..0f3438d 100644 --- a/controller/internal/backup/recovery_unit.go +++ b/controller/internal/backup/recovery_unit.go @@ -195,6 +195,11 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { stackName, RecoveryUnitPath(nsRoot, stackName), len(info.ImagePins), len(info.SecretEnvVars), len(info.DataKeyEnvVars), len(info.PortableSecrets), len(info.PortableSecretEnvVars), len(withheldSecretNames(info))) + + // R-379: bound the undo copies. AFTER a successful capture and never before one — the capture is + // the point at which nothing is depending on those files, whereas the restore path is precisely + // where deleting one would remove the undo at the moment it is needed. + m.pruneUndoCopies(AppDBDumpPath(nsRoot, stackName), stackName) return nil } diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index 82435ac..4981721 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -164,6 +164,20 @@ type Settings struct { // Storage paths registry StoragePaths []StoragePath `json:"storage_paths,omitempty"` + // RestoreHolds (R-379/R-380, v0.220.0) records apps this controller is DELIBERATELY keeping + // stopped because a database restore failed AND the rollback to the customer's own pre-restore + // copy also failed. Keyed by stack name. + // + // WHY THIS AND NOT `AppConfig.DesiredState`: that field is the CUSTOMER'S stated intent — what + // they asked for. Writing our own failure into it would make our fault indistinguishable from + // their choice, which is the exact confusion its own comment says it exists to end. The fenced + // act is "writing DesiredState from the restore path"; reading it stays fine. + // + // WHY NOT the app-stop marker: that marker means "owed a restart". A held app is not owed one — + // leaving the marker active would have Recover() start the broken app at the next controller + // boot, hours later and quietly, which is the outcome the hold exists to prevent. + RestoreHolds map[string]RestoreHold `json:"restore_holds,omitempty"` + // Cross-drive restic repo password (auto-generated on first use) CrossDriveResticPassword string `json:"cross_drive_restic_password,omitempty"` @@ -1515,6 +1529,65 @@ func InferStorageLabel(path string) string { return fmt.Sprintf("Tárhely (%s)", base) } +// RestoreHold is one app held stopped after a failed restore whose rollback also failed. It carries +// the reason so every refusal can NAME it — a refusal without a reason and a route is how a customer +// is left with nothing to do. +type RestoreHold struct { + Stack string `json:"stack"` + At string `json:"at"` // RFC3339 UTC + ReplayError string `json:"replay_error,omitempty"` // what the restore hit + RollbackErr string `json:"rollback_error,omitempty"` // what the rollback then hit + SafetyDump string `json:"safety_dump,omitempty"` // basename of the undo copy that could not be applied +} + +// SetRestoreHold records a hold. Modelled on SetDisconnected: a condition, plus what it is holding. +func (s *Settings) SetRestoreHold(h RestoreHold) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.RestoreHolds == nil { + s.RestoreHolds = map[string]RestoreHold{} + } + s.RestoreHolds[h.Stack] = h + if s.log != nil { + s.log.Printf("[WARN] [settings] restore hold SET for %s — the app stays stopped until it is cleared", h.Stack) + } + return s.save() +} + +// GetRestoreHold returns the hold for a stack, if one is in force. +func (s *Settings) GetRestoreHold(stack string) (RestoreHold, bool) { + s.mu.RLock() + defer s.mu.RUnlock() + h, ok := s.RestoreHolds[stack] + return h, ok +} + +// ListRestoreHolds returns every hold in force. +func (s *Settings) ListRestoreHolds() []RestoreHold { + s.mu.RLock() + defer s.mu.RUnlock() + out := make([]RestoreHold, 0, len(s.RestoreHolds)) + for _, h := range s.RestoreHolds { + out = append(out, h) + } + return out +} + +// ClearRestoreHold removes a hold. Returns false when there was none, so a caller can tell "cleared" +// from "there was nothing to clear" rather than reporting success either way. +func (s *Settings) ClearRestoreHold(stack string) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + if _, ok := s.RestoreHolds[stack]; !ok { + return false, nil + } + delete(s.RestoreHolds, stack) + if s.log != nil { + s.log.Printf("[INFO] [settings] restore hold CLEARED for %s", stack) + } + return true, s.save() +} + // SetDisconnected marks a storage path as disconnected (or connected) and records which stacks were stopped. func (s *Settings) SetDisconnected(path string, disconnected bool, stoppedStacks []string) error { s.mu.Lock() diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index e747d47..68ce09b 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -1157,6 +1157,11 @@ type AppBackupRow struct { // e.g., "DB + Konfiguráció + Adatok", "DB + Konfiguráció", "Konfiguráció" BackupContents string + // RestoreHeld (R-379) — this app is deliberately stopped because a database restore failed AND + // the rollback failed. Distinct from any backup status: it is about the app's LIVE data, not its + // copies, which is why it drives the row red rather than yellow. + RestoreHeld bool + // Tier 1: Nightly backup (always exists) Tier1LastRun string // RFC3339 time of the newest recovery-unit artifact ("" = no unit yet) Tier1LastStatus string // "ok", "error", "" @@ -1361,6 +1366,16 @@ func (s *Server) buildAppBackupRows(status *backup.FullBackupStatus) []AppBackup row.Status = "yellow" row.StatusText = "Adatbázis mentés sikertelen" } + // R-379/R-380: a HELD app must never read as healthy. Last, so it wins over both branches + // above — a warning beside a green tick is read as a success, and this is the one state where + // the customer's data may not be intact. Measured 2026-08-22: after a failed MariaDB replay + // the app reported `health=healthy, running=true, restarts=0` while its schema-version table + // held zero rows. + if held, why := s.backupMgr.RestoreHoldFor(app.StackName); held { + row.Status = "red" + row.StatusText = why + row.RestoreHeld = true + } // Tier 2 (off-drive copy) status, from the config the Tier 2 runner persists. if cd := s.settings.GetCrossDriveConfig(app.StackName); cd != nil {