diff --git a/CHANGELOG.md b/CHANGELOG.md index ba10576..501d7af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,82 @@ ## Changelog +### v0.190.0 — the boot-recovery story finished, and a regression v0.189.0 opened (2026-08-02, R-157 A · R-170 · R-171) + +**R-171 — a regression introduced by v0.189.0, found by reading the diff and CONFIRMED on hardware +before anything was written.** v0.189.0 correctly replaced `isBootOrphan`'s `len(Containers) > 0` +term with the customer's recorded intent. But the **drive-absent gate** stops apps with `compose +down` (zero containers) and never touches `desired_state`, because it is not the customer — so a +gate-stopped app began reading as a boot orphan. Observed live on guest 9201 with the drive held +unmounted: + +``` +[gate] drive ABSENT /mnt/felhom-drives/hdd_1 — stopped+blocked 2 app(s): [calibre-web immich] +[bootrecon] Boot reconciliation: 1 boot-orphaned app(s) found: [calibre-web] — up to 2 attempt(s) +[bootrecon] attempt 1/2: start "calibre-web" failed … attempt 2/2: … gave up +[bootrecon] recovered=[] still down=[calibre-web] (the dead-app alarm now owns these) +``` + +The **write** hazard did not materialise: compose failed with `mkdir /mnt/felhom-drives/hdd_1/ +userdata: permission denied`, because the unbound mountpoint is host-root-owned and the guest is +unprivileged. **That protection is accidental** — no code chose it, no test pinned it, and it is one +`chown` (or one privileged guest) away from gone. The harm that DID occur is real on every box: two +wasted attempts and a **false dead-app alarm for an app the drive gate is deliberately holding**. + +The fix is not a new rule. The **API's own start path already refuses this** — +`startGatedByMissingDrive` returns a Hungarian refusal to the customer — and the sweep bypassed it by +calling `Manager.StartStack` directly. New consumer-side seam `bootrecon.StartGate`, wired in +`main.go`, gives the sweep the same question to ask. Fail-safe by contract: **cannot determine ⇒ do +not start.** New `Manager.DriveLive` reuses the userdata belt's own `isMountPoint` seam so the two +cannot drift. Held apps are reported as `HeldByDrive`, deliberately **not** as `StillDown` — that is +the alarm's bucket and putting them there is the false alarm being removed. +Evidence: `felhom.eu/documentation/audits/DIAG-bootrecon-drive-absent-2026-08-02.md`. + +**R-157 mechanism A — the sweep that looked once.** `runBootReconcile` waited 5 s and swept exactly +once, deriving its candidate set from a fleet docker was still restoring; measured failing on **three +of six hard resets**. It is now a **settle-then-sweep window**: sample the fleet (name, state, +container count) every **5 s**, call it settled after **3 identical samples**, and sweep **once**, at +the end, on a settled fleet. The window terminates on whichever comes first — settled, or a **50 s +budget** — and the log says which, because "settled and found nothing" and "ran out of time still +churning" are different facts about the box. + +**The budget is 50 s and not 60 s because a test said so.** `bootReconcileSettle` (5 s) + budget + +one `DefaultRetryDelay` (30 s) must stay under `deadAppBootGrace` (90 s) so a successful recovery is +SILENT. 60 s was the first choice; `TestBootWindow_CommonCaseFitsInsideTheDeadAppGrace` rejected it +at 95 s. Extending the grace to fit was rejected outright (§8.3) — that hides a late recovery instead +of reporting it. A window that genuinely overruns now emits a **`LATE RECOVERY` WARN naming the +apps**, so a stale alarm never stands without counter-evidence. + +**Sampling is read-only and there is still exactly ONE sweep.** Sweeping per sample was rejected: the +sweep's own `StartStack` changes the fleet, so it would never observe a settled one. The per-app +attempt bound is untouched — this widens a bounded window, it does not remove the bound. + +**Widening the window made two more holders reachable (§8.2), so the gate covers all three.** The old +T+5 s sweep never overlapped a **quiesce** (starting an app mid-backup defeats the point of +quiescing) or a **running app-data operation** (restarting an app under its own tar). Both are now +refused through the same seam, reusing `quiesce.SuppressedStacks()` and a new read-only +`AppStopGuard.HeldStacks()` rather than second implementations. + +**R-170 — the second boot gate stops guessing.** `shouldRecreateOnBoot` still ended in +`&& hasContainers`, so the two boot gates disagreed about the same question. It now reads +`desired_state` with the identical three-way table: `stopped` → never; `running` → recreate whatever +the container count; **absent → exactly the pre-v0.190.0 `hasContainers` behaviour**. Its comment +argued at length *for* the container count and has been rewritten — a correct implementation under a +comment arguing the opposite is worse than either alone. **`presentStable` is untouched and still +load-bearing**: an app whose drive is absent is never recreated here, which is the very term the boot +sweep was missing. The agreement between the gates is pinned from **both sides** against an identical +fixture table (`TestBothBootGatesAgreeOnIntent` / `TestShouldRecreateOnBoot_AgreesWithBootrecon`), +because the two cannot be called from one package without an import cycle. + +**Tests: +25 across 3 packages (27/27 packages green).** Timing is tested by shrinking the window +constants, never by sleeping. Red-proofs, each observed FAIL then restored: A (restore the +single-sweep shape), B (remove the budget → the test **hangs**, the unbounded shape), C (drop the +`desired_state: stopped` branch → the customer-stopped app is started **twice** by the widened +window), D (restore `&& hasContainers`), G (remove the start-gate check → the drive-absent app is +started), H (comment out `SetDriveGate` → fails while the string is **still present**, which is what +the AST walk is for). + +No hub change, no agent coupling, no user-visible string, no backup/restore/catalogue change. + ### v0.189.0 — the box stops guessing what the customer wanted (2026-08-02, R-166 / decision D-b) **The defect.** When an app was not running, the controller had to work out *why*, and it worked it diff --git a/REUSE.md b/REUSE.md index 10ec8e6..b0dfa69 100644 --- a/REUSE.md +++ b/REUSE.md @@ -89,6 +89,9 @@ | `appbackup.DBServiceNames` / `dbTypeForImage` (R-47, v0.153.0) | controller/internal/appbackup/dbservices.go | `(composePath string) ([]string, error)` | naming the compose SERVICE(s) holding a database, sorted | yaml.v3 `services:` MAP parse — **never a line scan** (immich's top-level `immich_ml_cache:` / `immich_postgres_data:` volume keys look exactly like services). `dbTypeForImage` is shared with `DiscoverDatabases`, which is what makes "a dump exists ⇒ a service can be named" hold. An error means CANNOT-TELL, never "no database" — callers refuse when a dump exists | | `Manager.StartStack/StopStack/RestartStack/UpdateStack` | controller/internal/stacks/manager.go | `(name string) error` | Lifecycle | Protected stacks refuse stop; all funnel through composeExec. **NOT writers of desired state (R-166)** — 14 call sites, only 2 are the customer; recording intent here would make a nightly backup indistinguishable from the customer pressing Stop. Use `SetDesiredState` at the intent point instead | | `Manager.SetDesiredState` / `DesiredStateOf` / `BackfillDesiredState` (R-166, v0.189.0) | controller/internal/stacks/desiredstate.go | `(name, desired string) error` / `(Stack) string` / `() int` | THE customer-intent record — `app.yaml` `desired_state`, tri-state `""`/`running`/`stopped` | **ONE OWNER: the customer's action.** Writers are the API action switch, `DeployStack`, `UpdateOptionalConfig`'s redeploy branch, and the `.fab` restore adapter — nothing else, ever. **`""` (absent) means UNKNOWN, never "running"**: every pre-v0.189.0 app.yaml reads absent, so treating it as running would start every deliberately-stopped app on upgrade. Write intent BEFORE the act and REFUSE the act if it fails (§8.2). Backfill is **running-only** — never infer `stopped` from zero containers, that inference IS the defect | +| `Manager.DriveLive` (R-171, v0.190.0) | controller/internal/stacks/deploy.go | `(hddPath string) bool` | is an app's data drive a live mountpoint RIGHT NOW | Wraps the **same** `isMountPoint` seam the userdata belt uses (`manager.go`) — never write a second liveness check, the two would drift invisibly. The system/local path is legitimately not a mountpoint and returns true | +| `bootrecon.StartGate` (R-171, v0.190.0) | controller/internal/bootrecon/bootrecon.go | `MayStart(stack) (bool, reason)` | THE one question the boot sweep asks before starting anything | **Fail-safe: cannot determine ⇒ return FALSE.** One seam for all three holders (absent drive · quiesce · an in-flight app-data operation) because they differ only in the reason string. Implemented in `main.go` (`bootDriveGate`) reusing `quiesce.SuppressedStacks()`, `AppStopGuard.HeldStacks()` and `Manager.DriveLive` — never re-derive any of them. Held apps go to `Result.HeldByDrive`, **never** `StillDown` (that is the dead-app alarm's bucket) | +| the boot settle window (R-157 A, v0.190.0) | controller/cmd/controller/main.go | `bootReconcileSample` / `StableFor` / `Budget` | sample the fleet until it stops changing, then sweep ONCE | **settle + budget + one `DefaultRetryDelay` must stay under `deadAppBootGrace`** — pinned by `TestBootWindow_CommonCaseFitsInsideTheDeadAppGrace`, which is why the budget is 50 s and not 60 s. Sampling is READ-ONLY; sweeping per sample would never see a settled fleet (the sweep's own StartStack changes it). A late recovery is REPORTED (`recordLateRecovery`), never hidden by widening the grace | | `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 | | `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 | | `resolveContainerState` / `aggregateState` | controller/internal/stacks/manager.go | `(dockerState, dockerStatus)` / `([]ContainerInfo)` | State classification | `.State` says "running" even when unhealthy — `.Status` parse is the fix | diff --git a/controller/README.md b/controller/README.md index da2b127..a0093c3 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1902,6 +1902,37 @@ upgrade. Where intent is unknown the sweep falls back to the old inference rathe answer, and a running-only startup **backfill** converges the unambiguous cases (deployed and observed up) without waiting for a button press. `stopped` is never backfilled from any signal. +**The sweep no longer looks only once (R-157 mechanism A, v0.190.0).** It used to sample the fleet at +T+5 s and return; at that moment docker is still restoring containers after a hard reset, so an app +that had not yet settled into a down state was never a candidate — **measured failing on three of six +hard resets**. It is now a **settle-then-sweep window**: the fleet (name, state, container count) is +sampled every 5 s, called settled after 3 identical samples, and swept **once**, at the end, on a +settled fleet. The window ends on whichever comes first — settled, or a **50 s budget** — and the log +says which. Sampling is read-only and there is still exactly one sweep with its per-app attempt bound +intact: this widens a bounded window, it does not remove the bound. `settle (5 s) + budget (50 s) + +one 30 s retry = 85 s` stays inside the 90 s `deadAppBootGrace`, which is what keeps a successful +recovery silent; a window that genuinely overruns emits a `LATE RECOVERY` WARN naming the apps rather +than the grace being widened to hide it. + +**The sweep asks before it starts (R-171, v0.190.0).** Three things legitimately hold an app down, +and starting it would be the wrong repair for all three: its **data drive is absent** (compose would +create the bind sources on the guest rootfs — the hazard the drive gate exists to prevent), a +**quiesce** is holding it for a whole-guest backup, or an **app-data operation** (volume dump, offsite +restore, `.fab` export) is holding it. All three are refused through one seam, reusing the signals +their owners already publish. **Fail-safe: a drive whose liveness cannot be determined is treated as +absent.** Held apps are reported separately from `StillDown` — they are not a fault the sweep failed +to fix, and reporting them as one is a false alarm. This closed a regression v0.189.0 introduced: +before it, a drive-gate-stopped app read as `running` + zero containers, so the sweep started it, +burned both attempts and handed it to the dead-app alarm. + +**Both boot gates read intent (R-170, v0.190.0).** There are two: the R-52 sweep above, and the +drive-backed **boot recreate gate** (`shouldRecreateOnBoot`, `internal/web/intermediary.go`), which +re-creates a drive-backed app onto its re-propagated drive after a guest reboot. Until v0.190.0 the +second still ended in `&& hasContainers`, so the two disagreed about the same question. It now uses +the identical three-way table — `stopped` → never, `running` → recreate whatever the container count, +absent → exactly the old `hasContainers` behaviour. Its drive-presence term is untouched and still +load-bearing: an app whose drive is absent is never recreated there either. + **Desired state — who owns it (R-166, v0.189.0).** `app.yaml` gains `desired_state`, a tri-state `""` / `running` / `stopped`. It is written by **the customer's own action and nothing else**: the `/api/stacks/{name}/{action}` switch (`start`/`restart`/`update` → running, `stop` → stopped), diff --git a/controller/cmd/controller/appstop_wiring_test.go b/controller/cmd/controller/appstop_wiring_test.go index e1533a7..2a796a9 100644 --- a/controller/cmd/controller/appstop_wiring_test.go +++ b/controller/cmd/controller/appstop_wiring_test.go @@ -166,3 +166,67 @@ func TestMainReportsTheInterruptedOperation(t *testing.T) { "healthy boot would page the operator about a backup that was never interrupted") } } + +// --- R-171 seam: the boot drive gate must be WIRED in production ------------------------------- + +// TestMainWiresBootDriveGate is the Group-H seam test. An unwired drive gate is not a crash — it is +// SILENTLY the pre-v0.190.0 behaviour, which started apps onto absent drives (observed live, +// audits/DIAG-bootrecon-drive-absent-2026-08-02.md). Every behavioural test in internal/bootrecon +// still passes with the wiring gone, which is exactly the hole this walks the AST to close. +// +// AST, not strings.Contains: a commented-out call still contains the string — the distinction that +// made a previous version of this project's own seam test pass its red-proof (2026-07-21). +func TestMainWiresBootDriveGate(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + + // (a) the settings handle the gate reads is assigned somewhere in main(). + assigned := false + for _, name := range assignedIdentsIn(mainBody(t)) { + if name == "bootDriveSettings" { + assigned = true + } + } + if !assigned { + t.Fatal("func main() no longer assigns bootDriveSettings — the boot drive gate would read a " + + "nil settings handle and could not see a disconnected drive") + } + + // (b) SetDriveGate is actually called where the reconciler is constructed. + called := false + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "SetDriveGate" { + called = true + } + return true + }) + if !called { + t.Fatal("main.go no longer calls SetDriveGate on the boot reconciler — the sweep would start " + + "apps whose data drive is absent (R-171, a regression observed live on 2026-08-02)") + } +} + +// assignedIdentsIn returns the names assigned to in a block (plain `=` and `:=`). +func assignedIdentsIn(body *ast.BlockStmt) []string { + var names []string + ast.Inspect(body, func(n ast.Node) bool { + as, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + for _, lhs := range as.Lhs { + if id, ok := lhs.(*ast.Ident); ok { + names = append(names, id.Name) + } + } + return true + }) + return names +} diff --git a/controller/cmd/controller/bootwindow_test.go b/controller/cmd/controller/bootwindow_test.go new file mode 100644 index 0000000..15f7799 --- /dev/null +++ b/controller/cmd/controller/bootwindow_test.go @@ -0,0 +1,311 @@ +package main + +import ( + "context" + "io" + "log" + "strings" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/bootrecon" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-157 mechanism A — the sweep that looked once. +// +// TIMING IS NOT TESTED BY SLEEPING (§10). The window's constants are package vars, so each test +// shrinks them to sub-millisecond values: the CONTRACT under test is "how many samples, and what +// ends the window", not "how long a second is". A test that waited real seconds would be slow, +// flaky, and would still not prove the contract. + +// windowStacks is a StackProvider whose fleet CHANGES over successive GetStacks() calls — which is +// the whole point: the pre-v0.190.0 sweep sampled once and could not see a late settler. +type windowStacks struct { + // frames is the fleet as seen on each successive GetStacks() call; the last frame repeats. + frames [][]stacks.Stack + calls int + starts map[string]int + onStart func(*windowStacks, string) + // cycle makes the fleet NEVER settle: frames repeat forever instead of the last one sticking. + // Required by the budget test — with frames that eventually stop changing, the window terminates + // by SETTLING even with the budget removed, so the red-proof would not reach the hang it exists + // to demonstrate. + cycle bool +} + +func (w *windowStacks) GetStacks() []stacks.Stack { + i := w.calls + w.calls++ + if i >= len(w.frames) { + if w.cycle { + i = i % len(w.frames) + } else { + i = len(w.frames) - 1 + } + } + return w.frames[i] +} + +func (w *windowStacks) RefreshStatus() error { return nil } + +func (w *windowStacks) StartStack(name string) error { + if w.starts == nil { + w.starts = map[string]int{} + } + w.starts[name]++ + if w.onStart != nil { + w.onStart(w, name) + } + return nil +} + +// shrinkWindow makes the window fast and deterministic, and restores the shipped values after. +func shrinkWindow(t *testing.T, sample time.Duration, stableFor int, budget time.Duration) { + t.Helper() + os, ost, ob, osettle := bootReconcileSample, bootReconcileStableFor, bootReconcileBudget, bootReconcileSettle + t.Cleanup(func() { + bootReconcileSample, bootReconcileStableFor, bootReconcileBudget, bootReconcileSettle = os, ost, ob, osettle + }) + bootReconcileSample, bootReconcileStableFor, bootReconcileBudget = sample, stableFor, budget + bootReconcileSettle = time.Millisecond +} + +// captureSweep replaces the sweep with a recorder and returns the fleet it was handed. +func captureSweep(t *testing.T) *[][]stacks.Stack { + t.Helper() + orig := bootReconcileFn + t.Cleanup(func() { bootReconcileFn = orig }) + var seen [][]stacks.Stack + bootReconcileFn = func(_ context.Context, mgr bootrecon.StackProvider, _ *log.Logger) bootrecon.Result { + seen = append(seen, mgr.GetStacks()) + return bootrecon.Result{} + } + return &seen +} + +func upStack(name string) stacks.Stack { + return stacks.Stack{ + Name: name, Deployed: true, State: stacks.StateRunning, + Containers: []stacks.ContainerInfo{{Name: name, State: stacks.StateRunning}}, + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: stacks.DesiredStateRunning}, + } +} + +// settlingLate is the R-157-A shape: at T+5s the app is still `starting` with its containers coming +// up, and it only comes to rest in a DOWN state later. +func settlingLate(name string) stacks.Stack { + return stacks.Stack{ + Name: name, Deployed: true, State: stacks.StateStarting, + Containers: []stacks.ContainerInfo{{Name: name, State: stacks.StateStarting}}, + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: stacks.DesiredStateRunning}, + } +} + +func settledDown(name string) stacks.Stack { + return stacks.Stack{ + Name: name, Deployed: true, State: stacks.StateExited, + Containers: []stacks.ContainerInfo{{Name: name, State: stacks.StateExited}}, + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: stacks.DesiredStateRunning}, + } +} + +// --- Group A / Scenario B — a late settler IS swept ----------------------------------------------- + +func TestBootWindow_LateSettlerIsSweptOnASettledFleet(t *testing.T) { + // The fleet is still moving for the first frames and settles only later. The sweep must run + // AFTER it settles and must be handed the SETTLED fleet — because the pre-v0.190.0 defect was a + // candidate set derived from a fleet that had not finished moving. + // + // RED-PROOF: restore the single-sweep shape (delete the sampling loop so runBootReconcile calls + // bootReconcileFn straight after the settle delay) and this test fails — the sweep is handed the + // `starting` frame, in which the app is not a down-state candidate at all. + // Demonstrated in REPORT.md §4. + shrinkWindow(t, time.Millisecond, 2, 500*time.Millisecond) + seen := captureSweep(t) + + w := &windowStacks{frames: [][]stacks.Stack{ + {settlingLate("immich")}, // T+5s: still coming up + {settlingLate("immich")}, + {settledDown("immich")}, // settles into a down state only now + {settledDown("immich")}, + {settledDown("immich")}, + }} + + runBootReconcile(context.Background(), w, log.New(io.Discard, "", 0)) + + if len(*seen) != 1 { + t.Fatalf("the sweep ran %d times, want exactly 1 — the window samples, it does not sweep per sample", len(*seen)) + } + got := (*seen)[0] + if len(got) != 1 || got[0].State != stacks.StateExited { + t.Fatalf("the sweep was handed state=%v, want the SETTLED (exited) fleet — a candidate set "+ + "derived from a still-moving fleet is exactly the R-157 mechanism-A defect", got) + } +} + +func TestBootWindow_SweepRunsExactlyOnceEvenOnAQuietBoot(t *testing.T) { + shrinkWindow(t, time.Millisecond, 2, 500*time.Millisecond) + seen := captureSweep(t) + w := &windowStacks{frames: [][]stacks.Stack{{upStack("bookstack")}}} + + runBootReconcile(context.Background(), w, log.New(io.Discard, "", 0)) + + if len(*seen) != 1 { + t.Fatalf("sweeps=%d, want exactly 1 on a quiet boot", len(*seen)) + } +} + +// --- Group B / Scenario C — the window TERMINATES ------------------------------------------------- + +func TestBootWindow_BudgetEndsAForeverChangingFleet(t *testing.T) { + // A fleet that never stops changing must not sample forever. The budget ends it, the sweep runs + // once anyway (a churning box is exactly the box that needs it), and the log SAYS the budget + // ended it — "settled and found nothing" and "ran out of time" are different facts. + // + // RED-PROOF: remove the `time.Since(started) < bootReconcileBudget` loop condition and this test + // hangs — the unbounded-loop shape §5 bans. Demonstrated in REPORT.md §4 (observed as a timeout). + shrinkWindow(t, time.Millisecond, 3, 30*time.Millisecond) + seen := captureSweep(t) + var buf strings.Builder + + // Every frame differs, so `stable` can never reach stableFor. + frames := make([][]stacks.Stack, 0, 200) + for i := 0; i < 200; i++ { + s := upStack("immich") + s.Containers = make([]stacks.ContainerInfo, i%7) // container count changes every sample + frames = append(frames, []stacks.Stack{s}) + } + w := &windowStacks{frames: frames, cycle: true} + + done := make(chan struct{}) + go func() { + runBootReconcile(context.Background(), w, log.New(&buf, "", 0)) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("runBootReconcile did not terminate on a forever-changing fleet — this is the " + + "unbounded restart-loop shape the package's own boundary forbids") + } + + if len(*seen) != 1 { + t.Fatalf("sweeps=%d, want exactly 1 after the budget expired", len(*seen)) + } + if out := buf.String(); !strings.Contains(out, "budget") { + t.Fatalf("the log does not say the BUDGET ended the window, so a churning boot reads like a "+ + "quiet one:\n%s", out) + } +} + +func TestBootWindow_SettledPathSaysSettled(t *testing.T) { + shrinkWindow(t, time.Millisecond, 2, 500*time.Millisecond) + captureSweep(t) + var buf strings.Builder + w := &windowStacks{frames: [][]stacks.Stack{{upStack("docmost")}}} + + runBootReconcile(context.Background(), w, log.New(&buf, "", 0)) + + out := buf.String() + if !strings.Contains(out, "settled") { + t.Fatalf("a settled window must say so — otherwise it is indistinguishable from a budget "+ + "expiry:\n%s", out) + } + if strings.Contains(out, "budget") { + t.Fatalf("a settled window must NOT claim the budget ended it:\n%s", out) + } +} + +func TestBootWindow_CancelledContextStopsImmediately(t *testing.T) { + shrinkWindow(t, time.Millisecond, 3, time.Second) + seen := captureSweep(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + runBootReconcile(ctx, &windowStacks{frames: [][]stacks.Stack{{upStack("x")}}}, log.New(io.Discard, "", 0)) + if len(*seen) != 0 { + t.Fatalf("the sweep ran %d times on a cancelled context, want 0", len(*seen)) + } +} + +// --- Group C / Scenario D — a customer's Stop survives the WIDENED window ------------------------- + +func TestBootWindow_CustomerStoppedAppSurvivesEveryPass(t *testing.T) { + // THE REGRESSION THIS TASK COULD INTRODUCE. A longer window means more chances to resurrect an + // app the customer deliberately stopped. It must survive the whole window — this drives the REAL + // bootrecon sweep (not the captured stub), so the desired-state check is genuinely exercised. + // + // RED-PROOF: drop the DesiredStateStopped branch from isBootOrphan (make it fall through to the + // running case) and this test fails with a start count of 1. Demonstrated in REPORT.md §4. + shrinkWindow(t, time.Millisecond, 2, 200*time.Millisecond) + + stopped := stacks.Stack{ + Name: "nextcloud", Deployed: true, State: stacks.StateStopped, Containers: nil, + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: stacks.DesiredStateStopped}, + } + // The fleet churns around it, so the window runs many passes before settling. + frames := [][]stacks.Stack{ + {stopped, settlingLate("immich")}, + {stopped, settlingLate("immich")}, + {stopped, settledDown("immich")}, + {stopped, upStack("immich")}, + {stopped, upStack("immich")}, + {stopped, upStack("immich")}, + } + w := &windowStacks{frames: frames, onStart: func(w *windowStacks, _ string) {}} + + runBootReconcile(context.Background(), w, log.New(io.Discard, "", 0)) + + if n := w.starts["nextcloud"]; n != 0 { + t.Fatalf("the customer-stopped app was started %d time(s) by the widened window — this is the "+ + "regression a longer window makes possible and it is the worst outcome available here", n) + } +} + +// --- §8.3 — a late recovery is REPORTED, never hidden --------------------------------------------- + +func TestRecordLateRecovery_WarnsWhenTheGraceHasAlreadyExpired(t *testing.T) { + var buf strings.Builder + lg := log.New(&buf, "", 0) + // started far enough back that settle + elapsed exceeds the 90 s grace + recordLateRecovery(lg, time.Now().Add(-(deadAppBootGrace + 10*time.Second)), bootrecon.Result{Recovered: []string{"immich"}}) + out := buf.String() + if !strings.Contains(out, "LATE RECOVERY") || !strings.Contains(out, "immich") { + t.Fatalf("a recovery past the dead-app grace must be reported by name — otherwise a stale "+ + "alarm stands with no counter-evidence (§8.3):\n%s", out) + } +} + +func TestRecordLateRecovery_SilentInsideTheGrace(t *testing.T) { + var buf strings.Builder + recordLateRecovery(log.New(&buf, "", 0), time.Now(), bootrecon.Result{Recovered: []string{"immich"}}) + if buf.Len() != 0 { + t.Fatalf("a recovery INSIDE the grace must stay silent — that is what makes a successful "+ + "recovery invisible to the customer:\n%s", buf.String()) + } +} + +func TestRecordLateRecovery_SilentWhenNothingRecovered(t *testing.T) { + var buf strings.Builder + recordLateRecovery(log.New(&buf, "", 0), time.Now().Add(-time.Hour), bootrecon.Result{}) + if buf.Len() != 0 { + t.Fatalf("nothing was recovered, so there is nothing late to report:\n%s", buf.String()) + } +} + +// --- The window's constants must fit the grace they are justified against ------------------------- + +func TestBootWindow_CommonCaseFitsInsideTheDeadAppGrace(t *testing.T) { + // The comment on the window constants justifies them against deadAppBootGrace. A comment + // asserting an invariant needs a test pinning it, or it is a wish. + common := bootReconcileSettle + bootReconcileBudget + bootrecon.DefaultRetryDelay + if common > deadAppBootGrace { + t.Fatalf("settle(%s) + budget(%s) + one retry(%s) = %s exceeds the %s dead-app grace — the "+ + "COMMON case must stay silent, or every slow boot alerts", + bootReconcileSettle, bootReconcileBudget, bootrecon.DefaultRetryDelay, common, deadAppBootGrace) + } + if bootReconcileSample <= 0 || bootReconcileStableFor < 2 { + t.Fatalf("sample=%s stableFor=%d — one sample cannot distinguish 'settled' from 'sampled "+ + "between two docker events'", bootReconcileSample, bootReconcileStableFor) + } +} diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index e1431d6..9448dd4 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -13,6 +13,7 @@ import ( "os/exec" "os/signal" "path/filepath" + "sort" "syscall" "time" @@ -258,6 +259,12 @@ func main() { // sweep, deliberately AFTER the quiesce recovery above so the two never race for the same stack, // and entirely inside deadAppBootGrace so a successful recovery is silent and a failed one still // alerts honestly. Never touches an app the customer stopped — see internal/bootrecon. + // R-171: hand the boot sweep the settings it needs to answer "is this app's drive live?" BEFORE + // the goroutine starts — an unwired gate is silently the pre-v0.190.0 behaviour that started apps + // onto absent drives. TestMainWiresBootDriveGate walks this file's AST for the assignment. + bootDriveSettings = sett + bootQuiesceLoop = quiesceLoop + bootAppStopGuard = appStopGuard go runBootReconcile(ctx, stackMgr, logger) // --- Start CPU collector --- @@ -1258,27 +1265,247 @@ func noteDeadAppScan(logger *log.Logger, scans, evaluated, down int) { scans, evaluated, down) } -// bootReconcileSettle lets the initial scan, the first status refresh and the quiesce recovery -// settle before the R-52 sweep decides what "down" means. 5 s + at most one 30 s retry gap keeps -// the whole sweep inside deadAppBootGrace (90 s), which is what makes a successful recovery silent. +// bootReconcileSettle is the delay before the FIRST observation. It lets the initial scan, the first +// status refresh and the two crash recoveries land before the R-52 sweep decides what "down" means. var bootReconcileSettle = 5 * time.Second +// ── R-157 mechanism A: the window, and why these three numbers ─────────────────────────────────── +// +// Until v0.190.0 the sweep looked exactly ONCE, at T+5 s, and returned. At T+5 s docker is still +// restoring containers after a hard reset, so an app that has not yet settled into a down state is +// not a candidate — and because the sweep never looked again, it stayed down. Measured failing on +// THREE OF SIX hard resets (CAMPAIGN-10). The predicate was never the problem; the single +// observation was. +// +// The fix is a window that SETTLES rather than a timer that runs forever (§5: an unbounded loop +// papers over a genuinely broken app and hammers docker). Three constants, each chosen against the +// 90 s dead-app boot grace: +// +// - bootReconcileSample = 5 s. Fine enough that a container settling at T+40 s is seen within one +// sample, coarse enough that a quiet boot costs ~12 cheap GetStacks() calls, not hundreds. +// - bootReconcileStableFor = 3 consecutive identical samples (15 s of no change) before the fleet +// is called settled. One sample cannot distinguish "settled" from "sampled between two docker +// events"; three spans the gap between a container exiting and its restart policy re-creating it. +// - bootReconcileBudget = 50 s. THE BINDING CONSTRAINT, and it is arithmetic, not taste: +// bootReconcileSettle (5 s) + budget (50 s) + ONE DefaultRetryDelay (30 s) inside the final +// sweep = 85 s, which must stay under deadAppBootGrace (90 s) so a recovery that works is +// SILENT. 60 s was the first choice and TestBootWindow_CommonCaseFitsInsideTheDeadAppGrace +// rejected it at 95 s — the test is the reason this number is 50 and not a round 60. +// Extending the grace to make a bigger budget fit was rejected (§8.3): that hides a late +// recovery rather than reporting it. A window that genuinely overruns is reported instead — +// see recordLateRecovery below. +// +// The window ends on WHICHEVER COMES FIRST — settled, or budget exhausted — and the log says which, +// because "settled and found nothing" and "ran out of time still churning" are different facts about +// the box and must not read the same (the v0.91.2 lesson). +var ( + bootReconcileSample = 5 * time.Second + bootReconcileStableFor = 3 + bootReconcileBudget = 50 * time.Second +) + // bootReconcileFn is the R-52 sweep, a package var purely so the wiring below is testable from // package main (the v0.154.0 / v0.91.0 lesson: a seam proven only through injection proves the // component and not the caller). var bootReconcileFn = func(ctx context.Context, mgr bootrecon.StackProvider, logger *log.Logger) bootrecon.Result { - return bootrecon.New(mgr, logger).Run(ctx) + r := bootrecon.New(mgr, logger) + // R-171: the sweep must not start an app whose data drive is absent. Wired HERE, at the one + // place the sweep is constructed, so there is no path that builds an ungated reconciler. + if sm, ok := mgr.(*stacks.Manager); ok { + r.SetDriveGate(bootDriveGate{mgr: sm, sett: bootDriveSettings}) + } + return r.Run(ctx) } -// runBootReconcile waits out the settle window, then performs exactly one bounded recovery sweep. -// Called from main() in a goroutine; returns after the single sweep — there is no loop by design. +// bootDriveSettings / bootStartHolders are what the boot start gate reads. Init-only, set in main() +// before the reconcile goroutine is launched; both are nil-safe (see MayStart). +var ( + bootDriveSettings *settings.Settings + bootQuiesceLoop *quiesce.Loop + bootAppStopGuard *backup.AppStopGuard +) + +// bootDriveGate answers bootrecon.StartGate for the real controller. It enforces §8.2: an app that +// something else is deliberately holding must NOT be started by the boot sweep. +// +// THE THREE HOLDERS, in the order they are checked. The first two only became reachable when R-157 +// mechanism A widened the window — the old T+5 s single sweep never overlapped a quiesce or a +// running app-data operation, and that is exactly why widening it needed these: +// +// 1. QUIESCE — the whole-guest backup loop stops app stacks and restarts exactly the ones it +// stopped. Starting one mid-backup would put a running app inside a snapshot that is supposed to +// be clean-shutdown-consistent, which is the entire point of quiescing. `SuppressedStacks` is +// the set it already publishes for precisely this "an app WE stopped is not a fault" question, +// so reusing it means the two cannot drift. +// 2. THE APP-STOP GUARD — a volume dump / offbox reconstitute / .fab export that is CURRENTLY +// holding an app down. Its own Recover already ran to completion before this goroutine started, +// so the marker seen here belongs to an operation running NOW, not to a crashed one. +// 3. THE DRIVE — R-171. Two questions, because they fail in opposite directions and neither alone +// is sufficient at boot: +// • the settings flags (`Disconnected`/`Decommissioned`) — the SAME signal the API's own +// `startGatedByMissingDrive` uses, so the customer's path and the sweep cannot disagree about +// whether an app may start. But they are the drive gate's bookkeeping, and inside the boot +// window that gate may not have ticked yet, so a genuinely absent drive can still read as +// connected. +// • `Manager.DriveLive` — the live mountpoint check, the SAME `isMountPoint` seam the userdata +// belt already uses. It answers immediately and needs no gate tick. +// +// Fail-safe per bootrecon.StartGate's contract: anything that cannot be determined returns false. +type bootDriveGate struct { + mgr *stacks.Manager + sett *settings.Settings +} + +func (g bootDriveGate) MayStart(stackName string) (bool, string) { + // 1. quiesce (nil-safe on an unprovisioned guest: SuppressedStacks returns nil) + if bootQuiesceLoop.SuppressedStacks()[stackName] { + return false, "a whole-guest backup (quiesce) is holding it — the quiesce loop restarts its own stacks" + } + // 2. an app-data operation in flight + for _, held := range bootAppStopGuard.HeldStacks() { + if held == stackName { + return false, "an app-data operation is holding it — the app-stop guard restarts it when the operation ends" + } + } + // 3. the drive + cfg := g.mgr.LoadAppConfigByName(stackName) + if cfg == nil { + // CANNOT DETERMINE. A deployed app whose app.yaml will not load cannot have its drive + // resolved, so the fail-safe direction applies rather than a hopeful start. + return false, "app.yaml could not be read" + } + hdd := cfg.Env["HDD_PATH"] + if hdd == "" { + return true, "" // SSD-resident: there is no external drive to be absent (mirrors the API gate) + } + if g.sett != nil { + for _, sp := range g.sett.GetStoragePaths() { + if sp.Path != hdd { + continue + } + if sp.Decommissioned { + return false, "drive " + hdd + " is decommissioned" + } + if sp.Disconnected { + return false, "drive " + hdd + " is flagged disconnected" + } + } + } + if !g.mgr.DriveLive(hdd) { + return false, "drive " + hdd + " is not a live mountpoint" + } + return true, "" +} + +// bootFleetSample is a comparable snapshot of one deployed app — name, state and container count, +// per §8.1. Container count is in it deliberately: a stack can go from 3 containers to 0 without its +// aggregate state changing, and that IS the boot still moving. +type bootFleetSample struct { + name string + state string + containers int +} + +// sampleBootFleet returns the fleet snapshot, sorted, so two samples compare by equality. +func sampleBootFleet(mgr bootrecon.StackProvider) []bootFleetSample { + stacksNow := mgr.GetStacks() + out := make([]bootFleetSample, 0, len(stacksNow)) + for _, s := range stacksNow { + if !s.Deployed { + continue + } + out = append(out, bootFleetSample{name: s.Name, state: string(s.State), containers: len(s.Containers)}) + } + sort.Slice(out, func(i, j int) bool { return out[i].name < out[j].name }) + return out +} + +func sameBootFleet(a, b []bootFleetSample) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// runBootReconcile waits out the settle delay, then samples the fleet until it stops changing (or +// the budget runs out) and performs the bounded sweep ONCE, at the end. +// +// Sweeping on every sample was rejected: the sweep's own StartStack changes the fleet, so a +// sweep-per-sample would never observe a settled fleet and would race docker's restore. Sampling is +// read-only; exactly one sweep runs, and it re-derives its candidate set from a settled fleet — +// which is the whole point, since the pre-v0.190.0 bug was a candidate set derived too early. +// +// Called from main() in a goroutine. func runBootReconcile(ctx context.Context, mgr bootrecon.StackProvider, logger *log.Logger) { select { case <-ctx.Done(): return case <-time.After(bootReconcileSettle): } - bootReconcileFn(ctx, mgr, logger) + + started := time.Now() + prev := sampleBootFleet(mgr) + stable := 1 + settled := false + + for time.Since(started) < bootReconcileBudget { + select { + case <-ctx.Done(): + return + case <-time.After(bootReconcileSample): + } + cur := sampleBootFleet(mgr) + if sameBootFleet(prev, cur) { + stable++ + } else { + // Not settled — the boot is still moving. Log at DEBUG: at 5 s cadence an INFO line per + // sample would bury the one line that matters, which is the verdict below. + logger.Printf("[DEBUG] [bootrecon] boot window: fleet still changing (%d app(s)) — resampling", len(cur)) + stable = 1 + } + prev = cur + if stable >= bootReconcileStableFor { + settled = true + break + } + } + + if settled { + logger.Printf("[INFO] [bootrecon] boot window: fleet settled after %.0fs (%d identical samples %s apart) — sweeping", + time.Since(started).Seconds(), bootReconcileStableFor, bootReconcileSample) + } else { + // NOT a failure — a box whose apps are still churning at the budget is exactly the box that + // most needs the sweep. But it is a different fact from "settled", and saying so is what makes + // a stuck boot visible instead of looking like a quiet one. + logger.Printf("[INFO] [bootrecon] boot window: budget %s exhausted while the fleet was still changing — sweeping anyway", + bootReconcileBudget) + } + + res := bootReconcileFn(ctx, mgr, logger) + recordLateRecovery(logger, started, res) +} + +// recordLateRecovery keeps §8.3 honest. The window can finish AFTER deadAppBootGrace, and when it +// does the dead-app alarm has already fired for an app this sweep then recovered. Extending the +// grace to hide that was rejected; reporting it is the alternative, so the record is truthful and +// the operator is not left with a stale alarm and no counter-evidence. +// +// Measured from controller start, which is what the grace is measured from. +func recordLateRecovery(logger *log.Logger, started time.Time, res bootrecon.Result) { + if len(res.Recovered) == 0 { + return + } + elapsed := bootReconcileSettle + time.Since(started) + if elapsed <= deadAppBootGrace { + return + } + logger.Printf("[WARN] [bootrecon] LATE RECOVERY: %d app(s) recovered %.0fs after start, past the %s dead-app grace — an alert may already have fired for: %v", + len(res.Recovered), elapsed.Seconds(), deadAppBootGrace, res.Recovered) } // scanDeployedAppRunStates returns the fix-3 view of the deployed apps: the DEAD ones (for the diff --git a/controller/internal/backup/appstop_marker.go b/controller/internal/backup/appstop_marker.go index 5d9a043..afd38d1 100644 --- a/controller/internal/backup/appstop_marker.go +++ b/controller/internal/backup/appstop_marker.go @@ -229,6 +229,24 @@ func (g *AppStopGuard) Recover() *AppStopRecovery { return res } +// HeldStacks returns the stacks an app-data operation is CURRENTLY holding down, or nil. +// +// Read-only and nil-safe. It exists for the boot reconciler (§8.2): once R-157 mechanism A widened +// the boot window, the sweep could overlap a running volume dump or export and "recover" an app that +// is deliberately stopped mid-operation — restarting it under a tar, which is the inconsistency the +// stop was taken to avoid. Recover() has already run to completion by then, so a marker seen through +// this method belongs to an operation running NOW, not to a crashed one. +func (g *AppStopGuard) HeldStacks() []string { + if g == nil || g.path == "" { + return nil + } + m, ok := g.read() + if !ok || !m.Active { + return nil + } + return append([]string(nil), m.Stacks...) +} + // ---- marker persistence (atomic, 0600) — the quiesce shape ------------------------------------ func (g *AppStopGuard) write(m AppStopMarker) error { diff --git a/controller/internal/bootrecon/bootrecon.go b/controller/internal/bootrecon/bootrecon.go index 2616e39..4cb6030 100644 --- a/controller/internal/bootrecon/bootrecon.go +++ b/controller/internal/bootrecon/bootrecon.go @@ -41,6 +41,45 @@ type StackProvider interface { RefreshStatus() error } +// StartGate answers the one question this package must ask before starting anything: **may this app +// be started right now?** Declared consumer-side, in the style of StackProvider, so `bootrecon` +// still imports `stacks` alone and knows nothing about settings, the agent, quiesce or the web layer. +// +// It is ONE seam rather than three because the three reasons a boot orphan must NOT be started share +// a shape — something else is deliberately holding this app — and differ only in the reason string: +// +// the drive-absent gate stopped it → its drive is not live (R-171, below) +// a quiesce is holding it for a backup → the quiesce loop restarts its own stacks +// an app-data operation stopped it → the app-stop guard's own Recover owns it +// +// A widened boot window (R-157 mechanism A) is what makes the last two reachable at all: the old +// T+5 s single sweep never overlapped them. +// +// R-171 — WHY THIS EXISTS, and it is a regression this package caused. Until v0.189.0 the sweep +// required a stack to still HAVE containers, and an app the drive-absent gate had stopped has zero, +// so such apps were skipped by accident. v0.189.0 replaced that term with the customer's recorded +// intent — correctly — and the drive gate does NOT change `desired_state` (it is not the customer), +// so a gate-stopped app now reads as `running` + zero containers, i.e. a boot orphan. Observed live +// on 2026-08-02: the sweep found and started an app whose drive was unmounted, burned both attempts, +// and handed it to the dead-app alarm — a false alarm about an app the drive gate is deliberately +// holding (audits/DIAG-bootrecon-drive-absent-2026-08-02.md). +// +// The rule itself is not new and is not invented here: the API's own start path already refuses this +// (`startGatedByMissingDrive`, internal/api/router.go) with a Hungarian message to the customer. The +// sweep simply bypassed it by calling Manager.StartStack directly. This seam gives the sweep the +// same question to ask. +// +// CONTRACT — the answer is fail-safe by design (§8.4): an implementation that CANNOT DETERMINE +// whether the drive is live must return false, not true. Not starting is recoverable — the drive +// gate's `Return` branch restarts the app when the drive comes back, and the dead-app alarm reports +// it meanwhile. Starting on an absent drive is not recoverable by anything automatic: compose +// creates the bind sources wherever the mountpoint currently points, which is the guest rootfs. +type StartGate interface { + // MayStart reports whether the named stack may be started. The reason is for the log line and is + // only read when may is false. + MayStart(stackName string) (may bool, reason string) +} + const ( // DefaultAttempts is the total number of start attempts per boot (not per app per retry-forever). DefaultAttempts = 2 @@ -58,6 +97,13 @@ type Reconciler struct { // sleep is the inter-attempt wait; injectable so tests never spend 30 real seconds. sleep func(context.Context, time.Duration) + + // startGate (R-171) refuses to start an app something else is deliberately holding. nil = NOT + // WIRED, which means "this caller has no such concept" and is permissive — the test fixtures' + // case. It is NOT the same as "cannot determine", which the gate itself answers with false (see + // StartGate's contract). Production MUST wire it; TestMainWiresBootDriveGate walks main.go's AST + // for the call, because an unwired seam here is silently the pre-v0.190.0 behaviour. + startGate StartGate } // Result is the outcome, returned for logging/testing (the hub learns about failures only through @@ -67,6 +113,23 @@ type Result struct { Recovered []string // running again by the end StillDown []string // still down after the last attempt — the alarm's problem now Attempts int // attempts actually made (0 when there was nothing to do) + // HeldByDrive (R-171) are apps that ARE boot orphans by intent but which something else is + // deliberately holding (an absent drive, a quiesce, an app-data operation), so they were not + // started. Reported separately from StillDown because they are not a fault this sweep failed to + // fix — the holder owns their recovery. Collapsing the two would put a deliberately-held app in + // the same bucket as a broken one, which is the false alarm R-171 removes. + HeldByDrive []string +} + +// SetDriveGate wires the R-171 start refusal. INIT-ONLY — call once, before Run. +func (r *Reconciler) SetDriveGate(g StartGate) { r.startGate = g } + +// mayStart asks the gate, or allows when none is wired (see the startGate field comment). +func (r *Reconciler) mayStart(stackName string) (bool, string) { + if r.startGate == nil { + return true, "" + } + return r.startGate.MayStart(stackName) } // New builds a Reconciler with the shipped defaults. @@ -109,9 +172,9 @@ func sleepCtx(ctx context.Context, d time.Duration) { // customer stopped this" and left alone. The safety goal was right and still holds. The SIGNAL was // wrong, because zero containers has at least three causes and the count cannot tell them apart: // -// a deliberate Stop → must stay down -// a power cut mid-compose, or an interrupted deploy → must come back -// a backup that stopped the app and died before restarting it → must come back +// a deliberate Stop → must stay down +// a power cut mid-compose, or an interrupted deploy → must come back +// a backup that stopped the app and died before restarting it → must come back // // Two of those three were silently unrecoverable: the app simply stayed gone until a human noticed. // The count was never capable of separating them, so the fix is not a better inference — it is to @@ -162,16 +225,33 @@ func (r *Reconciler) Run(ctx context.Context) Result { pending := map[string]bool{} for _, s := range r.stacks.GetStacks() { - if isBootOrphan(s) { - pending[s.Name] = true - res.Candidates = append(res.Candidates, s.Name) + if !isBootOrphan(s) { + continue } + // R-171: intent says this app should be running and it is not — but if something else is + // deliberately holding it (absent drive, quiesce, an app-data operation), starting it is the + // wrong repair. Refuse, loudly, and let the holder own it. + if live, reason := r.mayStart(s.Name); !live { + res.HeldByDrive = append(res.HeldByDrive, s.Name) + r.logger.Printf("[INFO] [bootrecon] %q is a boot orphan by intent but is HELD (%s) — NOT starting it; whatever is holding it owns its recovery", + s.Name, reason) + continue + } + pending[s.Name] = true + res.Candidates = append(res.Candidates, s.Name) } sortStrings(res.Candidates) + sortStrings(res.HeldByDrive) if len(pending) == 0 { // The healthy path must be observable — "no alarms" and "never ran" have to be - // distinguishable in a log (the v0.91.2 lesson). + // distinguishable in a log (the v0.91.2 lesson). "Nothing to start" and "everything I found + // is held by an absent drive" must be distinguishable too, or the held case reads as healthy. + if len(res.HeldByDrive) > 0 { + r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: nothing to start — %d app(s) held (absent drive / quiesce / app-data operation): %v", + len(res.HeldByDrive), res.HeldByDrive) + return res + } r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: no boot-orphaned apps (nothing to start)") return res } diff --git a/controller/internal/bootrecon/drivegate_test.go b/controller/internal/bootrecon/drivegate_test.go new file mode 100644 index 0000000..c303c49 --- /dev/null +++ b/controller/internal/bootrecon/drivegate_test.go @@ -0,0 +1,217 @@ +package bootrecon + +import ( + "context" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-171 — the sweep must not start an app whose data drive is absent. +// +// This is a REGRESSION TEST for a defect this package caused: v0.189.0 replaced the container-count +// term with recorded intent, and a drive-gate-stopped app reads as `running` + zero containers, i.e. +// a boot orphan. Confirmed live on 2026-08-02 (the sweep started it, burned both attempts, and +// handed it to the dead-app alarm). + +// fakeDriveGate answers a scripted liveness verdict per app. +type fakeDriveGate struct { + dead map[string]string // app → reason it is not live + asked []string + unsure map[string]bool // app → the gate cannot determine (must be treated as NOT live) +} + +func (g *fakeDriveGate) MayStart(name string) (bool, string) { + g.asked = append(g.asked, name) + if r, ok := g.dead[name]; ok { + return false, r + } + if g.unsure[name] { + return false, "cannot determine" + } + return true, "" +} + +// --- Group G — Scenario A / Part 0's finding, as a test ------------------------------------------ + +func TestReconcile_DriveAbsentApp_IsNeverStarted(t *testing.T) { + // The exact live shape: the drive gate stopped it (zero containers), it is still recorded + // `running` because the gate is not the customer, and its drive is gone. + // + // RED-PROOF: delete the `if live, reason := r.driveLive(...)` block from Run and this test fails + // with a start count of 1 — which is precisely what was observed on the box before the fix. + // Demonstrated in REPORT.md §4. + app := withDesired(vanished("calibre-web"), stacks.DesiredStateRunning) + f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp} + r, _ := newTestReconciler(f) + gate := &fakeDriveGate{dead: map[string]string{"calibre-web": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"}} + r.SetDriveGate(gate) + + res := r.Run(context.Background()) + + if n := f.starts["calibre-web"]; n != 0 { + t.Fatalf("an app whose data drive is ABSENT was started %d time(s) — compose would create its "+ + "bind sources on the guest rootfs, which is the hazard the drive gate exists to prevent", n) + } + if len(res.Candidates) != 0 { + t.Fatalf("a drive-held app was listed as a start candidate: %v", res.Candidates) + } + if len(res.HeldByDrive) != 1 || res.HeldByDrive[0] != "calibre-web" { + t.Fatalf("HeldByDrive = %v, want [calibre-web] — a held app must be reported, not silently dropped", res.HeldByDrive) + } + if len(res.StillDown) != 0 { + t.Fatalf("a deliberately-held app was reported as StillDown %v — that is the dead-app alarm's "+ + "bucket, and putting it there is the false alarm this fix removes", res.StillDown) + } + if res.Attempts != 0 { + t.Fatalf("attempts=%d, want 0 — nothing should have been attempted", res.Attempts) + } +} + +func TestReconcile_UndeterminableDrive_IsNotStarted(t *testing.T) { + // §8.4's fail-safe direction. "Cannot determine" must behave exactly like "absent": not starting + // is recoverable (the drive gate's Return branch owns it); starting on an absent drive is not. + app := withDesired(vanished("immich"), stacks.DesiredStateRunning) + f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp} + r, _ := newTestReconciler(f) + r.SetDriveGate(&fakeDriveGate{unsure: map[string]bool{"immich": true}}) + + res := r.Run(context.Background()) + + if len(f.starts) != 0 { + t.Fatalf("an app whose drive liveness could NOT be determined was started: %v — the fail-safe "+ + "direction is to refuse", f.starts) + } + if len(res.HeldByDrive) != 1 { + t.Fatalf("HeldByDrive = %v, want the undeterminable app held", res.HeldByDrive) + } +} + +func TestReconcile_LiveDriveApp_IsStillRecovered(t *testing.T) { + // The gate must not become a blanket refusal — an app on a LIVE drive is still the R-157 case + // and must still be recovered. Without this, a "fix" that returns false always would pass the + // test above and silently disable the whole feature. + app := withDesired(vanished("bookstack"), stacks.DesiredStateRunning) + f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp} + r, _ := newTestReconciler(f) + gate := &fakeDriveGate{} + r.SetDriveGate(gate) + + res := r.Run(context.Background()) + + if f.starts["bookstack"] == 0 { + t.Fatal("an app on a LIVE drive was not recovered — the drive gate must refuse absent drives, not all of them") + } + if len(res.HeldByDrive) != 0 { + t.Fatalf("an app on a live drive was reported held: %v", res.HeldByDrive) + } + if len(gate.asked) != 1 || gate.asked[0] != "bookstack" { + t.Fatalf("the gate was asked %v, want exactly [bookstack] — one question per candidate", gate.asked) + } +} + +func TestReconcile_DriveGateIsOnlyAskedAboutOrphans(t *testing.T) { + // A running app and a customer-stopped app are not candidates, so the gate must never be asked + // about them. Asking is not merely wasteful: the production gate reads app.yaml off disk per + // call, and a stopped app's drive being absent is not a fault anyone should hear about. + running := withDesired(stacks.Stack{ + Name: "docmost", Deployed: true, State: stacks.StateRunning, + Containers: []stacks.ContainerInfo{{Name: "docmost", State: stacks.StateRunning}}, + }, stacks.DesiredStateRunning) + stopped := withDesired(vanished("nextcloud"), stacks.DesiredStateStopped) + orphan := withDesired(vanished("immich"), stacks.DesiredStateRunning) + + f := &fakeStacks{list: []stacks.Stack{running, stopped, orphan}, onStart: comesUp} + r, _ := newTestReconciler(f) + gate := &fakeDriveGate{} + r.SetDriveGate(gate) + r.Run(context.Background()) + + if len(gate.asked) != 1 || gate.asked[0] != "immich" { + t.Fatalf("the drive gate was asked about %v, want exactly [immich] — only boot orphans", gate.asked) + } +} + +func TestReconcile_NoDriveGateWired_IsPermissive(t *testing.T) { + // nil gate = "this caller has no drive concept" (the fixtures' case), NOT "cannot determine". + // Production wiring is pinned separately by TestMainWiresBootDriveGate — an unwired gate here + // would silently be the pre-v0.190.0 behaviour, which is why that AST test exists. + app := withDesired(vanished("immich"), stacks.DesiredStateRunning) + f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp} + r, _ := newTestReconciler(f) + + r.Run(context.Background()) + + if f.starts["immich"] == 0 { + t.Fatal("with no drive gate wired the sweep must behave as before — the nil case is permissive") + } +} + +// --- Scenario F — the two boot gates agree ------------------------------------------------------- + +func TestBothBootGatesAgreeOnIntent(t *testing.T) { + // R-170 + R-166: isBootOrphan and shouldRecreateOnBoot answer the SAME question — did the + // customer want this running? — and until v0.190.0 they answered it with different signals. + // + // shouldRecreateOnBoot lives in internal/web and cannot be called from here without an import + // cycle, so this test pins THIS side of the agreement and its sibling + // TestShouldRecreateOnBoot_AgreesWithBootrecon (internal/web) pins the other, against the same + // fixture table. Both must be updated together if the table changes. + cases := []struct { + desired string + containers int + wantWanted bool // "the customer wanted this running" + }{ + {stacks.DesiredStateStopped, 0, false}, + {stacks.DesiredStateStopped, 2, false}, + {stacks.DesiredStateRunning, 0, true}, + {stacks.DesiredStateRunning, 2, true}, + {stacks.DesiredStateUnknown, 0, false}, // legacy: zero containers ⇒ treated as stopped + {stacks.DesiredStateUnknown, 2, true}, // legacy: containers present ⇒ treated as wanted + } + for _, c := range cases { + s := stacks.Stack{ + Name: "app", Deployed: true, State: stacks.StateExited, + Containers: make([]stacks.ContainerInfo, c.containers), + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: c.desired}, + } + if got := isBootOrphan(s); got != c.wantWanted { + t.Fatalf("isBootOrphan(desired=%q containers=%d) = %v, want %v — the two boot gates must "+ + "answer the intent question identically", c.desired, c.containers, got, c.wantWanted) + } + } +} + +// --- Group F / §8.2 — every holder the widened window can now overlap ---------------------------- + +func TestReconcile_HeldByAnyHolder_IsNeverStarted(t *testing.T) { + // §8.2's table, one case per row that the gate is responsible for. The reasons differ; the + // required behaviour is identical, which is why they share one seam. + // + // The first two rows only became reachable when R-157 mechanism A widened the boot window — the + // old T+5 s single sweep never overlapped a quiesce or a running app-data operation. Widening the + // window without these would have traded a fixed bug for two new ones. + for _, reason := range []string{ + "a whole-guest backup (quiesce) is holding it — the quiesce loop restarts its own stacks", + "an app-data operation is holding it — the app-stop guard restarts it when the operation ends", + "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint", + } { + app := withDesired(vanished("immich"), stacks.DesiredStateRunning) + f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp} + r, _ := newTestReconciler(f) + r.SetDriveGate(&fakeDriveGate{dead: map[string]string{"immich": reason}}) + + res := r.Run(context.Background()) + + if len(f.starts) != 0 { + t.Fatalf("held by %q but started anyway: %v", reason, f.starts) + } + if len(res.HeldByDrive) != 1 { + t.Fatalf("held by %q but not reported as held: %+v", reason, res) + } + if len(res.StillDown) != 0 { + t.Fatalf("held by %q and reported as StillDown %v — that is the dead-app alarm's bucket", + reason, res.StillDown) + } + } +} diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index 80ea086..ea23b71 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -724,6 +724,23 @@ func (m *Manager) UpdateOptionalConfig(stackName string, values map[string]strin return m.RefreshStatus() } +// DriveLive reports whether an app's data drive is a live mountpoint right now. +// +// It is the SAME signal the userdata belt uses (manager.go, the `isMountPoint` seam) rather than a +// second implementation, so the two can never disagree about whether a drive is there — a drift that +// would be invisible until one of them acted on it. The system/local path is legitimately not a +// mountpoint and is never gated, exactly as the belt treats it. +// +// R-171: exported because the boot reconciler must ask this question and lives in another package. +// Before v0.190.0 nothing asked it on that path, so the sweep started apps whose drive was absent — +// observed live on 2026-08-02 (audits/DIAG-bootrecon-drive-absent-2026-08-02.md). +func (m *Manager) DriveLive(hddPath string) bool { + if hddPath == "" || hddPath == m.sysDataPath { + return true // SSD-resident: no external drive to be absent + } + return m.isMountPoint(hddPath) +} + // LoadAppConfigByName reads app.yaml for a named stack. Returns nil if not found. func (m *Manager) LoadAppConfigByName(stackName string) *AppConfig { stack, ok := m.GetStack(stackName) diff --git a/controller/internal/web/intermediary.go b/controller/internal/web/intermediary.go index f62fb10..a64a7e2 100644 --- a/controller/internal/web/intermediary.go +++ b/controller/internal/web/intermediary.go @@ -11,6 +11,7 @@ import ( "gitea.dooplex.hu/admin/felhom-controller/internal/agentapi" "gitea.dooplex.hu/admin/felhom-controller/internal/settings" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" ) // bootBindWait / bootBindPoll bound the readiness gate in processGuestBootChange. On a guest reboot @@ -106,31 +107,54 @@ func agentWhere(registeredPath string) string { // 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: +// ── R-170: intent replaced the container count (v0.190.0) ──────────────────────────────────────── // -// - 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. +// The question this gate has to answer is "did the customer want this app running?", and until +// v0.190.0 it answered by counting containers, exactly as bootrecon.isBootOrphan did: // -// `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. +// ZERO containers → a UI Stop is `compose down`, which REMOVES them → the customer stopped it. +// +// R-55 added that term for a real defect and it was the right fix FOR THE SIGNAL AVAILABLE THEN: +// before it, the gate silently undid a customer's Stop on every guest reboot — including apps +// stopped deliberately to free resources for others, which is precisely when resurrecting them is +// most harmful. `deployed` could not answer it: it is a deploy-lifecycle flag and stays true across +// a Stop. +// +// But the count has at least three causes and cannot separate them — a deliberate Stop, a power cut +// mid-compose, an interrupted backup — so R-166 replaced it in `bootrecon` with the customer's +// RECORDED intent (`desired_state` in app.yaml). This gate was left on the old signal for one +// release, which left the two boot gates disagreeing about the same question. They now agree: +// +// stopped → NEVER recreate. The customer said so; no observation overrides it. +// running → recreate, whatever the container count. This is the case the count could not see. +// absent → fall back to `hasContainers`, i.e. EXACTLY the pre-v0.190.0 behaviour. +// +// The absent branch is not a leftover. Every app.yaml written before v0.189.0 lacks the field, so +// absent is what an upgraded box reads for every app nobody has pressed a button on since; treating +// it as `running` would recreate — and therefore start — apps their owners had deliberately stopped, +// fleet-wide, on the first reboot after the upgrade. // // The evidence is read from the snapshot taken BEFORE any recreate runs, because `recreate` itself -// calls StopStack (`compose down`) and so destroys it. +// calls StopStack (`compose down`) and so destroys it (R-55). // -// 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 +// NOTE on the drive-absent gate: `presentStable[hdd]` is still required and is still load-bearing. +// An app whose drive is absent is never recreated here no matter what its intent says — it is +// recorded in StoragePath.StoppedStacks and restarted by ReconcileDriveGates' `Return` branch, which +// runs on the same loop tick. That term is what keeps this gate safe; it is the term the BOOT SWEEP +// was missing until R-171 (audits/DIAG-bootrecon-drive-absent-2026-08-02.md), and it must not be +// dropped in sympathy with the count. +func shouldRecreateOnBoot(deployed bool, hdd string, presentStable map[string]bool, hasContainers bool, desired string) bool { + if !deployed || hdd == "" || !strings.HasPrefix(hdd, StableParentDir+"/") || !presentStable[hdd] { + return false + } + switch desired { + case stacks.DesiredStateStopped: + return false + case stacks.DesiredStateRunning: + return true + default: + return hasContainers // absent/legacy — byte-identical to the pre-v0.190.0 rule + } } // defaultPromotionTarget decides M1 (never leave zero default). If the path being decommissioned is NOT @@ -423,6 +447,9 @@ func (s *Server) processGuestBootChange() { bootStacks = append(bootStacks, bootStack{ name: st.Name, deployed: cfg.Deployed, hdd: cfg.Env["HDD_PATH"], state: string(st.State), hasContainers: len(st.Containers) > 0, + // R-170: read intent from the app.yaml just loaded, not from st.AppConfig — cfg is the + // fresh on-disk read this loop already performs, so the two cannot disagree. + desired: cfg.DesiredState, }) } recreate := func(bs bootStack) { @@ -443,7 +470,7 @@ func (s *Server) processGuestBootChange() { 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) + s.logger.Printf("[INFO] [gate] boot %s: %d drive-backed app(s) left stopped on purpose (recorded Stop, or a legacy app.yaml at zero containers)", resp.GuestBootID, leftStopped) } if serr := s.settings.SetLastGuestBootID(resp.GuestBootID); serr != nil { s.logger.Printf("[WARN] [gate] persist boot-id: %v", serr) @@ -458,8 +485,13 @@ type bootStack struct { 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. + // before any recreate runs, since recreate's StopStack erases it. Since R-170 it is only consulted + // for apps with NO recorded intent (legacy app.yaml), but it is still sampled for all of them — + // the legacy branch needs it and the snapshot has exactly one chance to take it. hasContainers bool + // desired is the customer's recorded intent (stacks.DesiredState*), "" when the app.yaml predates + // v0.189.0. R-170: this is what the gate decides on now, with hasContainers as the legacy fallback. + desired string } // recreateDriveBackedApps recreates every deployed drive-backed app whose drive bind is live, then @@ -474,11 +506,14 @@ type bootStack struct { // 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, bs.hasContainers) { + if !shouldRecreateOnBoot(bs.deployed, bs.hdd, presentStable, bs.hasContainers, bs.desired) { if bs.deployed && strings.HasPrefix(bs.hdd, StableParentDir+"/") { switch { - case presentStable[bs.hdd] && !bs.hasContainers: - leftStopped++ // drive IS live; the app is at zero containers → stopped on purpose + case presentStable[bs.hdd]: + // The drive IS live and we still declined: the customer's recorded Stop, or a legacy + // app.yaml at zero containers. Both are "left stopped on purpose", which is what this + // counter has always meant — only the signal behind it changed (R-170). + leftStopped++ default: skipped++ // a deployed drive-backed app whose bind never went live → gate's job } diff --git a/controller/internal/web/intermediary_test.go b/controller/internal/web/intermediary_test.go index edf437c..87e95f8 100644 --- a/controller/internal/web/intermediary_test.go +++ b/controller/internal/web/intermediary_test.go @@ -6,6 +6,8 @@ import ( "gitea.dooplex.hu/admin/felhom-controller/internal/agentapi" "gitea.dooplex.hu/admin/felhom-controller/internal/settings" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" ) func TestAgentWhere(t *testing.T) { @@ -65,7 +67,7 @@ func TestShouldRecreateOnBoot(t *testing.T) { {"no HDD_PATH (SSD-resident)", true, "", true, false}, } for _, c := range cases { - if got := shouldRecreateOnBoot(c.deployed, c.hdd, present, c.hasContainers); got != c.want { + if got := shouldRecreateOnBoot(c.deployed, c.hdd, present, c.hasContainers, stacks.DesiredStateUnknown); got != c.want { t.Errorf("%s: shouldRecreateOnBoot = %v, want %v", c.name, got, c.want) } } @@ -158,7 +160,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, true) { + if !shouldRecreateOnBoot(true, flash, live, true, stacks.DesiredStateUnknown) { t.Fatalf("with the live bind present, the drive-backed app MUST be recreated") } } @@ -170,7 +172,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, true) { + if shouldRecreateOnBoot(true, flash, oldPresent, true, stacks.DesiredStateUnknown) { t.Fatalf("companion: a single early sample reads the not-yet-live bind as absent and must MISS it") } } @@ -192,7 +194,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, true) { + if shouldRecreateOnBoot(true, usb, live, true, stacks.DesiredStateUnknown) { t.Fatalf("an absent drive's app must NOT be recreated here (the gate owns drive-absent)") } } diff --git a/controller/internal/web/recreate_intent_test.go b/controller/internal/web/recreate_intent_test.go new file mode 100644 index 0000000..35ee5b0 --- /dev/null +++ b/controller/internal/web/recreate_intent_test.go @@ -0,0 +1,147 @@ +package web + +import ( + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-170 — the drive-backed boot recreate gate reads the customer's recorded intent instead of +// counting containers, so the two boot gates stop disagreeing about the same question. + +const rgDrive = "/mnt/felhom-drives/hdd_1" + +func rgPresent() map[string]bool { return map[string]bool{rgDrive: true} } + +// --- Group D / Scenario E — the three-way table, every row ---------------------------------------- + +func TestShouldRecreateOnBoot_IntentTable(t *testing.T) { + // RED-PROOF: restore the old `&& hasContainers` ending (i.e. ignore `desired` entirely) and the + // `running/no containers` row fails — that row is the whole point of R-170, and it is the shape a + // power cut mid-compose leaves behind on a drive-backed app. + // Demonstrated in REPORT.md §4. + cases := []struct { + name string + desired string + hasContainers bool + want bool + }{ + {"stopped + no containers", stacks.DesiredStateStopped, false, false}, + {"stopped + containers present", stacks.DesiredStateStopped, true, false}, + {"running + no containers", stacks.DesiredStateRunning, false, true}, + {"running + containers present", stacks.DesiredStateRunning, true, true}, + {"legacy + no containers", stacks.DesiredStateUnknown, false, false}, + {"legacy + containers present", stacks.DesiredStateUnknown, true, true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := shouldRecreateOnBoot(true, rgDrive, rgPresent(), c.hasContainers, c.desired) + if got != c.want { + t.Fatalf("shouldRecreateOnBoot(desired=%q hasContainers=%v) = %v, want %v", + c.desired, c.hasContainers, got, c.want) + } + }) + } +} + +func TestShouldRecreateOnBoot_StoppedIsNeverRecreated_WhateverElseIsTrue(t *testing.T) { + // The safety property R-55 added this gate's filter for, restated on the new signal: a customer's + // Stop must survive a guest reboot. It must hold across every other axis. + for _, hasContainers := range []bool{true, false} { + if shouldRecreateOnBoot(true, rgDrive, rgPresent(), hasContainers, stacks.DesiredStateStopped) { + t.Fatalf("a recorded Stop was recreated (hasContainers=%v) — this silently undoes the "+ + "customer's decision on every guest reboot, which is the defect R-55 existed to fix", + hasContainers) + } + } +} + +func TestShouldRecreateOnBoot_DriveAbsentStillWinsOverIntent(t *testing.T) { + // The `presentStable` term is load-bearing and must NOT have been dropped in sympathy with the + // container count. An app whose drive is absent is never recreated here, whatever its intent — + // the drive gate's Return branch owns it. This is the exact term the BOOT SWEEP was missing + // (R-171), so removing it here would reproduce that hazard in the other gate. + absent := map[string]bool{rgDrive: false} + for _, desired := range []string{stacks.DesiredStateRunning, stacks.DesiredStateStopped, stacks.DesiredStateUnknown} { + if shouldRecreateOnBoot(true, rgDrive, absent, true, desired) { + t.Fatalf("an app whose drive is ABSENT was recreated (desired=%q) — the drive-presence "+ + "term is load-bearing and must outrank intent", desired) + } + } +} + +func TestShouldRecreateOnBoot_OtherGuardsSurviveTheRewrite(t *testing.T) { + // deployed / stable-parent-prefix / HDD_PATH were terms before R-170 and must still be, at the + // strongest intent available — the rewrite reordered the terms and a reorder is how a guard + // silently disappears. + if shouldRecreateOnBoot(false, rgDrive, rgPresent(), true, stacks.DesiredStateRunning) { + t.Fatal("a NOT-deployed app was recreated") + } + if shouldRecreateOnBoot(true, "", rgPresent(), true, stacks.DesiredStateRunning) { + t.Fatal("an app with no HDD_PATH (SSD-resident) was recreated by the DRIVE gate") + } + sys := "/mnt/sys_drive/felhom-data" + if shouldRecreateOnBoot(true, sys, map[string]bool{sys: true}, true, stacks.DesiredStateRunning) { + t.Fatal("a non-stable-parent (system) path was recreated by the drive gate") + } +} + +// --- Scenario F — the two boot gates agree -------------------------------------------------------- + +func TestShouldRecreateOnBoot_AgreesWithBootrecon(t *testing.T) { + // The sibling of bootrecon's TestBothBootGatesAgreeOnIntent, over the SAME fixture table. The two + // gates cannot be called from one package without an import cycle, so the agreement is pinned + // from both sides against an identical table. If this table changes, change the other. + // + // Both answer: "did the customer want this app running?" On a live drive, this gate's verdict + // must equal that answer exactly. + cases := []struct { + desired string + hasContainers bool + wantWanted bool + }{ + {stacks.DesiredStateStopped, false, false}, + {stacks.DesiredStateStopped, true, false}, + {stacks.DesiredStateRunning, false, true}, + {stacks.DesiredStateRunning, true, true}, + {stacks.DesiredStateUnknown, false, false}, + {stacks.DesiredStateUnknown, true, true}, + } + for _, c := range cases { + got := shouldRecreateOnBoot(true, rgDrive, rgPresent(), c.hasContainers, c.desired) + if got != c.wantWanted { + t.Fatalf("gate disagreement: shouldRecreateOnBoot(desired=%q containers=%v) = %v, but "+ + "bootrecon.isBootOrphan answers %v for the same facts. Two boot gates answering the "+ + "same question differently is how R-157 mechanism B survived a year", + c.desired, c.hasContainers, got, c.wantWanted) + } + } +} + +// --- the counter that reports the honoured path --------------------------------------------------- + +func TestRecreateDriveBackedApps_RecordedStopCountsAsLeftStopped(t *testing.T) { + // `leftStopped` has always meant "deliberately not touched", and it is reported at INFO as the + // gate working as intended. After R-170 a recorded Stop must land in that bucket — NOT in + // `skipped`, which is the "bind never went live" failure and fires a WARN. + apps := []bootStack{ + {name: "stopped-app", deployed: true, hdd: rgDrive, hasContainers: false, desired: stacks.DesiredStateStopped}, + {name: "legacy-stopped", deployed: true, hdd: rgDrive, hasContainers: false, desired: stacks.DesiredStateUnknown}, + {name: "wanted-running", deployed: true, hdd: rgDrive, hasContainers: false, desired: stacks.DesiredStateRunning}, + } + var recreated []string + n, skipped, leftStopped := recreateDriveBackedApps(apps, rgPresent(), + func(bs bootStack) { recreated = append(recreated, bs.name) }, func() {}) + + if n != 1 || len(recreated) != 1 || recreated[0] != "wanted-running" { + t.Fatalf("recreated=%v (n=%d), want exactly [wanted-running] — the zero-container app the "+ + "customer wants RUNNING is the R-170 case", recreated, n) + } + if leftStopped != 2 { + t.Fatalf("leftStopped=%d, want 2 (the recorded Stop and the legacy zero-container app)", leftStopped) + } + if skipped != 0 { + t.Fatalf("skipped=%d, want 0 — the drive is live, so nothing was skipped for a missing bind; "+ + "counting a deliberate Stop as skipped would fire a WARN for healthy behaviour", skipped) + } +}