From ac3790a11b6f5ada6e1c5203e230177d430df964 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 21 Jul 2026 14:53:11 +0200 Subject: [PATCH] gate: the boot bind gate honours a customer's Stop (R-55, v0.157.0) shouldRecreateOnBoot keyed on Deployed+drive-present alone. Deployed stays true across a Stop, so a drive-backed app the customer switched off was silently restarted on every guest reboot (proven live: immich). Requires len(Containers)>0 as well - R-52's existing-Exited vs absent distinction. A UI Stop is compose down and removes the containers; a guest that went down under a running app leaves them. Container STATE is still deliberately NOT a filter: that would miss a not-yet-restarted or stuck-Exited app, which is the bug the boot-id path exists to fix. Evidence sampled before any recreate - recreate's own StopStack erases it. Honoured Stops counted and logged separately from no-live-bind skips. --- CHANGELOG.md | 43 +++++++- controller/README.md | 14 +++ controller/internal/web/intermediary.go | 74 ++++++++++--- controller/internal/web/intermediary_test.go | 110 ++++++++++++++----- 4 files changed, 200 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5202382..31f78ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,47 @@ ## Changelog -### v0.156.0 — a dead primary alerts (R-51); a boot orphan restarts itself (R-52) (2026-07-21) +### v0.157.0 — the boot bind gate honours a customer's Stop (R-55) (2026-07-21) + +**Your Stop now means Stop across a guest reboot for drive-backed apps too** — the guarantee R-52 +already gave every other app. Found by STOP-1's R-52 leg on 2026-07-21, which was designed to prove +the opposite: immich, stopped from the UI seconds earlier, came back running after the reboot. + +The boot bind gate (`internal/web/intermediary.go`) keyed its recreate on +`Deployed && HDD_PATH && drive-present` alone. `Deployed` is a deploy-lifecycle flag — it stays true +across a Stop — so the gate had no way to tell "the guest went down under this app" from "the +customer switched this off", and it resurrected both. R-52 was never implicated: its own gate behaved +exactly as specified (immich, at zero containers, was never a candidate for it). The gate simply +reaches every drive-backed app first. + +**The fix is R-52's own predicate, translated.** `shouldRecreateOnBoot` now also requires +`len(Stack.Containers) > 0` (from `docker ps -a`, so `Exited` containers count): + +- containers EXIST but are down → the guest went down under the app; docker's records survive the + reboot → boot orphan → recreate, as before. +- ZERO containers → a UI Stop is `compose down`, which REMOVES the containers → deliberate → leave it. + +**What deliberately did NOT change: container STATE is still not a filter.** That is the original +design's load-bearing part — a `State != stopped` filter misses an app that simply hasn't been +auto-restarted yet after the boot, or is stuck `Exited` on a create-time bind failure with +`RestartCount=0`. `hasContainers` is a different question ("does docker still have records of it") +and, unlike liveness, it survives a reboot as a statement of intent. `TestShouldRecreateOnBoot` now +pins both axes at once — they pull in opposite directions, which is the whole difficulty of this gate. + +- **Ordering trap, handled:** the evidence is sampled into the `bootStack` snapshot BEFORE any + recreate runs, because `recreate` calls `StopStack` (`compose down`) and so destroys the very + signal the decision needs. +- **The drive-absent gate is not regressed.** Apps it stopped are also at zero containers, so this + path now skips them — correctly: they are recorded in `StoragePath.StoppedStacks` and restarted by + `ReconcileDriveGates`' `Return` branch, which runs on the same `driveGateLoop` tick. +- **Honoured Stops are observable.** `leftStopped` is counted and logged separately from `skipped` + at INFO (`… left stopped — zero containers means the customer stopped them on purpose`). + Conflating them would have fired a WARN about a missing drive bind for an app behaving exactly as + asked, and a silent correct path is how an inert seam hides. +- **Red-proof (run):** dropping `hasContainers` from the predicate makes + `TestRecreateDriveBackedApps_HonoursCustomerStop` fail with `recreated=[romm immich]` — the live + defect, by name. + +## v0.156.0 — a dead primary alerts (R-51); a boot orphan restarts itself (R-52) (2026-07-21) **No new agent coupling — MinAgent stays 0.90.0.** Two independent failures from the same live audit, both unattended-resilience holes: the box was broken and nobody was told, then the box could diff --git a/controller/README.md b/controller/README.md index a7301a8..ab2e192 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1113,6 +1113,20 @@ not just those with HDD data. Non-HDD apps can configure destination, method, an > covered; drives that never go live in the window are left to the drive-absent gate. `processGuestBootChange` > also runs on every periodic `driveGateLoop` tick now (idempotent, boot-id gated) so a momentarily-unreachable > agent right after a guest reboot no longer permanently strands recovery. +> **v0.157.0 — R-55, the gate now honours a customer's Stop.** Until this version the recreate keyed on +> `Deployed && HDD_PATH && drive-present` alone, so a drive-backed app the customer had deliberately +> Stopped was silently restarted on every guest reboot (proven live: immich, stopped from the UI seconds +> earlier, came back running). `shouldRecreateOnBoot` now also requires the app to still HAVE containers +> (`len(Stack.Containers) > 0`, from `docker ps -a`, so `Exited` ones count) — R-52's `existing-Exited vs +> absent` distinction (`bootrecon.isBootOrphan`) translated to this gate. A UI Stop is `compose down`, +> which REMOVES the containers; a guest that went down under a running app leaves them present. **State is +> still NOT a filter** — that part of the original design is load-bearing and unchanged; `hasContainers` +> answers a different question ("does docker still have records of it") which, unlike liveness, survives a +> reboot as a statement of intent. The evidence is sampled BEFORE any recreate, because recreate's own +> `StopStack` erases it. Apps stopped by the drive-absent gate are also at zero containers and are likewise +> left alone here — they are restored by `ReconcileDriveGates`' `Return` branch from +> `StoragePath.StoppedStacks`, on the same loop tick. Honoured Stops are logged at INFO (`left stopped …`), +> counted separately from the "no live bind" skips so an intended outcome never fires a WARN. > **Agent-path prerequisite (also v0.71.0):** the whole drive gate needs `cfg.LocalAPI.Endpoint` (the > per-guest agent local API). `bootstrap.MaybeIngest` now calls `ensureLocalAPI` on the already-configured > path — merging `local_api` from `bootstrap.json` into an existing controller.yaml that lacks it (seeded diff --git a/controller/internal/web/intermediary.go b/controller/internal/web/intermediary.go index bb5e01d..b45040c 100644 --- a/controller/internal/web/intermediary.go +++ b/controller/internal/web/intermediary.go @@ -98,15 +98,39 @@ func agentWhere(registeredPath string) string { } // shouldRecreateOnBoot is the PURE decision for the boot-id recreate: on a fresh guest boot, recreate -// EVERY deployed drive-backed app whose drive is present (BoundUnderParent) onto its (re-propagated) -// drive. It is DETERMINISTIC — it depends ONLY on `app.yaml says should run` (deployed) + drive-present, -// NOT on the app's current container state. The current state must NOT be a filter: a momentarily-stopped -// app on a fresh reboot (docker hasn't auto-restarted it yet) would otherwise be MISSED — the exact bug -// the boot-id path replaces (the old container-uptime sample, and a State!=stopped filter, both miss it). -// (Tradeoff: a UI-stopped drive-backed app is brought back on a guest reboot — `deployed` is the only -// "should run" signal app.yaml carries; the gate manages drive-backed app lifecycle otherwise.) -func shouldRecreateOnBoot(deployed bool, hdd string, presentStable map[string]bool) bool { - return deployed && hdd != "" && strings.HasPrefix(hdd, StableParentDir+"/") && presentStable[hdd] +// a deployed drive-backed app whose drive is present (BoundUnderParent) onto its (re-propagated) drive. +// +// It is DETERMINISTIC and it still does NOT filter on the app's current container STATE. That remains +// load-bearing: a momentarily-stopped app on a fresh reboot (docker hasn't auto-restarted it yet, or +// its create-time bind failed with RestartCount=0) must be recreated, and both the old +// container-uptime sample and a `State != stopped` filter MISS exactly that case. Do not reintroduce +// a state filter here. +// +// R-55: it DOES filter on whether the app still HAS containers, which is a different question and the +// one that tells the truth about intent. This is R-52's `existing-Exited vs absent` distinction +// (bootrecon.isBootOrphan), translated to this gate: +// +// - containers EXIST but are down → the guest went down under the app; docker's own records survive +// the reboot, so this is a boot orphan → recreate. +// - ZERO containers → a UI Stop is `compose down`, which REMOVES the containers. +// Nothing else in the controller leaves a deployed app at zero containers. → the customer stopped +// this on purpose → LEAVE IT ALONE. +// +// `deployed` cannot answer this: it is a deploy-lifecycle flag and stays true across a Stop. Before +// R-55 the gate had no other signal and therefore silently undid a customer's Stop on every guest +// reboot — including when apps were stopped deliberately to free resources for others, which is +// precisely when resurrecting them is most harmful. +// +// The evidence is read from the snapshot taken BEFORE any recreate runs, because `recreate` itself +// calls StopStack (`compose down`) and so destroys it. +// +// NOTE on the drive-absent gate: apps it stopped are also at zero containers, so they are skipped +// here too. That is correct — they are recorded in StoragePath.StoppedStacks and restarted by +// ReconcileDriveGates' `Return` branch, which runs on the same loop tick. Their recovery is that +// path's job, not this one's. +func shouldRecreateOnBoot(deployed bool, hdd string, presentStable map[string]bool, hasContainers bool) bool { + return deployed && hdd != "" && strings.HasPrefix(hdd, StableParentDir+"/") && + presentStable[hdd] && hasContainers } // defaultPromotionTarget decides M1 (never leave zero default). If the path being decommissioned is NOT @@ -383,7 +407,11 @@ func (s *Server) processGuestBootChange() { if cfg == nil { continue } - bootStacks = append(bootStacks, bootStack{name: st.Name, deployed: cfg.Deployed, hdd: cfg.Env["HDD_PATH"], state: string(st.State)}) + // hasContainers is sampled HERE, before any recreate — recreate's StopStack destroys it (R-55). + bootStacks = append(bootStacks, bootStack{ + name: st.Name, deployed: cfg.Deployed, hdd: cfg.Env["HDD_PATH"], state: string(st.State), + hasContainers: len(st.Containers) > 0, + }) } recreate := func(bs bootStack) { s.logger.Printf("[INFO] [gate] boot %s: live bind confirmed — recreating drive-backed app %s (state=%s) onto %s", resp.GuestBootID, bs.name, bs.state, bs.hdd) @@ -396,10 +424,15 @@ func (s *Server) processGuestBootChange() { s.logger.Printf("[INFO] [gate] boot %s: re-syncing FileBrowser mounts against the live binds", resp.GuestBootID) go s.SyncFileBrowserMounts() } - _, skipped := recreateDriveBackedApps(bootStacks, presentStable, recreate, syncFB) + _, skipped, leftStopped := recreateDriveBackedApps(bootStacks, presentStable, recreate, syncFB) if skipped > 0 { s.logger.Printf("[WARN] [gate] boot %s: %d drive-backed app(s) had no live bind within %s — leaving to the drive gate", resp.GuestBootID, skipped, bootBindWait) } + if leftStopped > 0 { + // INFO, not WARN: this is the gate working as intended (R-55). Make the honoured path + // observable — a silent correct path is how an inert seam hides. + s.logger.Printf("[INFO] [gate] boot %s: %d drive-backed app(s) left stopped — zero containers means the customer stopped them on purpose", resp.GuestBootID, leftStopped) + } if serr := s.settings.SetLastGuestBootID(resp.GuestBootID); serr != nil { s.logger.Printf("[WARN] [gate] persist boot-id: %v", serr) } @@ -411,6 +444,10 @@ type bootStack struct { deployed bool hdd string state string + // hasContainers is len(Stack.Containers) > 0, from `docker ps -a` — so Exited containers COUNT. + // R-55's running-at-shutdown signal: a UI Stop is `compose down` and leaves zero. MUST be sampled + // before any recreate runs, since recreate's StopStack erases it. + hasContainers bool } // recreateDriveBackedApps recreates every deployed drive-backed app whose drive bind is live, then @@ -419,11 +456,20 @@ type bootStack struct { // ran once pollLiveBinds confirmed the live binds), so FileBrowser's mounts reflect the now-live drives // instead of going stale (the gap a host/guest reboot left before this fix). syncFB is always called so // FileBrowser reflects the current bind state even if no app needed recreating. Pure (ops injected). -func recreateDriveBackedApps(stacks []bootStack, presentStable map[string]bool, recreate func(bootStack), syncFB func()) (recreated, skipped int) { +// R-55: `leftStopped` counts drive-backed apps deliberately NOT touched because they have zero +// containers (a customer Stop). It is reported separately from `skipped` — conflating the two would +// make an honoured Stop look like the "bind never went live" failure and fire a WARN for healthy, +// intended behaviour. +func recreateDriveBackedApps(stacks []bootStack, presentStable map[string]bool, recreate func(bootStack), syncFB func()) (recreated, skipped, leftStopped int) { for _, bs := range stacks { - if !shouldRecreateOnBoot(bs.deployed, bs.hdd, presentStable) { + if !shouldRecreateOnBoot(bs.deployed, bs.hdd, presentStable, bs.hasContainers) { if bs.deployed && strings.HasPrefix(bs.hdd, StableParentDir+"/") { - skipped++ // a deployed drive-backed app whose bind never went live → gate's job + switch { + case presentStable[bs.hdd] && !bs.hasContainers: + leftStopped++ // drive IS live; the app is at zero containers → stopped on purpose + default: + skipped++ // a deployed drive-backed app whose bind never went live → gate's job + } } continue } diff --git a/controller/internal/web/intermediary_test.go b/controller/internal/web/intermediary_test.go index efe43bd..32378d5 100644 --- a/controller/internal/web/intermediary_test.go +++ b/controller/internal/web/intermediary_test.go @@ -21,34 +21,92 @@ func TestAgentWhere(t *testing.T) { } } -// TestShouldRecreateOnBoot pins the DETERMINISTIC boot-id recreate decision: recreate EVERY deployed -// drive-backed present app, independent of its current container state. +// TestShouldRecreateOnBoot pins the boot-id recreate decision on BOTH axes it must get right at once: +// recreate every deployed drive-backed present app that still HAS containers, independent of what +// STATE those containers are in. // -// COMPANION GUARD: the pre-fix logic (the old `stackStartedRecently`, and even a `State!=stopped` filter) -// MISSED an app that was Stopped/Exited at the one-shot instant — docker hadn't auto-restarted it yet -// after the boot. The "exited" and "stopped" cases below are `true` here: a state-filtered impl returns -// false for them and the app stays down (exactly what happened live: 5 apps exited after a host reboot). +// COMPANION GUARD 1 (state must NOT filter): the pre-fix logic (the old `stackStartedRecently`, and +// even a `State!=stopped` filter) MISSED an app that was Stopped/Exited at the one-shot instant — +// docker hadn't auto-restarted it yet after the boot. The exited/stopped cases below are `true`: a +// state-filtered impl returns false for them and the app stays down (exactly what happened live: 5 +// apps exited after a host reboot). +// +// COMPANION GUARD 2 (R-55: containers MUST filter): an impl that ignores `hasContainers` resurrects +// an app the customer deliberately stopped, on every guest reboot. The zero-container cases below are +// `false`; the pre-R-55 impl returns true for them (proven live: immich, stopped from the UI seconds +// earlier, came back running). +// +// The two guards pull in opposite directions on purpose — that is the whole difficulty of this gate. +// `state` and `hasContainers` are DIFFERENT questions: "is it up right now" vs "does docker still +// have records of it", and only the second survives a reboot as a statement of intent. func TestShouldRecreateOnBoot(t *testing.T) { - present := map[string]bool{"/mnt/felhom-drives/felhom-flash": true} + const flash = "/mnt/felhom-drives/felhom-flash" + present := map[string]bool{flash: true} cases := []struct { - name string - deployed bool - hdd string - want bool + name string + deployed bool + hdd string + hasContainers bool + want bool }{ - {"deployed+present (recreate regardless of state)", true, "/mnt/felhom-drives/felhom-flash", true}, - {"drive absent (gate handles)", true, "/mnt/felhom-drives/felhom-usb", false}, - {"SSD path never", true, "/mnt/sys_drive/felhom-data", false}, - {"app.yaml not deployed", false, "/mnt/felhom-drives/felhom-flash", false}, - {"no HDD_PATH (SSD-resident)", true, "", false}, + // --- has containers: recreate regardless of what state they are in (guard 1) --- + {"deployed+present+containers (running)", true, flash, true, true}, + {"deployed+present+containers (exited after boot)", true, flash, true, true}, + {"deployed+present+containers (stuck create-time failure)", true, flash, true, true}, + + // --- R-55: zero containers == `compose down` == the customer's Stop (guard 2) --- + {"R-55 customer-stopped drive app (zero containers)", true, flash, false, false}, + {"R-55 gate-stopped app (zero containers; Return branch owns it)", true, flash, false, false}, + + // --- the pre-existing axes, unchanged --- + {"drive absent (gate handles)", true, "/mnt/felhom-drives/felhom-usb", true, false}, + {"SSD path never", true, "/mnt/sys_drive/felhom-data", true, false}, + {"app.yaml not deployed", false, flash, true, false}, + {"no HDD_PATH (SSD-resident)", true, "", true, false}, } for _, c := range cases { - if got := shouldRecreateOnBoot(c.deployed, c.hdd, present); got != c.want { + if got := shouldRecreateOnBoot(c.deployed, c.hdd, present, c.hasContainers); got != c.want { t.Errorf("%s: shouldRecreateOnBoot = %v, want %v", c.name, got, c.want) } } } +// TestRecreateDriveBackedApps_HonoursCustomerStop is the R-55 regression at the driver level, and it +// separates the two reasons an app is not recreated — a conflated counter would fire a WARN about a +// missing drive bind for an app that is stopped exactly as the customer asked. +// +// COMPANION GUARD: drop `hasContainers` from the predicate and `immich` is recreated — the exact live +// defect (REPORT §4b). Drop the counter split and `leftStopped` reads 0 while `skipped` reads 2. +func TestRecreateDriveBackedApps_HonoursCustomerStop(t *testing.T) { + const flash = "/mnt/felhom-drives/felhom-flash" + present := map[string]bool{flash: true} + stacks := []bootStack{ + // boot orphan: containers exist but are down → MUST be recreated. + {name: "romm", deployed: true, hdd: flash, state: "exited", hasContainers: true}, + // customer pressed Stop → compose down → zero containers → MUST be left alone. + {name: "immich", deployed: true, hdd: flash, state: "stopped", hasContainers: false}, + // drive-backed but the bind never went live → the drive gate's job, counted separately. + {name: "stranded", deployed: true, hdd: "/mnt/felhom-drives/felhom-usb", hasContainers: true}, + } + var recreated []string + n, skipped, leftStopped := recreateDriveBackedApps(stacks, present, + func(bs bootStack) { recreated = append(recreated, bs.name) }, func() {}) + + if len(recreated) != 1 || recreated[0] != "romm" { + t.Fatalf("recreated=%v, want exactly [romm] — a customer-stopped app must never be restarted", recreated) + } + for _, name := range recreated { + if name == "immich" { + t.Fatal("immich was stopped from the UI and was recreated anyway — R-55 regression") + } + } + if n != 1 || skipped != 1 || leftStopped != 1 { + t.Fatalf("recreated=%d skipped=%d leftStopped=%d, want 1/1/1 "+ + "(skipped = no live bind; leftStopped = honoured customer Stop — these must not be conflated)", + n, skipped, leftStopped) + } +} + // TestDefaultPromotionTarget pins M1 (never leave zero default). // // COMPANION GUARD: the pre-fix decommission blanked the default and promoted nothing — equivalent to this @@ -100,7 +158,7 @@ func TestPollLiveBinds_WaitsForLateBind(t *testing.T) { if nowT < goLive { // proves it WAITED through the rebind window (a single sample would return at t=0) t.Fatalf("poll returned at t=%s before the bind went live at %s — it did not wait (regression)", nowT, goLive) } - if !shouldRecreateOnBoot(true, flash, live) { + if !shouldRecreateOnBoot(true, flash, live, true) { t.Fatalf("with the live bind present, the drive-backed app MUST be recreated") } } @@ -112,7 +170,7 @@ func TestSingleEarlySample_MissesLateBind_Companion(t *testing.T) { flash := "/mnt/felhom-drives/felhom-flash" bindLiveAtBoot := func(string) bool { return false } // not yet live at the boot instant oldPresent := map[string]bool{flash: bindLiveAtBoot(flash)} - if shouldRecreateOnBoot(true, flash, oldPresent) { + if shouldRecreateOnBoot(true, flash, oldPresent, true) { t.Fatalf("companion: a single early sample reads the not-yet-live bind as absent and must MISS it") } } @@ -134,7 +192,7 @@ func TestPollLiveBinds_TimeoutLeavesAbsent(t *testing.T) { if nowT < bootBindWait { t.Fatalf("poll must run to the deadline for an absent drive, t=%s", nowT) } - if shouldRecreateOnBoot(true, usb, live) { + if shouldRecreateOnBoot(true, usb, live, true) { t.Fatalf("an absent drive's app must NOT be recreated here (the gate owns drive-absent)") } } @@ -146,18 +204,18 @@ func TestRecreateDriveBackedApps_SyncsFileBrowserAfterRecreate(t *testing.T) { flash := "/mnt/felhom-drives/felhom-flash" present := map[string]bool{flash: true} stacks := []bootStack{ - {name: "romm", deployed: true, hdd: flash}, // drive-backed, live → recreate - {name: "actualbudget", deployed: true, hdd: "/mnt/sys_drive/felhom-data"}, // SSD → not recreated - {name: "stranded", deployed: true, hdd: "/mnt/felhom-drives/felhom-usb"}, // drive-backed, bind NOT live → skipped + {name: "romm", deployed: true, hdd: flash, hasContainers: true}, // drive-backed, live → recreate + {name: "actualbudget", deployed: true, hdd: "/mnt/sys_drive/felhom-data", hasContainers: true}, // SSD → not recreated + {name: "stranded", deployed: true, hdd: "/mnt/felhom-drives/felhom-usb", hasContainers: true}, // drive-backed, bind NOT live → skipped } var seq []string recreate := func(bs bootStack) { seq = append(seq, "recreate:"+bs.name) } syncFB := func() { seq = append(seq, "syncFB") } - recreated, skipped := recreateDriveBackedApps(stacks, present, recreate, syncFB) + recreated, skipped, leftStopped := recreateDriveBackedApps(stacks, present, recreate, syncFB) - if recreated != 1 || skipped != 1 { - t.Fatalf("recreated=%d skipped=%d, want 1/1", recreated, skipped) + if recreated != 1 || skipped != 1 || leftStopped != 0 { + t.Fatalf("recreated=%d skipped=%d leftStopped=%d, want 1/1/0", recreated, skipped, leftStopped) } // FileBrowser sync MUST be invoked, and AFTER every recreate. if len(seq) != 2 || seq[0] != "recreate:romm" || seq[len(seq)-1] != "syncFB" {