diff --git a/CHANGELOG.md b/CHANGELOG.md index 26e31ca..2e4bdd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,42 @@ +## v0.103.0 — R-84: an agent restart no longer triggers a redundant backup (2026-07-26) + +**Observed live, not theorised.** Three redundant local backups ran on demo-felhom in a single +afternoon of deploys (2026-07-26). The backup `Store` is in-memory — its own doc comment says *"lost +on restart; the cadence re-populates"* — so after every restart `/backup/due` answered *"no +successful backup recorded yet"* and the controller dutifully took another one. + +On the local tier that is wasted minutes. **On the OFFSITE tier it is a wasted multi-hour WAN upload +after every agent deploy** — and agent deploys are routine. That is what moved R-84 from a tidy-up to +something close to a prerequisite for running the offsite tier at all. + +### Changed +- **`BackupRunner.NewestArchiveTime`** — when a backup last LANDED on this tier's storage, read from + the storage. +- **`localapi.BackupArchiveLister`** (optional extension to `BackupService`) — the due-check consults + it and takes whichever is newer, the in-memory record or the storage. + +### Why ask the storage instead of persisting the store + +- **It is ground truth, not remembered state.** A pruned or deleted archive correctly stops counting; + a persisted record would keep claiming a backup that no longer exists. +- **No new on-disk state, no migration**, and it is the same source `latestArchive` already trusts to + build the post-backup record. +- It answers only *"when did a backup last land"*, which is exactly what the due-check needs. The + richer fields (size, duration, uncovered volumes, error) stay with the real in-memory records — a + synthesized record would put invented numbers into the host-report, and the hub already covers + history via its own retained-report window (R-81). + +### Fail-safe directions +- Storage read **error** → fall back to the in-memory record. An unreadable storage must never make a + tier look freshly backed up, and must not suppress a backup either. +- Storage genuinely **empty** → due. The fix must not invent a backup. +- Archive present but **old** → still due. This is not a blanket suppressor. +- Service **without** the optional lister → pre-R-84 behaviour, unchanged. + +### Tests ++6, full suite green (29 packages). Red-proof observed: removing the fold-in reproduces +`Due:true Reason:no successful backup recorded yet` against a storage holding a 2 h-old backup. + ## v0.102.0 — R-82 Slice D: an unprovisioned tier DEFERS instead of failing (2026-07-26) Prerequisite for putting the offsite tier into the installer defaults (host-install 1.20.0). diff --git a/internal/backup/runner.go b/internal/backup/runner.go index 8dbf962..5b83a36 100644 --- a/internal/backup/runner.go +++ b/internal/backup/runner.go @@ -276,6 +276,42 @@ func (r *BackupRunner) latestArchive(ctx context.Context, vmid int) (string, int return vol, size, nil } +// NewestArchiveTime reports when this guest's newest backup archive LANDED ON THIS TARGET, from the +// storage itself. ok=false means the target genuinely holds no archive for this guest. +// +// R-84: this is the cure for the redundant-backup-after-restart problem. The agent's backup Store is +// in-memory ("lost on restart; the cadence re-populates"), so after every restart /backup/due +// reported "no successful backup recorded yet" and the controller dutifully took another one. On the +// local tier that is wasted minutes; on the OFFSITE tier it is a wasted multi-hour WAN upload after +// every agent deploy — and agent deploys are routine. Three redundant local backups were observed on +// demo-felhom in a single afternoon of deploys (2026-07-26). +// +// Asking the STORAGE rather than persisting the store is deliberate: +// - it is ground truth, not remembered state — if an archive was pruned or deleted it correctly +// stops counting, whereas a persisted record would keep claiming a backup that no longer exists; +// - it needs no new on-disk state and no migration; +// - it is the same source `latestArchive` already trusts to build the post-backup record. +// +// It answers ONLY "when did a backup last land", which is exactly what the due-check needs. The +// richer fields (size, duration, uncovered volumes, error) stay with the real in-memory records — a +// synthesized record would put invented numbers into the host-report. +func (r *BackupRunner) NewestArchiveTime(ctx context.Context, vmid int) (time.Time, bool, error) { + contents, err := r.api.StorageContent(ctx, r.target) + if err != nil { + return time.Time{}, false, err + } + var best int64 = -1 + for _, e := range contents { + if e.Content == "backup" && e.VMID == vmid && e.CTime > best { + best = e.CTime + } + } + if best < 0 { + return time.Time{}, false, nil + } + return time.Unix(best, 0).UTC(), true, nil +} + // parseBackupMode extracts the actual mode from a vzdump task log line `… backup mode: ` // (e.g. "INFO: backup mode: stop"). Returns "" if not found. func parseBackupMode(lines []string) string { diff --git a/internal/localapi/backup_tiers_test.go b/internal/localapi/backup_tiers_test.go index 3815acc..f375079 100644 --- a/internal/localapi/backup_tiers_test.go +++ b/internal/localapi/backup_tiers_test.go @@ -3,6 +3,7 @@ package localapi import ( "context" "encoding/json" + "errors" "io" "log/slog" "net/http" @@ -546,3 +547,124 @@ func TestBackupDue_StorageViewError_DoesNotSuppress(t *testing.T) { t.Fatalf("a storage-view error must not suppress the backup (fail toward due); got %+v", pbs.Data) } } + +// ── R-84: the cold in-memory store must not cause a redundant backup ───────────────────────── + +// archiveLister is a fakeBackups that ALSO knows when a backup last landed on its storage. +type archiveLister struct { + *fakeBackups + at time.Time + found bool + err error +} + +func (a archiveLister) NewestArchiveTime(context.Context, int) (time.Time, bool, error) { + return a.at, a.found, a.err +} + +func listerServer(t *testing.T, st *fakeStore, pbsSvc BackupService) http.Handler { + t.Helper() + srv, err := NewServer(Options{ + ListenAddr: "127.0.0.1:0", Guests: &fakeGuests{}, Backups: &fakeBackups{}, Store: st, + Storage: fakeStorage{targets: []hub.StorageTarget{{Name: "local"}, {Name: "felhom-pbs"}}}, + Tokens: staticTokens{"A": 8200}, + BackupTiers: []BackupTier{ + {TargetID: "local", Cadence: 24 * time.Hour, Primary: true, Service: &fakeBackups{}}, + {TargetID: "felhom-pbs", Cadence: 7 * 24 * time.Hour, Service: pbsSvc}, + }, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatal(err) + } + srv.baseCtx = context.Background() + srv.now = func() time.Time { return testNow } + return srv.Handler() +} + +func dueFor(t *testing.T, h http.Handler, target string) BackupDueResponse { + t.Helper() + var out struct { + Data BackupDueResponse `json:"data"` + } + if err := json.Unmarshal(do(t, h, "GET", "/backup/due?target="+target, "A", "").Body.Bytes(), &out); err != nil { + t.Fatal(err) + } + return out.Data +} + +// THE R-84 CASE. The in-memory store is EMPTY (the agent just restarted), but the storage holds a +// snapshot from 2 hours ago. The tier must NOT be due — otherwise every agent deploy costs a fresh +// multi-hour offsite upload. Three redundant local backups were observed on demo-felhom in one +// afternoon of deploys before this. +// +// COMPANION RED-PROOF (observed): delete the newestArchiveOn fold-in from handleBackupDue (the +// pre-R-84 shape, in-memory only) → this fails with +// "a restart must NOT make the tier due when the storage holds a 2h-old backup; +// got {... Due:true Reason:no successful backup recorded yet ...}". Restored. +func TestBackupDue_ColdStore_UsesStorageGroundTruth(t *testing.T) { + h := listerServer(t, &fakeStore{}, archiveLister{ + fakeBackups: &fakeBackups{}, at: testNow.Add(-2 * time.Hour), found: true, + }) + got := dueFor(t, h, "felhom-pbs") + if got.Due { + t.Fatalf("a restart must NOT make the tier due when the storage holds a 2h-old backup; got %+v", got) + } + if got.AgeSecs == nil || *got.AgeSecs != int64((2 * time.Hour).Seconds()) { + t.Fatalf("the age must come from the storage; got %+v", got) + } +} + +// Ground truth that is genuinely OLD still makes the tier due — this must not become a blanket +// suppressor. +func TestBackupDue_ColdStore_OldArchiveIsStillDue(t *testing.T) { + h := listerServer(t, &fakeStore{}, archiveLister{ + fakeBackups: &fakeBackups{}, at: testNow.Add(-9 * 24 * time.Hour), found: true, + }) + if got := dueFor(t, h, "felhom-pbs"); !got.Due { + t.Fatalf("a 9-day-old archive under a 7-day cadence MUST still be due; got %+v", got) + } +} + +// A storage that genuinely holds nothing → due. The fix must not invent a backup. +func TestBackupDue_ColdStore_NoArchiveIsDue(t *testing.T) { + h := listerServer(t, &fakeStore{}, archiveLister{fakeBackups: &fakeBackups{}, found: false}) + if got := dueFor(t, h, "felhom-pbs"); !got.Due { + t.Fatalf("no archive anywhere → due; got %+v", got) + } +} + +// A storage-read ERROR must fall back to the in-memory record, NOT be read as "a backup exists". +// An unreadable storage must never make a tier look freshly backed up. +func TestBackupDue_StorageReadError_DoesNotFakeFreshness(t *testing.T) { + h := listerServer(t, &fakeStore{}, archiveLister{ + fakeBackups: &fakeBackups{}, at: testNow, found: true, err: errStorageRead, + }) + if got := dueFor(t, h, "felhom-pbs"); !got.Due { + t.Fatalf("a storage-read error must not fake freshness — the in-memory record is empty, so DUE; got %+v", got) + } +} + +var errStorageRead = errors.New("simulated storage read failure") + +// The in-memory record WINS when it is newer than the storage listing — a backup that just finished +// this process lifetime is more current than a listing that may lag. +func TestBackupDue_InMemoryRecordWinsWhenNewer(t *testing.T) { + st := &fakeStore{} + st.RecordBackup(backupAt("felhom-pbs", 8200, time.Hour, true)) // 1h ago, in memory + h := listerServer(t, st, archiveLister{ + fakeBackups: &fakeBackups{}, at: testNow.Add(-9 * 24 * time.Hour), found: true, // stale listing + }) + got := dueFor(t, h, "felhom-pbs") + if got.Due { + t.Fatalf("the fresher in-memory record must win over a stale listing; got %+v", got) + } +} + +// A service WITHOUT the optional lister degrades to the pre-R-84 behaviour, unchanged. +func TestBackupDue_ServiceWithoutLister_UnchangedBehaviour(t *testing.T) { + h := listerServer(t, &fakeStore{}, &fakeBackups{}) // plain BackupService + if got := dueFor(t, h, "felhom-pbs"); !got.Due || got.Reason != "no successful backup recorded yet" { + t.Fatalf("a plain BackupService must behave exactly as before; got %+v", got) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 7a20673..360961d 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -41,6 +41,18 @@ type BackupService interface { BackupWithSnapshotHook(ctx context.Context, vmid int, onSnapshot func()) (hub.Backup, error) } +// BackupArchiveLister is an OPTIONAL extension to BackupService: "when did a backup last LAND on +// this tier's storage?", answered by the storage rather than by memory. *backup.BackupRunner +// satisfies it. +// +// R-84: the agent's backup Store is in-memory, so after every restart the due-check saw nothing and +// the controller took a redundant backup — a wasted multi-hour WAN upload on the offsite tier after +// every agent deploy. Consulting the storage makes the cold path truthful without persisting +// anything, and it self-corrects: a pruned archive correctly stops counting. +type BackupArchiveLister interface { + NewestArchiveTime(ctx context.Context, vmid int) (time.Time, bool, error) +} + // BackupTier (R-82) binds ONE backup tier's runner to its own policy. The agent builds one per // resolved config tier; the local API serves each independently so "local daily + PBS weekly" is // expressible over the wire, not just in config. @@ -882,17 +894,32 @@ func (s *Server) handleBackupDue(w http.ResponseWriter, r *http.Request, vmid in Reason: "target storage not present yet — tier deferred until it is provisioned", Target: echo}) return } - latest := s.latestSuccessfulBackupForTarget(r.Context(), vmid, tier.TargetID) - if latest == nil { + // Newest backup for THIS tier: the in-memory record if this process took one, otherwise the + // storage itself (R-84 — see BackupArchiveLister). Whichever is newer wins. + var newest time.Time + var haveNewest bool + var unparseable bool + if latest := s.latestSuccessfulBackupForTarget(r.Context(), vmid, tier.TargetID); latest != nil { + if t, ok2 := backupAge2(latest.StartedAt); ok2 { + newest, haveNewest = t, true + } else { + unparseable = true + } + } + if t, ok3 := s.newestArchiveOn(r.Context(), tier, vmid); ok3 && (!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 + } writeOK(w, BackupDueResponse{VMID: vmid, Due: true, Reason: "no successful backup recorded yet", Target: echo}) return } - age, ok2 := backupAge(latest.StartedAt, s.now()) - if !ok2 { - // Unparseable timestamp: 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 - } + 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}) @@ -1044,6 +1071,33 @@ func (s *Server) pickLatestBackup(ctx context.Context, vmid int, successOnly boo return latest } +// newestArchiveOn asks THIS TIER's storage when a backup last landed (R-84). Errors and +// 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) { + lister, ok := tier.Service.(BackupArchiveLister) + if !ok { + return time.Time{}, false + } + 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 t, found +} + +// backupAge2 parses an RFC3339 backup start time, returning it as a time. +func backupAge2(startedAt string) (time.Time, bool) { + t, err := time.Parse(time.RFC3339, startedAt) + if err != nil { + return time.Time{}, false + } + return t.UTC(), true +} + // backupAge parses an RFC3339 backup start time and returns its age relative to now. func backupAge(startedAt string, now time.Time) (time.Duration, bool) { t, err := time.Parse(time.RFC3339, startedAt)