diff --git a/CHANGELOG.md b/CHANGELOG.md index 615c076..d2ffe7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,38 @@ +## v0.99.0 — R-82: operator rulings — 2-week offsite retention + one backup at a time (2026-07-26) + +Implements two operator rulings of 2026-07-26. Both are behaviour changes on the multi-tier path +only; a single-tier config is untouched. + +### Ruling 1 — keep two weeks of weekly offsite backups +`localPruneSpec` refused to prune ANY PBS target. That blanket refusal is now scoped: an +**ADDITIONAL tier with an explicitly configured `keep_last`** may prune its PBS target +(`NewBackupRunnerFull(..., allowPBSPrune)`). + +**The refusal still applies in full to the PRIMARY tier, and that is not caution for its own sake:** +`BackupTarget()` defaults to `"felhom-pbs"` and `KeepLast()` defaults to 3, so a box with neither key +set would silently prune its offsite DR down to 3 restore points. An additional tier cannot have that +accident — its `keep_last` defaults to 0 (never prune), so any value there is a deliberate act. + +### Ruling 3 — let the first backup run as long as it needs; nothing else starts until it is done +- **Wait bound for an additional tier raised 6h → 12h.** Measured on demo-felhom: ~33 MB/min over + the wg link, so a first FULL ~10 GB snapshot projects to ~5h. 12h gives real margin on a slower + link while staying BOUNDED — a genuinely hung task must still surface eventually. +- **ONE BACKUP AT A TIME PER GUEST, ACROSS ALL TIERS.** `POST /backup` now refuses with **409** when + a DIFFERENT tier has a backup in flight, naming the busy tier and job. vzdump holds a guest lock, + so a concurrent second backup could not succeed anyway — but without this guard it would be + ATTEMPTED, fail on the lock, and record a spurious failure that leaves the tier permanently due. + - SAME tier in flight → still returns THAT job (202, idempotent) — unchanged. + - DIFFERENT tier in flight → 409 with **no data object**, so nothing is parseable as the caller's + own job. Handing back a foreign job id is precisely how a caller comes to believe its backup ran. + - **`snapshotted` now counts as in flight**, not just `running`. After the storage snapshot the + vzdump is still uploading and still holds the lock; the pre-R-82 check looked at `running` only, + leaving a window where a second POST started a real second vzdump. Latent bug, closed here. + +### Tests +Full suite green (29 packages). `TestBackupPost_SecondTierRefusedWhileAnotherInFlight`, +`…_SecondTierAllowedAfterFirstFinishes`, `…_SnapshottedCountsAsInFlight`, and the per-tier wait-bound +test updated to 12h. Red-proof for the wait bound observed and restored. + ## v0.98.0 — R-82 Slice A fix: per-tier vzdump wait bound (the 30-minute false failure) (2026-07-26) **Found by live validation on demo-felhom, not by review.** The first real PBS-targeted backup ran diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index c584b87..d775082 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1286,7 +1286,10 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St if t.KeepLast > 0 { prune = fmt.Sprintf("keep-last=%d", t.KeepLast) } - r := backup.NewBackupRunnerWithWait(px, t.TargetID, "", "felhom local-api", prune, t.WaitTimeout, logger) + // Pruning a PBS target is allowed ONLY for an additional tier with an explicit keep_last + // (the primary's target AND retention both default, so it could prune the DR by accident). + allowPBSPrune := !t.Primary && t.KeepLast > 0 + r := backup.NewBackupRunnerFull(px, t.TargetID, "", "felhom local-api", prune, t.WaitTimeout, allowPBSPrune, logger) if t.Primary { runner = r } @@ -1298,7 +1301,8 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St Service: r, }) logger.Info("backup tier armed", "target", t.TargetID, "cadence", t.Cadence.String(), - "keep_last", t.KeepLast, "wait_timeout", t.WaitTimeout.String(), "primary", t.Primary) + "keep_last", t.KeepLast, "wait_timeout", t.WaitTimeout.String(), + "prune_pbs_allowed", allowPBSPrune, "primary", t.Primary) } // Guest data-drive passthrough (slice 10 P2): a root-CLI runner for the `pct set` bind + chown // (same fenced ExecRunner the host-storage + provision back-half use). diff --git a/internal/backup/runner.go b/internal/backup/runner.go index 4d5c4a9..8dbf962 100644 --- a/internal/backup/runner.go +++ b/internal/backup/runner.go @@ -42,7 +42,17 @@ type BackupRunner struct { // right for a local vzdump and badly wrong for an offsite PBS upload (see the 2026-07-26 live // failure recorded on config.BackupTargetConfig.WaitTimeoutSeconds). 0 → 30m (legacy). waitTimeout time.Duration - logger *slog.Logger + // allowPBSPrune permits `--prune-backups` on a PBS-type target. OFF by default and ON only for + // an ADDITIONAL tier whose keep_last was set explicitly (operator ruling 2026-07-26: keep two + // weeks of weekly offsite backups). + // + // The blanket PBS refusal it replaces existed for a real reason and still applies to the + // PRIMARY tier: 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 — its keep_last defaults to 0 (never prune), so any value + // there is a deliberate act. + allowPBSPrune bool + logger *slog.Logger now func() time.Time } @@ -55,6 +65,12 @@ func NewBackupRunner(api BackupAPI, target string, mode proxmox.BackupMode, note // NewBackupRunnerWithWait is NewBackupRunner plus an explicit vzdump wait bound (0 → 30m). func NewBackupRunnerWithWait(api BackupAPI, target string, mode proxmox.BackupMode, notes, retention string, waitTimeout time.Duration, logger *slog.Logger) *BackupRunner { + return NewBackupRunnerFull(api, target, mode, notes, retention, waitTimeout, false, logger) +} + +// NewBackupRunnerFull is the full constructor. allowPBSPrune must be true ONLY for an additional +// tier with an explicitly configured keep_last — see BackupRunner.allowPBSPrune. +func NewBackupRunnerFull(api BackupAPI, target string, mode proxmox.BackupMode, notes, retention string, waitTimeout time.Duration, allowPBSPrune bool, logger *slog.Logger) *BackupRunner { if mode == "" { mode = proxmox.ModeSnapshot } @@ -65,7 +81,8 @@ func NewBackupRunnerWithWait(api BackupAPI, target string, mode proxmox.BackupMo waitTimeout = 30 * time.Minute } return &BackupRunner{api: api, target: target, mode: mode, notes: notes, retention: retention, - waitTimeout: waitTimeout, logger: logger, now: func() time.Time { return time.Now().UTC() }} + waitTimeout: waitTimeout, allowPBSPrune: allowPBSPrune, logger: logger, + now: func() time.Time { return time.Now().UTC() }} } // localPruneSpec returns the `--prune-backups` spec to apply to THIS backup, or "" to skip pruning. It @@ -84,8 +101,11 @@ func (r *BackupRunner) localPruneSpec(ctx context.Context) string { } for _, s := range stores { if s.Storage == r.target { - if s.Type == "pbs" { - return "" // PBS retention is out of scope — never prune the offsite DR + if s.Type == "pbs" && !r.allowPBSPrune { + // Not opted in → never prune the offsite DR (the pre-R-82 rule, and still the rule + // for the primary tier, whose target+retention both DEFAULT and could prune by + // accident). + return "" } return r.retention } diff --git a/internal/config/backup_tiers_test.go b/internal/config/backup_tiers_test.go index 163a197..54c419d 100644 --- a/internal/config/backup_tiers_test.go +++ b/internal/config/backup_tiers_test.go @@ -166,8 +166,8 @@ func TestBackupTiers_WaitTimeoutIsPerTier(t *testing.T) { if tiers[0].WaitTimeout != 30*time.Minute { t.Fatalf("the PRIMARY must keep the historical 30m wait (unchanged behaviour); got %s", tiers[0].WaitTimeout) } - if tiers[1].WaitTimeout != 6*time.Hour { - t.Fatalf("an offsite tier must default to a GENEROUS wait — a false timeout is worse than a slow pass; got %s", tiers[1].WaitTimeout) + if tiers[1].WaitTimeout != 12*time.Hour { + t.Fatalf("an offsite tier must default to a GENEROUS wait (operator ruling: let the first backup run as long as needed) — a false timeout is worse than a slow pass; got %s", tiers[1].WaitTimeout) } // And it must be overridable per tier. b.ExtraTargets[0].WaitTimeoutSeconds = 3600 diff --git a/internal/config/config.go b/internal/config/config.go index 12ee139..e05e21b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -413,12 +413,13 @@ type BackupTargetConfig struct { // and one hanging 30 minutes is a genuine fault worth surfacing. Unchanged behaviour. // // An ADDITIONAL tier is by construction the offsite/WAN one in this design, where the binding -// constraint is uplink speed, not health. 6h covers a first FULL snapshot of a ~10 GB guest at the -// ~30 Mbit/s measured on demo-felhom (that first backup alone projects to ~5.5h); later -// incrementals are far quicker. Sized from the measurement, not guessed. +// constraint is uplink speed, not health. Measured on demo-felhom: ~33 MB/min over the wg link to +// Hetzner, so a first FULL ~10 GB snapshot projects to ~5h. Operator ruling 2026-07-26: "let the +// first backup run as long as needed" — 12h gives that real margin on a slower link while still +// being BOUNDED, so a genuinely hung task eventually surfaces instead of hanging forever. const ( defaultPrimaryTierWaitTimeout = 30 * time.Minute - defaultExtraTierWaitTimeout = 6 * time.Hour + defaultExtraTierWaitTimeout = 12 * time.Hour ) // BackupTier is a RESOLVED backup tier: one target, its own cadence, its own retention. The agent diff --git a/internal/localapi/backup_tiers_test.go b/internal/localapi/backup_tiers_test.go index bdecc5f..e6e8811 100644 --- a/internal/localapi/backup_tiers_test.go +++ b/internal/localapi/backup_tiers_test.go @@ -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. diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 7b5d753..b043f00 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -705,17 +705,34 @@ func (s *Server) handleBackup(w http.ResponseWriter, r *http.Request, vmid int) } key := backupJobKey{vmid: vmid, target: tier.TargetID} - // Single-flight per guest PER TIER: if a backup is already running for this guest ON THIS - // TIER, return that job (don't start a second concurrent vzdump to the same target). A - // DIFFERENT tier is a different job — that is what lets the weekly night run both backups - // inside one quiesce window without the second call being handed the first one's id. + // ONE BACKUP AT A TIME PER GUEST, ACROSS ALL TIERS (operator ruling 2026-07-26: "other backup + // shouldn't start until finished"). vzdump takes a guest lock, so a concurrent second backup + // could not succeed anyway — but without this guard it would be ATTEMPTED, fail on the lock, and + // record a spurious failure that leaves the tier permanently due. + // + // Two distinct cases, deliberately answered differently: + // - SAME tier already in flight → return THAT job (202). Idempotent: the caller re-polls it. + // - DIFFERENT tier in flight → 409. Not a new job, and NOT the other tier's job either — + // handing back a foreign job id is how a caller comes to believe its own backup ran. + // + // "In flight" includes `snapshotted`, not just `running`: after the storage snapshot the vzdump + // is still uploading and still holding the lock. Checking only `running` (the pre-R-82 code) + // left a window where a second POST would start a real second vzdump. s.jobsMu.Lock() - if cur := s.jobs[key]; cur != nil && cur.Phase == PhaseRunning { + if cur := s.jobs[key]; cur != nil && backupInFlight(cur.Phase) { job := *cur s.jobsMu.Unlock() writeStatus(w, http.StatusAccepted, true, BackupResponse{VMID: vmid, JobID: job.JobID, Phase: job.Phase, Target: echo}, "") return } + if busyTarget, busyJob, busy := s.otherTierInFlight(vmid, tier.TargetID); busy { + s.jobsMu.Unlock() + s.logger.Info("local-api: backup refused — another tier is still in flight", + "vmid", vmid, "requested_target", tier.TargetID, "busy_target", busyTarget, "busy_job", busyJob) + writeStatus(w, http.StatusConflict, false, nil, + "a backup is already in flight on target "+busyTarget+" (job "+busyJob+") — only one backup runs at a time per guest") + return + } // Job ids must be unique PER TIER, and by construction rather than by clock luck: two tiers // started inside the same nanosecond (the weekly both-due night, or any injected clock) would // otherwise collide and hand the second caller the first tier's id. The PRIMARY keeps the @@ -770,6 +787,26 @@ func (s *Server) handleBackup(w http.ResponseWriter, r *http.Request, vmid int) writeStatus(w, http.StatusAccepted, true, BackupResponse{VMID: vmid, JobID: jobID, Phase: PhaseRunning}, "") } +// backupInFlight reports whether a phase means "this backup still holds the guest". +// `snapshotted` counts: the storage snapshot is taken but the vzdump is still uploading. +func backupInFlight(phase string) bool { + return phase == PhaseRunning || phase == PhaseSnapshotted +} + +// otherTierInFlight reports whether a DIFFERENT tier has an in-flight backup for this guest. +// Caller must hold s.jobsMu. +func (s *Server) otherTierInFlight(vmid int, target string) (busyTarget, busyJob string, busy bool) { + for k, j := range s.jobs { + if k.vmid != vmid || k.target == target || j == nil { + continue + } + if backupInFlight(j.Phase) { + return k.target, j.JobID, true + } + } + return "", "", false +} + // markSnapshotted flips the guest's running job to the `snapshotted` phase (8B.2) — only if it is // still the current job and still running (don't regress done/failed, and don't touch a newer job). func (s *Server) markSnapshotted(key backupJobKey, jobID string) {