diff --git a/CHANGELOG.md b/CHANGELOG.md index ebd530e..4b57725 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,14 @@ consumer exists. Excluding them also makes that compare stable, since the copies every restore and prune. **The files are neither deleted nor hidden** — their visibility is a recorded design decision and it stands. +**One change made another unreachable, and only the live box showed it.** Excluding the undo copies +from `db_dumps` made that list STABLE across restores — which is correct — and that made +`CaptureRecoveryUnit`'s already-current early return start firing where it never had. The undo-copy +prune sat *after* that return, so the cap silently stopped applying: **four copies on disk against a +cap of three, counted on `demo-hp` minutes after the change.** The prune now runs above the check, +where it belongs — it is housekeeping on the dump directory and has nothing to do with whether the +manifest needs rewriting. + **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. diff --git a/controller/internal/backup/r361_canonical_dump_test.go b/controller/internal/backup/r361_canonical_dump_test.go index 63a3ddf..ccd1aae 100644 --- a/controller/internal/backup/r361_canonical_dump_test.go +++ b/controller/internal/backup/r361_canonical_dump_test.go @@ -234,3 +234,61 @@ func TestR361_FilterOutUndoCopies(t *testing.T) { t.Error("nil must yield an empty list, not a panic") } } + +// The undo-copy cap must hold even when the unit is ALREADY CURRENT. +// +// FOUND ON THE LIVE BOX, 2026-08-22, immediately after excluding `pre-restore-*` from `db_dumps`: +// four undo copies on disk against a cap of three. Excluding them made the list stable across +// restores, so `CaptureRecoveryUnit`'s already-current early return began firing — and the prune, +// which sat after it, stopped running in exactly the case it exists for. One change made the other +// unreachable, and only counting files on a real box showed it. +func TestR361_UndoCapHoldsWhenTheUnitIsAlreadyCurrent(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") + dumps := AppDBDumpPath(drive, "ex") + mustWrite(t, filepath.Join(dumps, "ex-postgres.sql"), "-- own") + + 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", + } + // First capture writes the manifest. + if err := m.CaptureRecoveryUnit("ex"); err != nil { + t.Fatal(err) + } + // Now the unit IS current. Drop five undo copies in, as five restores would. + for _, st := range []string{"20260101T000000Z", "20260201T000000Z", "20260301T000000Z", "20260401T000000Z", "20260501T000000Z"} { + mustWrite(t, filepath.Join(dumps, preRestoreDumpPrefix+st+"-ex-postgres.sql"), "-- undo") + } + // A capture that will take the already-current path — nothing else changed. + if err := m.CaptureRecoveryUnit("ex"); err != nil { + t.Fatal(err) + } + + n := 0 + ents, _ := os.ReadDir(dumps) + for _, e := range ents { + if strings.HasPrefix(e.Name(), preRestoreDumpPrefix) { + n++ + } + } + if n != maxUndoCopiesPerApp { + t.Fatalf("the cap must hold on the already-current path too: %d undo copies survived, want %d", n, maxUndoCopiesPerApp) + } + // And the app's own dump is not a prune candidate. + if _, err := os.Stat(filepath.Join(dumps, "ex-postgres.sql")); err != nil { + t.Fatal("the app's own dump was pruned") + } +} diff --git a/controller/internal/backup/recovery_unit.go b/controller/internal/backup/recovery_unit.go index a0b10c3..0e69288 100644 --- a/controller/internal/backup/recovery_unit.go +++ b/controller/internal/backup/recovery_unit.go @@ -160,6 +160,20 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { runID, dumpsAt = cur.OffsiteRunID, cur.DumpsAt } + // R-379/R-361: bound the undo copies BEFORE the already-current check, not after it. + // + // It used to sit at the end of this function, and that was fine only while `db_dumps` listed the + // `pre-restore-*` files: a new undo copy changed the list, so the check below never short-circuited + // and the prune always ran. R-361 excluded those files from `db_dumps` — correctly — and that made + // the list STABLE across restores, so the early return began firing and the prune became + // unreachable in exactly the case it exists for. Measured on demo-hp 2026-08-22: four undo copies + // on disk against a cap of three, immediately after the change. + // + // It is housekeeping on the dump directory and has nothing to do with whether the manifest needs + // rewriting, so it belongs above the check. Pruning cannot disturb `dbDumps`, which no longer + // contains those names at all. + m.pruneUndoCopies(AppDBDumpPath(nsRoot, stackName), stackName) + // Skip if the unit is already current — avoids needless drive writes on the periodic refresh. // The run-id is part of "current": an offsite run must re-stamp the manifest even when nothing // else changed, because the stamp is exactly the claim the restore path reads. @@ -215,7 +229,6 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { // 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 }