v0.103.0 — R-84: an agent restart no longer triggers a redundant backup

Observed live: three redundant local backups on demo-felhom in one afternoon of deploys. The backup Store is in-memory ('lost on restart; the cadence re-populates'), so after every restart /backup/due said 'no successful backup recorded yet' and the controller took another one. On the offsite tier that is a wasted multi-hour WAN upload after every agent deploy. - BackupRunner.NewestArchiveTime: when a backup last LANDED on this tier's storage, read from the storage. - localapi.BackupArchiveLister (optional BackupService extension): the due-check takes whichever is newer, the in-memory record or the storage. Asking the storage rather than persisting the store is deliberate: it is ground truth (a pruned archive correctly stops counting, where a persisted record would keep claiming a backup that no longer exists), needs no new on-disk state, and answers only 'when did a backup last land' — the richer fields stay with real records so the host-report never carries invented numbers. Fail-safes: read error -> fall back to memory (never fake freshness, never suppress); genuinely empty -> due; old archive -> still due; service without the lister -> unchanged. Red-proof observed; full suite green (29 packages).
2026-07-26 18:20:59 +02:00
parent e4f22f4c4f
commit 5acf1033a2
4 changed files with 259 additions and 8 deletions
@@ -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: <x>`
 // (e.g. "INFO: backup mode: stop"). Returns "" if not found.
 func parseBackupMode(lines []string) string {
@@ -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)
+	}
+}
@@ -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)