diff --git a/CHANGELOG.md b/CHANGELOG.md index 367bd2e..ba10576 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,104 @@ ## Changelog +### 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 +out by **counting containers**: zero containers meant "the customer stopped it" (leave alone), some +containers meant "something broke" (recover). That inference is wrong in two ways, and both were +silent: + +- a **power cut mid-compose** or an **interrupted deploy** also leaves an app with zero containers — + read as a deliberate stop, so the app simply stayed gone until a human noticed (**R-157 mechanism + B**); +- a **backup that stops an app** to copy it safely, then dies, leaves it stopped with **nothing on + disk** recording that a backup stopped it or that it was owed a restart. + +Neither is a guess the controller should be making, because the one fact that settles it — what the +customer actually asked for — **was written down nowhere**. `app.yaml` recorded that an app was +*installed*; it never recorded whether it was meant to be *running*. + +**Part 1 — desired state, owned by the customer's action.** `AppConfig` gains `desired_state`, a +**tri-state** `""` / `running` / `stopped` (`yaml:"desired_state,omitempty"`), with named constants. +`Manager.SetDesiredState` is the only writer, and its callers are the only places a human's decision +enters the system: the `/api/stacks/{name}/{action}` switch (`start`/`restart`/`update` → running, +`stop` → stopped), `DeployStack`, `UpdateOptionalConfig`'s redeploy branch, and the `.fab` import +adapter. Intent is written **BEFORE** the act, and an action whose intent cannot be recorded is +**REFUSED** — proceeding would recreate the ambiguity being removed. + +**`StartStack`/`StopStack` are deliberately NOT writers.** A census on 2026-08-02 found **14 call +sites, of which exactly 2 are the customer**; the other twelve are machines (quiesce, the backup +volume dump, offbox reconstitution, app export/restore, the storage drive-absent gate, the migration +engine, the boot reconciler itself). Recording intent in the primitive would make a nightly backup +indistinguishable from the customer pressing Stop — the exact confusion this release ends. + +**ABSENT MEANS UNKNOWN, NEVER "running" — the single most important line in the change.** Every +`app.yaml` on every existing box predates the field, so absent is what the whole fleet reads on +upgrade. Treating it as running would start, on the first boot after the upgrade, every app its owner +had deliberately stopped. Where intent is unknown the boot reconciler falls back to the **old +container-count rule, byte-for-byte**, rather than inventing an answer. + +**The boot-orphan decision (`bootrecon.isBootOrphan`), replacing the container-count term:** + +| `desired_state` | containers | result | +|---|---|---| +| `stopped` | any | never an orphan | +| `running` | 0 | **ORPHAN** — the R-157 case, invisible before this release | +| `running` | >0 + down | ORPHAN (unchanged) | +| `running` | >0 + up | not an orphan | +| absent | 0 | not an orphan — **exactly** the pre-v0.189.0 behaviour | +| absent | >0 + down | ORPHAN — **exactly** the pre-v0.189.0 behaviour | + +`Protected` and `Deploying` guards unchanged. A **running-only** startup backfill converges apps that +are deployed AND observed up; `stopped` is **never** backfilled, from any signal — inferring it from +zero containers is the defect itself, so an ambiguous app stays ambiguous and keeps legacy behaviour +until the customer next presses a button. + +**Part 2 — the app-stop crash marker (`backup.AppStopGuard`).** `/appstop-state.json`, +atomic (tmp + **fsync** + rename, 0600), modelled on the quiesce marker and deliberately **its own +file** — same shape, different owner, different lifetime; sharing would give one file two writers. +Written **before** the stop, cleared only after a restart that **succeeded**; a FAILED restart keeps +it so the next startup retries. `Recover()` runs at startup and **completes before** the +boot-reconcile goroutine is launched, so an app the marker explains is not also reported as an +unexplained boot orphan. A corrupt marker is quarantined loudly, never silently skipped. + +**A `defer` is not the mechanism, and the code says so.** Campaign 8 fault 10 established on live +hardware that a SIGKILL runs no deferred function; the marker is what covers the hard crash. Its +test simulates a real abort (an unwind that skips the restart statement) rather than a graceful +return — an earlier version of that test called `Begin` itself and **survived the red-proof that +deleted the production call**, which is exactly the hollowness §10 exists to catch. + +**All three stop-and-restart sites are covered**, with no uncovered sibling to imply the class is +handled: `DumpAppVolumesSafe`, `offbox_reconstitute.go` (all four bring-up paths, via one +`restartStack` closure so the success path cannot silently skip the clear), and `appexport`'s export +— the last through a two-method consumer-side seam so the exporter shares the ONE marker file instead +of opening a second. The reason string lives only in `backup`; the adapter in `main.go` supplies it. + +**Also fixed, and it would have silently eaten this feature: `SaveAppConfig` rebuilt `AppConfig` +field-by-field.** That is the R-100 shape (v0.181.0 shipped with two live instances of it). The +literal named five fields, so the sixth — `desired_state` — would have been **dropped on every save**, +and nine call sites share that path: a customer's Stop would have been erased by the next unrelated +`app.yaml` write. Replaced with copy-and-overlay (`saveCfg := *cfg`), safe by construction. Measured +and documented: `app.yaml` does **not** round-trip keys the struct does not model (the trip goes +through the struct), pinned by `TestSaveAppConfig_UnknownYAMLKeysAreDropped`. + +**Operator visibility (§2.4).** An interrupted operation rides the **existing** `backup_failed` event +type. A new type would need the hub's `allowedEventTypes` + `customerMessages` pair changed — a wire +change, and this release ships **no hub change and no hub version bump**. `Recover()` **returns** its +outcome rather than pushing it through a notifier seam, because it must complete before the boot +reconciler (`main.go:~236`) while the notifier is not constructed until `~307`; a seam wired after the +fact is a seam that never fires. + +**No user-visible string changed** — N/A for UI work. No template, funcmap, notifier-type, event-type, +backup-content, retention, tier or restore change. **No agent coupling; MinAgent unchanged.** + +**Tests: +37 across 5 packages (27/27 packages green).** Red-proofs, each observed FAIL then restored: +B (restore `len(Containers) > 0`), C (treat absent as running — the fleet-wide upgrade regression), +D (drop the up-state guard from the backfill), E (delete the production `Begin` call), H (restore the +field-by-field `SaveAppConfig` literal), §8.2 (move the intent write below the action switch), and +the seam test I — which fails while the commented-out call **is still present as a substring**, the +distinction that made the controller's first version of that test pass its own red-proof in +2026-07-21. + ### CI — the gate entry point runs on every push (2026-08-02, R-168) — NO VERSION BUMP **No version bump, no build, no deploy** — this adds a workflow file only. Stated explicitly so the diff --git a/REUSE.md b/REUSE.md index 35b0bab..10ec8e6 100644 --- a/REUSE.md +++ b/REUSE.md @@ -87,7 +87,9 @@ | `Manager.PersistUnitRedeployConfig` (R-47, v0.153.0) | controller/internal/stacks/deploy.go | `(name, env map[string]string) error` | the PERSIST half of `RedeployFromEnv` — app.yaml + locked fields + in-memory flags, **starts nothing** | **TRAP: the restore paths must use THIS, never `RedeployFromEnv`.** RedeployFromEnv ends in a full `up -d`, which before the replay IS the H4 race. RedeployFromEnv is now literally this + the unchanged up-and-report tail | | `Manager.StartStackServices` (R-47, v0.153.0) | controller/internal/stacks/manager.go | `(name string, services []string) error` | scoped `compose up -d ...` — the DB-only window a dump is replayed in | **REFUSES an empty list** (argument-less `up -d` is a FULL start — the one silent fall-through that would reintroduce the race). No `logPostStartStatus`: the app containers are absent on purpose. Never `RestartStack` here — it is a full up in disguise | | `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 | +| `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 | +| `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 | | `Manager.logPostStartStatus` | controller/internal/stacks/manager.go | `(name, stackDir, env)` | Async post-start verification | compose up exits 0 on crash-loops; this is the detection. Goroutine + 3s, never blocks | diff --git a/controller/README.md b/controller/README.md index 10df1ca..da2b127 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1868,17 +1868,59 @@ invariant changes, revisit the suppression. callers rely on stopped counting as down). An out-of-band `docker compose stop` leaves the containers present → `StateExited` → still alerts, which is correct (out-of-band tampering is reportable). -**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). +**Boot desired-state reconciliation (R-52, v0.156.0, `internal/bootrecon`; rebuilt on recorded intent +in R-166, v0.189.0).** 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 and app-stop recoveries, so the three never race) the controller performs **one bounded +sweep**: every deployed, non-protected, not-mid-deploy stack that is down gets `StartStack`, at most +**2 attempts 30 s apart**, then it stops and the alarm owns the problem. Never a restart loop. 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). + +**What "down" means here changed in v0.189.0.** Until then the sweep required the stack to still HAVE +containers, because the UI's Stop is `compose down` (which removes them) and "zero containers" was +read as a deliberate stop. That inference was wrong in two silent ways: a **power cut mid-compose** +and an **interrupted deploy** also leave zero containers, and both were skipped as "the customer +stopped it" and left down indefinitely. Since R-166 the sweep reads the customer's **recorded +intent** (`desired_state` in `app.yaml`) instead: + +| `desired_state` | containers | result | +|---|---|---| +| `stopped` | any | **never** started — the customer said so, and no observation overrides it | +| `running` | 0 | **recovered** — the power-cut / interrupted-deploy case, invisible before v0.189.0 | +| `running` | >0 and down | **recovered** (unchanged) | +| `running` | >0 and up | left alone | +| absent (legacy) | 0 | **not** started — byte-identical to the pre-v0.189.0 behaviour | +| absent (legacy) | >0 and down | **recovered** — byte-identical to the pre-v0.189.0 behaviour | + +**Absent means UNKNOWN, never "running".** Every `app.yaml` written before v0.189.0 lacks the field, +so absent is what an upgraded box reads for every app that has not been started or stopped since; +reading it as "running" would start every deliberately-stopped app on the first boot after the +upgrade. Where intent is unknown the sweep falls back to the old inference rather than inventing an +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. + +**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), +`DeployStack`, `UpdateOptionalConfig`'s redeploy branch, and the `.fab` import. `StartStack` and +`StopStack` are deliberately **not** writers — a census found 14 callers of which only 2 are the +customer, and recording intent in the primitive would make a nightly backup indistinguishable from +the customer pressing Stop, which is the confusion the feature exists to end. Intent is written +**before** the act, and an action whose intent cannot be recorded is **refused**. + +**Interrupted app-data operations (R-166, v0.189.0, `backup.AppStopGuard`).** A volume dump, an +off-site reconstitution and a `.fab` export all stop an app, work on its data, and start it again. +A controller killed inside that window left the app down with nothing on disk recording why or that +it was owed a restart. A persisted marker (`/appstop-state.json` — its **own** file, never +quiesce's, so one file has one writer) is now written **before** the stop and cleared only after a +restart that succeeded; a failed restart deliberately keeps it. At startup `Recover()` restarts the +recorded apps, clears the marker, and its outcome is reported to the operator on the existing +`backup_failed` event — an interrupted operation means the backup did not complete. **The `defer` in +those functions is not the mechanism**: a SIGKILL runs no deferred function (Campaign 8 fault 10, on +live hardware), which is exactly what the marker covers. #### Default Enabled Events diff --git a/controller/cmd/controller/appstop_wiring_test.go b/controller/cmd/controller/appstop_wiring_test.go new file mode 100644 index 0000000..e1533a7 --- /dev/null +++ b/controller/cmd/controller/appstop_wiring_test.go @@ -0,0 +1,168 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) + +// R-166 §10 seam discipline — the recovery and the backfill are seams, and a seam that is never +// called is the defect class this project has shipped four times: a correct component, green unit +// tests that inject it directly, and no production caller. +// +// These walk main.go's AST. NOT strings.Contains — the sibling bootrecon test records the reason at +// first hand: 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. + +// mainBody returns func main()'s body from main.go, parsed. +func mainBody(t *testing.T) *ast.BlockStmt { + t.Helper() + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + for _, decl := range f.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Name.Name == "main" && fn.Body != nil { + return fn.Body + } + } + t.Fatal("func main() not found in main.go") + return nil +} + +// callsInMain returns, in source order, the names of every call in func main() whose function +// expression is `x.Sel(...)` or `Sel(...)` — enough to identify the wiring calls by name. +func callsInMain(t *testing.T, body *ast.BlockStmt) []string { + t.Helper() + var names []string + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + names = append(names, fun.Sel.Name) + case *ast.Ident: + names = append(names, fun.Name) + } + return true + }) + return names +} + +func indexOfCall(names []string, want string) int { + for i, n := range names { + if n == want { + return i + } + } + return -1 +} + +// TestMainWiresAppStopRecovery is the Group-I seam test. Comment out the `appStopGuard.Recover()` +// line in main.go and this fails, where every behavioural test in internal/backup still passes. +func TestMainWiresAppStopRecovery(t *testing.T) { + names := callsInMain(t, mainBody(t)) + + if indexOfCall(names, "NewAppStopGuard") < 0 { + t.Fatal("func main() no longer builds the R-166 app-stop guard — nothing writes or reads the marker") + } + if indexOfCall(names, "SetStarter") < 0 { + t.Fatal("func main() no longer calls SetStarter on the app-stop guard — Recover would find the " + + "marker and be unable to start anything, leaving every interrupted app down") + } + if indexOfCall(names, "Recover") < 0 { + t.Fatal("func main() no longer calls Recover() on the app-stop guard — apps left stopped by an " + + "interrupted backup stay down forever (the R-166 defect, un-fixed)") + } + if indexOfCall(names, "SetAppStopGuard") < 0 { + t.Fatal("func main() no longer hands the recovered guard to the backup manager — the manager " + + "would build a SECOND guard over the same file, i.e. one file with two owners") + } + if indexOfCall(names, "SetStopGuard") < 0 { + t.Fatal("func main() no longer wires the exporter's stop guard — the .fab export path would be " + + "the one uncovered stop-and-restart site, which is how a reader concludes the class is handled") + } +} + +// TestMainWiresDesiredStateBackfill pins the Part-1.5 call. +func TestMainWiresDesiredStateBackfill(t *testing.T) { + if indexOfCall(callsInMain(t, mainBody(t)), "BackfillDesiredState") < 0 { + t.Fatal("func main() no longer calls BackfillDesiredState — every existing app would stay on " + + "legacy inference until someone pressed a button on it") + } +} + +// TestAppStopRecoveryPrecedesTheBootReconciler is §8.4's ORDERING requirement, and it is the reason +// the recovery returns its result instead of pushing it through a notifier seam. +// +// The recovery must COMPLETE — not merely be reached — before `go runBootReconcile(...)` is +// launched. If the boot reconciler ran first it would see an app the marker already explains, list +// it as an unexplained boot orphan, and one fault would be reported as two. +func TestAppStopRecoveryPrecedesTheBootReconciler(t *testing.T) { + names := callsInMain(t, mainBody(t)) + + recover := indexOfCall(names, "Recover") + bootrecon := indexOfCall(names, "runBootReconcile") + backfill := indexOfCall(names, "BackfillDesiredState") + + if recover < 0 || bootrecon < 0 || backfill < 0 { + t.Fatalf("missing a call: Recover=%d runBootReconcile=%d BackfillDesiredState=%d", recover, bootrecon, backfill) + } + if recover >= bootrecon { + t.Fatal("the app-stop Recover no longer runs BEFORE the boot reconciler is launched — an app " + + "the marker explains would also be reported as an unexplained boot orphan (§8.4)") + } + if backfill >= bootrecon { + t.Fatal("the desired-state backfill no longer runs BEFORE the boot reconciler — the reconciler " + + "would decide from intent the backfill had not yet written") + } + if recover >= backfill { + t.Fatal("the backfill no longer runs AFTER the app-stop recovery — an app the recovery just " + + "restarted would still read as down and be left unrecorded") + } +} + +// TestMainReportsTheInterruptedOperation pins §2.4: the recovery's outcome reaches the operator. +// +// The reporting call is deliberately far from the recovery (the notifier does not exist yet at +// recovery time), which is exactly the distance across which a wiring gets dropped. +func TestMainReportsTheInterruptedOperation(t *testing.T) { + body := mainBody(t) + names := callsInMain(t, body) + + if indexOfCall(names, "NotifyBackupFailed") < 0 { + t.Fatal("func main() no longer reports an interrupted app-data operation to the operator — the " + + "controller died mid-backup and nobody is told (§2.4)") + } + // It must be guarded, not unconditional: a box with nothing to recover must not email an operator + // on every single boot. + guarded := false + ast.Inspect(body, func(n ast.Node) bool { + ifst, ok := n.(*ast.IfStmt) + if !ok || ifst.Cond == nil { + return true + } + bin, ok := ifst.Cond.(*ast.BinaryExpr) + if !ok { + return true + } + x, ok := bin.X.(*ast.Ident) + if !ok || x.Name != "appStopRecovery" { + return true + } + for _, name := range callsInMain(t, ifst.Body) { + if name == "NotifyBackupFailed" { + guarded = true + } + } + return true + }) + if !guarded { + t.Fatal("the interrupted-operation alert is not guarded by `if appStopRecovery != nil` — every " + + "healthy boot would page the operator about a backup that was never interrupted") + } +} diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index b59d864..e1431d6 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -227,6 +227,31 @@ func main() { // Recover FIRST (restart any stacks left stopped by a crash mid-quiesce), then start the loop. quiesceLoop := startQuiesceLoop(ctx, cfg, sett, stackMgr, logger) + // --- R-166: recover apps left stopped by an interrupted app-data operation --- + // A volume dump, an offsite reconstitution or a `.fab` export stops the app, works on its data, + // and starts it again. A controller killed inside that window used to leave the app down with + // NOTHING on disk explaining it — and a stopped app has zero containers, which the boot + // reconciler below read as a deliberate customer stop and left alone, indefinitely. + // + // ORDERING IS LOAD-BEARING (§8.4) and this call must COMPLETE, not merely be reached, before the + // boot-reconcile goroutine is launched: an app the marker already explains must not also be + // reported as an unexplained boot orphan. Same position and same reason as the quiesce Recover + // immediately above. + // The guard is built HERE rather than taken from the backup manager because that manager is not + // constructed until ~40 lines below — and moving its construction up to suit this would be a far + // wider change than moving one object down. It is handed to the manager (SetAppStopGuard) and to + // the exporter later, so all three share ONE guard over ONE file. + appStopGuard := backup.NewAppStopGuard(filepath.Join(cfg.Paths.DataDir, "appstop-state.json"), logger) + appStopGuard.SetStarter(stackMgr) + appStopRecovery := appStopGuard.Recover() + + // --- R-166: desired-state backfill (running-only) --- + // Converge the apps whose intent is unambiguous — deployed and observed UP — so the fleet stops + // depending on legacy inference without waiting for a button press. NEVER backfills "stopped": + // zero containers cannot distinguish a deliberate stop from a power cut, and that inference is + // the defect. Runs after the two recoveries so a just-restarted app is counted as running. + stackMgr.BackfillDesiredState() + // --- 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 @@ -277,6 +302,9 @@ func main() { } if cfg.Backup.Enabled { backupMgr = backup.NewManager(cfg, sett, logger) + // R-166: use the guard that already ran Recover at startup, not a second one over the same + // file (see SetAppStopGuard — one file, one owner). + backupMgr.SetAppStopGuard(appStopGuard) backupMgr.SetStackProvider(stackProv) backupMgr.SetVersion(Version) // O4: restore-from-unit generates a replacement for an unrecoverable RESETTABLE secret @@ -314,6 +342,19 @@ func main() { quiesceLoop.SetTierNotifier(quiesceTierNotifier{n: notifier}) } + // R-166 §2.4: report an interrupted app-data operation to the operator, HERE, because the + // recovery itself had to run before the boot reconciler (line ~236) and the notifier does not + // exist until this line. An interrupted operation means the controller died mid-backup and that + // backup did not complete — operator-grade news even when every app came back. + // + // It rides the EXISTING `backup_failed` event type rather than a new one: a new type needs the + // hub's allowedEventTypes + customerMessages pair changed, which is a wire change, and this + // release ships no hub change. A controller emitting an unlisted type gets a flat 400 from + // POST /event. Reachability is covered by TestAppStopRecoveryIsWired. + if appStopRecovery != nil { + notifier.NotifyBackupFailed(appStopRecovery.Message(), appStopRecovery.Detail()) + } + // --- Initialize the app-email SMTP shim (mailrelay) --- // In-process shim: apps → shim → hub → Resend (the Resend key stays hub-side). It runs only // when the controller has a hub (URL+key) AND the operational kill-switch is on; the runtime @@ -975,6 +1016,11 @@ func main() { exportProv := &exportAdapter{mgr: stackMgr, encKey: encKey} appExporter := appexport.NewExporter(exportProv, logger, Version) appExporter.SetDebug(cfg.Logging.Level == "debug") + // R-166: the exporter stops apps too (export with "stop the app first"), so it shares the backup + // manager's ONE marker file rather than opening a second one — one file, one recovery. Without + // this the export path would be the uncovered sibling of two covered ones, which is how a reader + // concludes the whole class is handled (§2.2). + appExporter.SetStopGuard(exportStopGuard{g: appStopGuard}) apiRouter.SetDebug(cfg.Logging.Level == "debug") // --- Initialize web server --- @@ -1738,6 +1784,17 @@ func (a *exportAdapter) GetStacksBaseDir() string { return a.mgr.GetStacksBaseDir() } +// exportStopGuard adapts *backup.AppStopGuard to the exporter's reason-free seam (R-166). The reason +// is supplied HERE rather than passed in, so backup.ReasonAppExport's value exists in exactly one +// place and the two packages cannot drift apart. +type exportStopGuard struct{ g *backup.AppStopGuard } + +func (a exportStopGuard) Begin(opID string, stacks []string) error { + return a.g.Begin(opID, backup.ReasonAppExport, stacks) +} + +func (a exportStopGuard) End() { a.g.End() } + func (a *exportAdapter) SaveEncryptedAppConfig(stackDir string, env map[string]string) error { meta := stacks.LoadMetadata(stackDir) sensitiveVars := stacks.SensitiveEnvVars(&meta) @@ -1745,6 +1802,12 @@ func (a *exportAdapter) SaveEncryptedAppConfig(stackDir string, env map[string]s Deployed: true, DeployedAt: time.Now().Format(time.RFC3339), Env: env, + // R-166 — a CUSTOMER-INTENT POINT, and the one that is not the API action switch. Importing + // a `.fab` bundle is the customer installing that app on this box, and the import path starts + // it (appexport/restore.go). Without this the app would come back from a restore with NO + // recorded intent and fall to legacy boot behaviour — meaning a power cut days later would + // strand it, which is exactly the failure this release exists to remove. + DesiredState: stacks.DesiredStateRunning, } return stacks.SaveAppConfig(stackDir, cfg, a.encKey, sensitiveVars) } diff --git a/controller/internal/api/desiredstate_intent_test.go b/controller/internal/api/desiredstate_intent_test.go new file mode 100644 index 0000000..efdd5f3 --- /dev/null +++ b/controller/internal/api/desiredstate_intent_test.go @@ -0,0 +1,149 @@ +package api + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-166 Part 1.3 — THE CUSTOMER-INTENT POINT. +// +// `stackMgr` is a concrete *stacks.Manager, so actionStack cannot be driven with a fake without +// Docker. The two properties that actually carry the correctness are therefore pinned the only way +// they can be: the mapping is a pure function with its own table test, and the ORDER (§8.2) is +// asserted structurally over actionStack's AST. Both fail if someone reverses the write and the act, +// which is the mistake that would undo a customer's Stop at the next boot. + +func TestDesiredStateForAction_MapsEveryAction(t *testing.T) { + cases := []struct { + action string + want string + ok bool + }{ + {"start", stacks.DesiredStateRunning, true}, + // restart and update both END in `compose up -d`, so a customer who presses either is asking + // for the app to be up afterwards. + {"restart", stacks.DesiredStateRunning, true}, + {"update", stacks.DesiredStateRunning, true}, + {"stop", stacks.DesiredStateStopped, true}, + // Anything unrecognised records NOTHING rather than guessing — a future action must not + // silently acquire an intent it was never meant to carry. + {"", "", false}, + {"delete", "", false}, + {"pause", "", false}, + } + for _, tc := range cases { + got, ok := desiredStateForAction(tc.action) + if got != tc.want || ok != tc.ok { + t.Fatalf("desiredStateForAction(%q) = (%q, %v), want (%q, %v)", tc.action, got, ok, tc.want, tc.ok) + } + } +} + +func TestDesiredStateForAction_NeverRecordsStoppedForANonStop(t *testing.T) { + // The asymmetry that matters: writing "stopped" for anything other than a Stop would permanently + // disable auto-recovery for an app nobody stopped. + for _, a := range []string{"start", "restart", "update", "deploy", "delete", ""} { + if got, _ := desiredStateForAction(a); got == stacks.DesiredStateStopped { + t.Fatalf("action %q maps to desired_state=stopped", a) + } + } +} + +// TestActionStack_RecordsIntentBeforeActing is §8.2, asserted structurally. +// +// If the SetDesiredState call moved BELOW the action switch, a stop could remove every container +// while app.yaml still recorded `running` — and the boot reconciler would then start an app the +// customer had just deliberately stopped. That is the single worst outcome available in Part 1, and +// no behavioural test in this package can reach it without a Docker daemon. +func TestActionStack_RecordsIntentBeforeActing(t *testing.T) { + body := funcBody(t, "actionStack") + + setPos, switchPos := -1, -1 + ast.Inspect(body, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CallExpr: + if sel, ok := node.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "SetDesiredState" && setPos < 0 { + setPos = int(node.Pos()) + } + case *ast.SwitchStmt: + // The action switch is the one whose tag is the `action` identifier. + if id, ok := node.Tag.(*ast.Ident); ok && id.Name == "action" && switchPos < 0 { + switchPos = int(node.Pos()) + } + } + return true + }) + + if setPos < 0 { + t.Fatal("actionStack no longer calls SetDesiredState — the customer's start/stop decision is " + + "recorded nowhere, which is the R-166 defect un-fixed") + } + if switchPos < 0 { + t.Fatal("actionStack no longer has a `switch action` — this test needs updating") + } + if setPos >= switchPos { + t.Fatal("actionStack records the desired state AFTER performing the action (§8.2 violated): a " + + "stop whose intent write fails or lands late leaves zero containers with `running` " + + "recorded, and the boot reconciler would restart an app the customer just stopped") + } +} + +// TestActionStack_RefusesTheActionWhenIntentCannotBeRecorded pins the other half of §8.2: a failed +// write REFUSES the act. Proceeding anyway would perform a stop that nothing records — exactly the +// ambiguity this release removes. +func TestActionStack_RefusesTheActionWhenIntentCannotBeRecorded(t *testing.T) { + body := funcBody(t, "actionStack") + + refuses := false + ast.Inspect(body, func(n ast.Node) bool { + ifst, ok := n.(*ast.IfStmt) + if !ok || ifst.Init == nil { + return true + } + // Look for `if derr := ...SetDesiredState(...); derr != nil { ... return }` + assign, ok := ifst.Init.(*ast.AssignStmt) + if !ok || len(assign.Rhs) != 1 { + return true + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "SetDesiredState" { + return true + } + for _, stmt := range ifst.Body.List { + if _, isReturn := stmt.(*ast.ReturnStmt); isReturn { + refuses = true + } + } + return true + }) + + if !refuses { + t.Fatal("actionStack does not RETURN when SetDesiredState fails — it would go on to stop or " + + "start an app whose intent could not be recorded (§8.2)") + } +} + +// funcBody parses router.go and returns the named method's body. +func funcBody(t *testing.T, name string) *ast.BlockStmt { + t.Helper() + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "router.go", nil, 0) + if err != nil { + t.Fatalf("parse router.go: %v", err) + } + for _, decl := range f.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Name.Name == name && fn.Body != nil { + return fn.Body + } + } + t.Fatalf("func %s not found in router.go", name) + return nil +} diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index c5e4531..d5e9ed0 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -534,6 +534,24 @@ func (r *Router) startGatedByMissingDrive(name string) (bool, string) { return false, "" } +// desiredStateForAction maps a stack action to the customer intent it expresses, or (_, false) for +// an action that expresses none. Pure, so the §8.1/§1.3 mapping is testable without a Manager. +// +// `restart` and `update` both mean running: a customer who updates or restarts an app is asking for +// it to be up afterwards, and both end in `compose up -d`. Anything not listed here — an unknown +// action string — records nothing rather than guessing, so a future action cannot silently acquire +// an intent it was never meant to carry. +func desiredStateForAction(action string) (string, bool) { + switch action { + case "start", "restart", "update": + return stacks.DesiredStateRunning, true + case "stop": + return stacks.DesiredStateStopped, true + default: + return "", false + } +} + func (r *Router) actionStack(w http.ResponseWriter, action, name string) { r.logger.Printf("[INFO] [api] %s requested for stack: %s", action, name) r.dbg("actionStack: action=%s name=%s", action, name) @@ -579,6 +597,26 @@ func (r *Router) actionStack(w http.ResponseWriter, action, name string) { } } + // R-166: THE CUSTOMER-INTENT POINT. This switch is where a human's decision about whether their + // app should be running enters the system, and until v0.189.0 that decision was recorded nowhere + // — so the box had to infer it from container counts, and inferred wrong for a power cut and for + // an interrupted backup alike. + // + // Written BEFORE the act (§8.2) and a failed write REFUSES the act: performing a stop whose + // intent could not be recorded would recreate exactly the ambiguity this closes. Both gates that + // can legitimately refuse an action (protected-stack, drive-absent, memory) have already run + // above, so nothing is recorded for an action that was never going to happen. + if desired, ok := desiredStateForAction(action); ok { + if derr := r.stackMgr.SetDesiredState(name, desired); derr != nil { + r.logger.Printf("[ERROR] [api] %s for %s refused: could not record desired state: %v", action, name, derr) + writeJSON(w, http.StatusInternalServerError, apiResponse{ + OK: false, + Error: "A művelet nem hajtható végre: az alkalmazás beállításai nem menthetők.", + }) + return + } + } + var err error switch action { case "start": diff --git a/controller/internal/appexport/export.go b/controller/internal/appexport/export.go index 165779b..dafe0fc 100644 --- a/controller/internal/appexport/export.go +++ b/controller/internal/appexport/export.go @@ -96,10 +96,26 @@ type Exporter struct { // computation walks. Nil → the real os.ReadDir-based lister. dirLister func(dir string) []string + // stopGuard (R-166) marks the stop→export→start window so a controller killed inside it leaves a + // durable record that the app is owed a restart. Declared consumer-side as a two-method interface + // so this package does not import internal/backup; main.go passes the backup manager's guard, so + // BOTH packages write ONE marker file — an exporter with its own file would be a second writer + // racing the same recovery. Nil = not wired (tests): the export runs exactly as it did before. + stopGuard appStopGuard + mu sync.Mutex activeJob *Job } +// appStopGuard is the app-stop crash-marker seam. The REASON is deliberately not a parameter: it is +// always "app export" from here, and the adapter in main.go supplies it. Passing it as a string +// would duplicate backup.ReasonAppExport's value in a second package with nothing keeping the two in +// step — a drift this codebase has paid for before (the offbox key that was guessed, R-7b). +type appStopGuard interface { + Begin(opID string, stacks []string) error + End() +} + // NewExporter creates a new export/import engine. func NewExporter(provider ExportStackProvider, logger *log.Logger, version string) *Exporter { return &Exporter{ @@ -109,6 +125,18 @@ func NewExporter(provider ExportStackProvider, logger *log.Logger, version strin } } +// SetStopGuard wires the app-stop crash marker. INIT-ONLY — call once at startup, before any export. +func (e *Exporter) SetStopGuard(g appStopGuard) { e.stopGuard = g } + +// stopGuardBegin records the app-stop marker before an export stops an app. An unwired guard is a +// no-op (pre-v0.189.0 behaviour), never an error — a test exporter must not be forced to have one. +func (e *Exporter) stopGuardBegin(stackName string) error { + if e.stopGuard == nil { + return nil + } + return e.stopGuard.Begin("app-export:"+stackName, []string{stackName}) +} + // SetDebug enables or disables verbose debug logging. func (e *Exporter) SetDebug(debug bool) { e.debug = debug @@ -226,6 +254,14 @@ func (e *Exporter) executeExport(req ExportRequest, job *Job) { // Optionally stop the app wasRunning := false if req.StopApp && e.provider.IsStackRunning(req.StackName) { + // R-166: mark BEFORE the stop. The defer below covers the graceful exits; it does NOT cover a + // SIGKILL or a power cut, which run no deferred function (Campaign 8 fault 10, on live + // hardware) — only this marker does, and a big export is a long window to be killed in. + if err := e.stopGuardBegin(req.StackName); err != nil { + e.failJob(job, step, "Az alkalmazás leállítása előtti jelölő nem menthető — az exportálás nem indult el.") + e.logger.Printf("[ERROR] Export: could not record the app-stop marker for %s (refusing to stop it unprotected): %v", req.StackName, err) + return + } wasRunning = true e.logger.Printf("[INFO] Export: stopping %s", req.StackName) e.debugf("stopping stack %s before export", req.StackName) @@ -246,6 +282,11 @@ func (e *Exporter) executeExport(req ExportRequest, job *Job) { e.logger.Printf("[WARN] Export: could not restart %s: %v", req.StackName, err) } else { e.debugf("stack %s restarted successfully", req.StackName) + // Cleared only on a restart that succeeded — a failed one keeps the marker so the + // next startup retries. + if e.stopGuard != nil { + e.stopGuard.End() + } } }() } diff --git a/controller/internal/backup/appstop_marker.go b/controller/internal/backup/appstop_marker.go new file mode 100644 index 0000000..5d9a043 --- /dev/null +++ b/controller/internal/backup/appstop_marker.go @@ -0,0 +1,281 @@ +package backup + +import ( + "encoding/json" + "fmt" + "log" + "os" + "path/filepath" + "sort" + "time" +) + +// ── The app-stop marker (R-166 part 2, decision D-b "in-flight operations") ─────────────────────── +// +// Several operations stop a customer's app, do something to its data, and start it again. Between +// the stop and the start, NOTHING ON DISK RECORDED THAT AN APP WAS OWED A RESTART. A controller that +// died in that window left the app down with no explanation anywhere — and because a stopped app has +// zero containers, the boot reconciler read it as a deliberate customer stop and deliberately left +// it alone. Silently, indefinitely. +// +// A `defer` is NOT the fix and must never be described as one. Campaign 8 fault 10 established this +// on live hardware: a SIGKILL runs no deferred function, and what brought the quiesce loop's stacks +// back was its persisted marker read by Recover() one second after restart. The defer covers the +// graceful exits; the marker covers the hard crash and the power cut. This file is that marker for +// the app-data path, modelled directly on internal/quiesce's. +// +// WHY ITS OWN FILE, not quiesce's: one file, one writer. Quiesce's marker records a whole-guest +// backup window and is written by the quiesce loop; this one records an app-data operation and is +// written by the backup manager and the exporter. Sharing the file would give it two writers with +// two lifetimes, and one clearing the other's record is a stranded app by a different route. +// +// SAFETY (D-b's binding rule): losing this file must never be worse than not having it. A lost or +// corrupt marker means the app is not auto-restarted by THIS mechanism — which is precisely the +// pre-v0.189.0 position, not a new hazard. It never deletes, restores, or touches a backup artifact. + +// AppStopReason names WHY an app was stopped, so the recovery log tells an operator which operation +// was interrupted rather than merely that something was. +type AppStopReason string + +const ( + // ReasonVolumeDump — DumpAppVolumesSafe: stop, tar the volumes consistently, start. + ReasonVolumeDump AppStopReason = "volume_dump" + // ReasonOffboxReconstitute — a full offsite restore overwriting the app's files. + ReasonOffboxReconstitute AppStopReason = "offbox_reconstitute" + // ReasonAppExport — a .fab export taken with "stop the app first". + ReasonAppExport AppStopReason = "app_export" +) + +// humanReason is the operator-facing phrasing for each reason. +func (r AppStopReason) humanReason() string { + switch r { + case ReasonVolumeDump: + return "an app-data backup (volume dump)" + case ReasonOffboxReconstitute: + return "an off-site restore" + case ReasonAppExport: + return "an app export" + default: + return string(r) + } +} + +// AppStopMarker is the persisted "these apps were stopped by an operation that has not reported +// finishing — they are owed a restart" note. +type AppStopMarker struct { + Active bool `json:"active"` + OpID string `json:"op_id"` + Reason AppStopReason `json:"reason"` + Stacks []string `json:"stacks"` + StartedAt time.Time `json:"started_at"` +} + +// AppStopStarter is the one thing recovery needs: the ability to start a stack. StartStack must be +// idempotent (it is — `compose up -d` on a running stack is a no-op). +type AppStopStarter interface { + StartStack(name string) error +} + +// AppStopGuard owns one marker file. Construct with NewAppStopGuard; the zero value is inert (every +// method is a no-op on a nil guard), so a caller that was never wired degrades to pre-v0.189.0 +// behaviour instead of panicking. +type AppStopGuard struct { + path string + logger *log.Logger + now func() time.Time + // starter is only needed by Recover; Begin/End work without one. + starter AppStopStarter +} + +// AppStopRecovery is what Recover found and did. Returned rather than pushed through a notifier +// seam, because of a hard ordering constraint: Recover must COMPLETE before the boot reconciler is +// launched (§8.4, main.go:236) and the hub notifier is not constructed until main.go:307. A seam +// wired after the fact would be a seam that never fires — the "built but never wired" shape this +// project has now hit four times. Returning the outcome lets main.go report it the moment the +// notifier exists, and makes the reporting decision visible at the call site instead of buried here. +type AppStopRecovery struct { + Reason AppStopReason + OpID string + StartedAt time.Time + Restarted []string // apps started again by this recovery + Failed []string // apps that could NOT be restarted (the marker was kept for these) +} + +// Message is the operator-facing headline for an interrupted operation. +func (r *AppStopRecovery) Message() string { + if r == nil { + return "" + } + if len(r.Failed) > 0 { + return fmt.Sprintf("%s was interrupted by a controller restart and %d of %d app(s) could NOT be restarted", + r.Reason.humanReason(), len(r.Failed), len(r.Restarted)+len(r.Failed)) + } + return fmt.Sprintf("%s was interrupted by a controller restart — %d app(s) were left stopped and have been restarted", + r.Reason.humanReason(), len(r.Restarted)) +} + +// Detail is the machine-readable tail. App/stack NAMES only — never env values (§9.5). +func (r *AppStopRecovery) Detail() string { + if r == nil { + return "" + } + d := fmt.Sprintf("op=%s reason=%s started_at=%s restarted=%v", r.OpID, r.Reason, + r.StartedAt.UTC().Format(time.RFC3339), r.Restarted) + if len(r.Failed) > 0 { + d += fmt.Sprintf(" restart_failed=%v", r.Failed) + } + return d +} + +// NewAppStopGuard builds a guard over the given marker path. +func NewAppStopGuard(path string, logger *log.Logger) *AppStopGuard { + if logger == nil { + logger = log.Default() + } + return &AppStopGuard{path: path, logger: logger, now: time.Now} +} + +// SetStarter wires the stack-start seam used by Recover. INIT-ONLY — call once at startup, before +// Recover. Separate from the constructor because the guard is built alongside the backup manager, +// which learns its stack provider later (the same shape as SetStackProvider). +func (g *AppStopGuard) SetStarter(s AppStopStarter) { + if g == nil { + return + } + g.starter = s +} + +// Begin records that `stacks` are about to be stopped by `reason`. It MUST be called BEFORE the +// first stop — an error here means the marker could not be written, and the caller must not proceed +// to stop an app it cannot promise to restart. +func (g *AppStopGuard) Begin(opID string, reason AppStopReason, stackNames []string) error { + if g == nil || g.path == "" { + return nil // not wired — pre-v0.189.0 behaviour, never a hard failure + } + if len(stackNames) == 0 { + return nil + } + return g.write(AppStopMarker{ + Active: true, + OpID: opID, + Reason: reason, + Stacks: append([]string(nil), stackNames...), + StartedAt: g.now(), + }) +} + +// End clears the marker after a successful restart. Best-effort by contract: a failure to clear is +// logged, never returned as the operation's error — a stale marker costs one idempotent StartStack +// on the next boot, which is exactly D-b's "worst acceptable outcome" and far cheaper than failing +// a backup that actually succeeded. +func (g *AppStopGuard) End() { + if g == nil || g.path == "" { + return + } + if err := os.Remove(g.path); err != nil && !os.IsNotExist(err) { + g.logger.Printf("[ERROR] [appstop] could not clear the app-stop marker at %s: %v (a stale marker costs one idempotent restart at next startup)", g.path, err) + } +} + +// Recover restarts any apps left stopped by an operation that died before restarting them, then +// clears the marker. Call ONCE at startup, and — critically — call it to COMPLETION before the boot +// reconciler is launched, so an app this marker explains is not also reported as an unexplained boot +// orphan (§8.4). +// +// Idempotent: StartStack on a running stack is tolerated, and an absent or inactive marker is a +// no-op. On a restart FAILURE the marker is deliberately LEFT IN PLACE — the next startup retries, +// and in the meantime the app is down with desired_state:running, so the boot reconciler sees it as +// an orphan and the dead-app alarm owns it. Clearing a marker whose restart failed would erase the +// only durable record that an app is owed one. +// +// Returns nil when there was nothing to recover — so "no interrupted operation" and "the recovery +// never ran" are distinguishable to the caller, not only in a log (standing rule 3). +func (g *AppStopGuard) Recover() *AppStopRecovery { + if g == nil || g.path == "" { + return nil + } + m, ok := g.read() + if !ok || !m.Active || len(m.Stacks) == 0 { + return nil + } + if g.starter == nil { + g.logger.Printf("[ERROR] [appstop] crash recovery: %d app(s) were stopped by %s and are owed a restart, but no stack starter is wired — leaving the marker for the next startup: %v", + len(m.Stacks), m.Reason.humanReason(), m.Stacks) + return nil + } + + g.logger.Printf("[WARN] [appstop] crash recovery: %s (op %q) was interrupted and left %d app(s) stopped — restarting them: %v", + m.Reason.humanReason(), m.OpID, len(m.Stacks), m.Stacks) + + res := &AppStopRecovery{Reason: m.Reason, OpID: m.OpID, StartedAt: m.StartedAt} + for _, name := range m.Stacks { + if err := g.starter.StartStack(name); err != nil { + g.logger.Printf("[ERROR] [appstop] crash recovery: restart %s failed: %v", name, err) + res.Failed = append(res.Failed, name) + continue + } + g.logger.Printf("[INFO] [appstop] crash recovery: restarted %s after the interrupted %s", name, m.Reason.humanReason()) + res.Restarted = append(res.Restarted, name) + } + sort.Strings(res.Failed) + sort.Strings(res.Restarted) + + if len(res.Failed) > 0 { + g.logger.Printf("[ERROR] [appstop] crash recovery: %d app(s) could not be restarted — KEEPING the marker so the next startup retries; the dead-app alarm owns them meanwhile: %v", + len(res.Failed), res.Failed) + return res + } + g.End() + return res +} + +// ---- marker persistence (atomic, 0600) — the quiesce shape ------------------------------------ + +func (g *AppStopGuard) write(m AppStopMarker) error { + data, err := json.MarshalIndent(m, "", " ") + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(g.path), 0o755); err != nil { + return err + } + tmp := g.path + ".tmp" + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + os.Remove(tmp) + return err + } + // fsync before rename: the whole point is surviving a power cut, and a rename that lands ahead + // of the bytes it points at is a marker that reads as corrupt at exactly the wrong moment. + if err := f.Sync(); err != nil { + f.Close() + os.Remove(tmp) + return err + } + if err := f.Close(); err != nil { + os.Remove(tmp) + return err + } + return os.Rename(tmp, g.path) +} + +func (g *AppStopGuard) read() (AppStopMarker, bool) { + data, err := os.ReadFile(g.path) + if err != nil { + return AppStopMarker{}, false + } + var m AppStopMarker + if err := json.Unmarshal(data, &m); err != nil { + // Never a silent skip (§9.4): a corrupt marker is LOUD and the bad file is quarantined, so a + // genuinely interrupted operation leaves a trace instead of vanishing. Still returns false — + // "no usable marker ⇒ no recovery" is the correct contract, and matches quiesce's. + g.logger.Printf("[WARN] [appstop] the app-stop marker at %s is corrupt (%v) — quarantining; apps are NOT auto-restarted from it", g.path, err) + _ = os.Rename(g.path, fmt.Sprintf("%s.corrupt-%d", g.path, g.now().Unix())) + return AppStopMarker{}, false + } + return m, true +} diff --git a/controller/internal/backup/appstop_marker_test.go b/controller/internal/backup/appstop_marker_test.go new file mode 100644 index 0000000..cd56143 --- /dev/null +++ b/controller/internal/backup/appstop_marker_test.go @@ -0,0 +1,395 @@ +package backup + +import ( + "encoding/json" + "errors" + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-166 part 2 — the app-stop crash marker. +// +// THE DISCIPLINE THAT MATTERS HERE (§10): a `defer` is not crash-safety, so a test that lets the +// deferred cleanup run proves nothing about a crash. Every "interrupted" test below simulates a +// SIGKILL by never reaching the restart — the marker is written, the process conceptually dies, and +// a FRESH guard over the SAME file does the recovering. That is exactly what Campaign 8 fault 10 +// established on live hardware: a SIGKILL runs no deferred function, and what brought the stacks +// back was the marker read at startup. + +type fakeStarter struct { + starts []string + failWith map[string]error +} + +func (f *fakeStarter) StartStack(name string) error { + f.starts = append(f.starts, name) + if err := f.failWith[name]; err != nil { + return err + } + return nil +} + +func newGuard(t *testing.T, dir string) (*AppStopGuard, *fakeStarter) { + t.Helper() + s := &fakeStarter{} + g := NewAppStopGuard(filepath.Join(dir, "appstop-state.json"), log.New(io.Discard, "", 0)) + g.SetStarter(s) + return g, s +} + +func markerPath(dir string) string { return filepath.Join(dir, "appstop-state.json") } + +func markerExists(t *testing.T, dir string) bool { + t.Helper() + _, err := os.Stat(markerPath(dir)) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + return err == nil +} + +// --- Scenario E — a crash mid-backup brings the app back ----------------------------------------- + +func TestRecover_InterruptedVolumeDump_RestartsTheAppAndClearsTheMarker(t *testing.T) { + dir := t.TempDir() + + // --- process 1: an operation stops the app and is KILLED. No End(), no defer, no cleanup. --- + g1, _ := newGuard(t, dir) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatalf("Begin: %v", err) + } + if !markerExists(t, dir) { + t.Fatal("Begin did not write a marker — nothing would survive the kill") + } + // — g1 is abandoned deliberately; nothing else is called on it. + + // --- process 2: a fresh controller starts and recovers from the file alone. --- + g2, starter := newGuard(t, dir) + res := g2.Recover() + + if len(starter.starts) != 1 || starter.starts[0] != "immich" { + t.Fatalf("started %v, want exactly [immich] — the app was left stranded by the interrupted backup", starter.starts) + } + if res == nil || len(res.Restarted) != 1 || res.Restarted[0] != "immich" { + t.Fatalf("recovery result = %+v, want immich restarted", res) + } + if res.Reason != ReasonVolumeDump { + t.Fatalf("reason = %q, want %q — the operator must be told WHICH operation was interrupted", res.Reason, ReasonVolumeDump) + } + if markerExists(t, dir) { + t.Fatal("the marker survived a successful recovery — the next boot would restart the app again") + } + // The operator-facing text must name the interruption, not merely report a restart. + if msg := res.Message(); msg == "" || !strings.Contains(msg, "interrupted") { + t.Fatalf("operator message %q does not say the operation was interrupted", msg) + } +} + +func TestRecover_NoMarker_IsASilentNoOp(t *testing.T) { + dir := t.TempDir() + g, starter := newGuard(t, dir) + if res := g.Recover(); res != nil { + t.Fatalf("Recover reported %+v on a box with no marker", res) + } + if len(starter.starts) != 0 { + t.Fatalf("started %v with no marker present", starter.starts) + } +} + +func TestRecover_FailedRestart_KEEPSTheMarkerForTheNextStartup(t *testing.T) { + // The single most important failure behaviour: clearing a marker whose restart failed would + // erase the only durable record that an app is owed one. The app is genuinely still down. + dir := t.TempDir() + g1, _ := newGuard(t, dir) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich", "nextcloud"}); err != nil { + t.Fatal(err) + } + + g2, starter := newGuard(t, dir) + starter.failWith = map[string]error{"immich": errors.New("compose up: no such image")} + res := g2.Recover() + + if len(res.Failed) != 1 || res.Failed[0] != "immich" { + t.Fatalf("failed=%v, want [immich]", res.Failed) + } + if len(res.Restarted) != 1 || res.Restarted[0] != "nextcloud" { + t.Fatalf("restarted=%v, want [nextcloud] — one app failing must not abort the others", res.Restarted) + } + if !markerExists(t, dir) { + t.Fatal("the marker was cleared even though a restart FAILED — the next startup would not retry") + } + if msg := res.Message(); !strings.Contains(msg, "NOT be restarted") { + t.Fatalf("operator message %q does not report the failure", msg) + } + if d := res.Detail(); !strings.Contains(d, "restart_failed") || !strings.Contains(d, "immich") { + t.Fatalf("detail %q does not name which app failed", d) + } +} + +func TestRecover_IsIdempotentAcrossRepeatedStartups(t *testing.T) { + dir := t.TempDir() + g1, _ := newGuard(t, dir) + if err := g1.Begin("op", ReasonOffboxReconstitute, []string{"immich"}); err != nil { + t.Fatal(err) + } + g2, s2 := newGuard(t, dir) + g2.Recover() + g3, s3 := newGuard(t, dir) + g3.Recover() + + if len(s2.starts) != 1 { + t.Fatalf("first recovery started %v", s2.starts) + } + if len(s3.starts) != 0 { + t.Fatalf("a SECOND startup restarted %v again — the marker was not cleared", s3.starts) + } +} + +func TestRecover_CorruptMarkerIsQuarantinedNotSilentlySkipped(t *testing.T) { + // §9.4: never a silent skip. A corrupt marker cannot be acted on, but it must leave a trace — + // otherwise a genuinely interrupted operation vanishes without evidence. + dir := t.TempDir() + if err := os.WriteFile(markerPath(dir), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + g, starter := newGuard(t, dir) + if res := g.Recover(); res != nil { + t.Fatalf("a corrupt marker produced a recovery result %+v", res) + } + if len(starter.starts) != 0 { + t.Fatalf("apps were started from a corrupt marker: %v", starter.starts) + } + if markerExists(t, dir) { + t.Fatal("the corrupt marker was left in place — it would be re-read forever") + } + quarantined, _ := filepath.Glob(markerPath(dir) + ".corrupt-*") + if len(quarantined) != 1 { + t.Fatalf("the corrupt marker was not quarantined (found %d) — it was silently dropped", len(quarantined)) + } +} + +func TestRecover_NoStarterWiredKeepsTheMarker(t *testing.T) { + // D-b's safety rule: never worse than not having the file. With no starter the guard cannot act, + // so it must keep the record for a startup that can, rather than clear it and lose the app. + dir := t.TempDir() + g1, _ := newGuard(t, dir) + if err := g1.Begin("op", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatal(err) + } + g2 := NewAppStopGuard(markerPath(dir), log.New(io.Discard, "", 0)) // deliberately no SetStarter + if res := g2.Recover(); res != nil { + t.Fatalf("recovered without a starter: %+v", res) + } + if !markerExists(t, dir) { + t.Fatal("the marker was cleared with no starter wired — the app would never come back") + } +} + +func TestNilGuardIsInert(t *testing.T) { + // A caller that was never wired must degrade to pre-v0.189.0 behaviour, not panic. + var g *AppStopGuard + if err := g.Begin("op", ReasonVolumeDump, []string{"x"}); err != nil { + t.Fatalf("nil guard Begin returned %v", err) + } + g.End() + if res := g.Recover(); res != nil { + t.Fatalf("nil guard recovered %+v", res) + } +} + +func TestMarkerContentsAreDiagnosable(t *testing.T) { + dir := t.TempDir() + g, _ := newGuard(t, dir) + if err := g.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(markerPath(dir)) + if err != nil { + t.Fatal(err) + } + var m AppStopMarker + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("the marker on disk is not readable JSON: %v", err) + } + if !m.Active || m.OpID != "volume-dump:immich" || m.Reason != ReasonVolumeDump || + len(m.Stacks) != 1 || m.Stacks[0] != "immich" || m.StartedAt.IsZero() { + t.Fatalf("the marker does not record enough to diagnose the interruption: %+v", m) + } + // 0600 — it names customer apps. + fi, err := os.Stat(markerPath(dir)) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Perm() != 0o600 { + t.Fatalf("marker mode = %v, want 0600", fi.Mode().Perm()) + } +} + +func TestBeginWithNoStacksWritesNothing(t *testing.T) { + dir := t.TempDir() + g, _ := newGuard(t, dir) + if err := g.Begin("op", ReasonVolumeDump, nil); err != nil { + t.Fatal(err) + } + if markerExists(t, dir) { + t.Fatal("a marker was written for an operation that stops nothing") + } +} + +// --- Scenarios E/F — DumpAppVolumesSafe, the primary site ---------------------------------------- + +// inspectingProvider is the StackDataProvider slice DumpAppVolumesSafe touches. It records whether +// the marker file EXISTED at each step — the positive observable for the ordering property. An +// absent log line is not evidence (standing rule 3); the file's presence at the moment of the stop +// is. +// +// GetDockerVolumes returns nothing, so the dump itself is a no-op and no Docker is involved — the +// stop/start bracket around it is what is under test. +type inspectingProvider struct { + StackDataProvider + markerFile string + events []string + stopErr error + startErr error + markerPresentAtStop bool + markerAtStartCall bool + // panicOnVolumes simulates a hard abort (SIGKILL/power cut) at the point the dump begins: the + // unwind skips the restart statement, exactly as a kill would. + panicOnVolumes bool +} + +func (p *inspectingProvider) GetDockerVolumes(string) []string { + if p.panicOnVolumes { + panic("simulated hard abort mid-dump") + } + return nil +} + +func (p *inspectingProvider) StopStack(name string) error { + _, err := os.Stat(p.markerFile) + p.markerPresentAtStop = err == nil + p.events = append(p.events, "stop:"+name) + return p.stopErr +} + +func (p *inspectingProvider) StartStack(name string) error { + _, err := os.Stat(p.markerFile) + p.markerAtStartCall = err == nil + p.events = append(p.events, "start:"+name) + return p.startErr +} + +func newDumpManager(t *testing.T, dir string, p *inspectingProvider) *Manager { + t.Helper() + lg := log.New(io.Discard, "", 0) + m := &Manager{logger: lg, stackProvider: p, systemDataPath: dir} + m.appStop = NewAppStopGuard(markerPath(dir), lg) + return m +} + +func TestDumpAppVolumesSafe_MarkerCoversTheWholeStopStartWindow(t *testing.T) { + // Scenario F, the happy path: the marker is on disk BEFORE the stop, still on disk for the whole + // time the app is down, and GONE once the restart succeeds. + dir := t.TempDir() + p := &inspectingProvider{markerFile: markerPath(dir)} + m := newDumpManager(t, dir, p) + + if err := m.DumpAppVolumesSafe("immich"); err != nil { + t.Fatalf("DumpAppVolumesSafe: %v", err) + } + + if !p.markerPresentAtStop { + t.Fatal("the marker was NOT on disk when the app was stopped — a crash one instruction later " + + "strands the app, which is the entire failure this marker exists to prevent") + } + if !p.markerAtStartCall { + t.Fatal("the marker was already gone while the app was still down") + } + if markerExists(t, dir) { + t.Fatal("the marker survived a dump whose restart succeeded — the next boot would restart the app again") + } + if len(p.events) != 2 || p.events[0] != "stop:immich" || p.events[1] != "start:immich" { + t.Fatalf("events=%v, want [stop:immich start:immich]", p.events) + } +} + +func TestDumpAppVolumesSafe_Interrupted_RecoveryBringsTheAppBack(t *testing.T) { + // Scenario E end-to-end THROUGH THE PRODUCTION PATH, and WITHOUT running any cleanup. + // + // The abort is real: GetDockerVolumes panics, which unwinds out of DumpAppVolumesSafe AFTER the + // marker was written and the app stopped, and BEFORE the restart statement — and because that + // restart is a plain statement, not a defer, it never runs. That is the shape of a hard kill. + // + // The earlier version of this test called m.appStop.Begin itself, which meant it proved the + // marker type worked and NOT that DumpAppVolumesSafe uses it — it survived the red-proof that + // deleted the production Begin call. Driving the real function is what makes the proof bite. + // + // RED-PROOF: delete the `m.appStop.Begin(...)` call from DumpAppVolumesSafe and this test fails — + // nothing is written, so nothing is recovered. Demonstrated in REPORT.md §5. + dir := t.TempDir() + p := &inspectingProvider{markerFile: markerPath(dir), panicOnVolumes: true} + m := newDumpManager(t, dir, p) + + func() { + defer func() { + if recover() == nil { + t.Error("the simulated abort did not fire — this test proves nothing") + } + }() + _ = m.DumpAppVolumesSafe("immich") + }() + + if !p.markerPresentAtStop { + t.Fatal("the app was stopped before any marker existed") + } + if p.markerAtStartCall { + t.Fatal("the restart ran despite the abort — the simulation is wrong, not the code") + } + + // — a fresh one starts and recovers from the file alone. + g, starter := newGuard(t, dir) + res := g.Recover() + + if len(starter.starts) != 1 || starter.starts[0] != "immich" { + t.Fatalf("started %v — the app stopped by the interrupted dump was not brought back", starter.starts) + } + if res == nil || res.Reason != ReasonVolumeDump { + t.Fatalf("recovery did not name the volume dump as the interrupted operation: %+v", res) + } + if markerExists(t, dir) { + t.Fatal("the marker was not cleared after a successful recovery") + } +} + +func TestDumpAppVolumesSafe_FailedRestartKeepsTheMarker(t *testing.T) { + dir := t.TempDir() + p := &inspectingProvider{markerFile: markerPath(dir), startErr: errors.New("compose up failed")} + m := newDumpManager(t, dir, p) + + if err := m.DumpAppVolumesSafe("immich"); err == nil { + t.Fatal("a failed restart must surface as an error") + } + if !markerExists(t, dir) { + t.Fatal("the marker was cleared even though the restart FAILED — the app is still down and " + + "nothing records that it is owed a restart") + } +} + +func TestDumpAppVolumesSafe_FailedStopClearsTheMarker(t *testing.T) { + // Nothing was stopped, so nothing is owed a restart. A stranded marker here would cost a + // spurious restart at the next startup AND a false "a backup was interrupted" alert. + dir := t.TempDir() + p := &inspectingProvider{markerFile: markerPath(dir), stopErr: errors.New("stack is protected")} + m := newDumpManager(t, dir, p) + + if err := m.DumpAppVolumesSafe("traefik"); err == nil { + t.Fatal("a failed stop must surface as an error") + } + if markerExists(t, dir) { + t.Fatal("a marker was left behind for an app that was never stopped") + } +} diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index adb9e59..fb54315 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -32,6 +32,12 @@ type Manager struct { // tier2Notify, if set, is called after each Tier 2 copy (success: err==nil) for notifications. tier2Notify func(stackName, destLabel string, dur time.Duration, err error) + // appStop (R-166) is the crash marker for operations that stop an app, work on its data, and + // start it again. Written BEFORE the stop and cleared AFTER the restart, so a SIGKILL or a power + // cut in that window leaves a durable record that Recover honours at the next startup. Built in + // NewManager from cfg.Paths.DataDir — see appstop_marker.go for why it is not quiesce's file. + appStop *AppStopGuard + // offbox (Part B): the restic-SFTP exec seam (nil → real restic) + the failure→operator-alert hook. offboxRunner offboxRunner offboxNotify func(dur time.Duration, snapshots int, err error) @@ -216,10 +222,30 @@ func NewManager(cfg *config.Config, sett *settings.Settings, logger *log.Logger) settings: sett, systemDataPath: cfg.Paths.SystemDataPath, } + // R-166: its OWN file next to quiesce-state.json, never inside it — one file, one writer. + m.appStop = NewAppStopGuard(filepath.Join(cfg.Paths.DataDir, "appstop-state.json"), logger) m.reconcileCrashedRun() return m } +// AppStopGuard exposes the app-stop crash marker so the exporter (a different package with the same +// stop-work-start shape) can share the one marker file rather than opening a second one. +func (m *Manager) AppStopGuard() *AppStopGuard { return m.appStop } + +// SetAppStopGuard injects the guard instead of using the one NewManager built. INIT-ONLY — call once +// during single-threaded startup, before any backup runs. +// +// It exists because of a startup ORDERING constraint, not for testing: the guard's Recover must +// complete before the boot reconciler is launched (main.go:~236) and this manager is not constructed +// until ~line 272. So main.go builds the guard early, recovers, and hands the SAME object here — +// rather than a second guard over the same file, which would be one file with two owners, the exact +// shape this marker was kept out of quiesce's file to avoid. +func (m *Manager) SetAppStopGuard(g *AppStopGuard) { + if g != nil { + m.appStop = g + } +} + // reconcileCrashedRun makes the persisted offbox status truthful after a crash (campaign C1): a controller // that died mid-run left LastStatus="running" on disk (the in-memory single-flight mutex is gone with the // process, but the persisted status keeps lying "running" forever). Flip it to error with a Hungarian @@ -679,13 +705,28 @@ func atomicPromoteTar(tmpPath, finalPath string) error { // DumpAppVolumesSafe stops the stack before dumping volumes and restarts after. // Prevents inconsistent tars of live database volumes (e.g. PostgreSQL). // Protected stacks that reject StopStack will return an error — callers handle as warning. +// +// R-166: the stop→dump→start window is marked. Before this, a controller killed between the stop +// and the start left the app down with NOTHING on disk saying why or that it was owed a restart — +// and a stopped app has zero containers, which the boot reconciler then read as a deliberate +// customer stop and left alone. The marker is the mechanism, not the restart call below: a SIGKILL +// runs no deferred function (Campaign 8 fault 10, on live hardware), so only something already +// written to disk can survive it. func (m *Manager) DumpAppVolumesSafe(stackName string) error { if m.stackProvider == nil { return fmt.Errorf("no stack provider") } + // Intent before the act: refuse to stop an app we cannot promise to restart. + if err := m.appStop.Begin("volume-dump:"+stackName, ReasonVolumeDump, []string{stackName}); err != nil { + return fmt.Errorf("could not record the app-stop marker for %s (refusing to stop it unprotected): %w", stackName, err) + } + m.logger.Printf("[INFO] [backup] Stopping %s for safe volume dump", stackName) if err := m.stackProvider.StopStack(stackName); err != nil { + // Nothing was stopped, so nothing is owed a restart — clear rather than strand a marker that + // would cost a spurious (if harmless) restart at the next startup. + m.appStop.End() return fmt.Errorf("could not stop %s for volume dump: %w", stackName, err) } @@ -695,6 +736,10 @@ func (m *Manager) DumpAppVolumesSafe(stackName string) error { startErr := m.stackProvider.StartStack(stackName) if startErr != nil { m.logger.Printf("[ERROR] [backup] Failed to restart %s after volume dump: %v", stackName, startErr) + } else { + // Cleared ONLY on a restart that succeeded. A failed restart keeps the marker so the next + // startup retries — the app really is still owed one. + m.appStop.End() } // Surface both errors — callers must know if the app is left stopped diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index d02f06d..36ce0fa 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -276,6 +276,22 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of } // --- FILES ---------------------------------------------------------------------------------- + // R-166: mark the stop→restore→start window BEFORE stopping. A controller killed anywhere inside + // it used to leave the app down with nothing on disk recording that it was owed a restart — and a + // full offsite restore is a LONG window, so this is the shape most likely to be interrupted. + if err := m.appStop.Begin("offbox-reconstitute:"+stack, ReasonOffboxReconstitute, []string{stack}); err != nil { + return res, fmt.Errorf("a(z) %s leállítása előtti jelölő nem menthető: %w", stack, err) + } + // restartStack starts the app and clears the marker ONLY when the start actually succeeded — a + // failed start leaves the marker so the next startup retries. Every bring-up below goes through + // it; a bare StartStack here would clear nothing and strand the marker on the success path. + restartStack := func() error { + err := m.stackProvider.StartStack(stack) + if err == nil { + m.appStop.End() + } + return err + } if err := m.stackProvider.StopStack(stack); err != nil { m.logger.Printf("[WARN] [offbox] could not stop %s before reconstitution: %v (continuing)", stack, err) } @@ -291,7 +307,7 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of if cErr != nil { // Best-effort bring-up: leaving the app stopped after a partial copy would turn a failed // restore into an outage. - if sErr := m.stackProvider.StartStack(stack); sErr != nil { + if sErr := restartStack(); sErr != nil { m.logger.Printf("[WARN] [offbox] %s: restart after failed placement also failed: %v", stack, sErr) } return res, fmt.Errorf("a(z) %s fájljainak visszaállítása sikertelen: %w", stack, cErr) @@ -308,7 +324,7 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of if hasDB { if err := m.stackProvider.StartStackServices(stack, dbServices); err != nil { // Best-effort bring-up: a failed restore must not also be an outage. - if sErr := m.stackProvider.StartStack(stack); sErr != nil { + if sErr := restartStack(); sErr != nil { m.logger.Printf("[WARN] [offbox] %s: full start after failed DB-only start also failed: %v", stack, sErr) } return res, fmt.Errorf("a(z) %s adatbázis-szolgáltatásának indítása sikertelen: %w", stack, err) @@ -316,13 +332,13 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of n, iErr := m.reimportDBDumpsFrom(ctx, stack, scratchDumpDir) res.DBsReplayed = n if iErr != nil { - if sErr := m.stackProvider.StartStack(stack); sErr != nil { + if sErr := restartStack(); sErr != nil { m.logger.Printf("[WARN] [offbox] %s: full start after failed replay also failed: %v", stack, sErr) } return res, fmt.Errorf("az adatbázis visszaállítása sikertelen: %w — a korábbi állapot mentése megvan: %s", iErr, filepath.Base(safety)) } } - if err := m.stackProvider.StartStack(stack); err != nil { + if err := restartStack(); err != nil { return res, fmt.Errorf("a(z) %s újraindítása sikertelen a fájlok visszaállítása után: %w", stack, err) } if err := m.waitForHealthy(stack, 90*time.Second); err != nil { diff --git a/controller/internal/bootrecon/bootrecon.go b/controller/internal/bootrecon/bootrecon.go index 7cb72e5..2616e39 100644 --- a/controller/internal/bootrecon/bootrecon.go +++ b/controller/internal/bootrecon/bootrecon.go @@ -11,10 +11,15 @@ // - **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. +// - **A user's Stop survives a reboot.** This is still the whole safety argument; only the way it +// is established changed. Until v0.189.0 it was inferred — the UI's Stop is `docker compose +// down`, which REMOVES containers, so "zero containers" was read as "the customer stopped it" +// and left alone. Since v0.189.0 (R-166) the customer's intent is RECORDED in app.yaml and read +// directly, because the inference could not distinguish a deliberate Stop from a power cut or an +// interrupted backup, and silently stranded both. An app.yaml with no recorded intent — every +// app on every box predating the field — keeps the old inference exactly. See isBootOrphan, +// TestReconcile_UserStoppedAppIsNeverStarted and +// TestReconcile_LegacyNoDesiredState_BehavesExactlyAsBefore. // // 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. @@ -87,17 +92,66 @@ func sleepCtx(ctx context.Context, d time.Duration) { // 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. +// - Deployed — an app that is installed. NOTE: `Deployed` means INSTALLED, not "wanted running"; +// the two were conflated until v0.189.0 and that conflation is what the desired-state term below +// repairs. // - 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. +// - desired state — see below. REPLACES the old container-count term. // - IsDownState — stopped/exited/degraded (R-51 included: a boot that half-started a stack is the // same interrupted-boot shape). +// +// ── WHY INTENT REPLACED THE CONTAINER COUNT (R-166, closing R-157 mechanism B) ──────────────────── +// +// This gate used to end in `len(s.Containers) > 0`, and its comment called that "the D-case guard": +// a UI Stop is `compose down`, which REMOVES containers, so zero containers was read as "the +// 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 +// +// 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 +// stop inferring and read what the customer actually asked for, which app.yaml now records. +// +// ── WHAT ABSENT STILL MEANS, AND WHY THE OLD BEHAVIOUR IS KEPT ──────────────────────────────────── +// +// DesiredStateUnknown falls back to the ORIGINAL container-count rule, byte-for-byte. This is the +// single most important line in the change. Every app.yaml on every existing box predates the field, +// so absent is what the whole fleet reads on upgrade; treating absent as "running" would start, on +// the first boot after the upgrade, every app its owner had deliberately stopped. The fallback is +// what makes this feature inert for an app nobody has pressed a button on since — see +// TestReconcile_LegacyNoDesiredState_BehavesExactlyAsBefore and its red-proof. +// +// The full decision table (§8.1): +// +// desired containers state → result +// stopped any any → never an orphan (the customer said so) +// running 0 — → ORPHAN ← the R-157 case, invisible before v0.189.0 +// running >0 IsDownState → ORPHAN (unchanged) +// running >0 up → not an orphan +// absent 0 — → not an orphan (exactly the pre-v0.189.0 behaviour) +// absent >0 IsDownState → ORPHAN (exactly the pre-v0.189.0 behaviour) func isBootOrphan(s stacks.Stack) bool { - return s.Deployed && !s.Protected && !s.Deploying && - len(s.Containers) > 0 && stacks.IsDownState(s.State) + if !s.Deployed || s.Protected || s.Deploying { + return false + } + switch stacks.DesiredStateOf(s) { + case stacks.DesiredStateStopped: + // The customer pressed Stop. No observation may overturn that — not a missing container, not + // a down state, not a reboot. Nothing else in this package starts an app. + return false + case stacks.DesiredStateRunning: + // Wanted running. ANY way of not being up is a fault to repair, including having no + // containers at all — which is the case the old count term structurally could not see. + return len(s.Containers) == 0 || stacks.IsDownState(s.State) + default: + // DesiredStateUnknown — legacy. Keep the pre-R-166 rule exactly. + return 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 diff --git a/controller/internal/bootrecon/desiredstate_test.go b/controller/internal/bootrecon/desiredstate_test.go new file mode 100644 index 0000000..8ef48d0 --- /dev/null +++ b/controller/internal/bootrecon/desiredstate_test.go @@ -0,0 +1,236 @@ +package bootrecon + +import ( + "context" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-166 / decision D-b: the boot reconciler reads the CUSTOMER'S RECORDED INTENT instead of +// inferring it from a container count. These tests are the §8.1 decision table, one row each, plus +// the two red-proofs that make the safety properties falsifiable. +// +// The assertions are EFFECTS — which apps the sweep actually started — not "isBootOrphan returned +// true". A predicate can be right while the sweep does nothing with it. + +// withDesired returns a copy of s carrying a recorded desired state. +func withDesired(s stacks.Stack, desired string) stacks.Stack { + s.AppConfig = &stacks.AppConfig{Deployed: true, DesiredState: desired} + return s +} + +// vanished is the R-157 shape this whole change exists to see: the app is deployed and wanted +// running, and its containers are simply GONE — a power cut mid-compose, or an interrupted deploy. +// Byte-identical on the Docker side to a user stop, which is exactly why the old container-count +// rule could not tell them apart. +func vanished(name string) stacks.Stack { + return stacks.Stack{Name: name, Deployed: true, State: stacks.StateStopped, Containers: nil} +} + +// runSweep runs one full reconciliation and returns which apps were started, and how often. +func runSweep(t *testing.T, list []stacks.Stack) (*fakeStacks, Result) { + t.Helper() + f := &fakeStacks{list: list, onStart: comesUp} + r, _ := newTestReconciler(f) + res := r.Run(context.Background()) + return f, res +} + +// --- Scenario A — the customer's Stop survives everything --------------------------------------- + +func TestReconcile_DesiredStopped_IsNeverStartedAndNeverACandidate(t *testing.T) { + // Recorded stopped, and down in every way the box can be down: no containers at all, and (second + // app) containers present but exited. Neither may be touched, and neither may even be LISTED — + // a candidate that is never started still tells the operator an app is broken when it is not. + f, res := runSweep(t, []stacks.Stack{ + withDesired(vanished("nextcloud"), stacks.DesiredStateStopped), + withDesired(bootOrphan("immich"), stacks.DesiredStateStopped), + }) + + if len(f.starts) != 0 { + t.Fatalf("an app the customer deliberately stopped was started: %v", f.starts) + } + if len(res.Candidates) != 0 { + t.Fatalf("desired=stopped app listed as a boot orphan: %v", res.Candidates) + } + if res.Attempts != 0 { + t.Fatalf("attempts=%d, want 0 — the sweep should have had nothing to do", res.Attempts) + } +} + +// --- Scenario B — the power-cut app comes back (THE R-157 CASE) --------------------------------- + +func TestReconcile_DesiredRunning_ZeroContainers_IsRecovered(t *testing.T) { + // THE POINT OF THE RELEASE. Before v0.189.0 this app was invisible to the reconciler: zero + // containers failed the `len(s.Containers) > 0` term, so it was skipped as "the customer stopped + // it" and stayed down until a human noticed. + // + // RED-PROOF: restore that term in isBootOrphan's DesiredStateRunning branch — i.e. make it + // return len(s.Containers) > 0 && stacks.IsDownState(s.State) + // and this test fails with `zero starts`. Demonstrated in REPORT.md §5. + f, res := runSweep(t, []stacks.Stack{withDesired(vanished("immich"), stacks.DesiredStateRunning)}) + + if f.starts["immich"] == 0 { + t.Fatalf("an app recorded desired=running with zero containers was NOT started — this is the R-157 defect") + } + if len(res.Recovered) != 1 || res.Recovered[0] != "immich" { + t.Fatalf("recovered=%v, want [immich]", res.Recovered) + } + if len(res.StillDown) != 0 { + t.Fatalf("still down after a successful start: %v", res.StillDown) + } +} + +func TestReconcile_DesiredRunning_ContainersDown_IsRecovered(t *testing.T) { + // The pre-existing F5 shape, unchanged by R-166 — proven still covered so the rewrite cannot + // have traded one case for the other. + f, _ := runSweep(t, []stacks.Stack{withDesired(bootOrphan("calibre-web"), stacks.DesiredStateRunning)}) + if f.starts["calibre-web"] == 0 { + t.Fatal("an app recorded desired=running with exited containers was not started") + } +} + +func TestReconcile_DesiredRunning_AlreadyUp_IsLeftAlone(t *testing.T) { + up := withDesired(stacks.Stack{ + Name: "vaultwarden", Deployed: true, State: stacks.StateRunning, + Containers: []stacks.ContainerInfo{{Name: "vw", State: stacks.StateRunning}}, + }, stacks.DesiredStateRunning) + + f, res := runSweep(t, []stacks.Stack{up}) + if len(f.starts) != 0 { + t.Fatalf("a running app was restarted: %v", f.starts) + } + if len(res.Candidates) != 0 { + t.Fatalf("a running app was listed as a boot orphan: %v", res.Candidates) + } +} + +// --- Scenario C — a legacy app.yaml behaves EXACTLY as it does today ----------------------------- + +func TestReconcile_LegacyNoDesiredState_BehavesExactlyAsBefore(t *testing.T) { + // THE MOST DANGEROUS MISTAKE AVAILABLE IN THIS CHANGE. Every app.yaml on every existing box was + // written before desired_state existed, so `absent` is what the whole fleet reads on upgrade. + // Treating absent as "running" would start, on the first boot after the upgrade, every app its + // owner had deliberately stopped — silently, fleet-wide. + // + // Both legacy rows of §8.1 asserted together, because the safety property is the PAIR: absent + + // zero containers must be skipped, and absent + down containers must still be recovered. A + // change that broke only one of them would look correct from the other. + // + // RED-PROOF: make the `default:` branch of isBootOrphan return + // len(s.Containers) == 0 || stacks.IsDownState(s.State) + // (i.e. treat absent as running) and this test fails on the "started" assertion. + // Demonstrated in REPORT.md §5. + legacyStopped := vanished("nextcloud") // no AppConfig at all — the true legacy shape + legacyOrphan := bootOrphan("calibre-web") // no AppConfig, containers present and exited + legacyOrphan.AppConfig = nil + legacyStopped.AppConfig = nil + + f, res := runSweep(t, []stacks.Stack{legacyStopped, legacyOrphan}) + + if n := f.starts["nextcloud"]; n != 0 { + t.Fatalf("a LEGACY app with no recorded intent and zero containers was started %d time(s) — "+ + "this is the upgrade regression that restarts apps customers deliberately stopped", n) + } + if f.starts["calibre-web"] == 0 { + t.Fatal("a LEGACY boot orphan (containers present, exited) was not recovered — the pre-R-166 behaviour regressed") + } + if len(res.Candidates) != 1 || res.Candidates[0] != "calibre-web" { + t.Fatalf("candidates=%v, want exactly [calibre-web]", res.Candidates) + } +} + +func TestReconcile_LegacyAppConfigPresentButFieldAbsent_IsAlsoLegacy(t *testing.T) { + // An app.yaml that EXISTS but predates the field: AppConfig is non-nil, DesiredState is "". + // This is the realistic fleet shape (nil AppConfig only happens with no app.yaml at all), and it + // must take the same legacy path — a nil-vs-empty distinction slipping in here would silently + // split the fleet in two. + s := vanished("immich") + s.AppConfig = &stacks.AppConfig{Deployed: true} // DesiredState is the zero value + f, _ := runSweep(t, []stacks.Stack{s}) + if len(f.starts) != 0 { + t.Fatalf("an app.yaml with no desired_state key was treated as running: %v", f.starts) + } +} + +// --- The §8.1 table, every row, in one place ---------------------------------------------------- + +func TestIsBootOrphan_DecisionTable(t *testing.T) { + cases := []struct { + name string + desired string + containers int + state stacks.ContainerState + want bool + }{ + {"stopped/no containers", stacks.DesiredStateStopped, 0, stacks.StateStopped, false}, + {"stopped/down containers", stacks.DesiredStateStopped, 2, stacks.StateExited, false}, + {"stopped/running", stacks.DesiredStateStopped, 2, stacks.StateRunning, false}, + {"running/no containers", stacks.DesiredStateRunning, 0, stacks.StateStopped, true}, + {"running/down containers", stacks.DesiredStateRunning, 2, stacks.StateExited, true}, + {"running/degraded", stacks.DesiredStateRunning, 2, stacks.StateDegraded, true}, + {"running/up", stacks.DesiredStateRunning, 2, stacks.StateRunning, false}, + {"absent/no containers", stacks.DesiredStateUnknown, 0, stacks.StateStopped, false}, + {"absent/down containers", stacks.DesiredStateUnknown, 2, stacks.StateExited, true}, + {"absent/up", stacks.DesiredStateUnknown, 2, stacks.StateRunning, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := stacks.Stack{ + Name: "app", Deployed: true, State: tc.state, + Containers: make([]stacks.ContainerInfo, tc.containers), + AppConfig: &stacks.AppConfig{Deployed: true, DesiredState: tc.desired}, + } + if got := isBootOrphan(s); got != tc.want { + t.Fatalf("isBootOrphan(desired=%q containers=%d state=%s) = %v, want %v", + tc.desired, tc.containers, tc.state, got, tc.want) + } + }) + } +} + +func TestIsBootOrphan_ExistingGuardsSurviveTheRewrite(t *testing.T) { + // Protected and Deploying were guards before R-166 and must still be, at the strongest desired + // state available — the rewrite reordered the terms, and a reorder is exactly how a guard gets + // dropped without anyone noticing. + base := func() stacks.Stack { + return withDesired(vanished("traefik"), stacks.DesiredStateRunning) + } + protected := base() + protected.Protected = true + if isBootOrphan(protected) { + t.Fatal("a PROTECTED stack became a boot orphan — the base-stack self-heal owns those") + } + deploying := base() + deploying.Deploying = true + if isBootOrphan(deploying) { + t.Fatal("a DEPLOYING stack became a boot orphan — mid-deploy is not a fault") + } + notDeployed := base() + notDeployed.Deployed = false + if isBootOrphan(notDeployed) { + t.Fatal("a stack that is not deployed became a boot orphan") + } +} + +// --- Scenario G — the two recoveries do not fight ------------------------------------------------ + +func TestReconcile_AppAlreadyRestartedByTheMarker_IsNotAlsoAnOrphan(t *testing.T) { + // §8.4's REPORTING requirement. The app-stop marker's Recover runs to completion before this + // sweep is launched, so by the time the reconciler looks, the app it restarted is UP. It must + // therefore not appear as a candidate at all — an app the marker already explained must not also + // be reported as an unexplained boot orphan, or one fault reads as two. + restoredByMarker := withDesired(stacks.Stack{ + Name: "immich", Deployed: true, State: stacks.StateRunning, + Containers: []stacks.ContainerInfo{{Name: "immich-server", State: stacks.StateRunning}}, + }, stacks.DesiredStateRunning) + + f, res := runSweep(t, []stacks.Stack{restoredByMarker}) + if len(res.Candidates) != 0 { + t.Fatalf("an app the marker had already restarted was ALSO reported as a boot orphan: %v", res.Candidates) + } + if n := f.starts["immich"]; n != 0 { + t.Fatalf("the app was started a second time (%d) — one fault, one start", n) + } +} diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index bb12f07..80ea086 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -104,6 +104,23 @@ type AppConfig struct { // EmailEnabled is the per-app app-email toggle (default off). When on AND the global toggle is // on AND the app has an smtp_mapping, the controller injects the relay SMTP env at compose time. EmailEnabled bool `yaml:"email_enabled,omitempty" json:"email_enabled,omitempty"` + // DesiredState (R-166 / decision D-b) is what the CUSTOMER asked for: DesiredStateRunning or + // DesiredStateStopped. It is TRI-state, and the third value is the entire safety property: + // + // ABSENT ("") MEANS UNKNOWN — IT NEVER MEANS "running". + // + // Every app.yaml on every existing box was written before this field existed, so absent is the + // overwhelmingly common value on upgrade. Reading it as "running" would start, on the next boot + // after the upgrade, every app its owner deliberately stopped — fleet-wide, silently. Where the + // state is unknown the boot reconciler falls back to its pre-R-166 behaviour instead of inventing + // an answer (see internal/bootrecon.isBootOrphan and the §8.1 table it implements). + // + // ONE OWNER: the customer's own action writes this and nothing else does. StartStack/StopStack + // are NOT writers — twelve of their fourteen callers are machines (quiesce, the backup volume + // dump, app export, the storage gate, migration, the boot reconciler), and recording intent in + // the primitive would make a nightly backup indistinguishable from the customer pressing Stop, + // which is the exact confusion this field exists to end. Writers: SetDesiredState's callers. + DesiredState string `yaml:"desired_state,omitempty" json:"desired_state,omitempty"` } // DeployRequest contains the user-provided values from the deploy form. @@ -330,6 +347,12 @@ func (m *Manager) DeployStack(req DeployRequest) (string, error) { DeployedAt: time.Now().UTC().Format(time.RFC3339), Env: env, LockedFields: lockedFields, + // R-166: deploying an app IS the customer asking for it to run, and this is the + // intent-before-the-act write (§8.2). Recorded on the transitional Deployed:false write too, + // which is harmless and correct: nothing reads desired state on a stack that is not deployed + // (isBootOrphan gates on Deployed first), and if the compose-up then fails, runComposeDeploy + // reverts Deployed to false — so a failed deploy can never present as an app owed a restart. + DesiredState: DesiredStateRunning, } diskCfg := *appCfg @@ -670,6 +693,26 @@ func (m *Manager) UpdateOptionalConfig(stackName string, values map[string]strin // If deployed, recreate containers to pick up new env vars // (docker compose restart does NOT pick up new env vars — must use up -d) if stack.Deployed { + // R-166 — the THIRD customer-intent point, alongside the API action switch and deploy/import. + // This branch runs `up -d`, so the customer editing an app's settings ends with the app + // RUNNING; recording that keeps intent and reality in step. Written before the act (§8.2). + // + // Deliberately inside the `stack.Deployed` branch only: the other branch starts nothing, so + // it expresses no opinion about whether the app should run. Set on the already-loaded appCfg + // rather than through SetDesiredState so it rides the save just above instead of rewriting + // app.yaml twice — the load-then-save is what makes that safe (SaveAppConfig copies-and- + // overlays, so no other field is disturbed). + if appCfg.DesiredState != DesiredStateRunning { + appCfg.DesiredState = DesiredStateRunning + if err := SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil { + return fmt.Errorf("recording desired state before applying the new config: %w", err) + } + m.mu.Lock() + if s, ok := m.stacks[stackName]; ok && s.AppConfig != nil { + s.AppConfig.DesiredState = DesiredStateRunning + } + m.mu.Unlock() + } m.logger.Printf("[INFO] [stacks] Restarting %s to apply new optional config", stackName) env := m.stackEnv(stackDir) if _, err := m.composeExecCustomEnv(stackDir, env, "up", "-d"); err != nil { @@ -741,14 +784,24 @@ func LoadAppConfig(stackDir string) *AppConfig { func SaveAppConfig(stackDir string, cfg *AppConfig, encKey []byte, sensitiveVars []string) error { encryptedCount := 0 - // Clone env and encrypt sensitive values - saveCfg := &AppConfig{ - Deployed: cfg.Deployed, - DeployedAt: cfg.DeployedAt, - Env: make(map[string]string, len(cfg.Env)), - LockedFields: cfg.LockedFields, - EmailEnabled: cfg.EmailEnabled, - } + // COPY-AND-OVERLAY, never a field-by-field rebuild (the R-100 lesson, v0.181.0). + // + // This used to be a struct literal naming five fields. That shape is safe exactly until someone + // adds a sixth: the new field is silently dropped on every save, and because the save path is + // shared by nine call sites the loss shows up far from the code that caused it. R-100 shipped + // with two live instances of precisely this bug (offboxConfigHandler and ApplyOffsiteTarget both + // rebuilt a target field-by-field and erased LastSuccess). + // + // A value copy carries EVERY field the struct has, including ones added after this line was + // written, so it is safe by construction. Only Env is rebuilt below — it is the one field that + // needs transforming (encryption), and it must not alias the caller's map. + // + // LIMITATION, measured not assumed (TestSaveAppConfig_UnknownYAMLKeysAreDropped): keys present in + // the on-disk YAML that this struct does not model are NOT preserved — the round-trip goes + // through the struct, so yaml.Unmarshal discards them before this function ever sees them. That + // is unchanged by R-166 and is why every writer must load-then-save rather than construct. + saveCfg := *cfg + saveCfg.Env = make(map[string]string, len(cfg.Env)) sensitiveSet := make(map[string]bool, len(sensitiveVars)) for _, v := range sensitiveVars { sensitiveSet[v] = true diff --git a/controller/internal/stacks/desiredstate.go b/controller/internal/stacks/desiredstate.go new file mode 100644 index 0000000..02fa00e --- /dev/null +++ b/controller/internal/stacks/desiredstate.go @@ -0,0 +1,158 @@ +package stacks + +import ( + "fmt" + "path/filepath" +) + +// Desired-state values for AppConfig.DesiredState (R-166, decision D-b). +// +// THREE values, and the empty one is load-bearing — see the field's own comment in deploy.go. +// Named constants rather than bare strings so a typo is a compile error and every reader can be +// found with one grep. +const ( + // DesiredStateUnknown is the absent value: nobody has told us what the customer wants. It is the + // value of every app.yaml written before v0.189.0. It NEVER means "running". + DesiredStateUnknown = "" + // DesiredStateRunning — the customer asked for this app to be running. An app in this state that + // is not running is a fault the boot reconciler repairs, HOWEVER it came to be down. + DesiredStateRunning = "running" + // DesiredStateStopped — the customer pressed Stop. Nothing may start it again on its own. + DesiredStateStopped = "stopped" +) + +// SetDesiredState records the CUSTOMER's intent for a stack in its app.yaml. +// +// THE OWNERSHIP RULE, and the reason this is a separate function rather than a line inside +// StopStack/StartStack: desired state is written by the customer's own action and by nothing else. +// A census of the two primitives on 2026-08-02 found fourteen call sites, of which exactly two are +// the customer (the API action switch and the deploy path). The other twelve are machines — the +// quiesce loop, the backup volume dump, offbox reconstitution, app export/restore, the storage +// drive-absent gate, the migration engine and the boot reconciler itself. If the primitive recorded +// intent, a nightly backup stopping an app for a consistent volume dump would be indistinguishable +// from the customer stopping it, and the app would never come back. That confusion is the defect +// R-166 exists to end, so it must not be reintroduced one layer down. +// +// Callers MUST write intent BEFORE performing the act (§8.2), and MUST refuse the act if this +// returns an error. The asymmetry is deliberate: +// +// - Stop: intent first. If the write lands and the stop then fails, the record says "stopped" +// while the app runs — harmless, because the reconciler only ever acts on apps that are DOWN. +// The reverse order risks an app with zero containers and "running" still recorded, i.e. a +// deliberate stop undone at the next boot. +// - Start: intent first. If the start then fails, the reconciler retries it later — which is +// exactly what is wanted. +// +// An app with no app.yaml is a no-op, not an error: no app.yaml means nothing is deployed in that +// directory, and every consumer of desired state gates on Deployed first, so there is no intent to +// record and nothing that could read one. +func (m *Manager) SetDesiredState(name, desired string) error { + switch desired { + case DesiredStateRunning, DesiredStateStopped: + default: + // DesiredStateUnknown is deliberately NOT settable. "Unknown" is the absence of a record, + // and a caller asking to write it is a caller that has confused "no opinion" with "stopped". + return fmt.Errorf("desired state %q is not one of %q/%q", desired, DesiredStateRunning, DesiredStateStopped) + } + + stack, ok := m.GetStack(name) + if !ok { + return fmt.Errorf("stack %q not found", name) + } + stackDir := filepath.Dir(stack.ComposePath) + + cfg := LoadAppConfig(stackDir) + if cfg == nil { + m.logger.Printf("[DEBUG] [stacks] desired state %s=%s: no app.yaml — nothing deployed here, nothing to record", name, desired) + return nil + } + if cfg.DesiredState == desired { + return nil // already recorded — do not rewrite app.yaml for no change + } + + previous := cfg.DesiredState + cfg.DesiredState = desired + meta := LoadMetadata(stackDir) + if err := SaveAppConfig(stackDir, cfg, m.encKey, SensitiveEnvVars(&meta)); err != nil { + // NEVER swallowed: the caller refuses the action on this error, because an act whose intent + // could not be recorded is exactly the ambiguity this feature removes. + return fmt.Errorf("recording desired state %q for stack %s: %w", desired, name, err) + } + m.logger.Printf("[INFO] [stacks] desired state for %s recorded as %q (was %q)", name, desired, previous) + + // Keep the in-memory view in step so nothing reads a stale intent between here and the next + // ScanStacks. Under the same lock every other AppConfig mutation uses. + m.mu.Lock() + if s, ok := m.stacks[name]; ok && s.AppConfig != nil { + s.AppConfig.DesiredState = desired + } + m.mu.Unlock() + return nil +} + +// DesiredStateOf returns the recorded customer intent for a stack, or DesiredStateUnknown when +// there is none (no app.yaml, or an app.yaml predating v0.189.0). +func DesiredStateOf(s Stack) string { + if s.AppConfig == nil { + return DesiredStateUnknown + } + return s.AppConfig.DesiredState +} + +// BackfillDesiredState writes DesiredStateRunning for every deployed app that has NO recorded +// desired state AND is observed UP right now. Returns how many were backfilled. Call ONCE at +// startup, before the boot reconciler. +// +// RUNNING-ONLY, AND THAT IS NOT AN OVERSIGHT. The one inference available for the other direction — +// "zero containers, therefore the customer stopped it" — IS THE DEFECT R-166 exists to remove. A +// power cut mid-compose, an interrupted deploy and a deliberate Stop all leave an app with zero +// containers, and nothing on disk distinguishes them. So an ambiguous app is left ambiguous: it +// keeps the legacy boot behaviour (never auto-started) until the customer next presses a button, +// which is both the safe outcome and byte-identical to what the box did before this feature. +// +// A running app is the one observation that IS unambiguous — an app that is up was, at some point, +// asked to be up — so it converges without waiting for a button press. +func (m *Manager) BackfillDesiredState() int { + backfilled := 0 + skippedAmbiguous := 0 + for _, s := range m.GetStacks() { + if !s.Deployed || s.Protected || s.Deploying { + continue + } + if DesiredStateOf(s) != DesiredStateUnknown { + continue + } + if !isObservedUp(s) { + skippedAmbiguous++ + continue + } + if err := m.SetDesiredState(s.Name, DesiredStateRunning); err != nil { + m.logger.Printf("[WARN] [stacks] desired-state backfill: %s: %v", s.Name, err) + continue + } + backfilled++ + } + // A positive observable either way (standing rule 3): "0 backfilled" and "the backfill never + // ran" must not look the same in a log. + m.logger.Printf("[INFO] [stacks] desired-state backfill: %d app(s) recorded as running, %d left unrecorded (state ambiguous — legacy boot behaviour retained)", + backfilled, skippedAmbiguous) + return backfilled +} + +// isObservedUp reports whether a stack's AGGREGATE state is up right now. +// +// It is deliberately an allow-list of up-states rather than !IsDownState: IsDownState excludes +// restarting, unknown and deploying, so its negation would call a crash-looping or unreadable stack +// "up" and backfill an intent from it. Only a positive reading may seed a durable record. +// +// aggregateState (manager.go) already walks EVERY container and lets any unhealthy or mixed result +// win, so a partly-dead app cannot reach here reading healthy — D-b's every-container requirement is +// met upstream and is deliberately not re-implemented. +func isObservedUp(s Stack) bool { + switch s.State { + case StateRunning, StateStarting: + return true + default: + return false + } +} diff --git a/controller/internal/stacks/desiredstate_test.go b/controller/internal/stacks/desiredstate_test.go new file mode 100644 index 0000000..38b28b8 --- /dev/null +++ b/controller/internal/stacks/desiredstate_test.go @@ -0,0 +1,338 @@ +package stacks + +import ( + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +// R-166 / decision D-b — desired state is owned by the customer's action, persisted in app.yaml, and +// backfilled only from an UNAMBIGUOUS observation. +// +// Every assertion here is on the FILE ON DISK (or on the started/skipped effect), never on "no error +// returned": the whole feature is a durable record, so a test that does not read the record back has +// proven nothing. + +// newDSManager builds a Manager over a temp stacks dir, with `names` registered as stacks. Real FS +// (t.TempDir) because the thing under test is a file write. +func newDSManager(t *testing.T, names ...string) (*Manager, string) { + t.Helper() + root := t.TempDir() + cfg := &config.Config{} + m := &Manager{cfg: cfg, logger: log.New(io.Discard, "", 0), stacks: map[string]*Stack{}} + for _, n := range names { + dir := filepath.Join(root, n) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + compose := filepath.Join(dir, "docker-compose.yml") + if err := os.WriteFile(compose, []byte("services: {}\n"), 0o644); err != nil { + t.Fatal(err) + } + m.stacks[n] = &Stack{Name: n, ComposePath: compose} + } + return m, root +} + +func stackDirOf(root, name string) string { return filepath.Join(root, name) } + +// writeAppYAML puts an app.yaml on disk verbatim — so a LEGACY file (no desired_state key) can be +// modelled exactly, rather than approximated through the struct that added the key. +func writeAppYAML(t *testing.T, dir, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, "app.yaml"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +func readAppYAML(t *testing.T, dir string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(dir, "app.yaml")) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +// --- Group A/B — the intent is persisted, and only the two legal values are accepted -------------- + +func TestSetDesiredState_PersistsStoppedToDisk(t *testing.T) { + m, root := newDSManager(t, "immich") + dir := stackDirOf(root, "immich") + writeAppYAML(t, dir, "deployed: true\ndeployed_at: \"2026-08-01T10:00:00Z\"\nenv:\n HDD_PATH: /mnt/hdd_1\n") + + if err := m.SetDesiredState("immich", DesiredStateStopped); err != nil { + t.Fatalf("SetDesiredState: %v", err) + } + + got := LoadAppConfig(dir) + if got == nil { + t.Fatal("app.yaml disappeared") + } + if got.DesiredState != DesiredStateStopped { + t.Fatalf("desired_state on disk = %q, want %q", got.DesiredState, DesiredStateStopped) + } + // The rest of the file must be intact — this write must not cost the app its deploy record. + if !got.Deployed || got.Env["HDD_PATH"] != "/mnt/hdd_1" || got.DeployedAt == "" { + t.Fatalf("recording intent damaged the config: %+v", got) + } + if raw := readAppYAML(t, dir); !strings.Contains(raw, "desired_state: stopped") { + t.Fatalf("the YAML key is not on disk:\n%s", raw) + } +} + +func TestSetDesiredState_RunningAndStoppedRoundTrip(t *testing.T) { + m, root := newDSManager(t, "app") + dir := stackDirOf(root, "app") + writeAppYAML(t, dir, "deployed: true\nenv: {}\n") + + for _, want := range []string{DesiredStateRunning, DesiredStateStopped, DesiredStateRunning} { + if err := m.SetDesiredState("app", want); err != nil { + t.Fatalf("SetDesiredState(%q): %v", want, err) + } + if got := LoadAppConfig(dir).DesiredState; got != want { + t.Fatalf("after SetDesiredState(%q), disk says %q", want, got) + } + } +} + +func TestSetDesiredState_RefusesUnknownAndArbitraryValues(t *testing.T) { + m, root := newDSManager(t, "app") + dir := stackDirOf(root, "app") + writeAppYAML(t, dir, "deployed: true\ndesired_state: running\nenv: {}\n") + + for _, bad := range []string{DesiredStateUnknown, "paused", "RUNNING", "true"} { + if err := m.SetDesiredState("app", bad); err == nil { + t.Fatalf("SetDesiredState(%q) was accepted — only running/stopped are writable, and "+ + "'unknown' in particular must be the ABSENCE of a record, never a written value", bad) + } + } + // A refused write must not have touched the file. + if got := LoadAppConfig(dir).DesiredState; got != DesiredStateRunning { + t.Fatalf("a refused write changed the record to %q", got) + } +} + +func TestSetDesiredState_NoAppYAMLIsANoOpNotAnError(t *testing.T) { + // No app.yaml = nothing deployed in that dir. Every consumer gates on Deployed first, so there + // is no intent to record — and returning an error here would refuse a customer's Start on a + // stack that simply is not installed. + m, root := newDSManager(t, "app") + if err := m.SetDesiredState("app", DesiredStateRunning); err != nil { + t.Fatalf("want a silent no-op, got %v", err) + } + if _, err := os.Stat(filepath.Join(stackDirOf(root, "app"), "app.yaml")); !os.IsNotExist(err) { + t.Fatal("an app.yaml was created for a stack that has none") + } +} + +func TestSetDesiredState_UnknownStackIsAnError(t *testing.T) { + m, _ := newDSManager(t) + if err := m.SetDesiredState("ghost", DesiredStateStopped); err == nil { + t.Fatal("SetDesiredState on an unknown stack silently succeeded") + } +} + +// --- Group H (§1.2) — the save path preserves what it is given ---------------------------------- + +func TestSaveAppConfig_PreservesEveryKnownFieldAcrossLoadSave(t *testing.T) { + // THE R-100 SHAPE. SaveAppConfig used to rebuild AppConfig from a five-field struct literal, so + // any field added later was dropped on every save — and nine call sites share this path, so the + // loss would surface far from its cause. DesiredState is exactly such a later field: without the + // copy-and-overlay, a customer's Stop would be erased by the next unrelated app.yaml write (an + // email-toggle change, an optional-config edit, the encryption migration). + // + // RED-PROOF: replace `saveCfg := *cfg` with the old literal + // saveCfg := AppConfig{Deployed: cfg.Deployed, DeployedAt: cfg.DeployedAt, + // Env: ..., LockedFields: cfg.LockedFields, EmailEnabled: cfg.EmailEnabled} + // and this test fails on desired_state. Demonstrated in REPORT.md §5. + dir := t.TempDir() + orig := &AppConfig{ + Deployed: true, + DeployedAt: "2026-08-02T09:00:00Z", + Env: map[string]string{"HDD_PATH": "/mnt/hdd_1", "SUBDOMAIN": "fotok"}, + LockedFields: []string{"HDD_PATH"}, + EmailEnabled: true, + DesiredState: DesiredStateStopped, + } + if err := SaveAppConfig(dir, orig, nil, nil); err != nil { + t.Fatalf("first save: %v", err) + } + + // Load and save again WITHOUT touching anything — the round-trip an unrelated writer performs. + reloaded := LoadAppConfig(dir) + if reloaded == nil { + t.Fatal("load returned nil") + } + if err := SaveAppConfig(dir, reloaded, nil, nil); err != nil { + t.Fatalf("second save: %v", err) + } + + got := LoadAppConfig(dir) + if got.DesiredState != DesiredStateStopped { + t.Fatalf("desired_state was LOST across load→save (got %q) — a customer's Stop would be "+ + "erased by any unrelated app.yaml write", got.DesiredState) + } + if !got.Deployed || got.DeployedAt != orig.DeployedAt || !got.EmailEnabled { + t.Fatalf("a known field was lost across load→save: %+v", got) + } + if len(got.LockedFields) != 1 || got.LockedFields[0] != "HDD_PATH" { + t.Fatalf("locked_fields lost: %v", got.LockedFields) + } + if got.Env["HDD_PATH"] != "/mnt/hdd_1" || got.Env["SUBDOMAIN"] != "fotok" { + t.Fatalf("env lost: %v", got.Env) + } +} + +func TestSaveAppConfig_UnknownYAMLKeysAreDropped(t *testing.T) { + // MEASURED, NOT ASSUMED (§1.2 / §15.12). The answer is NO: app.yaml does not round-trip keys the + // struct does not model, because the trip goes through the struct and yaml.Unmarshal discards + // them before SaveAppConfig is ever reached. + // + // This test exists to make that limitation VISIBLE rather than discovered later. It is not a + // defect introduced here and R-166 does not widen it — but it is the reason every writer must + // load-then-save, and the reason a hand-edited app.yaml annotation will not survive. + dir := t.TempDir() + writeAppYAML(t, dir, "deployed: true\ndesired_state: running\nenv:\n A: b\nfuture_field: keep-me\n") + + cfg := LoadAppConfig(dir) + if cfg == nil { + t.Fatal("load returned nil") + } + if err := SaveAppConfig(dir, cfg, nil, nil); err != nil { + t.Fatalf("save: %v", err) + } + + raw := readAppYAML(t, dir) + if strings.Contains(raw, "future_field") { + t.Fatal("an unknown key SURVIVED — the documented limitation no longer holds; update the " + + "comment on SaveAppConfig and REPORT.md §12, which both state that it does not") + } + // The modelled fields must of course survive. + if got := LoadAppConfig(dir); got.DesiredState != DesiredStateRunning || got.Env["A"] != "b" { + t.Fatalf("a MODELLED field was lost: %+v", got) + } +} + +// --- Scenario D — backfill is running-only, and never invents "stopped" -------------------------- + +func TestBackfillDesiredState_RunningIsRecorded_AmbiguousIsLeftAlone(t *testing.T) { + // Two legacy apps, no desired_state on either. One is observed RUNNING — unambiguous, so its + // intent converges without waiting for a button press. One has ZERO CONTAINERS — the ambiguous + // case that could be a deliberate stop, a power cut or an interrupted deploy, which is precisely + // the inference R-166 exists to remove. It must be left with NO record. + // + // RED-PROOF: delete the `if !isObservedUp(s) { ... continue }` guard in BackfillDesiredState and + // this test fails — the stopped app gets `running` written and would be started at the next boot. + // Demonstrated in REPORT.md §5. + m, root := newDSManager(t, "running-app", "stopped-app") + writeAppYAML(t, stackDirOf(root, "running-app"), "deployed: true\nenv: {}\n") + writeAppYAML(t, stackDirOf(root, "stopped-app"), "deployed: true\nenv: {}\n") + + m.stacks["running-app"].Deployed = true + m.stacks["running-app"].State = StateRunning + m.stacks["running-app"].AppConfig = LoadAppConfig(stackDirOf(root, "running-app")) + m.stacks["stopped-app"].Deployed = true + m.stacks["stopped-app"].State = StateStopped + m.stacks["stopped-app"].AppConfig = LoadAppConfig(stackDirOf(root, "stopped-app")) + + if n := m.BackfillDesiredState(); n != 1 { + t.Fatalf("backfilled %d, want exactly 1", n) + } + + if got := LoadAppConfig(stackDirOf(root, "running-app")).DesiredState; got != DesiredStateRunning { + t.Fatalf("a deployed, RUNNING app was not backfilled: desired_state=%q", got) + } + if got := LoadAppConfig(stackDirOf(root, "stopped-app")).DesiredState; got != DesiredStateUnknown { + t.Fatalf("an AMBIGUOUS app (zero containers) was given desired_state=%q — inferring intent "+ + "from a container count is the exact defect R-166 removes", got) + } +} + +func TestBackfillDesiredState_NeverWritesStopped_AndNeverOverwrites(t *testing.T) { + // Two invariants that must hold no matter what is observed: + // 1. "stopped" is never written by the backfill, from any signal, ever. + // 2. an EXISTING record is never overwritten — the customer's own decision outranks any + // observation, so a stopped-but-somehow-running app keeps its recorded stop. + m, root := newDSManager(t, "exited", "degraded", "restarting", "already-stopped") + for _, n := range []string{"exited", "degraded", "restarting"} { + writeAppYAML(t, stackDirOf(root, n), "deployed: true\nenv: {}\n") + } + writeAppYAML(t, stackDirOf(root, "already-stopped"), "deployed: true\ndesired_state: stopped\nenv: {}\n") + + states := map[string]ContainerState{ + "exited": StateExited, "degraded": StateDegraded, + "restarting": StateRestarting, "already-stopped": StateRunning, + } + for n, st := range states { + m.stacks[n].Deployed = true + m.stacks[n].State = st + m.stacks[n].AppConfig = LoadAppConfig(stackDirOf(root, n)) + } + + m.BackfillDesiredState() + + for _, n := range []string{"exited", "degraded", "restarting"} { + if got := LoadAppConfig(stackDirOf(root, n)).DesiredState; got != DesiredStateUnknown { + t.Fatalf("%s (state=%s) was backfilled to %q — only a POSITIVE up-reading may seed a record", + n, states[n], got) + } + } + if got := LoadAppConfig(stackDirOf(root, "already-stopped")).DesiredState; got != DesiredStateStopped { + t.Fatalf("the backfill OVERWROTE a customer's recorded stop with %q", got) + } +} + +func TestBackfillDesiredState_SkipsProtectedAndUndeployed(t *testing.T) { + m, root := newDSManager(t, "traefik", "not-deployed") + writeAppYAML(t, stackDirOf(root, "traefik"), "deployed: true\nenv: {}\n") + writeAppYAML(t, stackDirOf(root, "not-deployed"), "deployed: false\nenv: {}\n") + + m.stacks["traefik"].Deployed = true + m.stacks["traefik"].Protected = true + m.stacks["traefik"].State = StateRunning + m.stacks["traefik"].AppConfig = LoadAppConfig(stackDirOf(root, "traefik")) + m.stacks["not-deployed"].Deployed = false + m.stacks["not-deployed"].State = StateRunning + m.stacks["not-deployed"].AppConfig = LoadAppConfig(stackDirOf(root, "not-deployed")) + + if n := m.BackfillDesiredState(); n != 0 { + t.Fatalf("backfilled %d, want 0 — protected stacks have their own supervision and an "+ + "undeployed stack has no intent to record", n) + } + if got := LoadAppConfig(stackDirOf(root, "traefik")).DesiredState; got != DesiredStateUnknown { + t.Fatalf("a PROTECTED stack was backfilled to %q", got) + } +} + +// --- The in-memory view keeps step with the disk ------------------------------------------------- + +func TestSetDesiredState_UpdatesTheInMemoryStackToo(t *testing.T) { + // Otherwise a reader between this write and the next ScanStacks sees a stale intent — and the + // dashboard reads GetStacks on every render. + m, root := newDSManager(t, "app") + dir := stackDirOf(root, "app") + writeAppYAML(t, dir, "deployed: true\nenv: {}\n") + m.stacks["app"].Deployed = true + m.stacks["app"].AppConfig = LoadAppConfig(dir) + + if err := m.SetDesiredState("app", DesiredStateStopped); err != nil { + t.Fatal(err) + } + for _, s := range m.GetStacks() { + if s.Name == "app" && DesiredStateOf(s) != DesiredStateStopped { + t.Fatalf("in-memory desired state = %q, disk says stopped", DesiredStateOf(s)) + } + } +} + +func TestDesiredStateOf_NilAppConfigIsUnknown(t *testing.T) { + if got := DesiredStateOf(Stack{Name: "x"}); got != DesiredStateUnknown { + t.Fatalf("a stack with no AppConfig reported desired state %q — absent must read as unknown", got) + } +}