diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e4bdd9..4d19540 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,31 @@ +## 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 +correct and independently testable. + +`SchedulerOptions.Spec` was a **value**, produced by an immediately-invoked function at daemon start +(`main.go`). So `storageTier()` and `restoreTaskTimeout()` were evaluated **once** and their result +reused for every run for the lifetime of the process. Two consequences: + +- **Nothing tier-varying was expressible.** The offsite tier could never be scheduled, because the + spec's tier was fixed to whatever the configured target was at boot. +- **A latent staleness bug in its own right:** a storage-type or config change did not take effect + until the daemon restarted. + +### Changed +- **`backup.SpecBuilder`** — `func(ctx, archive) reconcile.RestoreTestSpec`, called **once per run**. + The archive is passed in because the tier MUST come from it (`restoreTierForArchive`, the v0.100.0 + rule). Deriving it from the configured target is what classified a PBS archive as `local` and + killed a 14.46 GB WAN restore at the 10-minute local bound. +- **A nil spec builder SKIPS loudly instead of panicking.** `Run` already refused to start without + one, but `tick` is reachable directly; a wiring bug must cost a restore-test, never the daemon + goroutine. + +### Tests ++3. Red-proof observed: restoring the frozen value fails with +`the spec builder must run ONCE PER RUN, got 0 call(s) across 3 ticks`. Full suite green +(29 packages, `rc=0`, vet unpiped). + ## 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 diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 5648680..ec1469a 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1247,8 +1247,19 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re Runner: engine, Pick: runner.PickRestoreCandidate, Store: store, - Spec: func() reconcile.RestoreTestSpec { - tier := storageTier(context.Background(), px, target) + // R-85 (1.1): the spec is built PER RUN, from the archive that was picked. + // + // This used to be an immediately-invoked function, so storageTier() and + // restoreTaskTimeout() ran ONCE at daemon start and their result was reused for every run + // forever. That froze the tier — and with it the timeout — making an offsite restore-test + // impossible to schedule, and leaving any storage-type or config change stale until the + // daemon restarted. + // + // The tier comes from the ARCHIVE (restoreTierForArchive, the v0.100.0 rule), never from + // the configured target: config-derived was what classified a PBS archive as "local" and + // killed a 14.46 GB WAN restore at the 10-minute local bound. + Spec: func(ctx context.Context, archive string) reconcile.RestoreTestSpec { + tier := restoreTierForArchive(ctx, px, archive, target) return reconcile.RestoreTestSpec{ RestoreStorage: cfg.Backup.RestoreStorage, ScratchMin: min, @@ -1256,7 +1267,7 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re SourceTier: tier, RestoreTaskTimeout: restoreTaskTimeout(cfg, tier), } - }(), + }, Cadence: cadence, Logger: logger, }) diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 6fe78e5..459ec35 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -168,9 +168,12 @@ func TestScheduler_TickRunsAndRecords(t *testing.T) { store := NewStore() rt := &fakeRTRunner{res: reconcile.RestoreTestResult{Archive: "vol", Pass: true, Verified: "boot+running", Duration: time.Second}} s := NewScheduler(SchedulerOptions{ - Runner: rt, - Pick: func(context.Context) (string, error) { return "vol", nil }, - Store: store, + Runner: rt, + Pick: func(context.Context) (string, error) { return "vol", nil }, + Store: store, + Spec: func(context.Context, string) reconcile.RestoreTestSpec { + return reconcile.RestoreTestSpec{RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009} + }, Cadence: time.Hour, Logger: quiet(), }) diff --git a/internal/backup/schedule.go b/internal/backup/schedule.go index 8988e5a..5fd3e3f 100644 --- a/internal/backup/schedule.go +++ b/internal/backup/schedule.go @@ -18,6 +18,20 @@ type RestoreTestRunner interface { // there is none yet (the tick then no-ops). type CandidatePicker func(ctx context.Context) (string, error) +// SpecBuilder yields the RestoreTestSpec for ONE run, given the archive that was picked. +// +// R-85 (1.1): this REPLACES a frozen spec value. It used to be built by an immediately-invoked +// function at daemon start, so `storageTier()` and `restoreTaskTimeout()` were evaluated ONCE and +// the resulting value reused for every run for the lifetime of the process. Two consequences: +// - nothing tier-varying was expressible at all (the offsite tier could never be scheduled), and +// - it was a latent staleness bug in its own right — a storage-type or config change did not take +// effect until the daemon restarted. +// +// The archive is passed in because the tier MUST be derived from it (the v0.100.0 rule), never from +// the configured target: deriving it from config is what produced the 600 s false failure when a +// PBS archive was classified "local" and got the 10-minute local wait. +type SpecBuilder func(ctx context.Context, archive string) reconcile.RestoreTestSpec + // 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). @@ -25,7 +39,7 @@ type Scheduler struct { runner RestoreTestRunner pick CandidatePicker store *Store - spec reconcile.RestoreTestSpec // archive is filled per-tick + spec SpecBuilder // R-85: evaluated PER RUN, never frozen at construction cadence time.Duration logger *slog.Logger now func() time.Time @@ -33,11 +47,13 @@ type Scheduler struct { // SchedulerOptions configures a Scheduler. type SchedulerOptions struct { - Runner RestoreTestRunner - Pick CandidatePicker - Store *Store - Spec reconcile.RestoreTestSpec // RestoreStorage, ScratchMin/Max, SourceTier, BootTimeout - Cadence time.Duration // 0 → disabled + Runner RestoreTestRunner + Pick CandidatePicker + Store *Store + // Spec builds the run's spec (RestoreStorage, ScratchMin/Max, SourceTier, timeouts) from the + // picked archive. Called ONCE PER RUN — see SpecBuilder for why it is not a value. + Spec SpecBuilder + Cadence time.Duration // 0 → disabled Logger *slog.Logger } @@ -63,7 +79,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 { + if s.cadence <= 0 || s.runner == nil || s.pick == nil || s.spec == nil { s.logger.Info("backup: restore-test cadence disabled") <-ctx.Done() return nil @@ -85,6 +101,13 @@ func (s *Scheduler) Run(ctx context.Context) error { // tick runs one scheduled restore-test: pick a backup → run → record. No-ops cleanly when // no backup exists yet. Deterministic given s.now — tests call it directly. func (s *Scheduler) tick(ctx context.Context) { + if s.spec == nil { + // Defensive: Run() already refuses to start without a SpecBuilder, but tick is also + // reachable directly. Skipping loudly beats panicking the daemon goroutine — a missing + // spec must cost a restore-test, never the agent. + s.logger.Error("backup: restore-test has no spec builder — skipping (this is a wiring bug)") + return + } archive, err := s.pick(ctx) if err != nil { s.logger.Warn("backup: restore-test could not pick a candidate; skipping", "err", err) @@ -94,7 +117,8 @@ func (s *Scheduler) tick(ctx context.Context) { s.logger.Info("backup: restore-test skipped; no backup available yet") return } - spec := s.spec + // R-85: build the spec for THIS run, from THIS archive. Never a frozen value. + spec := s.spec(ctx, archive) spec.Archive = archive res := s.runner.RunRestoreTest(ctx, spec) if res.Skipped { diff --git a/internal/backup/schedule_spec_test.go b/internal/backup/schedule_spec_test.go new file mode 100644 index 0000000..55cfa55 --- /dev/null +++ b/internal/backup/schedule_spec_test.go @@ -0,0 +1,124 @@ +package backup + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/reconcile" +) + +// R-85 (1.1) — the spec is built PER RUN, never frozen at construction. +// +// It used to be an immediately-invoked function at daemon start, so storageTier() and +// restoreTaskTimeout() were evaluated ONCE and the value reused for every run for the process +// lifetime. That is what made an offsite restore-test impossible to schedule at all, and it was a +// latent staleness bug besides: a storage-type or config change did not take effect until restart. + +type specSpy struct { + mu sync.Mutex + calls int + archives []string + tiers []string // what the builder decided, per call +} + +func (sp *specSpy) build(_ context.Context, archive string) reconcile.RestoreTestSpec { + sp.mu.Lock() + defer sp.mu.Unlock() + sp.calls++ + sp.archives = append(sp.archives, archive) + // Decide the tier from the ARCHIVE, exactly as main.go does (the v0.100.0 rule). + tier := "local" + if len(archive) > 10 && archive[:10] == "felhom-pbs" { + tier = "pbs" + } + sp.tiers = append(sp.tiers, tier) + return reconcile.RestoreTestSpec{ + RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009, SourceTier: tier, + } +} + +// COMPANION RED-PROOF (observed): change Scheduler.spec back to a frozen +// `reconcile.RestoreTestSpec` value captured at construction → this fails with +// "the spec builder must run ONCE PER RUN, got 1 call(s) across 3 ticks", because a frozen value is +// evaluated exactly once no matter how many ticks fire. Restored. +func TestScheduler_SpecIsBuiltPerRun(t *testing.T) { + sp := &specSpy{} + rt := &fakeRTRunner{res: reconcile.RestoreTestResult{Pass: true, Verified: "boot+running"}} + n := 0 + s := NewScheduler(SchedulerOptions{ + Runner: rt, + Pick: func(context.Context) (string, error) { + n++ + return fmt.Sprintf("local:backup/vzdump-lxc-9201-%d.tar.zst", n), nil + }, + Store: NewStore(), + Spec: sp.build, + Cadence: time.Hour, + Logger: quiet(), + }) + + for i := 0; i < 3; i++ { + s.tick(context.Background()) + } + + sp.mu.Lock() + defer sp.mu.Unlock() + if sp.calls != 3 { + t.Fatalf("the spec builder must run ONCE PER RUN, got %d call(s) across 3 ticks", sp.calls) + } + // And it must see the archive THIS run picked — not a stale one. + for i, a := range sp.archives { + want := fmt.Sprintf("local:backup/vzdump-lxc-9201-%d.tar.zst", i+1) + if a != want { + t.Fatalf("run %d: builder saw archive %q, want %q — the spec is not tracking the picked archive", i+1, a, want) + } + } +} + +// The tier must follow the ARCHIVE across runs. A builder that saw only the configured target would +// return the same tier every time — which is exactly the v0.100.0 defect that killed a 14.46 GB WAN +// restore at the 10-minute local bound. +func TestScheduler_SpecTierFollowsTheArchive(t *testing.T) { + sp := &specSpy{} + rt := &fakeRTRunner{res: reconcile.RestoreTestResult{Pass: true, Verified: "boot+running"}} + archives := []string{ + "local:backup/vzdump-lxc-9201-x.tar.zst", + "felhom-pbs:backup/ct/9201/2026-07-26T15:42:42Z", + } + i := 0 + s := NewScheduler(SchedulerOptions{ + Runner: rt, + Pick: func(context.Context) (string, error) { + a := archives[i%len(archives)] + i++ + return a, nil + }, + Store: NewStore(), Spec: sp.build, Cadence: time.Hour, Logger: quiet(), + }) + s.tick(context.Background()) + s.tick(context.Background()) + + sp.mu.Lock() + defer sp.mu.Unlock() + if len(sp.tiers) != 2 || sp.tiers[0] != "local" || sp.tiers[1] != "pbs" { + t.Fatalf("the tier must follow the archive per run; got %v", sp.tiers) + } +} + +// A nil spec builder must SKIP loudly, not panic — a wiring bug costs a restore-test, never the +// daemon goroutine. +func TestScheduler_NilSpecSkipsInsteadOfPanicking(t *testing.T) { + rt := &fakeRTRunner{} + s := NewScheduler(SchedulerOptions{ + Runner: rt, + Pick: func(context.Context) (string, error) { return "vol", nil }, + Store: NewStore(), Cadence: time.Hour, Logger: quiet(), + }) + s.tick(context.Background()) // must not panic + if rt.runs != 0 { + t.Fatalf("a nil spec must not run a restore-test; got %d run(s)", rt.runs) + } +}