diff --git a/CHANGELOG.md b/CHANGELOG.md index 499f91a..eb864f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,43 @@ ## Changelog +### v0.178.0 — R-88 Part 2 (controller) + R-97c comment fix (2026-07-27) — **MinAgent: 0.105.0** for the age_state semantics + +**The safety valve now needs a licence.** `scheduledRunAllowed` fired on ANY nil age — "no recorded +backup yet, never withhold the first one". With agent v0.105.0 the age carries a STATE, and only a +POSITIVE claim licenses the bypass: + +| `age_state` | licenses the valve? | why | +|---|---|---| +| `absent` | **yes** | the agent looked; there is genuinely nothing there | +| `unknown` | **no** | unreadable storage — this is the whole fix | +| `known` | n/a | a real age; the age comparison decides | +| *(empty)* | **yes** | pre-v0.105.0 agent — see below | + +**A missing field means LEGACY, not unknown, and that is deliberate.** Reading an old agent's silence +as "unknown" looks safer and regresses Scenario D: the valve would stop firing on every un-upgraded +box, so a genuinely new box would never take its first backup outside its window and nobody would +notice for weeks. Preserving the KNOWN behaviour is correct; the MinAgent floor drives the upgrade. +The degrade is logged **once** per process, the `logTierDegradeOnce` shape. An unrecognised FUTURE +value also maps to legacy — a newer agent inventing a fourth state must not inherit "unknown" +semantics from a controller that has never heard of it. + +**Caught while doing it, and worth naming:** `TieredBackend` is satisfied by a RUNTIME type assertion +in `resolveDueTiers`, so when `DueFor`'s signature changed the whole repo still built and vetted +clean while `quiesceBackend` silently stopped satisfying the interface — which would have degraded +every box to the untargeted single-tier path, losing R-82's multi-tier backups entirely, with no +error anywhere. `TestQuiesceBackendSatisfiesTieredBackend` is now the compile-time witness. Sixth +instance of the inert-seam class. + +**R-97c follow-through:** the comment in `internal/notify` claiming these event types are +operator-only "because they have no customerMessages entry" was **wrong** and is corrected — the hub +falls back to the raw message when the entry is missing, and the only customer gate is +`prefs.EnabledEvents`. Enforcement is hub-side `operatorOnlyEvents` (hub >= v0.79.0). + +Unchanged: the R-88 Part 1 breaker and its timings, the window bounds, `dropBackedOffTiers`, and +`TriggerNow` (still ungated by everything). + +Tests +7 (6 age-state + 1 interface witness); 27 packages ok. Red-proofs observed for Scenarios A, B and C. + ### v0.177.0 — R-97: a failing backup is HEARD, and stops blaming the apps (2026-07-27) — MinAgent unchanged; requires hub >= v0.78.0 **R-97a — the whole-guest tier had no route to the hub.** `internal/quiesce` did not import diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index f7ba682..300618a 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1741,9 +1741,11 @@ func (b quiesceBackend) Tiers(ctx context.Context) ([]quiesce.BackupTier, error) return out, nil } -func (b quiesceBackend) DueFor(ctx context.Context, target string) (bool, *int64, error) { +func (b quiesceBackend) DueFor(ctx context.Context, target string) (bool, *int64, string, error) { r, err := b.c.BackupDueFor(ctx, target) - return r.Due, r.AgeSecs, err + // AgeState is passed through RAW; quiesce.ageStateFromWire owns the mapping, including the + // legacy-vs-unknown distinction. An empty string here means a pre-v0.105.0 agent. + return r.Due, r.AgeSecs, r.AgeState, err } func (b quiesceBackend) StartBackupFor(ctx context.Context, target string) (string, error) { r, err := b.c.StartBackupFor(ctx, target) diff --git a/controller/cmd/controller/tieredbackend_assert_test.go b/controller/cmd/controller/tieredbackend_assert_test.go new file mode 100644 index 0000000..28df536 --- /dev/null +++ b/controller/cmd/controller/tieredbackend_assert_test.go @@ -0,0 +1,19 @@ +package main + +import ( + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/quiesce" +) + +// R-88 Part 2 — TieredBackend is satisfied by a RUNTIME type assertion in `resolveDueTiers` +// (`l.backend.(TieredBackend)`), NOT at compile time. So when DueFor's signature changed, the whole +// repo still built and vetted clean while `quiesceBackend` silently stopped satisfying the +// interface — which would have degraded every box to the untargeted single-tier path, losing R-82's +// multi-tier backups entirely, with no error anywhere. +// +// This compile-time assertion is the only thing that catches it. It caught it during R-88 Part 2. +// Do not delete it; a runtime-asserted interface needs a compile-time witness. +func TestQuiesceBackendSatisfiesTieredBackend(t *testing.T) { + var _ quiesce.TieredBackend = quiesceBackend{} +} diff --git a/controller/internal/agentapi/client.go b/controller/internal/agentapi/client.go index 7968f14..46542a5 100644 --- a/controller/internal/agentapi/client.go +++ b/controller/internal/agentapi/client.go @@ -174,6 +174,14 @@ type DueResponse struct { Due bool `json:"due"` Reason string `json:"reason"` AgeSecs *int64 `json:"age_seconds"` + // AgeState (R-88 Part 2, agent >= v0.105.0) says WHY AgeSecs is nil: "absent" (a positive + // determination that no backup has ever landed) or "unknown" (the agent could not tell — + // unreadable storage, unparseable timestamp). "known" accompanies a real age. + // + // EMPTY MEANS LEGACY — an agent older than v0.105.0 simply omits the field. It does NOT mean + // "unknown", and the distinction is load-bearing: see quiesce.ageStateFromWire. Never + // discriminate on Reason instead; those strings are operator copy and will drift. + AgeState string `json:"age_state"` } // BackupResponse mirrors the agent's POST /backup payload. diff --git a/controller/internal/agentapi/features.go b/controller/internal/agentapi/features.go index 85fac1a..cfa841f 100644 --- a/controller/internal/agentapi/features.go +++ b/controller/internal/agentapi/features.go @@ -29,6 +29,12 @@ const FeatureNetstorageVerify Feature = "netstorage_verify" // (GET/POST /guest/memory) shipped together, so GET /guest/memory IS the capability signal. const FeatureGuestMemoryResize Feature = "guest_memory_resize" +// FeatureBackupAgeState is R-88 Part 2 (agent v0.105.0): GET /backup/due carries `age_state`, +// distinguishing "never backed up" (absent) from "could not tell" (unknown). There is no route +// probe for it — the signal is a FIELD on an existing route, so the version floor is the gate and +// an empty field means legacy. +const FeatureBackupAgeState Feature = "backup_age_state" + // SupportState is a probe verdict. The zero value is SupportUnknown (fail-open: unknown never // refuses — the existing agent-error paths speak honestly when the agent is down). type SupportState int @@ -99,6 +105,8 @@ var errNoMemoryProbe = errors.New("agentapi: prober does not support the guest-m var featureMinAgent = map[Feature]string{ FeatureNetstorageVerify: "0.81.0", FeatureGuestMemoryResize: "0.90.0", + // R-88 Part 2: /backup/due carries age_state, distinguishing "never backed up" from "cannot tell". + FeatureBackupAgeState: "0.105.0", } // AgentVersionReporter is optionally implemented by a SupportProber (*Client is one): it reports diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index 2edbe80..af27aee 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -791,8 +791,11 @@ func statusRank(status string) int { // — while it is still retrying behind the R-88 breaker. A customer can take no action on a failed // whole-guest backup; that is the same harm R-97b removes, re-introduced through the front door. // -// These follow the R-85 precedent exactly: a type in the hub's `allowedEventTypes` with NO -// `customerMessages` entry, so the dispatcher structurally cannot route it to a customer. +// Operator-only is enforced hub-side by `notify.operatorOnlyEvents` (hub >= v0.79.0, R-97c), NOT by +// the absence of a `customerMessages` entry — v0.177.0 claimed the latter and was WRONG: the hub +// falls back to the raw message when the entry is missing, and the only customer gate is +// `prefs.EnabledEvents`, which is configuration. Adding a type to the allowlist does NOT make it +// operator-only; it must go in that register too. // // HUB DEPENDENCY: both types MUST be present in the hub's allowedEventTypes or POST /event 400s // (the recorded allowlist gotcha). Do not deploy this controller ahead of that hub change. diff --git a/controller/internal/quiesce/agestate_test.go b/controller/internal/quiesce/agestate_test.go new file mode 100644 index 0000000..01b9a44 --- /dev/null +++ b/controller/internal/quiesce/agestate_test.go @@ -0,0 +1,173 @@ +package quiesce + +import ( + "context" + "log" + "strings" + "testing" +) + +// R-88 Part 2 (controller half) — a nil age is no longer self-licensing. +// +// The valve fires only on a POSITIVE claim of "never backed up". These tests assert BEHAVIOUR (were +// stacks stopped?), never a log line — a controller that logs the right thing and then does the +// wrong thing must fail here. + +// stateBackend is a tierBackend whose DueFor also reports an age_state wire string. +type stateBackend struct { + *tierBackend + wire map[string]string // target → age_state as sent by the agent ("" = legacy) +} + +func (b *stateBackend) DueFor(ctx context.Context, target string) (bool, *int64, string, error) { + due, age, _, err := b.tierBackend.DueFor(ctx, target) + return due, age, b.wire[target], err +} + +func stateLoop(t *testing.T, wire map[string]string, st *fakeStacks, logTo *strings.Builder) *Loop { + t.Helper() + be := newTierBackend() + be.tiers = []BackupTier{{Target: "local"}} + be.dueSet["local"] = true + be.phases["local"] = []string{phaseDone} + sb := &stateBackend{tierBackend: be, wire: wire} + l := windowLoop(t, sb, st, "02:30", atBudapest(12, 0)) // 12:00 — firmly OUTSIDE [04:30, 08:30) + if logTo != nil { + l.logger = log.New(logTo, "", 0) + } + return l +} + +// ── SCENARIO A — UNKNOWN does not bypass the window ────────────────────────────────────────── +// +// COMPANION RED-PROOF (observed): make valveLicensed return true for AgeStateUnknown (the pre-fix +// behaviour, where any nil age fired the valve) and this fails with +// +// "R-88 Part 2: an UNKNOWN age bypassed the backup window and stopped 1 stack(s) — an unreadable +// storage must not masquerade as a first-ever backup" +// +// Restored. +func TestAgeState_UnknownDoesNotBypassTheWindow(t *testing.T) { + st := &fakeStacks{running: []string{"bookstack"}} + l := stateLoop(t, map[string]string{"local": "unknown"}, st, nil) + + if err := l.runOnce(context.Background()); err != nil { + t.Fatal(err) + } + if got := len(st.stoppedNames()); got != 0 { + t.Fatalf("R-88 Part 2: an UNKNOWN age bypassed the backup window and stopped %d stack(s) — "+ + "an unreadable storage must not masquerade as a first-ever backup", got) + } +} + +// ── SCENARIO B — ABSENT still runs outside the window ──────────────────────────────────────── +// +// B is what makes A safe. An implementation that never licensed the valve would pass A and silently +// starve every new box. +// +// COMPANION RED-PROOF (observed): drop AgeStateAbsent from valveLicensed (keeping only legacy) and +// this fails with +// +// "a genuine first-ever backup (absent) must RUN outside the window; 0 stack(s) stopped — the +// safety valve was lost and a new box would starve" +// +// Restored. +func TestAgeState_AbsentStillRunsOutsideTheWindow(t *testing.T) { + st := &fakeStacks{running: []string{"bookstack"}} + l := stateLoop(t, map[string]string{"local": "absent"}, st, nil) + + if err := l.runOnce(context.Background()); err != nil { + t.Fatal(err) + } + if len(st.stoppedNames()) == 0 { + t.Fatal("a genuine first-ever backup (absent) must RUN outside the window; 0 stack(s) stopped — " + + "the safety valve was lost and a new box would starve") + } +} + +// ── SCENARIO C — old agent, new controller: TODAY'S behaviour exactly ──────────────────────── +// +// Asserts BEHAVIOUR, not the degrade log line: a controller that logs the degrade and then defers +// would pass a log-only assertion while silently changing behaviour on every un-upgraded box. +// +// COMPANION RED-PROOF (observed): drop AgeStateLegacy from valveLicensed (treating a missing field +// as unknown — the "safer-looking" choice) and this fails with +// +// "C: a pre-v0.105.0 agent must behave EXACTLY as before — nil age fires the valve. 0 stack(s) +// stopped; an un-upgraded box just silently stopped backing up outside its window" +// +// Restored. +func TestAgeState_LegacyAgentKeepsTodaysBehaviour(t *testing.T) { + st := &fakeStacks{running: []string{"bookstack"}} + var logbuf strings.Builder + l := stateLoop(t, map[string]string{"local": ""}, st, &logbuf) // NO field on the wire + + if err := l.runOnce(context.Background()); err != nil { + t.Fatal(err) + } + if len(st.stoppedNames()) == 0 { + t.Fatal("C: a pre-v0.105.0 agent must behave EXACTLY as before — nil age fires the valve. " + + "0 stack(s) stopped; an un-upgraded box just silently stopped backing up outside its window") + } + // ...and the degrade must be VISIBLE, or a fleet drifts without anyone knowing. + if !strings.Contains(logbuf.String(), "age_state") { + t.Fatalf("the legacy degrade must be logged once; log:\n%s", logbuf.String()) + } +} + +// The degrade is logged ONCE, not every poll. +func TestAgeState_LegacyDegradeLoggedOnce(t *testing.T) { + st := &fakeStacks{running: []string{"bookstack"}} + var logbuf strings.Builder + l := stateLoop(t, map[string]string{"local": ""}, st, &logbuf) + for i := 0; i < 3; i++ { + if err := l.runOnce(context.Background()); err != nil { + t.Fatal(err) + } + } + if n := strings.Count(logbuf.String(), "pre-v0.105.0"); n != 1 { + t.Fatalf("the legacy degrade must be logged ONCE per process, got %d", n) + } +} + +// An unrecognised FUTURE state maps to legacy, not to unknown — a newer agent inventing a fourth +// value must not accidentally acquire "unknown" semantics from a controller that never heard of it. +func TestAgeState_UnrecognisedWireValueIsLegacy(t *testing.T) { + for _, wire := range []string{"", "known", "absent", "unknown", "quantum", "TRUE", "0"} { + got := ageStateFromWire(wire) + switch wire { + case "known", "absent", "unknown": + if string(got) != wire { + t.Errorf("%q must map to itself, got %q", wire, got) + } + default: + if got != AgeStateLegacy { + t.Errorf("%q must map to LEGACY (fail toward known behaviour), got %q", wire, got) + } + } + } +} + +// valveLicensed as a truth table — the contract, independent of the loop. +func TestAgeState_ValveLicenceTable(t *testing.T) { + age := int64(3600) + cases := []struct { + name string + t dueTier + want bool + }{ + {"absent licenses", dueTier{state: AgeStateAbsent}, true}, + {"legacy licenses (un-upgraded agent keeps old behaviour)", dueTier{state: AgeStateLegacy}, true}, + {"unknown does NOT license", dueTier{state: AgeStateUnknown}, false}, + {"known with a real age needs no licence", dueTier{state: AgeStateKnown, ageSecs: &age}, false}, + } + for _, c := range cases { + if got := valveLicensed([]dueTier{c.t}); got != c.want { + t.Errorf("%s: valveLicensed = %v, want %v", c.name, got, c.want) + } + } + // One unknown tier must not be licensed by a sibling that is merely known-with-age. + if valveLicensed([]dueTier{{state: AgeStateUnknown}, {state: AgeStateKnown, ageSecs: &age}}) { + t.Error("a known sibling must not license an unknown tier's valve") + } +} diff --git a/controller/internal/quiesce/invariant_test.go b/controller/internal/quiesce/invariant_test.go index 4a11fd3..90a2bf6 100644 --- a/controller/internal/quiesce/invariant_test.go +++ b/controller/internal/quiesce/invariant_test.go @@ -33,9 +33,10 @@ type agedBackend struct { ages map[string]*int64 } -func (a *agedBackend) DueFor(ctx context.Context, target string) (bool, *int64, error) { - due, _, err := a.tierBackend.DueFor(ctx, target) - return due, a.ages[target], err +func (a *agedBackend) DueFor(ctx context.Context, target string) (bool, *int64, string, error) { + due, _, _, err := a.tierBackend.DueFor(ctx, target) + // A real age implies a KNOWN state — that is what the agent would send. + return due, a.ages[target], string(AgeStateKnown), err } // SCENARIO D — a genuinely never-backed-up box still gets its first backup, outside the window. @@ -114,7 +115,7 @@ func TestContract_SafetyValveBoundary(t *testing.T) { {"past cadence+24h → the valve fires", h(49), true}, } for _, c := range cases { - if got := scheduledRunAllowed(outside, window, c.age, cadence24); got != c.want { + if got := scheduledRunAllowed(outside, window, c.age, true, cadence24); got != c.want { t.Errorf("CONTRACT VIOLATED: %s → scheduledRunAllowed = %v, want %v", c.name, got, c.want) } } diff --git a/controller/internal/quiesce/quiesce.go b/controller/internal/quiesce/quiesce.go index 4c9802d..aca10a1 100644 --- a/controller/internal/quiesce/quiesce.go +++ b/controller/internal/quiesce/quiesce.go @@ -100,6 +100,8 @@ type Loop struct { mu sync.Mutex // degradeOnce reports the pre-R-82 agent fallback exactly once per process (see tiers.go). degradeOnce sync.Once + // ageStateDegradeOnce reports a pre-v0.105.0 agent (no age_state) exactly once (R-88 Part 2). + ageStateDegradeOnce sync.Once // breaker (R-88) defers the QUIESCE for a tier whose backups keep failing, so a broken target // cannot stop the customer's apps every 5 minutes forever. Scheduled path only — see breaker.go. breaker *failureBreaker @@ -224,7 +226,7 @@ func (l *Loop) runOnce(ctx context.Context) error { // cadence+24h" — cannot be suppressed by a fresher sibling tier. if l.windowStartFn != nil { window := l.windowStartFn() - if !scheduledRunAllowed(l.now().In(budapestLocation()), window, oldestAge(dueTiers), l.cadence) { + if !scheduledRunAllowed(l.now().In(budapestLocation()), window, oldestAge(dueTiers), valveLicensed(dueTiers), l.cadence) { from, to := gateBounds(window) l.logger.Printf("[DEBUG] [quiesce] scheduled backup due but outside the backup window [%s–%s) — deferring to the next poll inside it", from, to) return nil @@ -288,6 +290,31 @@ func (l *Loop) dropBackedOffTiers(tiers []dueTier) []dueTier { return kept } +// valveLicensed reports whether ANY due tier holds a POSITIVE claim of "never backed up" — the only +// thing that may fire the window-gate safety valve on a nil age (R-88 Part 2). +// +// Two states license it, and the second is the important one: +// - AgeStateAbsent — the agent looked and there is genuinely nothing there; +// - AgeStateLegacy — a pre-v0.105.0 agent that cannot tell us either way. Preserving the OLD +// behaviour is correct here: reading its silence as "unknown" would stop the valve firing on +// every un-upgraded box, so a genuinely new box would never take its first backup outside its +// window and nobody would notice for weeks. The MinAgent floor drives the upgrade; the valve is +// not the place to force it. +// +// AgeStateUnknown does NOT license it. That is the entire fix: an unreadable storage no longer +// masquerades as a first-ever backup. +func valveLicensed(tiers []dueTier) bool { + for _, t := range tiers { + if t.ageSecs != nil { + continue // a real age needs no licence; the age comparison decides + } + if t.state == AgeStateAbsent || t.state == AgeStateLegacy { + return true + } + } + return false +} + // oldestAge returns the largest (most overdue) age among the due tiers; nil when any tier has never // backed up (nil age = "never", which is maximally overdue and must win). func oldestAge(tiers []dueTier) *int64 { @@ -569,26 +596,28 @@ const ( // absence of a signal was read as a specific value, and each time the fix was the same shape: // give "unknown" its own representation instead of letting it collapse into a real answer. // -// ── WHAT IS AND IS NOT FIXED HERE ──────────────────────────────────────────────────────────── +// ── CLOSED BY R-88 PART 2 (agent v0.105.0 + controller v0.178.0) ───────────────────────────── // -// The nil branch below STILL fires the valve, and that is currently correct-by-necessity, not by -// design: the controller cannot yet tell the two apart. The agent's `/backup/due` returns -// BYTE-IDENTICAL responses for "the storage read errored" and "there has genuinely never been a -// backup" — same `Due: true`, same `Reason: "no successful backup recorded yet"`, same nil -// `AgeSecs`. The root cause is agent-side: `newestArchiveOn` (localapi/server.go) documents that -// errors "degrade to unknown, never to no-backup", but its `(time.Time, bool)` signature cannot -// represent unknown, so the error collapses into a positive claim of "never". +// The value is now THREE-STATE, not merely "nil or not". The agent reports `age_state` on +// /backup/due — `known` / `absent` / `unknown` — and a nil age fires the valve only when +// `valveLicensed` finds a tier holding a POSITIVE claim of "never backed up". // -// Distinguishing them needs a new field on `/backup/due` plus a compat rule in both directions → -// tracked as its own task (R-88 Part 2, agent-side). Until then the R-88 BREAKER is what bounds the -// damage: an unknown-driven cycle may still run once outside the window, but it can no longer repeat -// every 5 minutes. +// It used to be that the agent returned BYTE-IDENTICAL responses for "the storage read errored" and +// "there has genuinely never been a backup" (same Due, same Reason, same nil AgeSecs), because +// `newestArchiveOn`'s `(time.Time, bool)` signature could not represent "unknown" — while its own +// doc comment promised exactly that. An unreadable storage therefore masqueraded as a first-ever +// backup and quiesced customer apps outside the window. Fixed at the source. // -// DO NOT "fix" this by deleting the nil branch. Scenario D — a genuinely never-backed-up box that is -// only ever powered on outside its window — depends on it, and TestContract_NeverBackedUp_RunsOutside -// -TheWindow will fail if you do. Silencing the valve would trade a loud bug for a silent one: a box -// that never backs up at all, with nobody noticing for weeks. -func scheduledRunAllowed(now time.Time, windowStart string, lastAgeSecs *int64, cadence time.Duration) bool { +// STILL LICENSED, DELIBERATELY: `AgeStateLegacy` — a pre-v0.105.0 agent that omits the field. Its +// silence must NOT be read as "unknown", or the valve stops firing on every un-upgraded box and a +// genuinely new box never takes its first backup. The MinAgent floor drives the upgrade instead. +// +// DO NOT "fix" this by deleting the nil branch, or by dropping the legacy case from valveLicensed. +// Scenario D — a genuinely never-backed-up box that is only ever powered on outside its window — +// depends on BOTH, and TestContract_NeverBackedUp_RunsOutsideTheWindow will fail if you do. +// Silencing the valve trades a loud bug for a silent one: a box that never backs up at all, with +// nobody noticing for weeks. +func scheduledRunAllowed(now time.Time, windowStart string, lastAgeSecs *int64, valveOK bool, cadence time.Duration) bool { startMin, err := backupwindow.ParseHHMM(windowStart) if err != nil { return true @@ -599,7 +628,10 @@ func scheduledRunAllowed(now time.Time, windowStart string, lastAgeSecs *int64, } // Outside the window: only the safety valve may run it. if lastAgeSecs == nil { - return true // no recorded backup yet — never withhold the first one + // R-88 Part 2: a nil age is no longer self-licensing. It fires the valve ONLY on a positive + // "never backed up" (or a legacy agent that cannot say). An UNKNOWN age — an unreadable + // storage — now defers, which is the whole point of this arc. + return valveOK } return time.Duration(*lastAgeSecs)*time.Second > cadence+24*time.Hour } diff --git a/controller/internal/quiesce/quiesce_window_test.go b/controller/internal/quiesce/quiesce_window_test.go index c319007..9eac489 100644 --- a/controller/internal/quiesce/quiesce_window_test.go +++ b/controller/internal/quiesce/quiesce_window_test.go @@ -40,7 +40,7 @@ func TestScheduledRunAllowed(t *testing.T) { {"unparseable window fails open", atBudapest(12, 0), "nonsense", h(20), true}, } for _, c := range cases { - if got := scheduledRunAllowed(c.now, c.window, c.age, cadence24); got != c.want { + if got := scheduledRunAllowed(c.now, c.window, c.age, true, cadence24); got != c.want { t.Errorf("%s: scheduledRunAllowed(%s, %q, age, cadence) = %v, want %v", c.name, c.now.Format("15:04"), c.window, got, c.want) } diff --git a/controller/internal/quiesce/tiers.go b/controller/internal/quiesce/tiers.go index 5b2295f..ff00d15 100644 --- a/controller/internal/quiesce/tiers.go +++ b/controller/internal/quiesce/tiers.go @@ -46,15 +46,54 @@ type TieredBackend interface { Backend // Tiers lists the agent's backup tiers, primary first. ErrTiersUnsupported ⇒ pre-R-82 agent. Tiers(ctx context.Context) ([]BackupTier, error) - DueFor(ctx context.Context, target string) (due bool, ageSecs *int64, err error) + // DueFor returns due-ness, the age (nil when unavailable) and the R-88 Part 2 age STATE as the + // agent sent it ("" = legacy agent, which is NOT the same as "unknown"). + DueFor(ctx context.Context, target string) (due bool, ageSecs *int64, ageState string, err error) StartBackupFor(ctx context.Context, target string) (jobID string, err error) BackupStatusFor(ctx context.Context, target string) (phase string, err error) } // dueTier is a tier this cycle must back up. +// AgeState (R-88 Part 2) is why a tier's age is nil. It mirrors the agent's `age_state` wire field. +// +// THE ZERO VALUE IS LEGACY, NOT UNKNOWN, and that is the whole point of the type. An agent older than +// v0.105.0 omits the field entirely; reading that silence as "unknown" would stop the controller +// firing its first-backup safety valve on un-upgraded boxes, so a genuinely new box would never back +// up outside its window and nobody would notice for weeks. Preserving the KNOWN behaviour is correct; +// the MinAgent floor is what drives the upgrade. +type AgeState string + +const ( + // AgeStateLegacy — the agent did not send the field. Behave exactly as before R-88 Part 2. + AgeStateLegacy AgeState = "" + // AgeStateKnown — the age is real. + AgeStateKnown AgeState = "known" + // AgeStateAbsent — a POSITIVE determination of "never backed up". The ONLY state (besides legacy) + // that may fire the window-gate safety valve. + AgeStateAbsent AgeState = "absent" + // AgeStateUnknown — the agent could not tell. Still due, but it must NOT bypass the window gate. + AgeStateUnknown AgeState = "unknown" +) + +// ageStateFromWire maps the agent's string to an AgeState, mapping anything unrecognised to LEGACY. +// +// An unknown FUTURE value is treated as legacy on purpose: a newer agent inventing a fourth state +// must not accidentally acquire "unknown" semantics from a controller that has never heard of it. +// Fail toward the behaviour we already understand. +func ageStateFromWire(s string) AgeState { + switch AgeState(s) { + case AgeStateKnown, AgeStateAbsent, AgeStateUnknown: + return AgeState(s) + default: + return AgeStateLegacy + } +} + type dueTier struct { target string // "" = the untargeted single-tier path (pre-R-82 agent) ageSecs *int64 + // state (R-88 Part 2) disambiguates a nil ageSecs. Empty = legacy agent. + state AgeState } // resolveDueTiers answers "what must this cycle back up?" — the dedup rule above, in one place. @@ -88,14 +127,20 @@ func (l *Loop) resolveDueTiers(ctx context.Context) (due []dueTier, degraded boo return l.resolveUntargeted(ctx) } for _, t := range tiers { - isDue, age, derr := tb.DueFor(ctx, t.Target) + isDue, age, wireState, derr := tb.DueFor(ctx, t.Target) if derr != nil { // One tier's due-check failing must not silently drop the OTHER tier's backup. l.logger.Printf("[ERROR] [quiesce] due-check failed for tier %q: %v (other tiers still evaluated)", t.Target, derr) continue } if isDue { - due = append(due, dueTier{target: t.Target, ageSecs: age}) + st := ageStateFromWire(wireState) + if st == AgeStateLegacy { + // A pre-v0.105.0 agent. Say so ONCE — the same shape as the pre-R-82 tier degrade, + // because a silent behaviour difference between boxes is how a fleet drifts unnoticed. + l.logAgeStateDegradeOnce() + } + due = append(due, dueTier{target: t.Target, ageSecs: age, state: st}) } } return due, false, nil @@ -113,6 +158,22 @@ func (l *Loop) resolveUntargeted(ctx context.Context) ([]dueTier, bool, error) { return []dueTier{{target: "", ageSecs: age}}, true, nil } +// logAgeStateDegradeOnce reports a pre-v0.105.0 agent exactly once per process (R-88 Part 2), the +// same shape and for the same reason as logTierDegradeOnce: a rollout is a steady state, so logging +// every poll would bury it, but logging zero times makes a real behaviour difference between boxes +// invisible. +// +// The behaviour on such an agent is DELIBERATELY today's: a nil age still fires the window-gate +// safety valve. Treating the missing field as "unknown" would look safer and would regress the +// first-backup guarantee on every un-upgraded box. +func (l *Loop) logAgeStateDegradeOnce() { + l.ageStateDegradeOnce.Do(func() { + l.logger.Printf("[WARN] [quiesce] agent does not report backup age_state (pre-v0.105.0) — " + + "an unreadable storage cannot be told apart from 'never backed up', so a nil age still " + + "bypasses the backup window as before. Upgrade the agent to close R-88.") + }) +} + // logTierDegradeOnce reports the pre-R-82 fallback exactly once per process. Once, because it is a // steady state during a rollout and would otherwise log every poll; but never zero times, because a // silent degrade is indistinguishable from multi-tier working. diff --git a/controller/internal/quiesce/tiers_test.go b/controller/internal/quiesce/tiers_test.go index 6f67cea..5bad53e 100644 --- a/controller/internal/quiesce/tiers_test.go +++ b/controller/internal/quiesce/tiers_test.go @@ -28,15 +28,15 @@ import ( // tierBackend is a multi-tier fake agent. phases[target] is the phase sequence returned by // successive BackupStatusFor calls for that tier. type tierBackend struct { - mu sync.Mutex - tiers []BackupTier - tiersErr error - dueSet map[string]bool - phases map[string][]string - phaseIdx map[string]int - started []string // targets StartBackupFor/StartBackup was called with, in order + mu sync.Mutex + tiers []BackupTier + tiersErr error + dueSet map[string]bool + phases map[string][]string + phaseIdx map[string]int + started []string // targets StartBackupFor/StartBackup was called with, in order untargetedDue bool - startErrOn string + startErrOn string // stacks (optional) lets a start sample how many restarts have happened SO FAR — the direct // way to assert "the app had not resumed when this tier started". stacks *fakeStacks @@ -55,10 +55,12 @@ func (b *tierBackend) Tiers(context.Context) ([]BackupTier, error) { } return b.tiers, nil } -func (b *tierBackend) DueFor(_ context.Context, target string) (bool, *int64, error) { +// DueFor returns a nil age with an EMPTY age_state — i.e. the pre-v0.105.0 (legacy) shape, which +// keeps every suite written before R-88 Part 2 asserting exactly the behaviour it always did. +func (b *tierBackend) DueFor(_ context.Context, target string) (bool, *int64, string, error) { b.mu.Lock() defer b.mu.Unlock() - return b.dueSet[target], nil, nil + return b.dueSet[target], nil, "", nil } func (b *tierBackend) StartBackupFor(_ context.Context, target string) (string, error) { b.mu.Lock() @@ -360,9 +362,9 @@ type dueErrBackend struct { errOn string } -func (d *dueErrBackend) DueFor(ctx context.Context, target string) (bool, *int64, error) { +func (d *dueErrBackend) DueFor(ctx context.Context, target string) (bool, *int64, string, error) { if target == d.errOn { - return false, nil, fmt.Errorf("simulated due-check failure") + return false, nil, "", fmt.Errorf("simulated due-check failure") } return d.tierBackend.DueFor(ctx, target) }