v0.99.0 — R-82 operator rulings: 2-week offsite retention + one backup at a time

Ruling 1 (2 weeks of weekly offsite backups): localPruneSpec's blanket PBS refusal is now scoped — an ADDITIONAL tier with an explicit keep_last may prune its PBS target. The refusal still applies in full to the PRIMARY tier, because BackupTarget() defaults to felhom-pbs and KeepLast() defaults to 3, so a box with neither key set would silently prune its offsite DR to 3 restore points. An additional tier cannot have that accident (keep_last defaults to 0). Ruling 3 (first backup runs as long as needed; nothing else starts until done): - additional-tier wait bound 6h -> 12h (measured ~33 MB/min => ~5h for a first full 10 GB snapshot; 12h gives margin but stays bounded so a hung task still surfaces) - ONE BACKUP AT A TIME PER GUEST across all tiers: POST /backup returns 409 when a DIFFERENT tier is in flight, naming the busy tier, with NO data object so nothing is parseable as the caller's own job. Same tier still returns that job (202, unchanged). - snapshotted now counts as in-flight, not just running — after the snapshot the vzdump is still uploading and holding the lock. The old check left a window where a second POST started a real second vzdump. Latent bug, closed. Full suite green (29 packages); red-proof observed and restored.
2026-07-26 15:05:54 +02:00
parent a667c269c7
commit 3d955e4edd
7 changed files with 203 additions and 30 deletions
@@ -6,6 +6,7 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
+	"strings"
 	"testing"
 	"time"

@@ -290,37 +291,112 @@ func TestBackupPost_UntargetedRoutesToPrimary(t *testing.T) {
 	}
 }

