R-492: delete the always-empty cfg.Paths.HDDPath (field, env binding, every fallback)

Readers (report builder, health check, /api/system, web primaryHDDPath, metrics collector, AutoDiscoverStoragePaths' fallback parameter) now use the storage registry only. An old controller.yaml still carrying paths.hdd_path keeps loading (non-strict YAML), pinned by TestR492_OldConfigWithHDDPathStillLoads. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
2026-10-05 21:45:53 +02:00
parent d15ad105be
commit 7b8216abac
11 changed files with 45 additions and 47 deletions
@@ -149,7 +149,7 @@
 | `backup.AppStopGuard` (`Begin`/`End`/`Recover`) (R-166, v0.189.0) | controller/internal/backup/appstop_marker.go | `(opID, reason, stacks) error` / `()` / `() *AppStopRecovery` | THE crash marker for stop→work→start windows (volume dump, offbox reconstitute, `.fab` export) | Its **own** file (`appstop-state.json`), never quiesce's — one file, one writer. **A `defer` is NOT the mechanism** (Campaign 8 fault 10: SIGKILL runs no defer); the marker is. Written BEFORE the stop, cleared ONLY after a restart that succeeded; a FAILED restart deliberately KEEPS it. `Recover` RETURNS its outcome rather than notifying, because it must complete before the boot reconciler while the notifier does not exist yet |
 | `backup.AppStopGuard.SuppressedStacks` + `markStopped` / `releaseStarted` / `ReleaseFailed` (R-330, v0.224.0) | controller/internal/backup/appstop_suppress.go | `() map[string]bool` (nil-safe on a nil *AppStopGuard); `ReleaseFailed(stacks ...string)` | **R-330** — an app a PER-APP operation is holding stopped (nightly volume dump, offbox reconstitute, `.fab` export) is not a fault | The **twin** of `quiesce.Loop.SuppressedStacks` above, and the two are unioned by `unionSuppressed` in main.go before `classifyRunStates` — **consult BOTH or the bug comes back**: R-330 shipped because the alarm read only the quiesce set while the per-app legs stopped apps through a different path. Rides `Begin`/`End`, so all three call sites got it with no call-site change. Grace is `appStopAlarmGrace` = 180 s, deliberately the SAME constant and derivation as quiesce's — two windows over one alarm that disagreed would be a bug on whichever path used the shorter one. **It must never latch**, and unlike quiesce's loop `End()` runs ONLY on a restart that succeeded: (1) every failure path calls `ReleaseFailed`, which drops the entry IMMEDIATELY so the app alarms on the next scan; (2) `Begin` REPLACES the set (one marker file = one operation); (3) `appStopMaxHold` (6 h) caps an open-ended hold and logs at WARN. **Deliberately NOT persisted** — after a crash the guard holds nothing and a down app must alarm. `ReleaseFailed` drops the suppression and KEEPS the durable marker; the two are independent and a test pins that |
 | `backup.ErrStartRefused` + `AppStopRecovery.Refused`/`Alarming()` (R-174, v0.191.0) | controller/internal/backup/appstop_marker.go | `errors.Is(err, ErrStartRefused)` / `() bool` | THE refusal-vs-failure split in the app-stop crash recovery | **A gated starter's refusal is NOT a restart failure.** `Recover`'s starter MUST be the gated `gatedAppStopStarter` (cmd/controller/main.go), never the raw `stacks.Manager` — that was the v0.189.0 defect, which started apps onto ABSENT drives at boot (R-171 one path over). A refusal goes to `Refused` (marker KEPT, silent), a real error to `Failed` (marker kept, ALARMS). Collapsing them routes a deliberate hold into `NotifyBackupFailed`, a customer-enabled type — the R-171 false alarm again. `main.go` must guard the notify with `Alarming()`, not `!= nil` |
-| `Manager.DeleteStack` / `RemoveStack` | controller/internal/stacks/delete.go | `(name, removeHDDData[, backupPaths])` | THE guarded removal paths | Orphan/protected/deploying/running checks + ProtectedHDDPaths filter before any RemoveAll. **R-442 (v0.236.0): the drive is the app's OWN `app.yaml` `HDD_PATH` (`appHDDPath`), never `cfg.Paths.HDDPath`; a data removal that cannot be resolved returns a typed `*RemoveRefusedError` BEFORE `compose down` — handlers `errors.As` it to 409 + `Message`** |
+| `Manager.DeleteStack` / `RemoveStack` | controller/internal/stacks/delete.go | `(name, removeHDDData[, backupPaths])` | THE guarded removal paths | Orphan/protected/deploying/running checks + ProtectedHDDPaths filter before any RemoveAll. **R-442 (v0.236.0): the drive is the app's OWN `app.yaml` `HDD_PATH` (`appHDDPath`), never a global (`cfg.Paths.HDDPath` was deleted in R-492); a data removal that cannot be resolved returns a typed `*RemoveRefusedError` BEFORE `compose down` — handlers `errors.As` it to 409 + `Message`** |
 | `resolveContainerState` / `aggregateState` | controller/internal/stacks/manager.go | `(dockerState, dockerStatus)` / `([]ContainerInfo)` | State classification | `.State` says "running" even when unhealthy — `.Status` parse is the fix |
 | `Manager.recordInstalledImages` (v0.233.0) | controller/internal/stacks/installed.go | `(name, stackDir string, env []string)` | writing down what each compose SERVICE is ACTUALLY running, into `app.yaml.installed_images` | Called after a successful compose up from `StartStack`/`RestartStack`/`UpdateStack`/`runComposeDeploy`. **Reads the CONTAINER, never `docker-compose.yml`** — that file is the value the syncer has already moved (spike §3: 25 minutes of disagreement). **A failed write NEVER refuses the action** — the deliberate OPPOSITE of `SetDesiredState`: intent refused, observation logged at ERROR. **NOT from `StartStackServices`** (the R-47 DB-only window would overwrite a complete record with a partial one). Skips the write when ref+digest are unchanged, and carries `at` forward so it means "running since". Its OWN seam (`installedExecFn`) with a **context + 30 s timeout** — the two existing exec helpers have neither |
 | `Manager.SetPin` / `AdoptPins` / `RenderPlanFor` / `AppliedComposePath` (v0.235.0) | controller/internal/stacks/pin.go | `SetPin(name, stackDir, pin, composeSrc) error` | THE version freeze — `app.yaml.pinned_images` + the stored `applied-compose.yml` | **`PinnedImages` is INTENT, `InstalledImages` is an OBSERVATION — never feed one from the other** (the R-166 category error, one field over). Four writers only: deploy, the guarded update (via `advancePinToCatalog`, which advances the pin and re-renders BEFORE the pull, and REFUSES the update if the pin cannot be written; a failed pull puts the pin BACK via `SetPin`), the restore adapter (this is what closes R-441), and `AdoptPins`. `AdoptPins` reuses `observationCoversTemplate` — do NOT write a second completeness rule — and skips loudly rather than inventing a pin. Absent pin = pre-v0.235.0 behaviour |
@@ -355,7 +355,7 @@ func main() {

 	// --- Auto-discover storage paths from deployed apps ---
 	discoveredPaths := discoverHDDPaths(cfg.Paths.StacksDir, logger)
-	sett.AutoDiscoverStoragePaths(discoveredPaths, cfg.Paths.HDDPath, logger)
+	sett.AutoDiscoverStoragePaths(discoveredPaths, logger)

 	// --- Load or create encryption key ---
 	encKeyPath := filepath.Join(cfg.Paths.DataDir, "encryption.key")
@@ -523,10 +523,7 @@ func main() {

 	if metricsStore != nil {
 		defer metricsStore.Close()
-		metricsHDDPath := cfg.Paths.HDDPath
-		if p := sett.GetDefaultStoragePath(); p != "" {
-			metricsHDDPath = p
-		}
+		metricsHDDPath := sett.GetDefaultStoragePath() // R-492: no global fallback any more
 		metricsCollector := metrics.NewMetricsCollector(metricsStore, cpuCollector, metricsHDDPath, logger)
 		metricsCollector.Start(ctx)
 		defer metricsCollector.Stop()
@@ -1171,9 +1171,9 @@ func (r *Router) triggerSync(w http.ResponseWriter, _ *http.Request) {
 }

 func (r *Router) systemInfo(w http.ResponseWriter, _ *http.Request) {
-	// R-490 / R-465: the same default-storage-path fallback every other reader of the always-empty
-	// cfg.Paths.HDDPath has; without it hdd_configured was false on every box.
-	hddPath := r.cfg.Paths.HDDPath
+	// R-490 / R-465 / R-492: the default storage path is the only source (the always-empty global
+	// paths.hdd_path was deleted in R-492); without it hdd_configured was false on every box.
+	hddPath := ""
 	if r.sett != nil {
 		if p := r.sett.GetDefaultStoragePath(); p != "" {
 			hddPath = p
@@ -164,7 +164,6 @@ type PathsConfig struct {
 	StacksDir      string `yaml:"stacks_dir"`
 	DataDir        string `yaml:"data_dir"`
 	SystemDataPath string `yaml:"system_data_path"`
-	HDDPath        string `yaml:"hdd_path"`
 }

 type WebConfig struct {
@@ -450,7 +449,6 @@ func applyEnvOverrides(cfg *Config) {
 	envStr("FELHOM_WEB_LISTEN", &cfg.Web.Listen)
 	envStr("FELHOM_WEB_PASSWORD_HASH", &cfg.Web.PasswordHash)
 	envStr("FELHOM_PATHS_STACKS_DIR", &cfg.Paths.StacksDir)
-	envStr("FELHOM_PATHS_HDD_PATH", &cfg.Paths.HDDPath)
 	envStr("FELHOM_LOGGING_LEVEL", &cfg.Logging.Level)
 	envStr("FELHOM_MONITORING_SYSTEM_HEALTH_INTERVAL", &cfg.Monitoring.SystemHealthInterval)
 }
@@ -0,0 +1,27 @@
+package config
+
+import (
+	"path/filepath"
+	"testing"
+)
+
+// R-492 — the global paths.hdd_path was deleted (it was empty on every box and every reader already
+// fell back to the storage registry). The consequence that matters is the OLD box: a controller.yaml
+// written before the deletion that still carries `hdd_path` (empty or not) must keep loading, with
+// every other field intact — a config that fails to parse is a controller that does not start.
+// RED-PROOF (REPORT): switch loadAndParse to a strict decoder (yaml KnownFields) and this fails.
+func TestR492_OldConfigWithHDDPathStillLoads(t *testing.T) {
+	for _, v := range []string{`""`, `/mnt/legacy_hdd`} {
+		y := minimalYAML(bcryptHash) + "paths:\n  stacks_dir: /opt/stacks\n  hdd_path: " + v + "\n"
+		cfg, err := LoadFromBytes([]byte(y))
+		if err != nil {
+			t.Fatalf("hdd_path=%s: an old config no longer loads: %v", v, err)
+		}
+		if cfg.Paths.StacksDir != filepath.FromSlash("/opt/stacks") && cfg.Paths.StacksDir != "/opt/stacks" {
+			t.Errorf("hdd_path=%s: neighbouring field lost: stacks_dir=%q", v, cfg.Paths.StacksDir)
+		}
+		if cfg.Customer.ID != "demo" {
+			t.Errorf("hdd_path=%s: customer.id lost: %q", v, cfg.Customer.ID)
+		}
+	}
+}
@@ -76,7 +76,7 @@ func RunHealthCheck(cfg *config.Config, cpuCollector *system.CPUCollector, stora

 	debug := cfg.Logging.Level == "debug" && logger != nil

-	hddPath := cfg.Paths.HDDPath
+	hddPath := "" // R-492: the global paths.hdd_path is gone; the registry is the only source
 	if len(storagePaths) > 0 {
 		hddPath = storagePaths[0].Path
 	}
@@ -66,7 +66,7 @@ func BuildReport(

 	// System info
 	staticInfo := metrics.GetStaticInfo()
-	hddPath := cfg.Paths.HDDPath
+	hddPath := "" // R-492: the global paths.hdd_path is gone; the registry is the only source
 	if len(storagePaths) > 0 {
 		hddPath = storagePaths[0].Path
 	}
@@ -1738,7 +1738,6 @@ func (s *Settings) SetStorageLabel(path, label string) error {
 // already in the registry. It is ADDITIVE: pre-existing entries are never removed,
 // modified, or reactivated.
 //   - discoveredPaths are pre-scanned HDD_PATH values from deployed apps' app.yaml.
-//   - fallbackHDDPath is the legacy controller.yaml paths.hdd_path (may be empty).
 //
 // Invariants:
 //   - A path already present in the registry IN ANY STATE (including a Decommissioned
@@ -1747,12 +1746,12 @@ func (s *Settings) SetStorageLabel(path, label string) error {
 //   - IsDefault is never flipped on an existing entry. A newly-discovered path becomes
 //     default ONLY if the registry currently has no default at all (and then only the
 //     first such new path).
-func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, fallbackHDDPath string, logger *log.Logger) {
+func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, logger *log.Logger) {
 	s.mu.Lock()
 	defer s.mu.Unlock()

 	if s.debug {
-		s.log.Printf("[DEBUG] [settings] AutoDiscoverStoragePaths discovered=%v fallback=%q existing=%d", discoveredPaths, fallbackHDDPath, len(s.StoragePaths))
+		s.log.Printf("[DEBUG] [settings] AutoDiscoverStoragePaths discovered=%v existing=%d", discoveredPaths, len(s.StoragePaths))
 	}

 	// Index existing paths (in ANY state) and whether a default already exists.
@@ -1765,7 +1764,7 @@ func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, fallbackHD
 		}
 	}

-	// Build the de-duplicated, cleaned candidate list (discovered first, then fallback).
+	// Build the de-duplicated, cleaned candidate list.
 	seen := make(map[string]bool)
 	var ordered []string
 	for _, p := range discoveredPaths {
@@ -1775,13 +1774,6 @@ func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, fallbackHD
 			ordered = append(ordered, cleaned)
 		}
 	}
-	if fallbackHDDPath != "" {
-		cleaned := filepath.Clean(fallbackHDDPath)
-		if cleaned != "" && cleaned != "." && !seen[cleaned] {
-			seen[cleaned] = true
-			ordered = append(ordered, cleaned)
-		}
-	}

 	added := 0
 	for _, path := range ordered {
@@ -30,7 +30,6 @@ func TestAutoDiscoverStoragePaths_Additive(t *testing.T) {
 		name        string
 		existing    []StoragePath
 		discovered  []string
-		fallback    string
 		wantPaths   []string // expected Path set after discovery (order-insensitive)
 		wantNewPath string   // a path expected to be newly added (may be "")
 		// assertions run against the resulting registry
@@ -101,17 +100,6 @@ func TestAutoDiscoverStoragePaths_Additive(t *testing.T) {
 				}
 			},
 		},
-		{
-			name: "fallback path registered when missing",
-			existing: []StoragePath{
-				{Path: "/mnt/felhom-usb", Label: "x", IsDefault: true, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z"},
-			},
-			discovered:  nil,
-			fallback:    "/mnt/legacy_hdd",
-			wantPaths:   []string{"/mnt/felhom-usb", "/mnt/legacy_hdd"},
-			wantNewPath: "/mnt/legacy_hdd",
-			check:       nil,
-		},
 	}

 	for _, tc := range tests {
@@ -119,7 +107,7 @@ func TestAutoDiscoverStoragePaths_Additive(t *testing.T) {
 			s := newTestSettings(t, cloneStoragePaths(tc.existing))
 			before := cloneStoragePaths(tc.existing)

-			s.AutoDiscoverStoragePaths(tc.discovered, tc.fallback, logger)
+			s.AutoDiscoverStoragePaths(tc.discovered, logger)

 			// Normalize with filepath.Clean so comparisons hold on both Linux (the deploy
 			// target) and Windows (the dev machine), where Clean uses backslashes.
@@ -158,7 +146,7 @@ func TestAutoDiscoverStoragePaths_DecommissionedNotReactivated(t *testing.T) {
 	before := cloneStoragePaths(existing)

 	// A deployed app still points at the decommissioned drive.
-	s.AutoDiscoverStoragePaths([]string{"/mnt/old_hdd", "/mnt/felhom-usb"}, "", logger)
+	s.AutoDiscoverStoragePaths([]string{"/mnt/old_hdd", "/mnt/felhom-usb"}, logger)

 	if len(s.StoragePaths) != 2 {
 		t.Fatalf("path count changed: got %d want 2 (%v)", len(s.StoragePaths), pathList(s))
@@ -399,15 +399,13 @@ func TestDeleteStack_R442_RemovesDeclaredDriveData(t *testing.T) {
 }

 // GetStackHDDData (the modal's source) reads the per-app record too: with HDD_PATH recorded it
-// lists the folder; the global config stays empty throughout.
+// lists the folder; there is no global config value to fall back to (deleted in R-492).
 func TestGetStackHDDData_R442_ReadsPerAppRecord(t *testing.T) {
 	drive := t.TempDir()
 	m, _, _ := newR442Manager(t, "app", driveCompose, driveAppYAML(drive), drive)
 	data := filepath.Join(drive, "appdata", "app")
 	plantFile(t, filepath.Join(data, "one.bin"))
-	if m.cfg.Paths.HDDPath != "" {
-		t.Fatal("fixture error: the global must stay empty to prove the per-app read")
-	}
+	// (R-492: the global paths.hdd_path no longer exists, so the per-app record is the only source.)
 	resp, err := m.GetStackHDDData("app")
 	if err != nil {
 		t.Fatal(err)
@@ -982,12 +982,10 @@ func (s *Server) findStackBySubdomain(subdomain string) (*stacks.Stack, bool) {
 	return nil, false
 }

-// primaryHDDPath returns the default storage path, or the legacy config value.
+// primaryHDDPath returns the default storage path ("" when none is registered — R-492 deleted the
+// always-empty legacy config value it used to fall back to).
 func (s *Server) primaryHDDPath() string {
-	if p := s.settings.GetDefaultStoragePath(); p != "" {
-		return p
-	}
-	return s.cfg.Paths.HDDPath
+	return s.settings.GetDefaultStoragePath()
 }

 func (s *Server) render(w http.ResponseWriter, name string, data interface{}) {