fix(backup): F3 — wire named-volume dumps into the app-data backup run

DumpAppVolumesSafe had NO production caller: no trigger ever produced
volume-dumps/, so named-volume app data (e.g. nextcloud's html volume) was never
captured into the recovery unit and the granular restore silently restored
nothing for class-B data (drill finding F3).

- runVolumeDumps: per-stack loop in runDBDumpsInternal, BEFORE
  captureAllRecoveryUnits (so manifests enumerate the fresh tars). Gate order is
  load-bearing: protected-stack and volume-check gates precede DumpAppVolumesSafe
  (which stops the stack before its own check — unconditional calls would bounce
  every volume-less app nightly). Disconnected/decommissioned drives skip with
  the same summary style as the DB loop.
- No silent partials: a per-stack failure lands as a FAIL summary entry, flips
  Success, and fails the run ("some backup steps failed: ..."), without aborting
  the other stacks.
- Zero-DB early return removed: volume-bearing apps without a database still get
  their class-B dump + unit refresh.
- dumpVolumesSafe seam (same style as the F17 discoverDBs/importDBDump seams) so
  the gating is unit-tested without Docker. Companion red-proof: neutering the
  volume gate fails TestRunVolumeDumps_GatesPrecedeDump (dump fired for the
  volume-less stack) and _VolumelessNeverStopped (verified, reverted).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6

controller/internal/backup/backup.go

@@ -40,6 +40,10 @@ type Manager struct {
 	discoverDBs  func(ctx context.Context) ([]DiscoveredDB, error)
 	importDBDump func(ctx context.Context, db DiscoveredDB, dumpPath string) error
 
+	// F3 volume-dump seam — overridable in tests so runVolumeDumps' gating (protected / volume-less /
+	// disconnected) can be unit-tested without Docker. Nil → the real DumpAppVolumesSafe.
+	dumpVolumesSafe func(stackName string) error
+
 	// migrationRunning, if set, reports whether a data migration is in progress. The scheduled
 	// backup paths skip when it returns true (Change 3 — backup ↔ migration mutual exclusion), so a
 	// nightly dump/Tier-2 can't race a migration copy/cleanup on the same drive.
@@ -202,20 +206,14 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error {
 		return err
 	}
 
+	// F3: no early return on zero DBs — volume-bearing apps without a database still need their
+	// class-B volume dump + recovery-unit refresh below (the DB loop simply has no iterations).
 	if len(dbs) == 0 {
 		m.logger.Printf("[INFO] [backup] No database containers found")
-		m.mu.Lock()
+	} else {
-		m.lastDBDump = &DBDumpStatus{
+		m.logger.Printf("[INFO] [backup] Discovered %d database(s): %s", len(dbs), dbNames(dbs))
-			LastRun:  time.Now(),
-			Success:  true,
-			Duration: time.Since(start),
-		}
-		m.mu.Unlock()
-		return nil
 	}
 
-	m.logger.Printf("[INFO] [backup] Discovered %d database(s): %s", len(dbs), dbNames(dbs))
-
 	// Dump each DB to its app's drive path
 	var results []DumpResult
 	allOK := true
@@ -270,6 +268,13 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error {
 		}
 	}
 
+	// F3: class-B leg — dump each app's named-volume data (stop → tar → restart). MUST run before
+	// captureAllRecoveryUnits so the manifests enumerate the fresh tars into VolumeDumps.
+	dbOK := allOK
+	volSummary, volDumped, volOK := m.runVolumeDumps()
+	summary = append(summary, volSummary...)
+	allOK = dbOK && volOK
+
 	duration := time.Since(start)
 	m.mu.Lock()
 	m.lastDBDump = &DBDumpStatus{
@@ -281,22 +286,91 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error {
 	m.mu.Unlock()
 
 	if allOK {
-		m.logger.Printf("[INFO] [backup] DB dump completed: %d databases, %s total (%s)",
+		m.logger.Printf("[INFO] [backup] App-data backup completed: %d databases (%s total), %d volume dump(s) (%s)",
-			len(results), humanizeBytes(totalSize), duration.Round(time.Millisecond))
+			len(results), humanizeBytes(totalSize), volDumped, duration.Round(time.Millisecond))
 	} else {
-		// Still refresh recovery units below — a partial DB failure shouldn't leave units stale.
+		// Still refresh recovery units below — a partial failure shouldn't leave units stale.
-		m.logger.Printf("[WARN] [backup] some database dumps failed; refreshing recovery units anyway")
+		m.logger.Printf("[WARN] [backup] some backup steps failed (%s); refreshing recovery units anyway",
+			strings.Join(failedSummaryLines(summary), "; "))
 	}
 
 	// Phase 2: refresh each deployed app's self-contained recovery unit (compose + manifest).
 	m.captureAllRecoveryUnits()
 
+	// No silent partials: a DB-dump or volume-dump failure fails the whole run.
 	if !allOK {
-		return fmt.Errorf("some database dumps failed")
+		return fmt.Errorf("some backup steps failed: %s", strings.Join(failedSummaryLines(summary), "; "))
 	}
 	return nil
 }
 
+// failedSummaryLines filters a run summary down to its FAIL entries (for logs/errors).
+func failedSummaryLines(summary []string) []string {
+	var failed []string
+	for _, s := range summary {
+		if strings.HasPrefix(s, "FAIL ") {
+			failed = append(failed, s)
+		}
+	}
+	return failed
+}
+
+// runVolumeDumps exports the Docker named-volume data of every deployed, unprotected stack whose
+// drive is writable — the class-B leg of the nightly app-data backup. (F3: DumpAppVolumesSafe
+// previously had NO production caller, so volume-dumps/ was never produced and the granular
+// restore had nothing to restore for named-volume apps.) Caller must hold the running flag.
+//
+// Gate ORDER is load-bearing: the volume check precedes DumpAppVolumesSafe, because the Safe
+// variant stops the stack before its own volume check — calling it unconditionally would bounce
+// every volume-less app on every nightly run. Per-stack isolation mirrors the DB loop: one app's
+// failure is recorded and does not abort the others.
+func (m *Manager) runVolumeDumps() (summary []string, dumped int, allOK bool) {
+	allOK = true
+	if m.stackProvider == nil {
+		return nil, 0, true
+	}
+	dump := m.dumpVolumesSafe
+	if dump == nil {
+		dump = m.DumpAppVolumesSafe
+	}
+
+	for _, stack := range m.stackProvider.ListDeployedStacks() {
+		// Never stop/dump infra stacks (felhom-controller, traefik, cloudflared).
+		if m.cfg != nil && m.cfg.IsProtectedStack(stack.Name) {
+			continue
+		}
+		// Volume check FIRST — a volume-less stack must not be stopped at all (see gate-order note).
+		if len(m.stackProvider.GetDockerVolumes(stack.Name)) == 0 {
+			if m.isDebug() {
+				m.logger.Printf("[DEBUG] [backup] %s has no named volumes — volume dump skipped", stack.Name)
+			}
+			continue
+		}
+		// Same drive-state skip guards as the DB-dump loop.
+		drivePath := m.GetAppDrivePath(stack.Name)
+		if m.settings != nil && m.settings.IsDisconnected(drivePath) {
+			m.logger.Printf("[WARN] [backup] Skipping volume dump for %s — drive disconnected: %s", stack.Name, drivePath)
+			summary = append(summary, fmt.Sprintf("SKIP %s volumes (drive disconnected)", stack.Name))
+			continue
+		}
+		if m.settings != nil && m.settings.IsDecommissioned(drivePath) {
+			m.logger.Printf("[WARN] [backup] Skipping volume dump for %s — drive decommissioned: %s", stack.Name, drivePath)
+			summary = append(summary, fmt.Sprintf("SKIP %s volumes (drive decommissioned)", stack.Name))
+			continue
+		}
+
+		if err := dump(stack.Name); err != nil {
+			allOK = false
+			summary = append(summary, fmt.Sprintf("FAIL %s volumes: %v", stack.Name, err))
+			m.logger.Printf("[ERROR] [backup] Volume dump failed for %s: %v", stack.Name, err)
+			continue
+		}
+		dumped++
+		summary = append(summary, fmt.Sprintf("OK %s volumes", stack.Name))
+	}
+	return summary, dumped, allOK
+}
+
 // DumpAppVolumes exports Docker named volumes to tar files for the given stack.
 // Tars are written to AppVolumeDumpPath(drivePath, stackName)/.
 // Uses "docker run alpine tar" (same pattern as appexport).

controller/internal/backup/volume_dumps_test.go

@@ -0,0 +1,193 @@
+package backup
+
+import (
+	"io"
+	"log"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/config"
+	"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
+)
+
+// volDumpFakeProvider is a StackDataProvider for the runVolumeDumps gating tests: a configurable
+// stack list with per-stack volumes + drive, and a StopStack recorder (the destructive act the
+// gates must prevent for volume-less/protected stacks).
+type volDumpFakeProvider struct {
+	stacks  []StackSummary
+	volumes map[string][]string
+	hdd     map[string]string
+	stopped []string
+}
+
+func (f *volDumpFakeProvider) GetStackComposePath(string) (string, bool) { return "", false }
+func (f *volDumpFakeProvider) ListDeployedStacks() []StackSummary        { return f.stacks }
+func (f *volDumpFakeProvider) GetStackHDDMounts(string) []string         { return nil }
+func (f *volDumpFakeProvider) GetStackHDDPath(name string) string        { return f.hdd[name] }
+func (f *volDumpFakeProvider) GetDockerVolumes(name string) []string     { return f.volumes[name] }
+func (f *volDumpFakeProvider) StopStack(name string) error {
+	f.stopped = append(f.stopped, name)
+	return nil
+}
+func (f *volDumpFakeProvider) StartStack(string) error          { return nil }
+func (f *volDumpFakeProvider) RefreshAndIsRunning(string) bool  { return true }
+func (f *volDumpFakeProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) {
+	return RecoveryInfo{}, false
+}
+func (f *volDumpFakeProvider) RecoverStackSecrets(string, []string) map[string]string { return nil }
+func (f *volDumpFakeProvider) RecreateStackFromUnit(string, string, map[string]string) error {
+	return nil
+}
+
+// TestRunVolumeDumps_GatesPrecedeDump proves Scenario D/E's gating: the dump is invoked ONLY for
+// a volume-bearing, unprotected stack on a writable drive. The negatives are the point —
+// volume-less (rallly-like), protected (traefik-like), and disconnected-drive stacks are never
+// dumped (and therefore never stopped, since stopping happens inside DumpAppVolumesSafe).
+// COMPANION red-proof: removing the volume gate makes the seam fire for "rallly" → this fails.
+func TestRunVolumeDumps_GatesPrecedeDump(t *testing.T) {
+	tmp := t.TempDir()
+	usbDrive := filepath.Join(tmp, "usb")
+	badDrive := filepath.Join(tmp, "gone")
+
+	sett, err := settings.Load(filepath.Join(tmp, "settings.json"), log.New(io.Discard, "", 0))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := sett.AddStoragePath(settings.StoragePath{Path: badDrive, Label: "gone"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := sett.SetDisconnected(badDrive, true, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := &config.Config{}
+	cfg.Paths.SystemDataPath = filepath.Join(tmp, "sys")
+	cfg.Stacks.Protected = []string{"traefik"}
+
+	fake := &volDumpFakeProvider{
+		stacks: []StackSummary{
+			{Name: "nextcloud"}, {Name: "rallly"}, {Name: "traefik"}, {Name: "diskapp"},
+		},
+		volumes: map[string][]string{
+			"nextcloud": {"nextcloud_nextcloud_html"},
+			"rallly":    nil,                        // volume-less — must NOT be stopped/dumped
+			"traefik":   {"traefik_data"},           // protected — never considered
+			"diskapp":   {"diskapp_data"},           // volume-bearing but drive disconnected
+		},
+		hdd: map[string]string{"nextcloud": usbDrive, "diskapp": badDrive},
+	}
+
+	m := &Manager{cfg: cfg, settings: sett, logger: log.New(io.Discard, "", 0),
+		systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake}
+	var dumpCalls []string
+	m.dumpVolumesSafe = func(name string) error {
+		dumpCalls = append(dumpCalls, name)
+		return nil
+	}
+
+	summary, dumped, ok := m.runVolumeDumps()
+	if !ok {
+		t.Fatalf("run should be ok, summary=%v", summary)
+	}
+	if len(dumpCalls) != 1 || dumpCalls[0] != "nextcloud" {
+		t.Errorf("dump invoked for %v, want exactly [nextcloud] (gates must exclude volume-less/protected/disconnected)", dumpCalls)
+	}
+	if dumped != 1 {
+		t.Errorf("dumped = %d, want 1", dumped)
+	}
+	if len(fake.stopped) != 0 {
+		t.Errorf("StopStack called for %v — the seam bypasses the real dump, so ANY stop means a gate leaked", fake.stopped)
+	}
+	// The disconnected drive appears as a SKIP in the summary (same style as the DB loop).
+	if !containsSummary(summary, "SKIP diskapp volumes (drive disconnected)") {
+		t.Errorf("summary missing disconnected SKIP entry: %v", summary)
+	}
+}
+
+// TestRunVolumeDumps_VolumelessNeverStopped drives the REAL DumpAppVolumesSafe path (no seam) with
+// only volume-less/protected stacks: the volume gate must keep them from ever being stopped. This
+// is the direct Scenario D negative — without the gate, DumpAppVolumesSafe stops the stack BEFORE
+// its own volume check, so this test fails with stopped=[rallly].
+func TestRunVolumeDumps_VolumelessNeverStopped(t *testing.T) {
+	cfg := &config.Config{}
+	cfg.Paths.SystemDataPath = filepath.Join(t.TempDir(), "sys")
+	cfg.Stacks.Protected = []string{"traefik"}
+
+	fake := &volDumpFakeProvider{
+		stacks:  []StackSummary{{Name: "rallly"}, {Name: "traefik"}},
+		volumes: map[string][]string{"traefik": {"traefik_data"}},
+	}
+	m := &Manager{cfg: cfg, logger: log.New(io.Discard, "", 0),
+		systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake}
+	// deliberately NO seam: the real DumpAppVolumesSafe would record StopStack on the fake.
+
+	if _, dumped, ok := m.runVolumeDumps(); !ok || dumped != 0 {
+		t.Fatalf("expected clean zero-dump run, dumped=%d ok=%v", dumped, ok)
+	}
+	if len(fake.stopped) != 0 {
+		t.Errorf("volume-less/protected stacks were stopped: %v", fake.stopped)
+	}
+}
+
+// TestRunVolumeDumps_FailureSurfaces proves no-silent-partial: a per-stack dump failure lands in
+// the summary as a FAIL entry, flips allOK, and does NOT abort the remaining stacks.
+func TestRunVolumeDumps_FailureSurfaces(t *testing.T) {
+	cfg := &config.Config{}
+	cfg.Paths.SystemDataPath = filepath.Join(t.TempDir(), "sys")
+
+	fake := &volDumpFakeProvider{
+		stacks: []StackSummary{{Name: "broken"}, {Name: "healthy"}},
+		volumes: map[string][]string{
+			"broken":  {"broken_data"},
+			"healthy": {"healthy_data"},
+		},
+	}
+	m := &Manager{cfg: cfg, logger: log.New(io.Discard, "", 0),
+		systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake}
+	m.dumpVolumesSafe = func(name string) error {
+		if name == "broken" {
+			return errTest
+		}
+		return nil
+	}
+
+	summary, dumped, ok := m.runVolumeDumps()
+	if ok {
+		t.Error("allOK must be false after a dump failure")
+	}
+	if dumped != 1 {
+		t.Errorf("the healthy stack must still be dumped after another's failure (dumped=%d)", dumped)
+	}
+	if !containsSummaryPrefix(summary, "FAIL broken volumes:") {
+		t.Errorf("summary missing FAIL entry: %v", summary)
+	}
+	// failedSummaryLines feeds the run's returned error — the FAIL entry must survive the filter.
+	if failed := failedSummaryLines(summary); len(failed) != 1 {
+		t.Errorf("failedSummaryLines = %v, want exactly the broken entry", failed)
+	}
+}
+
+var errTest = &testErr{}
+
+type testErr struct{}
+
+func (*testErr) Error() string { return "tar exploded" }
+
+func containsSummary(summary []string, want string) bool {
+	for _, s := range summary {
+		if s == want {
+			return true
+		}
+	}
+	return false
+}
+
+func containsSummaryPrefix(summary []string, prefix string) bool {
+	for _, s := range summary {
+		if strings.HasPrefix(s, prefix) {
+			return true
+		}
+	}
+	return false
+}