R-361: the safety dump destroyed the app's own database backup
gates / gates (push) Successful in 11s

writeSafetyDump called DumpOne into the app's OWN unit dir and renamed the result
to pre-restore-* afterwards. DumpOne writes <stack>-<dbtype>.sql - the app's
canonical dump - so every safety dump overwrote the app's real backup and then
moved it away, leaving the app with no database backup until the next nightly
run. A local restore-from-unit in that window tells the customer the app never
had a database.

The comment beside it asserted the rename meant it 'can never overwrite the app's
real dump'. False as written, and believed for four months. Measured live before
the fix: docmost and bookstack each held only pre-restore-* files and no
canonical dump.

DumpOneTo takes the final path and derives its own .tmp from it. DumpOne keeps
its signature and calls it with the canonical name. writeSafetyDump asks for its
own name directly; the rename is gone; the comment now states the invariant and
how it is enforced.

db_dumps no longer lists the undo copies. All three consumers of Manifest.DBDumps
were grepped and named - all inside recovery_unit.go, none reads it for recovery.
The files are neither deleted nor hidden.

Tests 1485 -> 1493. FIVE red-proofs, TWO PASSED first time and both are reported:
the behavioural tests inject the dump seam so a mutation inside DumpOneTo was
invisible, and 1.3 had no test at all. Guards added at the layer each defect
lives in; both mutations then convicted.
This commit is contained in:
2026-08-22 23:39:59 +02:00
parent 2024ed9982
commit 968c968559
11 changed files with 524 additions and 40 deletions
@@ -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 `<stack>-<dbtype>.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)
@@ -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 `<stack>-<dbtype>.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 `<stack>-<dbtype>.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
@@ -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 {
@@ -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
@@ -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 `<stack>-<dbtype>.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")
}
}
@@ -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")
+30 -1
View File
@@ -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
}