R-361 follow-on: the undo-copy prune must run above the already-current check

Excluding pre-restore-* from db_dumps made that list stable across restores, so
CaptureRecoveryUnit's already-current early return began firing where it never
had - and the prune, which sat after it, stopped running in exactly the case it
exists for. Measured on demo-hp minutes after the change: four undo copies on
disk against a cap of three.

The prune is housekeeping on the dump directory and is independent of whether
the manifest needs rewriting, so it belongs above the check. Pruning cannot
disturb dbDumps, which no longer contains those names.

Pinned by TestR361_UndoCapHoldsWhenTheUnitIsAlreadyCurrent; its red-proof moves
the call back below the return and the cap fails at 5.

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.

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")
+	}
+}

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
 }