R-86: restore-test follows the backup, not the clock (v0.121.0)
gates / gates (push) Failing after 7s
gates / gates (push) Failing after 7s
The ticker survives as the EVALUATION interval only. A tier is DUE when its
newest archive that has settled for `settle` (default 24h) has not been proven:
daily tier -> proved daily on yesterday's archive, weekly tier -> weekly on its
own, newborn -> UNKNOWN.
The trap avoided: the literal reading ("newest archive is >= 24h old") is NEVER
true on a daily tier, so it silently switches restore-testing off where it
matters most. Red-proved at 0 runs over 5 simulated days.
- state records WHICH archive was proven; legacy files keep their time and yield
no proven archive (each tier due once after the upgrade, deliberately)
- two knobs replace one: restore_test_eval_interval_seconds (6h, measured) and
restore_test_settle_seconds (24h). The old cadence key keeps its DISABLE
meaning verbatim and now seeds the settle lag, with a start-up WARN.
- due-check runs BEFORE the heavy-op gate (a frequent poll must not make a
starting backup record a failure, F-A1)
- candidate picker skips implausible archives (a phantom would be due forever)
- new read-only --selftest=restore-test-due prints the verdict + its cost
This commit is contained in:
+107
-59
@@ -32,22 +32,31 @@ 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)
|
||||
// TierPicker resolves the newest archive on a NAMED tier that landed AT OR BEFORE notAfter (the
|
||||
// settle cutoff), together with when it landed. (*BackupRunner).PickSettledRestoreCandidateOn
|
||||
// satisfies it. A zero notAfter means "no settle requirement".
|
||||
//
|
||||
// R-86 widened this seam from (target) → archive. The landing time is what makes the due-check's
|
||||
// verdict explainable — "archive X, which landed at T, has not been proven" — and the cutoff is
|
||||
// what makes the rule per-ARCHIVE-GENERATION instead of per-interval. "" must NOT be an error: a
|
||||
// brand-new offsite tier legitimately has nothing to restore yet.
|
||||
type TierPicker func(ctx context.Context, target string, notAfter time.Time) (archive string, landed time.Time, err 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).
|
||||
type Scheduler struct {
|
||||
runner RestoreTestRunner
|
||||
pick CandidatePicker
|
||||
store *Store
|
||||
spec SpecBuilder // R-85: evaluated PER RUN, never frozen at construction
|
||||
runner RestoreTestRunner
|
||||
pick CandidatePicker
|
||||
store *Store
|
||||
spec SpecBuilder // R-85: evaluated PER RUN, never frozen at construction
|
||||
// cadence is the EVALUATION interval (R-86) — how often "is anything due?" is asked. It is no
|
||||
// longer the thing that decides a test happens; see restoretest_due.go.
|
||||
cadence time.Duration
|
||||
logger *slog.Logger
|
||||
now func() time.Time
|
||||
// settle is how long an archive must have sat before it is a candidate (R-86).
|
||||
settle 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.
|
||||
@@ -64,9 +73,14 @@ type SchedulerOptions struct {
|
||||
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
|
||||
Spec SpecBuilder
|
||||
// Cadence is the EVALUATION interval — how often due-ness is asked, NOT how often a test runs
|
||||
// (R-86). 0 → disabled.
|
||||
Cadence time.Duration
|
||||
// Settle is how long an archive must have sat before it is a restore-test candidate (R-86).
|
||||
// 0 → no settle requirement (any archive is a candidate).
|
||||
Settle time.Duration
|
||||
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
|
||||
@@ -89,6 +103,7 @@ func NewScheduler(opts SchedulerOptions) *Scheduler {
|
||||
store: opts.Store,
|
||||
spec: opts.Spec,
|
||||
cadence: opts.Cadence,
|
||||
settle: opts.Settle,
|
||||
logger: logger,
|
||||
now: func() time.Time { return time.Now().UTC() },
|
||||
tiers: append([]string(nil), opts.Tiers...),
|
||||
@@ -98,9 +113,19 @@ func NewScheduler(opts SchedulerOptions) *Scheduler {
|
||||
}
|
||||
}
|
||||
|
||||
// Run fires a restore-test on the cadence until ctx is cancelled. A 0 cadence disables it
|
||||
// (the goroutine just waits for shutdown). It does NOT fire immediately on start (a restore
|
||||
// is heavy; the first runs one interval in) — on-demand runs use the selftest harness.
|
||||
// Run EVALUATES due-ness on the interval until ctx is cancelled, and runs a restore-test only when
|
||||
// a tier is actually due (R-86). A 0 interval disables it (the goroutine just waits for shutdown).
|
||||
//
|
||||
// The ticker survives as the evaluation interval and nothing else. It is emphatically NOT the
|
||||
// trigger any more: its phase is the process's uptime, and agent deploys reset it, which is exactly
|
||||
// the defect R-86 removes. What decides that a test happens is `EvaluateDue`.
|
||||
//
|
||||
// It still does NOT evaluate immediately on start — the first evaluation is one interval in. That
|
||||
// is an EARNED restraint, kept deliberately: a restore is heavy, agent restarts are routine, and a
|
||||
// crash-loop that evaluated at start would hammer a permanently-failing tier as fast as it could
|
||||
// restart. Due-ness does not expire while we wait, so the only cost is up to one interval of
|
||||
// latency on a tier that just became due. On-demand runs use `--selftest=restore-test`.
|
||||
//
|
||||
// Returns nil on ctx cancellation.
|
||||
func (s *Scheduler) Run(ctx context.Context) error {
|
||||
if s.cadence <= 0 || s.runner == nil || s.spec == nil || (s.pick == nil && !s.rotating()) {
|
||||
@@ -108,7 +133,8 @@ func (s *Scheduler) Run(ctx context.Context) error {
|
||||
<-ctx.Done()
|
||||
return nil
|
||||
}
|
||||
s.logger.Info("backup: restore-test scheduler starting", "cadence", s.cadence)
|
||||
s.logger.Info("backup: restore-test scheduler starting (per-archive due-check)",
|
||||
"eval_interval", s.cadence, "settle", s.settle)
|
||||
t := time.NewTicker(s.cadence)
|
||||
defer t.Stop()
|
||||
for {
|
||||
@@ -122,8 +148,12 @@ 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.
|
||||
// tick is ONE EVALUATION: gate → due-check → run the first due tier → record which archive was
|
||||
// proven. No-ops cleanly when nothing is due, when no backup exists yet, or when a heavy operation
|
||||
// is already in flight. Deterministic given s.now — tests call it directly.
|
||||
//
|
||||
// One run per evaluation, by construction (Scenario F): a second due tier is left DUE and picked up
|
||||
// by the next evaluation. Deferred, never cancelled, and never two multi-GB restores over one link.
|
||||
func (s *Scheduler) tick(ctx context.Context) {
|
||||
if s.spec == nil {
|
||||
// Defensive: Run() already refuses to start without a SpecBuilder, but tick is also
|
||||
@@ -132,27 +162,40 @@ func (s *Scheduler) tick(ctx context.Context) {
|
||||
s.logger.Error("backup: restore-test has no spec builder — skipping (this is a wiring bug)")
|
||||
return
|
||||
}
|
||||
// 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()
|
||||
|
||||
// The due-check runs BEFORE the gate is taken, and that ORDER is load-bearing under R-86.
|
||||
//
|
||||
// It used to be the other way round, and correctly so: the gate was held for one heavy run a
|
||||
// day, and the candidate lookup rode along inside it. Evaluations are now frequent, and the
|
||||
// lookup is a storage listing that for the offsite tier crosses the WAN. Holding the
|
||||
// one-heavy-operation gate for a read that answers "nothing to do" would open a small window at
|
||||
// EVERY evaluation in which a starting backup cannot acquire — and a backup that cannot acquire
|
||||
// does not merely wait, it records a failure and pages the operator (F-A1). A cheap poll must
|
||||
// not be able to manufacture that.
|
||||
//
|
||||
// Nothing is lost by checking first: due-ness does not expire, and the gate is still taken
|
||||
// before anything heavy begins.
|
||||
archive, target, err := s.pickForThisRun(ctx)
|
||||
if err != nil {
|
||||
s.logger.Warn("backup: restore-test could not pick a candidate; skipping", "err", err)
|
||||
return
|
||||
}
|
||||
if archive == "" {
|
||||
s.logger.Info("backup: restore-test skipped; no backup available yet")
|
||||
s.logger.Debug("backup: restore-test not due this evaluation")
|
||||
return
|
||||
}
|
||||
|
||||
// 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. A deferred tier stays DUE, so the next evaluation retries it.
|
||||
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, "target", target, "archive", archive)
|
||||
return
|
||||
}
|
||||
defer release()
|
||||
// R-85: build the spec for THIS run, from THIS archive. Never a frozen value.
|
||||
spec := s.spec(ctx, archive)
|
||||
spec.Archive = archive
|
||||
@@ -165,8 +208,10 @@ func (s *Scheduler) tick(ctx context.Context) {
|
||||
// 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)
|
||||
// R-86: the ARCHIVE is recorded, not merely the time — that is what makes the tier
|
||||
// not-due until a NEWER archive settles, and what makes a proof survive a restart.
|
||||
if err := s.rtState.RecordSuccess(target, archive, s.now()); err != nil {
|
||||
s.logger.Warn("backup: could not persist the restore-test proof state", "target", target, "err", err)
|
||||
}
|
||||
}
|
||||
switch {
|
||||
@@ -189,46 +234,49 @@ func (s *Scheduler) tick(ctx context.Context) {
|
||||
// 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.
|
||||
// pickForThisRun chooses the tier to test THIS evaluation: the first DUE tier, in oldest-proven
|
||||
// order.
|
||||
//
|
||||
// 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`.
|
||||
// R-86 changed what this answers. It used to answer "whose turn is it?", and the answer was always
|
||||
// somebody's — the ticker had fired, so a test was going to happen. It now answers "is anything
|
||||
// due?", and "nothing" is a normal, frequent and correct answer.
|
||||
//
|
||||
// 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.
|
||||
// OLDEST-FIRST (operator ruling 2026-07-26, Option 1) survives as the ORDER among due tiers: the
|
||||
// tier whose last successful restore-test is oldest goes first, never-proven first of all. It is
|
||||
// self-balancing, needs no config knob, and it still cannot starve a tier — but it no longer decides
|
||||
// that a test happens at all.
|
||||
//
|
||||
// Returns ("", "", nil) when nothing anywhere is testable.
|
||||
// A tier with no settled archive is SKIPPED, not failed — a brand-new offsite tier has nothing to
|
||||
// restore yet, and that is normal, not broken. A tier whose archives cannot be LISTED is likewise
|
||||
// skipped, loudly, and its error is returned only when no other tier was testable: one tier's
|
||||
// storage being unreadable must not cost the other tier its proof, and must not be silent either.
|
||||
//
|
||||
// Returns ("", "", nil) when nothing anywhere is due.
|
||||
func (s *Scheduler) pickForThisRun(ctx context.Context) (archive, target string, err error) {
|
||||
if !s.rotating() {
|
||||
// Pre-R-85 single-tier path (tests and any caller that wires only `Pick`): there is no tier
|
||||
// identity and no persisted proof here, so there is nothing to compare an archive against
|
||||
// and no due-check is possible. It runs on every evaluation, exactly as it always did.
|
||||
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)
|
||||
return a, "", perr
|
||||
}
|
||||
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.
|
||||
for _, v := range s.EvaluateDue(ctx) {
|
||||
if v.Err != nil {
|
||||
s.logger.Warn("backup: restore-test candidate lookup failed for a tier; trying the next",
|
||||
"target", t, "err", perr)
|
||||
"target", v.Target, "err", v.Err)
|
||||
if firstErr == nil {
|
||||
firstErr = perr
|
||||
firstErr = v.Err
|
||||
}
|
||||
continue
|
||||
}
|
||||
if a == "" {
|
||||
s.logger.Debug("backup: restore-test tier has no archive yet; trying the next", "target", t)
|
||||
if !v.Due {
|
||||
s.logger.Debug("backup: restore-test tier is not due", "target", v.Target, "reason", v.Reason)
|
||||
continue
|
||||
}
|
||||
s.logger.Info("backup: restore-test tier selected (oldest-proven first)", "target", t, "archive", a)
|
||||
return a, t, nil
|
||||
s.logger.Info("backup: restore-test tier is DUE (per-archive; oldest-proven first among due tiers)",
|
||||
"target", v.Target, "archive", v.Archive, "landed", v.Landed.Format(time.RFC3339), "reason", v.Reason)
|
||||
return v.Archive, v.Target, nil
|
||||
}
|
||||
if firstErr != nil {
|
||||
return "", "", firstErr
|
||||
|
||||
Reference in New Issue
Block a user