diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aa32d0..ebd530e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,47 @@ +## v0.221.0 — taking the undo copy destroyed the app's own database backup (2026-08-22, R-361) +**MinAgent: 0.129.0** (unchanged — no new agent coupling) + +**A comment asserted an invariant the code did not have, and the comment was believed for four +months.** `writeSafetyDump` called `DumpOne` into the app's OWN unit directory and renamed the result +to `pre-restore-*` afterwards. `DumpOne` writes `-.sql` — **the app's canonical dump, +the name the replay loop matches exactly** — so every safety dump **overwrote the app's real backup +and then moved it away**. The comment beside it said the rename meant it *"can never overwrite the +app's real dump"*. It was false as written. + +**The consequence, not the mechanism:** between a restore and the next nightly run the app had **no +database backup of its own**. A local restore-from-unit in that window finds no `.sql` and tells the +customer the app never had a database. + +**Measured live before the fix, on `demo-hp` 2026-08-22:** `docmost` and `bookstack` each held only +`pre-restore-*` files in their unit and **no canonical dump at all**. + +**The fix is a destination.** `DumpOneTo` takes the final path and derives its own `.tmp` from it; +`DumpOne` keeps its signature and calls it with the canonical name, so its other callers do not move. +`writeSafetyDump` now asks for `pre-restore--…` **directly** and the rename is gone. The +comment states the invariant and how it is enforced. + +**The scratch file matters too:** `.tmp` is derived from the FINAL path, so a nightly dump and a +safety dump running into the same directory cannot share it. + +**The manifest no longer lists the undo copies** (`db_dumps`). They are local material for a restore +that went wrong, not part of the app's recovery set. **Every consumer of `Manifest.DBDumps` was +grepped and named: there are three, all inside `recovery_unit.go`** — the declaration, this +enumeration, and the change-detection compare. Nothing reads it for recovery; no hub or agent +consumer exists. Excluding them also makes that compare stable, since the copies come and go with +every restore and prune. **The files are neither deleted nor hidden** — their visibility is a +recorded design decision and it stands. + +**Tests:** `internal/backup/r361_canonical_dump_test.go`, additions to +`internal/appbackup/r381_undo_naming_test.go`, `cmd/controller/r361_classifier_control_test.go`. +Test count 1485 → 1493. + +**Red-proofs: five planted, and TWO PASSED FIRST TIME — both reported.** The behavioural tests inject +the dump seam, so a mutation *inside* `DumpOneTo` was invisible to them; and Part 1.3 initially had no +test at all. Guards were added at the layer each defect lives in and both mutations then convicted. +**The assertion that convicts R-361 is the canonical dump's bytes, unchanged, across a restore** — a +test asserting merely that the undo copy exists passes just as well when the app's backup was +destroyed. + ## v0.220.2 — the operator route to clear a hold now says the restart is required (2026-08-22, R-379) **MinAgent: 0.129.0** (unchanged) diff --git a/controller/cmd/controller/r361_classifier_control_test.go b/controller/cmd/controller/r361_classifier_control_test.go new file mode 100644 index 0000000..3fadd1e --- /dev/null +++ b/controller/cmd/controller/r361_classifier_control_test.go @@ -0,0 +1,55 @@ +package main + +import ( + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// THE DETECTOR'S OWN POSITIVE CONTROL, at the layer it lives in. +// +// Part 3 of the 2026-08-22 R-361 session measured that a HELD app raises no dead-app alarm. That is +// an ABSENCE claim, and an absence claim is worthless unless the detector is shown working. On the +// live box it was hard to hold a stack in a down state long enough to see one: `aggregateState` +// checks `unhealthy > 0` BEFORE the mixed-case degraded branch (internal/stacks/manager.go), so an +// app whose database dies goes `degraded` for a moment and then `unhealthy` as its own healthcheck +// fails — and `unhealthy` is deliberately not in `IsDownState`. +// +// `classifyRunStates` is pure, so the control is exact here rather than a race against health probes. +func TestClassifyRunStates_PositiveControl_ADownStackDoesAlarm(t *testing.T) { + now := time.Now() + for _, st := range []stacks.ContainerState{stacks.StateDegraded, stacks.StateExited} { + sts := []stacks.Stack{{Name: "victim", Deployed: true, State: st}} + dead, states := classifyRunStates(sts, nil, nil, now) + if len(dead) != 1 { + t.Errorf("%s: the dead-app banner must fire, got %d entries", st, len(dead)) + } + if len(states) != 1 || !states[0].Down { + t.Errorf("%s: the run state must be Down, got %+v", st, states) + } + } +} + +// ...and the states measured on the live box do NOT alarm, which is why the held app was silent. +// This is the other half of the same control: it pins WHY, so the next reader does not re-derive it. +func TestClassifyRunStates_TheStatesMeasuredLiveDoNotAlarm(t *testing.T) { + now := time.Now() + cases := []struct { + state stacks.ContainerState + why string + }{ + {stacks.StateUnhealthy, "a held app, and any app whose database died, aggregates here — unhealthy is not in IsDownState"}, + {stacks.StateStopped, "a fully stopped app is whitelisted as a deliberate user stop unless quiesce failed to restart it"}, + } + for _, c := range cases { + sts := []stacks.Stack{{Name: "victim", Deployed: true, State: c.state}} + dead, states := classifyRunStates(sts, nil, nil, now) + if len(dead) != 0 { + t.Errorf("%s: expected no banner (%s), got %d", c.state, c.why, len(dead)) + } + if len(states) != 1 || states[0].Down { + t.Errorf("%s: expected not-Down (%s), got %+v", c.state, c.why, states) + } + } +} diff --git a/controller/internal/appbackup/dbdump.go b/controller/internal/appbackup/dbdump.go index 19be592..55c0e54 100644 --- a/controller/internal/appbackup/dbdump.go +++ b/controller/internal/appbackup/dbdump.go @@ -226,14 +226,43 @@ func DumpAll(ctx context.Context, dbs []DiscoveredDB, dumpDir string, logger *lo return results } -// DumpOne dumps a single database. +// CanonicalDumpName is the app's own dump filename for one database: `-.sql`. It is +// the name the replay loop matches EXACTLY, which is why nothing else may ever be written to it. +func CanonicalDumpName(db DiscoveredDB) string { + return fmt.Sprintf("%s-%s.sql", db.StackName, db.DBType) +} + +// DumpOne dumps a single database to the app's CANONICAL dump name inside dumpDir. +// +// Signature deliberately unchanged (R-361): it has callers outside this concern, and the defect was +// never in this function's contract — it was in a caller assuming a rename afterwards was equivalent +// to never writing the canonical name at all. Callers that need a different destination use +// DumpOneTo. func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.Logger, debug bool) DumpResult { + return DumpOneTo(ctx, db, filepath.Join(dumpDir, CanonicalDumpName(db)), logger, debug) +} + +// DumpOneTo dumps a single database to an EXPLICIT final path, deriving its scratch file from that +// path rather than from the canonical name. +// +// R-361, and the reason it exists: `writeSafetyDump` used to call DumpOne into the app's own unit +// directory and rename the result to `pre-restore-*` afterwards. DumpOne writes +// `-.sql` — THE APP'S REAL DUMP — so every safety dump destroyed the app's own backup +// and then moved it away, leaving the app with no database backup at all until the next nightly run. +// Measured live on `demo-hp` 2026-08-22: both `docmost` and `bookstack` had ONLY `pre-restore-*` +// files in their unit and no canonical dump. In that window a local restore-from-unit finds no `.sql` +// and tells the customer the app never had a database. +// +// The `.tmp` is derived from the FINAL path for the same reason: two dumps running into the same +// directory (a nightly dump beside a safety dump) must not share a scratch file. +func DumpOneTo(ctx context.Context, db DiscoveredDB, finalPath string, logger *log.Logger, debug bool) DumpResult { start := time.Now() result := DumpResult{DB: db} + dumpDir := filepath.Dir(finalPath) if debug { - logger.Printf("[DEBUG] DumpOne: starting dump for container=%s, stack=%s, dbType=%s, dumpDir=%s", - db.ContainerName, db.StackName, db.DBType, dumpDir) + logger.Printf("[DEBUG] DumpOne: starting dump for container=%s, stack=%s, dbType=%s, dest=%s", + db.ContainerName, db.StackName, db.DBType, finalPath) } // Ensure dump directory exists @@ -243,9 +272,7 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L return result } - filename := fmt.Sprintf("%s-%s.sql", db.StackName, db.DBType) - tmpPath := filepath.Join(dumpDir, filename+".tmp") - finalPath := filepath.Join(dumpDir, filename) + tmpPath := finalPath + ".tmp" // 5-minute timeout per dump dumpCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) @@ -374,11 +401,11 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L if debug { logger.Printf("[DEBUG] DumpOne: completed %s → %s (size=%s, valid=%v, tables=%d, duration=%s)", - db.ContainerName, filename, humanizeBytes(stat.Size()), + db.ContainerName, filepath.Base(finalPath), humanizeBytes(stat.Size()), result.Validation.Valid, result.Validation.TableCount, result.Duration.Round(time.Millisecond)) } - logger.Printf("[INFO] [backup] DB dump: %s → %s (%s, %s, %d tables)", db.ContainerName, filename, + logger.Printf("[INFO] [backup] DB dump: %s → %s (%s, %s, %d tables)", db.ContainerName, filepath.Base(finalPath), humanizeBytes(stat.Size()), result.Duration.Round(time.Millisecond), result.Validation.TableCount) return result diff --git a/controller/internal/appbackup/r381_undo_naming_test.go b/controller/internal/appbackup/r381_undo_naming_test.go index 8bd5e69..32a7b69 100644 --- a/controller/internal/appbackup/r381_undo_naming_test.go +++ b/controller/internal/appbackup/r381_undo_naming_test.go @@ -180,3 +180,73 @@ func TestImportDumpErrorDoesNotCarryEngineStderr(t *testing.T) { t.Error("the engine stderr is no longer logged either — the diagnostic must be ADDED to the operator log, not removed from everywhere") } } + +// R-361, AT THE LAYER THE DESTINATION IS HONOURED. +// +// WHY THIS EXISTS — recorded because a red-proof PASSED without it, for the second session running. +// The behavioural tests in internal/backup inject `m.safetyDumpFn`, so the REAL DumpOneTo never runs +// there: making DumpOneTo ignore its `finalPath` and write the canonical name anyway did not fail a +// single one of them. DumpOneTo shells out to docker, so its write path cannot be driven in a unit +// test; what CAN be pinned is the thing the mutation changes. +// +// TWO PROPERTIES, both mutation-detectable: +// 1. `finalPath` is never reassigned — the caller's destination is the destination. +// 2. the scratch file is derived from `finalPath`, not from the canonical name, so a nightly dump +// and a safety dump running into the same directory cannot share it. +func TestDumpOneToHonoursTheDestinationItWasGiven(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 == "DumpOneTo" && fn.Body != nil { + body = fn.Body + } + } + if body == nil { + t.Fatal("DumpOneTo not found — this guard can no longer prove anything") + } + + // 1. no assignment to finalPath + ast.Inspect(body, func(n ast.Node) bool { + as, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + for _, lhs := range as.Lhs { + if id, ok := lhs.(*ast.Ident); ok && id.Name == "finalPath" { + t.Errorf("DumpOneTo REASSIGNS finalPath at line %d — the caller's destination must be "+ + "the destination. This is R-361: the safety dump asked for its own name and got the "+ + "app's canonical dump written instead, destroying the app's only database backup.", + fset.Position(as.Pos()).Line) + } + } + return true + }) + + // 2. tmpPath is built from finalPath + var tmpFromFinal bool + 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 + } + id, ok := as.Lhs[0].(*ast.Ident) + if !ok || id.Name != "tmpPath" { + return true + } + bin, ok := as.Rhs[0].(*ast.BinaryExpr) + if ok { + if x, ok := bin.X.(*ast.Ident); ok && x.Name == "finalPath" { + tmpFromFinal = true + } + } + return true + }) + if !tmpFromFinal { + t.Error("tmpPath is not derived from finalPath — two dumps into the same directory could share " + + "a scratch file, which is the same collision R-361 is about, one file over") + } +} diff --git a/controller/internal/backup/appbackup_bridge.go b/controller/internal/backup/appbackup_bridge.go index 88e17b0..5dcdacf 100644 --- a/controller/internal/backup/appbackup_bridge.go +++ b/controller/internal/backup/appbackup_bridge.go @@ -69,6 +69,12 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L return appbackup.DumpOne(ctx, db, dumpDir, logger, debug) } +// DumpOneTo dumps to an EXPLICIT final path (R-361). The safety dump uses it so it never names — and +// therefore never destroys — the app's own `-.sql`. +func DumpOneTo(ctx context.Context, db DiscoveredDB, finalPath string, logger *log.Logger, debug bool) DumpResult { + return appbackup.DumpOneTo(ctx, db, finalPath, logger, debug) +} + // ImportDump replays a captured .sql dump back into a running DB container (F17 restore path). func ImportDump(ctx context.Context, db DiscoveredDB, dumpPath string, logger *log.Logger, debug bool) error { return appbackup.ImportDump(ctx, db, dumpPath, logger, debug) diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index 9dabb7e..ed1a107 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -183,18 +183,25 @@ func (m *Manager) writeSafetyDump(ctx context.Context, stackName, nsRoot string) } set := safetyDumpSet{Stamp: time.Now().UTC().Format("20060102T150405Z")} for _, db := range mine { - res := m.dumpForSafety(ctx, db, dumpDir) + // R-361: the undo copy is dumped STRAIGHT to its own name. It used to be dumped to the app's + // canonical `-.sql` and renamed afterwards, and the comment here asserted that + // the rename meant it "can never overwrite the app's real dump". THAT WAS FALSE AS WRITTEN: + // `DumpOne` writes the canonical name, so every safety dump destroyed the app's own backup and + // then moved it away — leaving the app with NO database backup until the next nightly run, and + // a local restore-from-unit in that window telling the customer the app never had a database. + // Measured live on demo-hp 2026-08-22: `docmost` and `bookstack` both held only `pre-restore-*` + // files and no canonical dump. + // + // THE INVARIANT, AND HOW IT IS NOW ENFORCED: nothing but the app's own dump is ever written to + // the canonical name, because the safety dump never names it — `DumpOneTo` takes the final path + // and derives its own `.tmp` from it, so neither the destination nor the scratch file can + // collide with a nightly dump running beside it. Pinned by + // TestR361_SafetyDumpLeavesTheCanonicalDumpByteIdentical. + safe := filepath.Join(dumpDir, fmt.Sprintf("%s%s-%s-%s.sql", preRestoreDumpPrefix, set.Stamp, stackName, db.DBType)) + res := m.dumpForSafety(ctx, db, safe) if res.Error != nil { 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, set.Stamp, stackName, db.DBType)) - if res.FilePath != safe { - if err := os.Rename(res.FilePath, safe); err != nil { - return safetyDumpSet{}, fmt.Errorf("a biztonsági mentés véglegesítése sikertelen: %w", err) - } - } // 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)) @@ -403,12 +410,15 @@ func (m *Manager) rollbackSafetyDump(ctx context.Context, stack string, set safe return nil } -// dumpForSafety is the DumpOne seam for the safety dump (tests inject; nil → the real DumpOne). -func (m *Manager) dumpForSafety(ctx context.Context, db DiscoveredDB, dumpDir string) DumpResult { +// dumpForSafety is the dump seam for the safety dump (tests inject; nil → the real DumpOneTo). +// +// R-361: it takes the FINAL PATH, not a directory. A directory argument is what allowed the callee to +// choose the canonical name, which is the whole defect. +func (m *Manager) dumpForSafety(ctx context.Context, db DiscoveredDB, finalPath string) DumpResult { if m.safetyDumpFn != nil { - return m.safetyDumpFn(ctx, db, dumpDir) + return m.safetyDumpFn(ctx, db, finalPath) } - return DumpOne(ctx, db, dumpDir, m.logger, m.isDebug()) + return DumpOneTo(ctx, db, finalPath, m.logger, m.isDebug()) } // ReconstituteFromOffsite makes the live app equal to a restored full-scratch snapshot: files diff --git a/controller/internal/backup/offbox_reconstitute_test.go b/controller/internal/backup/offbox_reconstitute_test.go index d3817b1..026a7dd 100644 --- a/controller/internal/backup/offbox_reconstitute_test.go +++ b/controller/internal/backup/offbox_reconstitute_test.go @@ -155,11 +155,12 @@ func reconFixture(t *testing.T, runID, dumpsAt string, dumpBody string) (*Manage // Seams: one DB, a safety dump that really writes a file, and a recording importer. db := DiscoveredDB{StackName: "immich", ContainerName: "immich-postgres", DBType: DBTypePostgres} m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return []DiscoveredDB{db}, nil } - m.SetSafetyDumpFn(func(_ context.Context, d DiscoveredDB, dir string) DumpResult { - p := filepath.Join(dir, "immich-postgres.sql") - _ = os.MkdirAll(dir, 0o755) - _ = os.WriteFile(p, []byte(pgDump(1)), 0o644) - return DumpResult{DB: d, FilePath: p, Size: 42} + // R-361: the seam now takes the FINAL PATH, not a directory — a directory argument is what let + // the callee choose the canonical name, which was the defect. + m.SetSafetyDumpFn(func(_ context.Context, d DiscoveredDB, finalPath string) DumpResult { + _ = os.MkdirAll(filepath.Dir(finalPath), 0o755) + _ = os.WriteFile(finalPath, []byte(pgDump(1)), 0o644) + return DumpResult{DB: d, FilePath: finalPath, Size: 42} }) var imported []string m.importDBDump = func(_ context.Context, _ DiscoveredDB, p string) error { diff --git a/controller/internal/backup/r355_safetydump_test.go b/controller/internal/backup/r355_safetydump_test.go index 2d43298..a5d06eb 100644 --- a/controller/internal/backup/r355_safetydump_test.go +++ b/controller/internal/backup/r355_safetydump_test.go @@ -34,15 +34,15 @@ func TestR355_SafetyDumpIsTakenForTheCorrectlyAttributedApp(t *testing.T) { {StackName: "paperless-ngx", DBType: DBTypePostgres, ContainerName: "paperless-postgres", ContainerID: "cid"}, }, nil } - m.safetyDumpFn = func(ctx context.Context, db DiscoveredDB, dumpDir string) DumpResult { - p := filepath.Join(dumpDir, string(db.StackName)+"-"+string(db.DBType)+".sql") - if err := os.MkdirAll(dumpDir, 0o755); err != nil { + m.safetyDumpFn = func(ctx context.Context, db DiscoveredDB, finalPath string) DumpResult { + // R-361: the seam takes the FINAL PATH now. + if err := os.MkdirAll(filepath.Dir(finalPath), 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(p, []byte("-- 72 tables\n"), 0o644); err != nil { + if err := os.WriteFile(finalPath, []byte("-- 72 tables\n"), 0o644); err != nil { t.Fatal(err) } - return DumpResult{DB: db, FilePath: p, Size: 13} + return DumpResult{DB: db, FilePath: finalPath, Size: 13} } // v0.220.0 (R-379): writeSafetyDump returns the SET it wrote, so a rollback can re-apply EVERY diff --git a/controller/internal/backup/r361_canonical_dump_test.go b/controller/internal/backup/r361_canonical_dump_test.go new file mode 100644 index 0000000..63a3ddf --- /dev/null +++ b/controller/internal/backup/r361_canonical_dump_test.go @@ -0,0 +1,236 @@ +package backup + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-361. Taking the undo copy DESTROYED the app's own database backup. +// +// `DumpOne` writes `-.sql` — the app's canonical dump, the name the replay loop +// matches exactly. `writeSafetyDump` called it into the app's OWN unit directory and renamed the +// result to `pre-restore-*` afterwards, so every restore overwrote the app's real backup and then +// moved it away. The app was left with no database backup at all until the next nightly run, and a +// local restore-from-unit in that window tells the customer the app never had a database. +// +// A comment at `offbox_reconstitute.go` asserted the rename meant it "can never overwrite the app's +// real dump". It was false as written. Measured live on demo-hp 2026-08-22: `docmost` and +// `bookstack` each held only `pre-restore-*` files and no canonical dump. +// +// THE DANGEROUS LOOKALIKE, named so nobody writes it by accident: a test asserting "the pre-restore +// file exists" passes just as well when the canonical dump was destroyed. The assertion that +// convicts is THE CANONICAL DUMP'S CONTENT, UNCHANGED. + +func sha256File(t *testing.T, p string) string { + t.Helper() + b, err := os.ReadFile(p) + if err != nil { + t.Fatalf("reading %s: %v", p, err) + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) +} + +// The whole of R-361, in one comparison. +func TestR361_SafetyDumpLeavesTheCanonicalDumpByteIdentical(t *testing.T) { + nsRoot := t.TempDir() + m := newSafetyTestManager() + db := DiscoveredDB{StackName: "app", DBType: DBTypePostgres, ContainerName: "app-postgres", ContainerID: "cid"} + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return []DiscoveredDB{db}, nil } + + // The app's OWN dump, already on disk — the thing a local restore reads. + dumpDir := AppDBDumpPath(nsRoot, "app") + if err := os.MkdirAll(dumpDir, 0o755); err != nil { + t.Fatal(err) + } + canonical := filepath.Join(dumpDir, "app-postgres.sql") + const ownContent = "-- THE APP'S OWN NIGHTLY DUMP\nCREATE TABLE nightly();\n" + if err := os.WriteFile(canonical, []byte(ownContent), 0o644); err != nil { + t.Fatal(err) + } + before := sha256File(t, canonical) + + // The real dump seam is NOT injected here on purpose for the destination question — we inject a + // writer that behaves like DumpOneTo (writes wherever it is told), so the test measures WHICH PATH + // writeSafetyDump asks for. That is the layer the defect lives in. + var askedFor []string + m.safetyDumpFn = func(_ context.Context, d DiscoveredDB, finalPath string) DumpResult { + askedFor = append(askedFor, finalPath) + if err := os.MkdirAll(filepath.Dir(finalPath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(finalPath, []byte("-- THE UNDO COPY\n"), 0o644); err != nil { + t.Fatal(err) + } + return DumpResult{DB: d, FilePath: finalPath, Size: 17} + } + + set, err := m.writeSafetyDump(context.Background(), "app", nsRoot) + if err != nil { + t.Fatalf("writeSafetyDump: %v", err) + } + + // THE ASSERTION THAT CONVICTS. + if _, err := os.Stat(canonical); err != nil { + t.Fatalf("the app's own dump is GONE after taking an undo copy: %v", err) + } + if after := sha256File(t, canonical); after != before { + t.Fatalf("the app's own dump CHANGED across the safety dump.\n before %s\n after %s\n"+ + "The undo must never be written to the canonical name.", before, after) + } + + // And the destination asked for was never the canonical name — the layer the defect lived at. + for _, p := range askedFor { + if filepath.Base(p) == "app-postgres.sql" { + t.Errorf("writeSafetyDump asked for the CANONICAL name %q — that is the defect itself", p) + } + if !strings.HasPrefix(filepath.Base(p), preRestoreDumpPrefix) { + t.Errorf("the undo copy must be asked for under its own prefix, got %q", filepath.Base(p)) + } + } + + // The undo exists beside it, under its own name. + if len(set.Files) != 1 { + t.Fatalf("one database → one undo copy, got %d", len(set.Files)) + } + if _, err := os.Stat(set.Files[0].Path); err != nil { + t.Fatalf("the undo copy is not on disk: %v", err) + } +} + +// Scenario B — two databases: BOTH canonical dumps survive, and the scratch files cannot collide. +func TestR361_TwoDatabases_BothCanonicalDumpsSurvive(t *testing.T) { + nsRoot := t.TempDir() + m := newSafetyTestManager() + pg := DiscoveredDB{StackName: "app", DBType: DBTypePostgres, ContainerName: "app-postgres", ContainerID: "a"} + my := DiscoveredDB{StackName: "app", DBType: DBTypeMariaDB, ContainerName: "app-maria", ContainerID: "b"} + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return []DiscoveredDB{pg, my}, nil } + + dumpDir := AppDBDumpPath(nsRoot, "app") + if err := os.MkdirAll(dumpDir, 0o755); err != nil { + t.Fatal(err) + } + want := map[string]string{} + for _, n := range []string{"app-postgres.sql", "app-mariadb.sql"} { + p := filepath.Join(dumpDir, n) + if err := os.WriteFile(p, []byte("-- own dump of "+n+"\n"), 0o644); err != nil { + t.Fatal(err) + } + want[n] = sha256File(t, p) + } + + var asked []string + m.safetyDumpFn = func(_ context.Context, d DiscoveredDB, finalPath string) DumpResult { + asked = append(asked, finalPath) + _ = os.WriteFile(finalPath, []byte("-- undo\n"), 0o644) + return DumpResult{DB: d, FilePath: finalPath, Size: 8} + } + + set, err := m.writeSafetyDump(context.Background(), "app", nsRoot) + if err != nil { + t.Fatal(err) + } + for n, w := range want { + got := sha256File(t, filepath.Join(dumpDir, n)) + if got != w { + t.Errorf("%s changed across the safety dump (%s → %s) — one database's own backup was destroyed", n, w, got) + } + } + if len(set.Files) != 2 { + t.Fatalf("two databases → two undo copies, got %d", len(set.Files)) + } + // The two undo destinations must differ, or one would overwrite the other. + if len(asked) == 2 && asked[0] == asked[1] { + t.Fatalf("both databases were dumped to the SAME path %q", asked[0]) + } +} + +// The scratch file is derived from the FINAL path, so a nightly dump and a safety dump running into +// the same directory cannot share it. Asserted at the layer that builds it. +func TestR361_ScratchFileCannotCollideWithTheNightlyDump(t *testing.T) { + dir := t.TempDir() + canonical := filepath.Join(dir, "app-postgres.sql") + undo := filepath.Join(dir, preRestoreDumpPrefix+"20260822T211114Z-app-postgres.sql") + // The contract DumpOneTo implements: tmp = final + ".tmp". + if canonical+".tmp" == undo+".tmp" { + t.Fatal("the two scratch paths are equal — a nightly dump and a safety dump would share a file") + } + if filepath.Base(undo+".tmp") == filepath.Base(canonical+".tmp") { + t.Fatalf("scratch basenames collide: %q vs %q", filepath.Base(undo+".tmp"), filepath.Base(canonical+".tmp")) + } +} + +// Part 1.3 — the manifest lists the app's OWN dumps and NOT the undo copies. +// +// BEHAVIOURAL, through the real CaptureRecoveryUnit, because the decision is about what ships +// off-site and a source-level check would not prove the manifest. +func TestR361_ManifestExcludesUndoCopies(t *testing.T) { + tmp := t.TempDir() + stackDir := filepath.Join(tmp, "stack") + drive := filepath.Join(tmp, "drive") + if err := os.MkdirAll(stackDir, 0o755); err != nil { + t.Fatal(err) + } + mustWrite(t, filepath.Join(stackDir, "docker-compose.yml"), "services:\n app:\n image: ex/app:1\n") + mustWrite(t, filepath.Join(stackDir, ".felhom.yml"), "display_name: Ex\n") + mustWrite(t, filepath.Join(stackDir, "app.yaml"), "deployed: true\nenv:\n SUBDOMAIN: ex\n") + + // The app's own dump, plus three undo copies of the shape a restore leaves behind. + dumps := AppDBDumpPath(drive, "ex") + mustWrite(t, filepath.Join(dumps, "ex-postgres.sql"), "-- the app's own dump") + for _, st := range []string{"20260822T160544Z", "20260822T162347Z", "20260822T162708Z"} { + mustWrite(t, filepath.Join(dumps, preRestoreDumpPrefix+st+"-ex-postgres.sql"), "-- undo") + } + + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(tmp, "system"), + stackProvider: &fakeRecoveryProvider{info: RecoveryInfo{ + StackDir: stackDir, DisplayName: "Ex", ImagePins: []string{"ex/app:1"}, + NonSecretEnv: map[string]string{"SUBDOMAIN": "ex", "HDD_PATH": drive}, + }, hdd: drive}, + version: "vtest", + } + if err := m.CaptureRecoveryUnit("ex"); err != nil { + t.Fatalf("capture: %v", err) + } + + man := readManifest(RecoveryUnitManifestPath(drive, "ex")) + if man == nil { + t.Fatal("no manifest written") + } + if len(man.DBDumps) != 1 || man.DBDumps[0] != "ex-postgres.sql" { + t.Fatalf("db_dumps must list ONLY the app's own dump, got %v", man.DBDumps) + } + // POSITIVE CONTROL for the absence: the undo copies really are on disk, so "not listed" is the + // manifest excluding them rather than the fixture never creating them. + n := 0 + ents, _ := os.ReadDir(dumps) + for _, e := range ents { + if strings.HasPrefix(e.Name(), preRestoreDumpPrefix) { + n++ + } + } + if n != 3 { + t.Fatalf("fixture: expected 3 undo copies on disk, found %d — the exclusion above would prove nothing", n) + } +} + +// filterOutUndoCopies, exactly. +func TestR361_FilterOutUndoCopies(t *testing.T) { + in := []string{"app-postgres.sql", preRestoreDumpPrefix + "20260822T160544Z-app-postgres.sql", "app-mariadb.sql"} + got := filterOutUndoCopies(in) + if len(got) != 2 || got[0] != "app-postgres.sql" || got[1] != "app-mariadb.sql" { + t.Fatalf("got %v, want the two canonical dumps only", got) + } + if len(filterOutUndoCopies(nil)) != 0 { + t.Error("nil must yield an empty list, not a panic") + } +} diff --git a/controller/internal/backup/r379_rollback_test.go b/controller/internal/backup/r379_rollback_test.go index 580ccb1..1882f34 100644 --- a/controller/internal/backup/r379_rollback_test.go +++ b/controller/internal/backup/r379_rollback_test.go @@ -191,12 +191,15 @@ func TestR379_ScenarioG_TwoDatabases_WholeSetRolledBack(t *testing.T) { {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 { + m.safetyDumpFn = func(_ context.Context, db DiscoveredDB, finalPath string) DumpResult { + // R-361: the seam takes the FINAL PATH now. + if err := os.MkdirAll(filepath.Dir(finalPath), 0o755); err != nil { t.Fatal(err) } - return DumpResult{DB: db, FilePath: p, Size: 42} + if err := os.WriteFile(finalPath, []byte(pgDump(1)), 0o644); err != nil { + t.Fatal(err) + } + return DumpResult{DB: db, FilePath: finalPath, Size: 42} } m.importDBDump = func(context.Context, DiscoveredDB, string) error { return errors.New("replay failed") } @@ -231,12 +234,15 @@ func TestR379_UndoSetIsThisRunOnly(t *testing.T) { 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 { + m.safetyDumpFn = func(_ context.Context, db DiscoveredDB, finalPath string) DumpResult { + // R-361: the seam takes the FINAL PATH now. + if err := os.MkdirAll(filepath.Dir(finalPath), 0o755); err != nil { t.Fatal(err) } - return DumpResult{DB: db, FilePath: p, Size: 1} + if err := os.WriteFile(finalPath, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + return DumpResult{DB: db, FilePath: finalPath, Size: 1} } // An OLDER undo copy from a previous run, already on disk. dumpDir := AppDBDumpPath(nsRoot, "app") diff --git a/controller/internal/backup/recovery_unit.go b/controller/internal/backup/recovery_unit.go index 0f3438d..a0b10c3 100644 --- a/controller/internal/backup/recovery_unit.go +++ b/controller/internal/backup/recovery_unit.go @@ -128,7 +128,23 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { checksums["app.yaml"] = sha256Hex(appYaml) configFiles = append(configFiles, "app.yaml") - dbDumps := listFileNames(AppDBDumpPath(nsRoot, stackName), ".sql") + // R-361: `db_dumps` lists the app's OWN dumps and NOT the `pre-restore-*` undo copies. + // + // THE DECISION, and the reasoning, because a decision recorded only in a commit message is a + // decision nobody finds: the undo copies are LOCAL material for a restore that went wrong, not + // part of the app's recovery set. Nothing reads them from the manifest — the reconstitution skips + // `pre-restore-*` at three sites, and a grep of every consumer of `Manifest.DBDumps` found only + // this file (the declaration, this enumeration, and the change-detection compare below). Listing + // them meant three copies per app were enumerated into the manifest and pushed off-site + // permanently, for no recovery value. + // + // It also makes the compare below STABLE: the undo copies come and go with every restore and + // every prune, so including them forced a manifest rewrite each time for a change that says + // nothing about the app's backup. + // + // The files themselves are NOT deleted and NOT hidden — their visibility is a recorded design + // (see preRestoreDumpPrefix). This is about the manifest only. + dbDumps := filterOutUndoCopies(listFileNames(AppDBDumpPath(nsRoot, stackName), ".sql")) volDumps := listFileNames(AppVolumeDumpPath(nsRoot, stackName), ".tar") version := m.versionLocked() @@ -526,3 +542,16 @@ func atomicWrite(path string, data []byte, perm os.FileMode) error { } return nil } + +// filterOutUndoCopies drops `pre-restore-*` names from a dump listing. R-361: the undo copies are not +// part of the app's recovery set — see the reasoning at the call site. +func filterOutUndoCopies(names []string) []string { + out := make([]string, 0, len(names)) + for _, n := range names { + if strings.HasPrefix(n, preRestoreDumpPrefix) { + continue + } + out = append(out, n) + } + return out +}