From 043c7622bcc0809977ff5b463bd8e71f0030ba6d Mon Sep 17 00:00:00 2001 From: Claude Code Date: Sun, 26 Jul 2026 21:00:42 +0200 Subject: [PATCH] R-85 Phase 2: tier rotation, persisted state, one heavy op at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scheduler could only ever see cfg.Backup.BackupTarget(), so the offsite tier's archives were never candidates — which is why demo-hp's DR tier reported 'applied' with zero snapshots for five days and nobody noticed. Selection: oldest-first (operator ruling, Option 1). Never-proven sorts first, which is where the offsite tier starts. Ties break on target id so ordering is deterministic rather than following Go's randomised map order. Rotation credit only on SUCCESS — a permanently failing tier must keep sorting first, not look freshly proven and stop being retried. - backup.RestoreTestState: persisted last-success per tier (atomic tmp+rename). This genuinely needs persistence unlike R-84: R-84 had ground truth to consult (the archive is still on the storage), whereas a restore-test destroys its scratch and leaves no artifact. Corrupt/missing file -> 'nothing proven'. - backup.InFlight: host-wide one-heavy-op gate shared with the local-API backup path. A LINK concern, not a lock one — an offsite restore pulls multi-GB over the same tunnel a backup pushes one, and at ~33 MB/min both drift toward timeout, which is how a healthy tier gets recorded as failed. Callers DEFER, never cancel. - PickRestoreCandidateOn: newest archive on a named tier; '' is not an error, or every fresh box looks broken for its first week. - An empty tier is skipped and the next tried; it cannot starve, since it is still least-recently-proven once it has an archive. - POST /backup joins the gate (409 naming the holder). Red-proofs A/E/F observed with the documented text. Full suite green (29 packages, rc=0). --- CHANGELOG.md | 48 ++++ cmd/felhom-agent/main.go | 31 ++- internal/backup/inflight.go | 57 +++++ internal/backup/restoretest_state.go | 139 +++++++++++ internal/backup/rotation_test.go | 335 +++++++++++++++++++++++++++ internal/backup/runner.go | 15 +- internal/backup/schedule.go | 111 ++++++++- internal/localapi/server.go | 25 +- 8 files changed, 743 insertions(+), 18 deletions(-) create mode 100644 internal/backup/inflight.go create mode 100644 internal/backup/restoretest_state.go create mode 100644 internal/backup/rotation_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d19540..1acf377 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,51 @@ +## v0.104.0-dev — R-85 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 +never candidates. That is why demo-hp's DR tier reported `applied` with **zero snapshots for five +days** and nobody noticed. + +### Selection — oldest-first (operator ruling 2026-07-26, Option 1) +The tier whose last **successful** restore-test is oldest goes first; **never-proven sorts first of +all**, which is exactly where the offsite tier starts. Self-balancing, no new config knob, and each +tier is covered every ~2 cadences — comfortably inside the 2-week offsite retention, so a tier is +never proven against an archive that is about to be pruned. Ties break on target id, because two +tiers proven in the same second would otherwise rotate by Go's randomised map order: untestable, and +occasionally starving. + +Rotation credit is given **only on success**. A tier that fails every time must keep sorting first — +otherwise a permanently broken tier would look freshly proven and quietly stop being retried. + +### Added +- **`backup.RestoreTestState`** — last successful restore-test per tier, persisted (atomic + tmp+rename). **This one genuinely needs persistence, unlike R-84**, and the difference is worth + stating because they look alike: R-84 had a GROUND TRUTH to consult (the archive is still on the + storage), so it never persisted anything. A restore-test destroys its scratch as its final act and + leaves no artifact — "did we prove this tier restores?" exists only as remembered state. A corrupt + or missing file degrades to "nothing proven", which is the correct starting point. +- **`backup.InFlight`** — the host-wide one-heavy-operation gate, shared by the restore-test + scheduler and the local-API backup path. Not a lock concern (the scratch VMID never touches the + live guest's vzdump lock) but a **LINK** concern: an offsite restore PULLS multi-GB over the same + tunnel an offsite backup PUSHES one. At the ~33 MB/min measured upstream, running both drives each + toward its timeout — which is how a healthy tier ends up recorded as failed. A caller that cannot + acquire **defers**; it never cancels what is already running. +- **`BackupRunner.PickRestoreCandidateOn`** — newest archive on a NAMED tier. `""` + nil error when + that tier holds none: **a tier with nothing to restore is not an error**, or every fresh box would + look broken for its first week. + +### Changed +- A tier with no archive is **skipped and the next tier tried**, not left to burn the cadence. It + cannot starve either — an empty tier is still the least recently proven, so it still sorts first + the moment it has an archive. +- `POST /backup` now also joins the gate: a **409** naming the holder when a restore-test is running. + +### Tests ++12. Red-proofs observed: +- **A** — the single-target picker yields `both tiers must be exercised across 4 cadences; got [local:… local:… local:… local:…]`. +- **E** — losing the state file yields `after a restart the OTHER tier must be next; got … twice (rotation state was lost)`. +- **F** — removing the gate yields `the restore-test must DEFER while a backup holds the gate; concurrent operations = 2`. **The count is the assertion** — "both completed" would pass against a fully concurrent implementation. + +Full suite green (29 packages, `rc=0`, vet unpiped). + ## v0.104.0-dev — R-85 Phase 1: the restore-test spec is built PER RUN (2026-07-26) Prerequisite for scheduling the offsite tier at all. Shipped on its own because it is independently diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index ec1469a..180f1d1 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -649,7 +649,11 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int // Self-restore-test scheduler (slice 6): the fourth daemon goroutine. Runs the restore- // test on the configured cadence (default 24h). Disabled cleanly when the cadence is off // OR the scratch band / restore storage is misconfigured — the daemon still runs. - scheduler := buildRestoreTestScheduler(cfg, px, engine, backupStore, logger) + // R-85: persisted per-tier restore-test state + the host-wide one-heavy-op gate, both shared + // with the local API so a backup and a restore-test can never run together. + rtState := backup.NewRestoreTestState(filepath.Join(cfg.OOB.WithDefaults().StateDir, "restore-test-state.json")) + heavyOps := &backup.InFlight{} + scheduler := buildRestoreTestScheduler(cfg, px, engine, backupStore, rtState, heavyOps, logger) // PBS verify loop (slice 6 Phase B): the fifth daemon goroutine. Cheap, key-free, // ciphertext-level integrity check on its own cadence (default 6h), reporting per-snapshot @@ -757,7 +761,7 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int return false }, } - localSrv := buildLocalAPIServer(cfg, px, backupStore, observer, driveKnown, hostOps, gate, collector, intentRec, guestBindStore, formatJobStore, logRing, escrowCeremonyCfg, logger, &localTokens) + localSrv := buildLocalAPIServer(cfg, px, backupStore, heavyOps, observer, driveKnown, hostOps, gate, collector, intentRec, guestBindStore, formatJobStore, logRing, escrowCeremonyCfg, logger, &localTokens) if localTokens != nil { defer localTokens.Close() } @@ -1232,7 +1236,7 @@ func readTrimmed(path string) (string, error) { // disables the cadence (returns a scheduler that just waits) when the cadence is off or the // scratch band / restore storage is invalid — a misconfig must not crash the daemon, and the // machinery still works on-demand via --selftest=restore-test. -func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *reconcile.Engine, store *backup.Store, logger *slog.Logger) *backup.Scheduler { +func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *reconcile.Engine, store *backup.Store, rtState *backup.RestoreTestState, inFlight *backup.InFlight, logger *slog.Logger) *backup.Scheduler { cadence := cfg.Backup.RestoreTestCadence() if cadence > 0 { if err := cfg.Backup.ValidateForRestoreTest(); err != nil { @@ -1243,6 +1247,12 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re min, max := cfg.Backup.ScratchBand() target := cfg.Backup.BackupTarget() runner := backup.NewBackupRunner(px, target, "", "felhom restore-test", "", logger) + // Every configured tier is a rotation candidate, not just the primary. + cfgTiers, _ := cfg.Backup.BackupTiers() // warnings already logged where the tiers are armed + tierIDs := make([]string, 0, len(cfgTiers)) + for _, t := range cfgTiers { + tierIDs = append(tierIDs, t.TargetID) + } return backup.NewScheduler(backup.SchedulerOptions{ Runner: engine, Pick: runner.PickRestoreCandidate, @@ -1270,6 +1280,14 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re }, Cadence: cadence, Logger: logger, + + // R-85: rotate across EVERY configured tier, oldest-proven first (operator ruling, Option 1). + // Before this the scheduler only ever saw cfg.Backup.BackupTarget(), so the offsite tier's + // archives were never candidates and the DR tier went unproven for its whole existence. + Tiers: tierIDs, + TierPick: runner.PickRestoreCandidateOn, + State: rtState, + InFlight: inFlight, }) } @@ -1278,7 +1296,7 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re // leaf (stable fingerprint). Any failure DISABLES the server (returns nil) WITHOUT crashing the // daemon — the host still reports/reconciles; only the controller channel is unavailable until // fixed. The opened token store is returned via outTokens so the caller can Close it. -func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.Store, observer *storage.Observer, driveTargets storage.KnownTargets, hostOps *storage.SudoHostOps, gate *reconcile.Gate, collector *hub.Collector, intent localapi.IntentRecorder, guestBinds *localapi.GuestBindStore, formatJobs *localapi.FormatJobStore, logRing *applog.Ring, escrowCeremony *localapi.EscrowCeremonyConfig, logger *slog.Logger, outTokens **localapi.TokenStore) *localapi.Server { +func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.Store, inFlight *backup.InFlight, observer *storage.Observer, driveTargets storage.KnownTargets, hostOps *storage.SudoHostOps, gate *reconcile.Gate, collector *hub.Collector, intent localapi.IntentRecorder, guestBinds *localapi.GuestBindStore, formatJobs *localapi.FormatJobStore, logRing *applog.Ring, escrowCeremony *localapi.EscrowCeremonyConfig, logger *slog.Logger, outTokens **localapi.TokenStore) *localapi.Server { if !cfg.LocalAPI.Enabled() { return nil } @@ -1357,6 +1375,7 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St Guests: px, Backups: runner, BackupTiers: apiTiers, // R-82: primary first; untargeted endpoints act on the primary + InFlight: inFlight, // R-85: shared with the restore-test scheduler (Scenario F) Store: store, Storage: observer, DriveTargets: driveTargets, // Impl-2a: registry+units drives for the /disks view (union w/ Observe storages) @@ -1846,7 +1865,7 @@ func runSelftestBringUp(ctx context.Context, cfg config.Config, logger *slog.Log Cores: sizing.Cores, MemoryMB: sizing.MemoryMB, RootfsGrowGB: sizing.RootfsGrowGB, DataVolGrowGB: sizing.DataVolGrowGB, DataVolMount: sizing.DataVolMount, SysDataGrowGB: sizing.SysDataGrowGB, SysDataMount: sizing.SysDataMount, - IslandBridge: cfg.LocalAPI.IslandBridge, IslandGuestAddr: cfg.LocalAPI.IslandGuestAddr, // R-50 island NIC (both empty = pre-R-50) + IslandBridge: cfg.LocalAPI.IslandBridge, IslandGuestAddr: cfg.LocalAPI.IslandGuestAddr, // R-50 island NIC (both empty = pre-R-50) } fmt.Printf(" bringing up %s → vmid %d on %s …\n", archive, vmid, cfg.Backup.RestoreStorage) res := engine.RunBringUp(ctx, spec) @@ -2010,7 +2029,7 @@ func runSelftestProvision(ctx context.Context, cfg config.Config, logger *slog.L Cores: a.sizing.Cores, MemoryMB: a.sizing.MemoryMB, RootfsGrowGB: a.sizing.RootfsGrowGB, DataVolGrowGB: a.sizing.DataVolGrowGB, DataVolMount: a.sizing.DataVolMount, SysDataGrowGB: a.sizing.SysDataGrowGB, SysDataMount: a.sizing.SysDataMount, - IslandBridge: cfg.LocalAPI.IslandBridge, IslandGuestAddr: cfg.LocalAPI.IslandGuestAddr, // R-50 island NIC (both empty = pre-R-50) + IslandBridge: cfg.LocalAPI.IslandBridge, IslandGuestAddr: cfg.LocalAPI.IslandGuestAddr, // R-50 island NIC (both empty = pre-R-50) }) if res.Err != nil || !res.Pass { fmt.Fprintf(os.Stderr, " [FAIL] front-half bring-up (vmid %d): %v\n", a.vmid, res.Err) diff --git a/internal/backup/inflight.go b/internal/backup/inflight.go new file mode 100644 index 0000000..af543f0 --- /dev/null +++ b/internal/backup/inflight.go @@ -0,0 +1,57 @@ +package backup + +import "sync" + +// InFlight is the host-wide "one heavy guest operation at a time" gate. +// +// R-85 (Scenario F). The operator's R-82 ruling was "one backup at a time per guest"; a restore-test +// must JOIN that single-flight rather than sit outside it. It is not a lock-contention concern — +// a restore-test uses a scratch VMID, so it never touches the live guest's vzdump lock. It is a +// LINK concern: an offsite restore PULLS a multi-GB archive while an offsite backup PUSHES one, over +// the same WireGuard tunnel. On the demo fleet that link runs at ~33 MB/min upstream; running both +// at once makes each slower and pushes both toward their timeouts, which is how a healthy tier ends +// up recorded as failed. +// +// It is deliberately host-wide and coarse rather than per-guest: these boxes carry one customer +// guest, and the resource being protected (the uplink) is shared by everything on the host anyway. +// +// The gate is ADVISORY in one direction only — it never cancels anything already running. A caller +// that cannot acquire DEFERS to its next cadence. Deferring a restore-test costs a few hours of +// coverage; cancelling a running backup costs the backup. +type InFlight struct { + mu sync.Mutex + what string // "" = idle +} + +// TryAcquire claims the gate for `what`. ok=false means something else holds it, and `busy` names +// it — the name matters, because "deferred" with no reason is indistinguishable from "broken". +func (g *InFlight) TryAcquire(what string) (release func(), busy string, ok bool) { + if g == nil { + // Not wired (older call sites, tests) → no gating, previous behaviour. + return func() {}, "", true + } + g.mu.Lock() + defer g.mu.Unlock() + if g.what != "" { + return nil, g.what, false + } + g.what = what + var once sync.Once + return func() { + once.Do(func() { + g.mu.Lock() + g.what = "" + g.mu.Unlock() + }) + }, "", true +} + +// Busy reports what currently holds the gate ("" = idle). +func (g *InFlight) Busy() string { + if g == nil { + return "" + } + g.mu.Lock() + defer g.mu.Unlock() + return g.what +} diff --git a/internal/backup/restoretest_state.go b/internal/backup/restoretest_state.go new file mode 100644 index 0000000..e078521 --- /dev/null +++ b/internal/backup/restoretest_state.go @@ -0,0 +1,139 @@ +package backup + +import ( + "encoding/json" + "os" + "path/filepath" + "sort" + "sync" + "time" +) + +// RestoreTestState persists the last SUCCESSFUL restore-test per backup tier. +// +// R-85 (1.4). This one genuinely needs PERSISTENCE, unlike R-84 — and the difference is worth +// stating, because the two look like the same problem and are not: +// +// - R-84 (backup freshness) had a GROUND TRUTH to consult: the archive is still on the storage, +// so the agent could ask "when did a backup last land?" and never persist anything. That is +// strictly better, because a pruned archive correctly stops counting. +// - A restore-test leaves NO artifact — the scratch guest is destroyed as its final act. There is +// nothing to query. "Did we prove this tier restores?" exists only as remembered state, so it +// must be written down or it is lost. +// +// Why it must survive a restart: rotation is oldest-first (the operator ruling), so an in-memory map +// would reset every tier to "never tested" on each restart. Ordering would then depend on map +// iteration order, and one tier could be starved indefinitely while the other is re-tested — with +// agent deploys as routine as they are, that is not a corner case. +// +// Only SUCCESS is recorded. A failed run must not satisfy rotation, or a tier that fails every time +// would look freshly proven and stop being retried — the same "a failure satisfies the cadence" +// trap the backup due-check avoids. +type RestoreTestState struct { + path string + mu sync.Mutex + last map[string]time.Time // target id → last SUCCESSFUL restore-test (UTC) +} + +// NewRestoreTestState opens (or creates) the state at path. A missing or unreadable file is NOT an +// error: it degrades to "nothing proven yet", which is the correct starting point and keeps a +// corrupt file from wedging the daemon. +func NewRestoreTestState(path string) *RestoreTestState { + s := &RestoreTestState{path: path, last: map[string]time.Time{}} + data, err := os.ReadFile(path) + if err != nil { + return s + } + var raw map[string]string + if json.Unmarshal(data, &raw) != nil { + return s + } + for target, ts := range raw { + if t, perr := time.Parse(time.RFC3339, ts); perr == nil { + s.last[target] = t.UTC() + } + } + return s +} + +// RecordSuccess stamps a tier as proven at t. Only call this for a PASSING restore-test. +func (s *RestoreTestState) RecordSuccess(target string, t time.Time) error { + if target == "" { + return nil + } + s.mu.Lock() + defer s.mu.Unlock() + s.last[target] = t.UTC() + return s.saveLocked() +} + +// LastSuccess returns when this tier was last proven (ok=false = never). +func (s *RestoreTestState) LastSuccess(target string) (time.Time, bool) { + s.mu.Lock() + defer s.mu.Unlock() + t, ok := s.last[target] + return t, ok +} + +// Snapshot returns a copy of the whole map — for the host-report gauge. +func (s *RestoreTestState) Snapshot() map[string]time.Time { + s.mu.Lock() + defer s.mu.Unlock() + out := make(map[string]time.Time, len(s.last)) + for k, v := range s.last { + out[k] = v + } + return out +} + +// OldestFirst orders targets by "least recently proven first"; never-proven sorts FIRST. +// +// This is the operator's 2026-07-26 ruling (Option 1): self-balancing, no new config knob, and it +// naturally prioritises a tier that has never been restore-tested at all — which on this fleet was +// the offsite tier, unproven for its entire existence. +// +// Ties break on target id so the order is deterministic; without that, two tiers proven in the same +// second would rotate by map iteration order, which is randomised in Go and would make the +// behaviour untestable and occasionally starving. +func (s *RestoreTestState) OldestFirst(targets []string) []string { + s.mu.Lock() + defer s.mu.Unlock() + out := append([]string(nil), targets...) + sort.SliceStable(out, func(i, j int) bool { + ti, oki := s.last[out[i]] + tj, okj := s.last[out[j]] + switch { + case !oki && !okj: + return out[i] < out[j] // both never proven → deterministic + case !oki: + return true // never proven wins + case !okj: + return false + case !ti.Equal(tj): + return ti.Before(tj) + default: + return out[i] < out[j] + } + }) + return out +} + +func (s *RestoreTestState) saveLocked() error { + raw := make(map[string]string, len(s.last)) + for target, t := range s.last { + raw[target] = t.UTC().Format(time.RFC3339) + } + data, err := json.MarshalIndent(raw, "", " ") + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(s.path), 0o755); err != nil { + return err + } + tmp := s.path + ".tmp" + if err := os.WriteFile(tmp, data, 0o600); err != nil { + os.Remove(tmp) + return err + } + return os.Rename(tmp, s.path) +} diff --git a/internal/backup/rotation_test.go b/internal/backup/rotation_test.go new file mode 100644 index 0000000..e10a177 --- /dev/null +++ b/internal/backup/rotation_test.go @@ -0,0 +1,335 @@ +package backup + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/reconcile" +) + +// R-85 Phase 2 — tier rotation, persisted state, and the one-heavy-operation gate. +// +// The failure this prevents is not hypothetical: demo-hp's DR tier reported `applied` with ZERO +// snapshots for five days and nobody noticed, because the scheduler could only ever see the primary +// tier. Rotation is what makes the offsite tier testable at all. + +// rotRunner records which archives it was asked to restore. +type rotRunner struct { + mu sync.Mutex + archives []string + pass bool +} + +func (r *rotRunner) RunRestoreTest(_ context.Context, spec reconcile.RestoreTestSpec) reconcile.RestoreTestResult { + r.mu.Lock() + defer r.mu.Unlock() + r.archives = append(r.archives, spec.Archive) + return reconcile.RestoreTestResult{ + Archive: spec.Archive, SourceTier: spec.SourceTier, + Pass: r.pass, Verified: "boot+running", + } +} +func (r *rotRunner) seen() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.archives...) +} + +// archiveFor is a TierPicker over a fixed map: target → archive ("" = that tier holds none). +func archiveFor(m map[string]string) TierPicker { + return func(_ context.Context, target string) (string, error) { + return m[target], nil + } +} + +func rotScheduler(t *testing.T, rr *rotRunner, st *RestoreTestState, pick TierPicker, gate *InFlight) *Scheduler { + t.Helper() + return NewScheduler(SchedulerOptions{ + Runner: rr, + Store: NewStore(), + Spec: func(_ context.Context, archive string) reconcile.RestoreTestSpec { + return reconcile.RestoreTestSpec{RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009} + }, + Cadence: time.Hour, + Logger: quiet(), + Tiers: []string{"local", "felhom-pbs"}, + TierPick: pick, + State: st, + InFlight: gate, + }) +} + +// ── SCENARIO A — both tiers get tested across consecutive cadences ─────────────────────────── +// +// COMPANION RED-PROOF (observed): restore the single-target picker — set `Tiers`/`TierPick` to nil +// so `pickForThisRun` falls back to `s.pick` on the primary runner — and this fails with +// "both tiers must be exercised across 4 cadences; got [local:… local:… local:… local:…]", +// i.e. the offsite tier never appears. That is today's behaviour, and it is why demo-hp's DR tier +// went unproven for its entire existence. +func TestRotation_BothTiersExercisedAcrossCadences(t *testing.T) { + rr := &rotRunner{pass: true} + st := NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")) + s := rotScheduler(t, rr, st, archiveFor(map[string]string{ + "local": "local:backup/vzdump-lxc-9201-x.tar.zst", + "felhom-pbs": "felhom-pbs:backup/ct/9201/2026-07-26T15:42:42Z", + }), &InFlight{}) + s.now = func() time.Time { return time.Now().UTC() } + + for i := 0; i < 4; i++ { + s.tick(context.Background()) + } + + got := rr.seen() + var sawLocal, sawPBS bool + for _, a := range got { + if len(a) >= 5 && a[:5] == "local" { + sawLocal = true + } + if len(a) >= 10 && a[:10] == "felhom-pbs" { + sawPBS = true + } + } + if !sawLocal || !sawPBS { + t.Fatalf("both tiers must be exercised across 4 cadences; got %v", got) + } + // Oldest-first must ALTERNATE, not clump — otherwise one tier is starved between visits. + if len(got) != 4 { + t.Fatalf("want 4 runs, got %d: %v", len(got), got) + } + if got[0] == got[1] { + t.Fatalf("consecutive runs hit the same tier — oldest-first is not rotating: %v", got) + } +} + +// A tier with NO archive is skipped, not failed, and the other tier still runs. A brand-new offsite +// tier legitimately has nothing to restore; turning that into a failure would make every fresh box +// look broken for its first week. +func TestRotation_EmptyTierSkippedNotFailed(t *testing.T) { + rr := &rotRunner{pass: true} + st := NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")) + s := rotScheduler(t, rr, st, archiveFor(map[string]string{ + "local": "local:backup/vzdump-lxc-9201-x.tar.zst", + "felhom-pbs": "", // provisioned but empty + }), &InFlight{}) + + s.tick(context.Background()) + got := rr.seen() + if len(got) != 1 || got[0][:5] != "local" { + t.Fatalf("an empty tier must be skipped and the testable one still run; got %v", got) + } +} + +// Nothing testable anywhere → a clean no-op, not an error and not a run. +func TestRotation_NoArchivesAnywhereIsANoOp(t *testing.T) { + rr := &rotRunner{pass: true} + s := rotScheduler(t, rr, NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")), + archiveFor(map[string]string{}), &InFlight{}) + s.tick(context.Background()) + if got := rr.seen(); len(got) != 0 { + t.Fatalf("no archives anywhere → no run; got %v", got) + } +} + +// A FAILED restore-test must NOT earn rotation credit, or a tier that fails every time would look +// freshly proven and quietly stop being retried. +func TestRotation_FailureEarnsNoCredit(t *testing.T) { + rr := &rotRunner{pass: false} + st := NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")) + s := rotScheduler(t, rr, st, archiveFor(map[string]string{ + "local": "local:backup/x.tar.zst", + "felhom-pbs": "felhom-pbs:backup/ct/9201/y", + }), &InFlight{}) + s.tick(context.Background()) + if _, ok := st.LastSuccess("local"); ok { + t.Fatal("a FAILED restore-test must not stamp the tier as proven") + } + if _, ok := st.LastSuccess("felhom-pbs"); ok { + t.Fatal("a FAILED restore-test must not stamp the tier as proven") + } +} + +// ── SCENARIO E — rotation survives a restart ───────────────────────────────────────────────── +// +// COMPANION RED-PROOF (observed): make the state in-memory (construct a fresh +// `NewRestoreTestState` on a DIFFERENT path for the second scheduler, i.e. lose the file) and this +// fails with "after a restart the OTHER tier must be next; got felhom-pbs" — the same tier repeats +// and the other is starved indefinitely, which with agent deploys as routine as they are is not a +// corner case. +func TestRotation_SurvivesRestart(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "rt.json") + picks := archiveFor(map[string]string{ + "local": "local:backup/x.tar.zst", + "felhom-pbs": "felhom-pbs:backup/ct/9201/y", + }) + + // First process: the OFFSITE tier is tested (never-proven sorts first). + rr1 := &rotRunner{pass: true} + st1 := NewRestoreTestState(path) + s1 := rotScheduler(t, rr1, st1, picks, &InFlight{}) + s1.tick(context.Background()) + first := rr1.seen() + if len(first) != 1 { + t.Fatalf("want one run, got %v", first) + } + + // --- restart: brand-new state object reading the SAME file --- + rr2 := &rotRunner{pass: true} + st2 := NewRestoreTestState(path) + s2 := rotScheduler(t, rr2, st2, picks, &InFlight{}) + s2.tick(context.Background()) + second := rr2.seen() + if len(second) != 1 { + t.Fatalf("want one run after restart, got %v", second) + } + + if second[0] == first[0] { + t.Fatalf("after a restart the OTHER tier must be next; got %s twice (rotation state was lost)", second[0]) + } +} + +// ── SCENARIO F — no collision with a backup ────────────────────────────────────────────────── +// +// COMPANION RED-PROOF (observed): drop the TryAcquire guard from `tick` and this fails with +// "the restore-test must DEFER while a backup holds the gate; concurrent operations = 2" — the +// count is the assertion, since "both completed" would pass against a fully concurrent +// implementation. +func TestRotation_DefersWhileABackupHoldsTheGate(t *testing.T) { + gate := &InFlight{} + release, _, ok := gate.TryAcquire("backup:felhom-pbs") + if !ok { + t.Fatal("precondition: the gate should have been free") + } + defer release() + + rr := &rotRunner{pass: true} + s := rotScheduler(t, rr, NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")), + archiveFor(map[string]string{"local": "local:backup/x.tar.zst"}), gate) + + s.tick(context.Background()) + + concurrent := 1 + len(rr.seen()) // the backup holding the gate, plus anything the tick started + if concurrent != 1 { + t.Fatalf("the restore-test must DEFER while a backup holds the gate; concurrent operations = %d", concurrent) + } +} + +// Once the backup releases, the next cadence proceeds — deferral must not be permanent. +func TestRotation_ResumesAfterTheGateFrees(t *testing.T) { + gate := &InFlight{} + release, _, _ := gate.TryAcquire("backup:local") + + rr := &rotRunner{pass: true} + s := rotScheduler(t, rr, NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")), + archiveFor(map[string]string{"local": "local:backup/x.tar.zst"}), gate) + + s.tick(context.Background()) + if len(rr.seen()) != 0 { + t.Fatal("should have deferred while held") + } + release() + s.tick(context.Background()) + if len(rr.seen()) != 1 { + t.Fatalf("must resume once the gate frees; got %v", rr.seen()) + } +} + +// The gate itself: one holder at a time, named, and release is idempotent. +func TestInFlight_Semantics(t *testing.T) { + g := &InFlight{} + rel, busy, ok := g.TryAcquire("backup:local") + if !ok || busy != "" { + t.Fatalf("first acquire must succeed; ok=%v busy=%q", ok, busy) + } + if _, busy2, ok2 := g.TryAcquire("restore-test"); ok2 || busy2 != "backup:local" { + t.Fatalf("second acquire must fail and NAME the holder; ok=%v busy=%q", ok2, busy2) + } + rel() + rel() // idempotent — a double release must not free someone else's later claim + if g.Busy() != "" { + t.Fatalf("gate should be idle after release; busy=%q", g.Busy()) + } + if _, _, ok3 := g.TryAcquire("restore-test"); !ok3 { + t.Fatal("gate must be reusable after release") + } +} + +// A nil gate means "not wired" → no gating, pre-R-85 behaviour. Keeps every existing caller working. +func TestInFlight_NilIsUngated(t *testing.T) { + var g *InFlight + rel, _, ok := g.TryAcquire("x") + if !ok { + t.Fatal("a nil gate must not block") + } + rel() + if g.Busy() != "" { + t.Fatal("a nil gate is never busy") + } +} + +// ── oldest-first ordering ──────────────────────────────────────────────────────────────────── + +func TestOldestFirst_Ordering(t *testing.T) { + st := NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")) + now := time.Now().UTC() + + // Never-proven sorts FIRST — the case that matters, since the offsite tier starts there. + if got := st.OldestFirst([]string{"local", "felhom-pbs"}); got[0] != "felhom-pbs" { + // both never proven → deterministic tie-break by id + if got[0] != "felhom-pbs" && got[0] != "local" { + t.Fatalf("unexpected: %v", got) + } + } + _ = st.RecordSuccess("local", now) + if got := st.OldestFirst([]string{"local", "felhom-pbs"}); got[0] != "felhom-pbs" { + t.Fatalf("a never-proven tier must sort before a proven one; got %v", got) + } + _ = st.RecordSuccess("felhom-pbs", now.Add(time.Hour)) + if got := st.OldestFirst([]string{"local", "felhom-pbs"}); got[0] != "local" { + t.Fatalf("the least recently proven must sort first; got %v", got) + } +} + +// Ordering must be DETERMINISTIC for equal timestamps, or two tiers proven in the same second would +// rotate by Go's randomised map iteration — untestable, and occasionally starving. +func TestOldestFirst_DeterministicOnTies(t *testing.T) { + st := NewRestoreTestState(filepath.Join(t.TempDir(), "rt.json")) + now := time.Now().UTC() + _ = st.RecordSuccess("b-tier", now) + _ = st.RecordSuccess("a-tier", now) + for i := 0; i < 20; i++ { + if got := st.OldestFirst([]string{"b-tier", "a-tier"}); got[0] != "a-tier" { + t.Fatalf("tie-break must be deterministic; iteration %d gave %v", i, got) + } + } +} + +// The state file round-trips, and a corrupt file degrades to "nothing proven" rather than wedging. +func TestRestoreTestState_PersistenceAndCorruption(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "rt.json") + now := time.Now().UTC().Truncate(time.Second) + + st := NewRestoreTestState(path) + if err := st.RecordSuccess("felhom-pbs", now); err != nil { + t.Fatal(err) + } + reopened := NewRestoreTestState(path) + got, ok := reopened.LastSuccess("felhom-pbs") + if !ok || !got.Equal(now) { + t.Fatalf("state must round-trip; got %v ok=%v want %v", got, ok, now) + } + + bad := filepath.Join(dir, "corrupt.json") + if err := os.WriteFile(bad, []byte("{{{not json"), 0o600); err != nil { + t.Fatal(err) + } + c := NewRestoreTestState(bad) + if _, ok := c.LastSuccess("felhom-pbs"); ok { + t.Fatal("a corrupt state file must degrade to 'nothing proven', not invent a timestamp") + } +} diff --git a/internal/backup/runner.go b/internal/backup/runner.go index 5b83a36..63da28d 100644 --- a/internal/backup/runner.go +++ b/internal/backup/runner.go @@ -243,7 +243,20 @@ func (r *BackupRunner) watchForSnapshot(ctx context.Context, upid string, onSnap // PickRestoreCandidate returns the newest backup archive on the target (any guest), or "" // when there is none — the restore-test then no-ops cleanly. func (r *BackupRunner) PickRestoreCandidate(ctx context.Context) (string, error) { - contents, err := r.api.StorageContent(ctx, r.target) + return r.PickRestoreCandidateOn(ctx, r.target) +} + +// PickRestoreCandidateOn is PickRestoreCandidate for an ARBITRARY tier's storage (R-85 1.2), so the +// scheduler can rotate across tiers instead of only ever seeing this runner's own target. +// +// Contract preserved: "" + nil error when the storage holds no archive. **A tier with nothing to +// restore is not an error** — a brand-new offsite tier legitimately has nothing yet, and turning +// that into a failure would make every fresh box look broken for its first week. +func (r *BackupRunner) PickRestoreCandidateOn(ctx context.Context, target string) (string, error) { + if target == "" { + return "", nil + } + contents, err := r.api.StorageContent(ctx, target) if err != nil { return "", err } diff --git a/internal/backup/schedule.go b/internal/backup/schedule.go index 5fd3e3f..28dcbbd 100644 --- a/internal/backup/schedule.go +++ b/internal/backup/schedule.go @@ -32,6 +32,11 @@ type CandidatePicker func(ctx context.Context) (string, error) // PBS archive was classified "local" and got the 10-minute local wait. type SpecBuilder func(ctx context.Context, archive string) reconcile.RestoreTestSpec +// TierPicker resolves the newest archive on a NAMED tier, or "" when that tier holds none. +// (*BackupRunner).PickRestoreCandidateOn satisfies it. "" must NOT be an error — a brand-new +// offsite tier legitimately has nothing to restore yet. +type TierPicker func(ctx context.Context, target string) (string, error) + // Scheduler runs the self-restore-test on an agent-internal cadence. It is the fourth daemon // goroutine; it does real restore→boot→destroy, so it only runs when the cadence is enabled // AND a valid scratch band is configured (validated by the caller before construction). @@ -43,6 +48,13 @@ type Scheduler struct { cadence time.Duration logger *slog.Logger now func() time.Time + + // R-85 tier rotation. All optional: without them the scheduler behaves exactly as before + // (single tier via `pick`), which keeps every existing caller and test working untouched. + tiers []string // configured tier target ids, primary first + tierPick TierPicker // newest archive on a named tier + rtState *RestoreTestState // persisted last-successful-per-tier (drives oldest-first) + inFlight *InFlight // shared with the backup path — Scenario F } // SchedulerOptions configures a Scheduler. @@ -55,6 +67,14 @@ type SchedulerOptions struct { Spec SpecBuilder Cadence time.Duration // 0 → disabled Logger *slog.Logger + + // R-85 (all optional — omit for the pre-R-85 single-tier behaviour): + // Tiers are the configured tier target ids (primary first); TierPick resolves an archive on a + // named tier; State persists last-successful-per-tier; InFlight is the shared one-heavy-op gate. + Tiers []string + TierPick TierPicker + State *RestoreTestState + InFlight *InFlight } // NewScheduler builds a Scheduler. @@ -64,13 +84,17 @@ func NewScheduler(opts SchedulerOptions) *Scheduler { logger = slog.Default() } return &Scheduler{ - runner: opts.Runner, - pick: opts.Pick, - store: opts.Store, - spec: opts.Spec, - cadence: opts.Cadence, - logger: logger, - now: func() time.Time { return time.Now().UTC() }, + runner: opts.Runner, + pick: opts.Pick, + store: opts.Store, + spec: opts.Spec, + cadence: opts.Cadence, + logger: logger, + now: func() time.Time { return time.Now().UTC() }, + tiers: append([]string(nil), opts.Tiers...), + tierPick: opts.TierPick, + rtState: opts.State, + inFlight: opts.InFlight, } } @@ -79,7 +103,7 @@ func NewScheduler(opts SchedulerOptions) *Scheduler { // is heavy; the first runs one interval in) — on-demand runs use the selftest harness. // Returns nil on ctx cancellation. func (s *Scheduler) Run(ctx context.Context) error { - if s.cadence <= 0 || s.runner == nil || s.pick == nil || s.spec == nil { + if s.cadence <= 0 || s.runner == nil || s.spec == nil || (s.pick == nil && !s.rotating()) { s.logger.Info("backup: restore-test cadence disabled") <-ctx.Done() return nil @@ -108,7 +132,19 @@ func (s *Scheduler) tick(ctx context.Context) { s.logger.Error("backup: restore-test has no spec builder — skipping (this is a wiring bug)") return } - archive, err := s.pick(ctx) + // Scenario F: join the one-heavy-operation-at-a-time gate. A restore-test PULLS a multi-GB + // archive over the same tunnel an offsite backup PUSHES one; running both saturates the link and + // drives each toward its timeout, which is how a healthy tier gets recorded as failed. DEFER — + // never cancel what is already running: a deferred restore-test costs hours of coverage, a + // cancelled backup costs the backup. + release, busy, ok := s.inFlight.TryAcquire("restore-test") + if !ok { + s.logger.Info("backup: restore-test deferred — a heavy operation is already in flight", "busy", busy) + return + } + defer release() + + archive, target, err := s.pickForThisRun(ctx) if err != nil { s.logger.Warn("backup: restore-test could not pick a candidate; skipping", "err", err) return @@ -126,6 +162,13 @@ func (s *Scheduler) tick(ctx context.Context) { } rt := ToHubRestoreTest(res, s.now()) s.store.RecordRestoreTest(rt) + // Rotation credit is given ONLY on success. A failing tier must keep sorting first, or a tier + // that fails every time would look freshly proven and quietly stop being retried. + if rt.Pass && s.rtState != nil && target != "" { + if err := s.rtState.RecordSuccess(target, s.now()); err != nil { + s.logger.Warn("backup: could not persist the restore-test rotation state", "target", target, "err", err) + } + } switch { case !rt.Pass: // A failing restore-test is the loudest DR signal there is. @@ -142,3 +185,53 @@ func (s *Scheduler) tick(ctx context.Context) { "archive", rt.SourceArchive, "duration_s", rt.DurationSeconds, "warnings", res.StartWarnings) } } + +// rotating reports whether multi-tier rotation is wired. +func (s *Scheduler) rotating() bool { return len(s.tiers) > 0 && s.tierPick != nil } + +// pickForThisRun chooses the tier and its newest archive. +// +// OLDEST-FIRST (operator ruling 2026-07-26, Option 1): the tier whose last SUCCESSFUL restore-test +// is oldest goes first, never-proven first of all. Self-balancing, no config knob, and it naturally +// prioritises a tier that has never been proven — which on this fleet was the offsite tier, unproven +// for its entire existence while reporting `applied`. +// +// A tier with no archives is SKIPPED, not failed, and the next tier is tried. Skipping to a testable +// tier is strictly better than burning the whole cadence: a brand-new offsite tier has nothing to +// restore yet, and that is normal, not broken. It cannot starve the empty tier either — as soon as +// it has an archive it still sorts first, because it is still the least recently proven. +// +// Returns ("", "", nil) when nothing anywhere is testable. +func (s *Scheduler) pickForThisRun(ctx context.Context) (archive, target string, err error) { + if !s.rotating() { + a, perr := s.pick(ctx) + return a, "", perr // pre-R-85 single-tier path; no rotation credit to record + } + order := s.tiers + if s.rtState != nil { + order = s.rtState.OldestFirst(s.tiers) + } + var firstErr error + for _, t := range order { + a, perr := s.tierPick(ctx, t) + if perr != nil { + // One tier's storage being unreadable must not block the others. + s.logger.Warn("backup: restore-test candidate lookup failed for a tier; trying the next", + "target", t, "err", perr) + if firstErr == nil { + firstErr = perr + } + continue + } + if a == "" { + s.logger.Debug("backup: restore-test tier has no archive yet; trying the next", "target", t) + continue + } + s.logger.Info("backup: restore-test tier selected (oldest-proven first)", "target", t, "archive", a) + return a, t, nil + } + if firstErr != nil { + return "", "", firstErr + } + return "", "", nil +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 360961d..5c0d511 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -16,6 +16,7 @@ import ( "sync" "time" + "gitea.dooplex.hu/admin/felhom-agent/internal/backup" "gitea.dooplex.hu/admin/felhom-agent/internal/escrow" "gitea.dooplex.hu/admin/felhom-agent/internal/hub" applog "gitea.dooplex.hu/admin/felhom-agent/internal/log" @@ -127,6 +128,9 @@ type Options struct { // BackupTiers (R-82) is the resolved multi-tier policy, primary first. OPTIONAL: nil → one tier // synthesized from Backups + BackupCadence, i.e. exactly the pre-R-82 behaviour. BackupTiers []BackupTier + // InFlight (R-85) is the host-wide one-heavy-operation gate shared with the restore-test + // scheduler. OPTIONAL: nil → no cross-gating (pre-R-85 behaviour). See backup.InFlight. + InFlight *backup.InFlight // Disk management (slice 8C) — OPTIONAL. When Disks + DiskGate are set, the /disks endpoints // are served; otherwise they report "not configured". DiskGate authorizes the destructive // (data-bearing) format path; Guests lists guests for the eject dependent-warning. @@ -240,8 +244,10 @@ type Server struct { // caller supplies no tiers it holds exactly one, synthesized from Backups+BackupCadence, which // is the pre-R-82 shape. tiers []BackupTier - logger *slog.Logger - now func() time.Time + // inFlight (R-85) is shared with the restore-test scheduler so the two never run together. + inFlight *backup.InFlight + logger *slog.Logger + now func() time.Time disks DiskOps // slice 8C (optional) diskGate StorageGate // slice 8C (optional) @@ -391,6 +397,7 @@ func NewServer(o Options) (*Server, error) { // (and every existing test) keeps working untouched. Exactly one tier is marked primary, and // the primary is always first, because that is what the untargeted endpoints act on. s.tiers = normalizeBackupTiers(o.BackupTiers, o.Backups, cadence) + s.inFlight = o.InFlight if s.backups == nil && len(s.tiers) > 0 { s.backups = s.tiers[0].Service } @@ -750,6 +757,19 @@ func (s *Server) handleBackup(w http.ResponseWriter, r *http.Request, vmid int) // otherwise collide and hand the second caller the first tier's id. The PRIMARY keeps the // pre-R-82 format byte-for-byte — an old controller stores this string and polls with it — so // only the additive tiers carry the target segment. + // R-85 Scenario F: a backup and a restore-test must never run together — both move multi-GB over + // the same tunnel. Acquired here (still holding jobsMu is fine: TryAcquire never blocks) and + // released when the fire-and-forget goroutine finishes. + release, busy, free := s.inFlight.TryAcquire("backup:" + tier.TargetID) + if !free { + s.jobsMu.Unlock() + s.logger.Info("local-api: backup refused — a heavy operation is already in flight", + "vmid", vmid, "requested_target", tier.TargetID, "busy", busy) + writeStatus(w, http.StatusConflict, false, nil, + "a heavy operation is already in flight ("+busy+") — only one runs at a time on this host") + return + } + jobID := "backup-" + strconv.Itoa(vmid) + "-" + strconv.FormatInt(s.now().UnixNano(), 10) if !tier.Primary && tier.TargetID != "" { jobID = "backup-" + strconv.Itoa(vmid) + "-" + tier.TargetID + "-" + strconv.FormatInt(s.now().UnixNano(), 10) @@ -767,6 +787,7 @@ func (s *Server) handleBackup(w http.ResponseWriter, r *http.Request, vmid int) base = context.Background() } go func() { + defer release() // R-85: free the host-wide gate when this backup finishes, however it ends // Outer bound = the tier's own wait bound + headroom for the pre/post work around WaitTask. // A fixed 2h here would silently cap a 6h offsite tier. outer := tier.WaitTimeout