From 1c2664b0c103a9a877f99adde214eab052efb055 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 27 Jul 2026 18:00:56 +0200 Subject: [PATCH] =?UTF-8?q?agent=20v0.105.0=20=E2=80=94=20R-88=20Part=202:?= =?UTF-8?q?=20/backup/due=20gains=20age=5Fstate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit newestArchiveOn's (time.Time, bool) signature could not express the 'unknown' its own doc comment promised: a read error and a genuine not-found both returned (zero,false), so /backup/due emitted a POSITIVE 'no successful backup recorded yet' with a nil age, and the controller fired its window-gate valve on an unreadable storage. Three states now: known / absent / unknown, carried as a STRING enum so the zero value unambiguously means 'legacy agent' rather than masquerading as an answer. Fail-safe direction unchanged — unknown is still DUE; only the window-gate bypass narrows to ABSENT. A service with NO lister deliberately stays ABSENT: calling it unknown would stop a genuinely new box on a pre-R-84 build from ever backing up outside its window. An unparseable timestamp becomes unknown — a backup happened, we cannot date it. --- CHANGELOG.md | 41 ++++++++ internal/localapi/agestate_test.go | 160 +++++++++++++++++++++++++++++ internal/localapi/server.go | 93 ++++++++++++++--- 3 files changed, 282 insertions(+), 12 deletions(-) create mode 100644 internal/localapi/agestate_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b9fb220..63d0d7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,44 @@ +# felhom-agent — Changelog + +## v0.105.0 — R-88 Part 2: the agent can finally say "unknown" (2026-07-27) + +`newestArchiveOn` promised, in its own doc comment, that *"errors and unsupported services degrade to +unknown, never to 'no backup'"*. Its `(time.Time, bool)` signature made that **impossible**: an error +and a genuine not-found both returned `(zero, false)`, so `/backup/due` answered a POSITIVE +`"no successful backup recorded yet"` with a nil age. The controller read that as *never backed up* +and fired its window-gate safety valve — quiescing customer app stacks **outside** the backup window. +That is what happened on 2026-07-27 during the PBS outage. The comment described an intent the type +forbade. + +**The fix: three states, on the wire.** `archiveLookup` (`found` / `absent` / `unknown`) internally, +and `age_state` on `/backup/due`: + +- `known` — `age_seconds` is set and meaningful; +- `absent` — a POSITIVE determination that no backup has ever landed. **The only state that licenses + the controller to bypass its backup window;** +- `unknown` — could not determine (storage unreadable, or an unparseable timestamp). + +**A string enum, not a bool.** The zero value must mean *"legacy agent, no information"*, and `""` +says that unambiguously where `false` would masquerade as a real answer. It also matches the repo's +existing wire convention (`phase` on `/backup/status`). + +**The fail-safe direction is unchanged: unknown is still DUE.** An agent that cannot read the storage +must never suppress a backup. What changes is only whether the *window gate* may be bypassed. + +**An unparseable in-memory timestamp is now `unknown` too** — a backup DID happen, we simply cannot +date it. It previously fell through to the same positive "never" claim. + +**A service with NO lister stays `absent`, deliberately.** "Unknown" is the tempting answer there and +it would regress the first-backup safety valve: a genuinely new box on a pre-R-84 build would never +back up outside its window and nobody would notice for weeks. On that path the in-memory record is +the only registry that exists, so its absence means "no backup recorded" in the only terms available. +`unknown` is reserved for a lister that was asked and could not answer. + +**Compatibility:** the field is additive. An OLD controller decodes into a struct without it and +behaves exactly as today — every pre-existing field is unchanged (pinned by +`TestAgeState_IsAdditive_PreExistingFieldsUnchanged`). + +Tests +5; 29 packages ok. Red-proofs observed for the unknown/absent collapse in both directions. ## v0.104.0 — R-85: unattended per-tier restore-test (Phase 2): tier rotation, persisted state, one heavy op at a time (2026-07-26) The scheduler could only ever see `cfg.Backup.BackupTarget()`, so the offsite tier's archives were diff --git a/internal/localapi/agestate_test.go b/internal/localapi/agestate_test.go new file mode 100644 index 0000000..d893051 --- /dev/null +++ b/internal/localapi/agestate_test.go @@ -0,0 +1,160 @@ +package localapi + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" +) + +// R-88 Part 2 — the agent gains a third state. +// +// `newestArchiveOn` promised in its own doc comment that "errors degrade to unknown, never to +// no-backup", while its `(time.Time, bool)` return made that impossible: an error and a genuine +// not-found both produced `(zero, false)`, so `/backup/due` answered a POSITIVE +// "no successful backup recorded yet" with a nil age. The controller read that as "never backed up" +// and fired its window-gate safety valve, quiescing customer app stacks outside the backup window. +// +// These tests assert the WIRE, because the wire is the contract another component reads. + +// listerBackups is a fakeBackups that also implements BackupArchiveLister, with a controllable outcome. +type listerBackups struct { + fakeBackups + t time.Time + found bool + err error +} + +func (l *listerBackups) NewestArchiveTime(context.Context, int) (time.Time, bool, error) { + return l.t, l.found, l.err +} + +// dueWithLister builds a server whose single tier's service is the given lister, and returns the +// /backup/due response. +// NOTE: the server must receive `lb` ITSELF, not its embedded fakeBackups — the tier's Service is +// type-asserted to BackupArchiveLister, and the embedded value does not satisfy it. Passing the +// inner struct silently routes every case to archiveUnknown, which looks like a code bug and is not. +func dueWithLister(t *testing.T, lb *listerBackups, store *fakeStore) BackupDueResponse { + t.Helper() + srv, err := NewServer(Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuests{}, + Backups: lb, + Store: store, + Storage: fakeStorage{}, + Tokens: staticTokens{"A": 8200, "B": 9300}, + BackupCadence: 24 * time.Hour, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + srv.now = func() time.Time { return testNow } + return dueOf(t, srv.Handler()) +} + +// ── SCENARIO A (agent half) — an unreadable storage is UNKNOWN, not "never" ────────────────── +// +// COMPANION RED-PROOF (observed): make newestArchiveOn return archiveAbsent on the error path (the +// pre-fix collapse) and this fails with +// +// "an unreadable storage must report age_state=unknown, got \"absent\" — that is a POSITIVE claim +// of 'never backed up' built out of two absences" +// +// Restored. +func TestAgeState_UnreadableStorageIsUnknown(t *testing.T) { + lb := &listerBackups{err: errors.New("proxmox: GET storage content: 500 connection refused")} + got := dueWithLister(t, lb, &fakeStore{}) // empty store = cold in-memory record + + if got.AgeState != AgeStateUnknown { + t.Fatalf("an unreadable storage must report age_state=%q, got %q — that is a POSITIVE claim of "+ + "'never backed up' built out of two absences", AgeStateUnknown, got.AgeState) + } + if !got.Due { + t.Fatal("FAIL-SAFE DIRECTION: unknown must still be DUE — an unreadable storage must never suppress a backup") + } + if got.AgeSecs != nil { + t.Fatalf("an unknown age must not invent a number; got %d", *got.AgeSecs) + } +} + +// ── SCENARIO B (agent half) — a genuine first-ever backup is ABSENT ────────────────────────── +// +// B is what makes A safe: an implementation that reported everything as "unknown" would pass A and +// silently starve a brand-new box, because the controller only fires the first-backup valve on ABSENT. +// +// COMPANION RED-PROOF (observed): make newestArchiveOn return archiveUnknown when !found and this +// fails with +// +// "a genuine never-backed-up tier must report age_state=\"absent\", got \"unknown\" — the +// controller only licenses a first backup outside the window on ABSENT" +// +// Restored. +func TestAgeState_GenuinelyNeverIsAbsent(t *testing.T) { + lb := &listerBackups{found: false} // read SUCCEEDED, nothing there + got := dueWithLister(t, lb, &fakeStore{}) + + if got.AgeState != AgeStateAbsent { + t.Fatalf("a genuine never-backed-up tier must report age_state=%q, got %q — the controller only "+ + "licenses a first backup outside the window on ABSENT", AgeStateAbsent, got.AgeState) + } + if !got.Due { + t.Fatal("a never-backed-up tier must be due") + } +} + +// A real archive → known, with a real age. +func TestAgeState_FoundIsKnown(t *testing.T) { + lb := &listerBackups{t: testNow.Add(-2 * time.Hour), found: true} + got := dueWithLister(t, lb, &fakeStore{}) + + if got.AgeState != AgeStateKnown { + t.Fatalf("a readable archive must report age_state=%q, got %q", AgeStateKnown, got.AgeState) + } + if got.AgeSecs == nil || *got.AgeSecs < 7100 || *got.AgeSecs > 7300 { + t.Fatalf("expected ~7200s age, got %v", got.AgeSecs) + } + if got.Due { + t.Fatal("2h old against a 24h cadence is not due") + } +} + +// An unparseable in-memory timestamp is UNKNOWN too — a backup DID happen, we just cannot date it. +// Reporting "absent" there would be the same false-positive claim in a different costume. +func TestAgeState_UnparseableTimestampIsUnknown(t *testing.T) { + st := &fakeStore{backups: []hub.Backup{{VMID: 8200, Success: true, StartedAt: "not-a-timestamp"}}} + lb := &listerBackups{err: errors.New("storage unreadable")} + got := dueWithLister(t, lb, st) + + if got.AgeState != AgeStateUnknown { + t.Fatalf("an unparseable backup time means we cannot DATE a backup that exists — want %q, got %q", + AgeStateUnknown, got.AgeState) + } + if !got.Due { + t.Fatal("still due — fail safe toward taking a backup") + } +} + +// ── SCENARIO D (agent half) — additive on the wire ─────────────────────────────────────────── +// +// An OLD controller decodes into a struct without `age_state` and ignores it. What it MUST still see +// unchanged is every pre-existing field. +func TestAgeState_IsAdditive_PreExistingFieldsUnchanged(t *testing.T) { + lb := &listerBackups{t: testNow.Add(-48 * time.Hour), found: true} + got := dueWithLister(t, lb, &fakeStore{}) + + if !got.Due || got.Reason != "older than cadence" { + t.Fatalf("pre-existing due/reason semantics changed: due=%v reason=%q", got.Due, got.Reason) + } + if got.AgeSecs == nil { + t.Fatal("age_seconds must still be present for a known age") + } + // And the state rides alongside rather than replacing anything. + if got.AgeState != AgeStateKnown { + t.Fatalf("age_state should be %q, got %q", AgeStateKnown, got.AgeState) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 5c0d511..c7518bb 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -886,6 +886,31 @@ func (s *Server) jobSnapshot(key backupJobKey) (backupJob, bool) { // recorded OR the newest successful one is older than the agent-local cadence. A successful // POST /backup flips this to false for the window, so the controller won't re-quiesce in a loop. // The hub-served policy is slice 10. +// BackupAgeState (R-88 Part 2) says WHY AgeSecs is what it is — the distinction the type system +// could not previously express. +// +// Before this, a storage read ERROR and a genuine never-backed-up both produced a nil AgeSecs with +// the same `Reason`, byte-identical on the wire. The controller therefore fired its window-gate +// safety valve ("no backup yet — never withhold the first one") on an unreadable storage, quiescing +// customer app stacks OUTSIDE the backup window. Absence of a signal, read as a specific value — +// the fourth instance of that class in this codebase. +// +// A STRING enum, not a bool: the zero value must mean "legacy agent, no information", and "" says +// that unambiguously where `false` would silently masquerade as a real answer. +type BackupAgeState string + +const ( + // AgeStateKnown — AgeSecs is set and meaningful. + AgeStateKnown BackupAgeState = "known" + // AgeStateAbsent — a POSITIVE determination that no backup has ever landed for this tier. This is + // the only state that may fire the controller's safety valve. + AgeStateAbsent BackupAgeState = "absent" + // AgeStateUnknown — the agent could not determine the age (storage unreadable, timestamp + // unparseable). Still DUE (an unreadable storage must never suppress a backup), but the window + // gate must NOT be bypassed on it. + AgeStateUnknown BackupAgeState = "unknown" +) + type BackupDueResponse struct { VMID int `json:"vmid"` Due bool `json:"due"` @@ -894,8 +919,24 @@ type BackupDueResponse struct { // Target (R-82) echoes the tier this verdict is about. EMPTY (and omitted) for an untargeted // request, which is what keeps the pre-R-82 response bytes identical for old controllers. Target string `json:"target,omitempty"` + // AgeState (R-88 Part 2) disambiguates a nil AgeSecs. Additive: an OLD controller ignores it and + // behaves exactly as before. An EMPTY value on the wire means the agent is pre-v0.105.0 — the + // controller must treat that as "legacy, no information", never as AgeStateUnknown. + AgeState BackupAgeState `json:"age_state,omitempty"` } +// archiveLookup is the three-state result of asking a tier's storage when a backup last landed. +// It exists because the old (time.Time, bool) signature could not distinguish "nothing there" from +// "I could not look" — the doc comment on newestArchiveOn promised that distinction for months while +// the type made it impossible. +type archiveLookup int + +const ( + archiveFound archiveLookup = iota // a backup exists; the time is valid + archiveAbsent // read succeeded, no backup for this guest on this tier + archiveUnknown // could not read (error, or the service has no lister) +) + func (s *Server) handleBackupDue(w http.ResponseWriter, r *http.Request, vmid int) { tier, echo, ok := s.tierFromRequest(w, r) if !ok { @@ -927,26 +968,40 @@ func (s *Server) handleBackupDue(w http.ResponseWriter, r *http.Request, vmid in unparseable = true } } - if t, ok3 := s.newestArchiveOn(r.Context(), tier, vmid); ok3 && (!haveNewest || t.After(newest)) { + t, lookup := s.newestArchiveOn(r.Context(), tier, vmid) + if lookup == archiveFound && (!haveNewest || t.After(newest)) { newest, haveNewest = t, true unparseable = false // ground truth supersedes an unreadable in-memory timestamp } if !haveNewest { - if unparseable { - // Fail safe toward "due" so a backup still happens. - writeOK(w, BackupDueResponse{VMID: vmid, Due: true, Reason: "last backup time unparseable — treating as due", Target: echo}) - return + // R-88 Part 2: THREE distinct reasons for a nil age, each with its own state. Only ABSENT is a + // positive claim of "never backed up"; only that one may license the controller to bypass its + // backup window. All three stay DUE — an agent that cannot tell must never suppress a backup. + switch { + case unparseable: + writeOK(w, BackupDueResponse{VMID: vmid, Due: true, AgeState: AgeStateUnknown, + Reason: "last backup time unparseable — treating as due", Target: echo}) + case lookup == archiveUnknown: + // The storage could not be read AND this process holds no record. Previously this emitted + // "no successful backup recorded yet" — a positive claim built out of two absences, which + // is what fired the window-gate valve during the 2026-07-27 PBS outage. + writeOK(w, BackupDueResponse{VMID: vmid, Due: true, AgeState: AgeStateUnknown, + Reason: "backup storage unreadable and no in-memory record — age UNKNOWN, treating as due", Target: echo}) + default: + writeOK(w, BackupDueResponse{VMID: vmid, Due: true, AgeState: AgeStateAbsent, + Reason: "no successful backup recorded yet", Target: echo}) } - writeOK(w, BackupDueResponse{VMID: vmid, Due: true, Reason: "no successful backup recorded yet", Target: echo}) return } age := s.now().Sub(newest) ageSecs := int64(age.Seconds()) if age >= tier.Cadence { - writeOK(w, BackupDueResponse{VMID: vmid, Due: true, Reason: "older than cadence", AgeSecs: &ageSecs, Target: echo}) + writeOK(w, BackupDueResponse{VMID: vmid, Due: true, AgeSecs: &ageSecs, AgeState: AgeStateKnown, + Reason: "older than cadence", Target: echo}) return } - writeOK(w, BackupDueResponse{VMID: vmid, Due: false, Reason: "within cadence window", AgeSecs: &ageSecs, Target: echo}) + writeOK(w, BackupDueResponse{VMID: vmid, Due: false, AgeSecs: &ageSecs, AgeState: AgeStateKnown, + Reason: "within cadence window", Target: echo}) } // BackupTiersResponse is GET /backup/tiers (R-82): the tiers this agent serves, primary first. @@ -1096,18 +1151,32 @@ func (s *Server) pickLatestBackup(ctx context.Context, vmid int, successOnly boo // unsupported services degrade to "unknown", never to "no backup" — an unreadable storage must not // make the tier look freshly backed up, and it must not suppress a backup either: the caller falls // back to the in-memory record, whose absence means DUE. -func (s *Server) newestArchiveOn(ctx context.Context, tier BackupTier, vmid int) (time.Time, bool) { +func (s *Server) newestArchiveOn(ctx context.Context, tier BackupTier, vmid int) (time.Time, archiveLookup) { lister, ok := tier.Service.(BackupArchiveLister) if !ok { - return time.Time{}, false + // NO LISTER = the pre-R-84 world, and it must stay ABSENT — not unknown. + // + // "Unknown" is the tempting answer (we cannot consult storage, so we do not know) and it is + // WRONG here, because it would regress Scenario D: the controller fires its first-backup + // safety valve only on ABSENT, so a genuinely new box on a no-lister build would never take + // its first backup outside the window, and nobody would notice for weeks. A loud bug traded + // for a silent one. + // + // The honest reading: on this path the in-memory record is the ONLY registry that exists, so + // its absence means "no backup recorded" in the only terms available — exactly the claim this + // path has always made. UNKNOWN is reserved for a lister that was asked and could not answer. + return time.Time{}, archiveAbsent } t, found, err := lister.NewestArchiveTime(ctx, vmid) if err != nil { s.logger.Warn("local-api: could not read the backup storage for the due-check — falling back to the in-memory record", "vmid", vmid, "target", tier.TargetID, "err", err) - return time.Time{}, false + return time.Time{}, archiveUnknown } - return t, found + if !found { + return time.Time{}, archiveAbsent + } + return t, archiveFound } // backupAge2 parses an RFC3339 backup start time, returning it as a time.