From 7b8216abac12e952c947cceabcd6c6226e24a1dd Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 5 Oct 2026 21:45:53 +0200 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- REUSE.md | 2 +- controller/cmd/controller/main.go | 7 ++--- controller/internal/api/router.go | 6 ++--- controller/internal/config/config.go | 2 -- .../config/r492_hdd_path_gone_test.go | 27 +++++++++++++++++++ controller/internal/monitor/healthcheck.go | 2 +- controller/internal/report/builder.go | 2 +- controller/internal/settings/settings.go | 14 +++------- .../settings/storage_discovery_test.go | 16 ++--------- .../internal/stacks/delete_r442_test.go | 6 ++--- controller/internal/web/server.go | 8 +++--- 11 files changed, 45 insertions(+), 47 deletions(-) create mode 100644 controller/internal/config/r492_hdd_path_gone_test.go diff --git a/REUSE.md b/REUSE.md index afea11e..b6be94a 100644 --- a/REUSE.md +++ b/REUSE.md @@ -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 | diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 14994b4..eba1901 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -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() diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index 801068b..1693336 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -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 diff --git a/controller/internal/config/config.go b/controller/internal/config/config.go index b6741aa..e3b6014 100644 --- a/controller/internal/config/config.go +++ b/controller/internal/config/config.go @@ -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) } diff --git a/controller/internal/config/r492_hdd_path_gone_test.go b/controller/internal/config/r492_hdd_path_gone_test.go new file mode 100644 index 0000000..db4751c --- /dev/null +++ b/controller/internal/config/r492_hdd_path_gone_test.go @@ -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) + } + } +} diff --git a/controller/internal/monitor/healthcheck.go b/controller/internal/monitor/healthcheck.go index e6fa5b2..18006c6 100644 --- a/controller/internal/monitor/healthcheck.go +++ b/controller/internal/monitor/healthcheck.go @@ -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 } diff --git a/controller/internal/report/builder.go b/controller/internal/report/builder.go index 0224e26..4e45abd 100644 --- a/controller/internal/report/builder.go +++ b/controller/internal/report/builder.go @@ -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 } diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index d244fea..23a9fd9 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -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 { diff --git a/controller/internal/settings/storage_discovery_test.go b/controller/internal/settings/storage_discovery_test.go index f82b897..4b5297c 100644 --- a/controller/internal/settings/storage_discovery_test.go +++ b/controller/internal/settings/storage_discovery_test.go @@ -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)) diff --git a/controller/internal/stacks/delete_r442_test.go b/controller/internal/stacks/delete_r442_test.go index 647fbfa..9ce5e28 100644 --- a/controller/internal/stacks/delete_r442_test.go +++ b/controller/internal/stacks/delete_r442_test.go @@ -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) diff --git a/controller/internal/web/server.go b/controller/internal/web/server.go index d09fc9c..f6e03a6 100644 --- a/controller/internal/web/server.go +++ b/controller/internal/web/server.go @@ -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{}) {