admin
968c968559
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.
253 lines
8.9 KiB
Go
253 lines
8.9 KiB
Go
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")
|
|
}
|
|
}
|
|
|
|
// 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")
|
|
}
|
|
}
|