v0.223.0: the app-down alarm reached nobody (R-329), and the stop nobody heard (R-386)
gates / gates (push) Successful in 11s
gates / gates (push) Successful in 11s
R-329. NotifyAppStartFailures emitted severity "warn". The hub accepts exactly
{info, warning, error, critical} and silently coerces anything else to "info",
which severityNotifies then drops BEFORE both legs. Banner shown, event stored,
POST 200, no mail sent. One word.
This is the second time: DiskAlertKind.Severity emitted "warn" until v0.215.0
and its own comment records that every warning-level disk alert went to nobody.
A comment recorded the lesson and nothing enforced it. The guard is now an AST
walk over the whole controller - grep cannot work here, since "warn" appears
legitimately nine times as a healthcheck status vocabulary.
The sweep found exactly one bad severity. Its limits are stated: the walk cannot
follow a variable, so all six dynamic call sites are registered by name with the
values each can take, and a new one fails the test. Two of the six were found by
the guard, not by the hand sweep before it.
Also pinned: fillwatch.Band.Severity() returns "" for BandOK, which would vanish
the same way. It is unreachable because Check() notifies only on escalation -
but that safety lives in a different function from the one that looks unsafe, so
the test asserts the consequence rather than the mapping.
app_start_failed gains a customer toggle, DEFAULT OFF, per operator ruling. The
operator is mailed either way: processOperator never consults customer prefs.
It is deliberately NOT in operatorOnlyEvents, which would make the toggle a lie.
R-386. classifyRunStates decided "the customer stopped this" from the STATE, so
every stopped stack was assumed deliberate. Measured on demo-hp: privatebin
stopped out of band, nine scans, zero events, zero banner - while the comment
beside it claimed an out-of-band stop still alerts.
DesiredState already records the answer and has exactly one writer. Stopped ->
no alarm; Running -> alarm; absent -> UNKNOWN, keep today's behaviour AND say
so. Absent stays silent deliberately: reading it as "nobody asked" would email
about every app anyone ever stopped, fleet-wide, on the first cycle after
upgrade. The gap is bounded not silent - IntentUnknown is set and the names are
logged at INFO on the heartbeat cadence. failedRestart still lifts a Stopped
intent, or F-CRIT-1 re-opens. No new DesiredState writer.
Two settings toggles each governed two alarms. "Lemez figyelmeztetes (90%+)"
also wrote disk_critical, the drive-is-FAILING alarm. Now four honest toggles;
12 became 15. A no-op save stores the existing slice verbatim, so byte identity
is by construction - without that guard the defaults case reorders, which the
red-proof caught.
Test count 1504 -> 1522. Five red-proofs, five seen failing; one passed first
time and is reported - that mutation was inert, not the test weak.
This commit is contained in:
+101
@@ -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)
|
||||
|
||||
|
||||
@@ -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)})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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))
|
||||
}
|
||||
@@ -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})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
})
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
// <input type="checkbox" name="event_x" ... checked> — `checked` before the closing angle.
|
||||
re := regexp.MustCompile(`<input type="checkbox" name="(event_[a-z_]+)"([^>]*)>`)
|
||||
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)
|
||||
}
|
||||
}
|
||||
@@ -43,8 +43,12 @@
|
||||
<span class="toggle-label">Távoli mentés — tárhelykeret-figyelmeztetés</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_disk_alerts" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "disk_warning"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Lemez figyelmeztetés (90%+)</span>
|
||||
<input type="checkbox" name="event_disk_warning" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "disk_warning"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Lemez betelőben (90% felett)</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_disk_critical" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "disk_critical"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Lemez megtelt vagy meghibásodott</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_storage_disconnected" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "storage_disconnected"}}checked{{end}}{{end}}{{end}}>
|
||||
@@ -59,8 +63,16 @@
|
||||
<span class="toggle-label">Rendszer állapot kritikus</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_expected_missed" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "expected_backup_missed"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Elvárt mentés elmaradt</span>
|
||||
<input type="checkbox" name="event_expected_backup_missed" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "expected_backup_missed"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Elvárt fájlmentés elmaradt</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_expected_dbdump_missed" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "expected_dbdump_missed"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Elvárt adatbázismentés elmaradt</span>
|
||||
</label>
|
||||
<label class="toggle">
|
||||
<input type="checkbox" name="event_app_start_failed" {{with .NotificationPrefs}}{{range .EnabledEvents}}{{if eq . "app_start_failed"}}checked{{end}}{{end}}{{end}}>
|
||||
<span class="toggle-label">Alkalmazás nem fut</span>
|
||||
</label>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
Reference in New Issue
Block a user