diff --git a/CHANGELOG.md b/CHANGELOG.md index f2ca61b..c674cb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,88 @@ +## 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) + +**`IsDownState` did NOT change.** That is the first thing to say, because the obvious fix here is the +wrong one. `unhealthy` is still excluded from the down set, byte-identical, for the reason recorded at +`manager.go:41-53`: an unhealthy container is *running*, and folding it in reintroduces the flapping +that exclusion exists to stop. No new container state was minted either — `StateDegraded` already +means exactly this and every consumer already handles it. **The defect was the ORDER of two questions, +and only the order moved.** + +### R-384 — a dead database hid behind its own unhealthy front end + +"Is a SUPERVISED member of this app dead?" and "is a RUNNING member failing its healthcheck?" are two +different questions. `aggregateState` returned `StateUnhealthy` the moment `unhealthy > 0`, and the +R-51 mixed-case block that asks the first question sat **below** it — so the second question was +answering the first, and always won. + +A two-container app whose database exits goes unhealthy seconds later *because* it cannot reach that +database. So the very symptom the dead database causes was what suppressed the alarm for it. +`unhealthy` is not a down state, so `classifyRunStates` never marked the app down, no banner appeared +and `app_start_failed` never fired. + +**Measured live on `demo-hp` 2026-08-22:** `bookstack-db` stopped at 21:27:01 and the F-OBS heartbeat +printed **"0 currently down"** throughout. This is R-51's own 18-hour immich failure returning through +a different door — the dead member hiding behind an *unhealthy survivor* instead of behind three live +helpers. + +**Two things had to move, and either one alone leaves the defect standing:** + +1. **The order.** The supervised-down test is hoisted above the `unhealthy`/`starting`/`restarting` + returns. +2. **The guard.** "Some members are up" now means **any member not in the down bucket** — running, + unhealthy, starting or restarting. The old guard was `running > 0`, counting `StateRunning` alone, + which made the R-51 block **unreachable in exactly the case it was written for**: an unhealthy + survivor beside a dead database counted as nothing up. + +**The benign case is untouched.** A one-shot init/migrate container that has finished has policy +`no`/`on-failure`, `supervisedPolicy` returns false, and the stack reads exactly as before. Without +that filter every app with a migration step would alarm on every start. + +**The priority comment was rewritten**, because it asserted an ordering the code no longer has, and a +comment asserting an invariant the code does not provide is what cost this project four months one +version ago. + +**Three existing subtests were AMENDED, and this is reported rather than buried.** +`TestAggregateState_UnchangedBranches` asserted that an unhealthy / starting / restarting member beat +an `exited` peer that was on `unless-stopped` — that is, it pinned the defect as if it were settled +behaviour. They keep their original intent ("the live member's state wins over a down member") with +the down member given a BENIGN policy, which is the only situation in which that sentence was ever +true. The supervised versions now assert `degraded`. + +**Every `IsDownState` consumer was walked and is named in `REPORT.md`.** Two change deliberately +(`classifyRunStates`, the intended fix; and `bootrecon`, which will now repair a half-started stack at +boot instead of calling it recovered). `isObservedUp` is an allow-list of `{running, starting}` and is +unaffected — verified, not assumed. The quiesce suppression is cycle-keyed and state-blind, so +R-97b's guarantee is untouched. + +### R-383 — the double-failure message promised an undo copy it never looked for + +When BOTH a database replay and its rollback fail, the message ended „a korábbi állapot mentése +megvan: " — *the previous state's backup exists*. It was built from the path `writeSafetyDump` +returned, **without ever asking the filesystem**. One of the two ways the rollback fails is that the +file is gone, so the sentence was most likely to be false in precisely the case it was printed. +Measured twice live, on v0.220.2 and v0.221.1. + +`undoCopyPhrase` now describes the undo copy **from disk**: present (named), partially present (both +halves named), missing (says so, and still names where it should have been), or never written. A +zero-length dump counts as missing — a 0-byte file restores nothing. The filename is **not** simply +dropped: R-351's lesson is that a refusal naming nothing forces a person to remember what the product +already knows. + +### Tests + +`internal/stacks/degraded_test.go` (R-384 unit + production-path wiring), +`cmd/controller/r384_dead_db_alarm_test.go` (the classifier consequence + the quiesce suppression), +`internal/backup/r383_undo_phrase_test.go` (the phrase + an AST seam test that the message is still +wired to the builder). Test count **1494 → 1504**. + +**Red-proofs: four planted, four SEEN FAILING.** The two halves of R-384 convict independently — +reverting the hoist reads `"unhealthy"`, the exact state the live box reported, and narrowing `up` +back to `running` reads `"unhealthy"`/`"starting"`/`"restarting"`. The classifier mutation returns an +empty banner. The R-383 mutation prints the false claim verbatim, and for an empty set printed +`megvan: .` — naming a file that never existed. Guards sit at the layer each defect lives in: the +ordering at `aggregateState`, the consequence at `classifyRunStates`, the claim at the phrase builder. + ## v0.221.1 — the undo-copy prune stopped running because another fix made its guard reachable (2026-08-23, R-361 follow-on) **MinAgent: 0.129.0** (unchanged — no new agent coupling) diff --git a/controller/cmd/controller/r361_classifier_control_test.go b/controller/cmd/controller/r361_classifier_control_test.go index 3fadd1e..760ce72 100644 --- a/controller/cmd/controller/r361_classifier_control_test.go +++ b/controller/cmd/controller/r361_classifier_control_test.go @@ -12,9 +12,15 @@ import ( // Part 3 of the 2026-08-22 R-361 session measured that a HELD app raises no dead-app alarm. That is // an ABSENCE claim, and an absence claim is worthless unless the detector is shown working. On the // live box it was hard to hold a stack in a down state long enough to see one: `aggregateState` -// checks `unhealthy > 0` BEFORE the mixed-case degraded branch (internal/stacks/manager.go), so an -// app whose database dies goes `degraded` for a moment and then `unhealthy` as its own healthcheck -// fails — and `unhealthy` is deliberately not in `IsDownState`. +// checked `unhealthy > 0` BEFORE the mixed-case degraded branch (internal/stacks/manager.go), so an +// app whose database died went `degraded` for a moment and then `unhealthy` as its own healthcheck +// failed — and `unhealthy` is deliberately not in `IsDownState`. +// +// **That sentence described R-384, and nobody filed it.** It was written here as an inconvenience +// while building this control; it was in fact the mechanism by which a dead database raised no alarm +// at all. Fixed in v0.222.0 by asking the supervised-down question FIRST — see +// `internal/stacks/manager.go` aggregateState and TestR384_DeadSupervisedMemberIsAskedAboutFirst. +// The past tense above is deliberate: the ordering it describes is no longer the code's. // // `classifyRunStates` is pure, so the control is exact here rather than a race against health probes. func TestClassifyRunStates_PositiveControl_ADownStackDoesAlarm(t *testing.T) { diff --git a/controller/cmd/controller/r384_dead_db_alarm_test.go b/controller/cmd/controller/r384_dead_db_alarm_test.go new file mode 100644 index 0000000..9a5a5e1 --- /dev/null +++ b/controller/cmd/controller/r384_dead_db_alarm_test.go @@ -0,0 +1,62 @@ +package main + +import ( + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-384, the classifier half of the chain. +// +// `internal/stacks` TestR384_WiresTheDeadDatabaseThroughTheRealPath drives docker ps → aggregateState +// and ends at the STATE. This starts from that state and asserts the CONSEQUENCE: the app is reported +// down, so the banner shows it and `NotifyAppStartFailures` has something to fire on. Asserting only +// the state would repeat case #9 of the invariant table — mechanism pinned, consequence unpinned. +// +// Measured live on `demo-hp` 2026-08-22: `bookstack-db` stopped at 21:27:01, the stack read +// `unhealthy`, and the F-OBS heartbeat printed "0 currently down" throughout. +// +// RED-PROOF (observed): with the R-384 hoist reverted, the upstream state is `unhealthy`, and feeding +// `unhealthy` here (the second subtest) shows exactly what the live box did — no banner, Down=false. +func TestR384_ADeadDatabaseBehindAnUnhealthyAppAlarms(t *testing.T) { + now := time.Now() + + // What v0.222.0 produces for the bookstack shape. + sts := []stacks.Stack{{Name: "bookstack", Deployed: true, State: stacks.StateDegraded}} + dead, states := classifyRunStates(sts, nil, nil, now) + if len(dead) != 1 || dead[0].Name != "bookstack" { + t.Fatalf("dead-app banner = %+v, want exactly one entry for bookstack", dead) + } + if dead[0].State != string(stacks.StateDegraded) { + t.Errorf("banner state = %q, want %q — the operator must see WHY", dead[0].State, stacks.StateDegraded) + } + if len(states) != 1 || !states[0].Down { + t.Fatalf("run states = %+v, want Down=true (this is what NotifyAppStartFailures reads)", states) + } + + // The pre-fix reading, kept as the contrast that makes the fix legible: `unhealthy` reaching this + // point is silent, and that silence is correct HERE — it is why the fix had to be upstream, in the + // ORDER of the questions, and not by widening IsDownState. + preFix := []stacks.Stack{{Name: "bookstack", Deployed: true, State: stacks.StateUnhealthy}} + deadPre, statesPre := classifyRunStates(preFix, nil, nil, now) + if len(deadPre) != 0 || statesPre[0].Down { + t.Fatalf("unhealthy must remain silent at the classifier: dead=%+v states=%+v — R-384 must "+ + "not have folded unhealthy into the down set", deadPre, statesPre) + } +} + +// Scenario E: the quiesce suppression must still hold for a stack that now reads `degraded`. +// R-97b's defect was telling a customer their app broke during an outage the BACKUP caused, and this +// change must not reopen that door — the suppression is cycle-keyed, so it must be state-blind. +func TestR384_QuiesceSuppressionStillHoldsForDegraded(t *testing.T) { + now := time.Now() + sts := []stacks.Stack{{Name: "bookstack", Deployed: true, State: stacks.StateDegraded}} + dead, states := classifyRunStates(sts, map[string]bool{"bookstack": true}, nil, now) + if len(dead) != 0 { + t.Fatalf("a quiesced stack alarmed: %+v — R-97b's exact defect, back through R-384's door", dead) + } + if states[0].Down { + t.Fatalf("a quiesced stack reported Down=true — the customer would be told the backup broke their app") + } +} diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index ed1a107..251c1bc 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -150,6 +150,54 @@ func (s safetyDumpSet) First() string { return s.Files[0].Path } +// undoCopyPhrase is the sentence the DOUBLE-FAILURE message uses to describe the customer's undo +// copy — and it says what is TRUE, which is the whole of R-383. +// +// THE BUG THIS EXISTS TO KILL. The double-failure branch ended with „a korábbi állapot mentése +// megvan: " — *the previous state's backup EXISTS* — built from the path `writeSafetyDump` +// returned and WITHOUT ever asking the filesystem. But one of the two ways `rollbackSafetyDump` fails +// is that the file is not there, so in exactly the case that sentence is printed it is most likely to +// be false. Measured twice live, on v0.220.2 and v0.221.1. +// +// A false reassurance is worse than no sentence: it is read at the moment the customer is deciding +// whether their data is recoverable, and it points support at a file that is not there. +// +// WHY NOT SIMPLY DROP THE FILENAME. R-351's lesson: a refusal that names nothing forces a person to +// remember what the product already knows. The operator needs the path either way — to fetch the +// file, or to look for it. So the absent case still names WHERE it should have been, and says +// plainly that it is not there. +// +// The check is `os.Stat`, deliberately not a readability or integrity test: this runs at the end of a +// failed restore on a machine that may be unwell, and the honest claim available here is presence. +// A zero-length file is reported as MISSING — a 0-byte dump restores nothing, and calling it present +// is the same false reassurance one step smaller. +func undoCopyPhrase(set safetyDumpSet) string { + var present, absent []string + for _, f := range set.Files { + if f.Path == "" { + continue + } + if st, err := os.Stat(f.Path); err == nil && !st.IsDir() && st.Size() > 0 { + present = append(present, filepath.Base(f.Path)) + continue + } + absent = append(absent, filepath.Base(f.Path)) + } + switch { + case len(present) > 0 && len(absent) == 0: + return "a korábbi állapot mentése megvan: " + strings.Join(present, ", ") + case len(present) > 0: + // Partial: name both halves. An app with two databases whose undo is half there is a + // different situation from either whole one, and support must not have to guess which. + return "a korábbi állapot mentése RÉSZBEN van meg — megvan: " + strings.Join(present, ", ") + + "; HIÁNYZIK: " + strings.Join(absent, ", ") + case len(absent) > 0: + return "a korábbi állapot mentését NEM találjuk a helyén (" + strings.Join(absent, ", ") + ")" + default: + return "a korábbi állapotról nem készült menthető másolat" + } +} + // writeSafetyDump dumps every live database of stack into the app's unit db-dumps dir under the // `pre-restore-` prefix, and returns the SET it wrote. Returns (zero, nil) when the app has no // database at all — a no-DB app has nothing to undo and must flow exactly as it did before @@ -704,9 +752,12 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack // operator ruling, 2026-08-22. m.logger.Printf("[ERROR] [offbox] %s: ROLLBACK ALSO FAILED (%v) — holding the app stopped; replay error was: %v", stack, rbErr, iErr) m.holdAppAfterFailedRollback(stack, iErr, rbErr) + // R-383: the undo copy is DESCRIBED FROM DISK, never from the path alone. See + // undoCopyPhrase — this sentence used to assert the file existed in exactly the + // branch where a missing file is one of the two causes. return res, fmt.Errorf("a(z) %s adatbázisának visszaállítása sikertelen, és a korábbi állapot visszatöltése sem sikerült. "+ "Az alkalmazást biztonsági okból LEÁLLÍTVA hagytuk, hogy az adatai ne sérüljenek tovább. "+ - "Vedd fel velünk a kapcsolatot — a korábbi állapot mentése megvan: %s", stack, filepath.Base(safety)) + "Vedd fel velünk a kapcsolatot — %s", stack, undoCopyPhrase(safetySet)) } res.RolledBack = true if sErr := restartStack(); sErr != nil { diff --git a/controller/internal/backup/r383_undo_phrase_test.go b/controller/internal/backup/r383_undo_phrase_test.go new file mode 100644 index 0000000..31473e7 --- /dev/null +++ b/controller/internal/backup/r383_undo_phrase_test.go @@ -0,0 +1,167 @@ +package backup + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-383 — the double-failure message claimed the undo copy EXISTED without ever asking the disk. +// +// THE DEFECT. The branch where BOTH the replay and the rollback failed ended with +// „a korábbi állapot mentése megvan: ". The filename came from the path `writeSafetyDump` +// returned. One of the two ways `rollbackSafetyDump` fails is that the file is NOT THERE — so the +// sentence was most likely to be false in precisely the case it was printed. Measured twice live, on +// v0.220.2 and v0.221.1. +// +// THE LAYER. The guard sits on the phrase builder, because that is where the claim is MADE. A test +// at the reconstitute level would need a whole failed off-site restore to reach one sentence, and the +// AST test below is what pins that the sentence is still wired to this builder. +// +// RED-PROOF (observed, see REPORT.md): replace undoCopyPhrase's body with the pre-fix shape +// +// return "a korábbi állapot mentése megvan: " + filepath.Base(set.First()) +// +// and TestR383_AbsentUndoCopyIsNotClaimedToExist fails with the message asserting the file exists. +func TestR383_AbsentUndoCopyIsNotClaimedToExist(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "pre-restore-20260823T120000Z-app-postgres.sql") + if err := os.WriteFile(real, []byte("-- a real dump\n"), 0o600); err != nil { + t.Fatal(err) + } + gone := filepath.Join(dir, "pre-restore-20260823T120000Z-app-mariadb.sql") + empty := filepath.Join(dir, "pre-restore-20260823T120000Z-app-empty.sql") + if err := os.WriteFile(empty, nil, 0o600); err != nil { + t.Fatal(err) + } + + set := func(paths ...string) safetyDumpSet { + s := safetyDumpSet{Stamp: "20260823T120000Z"} + for _, p := range paths { + s.Files = append(s.Files, safetyDumpFile{Path: p}) + } + return s + } + + cases := []struct { + name string + set safetyDumpSet + mustContain []string + mustNotHave []string + }{ + { + name: "present — the operator still gets the filename", + set: set(real), + mustContain: []string{"megvan", filepath.Base(real)}, + }, + { + // THE DEFECT ITSELF: the file is gone and the sentence used to say it was there. + name: "absent — must NOT claim it exists, must still name where it should be", + set: set(gone), + mustContain: []string{"NEM találjuk", filepath.Base(gone)}, + mustNotHave: []string{"mentése megvan"}, + }, + { + // A 0-byte dump restores nothing. Calling it present is the same false reassurance. + name: "zero-length — counts as missing", + set: set(empty), + mustContain: []string{"NEM találjuk", filepath.Base(empty)}, + mustNotHave: []string{"mentése megvan"}, + }, + { + name: "partial — both halves named, neither hidden", + set: set(real, gone), + mustContain: []string{"RÉSZBEN", filepath.Base(real), "HIÁNYZIK", filepath.Base(gone)}, + }, + { + name: "no undo was ever written — said plainly, not silently", + set: set(), + mustContain: []string{"nem készült"}, + mustNotHave: []string{"megvan"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := undoCopyPhrase(tc.set) + for _, want := range tc.mustContain { + if !strings.Contains(got, want) { + t.Errorf("phrase %q does not contain %q", got, want) + } + } + for _, bad := range tc.mustNotHave { + if strings.Contains(got, bad) { + t.Errorf("phrase %q contains %q — it asserts a file that is not on disk", got, bad) + } + } + }) + } +} + +// The seam, through production wiring (§10). A correct phrase builder nobody calls is R-106's defect +// — "seam built but never wired", four instances in this project. This walks the AST rather than +// grepping for a substring, so a commented-out call or a similarly-named local cannot satisfy it. +func TestR383_TheDoubleFailureMessageIsWiredToTheBuilder(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "offbox_reconstitute.go", nil, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + var holdCalled, phraseCalled 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.SelectorExpr: + if fn.Sel.Name == "holdAppAfterFailedRollback" { + holdCalled = true + } + case *ast.Ident: + if fn.Name == "undoCopyPhrase" { + phraseCalled = true + } + } + return true + }) + if !holdCalled { + t.Fatal("holdAppAfterFailedRollback is no longer called — the double-failure branch moved; " + + "re-point this test before trusting it") + } + if !phraseCalled { + t.Fatal("undoCopyPhrase is never called from offbox_reconstitute.go — the message is back to " + + "asserting a file it did not check (R-383)") + } + + // And the claim must not be re-typed OUTSIDE the builder. Inside undoCopyPhrase it is the + // verified branch and belongs there; anywhere else it is unconditional again, which is R-383. + // The builder's extent comes from the AST, so this cannot be defeated by moving the function. + var lo, hi token.Pos + for _, d := range f.Decls { + if fd, ok := d.(*ast.FuncDecl); ok && fd.Name.Name == "undoCopyPhrase" { + lo, hi = fd.Pos(), fd.End() + } + } + if lo == token.NoPos { + t.Fatal("undoCopyPhrase is not declared in offbox_reconstitute.go") + } + ast.Inspect(f, func(n ast.Node) bool { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + if lit.Pos() >= lo && lit.End() <= hi { + return true // inside the builder — the verified branch + } + if strings.Contains(lit.Value, "állapot mentése megvan") { + t.Errorf("%s: the unconditional claim is back outside undoCopyPhrase: %s", + fset.Position(lit.Pos()), lit.Value) + } + return true + }) +} diff --git a/controller/internal/stacks/degraded_test.go b/controller/internal/stacks/degraded_test.go index 47f7443..80b4d3e 100644 --- a/controller/internal/stacks/degraded_test.go +++ b/controller/internal/stacks/degraded_test.go @@ -87,30 +87,171 @@ func TestAggregateState_OneShotExitedMemberIsBenign(t *testing.T) { } // The unchanged branches: R-51 must not move any state the pre-existing aggregation produced. +// +// R-384 (v0.222.0) AMENDED THREE OF THESE CASES, and the amendment is the fix, not an accommodation. +// They read `{a: unhealthy|starting|restarting, b: EXITED}` with BOTH members on `unless-stopped`, +// and asserted that the live member's state won. That is precisely the defect R-384 closes: member +// `b` is a dead SUPERVISED container, and the case was pinning the wrong answer as if it were +// settled. A dead database beside an unhealthy front end read `unhealthy`, which is not a down +// state, so nothing ever alarmed — measured live on `demo-hp` 2026-08-22. +// +// The cases keep their ORIGINAL INTENT — "the live member's state still wins over a down member" — +// by giving `b` a BENIGN policy, which is the only situation in which that sentence was ever true. +// The supervised versions moved to TestR384_DeadSupervisedMemberIsAskedAboutFirst, where they now +// assert `degraded`. func TestAggregateState_UnchangedBranches(t *testing.T) { all := policyMap(t, map[string]string{"a": "unless-stopped", "b": "unless-stopped"}) + // `b` is a one-shot that finished: the live member's state must still win over it. + benignB := policyMap(t, map[string]string{"a": "unless-stopped", "b": "no"}) cases := []struct { name string containers []ContainerInfo + policy restartPolicyLookup want ContainerState }{ - {"no containers", nil, StateNotDeployed}, - {"all running", []ContainerInfo{{Name: "a", State: StateRunning}, {Name: "b", State: StateRunning}}, StateRunning}, - {"all stopped", []ContainerInfo{{Name: "a", State: StateExited}, {Name: "b", State: StateStopped}}, StateStopped}, - {"any unhealthy wins", []ContainerInfo{{Name: "a", State: StateUnhealthy}, {Name: "b", State: StateExited}}, StateUnhealthy}, - {"any starting wins over exited", []ContainerInfo{{Name: "a", State: StateStarting}, {Name: "b", State: StateExited}}, StateStarting}, - {"any restarting wins over exited", []ContainerInfo{{Name: "a", State: StateRestarting}, {Name: "b", State: StateExited}}, StateRestarting}, - {"single container exited", []ContainerInfo{{Name: "a", State: StateExited}}, StateStopped}, + {"no containers", nil, all, StateNotDeployed}, + {"all running", []ContainerInfo{{Name: "a", State: StateRunning}, {Name: "b", State: StateRunning}}, all, StateRunning}, + {"all stopped", []ContainerInfo{{Name: "a", State: StateExited}, {Name: "b", State: StateStopped}}, all, StateStopped}, + {"any unhealthy wins over a BENIGN exited", []ContainerInfo{{Name: "a", State: StateUnhealthy}, {Name: "b", State: StateExited}}, benignB, StateUnhealthy}, + {"any starting wins over a BENIGN exited", []ContainerInfo{{Name: "a", State: StateStarting}, {Name: "b", State: StateExited}}, benignB, StateStarting}, + {"any restarting wins over a BENIGN exited", []ContainerInfo{{Name: "a", State: StateRestarting}, {Name: "b", State: StateExited}}, benignB, StateRestarting}, + {"single container exited", []ContainerInfo{{Name: "a", State: StateExited}}, all, StateStopped}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := aggregateState(tc.containers, all); got != tc.want { + if got := aggregateState(tc.containers, tc.policy); got != tc.want { t.Fatalf("aggregateState = %q, want %q", got, tc.want) } }) } } +// R-384 (v0.222.0) — the ORDER was the defect, not the `unhealthy` exclusion. +// +// THE LIVE FAILURE THIS PINS. On `demo-hp` 2026-08-22 `bookstack-db` (MariaDB, `unless-stopped`) was +// stopped at 21:27:01. Its front end went `unhealthy` seconds later because it could not reach its +// database. `aggregateState` returned StateUnhealthy at the `unhealthy > 0` line and never reached +// the R-51 mixed-case block, so `classifyRunStates` never marked the app down and the F-OBS heartbeat +// printed "0 currently down" throughout. The dead database hid behind the unhealthy survivor. +// +// Two things had to move, and both are asserted here: +// 1. the supervised-down test runs BEFORE the unhealthy/starting/restarting returns; +// 2. "some members are up" counts ANY member not in the down bucket. The old guard was `running > 0` +// counting StateRunning alone, which made the block unreachable in exactly this case — an +// unhealthy survivor counted as nothing up. +// +// RED-PROOF (observed, see REPORT.md): move the hoisted block back below the `unhealthy > 0` return +// and the `unhealthy survivor` subtest fails with `aggregateState = "unhealthy", want "degraded"` — +// the exact state the live box reported. Narrowing `up` back to `running` alone fails the same +// subtest identically, so BOTH halves of the fix are convicted. +func TestR384_DeadSupervisedMemberIsAskedAboutFirst(t *testing.T) { + supervised := policyMap(t, map[string]string{"web": "unless-stopped", "db": "unless-stopped"}) + cases := []struct { + name string + survivor ContainerState + }{ + {"unhealthy survivor — the bookstack case measured live", StateUnhealthy}, + {"starting survivor", StateStarting}, + {"restarting survivor", StateRestarting}, + {"running survivor — R-51's original case, must not regress", StateRunning}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := aggregateState([]ContainerInfo{ + {Name: "web", State: tc.survivor, Status: "Up 3 minutes (unhealthy)"}, + {Name: "db", State: StateExited, Status: "Exited (0) 3 minutes ago"}, + }, supervised) + if got != StateDegraded { + t.Fatalf("survivor %q beside a dead SUPERVISED member: aggregateState = %q, want %q "+ + "— a dead database must not hide behind it", tc.survivor, got, StateDegraded) + } + // Non-hollow: the state is only worth anything if it actually alarms. + if !IsDownState(got) { + t.Fatalf("IsDownState(%q) = false — the state changed but nothing downstream alarms", got) + } + }) + } +} + +// The fenced act (§12): `IsDownState` must stay byte-identical. R-384 works by asking a PRIOR +// question, never by folding a running-but-failing container into the down set — that is the +// flapping the exclusion exists to stop. This is the consequence-level guard: if a future change +// widens IsDownState instead of reordering, this fails even though R-384's own tests still pass. +func TestR384_IsDownStateWasNotWidened(t *testing.T) { + for _, s := range []ContainerState{StateUnhealthy, StateRestarting, StateStarting, StatePaused, StateUnknown, StateDeploying} { + if IsDownState(s) { + t.Fatalf("IsDownState(%q) = true — R-384 must reorder the question, never widen the down set", s) + } + } + for _, s := range []ContainerState{StateStopped, StateExited, StateDegraded} { + if !IsDownState(s) { + t.Fatalf("IsDownState(%q) = false — R-384 must not narrow the down set either", s) + } + } +} + +// Scenario C, at the layer the defect would live: a migration step that finished must never alarm, +// and R-384 hoisted the code that decides it. Every catalog app with an init container passes through +// this, so a regression here alarms fleet-wide on every start. +func TestR384_FinishedOneShotStaysBenignBehindAnUnhealthyMember(t *testing.T) { + for _, policy := range []string{"no", "on-failure"} { + t.Run(policy, func(t *testing.T) { + got := aggregateState([]ContainerInfo{ + {Name: "app-web", State: StateUnhealthy, Status: "Up 1 minute (unhealthy)"}, + {Name: "app-migrate", State: StateExited, Status: "Exited (0) 1 minute ago"}, + }, policyMap(t, map[string]string{"app-web": "unless-stopped", "app-migrate": policy})) + if got != StateUnhealthy { + t.Fatalf("policy %q: aggregateState = %q, want %q — a finished migration is benign "+ + "and must not turn an unhealthy app into a down one", policy, got, StateUnhealthy) + } + if IsDownState(got) { + t.Fatalf("policy %q: a finished one-shot made the stack alarm", policy) + } + }) + } +} + +// Scenario B, unchanged and byte-identical: unhealthy with NOTHING dead must stay unhealthy and must +// not alarm. This is the flapping case the IsDownState exclusion exists to stop, and re-creating it +// would be the over-correction rather than the fix. +func TestR384_UnhealthyWithNothingDeadDoesNotAlarm(t *testing.T) { + for _, containers := range [][]ContainerInfo{ + {{Name: "solo", State: StateUnhealthy}}, + {{Name: "a", State: StateUnhealthy}, {Name: "b", State: StateUnhealthy}}, + {{Name: "a", State: StateUnhealthy}, {Name: "b", State: StateRunning}}, + } { + got := aggregateState(containers, policyMap(t, map[string]string{"solo": "unless-stopped", "a": "unless-stopped", "b": "unless-stopped"})) + if got != StateUnhealthy { + t.Fatalf("%d container(s), none down: aggregateState = %q, want %q", len(containers), got, StateUnhealthy) + } + if IsDownState(got) { + t.Fatalf("%d container(s), none down: the stack alarmed", len(containers)) + } + } +} + +// Scenario F: every member down. R-384 is a MEASUREMENT here, not a change (§4 of the task) — the +// all-down path must be byte-identical, so that whatever the live walk finds about single-container +// crashes is a separate, later decision and not something this change quietly moved. +func TestR384_AllMembersDownIsUnchanged(t *testing.T) { + supervised := policyMap(t, map[string]string{"a": "unless-stopped", "b": "unless-stopped"}) + cases := []struct { + name string + containers []ContainerInfo + }{ + {"single exited", []ContainerInfo{{Name: "a", State: StateExited}}}, + {"single stopped", []ContainerInfo{{Name: "a", State: StateStopped}}}, + {"both down", []ContainerInfo{{Name: "a", State: StateExited}, {Name: "b", State: StateStopped}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := aggregateState(tc.containers, supervised); got != StateStopped { + t.Fatalf("aggregateState = %q, want %q — R-384 must not move the all-down path", got, StateStopped) + } + }) + } +} + // The unhealthy/restarting/paused/unknown exclusions are the fix-3 contract (downstate_test.go owns // them). This asserts the ONE addition, so a future reader can see R-51 widened the set by exactly // one state and by nothing else. @@ -277,3 +418,52 @@ func TestRefreshStatus_InspectFailureStillDegrades(t *testing.T) { t.Fatalf("failed inspect was cached: %v", m.restartPolicyCache) } } + +// R-384 production-path wiring: the bookstack shape, through RefreshStatus. +// +// This is the SAME chain the box runs — docker ps → aggregateState → docker inspect → the stack map — +// with the fixture measured live on `demo-hp` 2026-08-22: `bookstack-db` stopped, `bookstack` itself +// still up but failing its healthcheck because it cannot reach its database. An aggregateState-only +// test proves the function and not the caller, and this defect lived in the caller's reading of it. +// +// The classifier half of the chain is pinned by +// TestR384_ADeadDatabaseBehindAnUnhealthyAppAlarms in cmd/controller — this test ends at the state, +// that one starts from it, and TestR384_IsDownStateWasNotWidened pins the join. +// +// RED-PROOF (observed): with the hoisted block moved back below the `unhealthy > 0` return, this +// fails with `bookstack state = "unhealthy", want "degraded"`. +func TestR384_WiresTheDeadDatabaseThroughTheRealPath(t *testing.T) { + dock := &scriptedDocker{ + ps: strings.Join([]string{ + psLine("bookstack", "running", "Up 3 minutes (unhealthy)", "bookstack"), + psLine("bookstack-db", "exited", "Exited (0) 3 minutes ago", "bookstack"), + psLine("docmost", "running", "Up 3 hours (healthy)", "docmost"), + }, "\n"), + policies: map[string]string{"bookstack-db": "unless-stopped"}, + } + m := &Manager{ + cfg: &config.Config{}, + logger: log.New(io.Discard, "", 0), + execFn: dock.exec, + stacks: map[string]*Stack{ + "bookstack": {Name: "bookstack", Deployed: true}, + "docmost": {Name: "docmost", Deployed: true}, + }, + } + if err := m.RefreshStatus(); err != nil { + t.Fatalf("RefreshStatus: %v", err) + } + got := m.stacks["bookstack"].State + if got != StateDegraded { + t.Fatalf("bookstack state = %q, want %q — a dead database must not hide behind its own "+ + "unhealthy front end (measured live 2026-08-22: this read %q and nothing alarmed)", + got, StateDegraded, StateUnhealthy) + } + // Non-hollow: the state only matters because it alarms. + if !IsDownState(got) { + t.Fatalf("IsDownState(%q) = false — the state moved but the app is still silently down", got) + } + if got := m.stacks["docmost"].State; got != StateRunning { + t.Fatalf("docmost state = %q, want %q — a healthy app must be untouched", got, StateRunning) + } +} diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index a513392..25b8997 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -775,10 +775,49 @@ func supervisedPolicy(policy string) bool { } // aggregateState determines the overall stack state from its containers. -// Priority: unhealthy/starting > restarting > all-running > degraded > stopped // -// policyOf is consulted ONLY for the mixed case (some members up, some down) and ONLY for the down -// members — see the mix branch. nil is allowed (every exited member then reads as supervised). +// Priority: **degraded > unhealthy/starting > restarting > all-running > stopped** +// +// R-384 (v0.222.0) MOVED `degraded` to the front, and the move is the whole fix. It used to sit last, +// below the `unhealthy > 0` return, which meant a different question was answering it. +// +// ── WHY DEGRADED IS ASKED FIRST ────────────────────────────────────────────────────────────── +// +// "Is a SUPERVISED member of this app dead?" and "is a RUNNING member failing its healthcheck?" are +// two different questions, and until v0.222.0 the second was allowed to answer the first. A two- +// container app whose database exits will normally have its front end go `unhealthy` moments later — +// it cannot reach its database. The old order returned `StateUnhealthy` immediately at that point and +// never reached the R-51 mixed-case block below, so the dead database was never asked about. +// `unhealthy` is deliberately NOT a down state (see IsDownState), so `classifyRunStates` never marked +// the app down and `NotifyAppStartFailures` never fired. +// +// **Measured live on `demo-hp` 2026-08-22:** `bookstack-db` stopped at 21:27:01 and the F-OBS +// heartbeat printed "0 currently down" throughout. That is R-51's own 18-hour immich failure back +// through a different door — the dead member now hiding behind an unhealthy survivor instead of +// behind three live helpers. +// +// ── WHAT "SOME MEMBERS ARE UP" MEANS, AND WHY IT WIDENED ───────────────────────────────────── +// +// The R-51 block was additionally guarded by `running > 0`, where `running` counts only StateRunning. +// That guard made the block UNREACHABLE in exactly the case it was written for: an unhealthy survivor +// beside a dead database counted as nothing up. "Up" now means **any member not in the down bucket** — +// running, unhealthy, starting or restarting — because each of those is a container Docker still has, +// and a dead supervised peer beside any of them is a fault either way. +// +// ── WHAT DID NOT CHANGE, DELIBERATELY ──────────────────────────────────────────────────────── +// +// `IsDownState` is byte-identical. `unhealthy` is still excluded from it, for the reason recorded +// there: an unhealthy container is RUNNING, and folding it in reintroduces the flapping that +// exclusion exists to stop. This change does not fold it in — it asks a prior question first. No new +// state was minted either: `StateDegraded` already means precisely this and every consumer already +// handles it. +// +// The benign case is untouched: a one-shot init/migrate container that has legitimately finished has +// policy `no`/`on-failure`, `supervisedPolicy` returns false, and the stack reads exactly as before. +// Without that filter every app with a migration step would alarm on every start. +// +// policyOf is consulted ONLY when some members are up and some are down, and ONLY for the down +// members. nil is allowed (every exited member then reads as supervised). func aggregateState(containers []ContainerInfo, policyOf restartPolicyLookup) ContainerState { if len(containers) == 0 { return StateNotDeployed @@ -809,6 +848,22 @@ func aggregateState(containers []ContainerInfo, policyOf restartPolicyLookup) Co total := len(containers) + // R-384 — ASKED FIRST. "Is a supervised member dead?" must be answered before "is a running + // member unhealthy?", because a dying database drags its front end unhealthy and the unhealthy + // return below then swallowed the whole question. `up` counts every member NOT in the down + // bucket; `running > 0` alone made this unreachable in the exact case R-51 was written for. + if up := running + unhealthy + starting + restarting; up > 0 && len(down) > 0 { + for _, c := range down { + policy := "" + if policyOf != nil { + policy = policyOf(c.Name) + } + if supervisedPolicy(policy) { + return StateDegraded + } + } + } + // Any unhealthy → whole stack is unhealthy if unhealthy > 0 { return StateUnhealthy @@ -832,19 +887,14 @@ func aggregateState(containers []ContainerInfo, policyOf restartPolicyLookup) Co // Mix (some members up, some down) — R-51. Until v0.156.0 this reported StateRunning // unconditionally ("partial"), which is why a dead immich-server behind three live helpers was // invisible to fix-3 for 18 hours. A down member whose restart policy says docker should be - // keeping it up is a FAULT → the whole stack is degraded (and degraded is a down state). A down - // member with policy `no`/`on-failure` is a one-shot init/migrate container that has legitimately - // finished → benign, the stack stays running. + // keeping it up is a FAULT → the whole stack is degraded (and degraded is a down state). + // + // R-384 (v0.222.0) HOISTED that supervised test to the top of this function, so by the time + // control reaches here every down member has already been proven BENIGN — policy `no`/`on-failure`, + // a one-shot init/migrate container that has legitimately finished. The stack stays running. This + // branch therefore no longer decides anything about supervision; it only records the benign + // verdict, and the test that pins it is the one that says a finished migration must not alarm. if running > 0 { - for _, c := range down { - policy := "" - if policyOf != nil { - policy = policyOf(c.Name) - } - if supervisedPolicy(policy) { - return StateDegraded - } - } return StateRunning }