diff --git a/CHANGELOG.md b/CHANGELOG.md index c674cb5..c897d3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,104 @@ +## v0.223.0 — the alarm we had just built reached nobody, and the stop nobody heard (2026-08-23, R-329 + R-386) +**MinAgent: 0.129.0** (unchanged — no new agent coupling) + +### R-329 — one word, and every `app_start_failed` was delivered to no one + +`NotifyAppStartFailures` emitted severity **`"warn"`**. The hub's vocabulary is exactly +`{info, warning, error, critical}` and it **silently coerces** anything else to `info` at ingest; +`severityNotifies` then drops `info`, returning **before both** the operator and the customer leg. So +the banner appeared, the hub stored the event, the POST returned 200, and no mail left the building. + +**This is the SECOND time.** `DiskAlertKind.Severity` emitted `"warn"` until v0.215.0, and its own doc +comment records that every Figyelmeztetés-level disk alert the product ever produced went to nobody. +The lesson was written down. Nothing enforced it. It happened again — and it hid for months only +because R-384's ordering defect meant the event could not fire at all, so a broken severity had +nothing to break. + +**The guard is now an AST walk over the whole controller** +(`TestR329_EveryEmittedSeverityIsInTheHubVocabulary`), not a comment and not a `strings.Contains`: +the word "warn" appears legitimately nine times in `internal/monitor` and `internal/selftest` as a +*healthcheck status*, and only one of those hits was the defect. + +**The sweep found exactly one bad severity and is reported in full**, including its limits. The walk +cannot follow a variable, so the six call sites that pass one are **registered by name with the values +each can take** — a new dynamic site fails the test rather than slipping past it. Two of those six +were found by the guard itself, not by the hand sweep that preceded it. + +**A latent one, found while sweeping and pinned rather than left:** `fillwatch.Band.Severity()` +returns `""` for `BandOK`, which would coerce to `info` and vanish. It is unreachable — `Check()` +notifies only on an *escalation* — but that safety lives in a different function from the one that +looks unsafe. `TestR329_FillwatchNeverEmitsTheEmptySeverity` pins the **consequence**, not the mapping. + +### R-329, part two — the app-down alarm gets a customer switch, DEFAULT OFF + +`app_start_failed` is now a customer-facing toggle („Alkalmazás nem fut"), and is **deliberately NOT +in `DefaultEnabledEvents`** (operator ruling, 2026-08-23). + +**The operator is emailed either way.** `processOperator` consults only `operatorOn`, the address and +a one-hour cooldown — never customer preferences. The toggle governs the customer leg alone. It is +also deliberately **not** added to `operatorOnlyEvents`: that would make the switch visible, +flickable and structurally incapable of delivering, which is worse than not offering it. + +### R-386 — ask the field that knows, instead of guessing from the state + +`classifyRunStates` decided "the customer stopped this" from `st.State == StateStopped`. **Every** +stopped stack was therefore assumed deliberate. Measured live on `demo-hp` 2026-08-23: `privatebin` +stopped out of band, nine dead-app scans over four minutes, **zero events and zero banner lines** — +while the comment beside the code claimed an out-of-band stop *"still alerts"*. + +The product already records the answer. `DesiredState` is what the customer asked for, it has exactly +**one writer** (their own action), and it is tri-state. The ruling, operator-approved: + +| Intent | Verdict | +|---|---| +| `Stopped` | the customer asked → **no alarm** (unchanged) | +| `Running` | nobody asked → **ALARM** (the fix) | +| absent | **UNKNOWN, never "running"** → no alarm, **and say so** | + +**The unknown case keeps today's behaviour deliberately.** Reading absent as "nobody asked" would, on +the first cycle after upgrade, email about every app any owner has ever deliberately stopped — +fleet-wide, from a field that predates the intent being asked of it. The backfill cannot help: it +seeds `Running` only from an observed-UP reading, so anything stopped at upgrade time stays unknown — +which is exactly the ambiguous population. + +**The gap is BOUNDED, not silent.** Every suppression sets `AppRunState.IntentUnknown`, and the +scheduler logs the names at INFO on the heartbeat cadence, so an operator can answer *"how many apps +am I blind to, and which?"*. **A rule without a mechanism is a wish** — the log line is the mechanism. +It closes itself as apps are started and stopped through the interface. + +`failedRestart` still lifts a `Stopped` intent, and that ordering is load-bearing: removing it +re-opens F-CRIT-1's indefinitely-silent dead app. Every existing suppression — quiesce grace, boot +grace, crash-loop threshold, the `Deploying` skip — is untouched. **No new `DesiredState` writer was +added**; the field keeps its single owner. + +### Two settings toggles secretly governed two alarms each + +„Lemez figyelmeztetés (90%+)" also wrote `disk_critical` — the drive-is-**failing** alarm. A customer +silencing a disk-nearly-full notice silenced "this drive is dying", and the label claimed only the +first. „Elvárt mentés elmaradt" had the same shape across the file-backup and database-dump misses. +Each is now two honestly-labelled toggles; **12 toggles became 15.** + +**The risk was never the split, it was the migration.** Every stored list was written by the OLD form +names. A no-op save now travels a different path, and a settings page that rewrites a setting while +merely rendering it would be worse than the defect. So a save whose event **set** is unchanged stores +the **existing slice verbatim** — byte-identity by construction, not by argument. The legacy compound +form names are still read, so a stale browser tab cannot drop a key. + +**This was not theoretical:** with the guard removed, the `defaults` case reorders. The red-proof +caught it. + +### Tests + +`internal/notify/r329_severity_contract_test.go`, `internal/fillwatch/r329_severity_test.go`, +`cmd/controller/r386_intent_test.go`, `internal/web/r329_toggle_split_test.go`. +Test count **1504 → 1522**. + +**Red-proofs: five planted, five seen failing — and ONE PASSED FIRST TIME AND IS REPORTED.** The +fillwatch mutation `next <= prev` → `next < prev` is **inert**: an earlier `if next == prev { continue }` +had already removed the equal case, so the code's behaviour did not change and the test was right to +pass. Removing the de-escalation guard outright convicts it. **A red-proof that passes needs the +mutation checked before either verdict is believed.** + ## v0.222.0 — an app whose database dies raised no alarm, because the wrong question answered first (2026-08-23, R-384 + R-383) **MinAgent: 0.129.0** (unchanged — no new agent coupling) diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 685df25..288217d 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -732,6 +732,7 @@ func main() { notifier.NotifyAppStartFailures(states) deadAppScans++ noteDeadAppScan(logger, deadAppScans, len(states), len(dead)) + noteUnknownIntentSuppressions(logger, states) return nil }) @@ -1731,6 +1732,36 @@ func noteDeadAppScan(logger *log.Logger, scans, evaluated, down int) { scans, evaluated, down) } +// noteUnknownIntentSuppressions reports every app whose dead-app alarm was suppressed ONLY because +// no customer intent was ever recorded (R-386, §4's ruling). +// +// **A rule without a mechanism is not a rule.** §4 chose to keep today's behaviour for the unknown +// case, which is the safe choice — but a safe choice that is invisible is indistinguishable from the +// defect it replaced. This line is what makes the gap BOUNDED rather than silent: an operator can +// grep one word and answer "how many apps am I blind to, and which?". +// +// It rides the same cadence as the F-OBS heartbeat rather than firing every 30 s, for the reason +// noteDeadAppScan records: a per-scan line is 2880 lines/day of noise, and noise is what made the +// original author choose silence. The population only changes when someone starts or stops an app. +func noteUnknownIntentSuppressions(logger *log.Logger, states []notify.AppRunState) { + if logger == nil || deadAppHeartbeatEvery <= 0 || deadAppScans%deadAppHeartbeatEvery != 0 { + return + } + var names []string + for _, s := range states { + if s.IntentUnknown { + names = append(names, s.Name) + } + } + if len(names) == 0 { + return + } + sort.Strings(names) + logger.Printf("[INFO] [deadapp] %d stopped app(s) have NO recorded customer intent, so their "+ + "dead-app alarm is suppressed by the unknown-intent fallback (R-386): %s. This closes itself "+ + "as each app is started or stopped through the interface.", len(names), strings.Join(names, ", ")) +} + // bootReconcileSettle is the delay before the FIRST observation. It lets the initial scan, the first // status refresh and the two crash recoveries land before the R-52 sweep decides what "down" means. var bootReconcileSettle = 5 * time.Second @@ -2169,9 +2200,58 @@ func classifyRunStates(sts []stacks.Stack, quiesced map[string]bool, failedResta // brief restart never reaches it and the two suppression windows compose into one bounded // delay. Quiesce suppression below still wins inside its own window. crashLooping := st.CrashLooping(now) - userStopped := st.State == stacks.StateStopped && !failedRestart[st.Name] + + // R-386: ASK THE FIELD THAT KNOWS, do not guess from the state. + // + // Until v0.223.0 this read `st.State == StateStopped && !failedRestart[...]` — i.e. EVERY + // stopped stack was assumed to be a deliberate customer stop. Measured on `demo-hp` + // 2026-08-23: `privatebin` stopped out of band, nine dead-app scans, zero events, zero + // banner. The comment above claimed an out-of-band stop "still alerts"; it did not, because + // `aggregateState` folds StateExited into the stopped counter and StateExited never survives + // aggregation. + // + // `DesiredState` is what the customer actually asked for, it has exactly ONE writer (the + // customer's own action — see the field's comment in internal/stacks/deploy.go), and it is + // TRI-state. The three-way ruling, operator-approved 2026-08-23: + // + // Stopped → the customer asked → no alarm (unchanged) + // Running → nobody asked → ALARM (the R-386 fix) + // absent → UNKNOWN, never "running" → no alarm, AND say so (see IntentUnknown) + // + // The absent case keeps today's behaviour deliberately. Reading unknown as "nobody asked" + // would, on the first cycle after this ships, email about every app any owner has ever + // deliberately stopped — fleet-wide, from a field that predates the intent it is being asked + // about. That is the same over-correction the tri-state exists to prevent, and the backfill + // cannot help: it seeds Running only from an observed-UP reading, so anything stopped at + // upgrade time stays unknown — which is exactly the ambiguous population. + // + // The gap is BOUNDED AND NAMED, not silent: IntentUnknown is set, the caller logs it at INFO + // with the app name, and an operator can therefore answer "how many apps am I blind to?". + // It closes itself as apps are started and stopped through the interface. + // + // `failedRestart` still lifts a Stopped intent, and that ordering is load-bearing: the + // quiesce loop stops stacks by the same path a customer does, so a stack it stopped and could + // NOT restart must alarm whatever the intent says. Removing that term re-opens F-CRIT-1's + // indefinitely-silent dead app. + intentUnknown := false + userStopped := false + if st.State == stacks.StateStopped && !failedRestart[st.Name] { + switch stacks.DesiredStateOf(st) { + case stacks.DesiredStateStopped: + userStopped = true + case stacks.DesiredStateRunning: + userStopped = false + default: // DesiredStateUnknown — the §4 fallback + userStopped = true + intentUnknown = true + } + } + down := (stacks.IsDownState(st.State) || crashLooping) && !userStopped && !quiesced[st.Name] - states = append(states, notify.AppRunState{Name: st.Name, DisplayName: st.Meta.DisplayName, Down: down}) + states = append(states, notify.AppRunState{ + Name: st.Name, DisplayName: st.Meta.DisplayName, Down: down, + IntentUnknown: intentUnknown && !down, + }) if down { dead = append(dead, web.DeadApp{Name: st.Name, DisplayName: st.Meta.DisplayName, State: string(st.State)}) } diff --git a/controller/cmd/controller/r386_intent_test.go b/controller/cmd/controller/r386_intent_test.go new file mode 100644 index 0000000..af738b8 --- /dev/null +++ b/controller/cmd/controller/r386_intent_test.go @@ -0,0 +1,270 @@ +package main + +import ( + "bytes" + "go/ast" + "go/parser" + "go/token" + "log" + "strings" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/notify" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-386 — the dead-app classifier asked the STATE whether the customer wanted an app stopped, when +// the product already records the ANSWER. +// +// THE DEFECT. `userStopped` was `st.State == StateStopped && !failedRestart[...]` — every stopped +// stack was assumed to be a deliberate customer stop. Measured live on `demo-hp` 2026-08-23: +// `privatebin` stopped out of band, nine dead-app scans over four minutes, **zero events and zero +// banner lines**, against a positive control from the same box seventeen minutes earlier. The comment +// beside the code claimed an out-of-band stop "still alerts". It did not. +// +// THE LAYER. These sit at `classifyRunStates`, which is the single derivation point for "this app is +// down" and the only place the guess was made. `classifyRunStates` is pure, so the fixture is exact +// rather than a race against docker. +// +// RED-PROOF (observed, see REPORT.md): restore the pre-fix predicate +// +// userStopped := st.State == stacks.StateStopped && !failedRestart[st.Name] +// +// and TestR386_OutOfBandStopWithRunningIntentAlarms fails with `dead-app banner = [], want privatebin` +// — which is exactly what the live box reported. + +func stoppedStack(name, intent string) stacks.Stack { + st := stacks.Stack{Name: name, Deployed: true, State: stacks.StateStopped} + if intent != stacks.DesiredStateUnknown { + st.AppConfig = &stacks.AppConfig{DesiredState: intent} + } + return st +} + +// Scenario D — R-386's measured case. Intent says Running; something stopped it anyway. +func TestR386_OutOfBandStopWithRunningIntentAlarms(t *testing.T) { + dead, states := classifyRunStates( + []stacks.Stack{stoppedStack("privatebin", stacks.DesiredStateRunning)}, nil, nil, time.Now()) + + if len(dead) != 1 || dead[0].Name != "privatebin" { + t.Fatalf("dead-app banner = %+v, want exactly privatebin — nobody asked for this app to be "+ + "stopped, so its being stopped is a fault (measured silent on demo-hp 2026-08-23)", dead) + } + if !states[0].Down { + t.Fatalf("run state = %+v, want Down=true (this is what NotifyAppStartFailures reads)", states[0]) + } + if states[0].IntentUnknown { + t.Errorf("IntentUnknown set for an app with a RECORDED intent — the log line would name an " + + "app that is not actually ambiguous") + } +} + +// Scenario C — the customer pressed Stop. Telling them their own action was a fault is the thing +// v0.164.0 was written to stop, and R-386 must not undo it. +func TestR386_CustomerStopStaysSilent(t *testing.T) { + dead, states := classifyRunStates( + []stacks.Stack{stoppedStack("docmost", stacks.DesiredStateStopped)}, nil, nil, time.Now()) + + if len(dead) != 0 { + t.Fatalf("a customer's own Stop raised an alarm: %+v", dead) + } + if states[0].Down { + t.Fatalf("run state = %+v, want Down=false — the customer asked for this", states[0]) + } + if states[0].IntentUnknown { + t.Errorf("IntentUnknown set for an app whose intent is RECORDED as stopped") + } +} + +// Scenario E — intent absent. §4's ruling: keep today's behaviour, and SAY SO. +func TestR386_AbsentIntentSuppressesButIsAnnounced(t *testing.T) { + dead, states := classifyRunStates( + []stacks.Stack{stoppedStack("legacy-app", stacks.DesiredStateUnknown)}, nil, nil, time.Now()) + + if len(dead) != 0 { + t.Fatalf("an app with NO recorded intent alarmed: %+v — on the first cycle after upgrade "+ + "that is every app any owner ever deliberately stopped, fleet-wide", dead) + } + if states[0].Down { + t.Fatalf("run state = %+v, want Down=false", states[0]) + } + // THE HALF THAT MAKES THE GAP BOUNDED RATHER THAN SILENT. + if !states[0].IntentUnknown { + t.Fatalf("IntentUnknown = false — the suppression happened but nothing records it, so an " + + "operator cannot answer 'how many apps am I blind to?'. A rule without a mechanism is " + + "not a rule") + } +} + +// And the log line itself — the mechanism §4 demands, asserted as an OBSERVABLE, not as "the +// function ran". The heartbeat cadence is deliberate; drive the counter to a firing scan. +func TestR386_UnknownIntentIsLoggedWithTheAppName(t *testing.T) { + var buf bytes.Buffer + logger := log.New(&buf, "", 0) + + states := []notify.AppRunState{ + {Name: "zulip", IntentUnknown: true}, + {Name: "legacy-app", IntentUnknown: true}, + {Name: "docmost"}, // recorded intent — must NOT appear + } + + saved := deadAppScans + t.Cleanup(func() { deadAppScans = saved }) + + // A non-firing scan says nothing (the 2880-lines/day lesson). + deadAppScans = 1 + noteUnknownIntentSuppressions(logger, states) + if buf.Len() != 0 { + t.Fatalf("logged on a non-heartbeat scan: %q", buf.String()) + } + + // A firing scan names every ambiguous app, and only those. + deadAppScans = deadAppHeartbeatEvery + noteUnknownIntentSuppressions(logger, states) + out := buf.String() + if !strings.Contains(out, "[INFO]") { + t.Errorf("not logged at INFO — it must survive a default logging.level box: %q", out) + } + for _, want := range []string{"zulip", "legacy-app", "2 stopped app(s)", "R-386"} { + if !strings.Contains(out, want) { + t.Errorf("log line %q does not contain %q", out, want) + } + } + if strings.Contains(out, "docmost") { + t.Errorf("log line names an app with a RECORDED intent: %q", out) + } + + // Nothing ambiguous → nothing said. + buf.Reset() + noteUnknownIntentSuppressions(logger, []notify.AppRunState{{Name: "docmost"}}) + if buf.Len() != 0 { + t.Errorf("logged with no ambiguous apps: %q", buf.String()) + } +} + +// Scenario F — F-CRIT-1 must not re-open. The quiesce loop stops stacks by the same path a customer +// does, so a stack it stopped and could NOT restart must alarm WHATEVER the intent says — including +// when the intent is `Stopped`, which is the case that would silently swallow it. +func TestR386_FailedRestartStillLiftsAStoppedIntent(t *testing.T) { + for _, intent := range []string{ + stacks.DesiredStateStopped, stacks.DesiredStateRunning, stacks.DesiredStateUnknown, + } { + name := intent + if name == "" { + name = "(absent)" + } + t.Run(name, func(t *testing.T) { + dead, states := classifyRunStates( + []stacks.Stack{stoppedStack("bookstack", intent)}, + nil, map[string]bool{"bookstack": true}, time.Now()) + + if len(dead) != 1 { + t.Fatalf("intent %q: a stack the quiesce loop stopped and FAILED to restart did not "+ + "alarm — this is F-CRIT-1's indefinitely-silent dead app, back through R-386's "+ + "door: %+v", intent, dead) + } + if !states[0].Down { + t.Fatalf("intent %q: run state = %+v, want Down=true", intent, states[0]) + } + }) + } +} + +// The quiesce suppression is unchanged and still wins inside its own window, for every intent. +func TestR386_QuiesceSuppressionIsUntouched(t *testing.T) { + for _, intent := range []string{stacks.DesiredStateRunning, stacks.DesiredStateStopped} { + dead, states := classifyRunStates( + []stacks.Stack{stoppedStack("bookstack", intent)}, + map[string]bool{"bookstack": true}, nil, time.Now()) + if len(dead) != 0 || states[0].Down { + t.Fatalf("intent %q: a quiesced stack alarmed (%+v) — R-97b's exact defect: the customer "+ + "told their app broke during an outage the backup caused", intent, dead) + } + } +} + +// A NON-stopped down state is unaffected by intent. A `degraded` stack (R-384) alarms regardless — +// intent only ever governed the StateStopped whitelist, and widening it would silence R-384. +func TestR386_IntentDoesNotReachNonStoppedDownStates(t *testing.T) { + st := stacks.Stack{ + Name: "bookstack", Deployed: true, State: stacks.StateDegraded, + AppConfig: &stacks.AppConfig{DesiredState: stacks.DesiredStateStopped}, + } + dead, states := classifyRunStates([]stacks.Stack{st}, nil, nil, time.Now()) + if len(dead) != 1 || !states[0].Down { + t.Fatalf("a degraded stack was silenced by a Stopped intent (%+v) — R-386 must not reach "+ + "past the StateStopped whitelist, or it undoes R-384", dead) + } +} + +// --- production wiring (§10) -------------------------------------------------------------------- +// +// A correct classifier the scheduler does not call is R-106's defect. This walks the AST of +// main.go and proves the real job wires BOTH halves: the classification and the announcement. +func TestR386_TheSchedulerWiresBothHalves(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 scan, announce, desiredRead bool + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + switch fn := call.Fun.(type) { + case *ast.Ident: + switch fn.Name { + case "scanDeployedAppRunStates": + scan = true + case "noteUnknownIntentSuppressions": + announce = true + } + case *ast.SelectorExpr: + // stacks.DesiredStateOf(st) — the intent read itself, inside classifyRunStates + if fn.Sel.Name == "DesiredStateOf" { + desiredRead = true + } + } + return true + }) + + if !scan { + t.Error("scanDeployedAppRunStates is never called from main.go — the detector is not wired") + } + if !announce { + t.Error("noteUnknownIntentSuppressions is never called from main.go — the unknown-intent gap " + + "is silent again, which is the half §4 required to make it bounded") + } + if !desiredRead { + t.Error("stacks.DesiredStateOf is never called from main.go — the classifier is back to " + + "guessing from the state (R-386)") + } +} + +// THE FENCE (§12): DesiredState has exactly ONE writer — the customer's own action. This session +// must not have added a second. Twelve of StopStack's fourteen callers are machines, so a writer in +// the wrong place makes a nightly backup indistinguishable from the customer pressing Stop. +func TestR386_NoNewDesiredStateWriterInMain(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatal(err) + } + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "SetDesiredState" { + t.Errorf("%s: SetDesiredState is called from main.go — the field has ONE writer, the "+ + "customer's own action. Reading it is fine; writing it here is the fenced act", + fset.Position(call.Pos())) + } + return true + }) + _ = strings.TrimSpace +} diff --git a/controller/internal/fillwatch/r329_severity_test.go b/controller/internal/fillwatch/r329_severity_test.go new file mode 100644 index 0000000..6ffdaa8 --- /dev/null +++ b/controller/internal/fillwatch/r329_severity_test.go @@ -0,0 +1,77 @@ +package fillwatch + +import "testing" + +// R-329 — `Band.Severity()` returns "" for BandOK, and "" is NOT in the hub's vocabulary +// {info, warning, error, critical}. The hub would coerce it to "info" and mail nobody. +// +// **This is safe today, and this test is what keeps it safe**, because the safety is not in +// `Severity()` at all — it is in `Check()`: the notify seam fires ONLY on an escalation +// (`if next <= prev { continue }`), and BandOK is the lowest band, so a notification can never carry +// it. That is an invariant held in one function about the behaviour of another, which is precisely +// the shape this project has shipped wrong nine times. +// +// THE LAYER. The first test pins the mapping (a unit fact). The second pins the CONSEQUENCE — that +// no notification can carry a band whose severity is empty — because the mapping being right is not +// what makes the product correct, and asserting only the mapping is case #9's mistake. +// +// RED-PROOF (observed, see REPORT.md): REMOVE the de-escalation `continue` block from Check() — +// every band change then notifies, and TestR329_FillwatchNeverEmitsTheEmptySeverity fails with +// `notified with band ok → severity "" (event type "")`. +// +// **A weaker mutation is INERT here, and it is worth knowing which:** changing `if next <= prev` to +// `if next < prev` does nothing, because an earlier `if next == prev { continue }` already removed +// the equal case. That mutation was tried first, the test passed, and the test was right to pass — +// the code had not changed behaviour. **A red-proof that passes is not automatically a weak test; +// check the mutation actually applied before believing either verdict.** +func TestR329_BandSeverityMapping(t *testing.T) { + for _, tc := range []struct { + band Band + want string + }{ + {BandWarning, "warning"}, + {BandCritical, "critical"}, + } { + if got := tc.band.Severity(); got != tc.want { + t.Errorf("Band(%v).Severity() = %q, want %q", tc.band, got, tc.want) + } + } + // Documented and deliberate: BandOK has no severity because it has no event. The next test is + // what proves that cannot leak. + if got := BandOK.Severity(); got != "" { + t.Errorf("BandOK.Severity() = %q — if this ever becomes a real severity, an all-clear starts "+ + "emailing customers; change it deliberately, not by accident", got) + } +} + +// The consequence: drive a real Watcher up and back down and assert that every notification carries +// a severity the hub will actually route. +func TestR329_FillwatchNeverEmitsTheEmptySeverity(t *testing.T) { + // Reuses the package's existing harness (REUSE.md §4) rather than inventing a second fixture. + h := newHarness(t, Target{Path: "/mnt/data", Label: "Adatok"}) + + // up to warning, up to critical, back down, back to ok — the full round trip, so a + // de-escalation notification would be caught if one ever started firing. + for _, pct := range []float64{50, 91, 97, 91, 50} { + h.set("/mnt/data", pct, 100-pct) + h.check(t) + } + + if len(h.events) == 0 { + // Positive control: a test that observes nothing proves nothing. If the fixture stopped + // crossing bands this would pass forever while checking air. + t.Fatal("no notifications at all — the fixture never crossed a band, so this test is checking " + + "nothing; fix the fixture before trusting a green") + } + for _, e := range h.events { + if e.Band.Severity() == "" || e.Band.EventType() == "" { + t.Errorf("notified with band %v → severity %q (event type %q): the hub coerces an unknown "+ + "severity to \"info\" and then drops it, so this alert would reach NOBODY", + e.Band, e.Band.Severity(), e.Band.EventType()) + } + if !map[string]bool{"info": true, "warning": true, "error": true, "critical": true}[e.Band.Severity()] { + t.Errorf("notified with severity %q, outside the hub vocabulary", e.Band.Severity()) + } + } + t.Logf("%d crossings notified, every one with a routable severity", len(h.events)) +} diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index e2c6f1f..f83f81a 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -508,6 +508,14 @@ type AppRunState struct { Name string DisplayName string Down bool + // IntentUnknown is set when this app is STOPPED and no customer intent was ever recorded, so the + // alarm was suppressed by the §4 fallback rather than by a decision anyone made (R-386). + // + // It rides here rather than being a third return value or a logger parameter so that the log line + // and the suppression come from the SAME computation — a separately-derived log is a second + // source of truth, and the two drift. `classifyRunStates` stays pure and its signature does not + // move, which is what lets its existing tests keep testing what they were written to test. + IntentUnknown bool } // NotifyAppStartFailures fires an `app_start_failed` hub event ONCE per running→down transition @@ -543,7 +551,14 @@ func (n *Notifier) NotifyAppStartFailures(apps []AppRunState) { if name == "" { name = a.Name } - n.emit("app_start_failed", "warn", + // R-329: "warning", NOT "warn". The hub's vocabulary is exactly + // {info, warning, error, critical} and it COERCES anything else to "info" at ingest, silently + // — after which severityNotifies drops it and NEITHER leg runs. See DiskAlertKind.Severity's + // doc comment, which records the same mistake shipping once before (v0.215.0). This one was + // worse: it was invisible for months because R-384's ordering defect meant the event could + // not fire at all, so a broken severity had nothing to break. + // Pinned by TestR329_EveryEmittedSeverityIsInTheHubVocabulary (AST walk over this package). + n.emit("app_start_failed", "warning", fmt.Sprintf("Telepített alkalmazás nem fut: %s", name), AppDetails{StackName: a.Name, DisplayName: a.DisplayName}) } diff --git a/controller/internal/notify/r329_severity_contract_test.go b/controller/internal/notify/r329_severity_contract_test.go new file mode 100644 index 0000000..8fb1d17 --- /dev/null +++ b/controller/internal/notify/r329_severity_contract_test.go @@ -0,0 +1,212 @@ +package notify + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// R-329 — the hub's severity vocabulary, pinned by an AST walk over the WHOLE controller. +// +// THE DEFECT THIS EXISTS TO KILL. `NotifyAppStartFailures` emitted severity `"warn"`. The hub accepts +// only {info, warning, error, critical} and **silently coerces** anything else to `info` at ingest, +// after which `severityNotifies` drops it and NEITHER the operator nor the customer leg runs. So the +// dashboard showed the app down, the hub stored the event, and no mail left the building. +// +// **This is the SECOND time.** `DiskAlertKind.Severity`'s own doc comment records that it emitted +// `"warn"` until v0.215.0, and that every Figyelmeztetés-level disk alert the product ever produced +// was delivered to nobody. A comment recorded the lesson; nothing enforced it; it happened again. +// **A comment is not a guard** — this is the guard. +// +// THE LAYER. This sits at the EMITTER, which is where the defect lives: the hub's coercion is +// deliberate and correct (a lost alarm is worse than a mis-routed one), so nothing downstream can +// detect a bad word — by design, the bad word ceases to exist at the hub's front door. The only +// place the mistake is still visible is where it is written. +// +// WHY AN AST WALK AND NOT grep. `strings.Contains` over source cannot tell an emitted severity from +// the word "warn" in a comment, a healthcheck status vocabulary (`internal/monitor`, +// `internal/selftest` both legitimately use "warn"), or a `case "warn":` in an unrelated switch. The +// 2026-08-23 sweep found nine such hits and exactly one real defect. +// +// RED-PROOF (observed, see REPORT.md): put `"warn"` back at notifier.go's app_start_failed emit and +// this fails naming the file, line, call and value. + +// hubSeverityVocabulary is the hub's set, verbatim. Sources, both named so a reader can check rather +// than trust: felhom.eu/hub/internal/api/handler.go (the ingest switch) and +// felhom.eu/hub/internal/notify/dispatcher.go (severityNotifies). +var hubSeverityVocabulary = map[string]bool{ + "info": true, "warning": true, "error": true, "critical": true, +} + +// severityArg names the functions that take a hub severity, and which argument it is. +var severityArg = map[string]int{ + "emit": 1, // (eventType, severity, message, details) + "PushEvent": 1, // (eventType, severity, message, details) +} + +// knownDynamicSeveritySites are the call sites that pass a VARIABLE rather than a literal, so this +// walk cannot check their value. Each was traced by hand on 2026-08-23 and every reachable value was +// in the vocabulary. **They are listed so that a NEW dynamic site cannot appear unnoticed** — the +// honest limit of an AST check is that it cannot follow a variable, and an unlisted limit is not a +// limit, it is a hole. +var knownDynamicSeveritySites = map[string]string{ + "internal/notify/notifier.go:emit": `pass-through of its own severity parameter to PushEvent — the value is checked at emit's CALLERS, above`, + "internal/notify/notifier.go:NotifyControllerUpdated": `local var: "info", or "error" when the update failed`, + "internal/notify/notifier.go:NotifyDRCompleted": `local var: "info", or "warning" when failCount > 0`, + "internal/notify/notifier.go:NotifyAgentChannelDown": `internal/channelhealth's classifier — every severity literal in checker.go is "warning" or "error"`, + "internal/notify/notifier.go:NotifyDiskHealthDegraded": `DiskAlertKind.Severity() — pinned behaviourally by TestR329_DiskAlertSeveritiesStillSatisfyTheContract`, + "cmd/controller/main.go:main": `fillwatch.Band.Severity() in the SetNotify closure — pinned by TestR329_FillwatchNeverEmitsTheEmptySeverity`, +} + +func walkControllerFiles(t *testing.T, fn func(path string, fset *token.FileSet, f *ast.File)) { + t.Helper() + root, err := filepath.Abs("../..") // the controller module root + if err != nil { + t.Fatal(err) + } + fset := token.NewFileSet() + err = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if d.Name() == "vendor" || d.Name() == ".git" || d.Name() == "node_modules" { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, perr := parser.ParseFile(fset, path, nil, parser.ParseComments) + if perr != nil { + return fmt.Errorf("parse %s: %w", path, perr) + } + rel, _ := filepath.Rel(root, path) + fn(rel, fset, f) + return nil + }) + if err != nil { + t.Fatal(err) + } +} + +// calleeName returns the bare function name for `foo(...)` and `x.foo(...)`. +func calleeName(call *ast.CallExpr) string { + switch fn := call.Fun.(type) { + case *ast.Ident: + return fn.Name + case *ast.SelectorExpr: + return fn.Sel.Name + } + return "" +} + +func TestR329_EveryEmittedSeverityIsInTheHubVocabulary(t *testing.T) { + var checked int + var dynamic []string + + walkControllerFiles(t, func(rel string, fset *token.FileSet, f *ast.File) { + // Attribute each call to the FuncDecl that encloses it, by walking declarations rather than + // trusting Inspect's visit order — a closure inside main() must read as "main", and a call in + // a var block must not inherit the previous function's name. + for _, decl := range f.Decls { + fd, isFunc := decl.(*ast.FuncDecl) + enclosing := "(file scope)" + var scope ast.Node = decl + if isFunc { + enclosing = fd.Name.Name + scope = fd + } + inspectForSeverity(t, rel, enclosing, scope, fset, &checked, &dynamic) + } + }) + + // The walk must actually have found call sites — a guard that silently examines nothing is the + // "instrument that can drop results" failure, and it would pass forever. + if checked < 15 { + t.Fatalf("only %d severity literals examined — the AST walk is not reaching the call sites; "+ + "fix this test before trusting a green from it", checked) + } + t.Logf("checked %d severity literals across the controller", checked) + + // The dynamic sites are a stated limit, and the list is the mechanism that keeps it stated. + sort.Strings(dynamic) + seen := map[string]bool{} + for _, d := range dynamic { + if seen[d] { + continue + } + seen[d] = true + if _, known := knownDynamicSeveritySites[d]; !known { + t.Errorf("NEW dynamic severity call site %q — this walk cannot check a variable's value. "+ + "Trace every value it can take; if all are in the hub vocabulary, add it to "+ + "knownDynamicSeveritySites with the reason. Do not delete this check.", d) + } + } + for known := range knownDynamicSeveritySites { + if !seen[known] { + t.Errorf("registered dynamic site %q no longer exists — remove it from "+ + "knownDynamicSeveritySites so the register stays honest", known) + } + } +} + +// The vocabulary itself must match the hub's, and DiskAlertKind.Severity — the function whose doc +// comment records the first occurrence — must still satisfy it. +func TestR329_DiskAlertSeveritiesStillSatisfyTheContract(t *testing.T) { + for _, k := range []DiskAlertKind{ + DiskAlertWarn, DiskAlertFailSelfReported, DiskAlertFailSectors, + DiskAlertFailTemperature, DiskAlertFailWorsened, + } { + if got := k.Severity(); !hubSeverityVocabulary[got] { + t.Errorf("DiskAlertKind(%d).Severity() = %q, not in the hub vocabulary", k, got) + } + } + // And the one that regressed: a warning must be a *warning*, not "warn" and not "info". + if got := DiskAlertWarn.Severity(); got != "warning" { + t.Errorf("DiskAlertWarn.Severity() = %q, want \"warning\"", got) + } +} + +// inspectForSeverity checks every severity-taking call inside one declaration. +func inspectForSeverity(t *testing.T, rel, enclosing string, scope ast.Node, fset *token.FileSet, checked *int, dynamic *[]string) { + t.Helper() + ast.Inspect(scope, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + idx, wanted := severityArg[calleeName(call)] + if !wanted || len(call.Args) <= idx { + return true + } + lit, isLit := call.Args[idx].(*ast.BasicLit) + if !isLit || lit.Kind != token.STRING { + *dynamic = append(*dynamic, rel+":"+enclosing) + return true + } + val, err := strconv.Unquote(lit.Value) + if err != nil { + t.Errorf("%s: unparseable severity literal %s", fset.Position(lit.Pos()), lit.Value) + return true + } + *checked++ + if !hubSeverityVocabulary[val] { + t.Errorf("%s: %s(...) emits severity %q, which is NOT in the hub's vocabulary "+ + "{info, warning, error, critical}.\n"+ + " The hub COERCES it to \"info\" at ingest, SILENTLY, and severityNotifies then "+ + "drops \"info\" — so this event is stored and emailed to NOBODY, on either leg.\n"+ + " This exact mistake shipped once before (DiskAlertKind.Severity, fixed v0.215.0).", + fset.Position(lit.Pos()), calleeName(call), val) + } + return true + }) +} diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index 68ce09b..cea8922 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -2016,6 +2016,50 @@ func (s *Server) settingsPasswordHandler(w http.ResponseWriter, r *http.Request) http.Redirect(w, r, "/login?flash="+flash, http.StatusFound) } +// sameEventSet reports whether two event lists hold the same keys, ignoring order and duplicates. +// Used ONLY to decide whether a save is a no-op; never to decide what to store. +func sameEventSet(a, b []string) bool { + if len(a) == 0 && len(b) == 0 { + return true + } + sa, sb := make(map[string]bool, len(a)), make(map[string]bool, len(b)) + for _, e := range a { + sa[e] = true + } + for _, e := range b { + sb[e] = true + } + if len(sa) != len(sb) { + return false + } + for e := range sa { + if !sb[e] { + return false + } + } + return true +} + +// dedupeEvents removes duplicates while PRESERVING ORDER. +// +// Order is not cosmetic here: the stored list is compared byte-for-byte by +// TestR329Part4_RoundTripIsByteIdentical, which exists because a settings page that quietly reorders +// or drops a key while merely RENDERING it would be a worse fault than the compound toggles it +// replaced. It is needed because the legacy compound form name and its two replacements can both be +// present in one POST — a browser still showing the old page, or a client that sends both. +func dedupeEvents(in []string) []string { + seen := make(map[string]bool, len(in)) + out := in[:0:0] + for _, e := range in { + if seen[e] { + continue + } + seen[e] = true + out = append(out, e) + } + return out +} + func (s *Server) settingsNotificationsHandler(w http.ResponseWriter, r *http.Request) { _ = r.ParseForm() @@ -2045,18 +2089,59 @@ func (s *Server) settingsNotificationsHandler(w http.ResponseWriter, r *http.Req "backup_failed", "db_dump_failed", "backup_integrity_failed", "crossdrive_failed", "offbox_enlarge_blocked", "storage_disconnected", "node_down", "health_critical", + // R-329: app_start_failed is customer-switchable but DEFAULT OFF — it is deliberately absent + // from settings.DefaultEnabledEvents (operator ruling, 2026-08-23). The OPERATOR is emailed + // regardless: processOperator consults operatorOn, the address and a cooldown, and never the + // customer's preferences. This toggle governs the customer leg only. + "app_start_failed", "storage_reconnected", "health_recovered", } { if r.FormValue("event_"+evt) == "on" { enabledEvents = append(enabledEvents, evt) } } - // Compound toggles: one checkbox → two event types - if r.FormValue("event_disk_alerts") == "on" { - enabledEvents = append(enabledEvents, "disk_warning", "disk_critical") + // R-329 Part 4 — WAS: two compound toggles, one checkbox writing TWO event types each. + // + // `event_disk_alerts` labelled "Lemez figyelmeztetés (90%+)" also governed `disk_critical` — the + // drive-is-FAILING alarm. A customer switching off a disk-nearly-full notice silently switched off + // "this drive is dying", and the label claimed only the first. Those are not the same decision. + // `event_expected_missed` had the same shape over the backup and database-dump misses. + // + // Each is now its own labelled toggle. **The compound form names are still READ**, so a browser + // still on the old page, or a bookmarked POST, keeps working and cannot silently drop a key — the + // migration risk here is a settings page that changes a setting while rendering it, which would be + // worse than the defect. Pinned by TestR329Part4_RoundTripIsByteIdentical. + for _, c := range []struct { + form string + events []string + }{ + {"event_disk_alerts", []string{"disk_warning", "disk_critical"}}, // legacy compound + {"event_disk_warning", []string{"disk_warning"}}, + {"event_disk_critical", []string{"disk_critical"}}, + {"event_expected_missed", []string{"expected_backup_missed", "expected_dbdump_missed"}}, // legacy compound + {"event_expected_backup_missed", []string{"expected_backup_missed"}}, + {"event_expected_dbdump_missed", []string{"expected_dbdump_missed"}}, + } { + if r.FormValue(c.form) == "on" { + enabledEvents = append(enabledEvents, c.events...) + } } - if r.FormValue("event_expected_missed") == "on" { - enabledEvents = append(enabledEvents, "expected_backup_missed", "expected_dbdump_missed") + enabledEvents = dedupeEvents(enabledEvents) + + // R-329 Part 4 — A SAVE THAT CHANGES NOTHING MUST STORE NOTHING NEW. + // + // Splitting the two compound toggles rewrote which form names produce which event keys, so a + // customer who merely OPENS this page and presses Save now travels a different code path than the + // one that wrote their stored list. If that path emits the same SET in a different ORDER, their + // stored bytes change for no reason a person asked for — and a settings page that quietly rewrites + // a setting while rendering it is a worse fault than the compound labels being fixed. + // + // So: if the submitted set is identical to what is already stored, keep the STORED slice verbatim. + // This is byte-identity by construction rather than by argument, which is the only kind worth + // having here. Pinned by TestR329Part4_RoundTripIsByteIdentical over both starting shapes. + if cur := s.settings.GetNotificationPrefs(); cur != nil && + sameEventSet(cur.EnabledEvents, enabledEvents) { + enabledEvents = cur.EnabledEvents } // EMPTY-EMAIL WIPE GUARD (2026-07-15 demo incident): a blank email box saved while events are diff --git a/controller/internal/web/r329_toggle_split_test.go b/controller/internal/web/r329_toggle_split_test.go new file mode 100644 index 0000000..36740dd --- /dev/null +++ b/controller/internal/web/r329_toggle_split_test.go @@ -0,0 +1,222 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "regexp" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// R-329 Part 4 — two settings checkboxes each governed TWO alarms, and said so in neither label. +// +// `event_disk_alerts`, labelled „Lemez figyelmeztetés (90%+)", also wrote `disk_critical` — the +// drive-is-FAILING alarm. A customer turning off a disk-nearly-full notice silently turned off "this +// drive is dying". `event_expected_missed` had the same shape across the file-backup and +// database-dump misses. +// +// THE RISK IS NOT THE SPLIT, IT IS THE MIGRATION. Every existing customer's stored list was written +// by the OLD form names. After the split they render through new ones, so a customer who merely opens +// the page and presses Save travels a different code path than the one that wrote their settings. +// **A settings page that quietly changes a setting while rendering it is worse than the defect being +// fixed**, so the round trip below is the real subject of this file. +// +// THE LAYER. This drives the REAL render (`settingsNotificationsPageHandler`) and the REAL save +// (`settingsNotificationsHandler`) over a REAL temp-file `Settings`, and compares the STORED slice. +// A test that only checked the handler's parsing would miss the half that matters: what the template +// actually ticks. +// +// RED-PROOF (observed, see REPORT.md): drop the `sameEventSet` no-op guard in the save handler and +// TestR329Part4_RoundTripIsByteIdentical/defaults fails, showing the stored order rewritten. + +// checkedBoxes renders the settings page and returns the form names the template ticked — i.e. +// exactly what a browser would POST if the customer pressed Save without touching anything. +func checkedBoxes(t *testing.T, s *Server) url.Values { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/settings/notifications", nil) + rr := httptest.NewRecorder() + s.settingsNotificationsPageHandler(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("render: HTTP %d", rr.Code) + } + body := rr.Body.String() + if !strings.Contains(body, "event_backup_failed") { + t.Fatalf("the notifications form did not render — this test would then prove nothing") + } + + // — `checked` before the closing angle. + re := regexp.MustCompile(`]*)>`) + out := url.Values{} + for _, m := range re.FindAllStringSubmatch(body, -1) { + if strings.Contains(m[2], "checked") { + out.Set(m[1], "on") + } + } + return out +} + +func TestR329Part4_RoundTripIsByteIdentical(t *testing.T) { + cases := []struct { + name string + stored []string + }{ + { + // SHAPE 1: the default list every provisioned customer starts with — it contains BOTH + // halves of BOTH compounds, and in an order that is NOT the save handler's order. + name: "defaults", + stored: append([]string(nil), settings.DefaultEnabledEvents...), + }, + { + // SHAPE 2: a customer who switched the compounds OFF — neither key present. + name: "compounds off", + stored: []string{"backup_failed", "node_down", "health_critical"}, + }, + { + // SHAPE 3: written by the OLD handler, so the compound pairs sit in its exact order. + name: "as the old handler wrote it", + stored: []string{ + "backup_failed", "db_dump_failed", "storage_disconnected", "node_down", + "health_critical", "storage_reconnected", + "disk_warning", "disk_critical", "expected_backup_missed", "expected_dbdump_missed", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s, sett := notifyGuardServer(t) + if err := sett.SetNotificationPrefs(&settings.NotificationPrefs{ + Email: "seed@felhom.eu", EnabledEvents: tc.stored, CooldownHours: 6, + }); err != nil { + t.Fatal(err) + } + before := append([]string(nil), sett.GetNotificationPrefs().EnabledEvents...) + + // Render, take exactly what the template ticked, post it back unchanged. + form := checkedBoxes(t, s) + if len(form) == 0 && len(tc.stored) > 0 { + t.Fatalf("the template ticked NOTHING for a customer with %d stored events — the "+ + "round trip would trivially 'pass' while silently wiping every setting", len(tc.stored)) + } + form.Set("notification_email", "seed@felhom.eu") + form.Set("cooldown_hours", "6") + + rr := postNotifications(t, s, form) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + + after := sett.GetNotificationPrefs().EnabledEvents + if !reflect.DeepEqual(before, after) { + t.Errorf("a no-op save CHANGED the stored settings.\n before: %v\n after: %v\n"+ + "A settings page must not rewrite a setting while merely rendering it.", before, after) + } + }) + } +} + +// The split itself: each half is now independently switchable. The whole point is that turning off +// "disk nearly full" must NOT turn off "disk failing". +func TestR329Part4_TheTwoDiskAlarmsAreIndependent(t *testing.T) { + s, sett := notifyGuardServer(t) + + // Only the FAILING alarm on — the mild one off. + rr := postNotifications(t, s, url.Values{ + "notification_email": {"a@b.hu"}, + "cooldown_hours": {"6"}, + "event_disk_critical": {"on"}, + }) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + got := sett.GetNotificationPrefs().EnabledEvents + if !reflect.DeepEqual(got, []string{"disk_critical"}) { + t.Fatalf("enabled = %v, want exactly [disk_critical] — a customer must be able to keep the "+ + "drive-is-failing alarm while silencing the 90%%-full notice", got) + } + + // And the mirror: the mild one on, the failing one off. + rr = postNotifications(t, s, url.Values{ + "notification_email": {"a@b.hu"}, + "cooldown_hours": {"6"}, + "event_disk_warning": {"on"}, + }) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + if got := sett.GetNotificationPrefs().EnabledEvents; !reflect.DeepEqual(got, []string{"disk_warning"}) { + t.Fatalf("enabled = %v, want exactly [disk_warning]", got) + } +} + +// The legacy compound form names must still be honoured — a browser left open on the old page, or a +// bookmarked POST, must not silently drop a key. +func TestR329Part4_LegacyCompoundNamesStillWork(t *testing.T) { + s, sett := notifyGuardServer(t) + rr := postNotifications(t, s, url.Values{ + "notification_email": {"a@b.hu"}, + "cooldown_hours": {"6"}, + "event_disk_alerts": {"on"}, + "event_expected_missed": {"on"}, + }) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + got := sett.GetNotificationPrefs().EnabledEvents + want := []string{"disk_warning", "disk_critical", "expected_backup_missed", "expected_dbdump_missed"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("legacy compound POST stored %v, want %v", got, want) + } +} + +// Both the legacy compound AND its replacement in one POST must not double-write a key. +func TestR329Part4_LegacyAndNewTogetherDoNotDuplicate(t *testing.T) { + s, sett := notifyGuardServer(t) + rr := postNotifications(t, s, url.Values{ + "notification_email": {"a@b.hu"}, + "cooldown_hours": {"6"}, + "event_disk_alerts": {"on"}, + "event_disk_warning": {"on"}, + "event_disk_critical": {"on"}, + }) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + got := sett.GetNotificationPrefs().EnabledEvents + if !reflect.DeepEqual(got, []string{"disk_warning", "disk_critical"}) { + t.Fatalf("stored %v — a duplicated key would be pushed to the hub and re-render oddly", got) + } +} + +// R-329 Part 1.3: the app-down toggle exists, is switchable, and is OFF by default. +func TestR329_AppStartFailedToggleExistsAndDefaultsOff(t *testing.T) { + for _, e := range settings.DefaultEnabledEvents { + if e == "app_start_failed" { + t.Fatalf("app_start_failed is in DefaultEnabledEvents — the operator ruled it OFF by " + + "default; the OPERATOR is emailed regardless, via processOperator, which never " + + "consults customer preferences") + } + } + + s, sett := notifyGuardServer(t) + // Default render must not tick it. + if _, ticked := checkedBoxes(t, s)["event_app_start_failed"]; ticked { + t.Errorf("the app-down toggle renders as ON for a fresh customer") + } + // And it must actually be switchable. + rr := postNotifications(t, s, url.Values{ + "notification_email": {"a@b.hu"}, + "cooldown_hours": {"6"}, + "event_app_start_failed": {"on"}, + }) + if rr.Code >= 400 { + t.Fatalf("save: HTTP %d", rr.Code) + } + if got := sett.GetNotificationPrefs().EnabledEvents; !reflect.DeepEqual(got, []string{"app_start_failed"}) { + t.Fatalf("enabled = %v, want [app_start_failed] — a visible toggle that stores nothing is a lie", got) + } +} diff --git a/controller/internal/web/templates/settings_notifications.html b/controller/internal/web/templates/settings_notifications.html index b022c6c..a056731 100644 --- a/controller/internal/web/templates/settings_notifications.html +++ b/controller/internal/web/templates/settings_notifications.html @@ -43,8 +43,12 @@ Távoli mentés — tárhelykeret-figyelmeztetés + + +