admin/felhom-agent
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

R-85 Phase 1: the restore-test spec is built PER RUN, not frozen at daemon start

Browse Source 
SchedulerOptions.Spec was a VALUE produced by an immediately-invoked function
at daemon start, so storageTier() and restoreTaskTimeout() were evaluated once
and reused for every run for the process lifetime. Nothing tier-varying was
expressible (the offsite tier could never be scheduled), and it was a latent
staleness bug besides: a storage-type or config change did not take effect
until restart.

- backup.SpecBuilder: func(ctx, archive) RestoreTestSpec, called once per run.
  The archive is passed because the tier MUST come from it (v0.100.0 rule) —
  config-derived 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: a wiring bug must cost a
  restore-test, never the daemon goroutine.

Red-proof observed. Full suite green (29 packages, rc=0).
This commit is contained in:
Claude Code
2026-07-26 20:47:28 +02:00
parent edde8a01ca
commit 765d8b3168
5 changed files with 204 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+28
View File
@@ -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
cmd/felhom-agent/main.go
+14 -3
View File
@@ -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,
})
internal/backup/backup_test.go
+6 -3
View File
@@ -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(),
})
internal/backup/schedule.go
+32 -8
View File
@@ -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 {
internal/backup/schedule_spec_test.go
+124
View File
@@ -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)
}
}