-// Single-flight is PER TIER. A PBS backup starting while the local one is still running must get
-// its OWN job id — this is what lets Slice B run both inside one quiesce window. Keying jobs by
-// vmid alone would hand the second call the first job's id and the controller would believe the
-// PBS backup finished when only the local one had.
-func TestBackupPost_SingleFlightIsPerTierNotPerGuest(t *testing.T) {
+// ONE BACKUP AT A TIME PER GUEST (operator ruling 2026-07-26). A second tier's POST while another
+// tier is still in flight must be REFUSED — vzdump holds the guest lock, so it could not succeed
+// anyway, and attempting it records a spurious failure that leaves the tier permanently due.
+//
+// Crucially it must NOT be handed the busy tier's job id: that is exactly how a caller comes to
+// believe its own backup ran.
+func TestBackupPost_SecondTierRefusedWhileAnotherInFlight(t *testing.T) {
 	localGate := make(chan struct{})
 	local := &fakeBackups{gate: localGate}
 	pbs := &fakeBackups{}
 	h := tieredServer(t, &fakeStore{}, local, pbs).Handler()

-	var first, second BackupResponse
+	var first BackupResponse
 	rr1 := do(t, h, "POST", "/backup", "A", "") // local; blocks on the gate
 	if err := json.Unmarshal(rr1.Body.Bytes(), &struct {
 		Data *BackupResponse `json:"data"`
 	}{Data: &first}); err != nil {
 		t.Fatal(err)
 	}
-	rr2 := do(t, h, "POST", "/backup?target=felhom-pbs", "A", "") // different tier → new job
+	waitFor(t, func() bool { return len(local.seen()) == 1 })
+
+	rr2 := do(t, h, "POST", "/backup?target=felhom-pbs", "A", "")
+	if rr2.Code != http.StatusConflict {
+		t.Fatalf("a second tier must be REFUSED while another is in flight; got %d body %s", rr2.Code, rr2.Body.String())
+	}
+	// The STRUCTURAL requirement: the refusal must not return a job the caller could mistake for
+	// its own. It is a 409 with ok=false and NO data object, so nothing is parseable as "my job".
+	// (Naming the busy job in the human-readable message is deliberate and useful for diagnosis —
+	// what must never happen is handing it back as BackupResponse.JobID on a 202.)
+	var envelope struct {
+		OK    bool            `json:"ok"`
+		Data  *BackupResponse `json:"data"`
+		Error string          `json:"error"`
+	}
+	if err := json.Unmarshal(rr2.Body.Bytes(), &envelope); err != nil {
+		t.Fatal(err)
+	}
+	if envelope.OK {
+		t.Fatalf("a refusal must not be ok=true; body %s", rr2.Body.String())
+	}
+	if envelope.Data != nil && envelope.Data.JobID != "" {
+		t.Fatalf("the refusal must NOT hand back a job id as the caller's own (got %q); body %s",
+			envelope.Data.JobID, rr2.Body.String())
+	}
+	if !strings.Contains(rr2.Body.String(), "local") {
+		t.Fatalf("the refusal must NAME the busy tier so the caller can diagnose; body %s", rr2.Body.String())
+	}
+	if got := len(pbs.seen()); got != 0 {
+		t.Fatalf("the refused tier must NOT have started a backup (ran %d times)", got)
+	}
+	close(localGate)
+}
+
+// Once the busy tier finishes, the other tier may start — and gets its OWN tier-scoped job id.
+func TestBackupPost_SecondTierAllowedAfterFirstFinishes(t *testing.T) {
+	local, pbs := &fakeBackups{}, &fakeBackups{}
+	h := tieredServer(t, &fakeStore{}, local, pbs).Handler()
+
+	var first, second BackupResponse
+	if err := json.Unmarshal(do(t, h, "POST", "/backup", "A", "").Body.Bytes(), &struct {
+		Data *BackupResponse `json:"data"`
+	}{Data: &first}); err != nil {
+		t.Fatal(err)
+	}
+	waitFor(t, func() bool { return len(local.seen()) == 1 })
+	// Wait for the local job to leave the in-flight phases.
+	waitFor(t, func() bool {
+		rr := do(t, h, "GET", "/backup/status", "A", "")
+		return strings.Contains(rr.Body.String(), `"phase":"done"`)
+	})
+
+	rr2 := do(t, h, "POST", "/backup?target=felhom-pbs", "A", "")
+	if rr2.Code != http.StatusAccepted {
+		t.Fatalf("after the first tier finished the second must be allowed; got %d body %s", rr2.Code, rr2.Body.String())
+	}
 	if err := json.Unmarshal(rr2.Body.Bytes(), &struct {
 		Data *BackupResponse `json:"data"`
 	}{Data: &second}); err != nil {
 		t.Fatal(err)
 	}
-	if first.JobID == "" || second.JobID == "" {
-		t.Fatalf("both tiers must get a job id; got %q / %q", first.JobID, second.JobID)
-	}
-	if first.JobID == second.JobID {
-		t.Fatalf("PER-TIER single-flight violated: the PBS request was handed the LOCAL job id %q — the controller would believe the PBS backup ran", first.JobID)
+	if second.JobID == first.JobID {
+		t.Fatalf("job ids must stay tier-scoped: %q vs %q", first.JobID, second.JobID)
 	}
 	waitFor(t, func() bool { return len(pbs.seen()) == 1 })
-	close(localGate)
+}
+
+// `snapshotted` still counts as in flight — the vzdump is uploading and still holds the guest lock.
+// Checking only `running` (the pre-R-82 code) left a window where a second POST started a real
+// second vzdump.
+func TestBackupPost_SnapshottedCountsAsInFlight(t *testing.T) {
+	gate := make(chan struct{})
+	local := &fakeBackups{gate: gate, fireSnapshot: true} // fires onSnapshot, then blocks
+	pbs := &fakeBackups{}
+	h := tieredServer(t, &fakeStore{}, local, pbs).Handler()
+
+	do(t, h, "POST", "/backup", "A", "")
+	waitFor(t, func() bool {
+		rr := do(t, h, "GET", "/backup/status", "A", "")
+		return strings.Contains(rr.Body.String(), `"phase":"snapshotted"`)
+	})
+
+	rr := do(t, h, "POST", "/backup?target=felhom-pbs", "A", "")
+	if rr.Code != http.StatusConflict {
+		t.Fatalf("a SNAPSHOTTED backup still holds the guest — a second tier must be refused; got %d body %s", rr.Code, rr.Body.String())
+	}
+	close(gate)
 }

 // Same tier, still single-flight: a second POST to a running tier returns the SAME job.