diff --git a/CHANGELOG.md b/CHANGELOG.md index e5403fd..5202382 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,64 @@ ## Changelog +### 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 +have fixed itself and did not. + +**R-51 — a multi-container app whose MAIN container is dead now counts as down.** On 2026-07-20 +`immich-server` sat `Exited` for **18 hours** with the app 100 % unreachable, and the box produced no +dead-app banner and no `app_start_failed` event — while single-container Calibre-Web, down for the +same reason, alerted in 90 seconds (AUDIT-vacation-remote-ops-2026-07-20 F4). + +The defect was one branch in `aggregateState`: a stack with *some* members running and *some* stopped +returned `StateRunning` — "partial" — and `IsDownState` (correctly) does not treat running as down. +So the alarm never had anything to fire on. *(The ROADMAP row's diagnosis — "aggregation classifies +such a stack `unhealthy`" — is wrong at the source; corrected in the row.)* + +- New `StateDegraded`. The mixed branch now asks each DOWN member for its restart policy: a member + docker is supposed to keep running (`always` / `unless-stopped`) makes the stack **degraded**, a + finished one-shot (`no` / `on-failure`) leaves it running. `IsDownState` gains `degraded` and + **nothing else** — the `unhealthy` / `restarting` / `paused` / `unknown` exclusions are byte- + identical, because folding `unhealthy` into down is what fix-3 removed the flapping by not doing. +- An **unreadable** policy counts as supervised (fail-CLOSED), the opposite of the IsDownState + fail-open rule and for a different reason: there the *state* is ambiguous, here a member is known + dead and only the excuse is missing. The P2 census backs it — all 53 catalog templates / 78 + services are `unless-stopped`, and zero one-shot containers exist today. +- The policy read is one `docker inspect` per down member of a *mixed* stack, cached per + container+state and pruned to the live container set, so the 10 s refresh does not grow a docker + call per container. +- Everything that asks "are there live containers here" learns the state too: quiesce + (`RunningAppStacks`), delete's stop-first guard, the export stop-first guard, telemetry, health + probes. Everything that asks "is this app working" counts it as down: the dashboard counter, the + stopped filter, the dead-app banner and the alarm. UI: „Részlegesen leállt", warn colour, and the + URL is flagged unpublished (Traefik 404s when the routed member is the dead one). + +**R-52 — an app the boot left behind now gets exactly one recovery.** The same shutdown left immich +and calibre-web `Exited` while ten sibling containers came back; the controller *reported* them for +18 hours and never started them (F5). + +- New `internal/bootrecon`: one bounded sweep at startup — at most 2 attempts, 30 s apart, then it + stops and the alarm owns the problem. **Never a restart loop.** +- **A deliberate Stop survives a reboot.** The UI's Stop is `compose down`, which REMOVES the + containers; an interrupted boot leaves them behind as `Exited`. So the boot-orphan signature is + "deployed, has containers, and they are down", and a zero-container stack is never touched. +- The whole sweep (5 s settle + one 30 s gap) fits inside the 90 s `deadAppBootGrace`, so a + successful recovery never alerts and a failed one alerts honestly. A test asserts that arithmetic + rather than leaving it to a comment. + +**Seam discipline (the reason both features have a wiring test).** Two inert-seam defects shipped in +the two days before this: controller v0.154.0 and agent v0.91.0, both a correct component with green +tests and no production caller. So the boot sweep is asserted from `package main` — including an AST +walk proving `func main()` actually contains the `go runBootReconcile(...)`. That test was written +first as a `strings.Contains` and **its own red-proof passed it**, because a commented-out call still +contains the string. Comments are not callers; the AST version fails as it should. + +Red-proofs (all run, all failed on the pre-fix shape, all restored): the mix branch reverted to +`return StateRunning` → the immich fixture and both production-path tests fail with `"running"`; the +boot hook commented out → the wiring test fails; the zero-container gate dropped → the user-stopped +app is started, which is the one thing R-52 must never do. + ### v0.155.0 — the restore wizard read the wrong "is something running" flag (2026-07-21) **No new agent coupling — MinAgent stays 0.90.0.** Fixes a defect shipped in v0.154.0 and found by diff --git a/REUSE.md b/REUSE.md index 38e4e83..51c4c9a 100644 --- a/REUSE.md +++ b/REUSE.md @@ -222,6 +222,9 @@ | The DB-only replay window (R-47, v0.153.0) | controller/internal/backup/{offbox_reconstitute,restore_unit}.go | both restore paths: stop → place/volumes → `StartStackServices(dbServices)` → replay → `StartStack` (full) | **THE ordering invariant.** Replaying while the whole stack is up lets the app's own schema management race the dump — measured at 2 s on 2026-07-19 (H4), replay aborted `already exists`. Fail-closed: a dump with NO identifiable DB service refuses BEFORE the first mutation. Every exit from the window (replay error, DB-only start error) MUST still do a best-effort full start, or a failed restore becomes an outage. `hasReplayableDump` excludes `pre-restore-` safety dumps — counting them would arm the window for an app with nothing to replay | | `Manager.OffsiteScratchPair` / `OffsitePairInfo` | controller/internal/backup/offbox_reconstitute.go | reads the restored scratch unit's manifest (`offsite_run_id` / `dumps_at`) + the R-44 sniff | the confirm-dialog honesty surface. All warn-level: a pre-v0.148 (unstamped) pair and an empty-looking dump are SURFACED, never blocked — a false positive that refused a legitimate restore would be worse than the skew | | `appbackup.DumpValidation.LooksEmpty` (R-44 sniff) | controller/internal/appbackup/dbdump.go | computed in ValidateDump's existing single pass; `userTableNames` is EXACT-match | size and table count are both useless as emptiness heuristics (the 2026-07-19 dump: 52MB, 60+ tables, zero users — all geodata). **TRAP: never widen to a substring match on "user"** — it would flag `user_metadata` / `album_user` / `user_audit` on every healthy single-user box. A row wider than the read buffer still counts as a row | +| `Manager.execFn` (func seam) + `restartPolicyLookup` / `inspectRestartPolicyFn` (R-51, v0.156.0) | controller/internal/stacks/manager.go | nil → real `exec.Command` / `docker inspect -f {{.HostConfig.RestartPolicy.Name}}` | `scriptedDocker` in controller/internal/stacks/degraded_test.go drives the WHOLE production path (docker ps → aggregateState → docker inspect) — an aggregateState-only test proves the function, not the caller. Policy answers are cached per container+state and pruned to the live `docker ps` set; a FAILED inspect is deliberately never cached (a hiccup must not pin a container to "unknown") and reads as SUPERVISED, i.e. fail-closed — the opposite of `IsDownState`'s fail-open, because there the state is ambiguous while here a member is known dead | +| `bootrecon.StackProvider` (R-52, v0.156.0) | controller/internal/bootrecon/bootrecon.go | `*stacks.Manager` (GetStacks/StartStack/RefreshStatus) | `fakeStacks` counts StartStack per app; the load-bearing assertion is the NEGATIVE — a zero-container stack (a UI Stop = `compose down` = containers removed) must record **0** starts, while a boot orphan (containers present, Exited) records exactly 1. `Reconciler.sleep` is injected so the 30 s gap costs nothing | +| `bootReconcileFn` + `runBootReconcile` (package-main seam, v0.156.0) | controller/cmd/controller/main.go | `bootrecon.New(mgr, logger).Run` | controller/cmd/controller/bootrecon_wiring_test.go. **The wiring itself is asserted by an AST walk** over `func main()`, not a `strings.Contains` — the substring version passed its own red-proof because a commented-out call still contains the string. Comments are not callers | | `report.SetPendingControllerLog` / `SetControllerLogSource` | controller/internal/report/selftail.go | ACK-armed consume-once self-log pull (the logtail.go shape) | selftail_test.go; source = `logBuffer.Lines`, wired once in main.go | | `util.ParseVersion` / `util.Version.Compare` | controller/internal/util/version.go | THE one semver comparator (house rule: never a second) — selfupdate aliases it; agentapi's MinAgent comparison uses it | rejects pre-release/dev/latest (callers fall back, never trust); numeric compare (0.100 > 0.81) | | `agentapi.AgentVersionReporter` + `featureMinAgent` | controller/internal/agentapi/features.go | version-first Supports (v0.82.0 header channel); probe = fallback for header-less agents | a coupled feature adds BOTH a featureProbes row AND a featureMinAgent row; v0.116.0: `SupportsWithSource` also reports HOW the verdict was reached (version/probe-cache/probe) for the gate log line | diff --git a/controller/README.md b/controller/README.md index d15f24e..a7301a8 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1517,6 +1517,33 @@ the real cooldown, the controller adds no timer). The boot grace prevents false controller's own startup while STILL firing for an app that never came up. This closes the campaign's 4-hour silent CWA death. +**Dead-primary alerting (R-51, v0.156.0).** fix-3 above only ever saw stacks that were *entirely* +down. A multi-container app whose MAIN container died while its helpers kept running aggregated to +`StateRunning` ("partial") and therefore alerted on nothing — `immich-server` was `Exited` for 18 h, +the app 100 % unreachable, with no banner and no event (F4, AUDIT-vacation-remote-ops-2026-07-20). +`aggregateState`'s mixed branch now inspects each DOWN member's docker restart policy: `always` / +`unless-stopped` means docker was supposed to be keeping it up, so the stack becomes +**`StateDegraded`** — a down state, so the existing banner and the existing `app_start_failed` event +fire unchanged. `no` / `on-failure` is a finished one-shot init/migrate container and stays benign. +An unreadable policy counts as supervised (fail-closed: a member is known dead, only the excuse is +missing). The `unhealthy` / `restarting` / `paused` / `unknown` exclusions are untouched — folding +`unhealthy` into down is precisely the flapping fix-3 avoided. UI: „Részlegesen leállt", warn +colour, counted with the stopped apps, URL flagged unpublished (Traefik withholds the route when the +routed member is the dead one). Policy reads are one `docker inspect` per down member of a mixed +stack, cached per container+state. + +**Boot desired-state reconciliation (R-52, v0.156.0, `internal/bootrecon`).** A `deployed: true` app +that missed its boot start used to stay down until a human noticed — the same shutdown that produced +F4 left immich and calibre-web `Exited` while ten sibling containers came back, and they were still +down 18 h later (F5). At startup (5 s after the quiesce recovery, so the two never race) the +controller performs **one bounded sweep**: every deployed, non-protected, not-mid-deploy stack that +still HAS containers and is down gets `StartStack`, at most **2 attempts 30 s apart**, then it stops +and the alarm owns the problem. Never a restart loop. **An app the customer stopped is never +touched** — the UI's Stop is `compose down`, which removes the containers, so "has containers and +they are down" is what distinguishes an interrupted boot from a deliberate stop. The whole sweep +fits inside the 90 s boot grace, so a successful recovery is silent and a failed one still alerts. +Outcome is logged per attempt at INFO; no new hub event (the existing alarm is the escalation). + #### Default Enabled Events Events the customer receives notifications for (configurable in settings): diff --git a/controller/cmd/controller/bootrecon_wiring_test.go b/controller/cmd/controller/bootrecon_wiring_test.go new file mode 100644 index 0000000..f146085 --- /dev/null +++ b/controller/cmd/controller/bootrecon_wiring_test.go @@ -0,0 +1,122 @@ +package main + +import ( + "context" + "go/ast" + "go/parser" + "go/token" + "io" + "log" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/bootrecon" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// §9 rule 6 — the seam-discipline test. Two inert-seam defects shipped in the two days before this +// task (controller v0.154.0 and agent v0.91.0), both the same shape: the component was correct, its +// unit tests injected the seam directly, and the PRODUCTION CALLER was never made. Everything was +// green and the feature did nothing. So R-52 gets its wiring asserted from package main, not only +// from internal/bootrecon. + +// TestRunBootReconcile_InvokesTheSweep pins the function main() actually calls: after the settle +// window it runs the sweep exactly once, with the manager it was handed. +func TestRunBootReconcile_InvokesTheSweep(t *testing.T) { + orig := bootReconcileFn + t.Cleanup(func() { bootReconcileFn = orig }) + origSettle := bootReconcileSettle + t.Cleanup(func() { bootReconcileSettle = origSettle }) + bootReconcileSettle = time.Millisecond + + calls := 0 + var gotMgr bootrecon.StackProvider + bootReconcileFn = func(_ context.Context, mgr bootrecon.StackProvider, _ *log.Logger) bootrecon.Result { + calls++ + gotMgr = mgr + return bootrecon.Result{} + } + + fake := &wiringStacks{} + runBootReconcile(context.Background(), fake, log.New(io.Discard, "", 0)) + + if calls != 1 { + t.Fatalf("the boot sweep ran %d times, want exactly 1 (start-once, never a loop)", calls) + } + if gotMgr != bootrecon.StackProvider(fake) { + t.Fatalf("the sweep was handed %v, want the stack manager main() owns", gotMgr) + } +} + +// A controller shutting down during its own settle window must not start anything. +func TestRunBootReconcile_CancelledDuringSettleDoesNothing(t *testing.T) { + orig := bootReconcileFn + t.Cleanup(func() { bootReconcileFn = orig }) + + calls := 0 + bootReconcileFn = func(context.Context, bootrecon.StackProvider, *log.Logger) bootrecon.Result { + calls++ + return bootrecon.Result{} + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + runBootReconcile(ctx, &wiringStacks{}, log.New(io.Discard, "", 0)) + + if calls != 0 { + t.Fatalf("the sweep ran %d times on a cancelled context, want 0", calls) + } +} + +// The call site itself. A function-variable test can only prove the function is correct — it cannot +// prove main() calls it, which is exactly the hole both inert-seam defects fell through. This walks +// main.go's AST for a `go runBootReconcile(...)` inside func main(); delete or comment out that line +// and this fails, where every behavioural test above would still pass. +// +// It is an AST walk and not a strings.Contains for a reason found while red-proofing it: a +// commented-out call still satisfies a substring match, so the text version passed the very +// red-proof it existed to fail. Comments are not code. +func TestMainWiresBootReconcile(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) + } + + found := false + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name != "main" || fn.Body == nil { + continue + } + ast.Inspect(fn.Body, func(n ast.Node) bool { + gostmt, ok := n.(*ast.GoStmt) + if !ok { + return true + } + if ident, ok := gostmt.Call.Fun.(*ast.Ident); ok && ident.Name == "runBootReconcile" { + found = true + } + return true + }) + } + if !found { + t.Fatal("func main() no longer starts the R-52 boot reconciliation with `go runBootReconcile(...)` " + + "— the sweep is inert (the v0.154.0 / v0.91.0 defect class: a correct component nobody calls)") + } +} + +// The settle window must stay inside the dead-app boot grace, or a successful recovery would alert. +func TestBootReconcileFitsInsideTheBootGrace(t *testing.T) { + worst := bootReconcileSettle + time.Duration(bootrecon.DefaultAttempts-1)*bootrecon.DefaultRetryDelay + if worst >= deadAppBootGrace { + t.Fatalf("worst-case sweep %s does not fit inside the %s boot grace — a successful "+ + "recovery would fire app_start_failed", worst, deadAppBootGrace) + } +} + +type wiringStacks struct{} + +func (w *wiringStacks) GetStacks() []stacks.Stack { return nil } +func (w *wiringStacks) StartStack(string) error { return nil } +func (w *wiringStacks) RefreshStatus() error { return nil } diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 85c410f..7ea09b0 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -25,6 +25,7 @@ import ( "gitea.dooplex.hu/admin/felhom-controller/internal/appexport" "gitea.dooplex.hu/admin/felhom-controller/internal/assets" "gitea.dooplex.hu/admin/felhom-controller/internal/backup" + "gitea.dooplex.hu/admin/felhom-controller/internal/bootrecon" "gitea.dooplex.hu/admin/felhom-controller/internal/bootstrap" "gitea.dooplex.hu/admin/felhom-controller/internal/channelhealth" cf "gitea.dooplex.hu/admin/felhom-controller/internal/cloudflare" @@ -225,6 +226,14 @@ func main() { // Recover FIRST (restart any stacks left stopped by a crash mid-quiesce), then start the loop. quiesceLoop := startQuiesceLoop(ctx, cfg, stackMgr, logger) + // --- R-52: boot desired-state reconciliation --- + // A deployed app that missed its boot start used to stay down until a human noticed (F5: immich + // and calibre-web sat Exited for ~18 h while ten siblings came back). One bounded start-once + // 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. + go runBootReconcile(ctx, stackMgr, logger) + // --- Start CPU collector --- cpuCollector := system.NewCPUCollector(5 * time.Second) cpuCollector.Start(ctx) @@ -1109,6 +1118,29 @@ func main() { // own boot. After the grace, an app that still isn't running alerts (the F11 dead-at-boot case). const deadAppBootGrace = 90 * time.Second +// 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. +var bootReconcileSettle = 5 * 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) +} + +// 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. +func runBootReconcile(ctx context.Context, mgr bootrecon.StackProvider, logger *log.Logger) { + select { + case <-ctx.Done(): + return + case <-time.After(bootReconcileSettle): + } + bootReconcileFn(ctx, mgr, logger) +} + // scanDeployedAppRunStates returns the fix-3 view of the deployed apps: the DEAD ones (for the // state-based dashboard banner) and EVERY deployed app's run state (for the notifier's one-event-per- // transition tracking). Deploying apps are skipped (mid-deploy is not a fault). Pure over GetStacks(). @@ -1472,7 +1504,9 @@ func (a *exportAdapter) GetStackHDDPath(name string) string { func (a *exportAdapter) IsStackRunning(name string) bool { s, ok := a.mgr.GetStack(name) - return ok && s.State == stacks.StateRunning + // StateDegraded (R-51) counts as running: the export must stop the still-live members before + // reading their volumes, exactly as it would for a fully running stack. + return ok && (s.State == stacks.StateRunning || s.State == stacks.StateDegraded) } func (a *exportAdapter) StopStack(name string) error { diff --git a/controller/internal/bootrecon/bootrecon.go b/controller/internal/bootrecon/bootrecon.go new file mode 100644 index 0000000..7cb72e5 --- /dev/null +++ b/controller/internal/bootrecon/bootrecon.go @@ -0,0 +1,197 @@ +// Package bootrecon implements R-52: the bounded, start-ONCE recovery of apps that were left +// behind by an interrupted boot. +// +// The live failure it closes (AUDIT-vacation-remote-ops-2026-07-20, finding F5): a pre-transport +// shutdown left `immich-server` and `calibre-web` Exited; ten sibling containers came back and +// those two did not, and they were still down ~18 hours later. The controller REPORTED them (the +// 30 s deadapp-check) but never started them — deployed-but-stopped was an alarm with no recovery. +// +// Two deliberate boundaries, both load-bearing: +// +// - **Bounded, never a loop.** At most `attempts` tries, `retryDelay` apart, then it stops and the +// alarm owns the problem. A restart loop would paper over a genuinely broken app forever and +// hammer docker while doing it. +// - **A user's Stop survives a reboot.** The UI's Stop is `docker compose down`, which REMOVES the +// containers; a boot interruption leaves them behind as Exited. So "has containers on disk that +// are down" is the boot-orphan signature, and a stack with ZERO containers is deliberately never +// touched. This distinction is the whole safety argument — see TestReconcile_UserStoppedAppIsNeverStarted. +// +// It runs inside the notifier's boot grace (cmd/controller/main.go `deadAppBootGrace`), so a +// successful recovery never fires an alert and a failed one alerts honestly once the grace expires. +package bootrecon + +import ( + "context" + "log" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// StackProvider is the slice of *stacks.Manager this package needs. Declared consumer-side so the +// tests can count StartStack calls without a docker anywhere near them. +type StackProvider interface { + GetStacks() []stacks.Stack + StartStack(name string) error + RefreshStatus() error +} + +const ( + // DefaultAttempts is the total number of start attempts per boot (not per app per retry-forever). + DefaultAttempts = 2 + // DefaultRetryDelay spaces the attempts. 2 × 30 s fits comfortably inside the 90 s boot grace, + // so a recovery that works is silent and one that does not is honest. + DefaultRetryDelay = 30 * time.Second +) + +// Reconciler performs the start-once sweep. Zero value is not usable — use New. +type Reconciler struct { + stacks StackProvider + logger *log.Logger + attempts int + retryDelay time.Duration + + // sleep is the inter-attempt wait; injectable so tests never spend 30 real seconds. + sleep func(context.Context, time.Duration) +} + +// Result is the outcome, returned for logging/testing (the hub learns about failures only through +// the existing app_start_failed alarm — this package deliberately pushes no events of its own). +type Result struct { + Candidates []string // boot-orphaned apps found + 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) +} + +// New builds a Reconciler with the shipped defaults. +func New(p StackProvider, logger *log.Logger) *Reconciler { + return &Reconciler{ + stacks: p, + logger: logger, + attempts: DefaultAttempts, + retryDelay: DefaultRetryDelay, + sleep: sleepCtx, + } +} + +func sleepCtx(ctx context.Context, d time.Duration) { + t := time.NewTimer(d) + defer t.Stop() + select { + case <-ctx.Done(): + case <-t.C: + } +} + +// isBootOrphan reports whether a stack is an app the boot left behind. +// +// The gate, term by term: +// - Deployed — an app the customer asked to have running. +// - not Protected — traefik/cloudflared/felhom-controller have their own supervision; this must +// never race the base-stack self-heal. +// - not Deploying — mid-deploy is not a fault. +// - has containers — the D-case guard: a UI Stop removes them, and a deliberate stop must survive +// a reboot. +// - IsDownState — stopped/exited/degraded (R-51 included: a boot that half-started a stack is the +// same interrupted-boot shape). +func isBootOrphan(s stacks.Stack) bool { + return s.Deployed && !s.Protected && !s.Deploying && + len(s.Containers) > 0 && stacks.IsDownState(s.State) +} + +// Run performs the sweep once and returns what happened. It is safe to call with no boot orphans +// (the quiet path logs one DEBUG-free INFO-free line — see below) and it never returns an error: +// a failure to start is an app-level fact the alarm reports, not a controller startup failure. +func (r *Reconciler) Run(ctx context.Context) Result { + var res 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) + } + } + sortStrings(res.Candidates) + + 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). + r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: no boot-orphaned apps (nothing to start)") + return res + } + + r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: %d boot-orphaned app(s) found: %v — up to %d attempt(s)", + len(res.Candidates), res.Candidates, r.attempts) + + for attempt := 1; attempt <= r.attempts && len(pending) > 0; attempt++ { + res.Attempts = attempt + for _, name := range sortedKeys(pending) { + if ctx.Err() != nil { + break + } + start := time.Now() + if err := r.stacks.StartStack(name); err != nil { + r.logger.Printf("[WARN] [bootrecon] Boot reconciliation attempt %d/%d: start %q failed after %.1fs: %v", + attempt, r.attempts, name, time.Since(start).Seconds(), err) + continue + } + r.logger.Printf("[INFO] [bootrecon] Boot reconciliation attempt %d/%d: started %q (took %.1fs)", + attempt, r.attempts, name, time.Since(start).Seconds()) + } + if ctx.Err() != nil { + break + } + + // Re-read reality rather than trusting a nil error: `compose up -d` exits 0 on a crash-loop + // (a session-critical invariant of this repo), so only a fresh docker ps can say whether the + // app is actually up. + if err := r.stacks.RefreshStatus(); err != nil { + r.logger.Printf("[WARN] [bootrecon] Boot reconciliation: status refresh failed: %v", err) + } + for _, s := range r.stacks.GetStacks() { + if pending[s.Name] && !stacks.IsDownState(s.State) { + delete(pending, s.Name) + res.Recovered = append(res.Recovered, s.Name) + } + } + + if len(pending) > 0 && attempt < r.attempts { + r.sleep(ctx, r.retryDelay) + } + } + + res.StillDown = sortedKeys(pending) + sortStrings(res.Recovered) + + if len(res.StillDown) == 0 { + r.logger.Printf("[INFO] [bootrecon] Boot reconciliation complete: %d app(s) recovered in %d attempt(s): %v", + len(res.Recovered), res.Attempts, res.Recovered) + } else { + // Deliberately no hub event here: the app_start_failed alarm fires on its own once the boot + // grace expires, and two events for one dead app is how an operator inbox becomes noise. + r.logger.Printf("[WARN] [bootrecon] Boot reconciliation gave up after %d attempt(s): recovered=%v still down=%v (the dead-app alarm now owns these)", + res.Attempts, res.Recovered, res.StillDown) + } + return res +} + +// --- tiny local helpers (no dependency on sort ordering semantics elsewhere) --- + +func sortedKeys(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sortStrings(out) + return out +} + +func sortStrings(s []string) { + for i := 1; i < len(s); i++ { + for j := i; j > 0 && s[j] < s[j-1]; j-- { + s[j], s[j-1] = s[j-1], s[j] + } + } +} diff --git a/controller/internal/bootrecon/bootrecon_test.go b/controller/internal/bootrecon/bootrecon_test.go new file mode 100644 index 0000000..09da95f --- /dev/null +++ b/controller/internal/bootrecon/bootrecon_test.go @@ -0,0 +1,280 @@ +package bootrecon + +import ( + "context" + "errors" + "io" + "log" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// fakeStacks counts StartStack calls per app — the assertion that matters in BOTH directions: +// scenario C needs exactly-bounded starts, scenario D needs a start count of ZERO. +type fakeStacks struct { + list []stacks.Stack + starts map[string]int + failWith map[string]error + // onStart mutates the world the way a real successful start would (fresh docker ps). + onStart func(f *fakeStacks, name string) + refreshN int +} + +func (f *fakeStacks) GetStacks() []stacks.Stack { return f.list } +func (f *fakeStacks) RefreshStatus() error { f.refreshN++; return nil } + +func (f *fakeStacks) StartStack(name string) error { + if f.starts == nil { + f.starts = map[string]int{} + } + f.starts[name]++ + if err := f.failWith[name]; err != nil { + return err + } + if f.onStart != nil { + f.onStart(f, name) + } + return nil +} + +func (f *fakeStacks) setState(name string, st stacks.ContainerState) { + for i := range f.list { + if f.list[i].Name == name { + f.list[i].State = st + } + } +} + +// comesUp is the ordinary success behaviour: the app is running after the start. +func comesUp(f *fakeStacks, name string) { f.setState(name, stacks.StateRunning) } + +func newTestReconciler(f *fakeStacks) (*Reconciler, *int) { + slept := 0 + r := New(f, log.New(io.Discard, "", 0)) + r.sleep = func(context.Context, time.Duration) { slept++ } + return r, &slept +} + +// bootOrphan is the F5 shape: deployed, containers still present on disk, all Exited — the boot +// interrupted them, nobody stopped them. +func bootOrphan(name string) stacks.Stack { + return stacks.Stack{ + Name: name, + Deployed: true, + State: stacks.StateExited, + Containers: []stacks.ContainerInfo{ + {Name: name + "-app", State: stacks.StateExited, Status: "Exited (0) 3 minutes ago"}, + }, + } +} + +// userStopped is the UI-Stop shape: `docker compose down` REMOVED the containers. +func userStopped(name string) stacks.Stack { + return stacks.Stack{Name: name, Deployed: true, State: stacks.StateStopped, Containers: nil} +} + +// --- Scenario C ------------------------------------------------------------------------------- + +func TestReconcile_BootOrphanGetsExactlyOneRecovery(t *testing.T) { + f := &fakeStacks{ + list: []stacks.Stack{bootOrphan("immich"), bootOrphan("calibre-web")}, + onStart: comesUp, + } + r, slept := newTestReconciler(f) + + res := r.Run(context.Background()) + + for _, name := range []string{"immich", "calibre-web"} { + if f.starts[name] != 1 { + t.Fatalf("StartStack(%q) called %d times, want exactly 1", name, f.starts[name]) + } + } + if len(res.Recovered) != 2 || len(res.StillDown) != 0 { + t.Fatalf("recovered=%v stillDown=%v, want both apps recovered", res.Recovered, res.StillDown) + } + if res.Attempts != 1 { + t.Fatalf("attempts = %d, want 1 — a success must not retry", res.Attempts) + } + if *slept != 0 { + t.Fatalf("slept %d times after a first-attempt success, want 0", *slept) + } +} + +// --- Scenario D (the WRONG case: this must assert the negative) --------------------------------- + +func TestReconcile_UserStoppedAppIsNeverStarted(t *testing.T) { + f := &fakeStacks{ + list: []stacks.Stack{userStopped("jellyfin"), bootOrphan("immich")}, + onStart: comesUp, + } + r, _ := newTestReconciler(f) + + res := r.Run(context.Background()) + + if n := f.starts["jellyfin"]; n != 0 { + t.Fatalf("StartStack(\"jellyfin\") called %d times, want 0 — a deliberate Stop must survive a reboot", n) + } + if f.starts["immich"] != 1 { + t.Fatalf("the real boot orphan was not started: %v", f.starts) + } + for _, c := range res.Candidates { + if c == "jellyfin" { + t.Fatalf("a zero-container stack must never be a reconciliation candidate: %v", res.Candidates) + } + } +} + +// --- Bounded, never a loop ---------------------------------------------------------------------- + +func TestReconcile_StopsAfterTwoAttemptsAndHandsOverToTheAlarm(t *testing.T) { + f := &fakeStacks{ + list: []stacks.Stack{bootOrphan("immich")}, + failWith: map[string]error{"immich": errors.New("compose up: exit status 1")}, + } + r, slept := newTestReconciler(f) + + res := r.Run(context.Background()) + + if f.starts["immich"] != DefaultAttempts { + t.Fatalf("StartStack called %d times, want exactly %d (bounded, never a loop)", f.starts["immich"], DefaultAttempts) + } + if *slept != DefaultAttempts-1 { + t.Fatalf("slept %d times, want %d (one wait BETWEEN attempts, never after the last)", *slept, DefaultAttempts-1) + } + if len(res.StillDown) != 1 || res.StillDown[0] != "immich" { + t.Fatalf("stillDown = %v, want [immich] — the alarm must inherit the failure", res.StillDown) + } +} + +// `compose up -d` exits 0 on a crash-loop, so a nil error is not proof the app is up. Only a +// re-read of docker ps can retire a candidate. +func TestReconcile_NilErrorIsNotProofTheAppCameUp(t *testing.T) { + f := &fakeStacks{list: []stacks.Stack{bootOrphan("immich")}} // onStart nil → stays Exited + r, _ := newTestReconciler(f) + + res := r.Run(context.Background()) + + if f.starts["immich"] != DefaultAttempts { + t.Fatalf("StartStack called %d times, want %d — a still-down app must be retried", f.starts["immich"], DefaultAttempts) + } + if len(res.StillDown) != 1 { + t.Fatalf("stillDown = %v, want the app still listed despite StartStack returning nil", res.StillDown) + } + if f.refreshN < 1 { + t.Fatalf("RefreshStatus was never called — the outcome was taken on trust") + } +} + +// A second-attempt success must still end clean (and must not alert). +func TestReconcile_SecondAttemptSucceeds(t *testing.T) { + f := &fakeStacks{list: []stacks.Stack{bootOrphan("immich")}} + f.onStart = func(fs *fakeStacks, name string) { + if fs.starts[name] >= 2 { + comesUp(fs, name) + } + } + r, slept := newTestReconciler(f) + + res := r.Run(context.Background()) + + if f.starts["immich"] != 2 { + t.Fatalf("StartStack called %d times, want 2", f.starts["immich"]) + } + if *slept != 1 { + t.Fatalf("slept %d times, want 1", *slept) + } + if len(res.StillDown) != 0 || len(res.Recovered) != 1 { + t.Fatalf("recovered=%v stillDown=%v, want a clean recovery on attempt 2", res.Recovered, res.StillDown) + } +} + +// --- The gate, term by term --------------------------------------------------------------------- + +func TestIsBootOrphan_Gate(t *testing.T) { + base := bootOrphan("app") + cases := []struct { + name string + mut func(s *stacks.Stack) + want bool + }{ + {"boot orphan", func(*stacks.Stack) {}, true}, + {"degraded counts (R-51 half-started boot)", func(s *stacks.Stack) { s.State = stacks.StateDegraded }, true}, + {"not deployed", func(s *stacks.Stack) { s.Deployed = false }, false}, + {"protected infra", func(s *stacks.Stack) { s.Protected = true }, false}, + {"mid-deploy", func(s *stacks.Stack) { s.Deploying = true }, false}, + {"no containers (UI Stop)", func(s *stacks.Stack) { s.Containers = nil }, false}, + {"running", func(s *stacks.Stack) { s.State = stacks.StateRunning }, false}, + {"unhealthy is not down", func(s *stacks.Stack) { s.State = stacks.StateUnhealthy }, false}, + {"restarting recovers itself", func(s *stacks.Stack) { s.State = stacks.StateRestarting }, false}, + {"paused is deliberate", func(s *stacks.Stack) { s.State = stacks.StatePaused }, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := base + s.Containers = append([]stacks.ContainerInfo(nil), base.Containers...) + tc.mut(&s) + if got := isBootOrphan(s); got != tc.want { + t.Fatalf("isBootOrphan = %v, want %v", got, tc.want) + } + }) + } +} + +// A cancelled context (controller shutting down mid-boot) must abandon the sweep, not soldier on. +func TestReconcile_ContextCancellationStops(t *testing.T) { + f := &fakeStacks{list: []stacks.Stack{bootOrphan("immich")}} + r, _ := newTestReconciler(f) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + res := r.Run(ctx) + + if f.starts["immich"] != 0 { + t.Fatalf("StartStack called %d times on a cancelled context, want 0", f.starts["immich"]) + } + if len(res.Candidates) != 1 { + t.Fatalf("candidates = %v, want the app still identified", res.Candidates) + } +} + +// The quiet path must be observable — "nothing to do" and "never ran" must not look identical. +func TestReconcile_QuietPathLogsAndStartsNothing(t *testing.T) { + f := &fakeStacks{list: []stacks.Stack{{Name: "immich", Deployed: true, State: stacks.StateRunning, + Containers: []stacks.ContainerInfo{{Name: "immich-app", State: stacks.StateRunning}}}}} + var buf logCapture + r := New(f, log.New(&buf, "", 0)) + r.sleep = func(context.Context, time.Duration) {} + + res := r.Run(context.Background()) + + if len(f.starts) != 0 { + t.Fatalf("a healthy box must produce zero starts, got %v", f.starts) + } + if len(res.Candidates) != 0 { + t.Fatalf("candidates = %v, want none", res.Candidates) + } + if !buf.contains("no boot-orphaned apps") { + t.Fatalf("the quiet path logged nothing identifiable: %q", buf.String()) + } +} + +type logCapture struct{ b []byte } + +func (l *logCapture) Write(p []byte) (int, error) { l.b = append(l.b, p...); return len(p), nil } +func (l *logCapture) String() string { return string(l.b) } +func (l *logCapture) contains(s string) bool { + return len(l.b) > 0 && bytesContains(l.b, []byte(s)) +} + +func bytesContains(hay, needle []byte) bool { + for i := 0; i+len(needle) <= len(hay); i++ { + if string(hay[i:i+len(needle)]) == string(needle) { + return true + } + } + return false +} + +var _ io.Writer = (*logCapture)(nil) diff --git a/controller/internal/report/telemetry.go b/controller/internal/report/telemetry.go index 5caf799..2d99539 100644 --- a/controller/internal/report/telemetry.go +++ b/controller/internal/report/telemetry.go @@ -228,7 +228,7 @@ func buildControllerTelemetry(telemetry []metrics.ContainerTelemetry, logs []met // etc. are excluded to avoid sending zero-value telemetry to the hub. func isStackRunning(state stacks.ContainerState) bool { switch state { - case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting: + case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting, stacks.StateDegraded: return true default: return false diff --git a/controller/internal/stacks/degraded_test.go b/controller/internal/stacks/degraded_test.go new file mode 100644 index 0000000..47f7443 --- /dev/null +++ b/controller/internal/stacks/degraded_test.go @@ -0,0 +1,279 @@ +package stacks + +import ( + "fmt" + "io" + "log" + "strings" + "sync" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +// R-51 (v0.156.0). The live defect these tests pin: on 2026-07-20 `immich-server` sat Exited for +// 18 hours behind three running helpers, the stack aggregated to StateRunning ("partial"), and +// because StateRunning is not a down state NOTHING fired — no dashboard banner, no +// `app_start_failed` hub event — while single-container Calibre-Web, down for the same reason, +// alerted in 90 s. Evidence: felhom.eu/documentation/audits/AUDIT-vacation-remote-ops-2026-07-20.md +// finding F4. +// +// RED-PROOF (recorded in REPORT.md): with the mix branch reverted to its pre-v0.156.0 body +// +// if running > 0 { return StateRunning } +// +// TestAggregateState_DeadSupervisedMemberIsDegraded fails with +// "aggregateState = running, want degraded", which is exactly the shape the audit observed. + +// immichLike is the F4 fixture: the primary Exited, the helpers up. +func immichLike() []ContainerInfo { + return []ContainerInfo{ + {Name: "immich-server", State: StateExited, Status: "Exited (137) 18 hours ago"}, + {Name: "immich-machine-learning", State: StateRunning, Status: "Up 18 hours"}, + {Name: "immich-redis", State: StateRunning, Status: "Up 18 hours (healthy)"}, + {Name: "immich-postgres", State: StateRunning, Status: "Up 18 hours (healthy)"}, + } +} + +// policyMap builds a lookup over a name→policy table; an unlisted name reads as UNKNOWN (""). +func policyMap(t *testing.T, m map[string]string) restartPolicyLookup { + t.Helper() + return func(name string) string { return m[name] } +} + +func TestAggregateState_DeadSupervisedMemberIsDegraded(t *testing.T) { + got := aggregateState(immichLike(), policyMap(t, map[string]string{ + "immich-server": "unless-stopped", + "immich-machine-learning": "unless-stopped", + "immich-redis": "unless-stopped", + "immich-postgres": "unless-stopped", + })) + if got != StateDegraded { + t.Fatalf("aggregateState = %q, want %q (a dead supervised primary must not read as running)", got, StateDegraded) + } + if !IsDownState(got) { + t.Fatalf("IsDownState(%q) = false — the whole point of R-51 is that this state alerts", got) + } +} + +// Scenario B: a one-shot init/migrate container that has legitimately finished must NOT alarm. +func TestAggregateState_OneShotExitedMemberIsBenign(t *testing.T) { + for _, policy := range []string{"no", "on-failure", ""} { + name := policy + if name == "" { + name = "(absent)" + } + t.Run(name, func(t *testing.T) { + containers := []ContainerInfo{ + {Name: "app-migrate", State: StateExited, Status: "Exited (0) 2 minutes ago"}, + {Name: "app-web", State: StateRunning, Status: "Up 2 minutes"}, + } + got := aggregateState(containers, policyMap(t, map[string]string{ + "app-migrate": policy, + "app-web": "unless-stopped", + })) + want := StateRunning + if policy == "" { + // UNKNOWN is deliberately fail-CLOSED — see supervisedPolicy. An absent policy in + // the compose file resolves to Docker's "no" at inspect time, so the "" case here + // is the INSPECT-FAILED case, not the no-restart-policy case. + want = StateDegraded + } + if got != want { + t.Fatalf("policy %q: aggregateState = %q, want %q", policy, got, want) + } + }) + } +} + +// The unchanged branches: R-51 must not move any state the pre-existing aggregation produced. +func TestAggregateState_UnchangedBranches(t *testing.T) { + all := policyMap(t, map[string]string{"a": "unless-stopped", "b": "unless-stopped"}) + cases := []struct { + name string + containers []ContainerInfo + want ContainerState + }{ + {"no containers", nil, StateNotDeployed}, + {"all running", []ContainerInfo{{Name: "a", State: StateRunning}, {Name: "b", State: StateRunning}}, StateRunning}, + {"all stopped", []ContainerInfo{{Name: "a", State: StateExited}, {Name: "b", State: StateStopped}}, StateStopped}, + {"any unhealthy wins", []ContainerInfo{{Name: "a", State: StateUnhealthy}, {Name: "b", State: StateExited}}, StateUnhealthy}, + {"any starting wins over exited", []ContainerInfo{{Name: "a", State: StateStarting}, {Name: "b", State: StateExited}}, StateStarting}, + {"any restarting wins over exited", []ContainerInfo{{Name: "a", State: StateRestarting}, {Name: "b", State: StateExited}}, StateRestarting}, + {"single container exited", []ContainerInfo{{Name: "a", State: StateExited}}, StateStopped}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := aggregateState(tc.containers, all); got != tc.want { + t.Fatalf("aggregateState = %q, want %q", got, tc.want) + } + }) + } +} + +// The unhealthy/restarting/paused/unknown exclusions are the fix-3 contract (downstate_test.go owns +// them). This asserts the ONE addition, so a future reader can see R-51 widened the set by exactly +// one state and by nothing else. +func TestIsDownState_DegradedIsTheOnlyAddition(t *testing.T) { + if !IsDownState(StateDegraded) { + t.Fatalf("IsDownState(degraded) = false, want true") + } + for _, s := range []ContainerState{StateUnhealthy, StateRestarting, StatePaused, StateUnknown} { + if IsDownState(s) { + t.Fatalf("IsDownState(%q) = true — R-51 must not touch the fix-3 exclusions", s) + } + } +} + +// --- production-path wiring test (§9 rule 6) --------------------------------------------------- +// +// Proves the chain the box actually runs: RefreshStatus → docker ps → aggregateState → docker +// inspect. An aggregateState-only test proves the function, not the caller. + +type scriptedDocker struct { + mu sync.Mutex + ps string + policies map[string]string + inspects []string // every container name inspected, in order +} + +func (s *scriptedDocker) exec(name string, args ...string) (string, error) { + s.mu.Lock() + defer s.mu.Unlock() + if name != "docker" { + return "", fmt.Errorf("unexpected command %q", name) + } + switch { + case len(args) > 0 && args[0] == "ps": + return s.ps, nil + case len(args) > 0 && args[0] == "inspect": + target := args[len(args)-1] + s.inspects = append(s.inspects, target) + p, ok := s.policies[target] + if !ok { + return "", fmt.Errorf("no such container: %s", target) + } + return p + "\n", nil + } + return "", fmt.Errorf("unexpected docker args %v", args) +} + +func (s *scriptedDocker) inspectCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.inspects) +} + +func psLine(name, state, status, project string) string { + return strings.Join([]string{name, "img:1", state, status, project}, "\t") +} + +func TestRefreshStatus_WiresDegradedThroughTheRealPath(t *testing.T) { + dock := &scriptedDocker{ + ps: strings.Join([]string{ + psLine("immich-server", "exited", "Exited (137) 18 hours ago", "immich"), + psLine("immich-redis", "running", "Up 18 hours (healthy)", "immich"), + psLine("calibre-web", "running", "Up 18 hours", "calibre-web"), + }, "\n"), + policies: map[string]string{"immich-server": "unless-stopped"}, + } + m := &Manager{ + cfg: &config.Config{}, + logger: log.New(io.Discard, "", 0), + execFn: dock.exec, + stacks: map[string]*Stack{ + "immich": {Name: "immich", Deployed: true}, + "calibre-web": {Name: "calibre-web", Deployed: true}, + }, + } + + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus: %v", err) + } + if got := m.stacks["immich"].State; got != StateDegraded { + t.Fatalf("immich state = %q, want %q (the F4 shape must reach the stack map)", got, StateDegraded) + } + if got := m.stacks["calibre-web"].State; got != StateRunning { + t.Fatalf("calibre-web state = %q, want %q — a healthy app must be untouched", got, StateRunning) + } + + // Only the DOWN member of the MIXED stack is inspected: never the running members, never the + // healthy stack. An inspect per container per 10 s refresh would be a real docker load. + if n := dock.inspectCount(); n != 1 { + t.Fatalf("docker inspect called %d times, want exactly 1 (%v)", n, dock.inspects) + } + + // Second refresh: the answer comes from the cache, so the inspect count must NOT move. + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus (2nd): %v", err) + } + if n := dock.inspectCount(); n != 1 { + t.Fatalf("docker inspect called %d times after a second refresh, want 1 — the cache is not being used", n) + } + if got := m.stacks["immich"].State; got != StateDegraded { + t.Fatalf("immich state after 2nd refresh = %q, want %q", got, StateDegraded) + } +} + +// A container that vanishes must not leave its policy behind — an unbounded cache in a process that +// runs for months is a slow leak, and a stale entry would answer for a recreated container. +func TestRestartPolicyCache_PrunesVanishedContainers(t *testing.T) { + dock := &scriptedDocker{ + ps: strings.Join([]string{ + psLine("app-init", "exited", "Exited (0) 1 minute ago", "app"), + psLine("app-web", "running", "Up 1 minute", "app"), + }, "\n"), + policies: map[string]string{"app-init": "no"}, + } + m := &Manager{ + cfg: &config.Config{}, + logger: log.New(io.Discard, "", 0), + execFn: dock.exec, + stacks: map[string]*Stack{"app": {Name: "app", Deployed: true}}, + } + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus: %v", err) + } + if got := m.stacks["app"].State; got != StateRunning { + t.Fatalf("state = %q, want running (a finished one-shot must not alarm)", got) + } + if len(m.restartPolicyCache) != 1 { + t.Fatalf("cache size = %d, want 1", len(m.restartPolicyCache)) + } + + // The one-shot container is reaped; only the web container remains. + dock.ps = psLine("app-web", "running", "Up 5 minutes", "app") + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus (2nd): %v", err) + } + if len(m.restartPolicyCache) != 0 { + t.Fatalf("cache size = %d after the container vanished, want 0: %v", len(m.restartPolicyCache), m.restartPolicyCache) + } +} + +// An inspect failure must not silence the alarm — see supervisedPolicy's fail-closed rationale. +func TestRefreshStatus_InspectFailureStillDegrades(t *testing.T) { + dock := &scriptedDocker{ + ps: strings.Join([]string{ + psLine("immich-server", "exited", "Exited (137) 1 hour ago", "immich"), + psLine("immich-redis", "running", "Up 1 hour", "immich"), + }, "\n"), + policies: map[string]string{}, // every inspect fails + } + m := &Manager{ + cfg: &config.Config{}, + logger: log.New(io.Discard, "", 0), + execFn: dock.exec, + stacks: map[string]*Stack{"immich": {Name: "immich", Deployed: true}}, + } + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus: %v", err) + } + if got := m.stacks["immich"].State; got != StateDegraded { + t.Fatalf("state = %q, want %q — an unreadable policy must not lose the alarm", got, StateDegraded) + } + // A failed inspect is deliberately NOT cached, so the next cycle retries. + if len(m.restartPolicyCache) != 0 { + t.Fatalf("failed inspect was cached: %v", m.restartPolicyCache) + } +} diff --git a/controller/internal/stacks/delete.go b/controller/internal/stacks/delete.go index c8c4d0c..5afd3ee 100644 --- a/controller/internal/stacks/delete.go +++ b/controller/internal/stacks/delete.go @@ -110,7 +110,9 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, } // Must be stopped (not running) - if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting { + // StateDegraded (R-51) counts as running here: a degraded stack still has LIVE containers, and + // deleting its directory out from under them would leave orphans behind. + if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting || stack.State == StateDegraded { return nil, fmt.Errorf("stack %q is still running — stop it first before deleting", name) } @@ -313,7 +315,9 @@ func (m *Manager) RemoveStack(name string, removeHDDData bool, backupPathsToRemo } // Must be stopped (not running) - if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting { + // StateDegraded (R-51) counts as running here: a degraded stack still has LIVE containers, and + // deleting its directory out from under them would leave orphans behind. + if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting || stack.State == StateDegraded { return nil, fmt.Errorf("stack %q is still running — stop it first before removing", name) } diff --git a/controller/internal/stacks/healthprobe.go b/controller/internal/stacks/healthprobe.go index 8652709..4b86fe1 100644 --- a/controller/internal/stacks/healthprobe.go +++ b/controller/internal/stacks/healthprobe.go @@ -27,7 +27,9 @@ func (m *Manager) RunHealthProbes() error { skippedNotDue := 0 skippedNoContainer := 0 for name, stack := range m.stacks { - if stack.State != StateRunning && stack.State != StateUnhealthy { + // StateDegraded (R-51) is probed too: its live members still answer, and the probe result + // only ever overrides StateRunning below, so a degraded stack can never be masked as unhealthy. + if stack.State != StateRunning && stack.State != StateUnhealthy && stack.State != StateDegraded { continue } hc := stack.Meta.HealthCheck diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index 88b72d8..5936e70 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -28,6 +28,7 @@ const ( StateStarting ContainerState = "starting" // running but health: starting StateUnhealthy ContainerState = "unhealthy" // running but health: unhealthy StateStopped ContainerState = "stopped" + StateDegraded ContainerState = "degraded" // multi-container stack: a SUPERVISED member is dead (R-51) StateRestarting ContainerState = "restarting" StateExited ContainerState = "exited" StatePaused ContainerState = "paused" @@ -38,13 +39,20 @@ const ( ) // IsDownState reports whether a container state means a DEPLOYED app is not running and won't recover -// on its own (fix-3, CAMPAIGN-3). Only `stopped` and `exited` qualify — a Docker "created"/"dead" -// container (a failed-at-boot app, the F11 case) resolves to `stopped`. Deliberately NOT `starting` -// / `unhealthy` (running, with their own health handling), `restarting` (self-recovering), -// `deploying` (mid-deploy), `paused` (a deliberate user action), or `unknown` (ambiguous — fail-open, -// never manufacture a dead-app alert from an inconclusive read). +// on its own (fix-3, CAMPAIGN-3). Only `stopped`, `exited` and `degraded` qualify — a Docker +// "created"/"dead" container (a failed-at-boot app, the F11 case) resolves to `stopped`. Deliberately +// NOT `starting` / `unhealthy` (running, with their own health handling), `restarting` +// (self-recovering), `deploying` (mid-deploy), `paused` (a deliberate user action), or `unknown` +// (ambiguous — fail-open, never manufacture a dead-app alert from an inconclusive read). +// +// R-51 (v0.156.0) added `degraded`: a multi-container stack whose SUPERVISED member is dead is as +// unreachable as a single-container app that exited (immich-server sat Exited for 18 h with the app +// 100 % dead and no alert, while single-container Calibre-Web alerted in 90 s). This is deliberately +// NOT the same as folding `unhealthy` into down — that exclusion stays byte-identical, because +// `unhealthy` is a *running* container whose healthcheck is failing and folding it in reintroduces +// the flapping fix-3 was added to stop. func IsDownState(s ContainerState) bool { - return s == StateStopped || s == StateExited + return s == StateStopped || s == StateExited || s == StateDegraded } // ContainerInfo holds status info about a single container within a stack. @@ -108,6 +116,17 @@ type Manager struct { backupRunning func() bool // mutual exclusion with the backup orchestrator (Change 3) migDoneHook func(*MigrationJob) // fired on successful completion (decommission policy lives in caller) testSeams *migSeams // nil in production; tests inject fakes + // R-51: docker restart policies for DOWN members of mixed stacks. Keyed by + // containerName+"|"+state so a transitioned or recreated container re-reads rather than + // answering from a stale entry; pruned every refresh to the live container set. Guarded by mu + // (every read/write happens under refreshStatusLocked's write lock). + restartPolicyCache map[string]string + // execFn replaces execCommand's process boundary in tests; nil in production. + execFn func(name string, args ...string) (string, error) + // inspectRestartPolicyFn is the docker-inspect seam for the above; nil in production + // (dockerRestartPolicy). Tests inject a scripted lookup and never touch docker. + inspectRestartPolicyFn func(containerName string) (string, error) + // isMountPoint reports whether a path is a live mountpoint; defaults to system.IsMountPoint. // Injectable so the userdata-belt drive-absent gate is testable (a t.TempDir is never a real mount). isMountPoint func(string) bool @@ -306,7 +325,7 @@ func (m *Manager) DeployedStackNames() []string { } // RunningAppStacks returns the names of deployed, NON-protected stacks that currently have -// containers up (running/starting/unhealthy/restarting) — the set the quiesce loop (slice 8B) +// containers up (running/starting/unhealthy/restarting/degraded) — the set the quiesce loop (slice 8B) // stops before an app-consistent backup and restarts after. Protected infra (traefik, cloudflared, // felhom-controller) is excluded so the controller never stops its own tunnel/proxy or itself. // Sorted for deterministic stop/start order. @@ -319,7 +338,9 @@ func (m *Manager) RunningAppStacks() []string { continue } switch stack.State { - case StateRunning, StateStarting, StateUnhealthy, StateRestarting: + // StateDegraded (R-51) belongs here: a degraded stack still has LIVE members, and the + // quiesce loop must stop them before an app-consistent backup and start them after. + case StateRunning, StateStarting, StateUnhealthy, StateRestarting, StateDegraded: names = append(names, name) } } @@ -451,6 +472,7 @@ func (m *Manager) refreshStatusLocked() error { } projectContainers := make(map[string][]ContainerInfo) + liveContainers := make(map[string]bool) totalContainers := 0 for _, line := range strings.Split(strings.TrimSpace(output), "\n") { @@ -469,8 +491,11 @@ func (m *Manager) refreshStatusLocked() error { Status: parts[3], } projectContainers[parts[4]] = append(projectContainers[parts[4]], ci) + liveContainers[ci.Name] = true totalContainers++ } + m.pruneRestartPolicyCacheLocked(liveContainers) + policyOf := m.restartPolicyLookupLocked() // fix-6: refreshStatusLocked runs every 10s (the status-refresh job) — its per-cycle enumeration // lines are TRACE (dropped from the debug ring) so they don't eat the post-incident window. A real @@ -492,7 +517,7 @@ func (m *Manager) refreshStatusLocked() error { } } else { stack.Containers = containers - stack.State = aggregateState(containers) + stack.State = aggregateState(containers, policyOf) } // Re-apply controller-side health probe results: if the last probe @@ -511,6 +536,59 @@ func (m *Manager) refreshStatusLocked() error { return nil } +// dockerRestartPolicy reads one container's configured restart policy via docker inspect. +// Returns ("", err) when the container is gone or the inspect fails — the caller treats that as +// UNKNOWN (see supervisedPolicy). +func (m *Manager) dockerRestartPolicy(containerName string) (string, error) { + if m.inspectRestartPolicyFn != nil { + return m.inspectRestartPolicyFn(containerName) + } + out, err := m.execCommand("docker", "inspect", "-f", "{{.HostConfig.RestartPolicy.Name}}", containerName) + if err != nil { + return "", err + } + return strings.TrimSpace(out), nil +} + +// restartPolicyLookupLocked returns the cached-and-memoizing lookup handed to aggregateState. +// MUST be called with m.mu held for writing (it populates the cache). +func (m *Manager) restartPolicyLookupLocked() restartPolicyLookup { + return func(name string) string { + key := name + "|policy" + if m.restartPolicyCache != nil { + if p, ok := m.restartPolicyCache[key]; ok { + return p + } + } + p, err := m.dockerRestartPolicy(name) + if err != nil { + // UNKNOWN — deliberately not cached, so a transient docker hiccup does not pin a + // container to "unknown" for the rest of the process lifetime. + m.logger.Printf("[WARN] [stacks] restart-policy inspect failed for container %q: %v (treating as supervised)", name, err) + return "" + } + if m.restartPolicyCache == nil { + m.restartPolicyCache = map[string]string{} + } + m.restartPolicyCache[key] = p + if m.isDebug() { + m.logger.Printf("[DEBUG] [stacks] restart-policy of down member %q = %q", name, p) + } + return p + } +} + +// pruneRestartPolicyCacheLocked drops cache entries for containers that no longer exist, so a +// long-lived controller cannot accumulate entries for deleted apps. MUST hold mu for writing. +func (m *Manager) pruneRestartPolicyCacheLocked(live map[string]bool) { + for key := range m.restartPolicyCache { + name := strings.TrimSuffix(key, "|policy") + if !live[name] { + delete(m.restartPolicyCache, key) + } + } +} + // resolveContainerState determines the effective state by combining Docker's // State field (running/exited/etc.) with the Status field that contains health info. // @@ -545,9 +623,35 @@ func resolveContainerState(dockerState, dockerStatus string) ContainerState { } } +// restartPolicyLookup returns a container's docker restart policy name ("always", "unless-stopped", +// "on-failure", "no"). An empty string means UNKNOWN — the inspect failed or no lookup was supplied. +type restartPolicyLookup func(containerName string) string + +// supervisedPolicy reports whether a restart policy means "docker is supposed to keep this container +// running" — i.e. its being Exited is a fault, not a design. +// +// UNKNOWN ("") counts as supervised, deliberately fail-CLOSED, which is the opposite of the +// IsDownState fail-open rule and for a different reason: there the input is an *ambiguous state*, +// here we already KNOW a member is dead and only the excuse is missing. The P2 census (2026-07-21, +// 53 catalog templates / 78 services) found **every** catalog service on `unless-stopped` and zero +// one-shot init/migrate containers, so "unknown" in the field is an inspect failure on a container +// that is almost certainly supervised. Missing a real dead-primary alarm is the failure that cost +// 18 h; a false alarm is a banner. +func supervisedPolicy(policy string) bool { + switch policy { + case "no", "on-failure": + return false + default: // "always", "unless-stopped", "" (unknown) + return true + } +} + // aggregateState determines the overall stack state from its containers. -// Priority: unhealthy/starting > restarting > all-running > stopped -func aggregateState(containers []ContainerInfo) ContainerState { +// Priority: unhealthy/starting > restarting > all-running > degraded > stopped +// +// policyOf is consulted ONLY for the mixed case (some members up, some down) and ONLY for the down +// members — see the mix branch. nil is allowed (every exited member then reads as supervised). +func aggregateState(containers []ContainerInfo, policyOf restartPolicyLookup) ContainerState { if len(containers) == 0 { return StateNotDeployed } @@ -557,6 +661,7 @@ func aggregateState(containers []ContainerInfo) ContainerState { unhealthy := 0 restarting := 0 stopped := 0 + var down []ContainerInfo for _, c := range containers { switch c.State { @@ -570,6 +675,7 @@ func aggregateState(containers []ContainerInfo) ContainerState { restarting++ case StateStopped, StateExited: stopped++ + down = append(down, c) } } @@ -595,8 +701,22 @@ func aggregateState(containers []ContainerInfo) ContainerState { if stopped == total { return StateStopped } - // Mix (some running, some stopped) — report as running (partial) + // Mix (some members up, some down) — R-51. Until v0.156.0 this reported StateRunning + // unconditionally ("partial"), which is why a dead immich-server behind three live helpers was + // invisible to fix-3 for 18 hours. A down member whose restart policy says docker should be + // keeping it up is a FAULT → the whole stack is degraded (and degraded is a down state). A down + // member with policy `no`/`on-failure` is a one-shot init/migrate container that has legitimately + // finished → benign, the stack stays running. if running > 0 { + for _, c := range down { + policy := "" + if policyOf != nil { + policy = policyOf(c.Name) + } + if supervisedPolicy(policy) { + return StateDegraded + } + } return StateRunning } @@ -1043,6 +1163,13 @@ func (m *Manager) composeExecCustomEnv(dir string, env []string, args ...string) } func (m *Manager) execCommand(name string, args ...string) (string, error) { + // execFn is the process-boundary seam (nil in production). It exists so R-51 can be proven + // through the REAL refreshStatusLocked path — docker ps → aggregateState → docker inspect — + // rather than only through an injected aggregation helper, which would prove the helper and + // not the caller (the v0.154.0 / v0.91.0 inert-seam class). + if m.execFn != nil { + return m.execFn(name, args...) + } cmd := exec.Command(name, args...) var stdout, stderr bytes.Buffer diff --git a/controller/internal/web/funcmap.go b/controller/internal/web/funcmap.go index 3b3677a..3a09dc1 100644 --- a/controller/internal/web/funcmap.go +++ b/controller/internal/web/funcmap.go @@ -37,7 +37,9 @@ func getTimezone() *time.Location { // so an unhealthy app with a dead URL isn't mistaken for a merely-degraded-but-reachable one. func routeUnpublished(state stacks.ContainerState) bool { switch state { - case stacks.StateUnhealthy, stacks.StateRestarting: + // StateDegraded (R-51): the dead member is typically the one Traefik routes to, so the public + // URL 404s exactly as it does for an unhealthy container. + case stacks.StateUnhealthy, stacks.StateRestarting, stacks.StateDegraded: return true default: return false @@ -64,6 +66,9 @@ func (s *Server) templateFuncMap() template.FuncMap { case stacks.StateRestarting: // a restart loop is a problem, not progress return "warn" + case stacks.StateDegraded: + // R-51: a supervised member is dead — a genuine failure, not a user action + return "warn" case stacks.StateStopped, stacks.StateExited: return "neutral" case stacks.StatePaused: @@ -84,6 +89,8 @@ func (s *Server) templateFuncMap() template.FuncMap { return "Telepítés..." case stacks.StateUnhealthy: return "Nem egészséges" + case stacks.StateDegraded: + return "Részlegesen leállt" case stacks.StateStopped, stacks.StateExited: return "Leállítva" case stacks.StateRestarting: @@ -102,7 +109,7 @@ func (s *Server) templateFuncMap() template.FuncMap { return "●" case stacks.StateStarting, stacks.StateDeploying: return "◐" - case stacks.StateUnhealthy: + case stacks.StateUnhealthy, stacks.StateDegraded: return "◑" case stacks.StateStopped, stacks.StateExited: return "○" @@ -119,7 +126,7 @@ func (s *Server) templateFuncMap() template.FuncMap { // and is not stopped/exited — used by templates for showing action buttons "isOperational": func(state stacks.ContainerState) bool { switch state { - case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting: + case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting, stacks.StateDegraded: return true default: return false @@ -201,7 +208,9 @@ func (s *Server) templateFuncMap() template.FuncMap { switch state { case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting: return "running" - case stacks.StateStopped, stacks.StateExited, stacks.StatePaused: + case stacks.StateStopped, stacks.StateExited, stacks.StatePaused, stacks.StateDegraded: + // R-51: degraded filters with the stopped set — the customer's question is + // "is it working", and a stack with a dead supervised member is not. return "stopped" default: if deployed { diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index 659f059..7e3ba0d 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -148,7 +148,9 @@ func (s *Server) dashboardHandler(w http.ResponseWriter, r *http.Request) { switch st.State { case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting: running++ - case stacks.StateStopped, stacks.StateExited: + // R-51: degraded counts with stopped — the dashboard counter answers "how many of my apps + // work", and a stack with a dead supervised member does not. + case stacks.StateStopped, stacks.StateExited, stacks.StateDegraded: stopped++ } }