diff --git a/CHANGELOG.md b/CHANGELOG.md index e4a84af..ab1c5e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,93 @@ ## Changelog +### v0.191.0 — warn before the wall comes down (2026-08-02, R-167 · R-158 · R-174) — MinAgent: none + +**Storage monitoring and backup alerts, decision D-c, landing BEFORE the `mp1`→`mp0` merge (D-a / +R-165) rather than with it.** D-a's own condition (2) says the monitoring ships in the same step and +never after, because the merge removes a wall that currently fails safely. Landing it first is +strictly better and costs nothing: the warnings go in and get proven on hardware while the wall is +still standing. **No disk layout is touched in this release.** + +**R-167 — the customer is warned BEFORE a filesystem fills, and the warning is the pair that already +existed.** New `internal/fillwatch`. Nothing warned before this: the first sign of a full filesystem +was a backup that did not happen, and the only related signal — the healthcheck's generic +`health_degraded` at 90% — looked at REGISTERED STORAGE PATHS ONLY, so the docker area (`mp0`) and +the system-data area (`mp1`, which holds every driveless app's retained recovery unit) were invisible, +and it never reported free bytes or named a drive. + +**`disk_warning` / `disk_critical` WERE ALREADY A COMPLETE PIPELINE WITH NO PRODUCER** — allowlisted +in the hub, carrying Hungarian copy, sitting in `settings.DefaultEnabledEvents`, with a UI checkbox +(`event_disk_alerts`) — and `grep` across all four repos found **zero emitters**. The sixth "built but +never wired" instance in this project. This release is their producer; minting a new near-duplicate +type would have left the pair inert forever. + +- **Two threshold terms, whichever trips first** — used ≥ **85%** OR free < **5 GiB** (critical: 95% / + 2 GiB). A percentage alone lies at both ends of this fleet's size range: 85% of a 20 G backup area + leaves 3 G, less than one DB-backed app's recovery unit (up to ~2× its data — measured 21.1 GB → + 40.2 GB, `07-backup-architecture.md` §7.5), while 85% of a 4 TB media drive leaves 600 G. +- **Edge-triggered on ESCALATION ONLY**, state persisted across restarts. De-escalation is silent and + re-arms. **Hysteresis dead zone** between clear (75% / 7 GiB) and warn holds the previous band, so a + filesystem on the line does not flap; the gap is pinned by a test, because a warn and clear + threshold that can be edited into equality is a flapping bug waiting to be introduced. +- **The hub owns cooldown — no controller-side timer** (the `offboxEnlargeBlockedNotify` precedent). +- **A nil usage read is NEVER a warning.** An absent, unmounted or unreadable filesystem is the drive + gate's business and already has its own alert; calling it "full" would be a false alarm with a + misleading cause. It also does not CLEAR an existing warning — a blipping drive must not silently + retract a true alarm. +- **Per FILESYSTEM, never per app** — one full disk holding ten apps would fire ten times, nine of them + noise. Watched: the app-data volume, the system-data volume, and every registered non-decommissioned + drive, de-duplicated by path and resolved at check time (a drive added between checks needs no + restart). Daily at **03:30**, deliberately before the nightly app-data legs so a customer about to + lose a backup to lack of space hears about it with a night's margin. + +**R-158 — the operator hears about a failed per-app backup.** `captureAllRecoveryUnits` logged +`[WARN] Recovery unit capture failed for %s` and stopped there; the manager carried three notify seams +and none for the unit capture. `/backups/apps` is the page a person opens to ask whether ONE app is +backed up, and it was the one page that never said. New `unitNotify` seam + `SetUnitNotify`, fired +**per app with the loop continuing** (one app's failure neither aborts nor silences its siblings), and +carrying the target filesystem's used/free bytes at the moment of failure — the overwhelmingly likely +cause is a full filesystem, and those numbers answer "why" without an operator logging in. `UnitSpace` +is **nil when the filesystem is unreadable** and renders as *"unavailable"*, never as zeros: "0 GB +free" and "we could not look" are opposite diagnoses. + +**It is OPERATOR-TIER (`recovery_unit_capture_failed`), deliberately NOT `backup_failed`.** That type +carries a `customerMessages` entry AND sits in `DefaultEnabledEvents`, so reusing it — which is what +R-158's own proposal said — would email the customer, in Hungarian, that their backup failed, about +something they cannot act on. D-c routes it to the operator and overrides the proposal. Operator-only +is enforced by the hub's `notify.operatorOnlyEvents` register, **not** by the absence of a +`customerMessages` entry; v0.78.0 claimed the latter and was wrong, and there is a red-proof here +demonstrating the customer receiving it when the register entry is removed. + +**R-174 — the app-stop guard stopped starting apps onto missing drives. A regression in v0.189.0 code, +found by review on 2026-08-02 and closed the same session.** `appStopGuard.SetStarter(stackMgr)` +handed `Recover` the RAW stack manager, whose `StartStack` has no drive gate. The guard runs **at +startup** — exactly when an external drive may not have come back — so a backup that stopped an app, +followed by a power cut and a drive that did not remount, ended with the app started onto a missing +drive. **This is R-171 one path over**, and the rule is not new: the API's own +`startGatedByMissingDrive` already refused this to the customer. + +- The starter is now wrapped in `gatedAppStopStarter`, using the **same** drive predicate the boot + sweep uses. `bootDriveGate` could NOT be reused whole and the reason is recorded in the code: its + holder #2 reads `bootAppStopGuard.HeldStacks()`, which during `Recover` is **the guard's own marker** + — it would refuse every recovery it was meant to perform — and holders #1/#2 read package-level vars + assigned *after* `Recover()` runs, so a whole-gate reuse would be correct only by accident of + nil-safety. Holder #3 (the drive) is extracted into `driveStartGate`, which now has two callers and + one implementation; a test pins that `bootDriveGate` keeps delegating to it. +- **A refusal is not a failure.** New `ErrStartRefused` + an `AppStopRecovery.Refused` bucket. Both + keep the marker — the operation is genuinely unfinished — but only `Failed` alarms. Collapsing them + would push a deliberately-held app into `NotifyBackupFailed`, a customer-enabled type, producing + exactly the R-171 false alarm this fix exists to prevent. `main.go` now guards the notify with + `Alarming()` rather than `!= nil`, and the pre-existing seam test was **tightened** to require it. +- Fail-safe per R-171's contract: cannot determine ⇒ do not start. The check stays per-caller and was + deliberately NOT pushed into `Manager.StartStack` (fourteen callers, most legitimate). + +**Tests:** 1157 → **1184** (+27). Every red-proof demonstrated failing and restored — the gate removed +from the guard's starter; the `unitNotify` call removed; the edge trigger removed (fires twice); the +clear thresholds edited into equality; and `recovery_unit_capture_failed` removed from +`operatorOnlyEvents`, which showed the customer receiving an operator event. Seam wirings are pinned by +walking `main.go`'s **AST**, not `strings.Contains` — a red-proof that comments out `SetNotify` fails +the test while the string is still in the file. + ### v0.190.0 — the boot-recovery story finished, and a regression v0.189.0 opened (2026-08-02, R-157 A · R-170 · R-171) **R-171 — a regression introduced by v0.189.0, found by reading the diff and CONFIRMED on hardware diff --git a/REUSE.md b/REUSE.md index b0dfa69..b8678b2 100644 --- a/REUSE.md +++ b/REUSE.md @@ -93,6 +93,7 @@ | `bootrecon.StartGate` (R-171, v0.190.0) | controller/internal/bootrecon/bootrecon.go | `MayStart(stack) (bool, reason)` | THE one question the boot sweep asks before starting anything | **Fail-safe: cannot determine ⇒ return FALSE.** One seam for all three holders (absent drive · quiesce · an in-flight app-data operation) because they differ only in the reason string. Implemented in `main.go` (`bootDriveGate`) reusing `quiesce.SuppressedStacks()`, `AppStopGuard.HeldStacks()` and `Manager.DriveLive` — never re-derive any of them. Held apps go to `Result.HeldByDrive`, **never** `StillDown` (that is the dead-app alarm's bucket) | | the boot settle window (R-157 A, v0.190.0) | controller/cmd/controller/main.go | `bootReconcileSample` / `StableFor` / `Budget` | sample the fleet until it stops changing, then sweep ONCE | **settle + budget + one `DefaultRetryDelay` must stay under `deadAppBootGrace`** — pinned by `TestBootWindow_CommonCaseFitsInsideTheDeadAppGrace`, which is why the budget is 50 s and not 60 s. Sampling is READ-ONLY; sweeping per sample would never see a settled fleet (the sweep's own StartStack changes it). A late recovery is REPORTED (`recordLateRecovery`), never hidden by widening the grace | | `backup.AppStopGuard` (`Begin`/`End`/`Recover`) (R-166, v0.189.0) | controller/internal/backup/appstop_marker.go | `(opID, reason, stacks) error` / `()` / `() *AppStopRecovery` | THE crash marker for stop→work→start windows (volume dump, offbox reconstitute, `.fab` export) | Its **own** file (`appstop-state.json`), never quiesce's — one file, one writer. **A `defer` is NOT the mechanism** (Campaign 8 fault 10: SIGKILL runs no defer); the marker is. Written BEFORE the stop, cleared ONLY after a restart that succeeded; a FAILED restart deliberately KEEPS it. `Recover` RETURNS its outcome rather than notifying, because it must complete before the boot reconciler while the notifier does not exist yet | +| `backup.ErrStartRefused` + `AppStopRecovery.Refused`/`Alarming()` (R-174, v0.191.0) | controller/internal/backup/appstop_marker.go | `errors.Is(err, ErrStartRefused)` / `() bool` | THE refusal-vs-failure split in the app-stop crash recovery | **A gated starter's refusal is NOT a restart failure.** `Recover`'s starter MUST be the gated `gatedAppStopStarter` (cmd/controller/main.go), never the raw `stacks.Manager` — that was the v0.189.0 defect, which started apps onto ABSENT drives at boot (R-171 one path over). A refusal goes to `Refused` (marker KEPT, silent), a real error to `Failed` (marker kept, ALARMS). Collapsing them routes a deliberate hold into `NotifyBackupFailed`, a customer-enabled type — the R-171 false alarm again. `main.go` must guard the notify with `Alarming()`, not `!= nil` | | `Manager.DeleteStack` / `RemoveStack` | controller/internal/stacks/delete.go | `(name, removeHDDData[, backupPaths])` | THE guarded removal paths | Orphan/protected/deploying/running checks + ProtectedHDDPaths filter before any RemoveAll | | `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 | @@ -172,6 +173,8 @@ | `bootstrap.MaybeIngest` / `RefreshConfig` | controller/internal/bootstrap/bootstrap.go | bootstrap.json → controller.yaml | Day-0 + refresh | Overwrites controller.yaml, NEVER settings.json | | `api.GracefulSelfRestart` | controller/internal/api/selfrestart.go | `(logger)` | Controller self-restart | Detached exit; bootstrap unit re-runs the image | | `Settings.AddPendingEvent/DrainPendingEvents` | controller/internal/settings/settings.go | offline event queue | Events while hub unreachable | — | +| `Manager.SetUnitNotify` + `UnitSpace` (R-158/R-167, v0.191.0) | controller/internal/backup/recovery_unit.go | `(func(stack string, err error, *UnitSpace))` | THE per-app Tier-1 recovery-unit capture failure alert — fires PER APP from `captureAllRecoveryUnits`, loop continues | **OPERATOR-TIER** (`recovery_unit_capture_failed`, in the hub's `operatorOnlyEvents`). **NEVER route it to `backup_failed`** — that type is in `DefaultEnabledEvents` and carries Hungarian copy, so it emails the CUSTOMER about a failure they cannot act on (D-c; R-158's own proposal said `backup_failed` and D-c overrides it). `UnitSpace` is **nil when the target filesystem is unreadable** and renders as *"unavailable"*, never as zeros — "0 GB free" and "we could not look" are opposite diagnoses. No controller-side cooldown: the hub owns it | +| `fillwatch.Watcher` (`New`/`SetNotify`/`Check`) (R-167, v0.191.0) | controller/internal/fillwatch/fillwatch.go | `(statePath, logger, targetsFn, usageFn)` → `Check() error` | THE customer fill warning — warns BEFORE a filesystem fills, per FILESYSTEM (never per app: one full disk holding ten apps would fire ten times) | Emits the **pre-existing** `disk_warning`/`disk_critical` pair, which was allowlisted + copy'd + default-enabled with **no producer in any repo** until now — do NOT mint a new type beside it. **Two threshold terms, whichever trips first** (85% / 5 GiB; critical 95% / 2 GiB) because a percentage alone lies at both ends of this fleet's size range. **Edge-triggered on ESCALATION ONLY**, state persisted; de-escalation is silent and re-arms. Hysteresis dead zone between clear (75% / 7 GiB) and warn — pinned by `TestThresholdsKeepTheirHysteresisGap`. **A nil usage read is NEVER a warning** (§8.4). The hub has **no `customerMessages` entry** for either type on purpose — an entry would override the dynamic message and discard the drive label + free space | ### Scheduler / time / UI diff --git a/controller/cmd/controller/appstop_wiring_test.go b/controller/cmd/controller/appstop_wiring_test.go index 2a796a9..05d54f7 100644 --- a/controller/cmd/controller/appstop_wiring_test.go +++ b/controller/cmd/controller/appstop_wiring_test.go @@ -4,6 +4,7 @@ import ( "go/ast" "go/parser" "go/token" + "strings" "testing" ) @@ -140,31 +141,56 @@ func TestMainReportsTheInterruptedOperation(t *testing.T) { } // It must be guarded, not unconditional: a box with nothing to recover must not email an operator // on every single boot. - guarded := false + // + // R-174 STRENGTHENED THIS. `!= nil` alone is no longer sufficient, because Recover now returns a + // non-nil result for a recovery that merely REFUSED starts (an absent data drive) — the drive + // gate working as designed. `NotifyBackupFailed` sends `backup_failed`, which is customer-enabled + // by default (settings.DefaultEnabledEvents), so a nil-only guard would email the customer + // "A biztonsági mentés sikertelen!" about an app nothing is wrong with. The guard must consult + // Alarming(). + guardedByNil, guardedByAlarming := false, 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 - } + carries := false for _, name := range callsInMain(t, ifst.Body) { if name == "NotifyBackupFailed" { - guarded = true + carries = true } } + if !carries { + return true + } + // Walk the whole condition: it may be `a != nil && a.Alarming()`. + ast.Inspect(ifst.Cond, func(c ast.Node) bool { + switch e := c.(type) { + case *ast.BinaryExpr: + if x, ok := e.X.(*ast.Ident); ok && x.Name == "appStopRecovery" && e.Op == token.NEQ { + guardedByNil = true + } + case *ast.CallExpr: + if sel, ok := e.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Alarming" { + if x, ok := sel.X.(*ast.Ident); ok && x.Name == "appStopRecovery" { + guardedByAlarming = true + } + } + } + return true + }) return true }) - if !guarded { - t.Fatal("the interrupted-operation alert is not guarded by `if appStopRecovery != nil` — every " + + if !guardedByNil { + t.Fatal("the interrupted-operation alert is not guarded by `appStopRecovery != nil` — every " + "healthy boot would page the operator about a backup that was never interrupted") } + if !guardedByAlarming { + t.Fatal("the interrupted-operation alert is not guarded by appStopRecovery.Alarming() — a " + + "recovery that only REFUSED starts (drive absent) would be reported through " + + "NotifyBackupFailed, a customer-enabled event type, telling the customer their backup " + + "failed when the drive gate was simply doing its job (R-174)") + } } // --- R-171 seam: the boot drive gate must be WIRED in production ------------------------------- @@ -213,6 +239,200 @@ func TestMainWiresBootDriveGate(t *testing.T) { } } +// --- R-174 seam: the app-stop guard's starter must be GATED in production ----------------------- + +// TestMainWiresGatedAppStopStarter pins Part 0's production wiring. `SetStarter(stackMgr)` — the raw +// manager, which is what shipped in v0.189.0 — compiles, passes every behavioural test in +// internal/backup (they inject their own gating starter), and silently starts apps onto absent +// drives at boot. The ONLY thing that distinguishes the fixed wiring from the broken one is the +// argument at the call site, so that is what this reads. +// +// AST, not strings.Contains: a commented-out call still contains the string. +func TestMainWiresGatedAppStopStarter(t *testing.T) { + body := mainBody(t) + + var arg ast.Expr + found := false + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "SetStarter" || len(call.Args) != 1 { + return true + } + // Only the app-stop guard's SetStarter, not some other type's. + if x, ok := sel.X.(*ast.Ident); !ok || x.Name != "appStopGuard" { + return true + } + arg, found = call.Args[0], true + return false + }) + if !found { + t.Fatal("func main() no longer calls appStopGuard.SetStarter — Recover would find the marker " + + "and be unable to start anything") + } + + // The argument must be a gatedAppStopStarter composite literal. A bare identifier (`stackMgr`) + // is precisely the v0.189.0 defect. + lit, ok := arg.(*ast.CompositeLit) + if !ok { + t.Fatalf("appStopGuard.SetStarter is wired with %T, not a gatedAppStopStarter literal — an "+ + "un-gated starter restarts apps onto MISSING drives at boot (R-174, the R-171 defect one "+ + "path over)", arg) + } + id, ok := lit.Type.(*ast.Ident) + if !ok || id.Name != "gatedAppStopStarter" { + t.Fatalf("appStopGuard.SetStarter is wired with a %v literal, want gatedAppStopStarter", lit.Type) + } + + // And that gate must be a driveStartGate — the SAME predicate the boot sweep uses, so the two + // cannot disagree about whether an app's drive is available. + gated := false + for _, el := range lit.Elts { + kv, ok := el.(*ast.KeyValueExpr) + if !ok { + continue + } + k, ok := kv.Key.(*ast.Ident) + if !ok || k.Name != "gate" { + continue + } + if gl, ok := kv.Value.(*ast.CompositeLit); ok { + if gid, ok := gl.Type.(*ast.Ident); ok && gid.Name == "driveStartGate" { + gated = true + } + } + } + if !gated { + t.Fatal("the app-stop starter's gate is not a driveStartGate — the crash recovery and the " + + "boot sweep would answer \"may this app start?\" from two different implementations, " + + "which is the drift the extraction exists to prevent") + } +} + +// TestBootDriveGateAndAppStopShareTheDrivePredicate pins the OTHER half of the same claim: the boot +// sweep must keep delegating to driveStartGate rather than growing its own copy of the drive checks. +// +// This is the "a comment asserting an invariant needs a test pinning it" rule. The claim — that the +// two gates cannot disagree — is true only while both call the same code. +func TestBootDriveGateAndAppStopShareTheDrivePredicate(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + + var mayStart *ast.FuncDecl + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name != "MayStart" || fn.Recv == nil || len(fn.Recv.List) != 1 { + continue + } + if id, ok := fn.Recv.List[0].Type.(*ast.Ident); ok && id.Name == "bootDriveGate" { + mayStart = fn + } + } + if mayStart == nil { + t.Fatal("bootDriveGate.MayStart not found in main.go") + } + + // It must call through to the shared predicate. + delegates := false + ast.Inspect(mayStart.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "MayStart" { + return true + } + if x, ok := sel.X.(*ast.SelectorExpr); ok && x.Sel.Name == "drive" { + delegates = true + } + return true + }) + if !delegates { + t.Fatal("bootDriveGate.MayStart no longer delegates to the shared driveStartGate — the boot " + + "sweep and the app-stop crash recovery would each carry their own drive logic, and the " + + "two can then disagree about whether an app may start (R-174)") + } +} + +// --- R-158 / R-167 seams: both new alerts must be WIRED in production --------------------------- + +// TestMainWiresTheUnitCaptureAlert pins Part 1's seam. `SetUnitNotify` is nil-safe by design, so an +// unwired seam is not a crash — it is SILENTLY the pre-v0.191.0 behaviour, in which a per-app Tier-1 +// capture failure is a `[WARN]` line and reaches no hub channel at all. Every behavioural test in +// internal/backup injects its own callback and passes with the production wiring gone, which is +// exactly the hole this closes. THIS PROJECT'S COUNT OF "BUILT BUT NEVER WIRED" REACHES FIVE WITH +// R-158 — the defect being fixed here IS an instance of it. +func TestMainWiresTheUnitCaptureAlert(t *testing.T) { + names := callsInMain(t, mainBody(t)) + + if indexOfCall(names, "SetUnitNotify") < 0 { + t.Fatal("func main() no longer calls backupMgr.SetUnitNotify — a per-app recovery-unit " + + "capture failure would reach no hub channel, which is R-158 un-fixed (the seam built " + + "and left disconnected, for the fifth time in this project)") + } + if indexOfCall(names, "NotifyRecoveryUnitCaptureFailed") < 0 { + t.Fatal("main.go no longer calls NotifyRecoveryUnitCaptureFailed — the seam is wired to " + + "something that pushes no event, which looks identical to a working alert from inside " + + "internal/backup") + } +} + +// TestMainWiresTheFillWatcher pins Part 2's seam. Three separate things can be dropped and each one +// silently reverts the customer to "nothing warns before a disk fills": the watcher can go +// unconstructed, its notify can go unwired (the Watcher is nil-safe), or it can never be scheduled. +func TestMainWiresTheFillWatcher(t *testing.T) { + body := mainBody(t) + names := callsInMain(t, body) + + if indexOfCall(names, "New") < 0 || !assignsIdent(body, "fillWatcher") { + t.Fatal("func main() no longer constructs the fill watcher — nothing warns the customer " + + "before a filesystem fills (R-167, decision D-c's customer half)") + } + if indexOfCall(names, "SetNotify") < 0 { + t.Fatal("func main() no longer calls SetNotify on the fill watcher — the Watcher is nil-safe, " + + "so it would run the checks, update its state, log, and tell the CUSTOMER nothing") + } + + // It must actually be scheduled: a watcher nobody calls is a watcher that never fires. + scheduled := false + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || (sel.Sel.Name != "Daily" && sel.Sel.Name != "Every") { + return true + } + lit, ok := call.Args[0].(*ast.BasicLit) + if ok && strings.Contains(lit.Value, "fill-watch") { + scheduled = true + } + return true + }) + if !scheduled { + t.Fatal("the fill watcher is never registered on the scheduler — it would be constructed, " + + "wired, and never run, which is indistinguishable from a filesystem that never fills") + } +} + +// assignsIdent reports whether a block assigns to the named identifier. +func assignsIdent(body *ast.BlockStmt, want string) bool { + for _, n := range assignedIdentsIn(body) { + if n == want { + return true + } + } + return false +} + // assignedIdentsIn returns the names assigned to in a block (plain `=` and `:=`). func assignedIdentsIn(body *ast.BlockStmt) []string { var names []string diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index d677b46..6f166fe 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -33,6 +33,7 @@ import ( cf "gitea.dooplex.hu/admin/felhom-controller/internal/cloudflare" "gitea.dooplex.hu/admin/felhom-controller/internal/config" "gitea.dooplex.hu/admin/felhom-controller/internal/crypto" + "gitea.dooplex.hu/admin/felhom-controller/internal/fillwatch" "gitea.dooplex.hu/admin/felhom-controller/internal/infra" "gitea.dooplex.hu/admin/felhom-controller/internal/integrations" "gitea.dooplex.hu/admin/felhom-controller/internal/mailrelay" @@ -242,8 +243,17 @@ func main() { // 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. + // + // R-174: the starter is GATED, never the raw manager. Recover runs here, at startup — exactly + // when an external drive may not have come back — and `Manager.StartStack` has no drive gate of + // its own, so the un-gated version started apps onto missing drives. The gate is the SAME + // `driveStartGate` the boot sweep uses (holder #3 of bootDriveGate), so the two cannot disagree. appStopGuard := backup.NewAppStopGuard(filepath.Join(cfg.Paths.DataDir, "appstop-state.json"), logger) - appStopGuard.SetStarter(stackMgr) + appStopGuard.SetStarter(gatedAppStopStarter{ + inner: stackMgr, + gate: driveStartGate{mgr: stackMgr, sett: sett}, + logger: logger, + }) appStopRecovery := appStopGuard.Recover() // --- R-166: desired-state backfill (running-only) --- @@ -358,8 +368,14 @@ func main() { // 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 { + // R-174: `Alarming()` and not merely `!= nil`. A recovery that ONLY refused starts is the drive + // gate working as designed, and `backup_failed` is customer-enabled by default — reporting a + // deliberate hold through it would email the customer "A biztonsági mentés sikertelen!" about an + // app nothing is wrong with. The refusal is already logged at WARN with its reason. + if appStopRecovery != nil && appStopRecovery.Alarming() { notifier.NotifyBackupFailed(appStopRecovery.Message(), appStopRecovery.Detail()) + } else if appStopRecovery != nil { + logger.Printf("[WARN] [appstop] %s — not alarming: %s", appStopRecovery.Message(), appStopRecovery.Detail()) } // --- Initialize the app-email SMTP shim (mailrelay) --- @@ -708,6 +724,26 @@ func main() { backupMgr.OffsiteFailureMessage(err, dur)) } }) + // R-158 / R-167 (D-c, operator half): a per-app Tier-1 recovery-unit capture failed. Until + // v0.191.0 this was a `[WARN]` line and nothing else — /backups/apps is the page you open to + // ask whether ONE app is backed up, and it was the one page that never said. Fires per app; + // the capture loop continues, so three failing apps produce three events and one failing app + // does not silence its siblings. + // + // OPERATOR-TIER, not backup_failed — a customer can take no action on a capture failure, and + // backup_failed is customer-enabled by default. The space figures ride along because the + // overwhelmingly likely cause is a full filesystem and they answer "why" without a login. + // No controller-side cooldown: the hub owns it. + backupMgr.SetUnitNotify(func(stackName string, err error, usage *backup.UnitSpace) { + d := notify.RecoveryUnitFailureDetails{App: stackName, Error: err.Error()} + if usage != nil { + d.TargetPath, d.UsedGB, d.AvailGB = usage.Path, usage.UsedGB, usage.AvailGB + d.TotalGB, d.UsedPercent, d.SpaceKnown = usage.TotalGB, usage.UsedPercent, true + } + notifier.NotifyRecoveryUnitCaptureFailed(fmt.Sprintf( + "Recovery unit capture FAILED for %q — the app has no fresh local (Tier-1) backup, and Tier-2/Tier-3 have nothing to copy. %s. Error: %v", + stackName, usage.String(), err), d) + }) // 3a: the pre-push enlargement gate blocked an app's userdata push (config+DB still saved). Edge- // triggered by the engine (only NEW blocks notify), so the hub's per-event-type cooldown suffices — // no controller-side timer (the hub owns cooldown). @@ -749,6 +785,49 @@ func main() { }) } + // --- R-167 (decision D-c, customer half): warn BEFORE a filesystem fills --- + // + // Nothing warned before v0.191.0. The first sign of a full filesystem was a backup that did not + // happen, and the only related signal — the healthcheck's generic `health_degraded` at 90% — looked + // at REGISTERED STORAGE PATHS ONLY, so the docker area and the system-data area (the one holding + // every driveless app's recovery unit) were invisible, and it never reported a free-byte figure or + // named a drive. + // + // CADENCE: DAILY, at 03:30. A fill is a slow-moving quantity — the thing that fills a disk is a + // customer's photo library or a nightly backup, not a spike — so a shorter interval buys no + // earlier warning and only costs statfs calls. 03:30 is deliberately BEFORE the nightly app-data + // legs (db-dump / tier2 / offbox), so a customer who is about to lose a backup to lack of space + // hears about it while there is still a night's margin, rather than after the failure. + // The interval is NOT a cooldown: repeats are impossible because the check is edge-triggered per + // filesystem, and the hub owns cooldown regardless. + fillWatcher := fillwatch.New( + filepath.Join(cfg.Paths.DataDir, "fillwatch-state.json"), logger, + func() []fillwatch.Target { return fillTargets(cfg, sett) }, + func(p string) *fillwatch.Usage { + di := system.GetDiskUsage(p) + if di == nil { + return nil // §8.4 — unreadable is NOT full; never fabricate a zero here + } + return &fillwatch.Usage{ + UsedPercent: di.UsedPercent, AvailGB: di.AvailGB, + UsedGB: di.UsedGB, TotalGB: di.TotalGB, + } + }) + if notifier != nil { + fillWatcher.SetNotify(func(e fillwatch.Event) { + // The CUSTOMER's event — deliberately not operator-only. A customer can free space, + // delete files or add a drive, so this alert is theirs (D-c). The dynamic Hungarian + // message carries the label and the free space; the hub has NO customerMessages entry for + // these two types precisely so the template cannot discard them. + notifier.PushEvent(e.Band.EventType(), e.Band.Severity(), e.Message, map[string]any{ + "path": e.Target.Path, "label": e.Target.Label, + "used_percent": e.Usage.UsedPercent, "avail_gb": e.Usage.AvailGB, + "total_gb": e.Usage.TotalGB, "band": e.Band.String(), + }) + }) + } + sched.Daily("fill-watch", "03:30", func(ctx context.Context) error { return fillWatcher.Check() }) + // --- Central hub reporting schedule --- if hubPusher != nil { if cfg.Hub.Enabled { @@ -1312,7 +1391,7 @@ var bootReconcileFn = func(ctx context.Context, mgr bootrecon.StackProvider, log // R-171: the sweep must not start an app whose data drive is absent. Wired HERE, at the one // place the sweep is constructed, so there is no path that builds an ungated reconciler. if sm, ok := mgr.(*stacks.Manager); ok { - r.SetDriveGate(bootDriveGate{mgr: sm, sett: bootDriveSettings}) + r.SetDriveGate(bootDriveGate{drive: driveStartGate{mgr: sm, sett: bootDriveSettings}}) } return r.Run(ctx) } @@ -1352,8 +1431,7 @@ var ( // // Fail-safe per bootrecon.StartGate's contract: anything that cannot be determined returns false. type bootDriveGate struct { - mgr *stacks.Manager - sett *settings.Settings + drive driveStartGate } func (g bootDriveGate) MayStart(stackName string) (bool, string) { @@ -1367,7 +1445,35 @@ func (g bootDriveGate) MayStart(stackName string) (bool, string) { return false, "an app-data operation is holding it — the app-stop guard restarts it when the operation ends" } } - // 3. the drive + // 3. the drive — the SHARED predicate, so this gate and the app-stop guard's crash recovery + // (R-174) cannot disagree about whether an app's drive is available. + return g.drive.MayStart(stackName) +} + +// driveStartGate is holder #3 of bootDriveGate, EXTRACTED so it has two callers and one +// implementation (R-174). +// +// WHY IT IS SEPARATE FROM bootDriveGate RATHER THAN REUSED WHOLE. The app-stop guard's Recover needs +// exactly this question and NOT the other two holders: +// +// - Holder #2 reads `bootAppStopGuard.HeldStacks()`, which during Recover is the guard's OWN +// marker — the very stacks being recovered. Reusing bootDriveGate there would refuse every +// recovery it was meant to perform, self-referentially. +// - Holders #1 and #2 read package-level vars assigned in main() at the `bootDriveSettings` block, +// which runs AFTER `appStopGuard.Recover()`. They are nil at recovery time, so a whole-gate +// reuse would be correct only by accident of nil-safety — and would silently invert the moment +// anyone moved either line. This project has shipped that class of accident before. +// +// Fail-safe per bootrecon.StartGate's contract: anything that cannot be determined returns false. +type driveStartGate struct { + mgr *stacks.Manager + sett *settings.Settings +} + +func (g driveStartGate) MayStart(stackName string) (bool, string) { + if g.mgr == nil { + return false, "no stack manager wired — the drive cannot be determined" + } cfg := g.mgr.LoadAppConfigByName(stackName) if cfg == nil { // CANNOT DETERMINE. A deployed app whose app.yaml will not load cannot have its drive @@ -1397,6 +1503,76 @@ func (g bootDriveGate) MayStart(stackName string) (bool, string) { return true, "" } +// gatedAppStopStarter is the app-stop guard's starter, wrapped in the drive gate (R-174). +// +// THE DEFECT IT CLOSES, found by review on 2026-08-02 in code shipped 2026-08-01 (v0.189.0): +// `appStopGuard.SetStarter(stackMgr)` handed Recover the RAW stack manager, whose `StartStack` has +// no drive gate. Recover runs at startup — precisely when an external drive may not have come back — +// so a backup that stopped an app, followed by a power cut and a drive that did not remount, ended +// with the app started onto a missing drive. That is R-171 one path over, and the rule is not new: +// the API's own `startGatedByMissingDrive` already refuses this to the customer. +// +// The check stays HERE, per-caller, and is deliberately NOT pushed into `Manager.StartStack` — it has +// fourteen callers and most of them legitimately start apps outside this concern. +type gatedAppStopStarter struct { + inner backup.AppStopStarter + gate driveStartGate + logger *log.Logger +} + +func (s gatedAppStopStarter) StartStack(name string) error { + if ok, why := s.gate.MayStart(name); !ok { + s.logger.Printf("[WARN] [appstop] refusing to restart %q after an interrupted operation: %s", name, why) + return fmt.Errorf("%w: %s", backup.ErrStartRefused, why) + } + return s.inner.StartStack(name) +} + +// fillTargets is §8.1's watch list: the app-data volume, the system-data volume, and every +// registered drive. Resolved at CHECK time, not at startup, so a drive added or decommissioned +// between checks is picked up without a controller restart. +// +// WHY THESE THREE AND NOT ONLY THE REGISTERED DRIVES — which is all the healthcheck ever looked at: +// +// - the APP-DATA volume (`mp0`, `/var/lib/docker`) holds every app's live named volumes; +// - the SYSTEM-DATA volume (`mp1`, `/mnt/sys_drive`) holds the retained recovery unit of every +// DRIVELESS app (architecture/07-backup-architecture.md §7.5) and is the smaller of the two at +// 20 G against 50 G — the very mismatch R-165 exists to remove. It is also the filesystem whose +// silent exhaustion R-158 measured; +// - the registered DRIVES hold the customer's own data. +// +// De-duplicated by path: on a box where a drive is not a separate mount these collapse, and warning +// twice about one filesystem is exactly the noise the per-filesystem rule exists to prevent. A +// DECOMMISSIONED drive is dropped — it is deliberately out of service, and its fill is not news. A +// DISCONNECTED one is kept in the list but will read as unreadable and be skipped by §8.4, which is +// the correct outcome: its absence already has its own alert. +func fillTargets(cfg *config.Config, sett *settings.Settings) []fillwatch.Target { + seen := make(map[string]bool) + var out []fillwatch.Target + add := func(path, label string) { + if path == "" || seen[path] { + return + } + seen[path] = true + out = append(out, fillwatch.Target{Path: path, Label: label}) + } + + // The docker data-root as seen from inside the guest, and the system-data area. + add(system.DockerVolumePath, "Alkalmazások területe") + if cfg != nil { + add(cfg.Paths.SystemDataPath, "Rendszer- és mentési terület") + } + if sett != nil { + for _, sp := range sett.GetStoragePaths() { + if sp.Decommissioned { + continue + } + add(sp.Path, sp.Label) + } + } + return fillwatch.SortTargets(out) +} + // bootFleetSample is a comparable snapshot of one deployed app — name, state and container count, // per §8.1. Container count is in it deliberately: a stack can go from 3 containers to 0 without its // aggregate state changing, and that IS the boot still moving. diff --git a/controller/internal/backup/appstop_drivegate_test.go b/controller/internal/backup/appstop_drivegate_test.go new file mode 100644 index 0000000..221379a --- /dev/null +++ b/controller/internal/backup/appstop_drivegate_test.go @@ -0,0 +1,229 @@ +package backup + +import ( + "errors" + "fmt" + "io" + "log" + "path/filepath" + "strings" + "testing" +) + +// R-174 — the app-stop guard's crash recovery must not start an app onto a MISSING drive. +// +// The defect these pin, found by review on 2026-08-02 in code shipped 2026-08-01 (v0.189.0): +// `appStopGuard.SetStarter(stackMgr)` handed Recover the raw stack manager, whose `StartStack` has +// no drive gate. Recover runs at STARTUP — exactly when an external drive may not have come back — +// so a backup that stopped an app, followed by a power cut and a drive that did not remount, ended +// with the app started onto a missing drive. R-171 one path over. +// +// THE SEAM UNDER TEST IS THE STARTER, not the gate: `internal/backup` must not import `stacks` or +// `settings`, so the production gate lives in `cmd/controller`. What is pinned here is the contract +// between them — that a starter returning ErrStartRefused produces a REFUSAL (marker kept, no alarm) +// and not a FAILURE. The production wiring itself is pinned by TestMainWiresGatedAppStopStarter. + +// gatingStarter is a starter whose gate refuses a named set of apps, in the shape the production +// `gatedAppStopStarter` uses: refuse BEFORE calling through, and wrap ErrStartRefused with a reason. +type gatingStarter struct { + inner *fakeStarter + refuse map[string]string // app → reason + refused []string +} + +func (s *gatingStarter) StartStack(name string) error { + if why, ok := s.refuse[name]; ok { + s.refused = append(s.refused, name) + return fmt.Errorf("%w: %s", ErrStartRefused, why) + } + return s.inner.StartStack(name) +} + +func newGatedGuard(t *testing.T, dir string, refuse map[string]string) (*AppStopGuard, *gatingStarter) { + t.Helper() + s := &gatingStarter{inner: &fakeStarter{}, refuse: refuse} + g := NewAppStopGuard(filepath.Join(dir, "appstop-state.json"), log.New(io.Discard, "", 0)) + g.SetStarter(s) + return g, s +} + +// --- Scenario A — the guard does not start an app onto a missing drive --------------------------- + +func TestRecover_DriveAbsent_RefusesTheStartAndKEEPSTheMarker(t *testing.T) { + dir := t.TempDir() + + // process 1: a volume dump stops immich, then the box loses power. No End(), no defer. + g1, _ := newGatedGuard(t, dir, nil) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatalf("Begin: %v", err) + } + // — and immich's drive does NOT come back. + + // process 2: a fresh controller starts. The drive is absent. + g2, starter := newGatedGuard(t, dir, map[string]string{ + "immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint", + }) + res := g2.Recover() + + if len(starter.inner.starts) != 0 { + t.Fatalf("started %v — the app was started onto a MISSING drive, which is the whole defect", + starter.inner.starts) + } + if res == nil { + t.Fatal("Recover returned nil — the refusal is invisible to the caller, so nothing can report it") + } + if len(res.Refused) != 1 || res.Refused[0] != "immich" { + t.Fatalf("refused=%v, want [immich]", res.Refused) + } + if len(res.Failed) != 0 { + t.Fatalf("failed=%v — a deliberate hold was recorded as a FAILURE. That bucket reaches "+ + "NotifyBackupFailed, which is customer-enabled by default, so the customer would be "+ + "emailed \"A biztonsági mentés sikertelen!\" about an app nothing is wrong with (R-171's "+ + "false-alarm shape one path over)", res.Failed) + } + if !markerExists(t, dir) { + t.Fatal("the marker was CLEARED after a refused start — the operation is genuinely " + + "unfinished, and clearing it erases the only durable record that immich is owed a restart") + } + // The refusal must name the app AND the reason, or an operator cannot act on it. + if d := res.Detail(); !strings.Contains(d, "held_by_drive") || !strings.Contains(d, "immich") { + t.Fatalf("detail %q does not name the held app", d) + } + if msg := res.Message(); !strings.Contains(msg, "HELD") || !strings.Contains(msg, "drive") { + t.Fatalf("operator message %q does not say the app is held by an absent drive", msg) + } +} + +// A refusal-only recovery MUST NOT alarm. This is the assertion that keeps the fix from being the +// bug it fixes: the drive gate doing its job is not a backup failure. +func TestRecover_RefusalOnly_IsNotAlarming(t *testing.T) { + dir := t.TempDir() + g1, _ := newGatedGuard(t, dir, nil) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatal(err) + } + g2, _ := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"}) + res := g2.Recover() + + if res.Alarming() { + t.Fatal("a recovery that only REFUSED starts reports as alarming — main.go would push it " + + "through NotifyBackupFailed and email the customer about a working drive gate") + } +} + +// A genuine failure alongside a refusal still alarms, and the two stay in different buckets. +func TestRecover_FailureAlongsideRefusal_StillAlarmsAndKeepsThemApart(t *testing.T) { + dir := t.TempDir() + g1, _ := newGatedGuard(t, dir, nil) + if err := g1.Begin("volume-dump:batch", ReasonVolumeDump, []string{"immich", "nextcloud", "homebox"}); err != nil { + t.Fatal(err) + } + + g2, starter := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"}) + starter.inner.failWith = map[string]error{"nextcloud": errors.New("compose up: no such image")} + res := g2.Recover() + + if len(res.Refused) != 1 || res.Refused[0] != "immich" { + t.Fatalf("refused=%v, want [immich]", res.Refused) + } + if len(res.Failed) != 1 || res.Failed[0] != "nextcloud" { + t.Fatalf("failed=%v, want [nextcloud]", res.Failed) + } + if len(res.Restarted) != 1 || res.Restarted[0] != "homebox" { + t.Fatalf("restarted=%v, want [homebox] — neither a refusal nor a failure may abort the loop", + res.Restarted) + } + if !res.Alarming() { + t.Fatal("a genuine restart FAILURE alongside a refusal no longer alarms — the refusal " + + "swallowed a real fault") + } + if !markerExists(t, dir) { + t.Fatal("the marker was cleared with work still owed") + } + // The message must not let the held app inflate the failure count. + msg := res.Message() + if !strings.Contains(msg, "1 of 2 app(s) could NOT be restarted") { + t.Fatalf("operator message %q miscounts: the held app must not be counted as a failure", msg) + } + if !strings.Contains(msg, "not counted as failures") { + t.Fatalf("operator message %q does not disclose the held app at all", msg) + } +} + +// --- Scenario B — a live drive still recovers normally, byte-identical to before ----------------- + +func TestRecover_DriveLive_RecoversExactlyAsBefore(t *testing.T) { + dir := t.TempDir() + g1, _ := newGatedGuard(t, dir, nil) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich", "nextcloud"}); err != nil { + t.Fatal(err) + } + + // Nothing refused — the gate says yes for both. + g2, starter := newGatedGuard(t, dir, nil) + res := g2.Recover() + + if len(starter.inner.starts) != 2 { + t.Fatalf("started %v, want both apps — the new gate refused a LEGITIMATE recovery", + starter.inner.starts) + } + if len(res.Refused) != 0 || len(res.Failed) != 0 { + t.Fatalf("refused=%v failed=%v, want neither on a live drive", res.Refused, res.Failed) + } + if len(res.Restarted) != 2 { + t.Fatalf("restarted=%v, want both", res.Restarted) + } + if markerExists(t, dir) { + t.Fatal("the marker survived a fully successful recovery — the next boot would restart the apps again") + } + if !res.Alarming() { + t.Fatal("a successful recovery no longer reports to the operator — the interrupted operation " + + "itself is what §2.4 wants reported, and it went silent") + } +} + +// The next startup, with the drive back, completes the recovery and clears the marker. This is what +// makes "keep the marker" a recovery rather than a leak. +func TestRecover_HeldAppIsRestartedOnceTheDriveReturns(t *testing.T) { + dir := t.TempDir() + g1, _ := newGatedGuard(t, dir, nil) + if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil { + t.Fatal(err) + } + + // Boot 1 — drive absent: refused, marker kept. + g2, _ := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"}) + if res := g2.Recover(); len(res.Refused) != 1 { + t.Fatalf("boot 1 refused=%v, want [immich]", res.Refused) + } + if !markerExists(t, dir) { + t.Fatal("boot 1 cleared the marker — boot 2 has nothing to act on and immich stays down forever") + } + + // Boot 2 — the drive is back. + g3, starter := newGatedGuard(t, dir, nil) + res := g3.Recover() + if len(starter.inner.starts) != 1 || starter.inner.starts[0] != "immich" { + t.Fatalf("boot 2 started %v, want [immich] — the held app was never picked up again", + starter.inner.starts) + } + if len(res.Restarted) != 1 { + t.Fatalf("boot 2 restarted=%v, want [immich]", res.Restarted) + } + if markerExists(t, dir) { + t.Fatal("boot 2 kept the marker after a fully successful recovery") + } +} + +// ErrStartRefused must be matched with errors.Is, i.e. it survives wrapping. A starter that returns +// a bare string reason would land in Failed and alarm — the exact collapse this type prevents. +func TestErrStartRefused_SurvivesWrapping(t *testing.T) { + err := fmt.Errorf("%w: drive /mnt/felhom-drives/hdd_1 is not a live mountpoint", ErrStartRefused) + if !errors.Is(err, ErrStartRefused) { + t.Fatal("a wrapped ErrStartRefused is no longer matched by errors.Is — every refusal would " + + "be recorded as a restart failure and alarm the customer") + } + if errors.Is(errors.New("compose up: no such image"), ErrStartRefused) { + t.Fatal("an ordinary restart failure matches ErrStartRefused — real faults would go silent") + } +} diff --git a/controller/internal/backup/appstop_marker.go b/controller/internal/backup/appstop_marker.go index afd38d1..e5406ef 100644 --- a/controller/internal/backup/appstop_marker.go +++ b/controller/internal/backup/appstop_marker.go @@ -2,6 +2,7 @@ package backup import ( "encoding/json" + "errors" "fmt" "log" "os" @@ -72,10 +73,27 @@ type AppStopMarker struct { // 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). +// +// R-174: production MUST pass a GATED starter, never the raw stack manager. Recover runs at STARTUP — +// exactly when an external drive may not have come back — and `Manager.StartStack` has no drive gate +// of its own. See `gatedAppStopStarter` in cmd/controller/main.go. type AppStopStarter interface { StartStack(name string) error } +// ErrStartRefused is what a gated starter returns when a DELIBERATE HOLDER — today the drive gate — +// says an app must not be started. Wrap it (`fmt.Errorf("%w: …", ErrStartRefused)`) so the reason +// survives; Recover matches with errors.Is. +// +// IT IS NOT A FAILURE, AND THE DISTINCTION IS THE WHOLE POINT OF THE TYPE. A refusal means the +// holder is doing its job and owns the restart; a failure means the restart was attempted and broke. +// Collapsing the two would put a deliberately-held app into `Failed`, which main.go reports through +// `NotifyBackupFailed` — a type that is customer-enabled by default (`settings.DefaultEnabledEvents`) +// and carries the Hungarian "A biztonsági mentés sikertelen!". That is R-171's defect one path over: +// a false alarm about an app the drive gate is deliberately holding. Both buckets keep the marker; +// only `Failed` alarms. +var ErrStartRefused = errors.New("start refused by a deliberate holder") + // 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. @@ -98,7 +116,21 @@ type AppStopRecovery struct { 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) + Failed []string // apps whose restart was ATTEMPTED and broke (the marker was kept for these) + // Refused are apps a deliberate holder said must not start — today, an absent data drive + // (R-174). The marker is kept for these too, but they are NOT a fault and MUST NOT alarm: the + // holder owns the restart. Separate from Failed for the reason recorded on ErrStartRefused. + Refused []string +} + +// Alarming reports whether this recovery is worth paging an operator about. A recovery that only +// REFUSED starts is the drive gate working as designed, and reporting it through the customer-enabled +// `backup_failed` type would be the R-171 false alarm one path over. +func (r *AppStopRecovery) Alarming() bool { + if r == nil { + return false + } + return len(r.Failed) > 0 || len(r.Restarted) > 0 } // Message is the operator-facing headline for an interrupted operation. @@ -107,11 +139,23 @@ func (r *AppStopRecovery) Message() string { 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", + m := 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)) + if len(r.Refused) > 0 { + m += fmt.Sprintf(" (a further %d are held by an absent drive and are not counted as failures)", len(r.Refused)) + } + return m } - return fmt.Sprintf("%s was interrupted by a controller restart — %d app(s) were left stopped and have been restarted", + if len(r.Refused) > 0 && len(r.Restarted) == 0 { + return fmt.Sprintf("%s was interrupted by a controller restart — %d app(s) are left stopped and HELD: their data drive is not available, so the drive gate restarts them when it returns", + r.Reason.humanReason(), len(r.Refused)) + } + m := 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)) + if len(r.Refused) > 0 { + m += fmt.Sprintf("; %d more are held by an absent drive", len(r.Refused)) + } + return m } // Detail is the machine-readable tail. App/stack NAMES only — never env values (§9.5). @@ -124,6 +168,9 @@ func (r *AppStopRecovery) Detail() string { if len(r.Failed) > 0 { d += fmt.Sprintf(" restart_failed=%v", r.Failed) } + if len(r.Refused) > 0 { + d += fmt.Sprintf(" held_by_drive=%v", r.Refused) + } return d } @@ -210,6 +257,15 @@ func (g *AppStopGuard) Recover() *AppStopRecovery { res := &AppStopRecovery{Reason: m.Reason, OpID: m.OpID, StartedAt: m.StartedAt} for _, name := range m.Stacks { if err := g.starter.StartStack(name); err != nil { + // R-174: a REFUSAL is not a failure. The starter's gate has said this app must not be + // started (an absent data drive), so the app is left down deliberately and the holder + // owns the restart. Logged at WARN with the reason, and kept out of Failed so it never + // reaches the customer-enabled backup_failed alarm — see ErrStartRefused. + if errors.Is(err, ErrStartRefused) { + g.logger.Printf("[WARN] [appstop] crash recovery: NOT restarting %s — %v; the marker is KEPT and the holder owns the restart", name, err) + res.Refused = append(res.Refused, name) + continue + } g.logger.Printf("[ERROR] [appstop] crash recovery: restart %s failed: %v", name, err) res.Failed = append(res.Failed, name) continue @@ -218,13 +274,23 @@ func (g *AppStopGuard) Recover() *AppStopRecovery { res.Restarted = append(res.Restarted, name) } sort.Strings(res.Failed) + sort.Strings(res.Refused) sort.Strings(res.Restarted) + // The marker is kept for BOTH unfinished outcomes, for the same reason and with different + // urgency: a failed restart is retried next startup, and a refused one is genuinely unfinished + // until its drive returns. Clearing it in either case would erase the only durable record that + // an app is owed a restart. 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 } + if len(res.Refused) > 0 { + g.logger.Printf("[WARN] [appstop] crash recovery: %d app(s) were deliberately NOT restarted (drive absent) — KEEPING the marker; this is the gate working, not a fault: %v", + len(res.Refused), res.Refused) + return res + } g.End() return res } diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index fb54315..6d1fc5a 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -32,6 +32,27 @@ 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) + // unitNotify (R-158 / R-167), if set, is called ONCE PER APP whose Tier-1 recovery-unit capture + // FAILED, and the capture loop continues to the next app. Wired in cmd/controller/main.go. + // + // WHY IT EXISTS. `/backups/apps` is the page a person opens to ask whether ONE app is backed up, + // and until now it was the one page that never said: a per-app capture failure was a `[WARN]` + // line and went no further. The manager had three notify seams and none for the unit capture — + // the FIFTH instance in this project of a mechanism built and left disconnected. + // + // IT CARRIES THE SPACE FIGURES DELIBERATELY. The overwhelmingly likely cause is a full + // filesystem, and an operator who has the used/free bytes at the moment of failure can act + // without logging in. It is the same pair of numbers the customer-facing fill warning reports, + // which is why the two ship together. + // + // OPERATOR-TIER. Routed to a hub event type that is in `notify.operatorOnlyEvents` — a customer + // can take no action on a capture failure. Deliberately NOT `backup_failed`, which is + // customer-enabled by default and would email them in Hungarian about it (D-c). + // + // NO CONTROLLER-SIDE COOLDOWN — the hub owns cooldown, per the offboxEnlargeBlockedNotify + // precedent. + unitNotify func(stackName string, err error, usage *UnitSpace) + // 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 diff --git a/controller/internal/backup/recovery_unit.go b/controller/internal/backup/recovery_unit.go index 98875b3..2d8a3ec 100644 --- a/controller/internal/backup/recovery_unit.go +++ b/controller/internal/backup/recovery_unit.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "gitea.dooplex.hu/admin/felhom-controller/internal/system" "gopkg.in/yaml.v3" ) @@ -196,8 +197,54 @@ func (m *Manager) CaptureRecoveryUnit(stackName string) error { return nil } +// UnitSpace is the target filesystem's occupancy at the moment a capture failed — the numbers that +// answer "why" without an operator logging in. Nil when the filesystem could not be read at all +// (system.GetDiskUsage returns nil on error), which is reported as unknown rather than as full. +type UnitSpace struct { + Path string + UsedGB float64 + AvailGB float64 + TotalGB float64 + UsedPercent float64 +} + +// String renders the space figures for an operator, or says plainly that they are unknown. An absent +// reading must never render as zeros — "0 GB free" and "we could not look" are opposite diagnoses. +func (u *UnitSpace) String() string { + if u == nil { + return "target filesystem usage unavailable" + } + return fmt.Sprintf("%s: %.1f/%.1f GB used (%.0f%%), %.1f GB free", + u.Path, u.UsedGB, u.TotalGB, u.UsedPercent, u.AvailGB) +} + +// SetUnitNotify wires the per-app recovery-unit capture failure alert (R-158 / R-167). INIT-ONLY — +// call once at startup, in main.go, alongside SetOffboxNotify. Nil-safe: an unwired seam is silently +// the pre-v0.191.0 behaviour, which is a `[WARN]` line and nothing else. +func (m *Manager) SetUnitNotify(fn func(stackName string, err error, usage *UnitSpace)) { + m.unitNotify = fn +} + +// unitTargetSpace reads the occupancy of the filesystem a unit for `stackName` would be written to. +// Nil on an unreadable path — never a fabricated zero (§8.4: an unreadable filesystem is not a full +// one, and the drive gate already owns the absent-drive case). +func (m *Manager) unitTargetSpace(stackName string) *UnitSpace { + path := m.GetAppDrivePath(stackName) + if path == "" { + return nil + } + di := system.GetDiskUsage(path) + if di == nil { + return nil + } + return &UnitSpace{ + Path: path, UsedGB: di.UsedGB, AvailGB: di.AvailGB, + TotalGB: di.TotalGB, UsedPercent: di.UsedPercent, + } +} + // captureAllRecoveryUnits refreshes the recovery unit for every deployed stack. Best-effort: -// a per-app failure is logged and does not abort the others. +// a per-app failure is logged, NOTIFIED (R-158), and does not abort the others. func (m *Manager) captureAllRecoveryUnits() { if m.stackProvider == nil { return @@ -209,6 +256,12 @@ func (m *Manager) captureAllRecoveryUnits() { } if err := m.CaptureRecoveryUnit(stack.Name); err != nil { m.logger.Printf("[WARN] [backup] Recovery unit capture failed for %s: %v", stack.Name, err) + // R-158: per app, and the loop CONTINUES — one app's failure must not silence the + // others, and it must not abort their captures either. The space figures are read at + // the moment of failure, because the point is to answer "why" (usually: no room). + if m.unitNotify != nil { + m.unitNotify(stack.Name, err, m.unitTargetSpace(stack.Name)) + } } } } diff --git a/controller/internal/backup/recovery_unit_notify_test.go b/controller/internal/backup/recovery_unit_notify_test.go new file mode 100644 index 0000000..40231af --- /dev/null +++ b/controller/internal/backup/recovery_unit_notify_test.go @@ -0,0 +1,182 @@ +package backup + +import ( + "io" + "log" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appbackup" +) + +// R-158 / R-167 (D-c, operator half) — a per-app Tier-1 recovery-unit capture failure must reach a +// hub channel. +// +// THE GAP THESE CLOSE. `captureAllRecoveryUnits` logged `[WARN] Recovery unit capture failed for %s` +// and stopped there. The manager carried three notify seams — tier2Notify, offboxNotify, +// offboxEnlargeBlockedNotify — and none for the unit capture, so the one page a person opens to ask +// whether ONE app is backed up (`/backups/apps`) was the one page that never said. Fifth instance in +// this project of a mechanism built and left disconnected. + +// unitFailProvider lists a fixed set of stacks and refuses GetStackRecoveryInfo for the named ones, +// which is the earliest real failure inside CaptureRecoveryUnit ("stack %q not found"). +type unitFailProvider struct { + stacks []string + fail map[string]bool + dir string +} + +func (p *unitFailProvider) GetStackComposePath(string) (string, bool) { return "", false } +func (p *unitFailProvider) ListDeployedStacks() []StackSummary { + out := make([]StackSummary, 0, len(p.stacks)) + for _, s := range p.stacks { + out = append(out, StackSummary{Name: s}) + } + return out +} +func (p *unitFailProvider) GetStackHDDMounts(string) []string { return nil } +func (p *unitFailProvider) GetStackHDDPath(string) string { return "" } +func (p *unitFailProvider) GetImportRoot() string { return "" } +func (p *unitFailProvider) GetDockerVolumes(string) []string { return nil } +func (p *unitFailProvider) StopStack(string) error { return nil } +func (p *unitFailProvider) StartStack(string) error { return nil } +func (p *unitFailProvider) RefreshAndIsRunning(string) bool { return true } +func (p *unitFailProvider) GetStackRecoveryInfo(name string) (RecoveryInfo, bool) { + if p.fail[name] { + return RecoveryInfo{}, false + } + return RecoveryInfo{StackDir: filepath.Join(p.dir, "stacks", name)}, true +} +func (p *unitFailProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } +func (p *unitFailProvider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { + return nil +} +func (p *unitFailProvider) StartStackServices(string, []string) error { return nil } +func (p *unitFailProvider) GetStackClassifiedBinds(string) ([]appbackup.ClassifiedBind, bool) { + return nil, false +} + +var _ appbackup.StackDataProvider = (*unitFailProvider)(nil) + +type unitEvent struct { + app string + err string + usage *UnitSpace +} + +func newUnitNotifyManager(t *testing.T, stacks []string, fail map[string]bool) (*Manager, *[]unitEvent) { + t.Helper() + dir := t.TempDir() + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: dir, + stackProvider: &unitFailProvider{stacks: stacks, fail: fail, dir: dir}, + } + var got []unitEvent + m.SetUnitNotify(func(name string, err error, usage *UnitSpace) { + got = append(got, unitEvent{app: name, err: err.Error(), usage: usage}) + }) + return m, &got +} + +// --- Scenario C — a local unit capture failure reaches the operator ------------------------------ + +func TestCaptureAll_FailureNotifiesOnceWithTheSpaceFigures(t *testing.T) { + m, got := newUnitNotifyManager(t, []string{"immich"}, map[string]bool{"immich": true}) + + m.captureAllRecoveryUnits() + + if len(*got) != 1 { + t.Fatalf("got %d unit-failure events, want exactly 1 — a per-app Tier-1 capture failure "+ + "reached no hub channel, which is the R-158 gap un-fixed", len(*got)) + } + e := (*got)[0] + if e.app != "immich" { + t.Fatalf("event names app %q, want immich — an operator cannot act on an unnamed app", e.app) + } + if e.err == "" { + t.Fatal("the event carries no error — the operator is told a capture failed but not why") + } + // The space figures are the point: the overwhelmingly likely cause is a full filesystem, and + // these answer "why" without an operator logging in. + if e.usage == nil { + t.Fatal("the event carries no space figures for a readable target filesystem — this is the " + + "pair of numbers that makes the alert actionable, and the same pair the customer fill " + + "warning reports (which is why the two ship together)") + } + if e.usage.Path == "" || e.usage.TotalGB <= 0 { + t.Fatalf("space figures are not populated: %+v", e.usage) + } +} + +// --- Scenario D — one failing app does not silence the others ------------------------------------ + +func TestCaptureAll_OneFailureDoesNotAbortOrDuplicate(t *testing.T) { + m, got := newUnitNotifyManager(t, + []string{"homebox", "immich", "nextcloud"}, + map[string]bool{"immich": true}) + + m.captureAllRecoveryUnits() + + if len(*got) != 1 { + t.Fatalf("got %d events, want exactly 1 — either the loop ABORTED on the middle app "+ + "(and its siblings were never captured), or one failure produced several events: %+v", + len(*got), *got) + } + if (*got)[0].app != "immich" { + t.Fatalf("event names %q, want immich", (*got)[0].app) + } + + // The siblings must have been ATTEMPTED after the failure — a positive observable, not the + // absence of an event. The provider records nothing, so assert via the failure set instead: + // flip the LAST app to failing and require both events. + m2, got2 := newUnitNotifyManager(t, + []string{"homebox", "immich", "nextcloud"}, + map[string]bool{"immich": true, "nextcloud": true}) + m2.captureAllRecoveryUnits() + if len(*got2) != 2 { + t.Fatalf("got %d events, want 2 — the app AFTER the first failure was never reached, so the "+ + "loop is aborting rather than continuing: %+v", len(*got2), *got2) + } + if (*got2)[0].app != "immich" || (*got2)[1].app != "nextcloud" { + t.Fatalf("events %+v, want immich then nextcloud in loop order", *got2) + } +} + +// A successful capture must be SILENT. An alert that fires on success is an alert an operator learns +// to ignore. +func TestCaptureAll_SuccessIsSilent(t *testing.T) { + m, got := newUnitNotifyManager(t, []string{"homebox"}, nil) + m.captureAllRecoveryUnits() + if len(*got) != 0 { + t.Fatalf("a successful capture fired %d event(s): %+v", len(*got), *got) + } +} + +// The seam must be nil-safe: an unwired notify is the pre-v0.191.0 behaviour (a WARN line), never a +// panic that takes the whole nightly backup down with it. +func TestCaptureAll_UnwiredNotifyDoesNotPanic(t *testing.T) { + dir := t.TempDir() + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: dir, + stackProvider: &unitFailProvider{stacks: []string{"immich"}, fail: map[string]bool{"immich": true}, dir: dir}, + } + m.captureAllRecoveryUnits() // no SetUnitNotify — must not panic +} + +// §8.4 in the failure direction: an unreadable target filesystem is reported as UNKNOWN, never as +// zeros. "0 GB free" and "we could not look" are opposite diagnoses, and rendering the second as the +// first is the presence-is-not-success trap pointing the other way. +func TestUnitSpace_NilRendersAsUnavailableNotZero(t *testing.T) { + var u *UnitSpace + s := u.String() + if !strings.Contains(s, "unavailable") { + t.Fatalf("nil UnitSpace renders as %q — it must say the reading is unavailable", s) + } + if strings.Contains(s, "0.0") { + t.Fatalf("nil UnitSpace renders zeros (%q) — an operator would read \"the disk is full\" "+ + "from a filesystem nobody could read", s) + } +} diff --git a/controller/internal/fillwatch/fillwatch.go b/controller/internal/fillwatch/fillwatch.go new file mode 100644 index 0000000..04230e2 --- /dev/null +++ b/controller/internal/fillwatch/fillwatch.go @@ -0,0 +1,373 @@ +// Package fillwatch warns the CUSTOMER that a filesystem is filling, BEFORE anything fails. +// +// R-167, operator decision D-c (2026-08-02, felhom.eu CONTEXT.md S-5). Nothing warned before this: +// the first sign of a full filesystem was a backup that did not happen. The healthcheck folded disk +// pressure into a generic `health_degraded` at 90%, for REGISTERED STORAGE PATHS ONLY — it never +// looked at the docker area or the system-data area, never reported free bytes, and never named the +// drive. +// +// WHY IT SHIPS BEFORE THE mp1→mp0 MERGE (D-a / R-165), not with it. Today an app that outgrows the +// 20 G backup area is refused per app with its last good recovery unit preserved byte-identical — +// a wall, but one that fails safely, one app at a time. The merge removes that wall so backups share +// the large area. D-a's own condition (2) says the monitoring lands in the same step and never after; +// landing it FIRST is strictly better and costs nothing, so the warnings go in and get proven on real +// hardware while the wall is still standing. +// +// WHY THE FILESYSTEM IS THE UNIT AND NOT THE APP. One full disk holding ten apps would produce ten +// identical warnings, nine of them noise. The customer's action — free space, delete files, add a +// drive — is per filesystem too. +package fillwatch + +import ( + "encoding/json" + "fmt" + "log" + "os" + "path/filepath" + "sort" + "strings" + "sync" +) + +// ── Thresholds ─────────────────────────────────────────────────────────────────────────────────── +// +// TWO TERMS, AND WHICHEVER TRIPS FIRST WINS, because a percentage alone lies at both ends of the +// size range this fleet actually has: 85% of a 20 G backup area leaves 3 G — not enough for one +// DB-backed app's recovery unit, which is up to ~2× its data (measured: 21.1 GB → 40.2 GB, +// architecture/07-backup-architecture.md §7.5) — while 85% of a 4 TB media drive leaves 600 G and is +// nothing to write home about. A free-bytes floor catches the first; a percentage catches the second. +const ( + // WarnUsedPercent / WarnFreeGiB — enter the warning band. + WarnUsedPercent = 85.0 + WarnFreeGiB = 5.0 + + // CritUsedPercent / CritFreeGiB — enter the critical band. Below 2 GiB a volume tar of almost any + // real app fails, so this is "the next backup will not complete", not "it is getting tight". + CritUsedPercent = 95.0 + CritFreeGiB = 2.0 + + // ClearUsedPercent / ClearFreeGiB — the RETURN threshold, deliberately well below the warn pair. + // Both must hold. Between clear and warn is a dead zone in which the previous band is HELD, so a + // filesystem hovering on the line does not flap between warned and cleared. The gap is pinned by + // TestThresholdsKeepTheirHysteresisGap — a warn and clear threshold that can be edited into + // equality is a flapping bug waiting to be introduced. + ClearUsedPercent = 75.0 + ClearFreeGiB = 7.0 +) + +// Band is a filesystem's fill state. Ordered: escalation is an increase. +type Band int + +const ( + BandOK Band = iota + BandWarning + BandCritical +) + +func (b Band) String() string { + switch b { + case BandWarning: + return "warning" + case BandCritical: + return "critical" + default: + return "ok" + } +} + +// EventType maps a band to the hub event type it emits. +// +// These two types already existed in the hub's allowedEventTypes, already carried Hungarian copy, +// already sat in the controller's DefaultEnabledEvents and already had a UI checkbox — and NOTHING +// IN ANY REPO EMITTED THEM. A complete customer pipeline with no producer, the sixth "built but +// never wired" instance in this project. This package is that producer; a new near-duplicate event +// type would have left the pair inert forever. +func (b Band) EventType() string { + switch b { + case BandCritical: + return "disk_critical" + case BandWarning: + return "disk_warning" + default: + return "" + } +} + +// Severity is the hub severity for a band. `disk_warning` must be "warning" and `disk_critical` +// "critical" — the dispatcher drops "info" entirely (severityNotifies), so getting this wrong stores +// the event and mails nobody. +func (b Band) Severity() string { + switch b { + case BandCritical: + return "critical" + case BandWarning: + return "warning" + default: + return "" + } +} + +// Usage is one filesystem's occupancy. Deliberately not `system.DiskUsageInfo` — this package must be +// testable without a real filesystem, and the seam is the reason it is. +type Usage struct { + UsedPercent float64 + AvailGB float64 + UsedGB float64 + TotalGB float64 +} + +// Target is a watched filesystem: the path to stat, and the name a CUSTOMER will recognise. +type Target struct { + Path string + Label string +} + +// Event is one crossing, handed to the notify seam. +type Event struct { + Target Target + Band Band + Usage Usage + Message string // the Hungarian customer text, already rendered +} + +// Watcher holds the persisted per-filesystem band and emits on escalation only. +type Watcher struct { + statePath string + logger *log.Logger + + // targets returns the filesystems to watch, resolved at CHECK time — a drive added or removed + // between checks must be picked up without a restart. + targets func() []Target + // usage reads one filesystem. NIL RESULT MEANS UNREADABLE, NEVER FULL (§8.4). + usage func(path string) *Usage + // notify pushes the customer event. Nil-safe. + notify func(Event) + + mu sync.Mutex + bands map[string]Band +} + +// New builds a Watcher over a state file. Nothing is read until Check runs. +func New(statePath string, logger *log.Logger, targets func() []Target, usage func(string) *Usage) *Watcher { + if logger == nil { + logger = log.Default() + } + return &Watcher{ + statePath: statePath, + logger: logger, + targets: targets, + usage: usage, + bands: make(map[string]Band), + } +} + +// SetNotify wires the customer event push. INIT-ONLY — call once at startup. +func (w *Watcher) SetNotify(fn func(Event)) { w.notify = fn } + +// classify decides the band from a reading AND the previous band, which is what makes the hysteresis +// work: between the clear and warn thresholds the previous band is HELD rather than recomputed. +// +// Pure, so it is unit-testable without a filesystem, a clock or a notifier. +func classify(u Usage, prev Band) Band { + switch { + case u.UsedPercent >= CritUsedPercent || u.AvailGB < CritFreeGiB: + return BandCritical + case u.UsedPercent >= WarnUsedPercent || u.AvailGB < WarnFreeGiB: + return BandWarning + case u.UsedPercent <= ClearUsedPercent && u.AvailGB >= ClearFreeGiB: + return BandOK + default: + // The dead zone. Holding `prev` is the whole hysteresis: a filesystem sitting at 80% neither + // warns (it is below the warn line) nor clears (it is above the clear line). + return prev + } +} + +// Check reads every target once and emits for each ESCALATION. Safe to call from the scheduler. +func (w *Watcher) Check() error { + if w == nil { + return nil + } + w.mu.Lock() + defer w.mu.Unlock() + w.loadLocked() + + targets := w.targets() + seen := make(map[string]bool, len(targets)) + changed := false + + for _, t := range targets { + if t.Path == "" { + continue + } + seen[t.Path] = true + u := w.usage(t.Path) + if u == nil { + // §8.4 — NOT-YET-MOUNTED IS NOT FULL. An absent, unmounted or unreadable filesystem is + // the drive gate's business and already has its own alert (storage_disconnected / + // backup_target_absent). Reporting it as "full" would be a false alarm with a misleading + // cause, and would tell the customer to delete files that are not the problem. + w.logger.Printf("[DEBUG] [fillwatch] %s: usage unreadable — skipped (an unreadable filesystem is not a full one)", t.Path) + continue + } + prev := w.bands[t.Path] + next := classify(*u, prev) + if next == prev { + continue + } + w.bands[t.Path] = next + changed = true + + if next <= prev { + // De-escalation, including the return to OK, is SILENT. The customer already acted, or + // the app deleted its own temp files; telling them a resolved problem is resolved is + // noise. The state change re-arms the warning, which is what Scenario F asks for. + w.logger.Printf("[INFO] [fillwatch] %s: %s → %s (%.0f%% used, %.1f GB free) — cleared silently, re-armed", + t.Path, prev, next, u.UsedPercent, u.AvailGB) + continue + } + + msg := Message(t, *u, next) + w.logger.Printf("[WARN] [fillwatch] %s (%q): %s → %s — %.0f%% used, %.1f GB free of %.1f GB; notifying the customer", + t.Path, t.Label, prev, next, u.UsedPercent, u.AvailGB, u.TotalGB) + if w.notify != nil { + w.notify(Event{Target: t, Band: next, Usage: *u, Message: msg}) + } + } + + // Forget filesystems that no longer exist (a decommissioned drive), so the state file cannot grow + // without bound and a re-added drive starts from OK rather than from a stale band. + for path := range w.bands { + if !seen[path] { + delete(w.bands, path) + changed = true + } + } + + if !changed { + return nil + } + return w.saveLocked() +} + +// Bands returns a copy of the current per-path state (diagnostics + tests). +func (w *Watcher) Bands() map[string]Band { + w.mu.Lock() + defer w.mu.Unlock() + out := make(map[string]Band, len(w.bands)) + for k, v := range w.bands { + out[k] = v + } + return out +} + +// ── State persistence ──────────────────────────────────────────────────────────────────────────── +// +// Persisted so a controller restart does not re-warn about a filesystem the customer has already +// been told about — the offboxEnlargeBlockedNotify discipline. LOSING IT IS THE ACCEPTABLE +// DIRECTION: at most one extra warning, never a missed one. THE HUB OWNS COOLDOWN; this package must +// never add a timer of its own. + +type persisted struct { + Bands map[string]string `json:"bands"` +} + +func (w *Watcher) loadLocked() { + if w.statePath == "" { + return + } + data, err := os.ReadFile(w.statePath) + if err != nil { + if !os.IsNotExist(err) { + w.logger.Printf("[WARN] [fillwatch] could not read %s: %v — starting from OK (at most one extra warning)", w.statePath, err) + } + return + } + var p persisted + if err := json.Unmarshal(data, &p); err != nil { + w.logger.Printf("[WARN] [fillwatch] %s is corrupt: %v — starting from OK (at most one extra warning)", w.statePath, err) + return + } + for path, band := range p.Bands { + switch band { + case "critical": + w.bands[path] = BandCritical + case "warning": + w.bands[path] = BandWarning + } + } +} + +func (w *Watcher) saveLocked() error { + if w.statePath == "" { + return nil + } + p := persisted{Bands: make(map[string]string, len(w.bands))} + for path, band := range w.bands { + if band == BandOK { + continue // OK is the default — do not persist it + } + p.Bands[path] = band.String() + } + data, err := json.MarshalIndent(p, "", " ") + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(w.statePath), 0o755); err != nil { + return err + } + tmp := w.statePath + ".tmp" + if err := os.WriteFile(tmp, data, 0o600); err != nil { + return err + } + return os.Rename(tmp, w.statePath) +} + +// ── Customer copy (Hungarian) ──────────────────────────────────────────────────────────────────── + +// Message renders the customer-facing Hungarian warning. +// +// IT MUST SAY WHAT TO DO, not merely that something is happening — a warning a customer cannot act +// on is an operator alert wearing the wrong clothes. It names the storage by its LABEL (the customer +// named it; a path means nothing to them) and gives the free space in GB. +// +// It is sent as the event's MESSAGE, and the hub deliberately has no `customerMessages` entry for +// these two types: `FormatCustomerEmail` PREFERS the entry over the message, so a static template +// would discard the label and the figures — exactly why `offbox_enlarge_blocked` and +// `disk_health_degraded` have none either. +func Message(t Target, u Usage, band Band) string { + name := t.Label + if name == "" { + name = t.Path + } + if band == BandCritical { + return fmt.Sprintf( + "A(z) „%s” tároló kritikusan megtelt: %s szabad hely maradt (%s foglalt). "+ + "A biztonsági mentések és az alkalmazások írásai bármikor meghiúsulhatnak. "+ + "Kérjük, mielőbb szabadíts fel helyet: törölj felesleges fájlokat, vagy csatlakoztass új meghajtót.", + name, hunGB(u.AvailGB), hunPercent(u.UsedPercent)) + } + return fmt.Sprintf( + "A(z) „%s” tároló %s foglalt — %s szabad hely maradt. "+ + "Kérjük, szabadíts fel helyet, mielőtt megtelik: törölj felesleges fájlokat, vagy csatlakoztass új meghajtót. "+ + "Ha megtelik, a biztonsági mentések meghiúsulnak.", + name, hunPercent(u.UsedPercent), hunGB(u.AvailGB)) +} + +// hunGB formats a GB figure the Hungarian way — decimal COMMA, one decimal place. A "4.2 GB" in a +// Hungarian sentence reads as a typo to the customer. +func hunGB(gb float64) string { + if gb >= 1024 { + return strings.Replace(fmt.Sprintf("%.1f TB", gb/1024), ".", ",", 1) + } + return strings.Replace(fmt.Sprintf("%.1f GB", gb), ".", ",", 1) +} + +func hunPercent(p float64) string { return fmt.Sprintf("%.0f%%", p) } + +// SortTargets gives Check a deterministic order, so a multi-filesystem crossing produces its events +// in a stable sequence (tests, and a readable log). +func SortTargets(ts []Target) []Target { + sort.Slice(ts, func(i, j int) bool { return ts[i].Path < ts[j].Path }) + return ts +} diff --git a/controller/internal/fillwatch/fillwatch_test.go b/controller/internal/fillwatch/fillwatch_test.go new file mode 100644 index 0000000..a5ba9c5 --- /dev/null +++ b/controller/internal/fillwatch/fillwatch_test.go @@ -0,0 +1,320 @@ +package fillwatch + +import ( + "io" + "log" + "path/filepath" + "strings" + "testing" +) + +// R-167 / decision D-c, customer half — the customer is warned BEFORE a fill, once. + +type harness struct { + w *Watcher + events []Event + usage map[string]*Usage + target []Target +} + +func newHarness(t *testing.T, targets ...Target) *harness { + t.Helper() + h := &harness{usage: map[string]*Usage{}, target: targets} + h.w = New(filepath.Join(t.TempDir(), "fillwatch.json"), log.New(io.Discard, "", 0), + func() []Target { return h.target }, + func(p string) *Usage { return h.usage[p] }) + h.w.SetNotify(func(e Event) { h.events = append(h.events, e) }) + return h +} + +func (h *harness) set(path string, usedPct, availGB float64) { + h.usage[path] = &Usage{UsedPercent: usedPct, AvailGB: availGB, TotalGB: 100, UsedGB: usedPct} +} + +func (h *harness) check(t *testing.T) { + t.Helper() + if err := h.w.Check(); err != nil { + t.Fatalf("Check: %v", err) + } +} + +var photos = Target{Path: "/mnt/felhom-drives/hdd_1", Label: "Fotók"} + +// --- Scenario E — warned once, and the second pass is silent ------------------------------------- + +func TestWarnsOnceThenIsSilent(t *testing.T) { + h := newHarness(t, photos) + h.set(photos.Path, 87, 4.2) + + h.check(t) + if len(h.events) != 1 { + t.Fatalf("got %d events on the first crossing, want 1 — the customer was not warned before "+ + "the fill, which is the whole customer half of D-c", len(h.events)) + } + e := h.events[0] + if e.Band != BandWarning { + t.Fatalf("band = %v, want warning", e.Band) + } + if e.Band.EventType() != "disk_warning" { + t.Fatalf("event type = %q, want disk_warning", e.Band.EventType()) + } + if e.Band.Severity() != "warning" { + t.Fatalf("severity = %q, want warning — the hub's severityNotifies DROPS \"info\", so a "+ + "wrong severity stores the event and mails nobody", e.Band.Severity()) + } + + // SECOND PASS, nothing changed. Edge-triggered means exactly nothing fires. + h.check(t) + if len(h.events) != 1 { + t.Fatalf("got %d events after an unchanged second pass, want still 1 — the warning is not "+ + "edge-triggered, so a daily schedule would re-warn the customer every single night", + len(h.events)) + } +} + +// The state must survive a restart — a fresh Watcher over the SAME file must not re-warn. +func TestEdgeStateSurvivesARestart(t *testing.T) { + dir := t.TempDir() + state := filepath.Join(dir, "fillwatch.json") + usage := map[string]*Usage{photos.Path: {UsedPercent: 87, AvailGB: 4.2, TotalGB: 100}} + targets := func() []Target { return []Target{photos} } + read := func(p string) *Usage { return usage[p] } + + var first []Event + w1 := New(state, log.New(io.Discard, "", 0), targets, read) + w1.SetNotify(func(e Event) { first = append(first, e) }) + if err := w1.Check(); err != nil { + t.Fatal(err) + } + if len(first) != 1 { + t.Fatalf("first controller: %d events, want 1", len(first)) + } + + // — a brand-new Watcher, same file. + var second []Event + w2 := New(state, log.New(io.Discard, "", 0), targets, read) + w2.SetNotify(func(e Event) { second = append(second, e) }) + if err := w2.Check(); err != nil { + t.Fatal(err) + } + if len(second) != 0 { + t.Fatalf("a restarted controller re-warned about an already-warned filesystem (%d events) — "+ + "the state is not persisted, so every restart nags the customer", len(second)) + } +} + +// --- Scenario F — it clears, and it can fire again ------------------------------------------------ + +func TestClearsSilentlyThenReArms(t *testing.T) { + h := newHarness(t, photos) + + h.set(photos.Path, 87, 4.2) + h.check(t) + if len(h.events) != 1 { + t.Fatalf("warn: got %d events, want 1", len(h.events)) + } + + // Drop below the CLEAR thresholds (both must hold): 70% used, 12 GB free. + h.set(photos.Path, 70, 12) + h.check(t) + if len(h.events) != 1 { + t.Fatalf("clearing fired an event (%d total) — a resolved problem must clear SILENTLY", len(h.events)) + } + if b := h.w.Bands()[photos.Path]; b != BandOK { + t.Fatalf("band after clearing = %v, want ok — it is latched, and the customer can never be "+ + "warned about this filesystem again", b) + } + + // Cross again — a NEW crossing must warn again. + h.set(photos.Path, 88, 3.9) + h.check(t) + if len(h.events) != 2 { + t.Fatalf("a NEW crossing after a clear produced %d events total, want 2 — the warning is "+ + "latched forever after one firing", len(h.events)) + } +} + +// The dead zone is the hysteresis. A filesystem that falls back to 80% — below warn, above clear — +// must NOT clear, or it flaps warned/cleared/warned as it wobbles across one line. +func TestDeadZoneHoldsThePreviousBand(t *testing.T) { + h := newHarness(t, photos) + h.set(photos.Path, 87, 4.2) + h.check(t) + + h.set(photos.Path, 80, 6) // between clear (75 / 7 GB) and warn (85 / 5 GB) + h.check(t) + if b := h.w.Bands()[photos.Path]; b != BandWarning { + t.Fatalf("band in the dead zone = %v, want warning held — without hysteresis a filesystem "+ + "hovering on the line flaps between warned and cleared", b) + } + if len(h.events) != 1 { + t.Fatalf("the dead zone produced an extra event (%d total)", len(h.events)) + } +} + +// Escalation warning → critical MUST fire: it is a different message and a different urgency. +func TestEscalationToCriticalFires(t *testing.T) { + h := newHarness(t, photos) + h.set(photos.Path, 87, 4.2) + h.check(t) + + h.set(photos.Path, 96, 1.4) + h.check(t) + if len(h.events) != 2 { + t.Fatalf("got %d events, want 2 — an escalation from warning to critical went unreported", len(h.events)) + } + e := h.events[1] + if e.Band != BandCritical || e.Band.EventType() != "disk_critical" { + t.Fatalf("second event = %v/%s, want critical/disk_critical", e.Band, e.Band.EventType()) + } + // De-escalating critical → warning must be silent (it is still bad; do not celebrate). + h.set(photos.Path, 87, 4.2) + h.check(t) + if len(h.events) != 2 { + t.Fatalf("a critical→warning de-escalation fired (%d total) — only escalation notifies", len(h.events)) + } +} + +// --- Scenario I / §8.4 — a nil usage read is never a warning ------------------------------------- + +func TestUnreadableFilesystemNeverWarns(t *testing.T) { + h := newHarness(t, photos) + // No entry in h.usage → the seam returns nil, which is what system.GetDiskUsage does on error. + h.check(t) + + if len(h.events) != 0 { + t.Fatalf("an UNREADABLE filesystem produced %d warning(s) — an absent, unmounted or "+ + "unreadable drive is the drive gate's business and has its own alert; reporting it as "+ + "\"full\" is a false alarm with a misleading cause, and tells the customer to delete "+ + "files that are not the problem (§8.4)", len(h.events)) + } + if b, ok := h.w.Bands()[photos.Path]; ok && b != BandOK { + t.Fatalf("an unreadable filesystem was recorded as %v", b) + } +} + +// An unreadable filesystem must not CLEAR an existing warning either — that would silently retract a +// true alarm the moment a drive blipped. +func TestUnreadableDoesNotClearAnExistingWarning(t *testing.T) { + h := newHarness(t, photos) + h.set(photos.Path, 87, 4.2) + h.check(t) + + delete(h.usage, photos.Path) // now unreadable + h.check(t) + if b := h.w.Bands()[photos.Path]; b != BandWarning { + t.Fatalf("band after an unreadable read = %v, want the warning HELD — a blipping drive "+ + "would otherwise silently retract a true alarm", b) + } +} + +// --- Group H — the thresholds keep their gap ------------------------------------------------------ + +// A warn and a clear threshold that can be edited into equality is a flapping bug waiting to be +// introduced. This pins the ORDERING and a real margin, not the literal numbers. +func TestThresholdsKeepTheirHysteresisGap(t *testing.T) { + if ClearUsedPercent >= WarnUsedPercent { + t.Fatalf("ClearUsedPercent (%.1f) must be strictly BELOW WarnUsedPercent (%.1f) — equal "+ + "thresholds make a filesystem sitting on the line warn, clear, warn, clear every check", + ClearUsedPercent, WarnUsedPercent) + } + if ClearFreeGiB <= WarnFreeGiB { + t.Fatalf("ClearFreeGiB (%.1f) must be strictly ABOVE WarnFreeGiB (%.1f) — the free-byte term "+ + "needs the same hysteresis as the percentage term, or it flaps on its own", + ClearFreeGiB, WarnFreeGiB) + } + // A margin, not merely an inequality: a 0.1-point gap is arithmetically a gap and practically none. + if WarnUsedPercent-ClearUsedPercent < 5 { + t.Fatalf("the used-percent hysteresis gap is %.1f points — too narrow to damp real wobble", + WarnUsedPercent-ClearUsedPercent) + } + if ClearFreeGiB-WarnFreeGiB < 1 { + t.Fatalf("the free-space hysteresis gap is %.1f GiB — too narrow", ClearFreeGiB-WarnFreeGiB) + } + if CritUsedPercent <= WarnUsedPercent || CritFreeGiB >= WarnFreeGiB { + t.Fatal("the critical band must be strictly tighter than the warning band on BOTH terms, " + + "or a filesystem can be critical without ever having been warned") + } +} + +// Both terms must be able to trip INDEPENDENTLY — that is the entire reason there are two. +func TestEitherTermCanTripTheWarning(t *testing.T) { + // A big drive: only 60% used, but under the free-space floor. 85% of a 4 TB drive leaves 600 GB, + // so the percentage alone would never fire here. + if got := classify(Usage{UsedPercent: 60, AvailGB: 3}, BandOK); got != BandWarning { + t.Fatalf("60%% used with 3 GB free classified as %v — the free-byte term did not trip, so a "+ + "large drive can run out of space without ever warning", got) + } + // A small volume: plenty of GB free in absolute terms is impossible here, so the percentage is + // what must fire. 88% of a 100 GB volume leaves 12 GB — above the 5 GiB floor. + if got := classify(Usage{UsedPercent: 88, AvailGB: 12}, BandOK); got != BandWarning { + t.Fatalf("88%% used with 12 GB free classified as %v — the percentage term did not trip", got) + } + if got := classify(Usage{UsedPercent: 50, AvailGB: 50}, BandOK); got != BandOK { + t.Fatalf("a healthy filesystem classified as %v", got) + } +} + +// --- The copy ------------------------------------------------------------------------------------ + +// The customer message must be ACTIONABLE and specific. A warning that says only "something is +// filling" is an operator alert wearing the wrong clothes. +func TestCustomerMessageNamesTheDriveTheSpaceAndTheAction(t *testing.T) { + msg := Message(photos, Usage{UsedPercent: 87, AvailGB: 4.2, TotalGB: 100}, BandWarning) + + if !strings.Contains(msg, "Fotók") { + t.Fatalf("the message does not name the storage by its LABEL — a path means nothing to a "+ + "customer. Got: %s", msg) + } + if !strings.Contains(msg, "4,2 GB") { + t.Fatalf("the message does not give the free space in Hungarian number format (decimal "+ + "COMMA). Got: %s", msg) + } + if !strings.Contains(msg, "87%") { + t.Fatalf("the message does not give the fill percentage. Got: %s", msg) + } + if !strings.Contains(msg, "szabadíts fel helyet") && !strings.Contains(msg, "szabadíts fel helyet:") { + t.Fatalf("the message does not tell the customer WHAT TO DO. Got: %s", msg) + } + // Design-system rule: no emoji in customer copy. + for _, r := range msg { + if r > 0x2100 { + t.Fatalf("the message contains an emoji/symbol %q — the design system forbids it in "+ + "customer copy. Got: %s", string(r), msg) + } + } + + crit := Message(photos, Usage{UsedPercent: 96, AvailGB: 1.4, TotalGB: 100}, BandCritical) + if crit == msg { + t.Fatal("the critical message is identical to the warning — the urgency must differ") + } + if !strings.Contains(crit, "1,4 GB") || !strings.Contains(crit, "Fotók") { + t.Fatalf("the critical message lost its specifics. Got: %s", crit) + } +} + +// A label-less target must still produce a usable message rather than an empty quote. +func TestMessageFallsBackToThePathWhenUnlabelled(t *testing.T) { + msg := Message(Target{Path: "/mnt/sys_drive"}, Usage{UsedPercent: 90, AvailGB: 1.5}, BandWarning) + if !strings.Contains(msg, "/mnt/sys_drive") { + t.Fatalf("an unlabelled target rendered without any identifier: %s", msg) + } +} + +// A decommissioned drive must fall out of the state file, so it cannot grow without bound and a +// re-added drive starts fresh. +func TestVanishedTargetIsForgotten(t *testing.T) { + h := newHarness(t, photos) + h.set(photos.Path, 87, 4.2) + h.check(t) + if _, ok := h.w.Bands()[photos.Path]; !ok { + t.Fatal("the warned filesystem was not recorded") + } + + h.target = nil // the drive is decommissioned + h.check(t) + if _, ok := h.w.Bands()[photos.Path]; ok { + t.Fatal("a removed filesystem kept its band — the state file grows without bound and a " + + "re-added drive would resume from a stale band instead of warning afresh") + } +} diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index a2aad0f..e815878 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -298,6 +298,38 @@ func (n *Notifier) NotifyBackupFailed(message, errMsg string) { n.PushEvent("backup_failed", "error", message, BackupDetails{Error: errMsg}) } +// RecoveryUnitFailureDetails is the machine-readable tail of a Tier-1 capture failure. App NAMES and +// byte figures only — never an env value (§9.5). +type RecoveryUnitFailureDetails struct { + App string `json:"app"` + Error string `json:"error"` + TargetPath string `json:"target_path,omitempty"` + UsedGB float64 `json:"used_gb,omitempty"` + AvailGB float64 `json:"avail_gb,omitempty"` + TotalGB float64 `json:"total_gb,omitempty"` + UsedPercent float64 `json:"used_percent,omitempty"` + // SpaceKnown distinguishes "we read the filesystem and it says these numbers" from "we could not + // read it". Without it, an unreadable target is indistinguishable from an empty one — the + // presence-is-not-success trap, in the other direction. + SpaceKnown bool `json:"space_known"` +} + +// NotifyRecoveryUnitCaptureFailed sends the OPERATOR-TIER alert for a per-app Tier-1 recovery-unit +// capture failure (R-158, D-c's operator half). +// +// DELIBERATELY NOT `backup_failed`. That type carries a `customerMessages` entry AND sits in +// `settings.DefaultEnabledEvents`, so reusing it would email the customer, in Hungarian, that their +// backup failed — an event they can take no action on. It is exactly the mistake R-97a avoided by +// minting `whole_guest_backup_failed`, and the reasoning is written into the hub's handler.go. +// R-158's original proposal named `backup_failed`; decision D-c routes this to the operator, and +// where the two disagree D-c wins. +// +// Operator-only is enforced by the hub's `notify.operatorOnlyEvents` register, NOT by the absence of +// a customerMessages entry — v0.78.0 claimed the latter and was wrong. +func (n *Notifier) NotifyRecoveryUnitCaptureFailed(message string, d RecoveryUnitFailureDetails) { + n.PushEvent("recovery_unit_capture_failed", "error", message, d) +} + // NotifyOffboxEnlargeBlocked sends a WARNING (not a failure) when an app's enlarged offsite push was // refused by the pre-push quota gate — its config+DB were still saved. Customer-facing (Hungarian // body). NOTE: the event type "offbox_enlarge_blocked" must be added to the hub's allowedEventTypes +