v0.222.0: ask whether a supervised member is DEAD before whether one is UNHEALTHY (R-384), and stop promising an undo copy nobody looked for (R-383)
gates / gates (push) Successful in 11s
gates / gates (push) Successful in 11s
R-384. aggregateState returned StateUnhealthy the moment unhealthy > 0, and the R-51 mixed-case block that asks "is a supervised member dead?" sat below it. A two-container app whose database exits goes unhealthy BECAUSE it cannot reach that database - so the 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 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. R-51's 18-hour immich failure, back through a different door. Two things moved, and either alone leaves the defect standing: the supervised test is hoisted above the unhealthy/starting/restarting returns, and "some members are up" now counts ANY member not in the down bucket. The old guard was running > 0, which made the R-51 block unreachable in exactly the case it was written for. IsDownState is byte-identical - unhealthy stays excluded, because an unhealthy container is running and folding it in reintroduces the flapping that exclusion exists to stop. No new state was minted. Only the ORDER changed. The priority comment was rewritten because it asserted an ordering the code no longer has. Three subtests in TestAggregateState_UnchangedBranches were AMENDED: they asserted an unhealthy/starting/restarting member beat an exited peer on unless-stopped, which pinned the defect as settled behaviour. They keep their intent with the down member given a benign policy. R-383. The double-failure message said the previous state's backup EXISTS, built from the returned path without asking the filesystem - and a missing file is one of the two ways that rollback fails. undoCopyPhrase now describes the copy from disk: present, partial, missing (still naming where it should be), or never written. Zero-length counts as missing. Test count 1494 -> 1504. Four red-proofs planted, four seen failing; the two halves of R-384 convict independently.
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user