From cb9e992c7a85002e4b6f2e5ba25e1c252c082511 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 10 Jul 2026 07:02:23 +0200 Subject: [PATCH] v0.110.0: offbox stale-lock self-heal (campaign C2) + crash-truthful status (C1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resticStep escalates a restic lock error to `unlock --remove-all` + one retry (safe: single-writer repo — sub-account isolation + single-flight mutex); plain `unlock` is stale-only and can't clear a crash lock across a container-hostname change. Pre-run stale unlock hygiene on run+restore. C1: NewManager flips a persisted LastStatus=running to a truthful error. Both red-proofed (A reproduces the exact campaign backup failure). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 22 ++++ controller/README.md | 8 ++ controller/internal/backup/backup.go | 24 ++++- controller/internal/backup/offbox.go | 54 ++++++++-- controller/internal/backup/offbox_test.go | 123 ++++++++++++++++++++++ 5 files changed, 224 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3416991..0f686f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ ## Changelog +### v0.110.0 — offbox stale-lock self-heal (campaign C2) + crash-truthful status (C1) (2026-07-10) + +Fixes the overnight campaign's HIGH finding: a crash mid-prune left a restic EXCLUSIVE lock the controller +couldn't clear, failing every subsequent offsite run until manual `restic unlock`. Root nuance from the +evidence: plain `restic unlock` (stale-only) does NOT clear it — the recreated container has a new hostname, +so restic can't verify the dead PID and won't treat the lock as stale for ~30 min. + +- **C2 — `internal/backup`:** `resticStep` wraps the backup/prune/restore restic calls: on a lock error + (`repository is already locked`) it escalates to `unlock --remove-all` and **retries the step ONCE**, + justified by the ARCHITECTURAL single-writer guarantee (one controller per repo via per-customer + sub-account isolation + the in-process single-flight mutex every caller holds → no live sibling). A second + lock failure surfaces the error (never loops). Plus cheap pre-run `unlock` (stale-only) hygiene before + every run + restore. **Boundary (documented):** a DR-cloned second controller writing the same repo would + defeat the single-writer premise — operator-supervised territory. +- **C1 — `NewManager.reconcileCrashedRun`:** on startup, a persisted `LastStatus="running"` (a controller + that died mid-run) flips to `error` + the Hungarian "megszakadt futás (a vezérlő újraindult futás közben)" + — truthful after a crash; the next successful run clears it. +- Tests + red-proofs: self-heal-and-retry (A, **red-proof:** neuter the escalation → the exact campaign + failure `offbox backup rallly: exit status 1` → FAIL); persistent-lock → one `--remove-all` + one retry, + error surfaced, no loop (B); pre-run stale unlock issued every run (C); no lock → `--remove-all` never + fires (E); crash-status flip (D, **red-proof:** drop the flip → status lies "running" → FAIL). + ### v0.109.1 — re-apply must preserve escrow custody + runtime status (live finding) (2026-07-10) Found deploying v0.109.0: including `QuotaGB` in the bridge's descriptor hash triggered a one-time diff --git a/controller/README.md b/controller/README.md index be72d34..1bdebaa 100644 --- a/controller/README.md +++ b/controller/README.md @@ -716,6 +716,14 @@ not just those with HDD data. Non-HDD apps can configure destination, method, an > anywhere) is a **hard error** → `LastStatus="error"` + operator alert (was a misleading `ok`/0 snapshots). > A *partial* run (some units missing) stays `ok` but sets a Hungarian **`LastWarning`** naming the skipped > apps, shown on `/backups`. +> - **Crash-lock self-heal (v0.110.0).** A crash mid-prune leaves a restic EXCLUSIVE lock that plain +> `restic unlock` can't clear (the recreated container's new hostname stops restic proving the dead PID +> stale for ~30 min). Every backup/prune/restore step runs through `resticStep`, which on a lock error +> escalates to `unlock --remove-all` + one retry — safe because the repo has a SINGLE legitimate writer +> (per-customer sub-account isolation + the in-process single-flight mutex). **Boundary:** a DR-cloned +> SECOND controller writing the same repo would defeat this premise — operator-supervised territory, out of +> scope for the auto-heal. A crash mid-run also flips a persisted `LastStatus="running"` to a truthful +> error on the next startup (self-corrects on the next successful run). > - **Secrets** (SSH key + auto-gen repo password) are **0600 files in the data dir** — never logged/committed. > - **Password custody + atomicity (v0.105.0, fork-4; pairs with agent v0.77.0).** The repo password is the > irreplaceable DATA key for the offsite tier, so it rides the **customer-recovery-code (R) escrow** diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 7ab2790..f892496 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -110,12 +110,34 @@ func NewManager(cfg *config.Config, sett *settings.Settings, logger *log.Logger) if cfg.Paths.SystemDataPath == "" { logger.Printf("[WARN] [backup] SystemDataPath is empty in config — SSD-only apps will not have correct backup paths") } - return &Manager{ + m := &Manager{ cfg: cfg, logger: logger, settings: sett, systemDataPath: cfg.Paths.SystemDataPath, } + m.reconcileCrashedRun() + return m +} + +// reconcileCrashedRun makes the persisted offbox status truthful after a crash (campaign C1): a controller +// that died mid-run left LastStatus="running" on disk (the in-memory single-flight mutex is gone with the +// process, but the persisted status keeps lying "running" forever). Flip it to error with a Hungarian +// "interrupted run" message; the next successful run overwrites it. No-op unless a run was actually in +// flight at the crash. +func (m *Manager) reconcileCrashedRun() { + if m.settings == nil { + return + } + t := m.settings.GetOffboxTarget() + if t == nil || t.LastStatus != "running" { + return + } + _ = m.settings.UpdateOffboxStatus(func(o *settings.OffboxTarget) { + o.LastStatus = "error" + o.LastError = "megszakadt futás (a vezérlő újraindult futás közben)" + }) + m.logger.Printf("[WARN] [offbox] previous run was interrupted by a controller restart — marking the last status as failed (self-corrects on the next run)") } // GetAppDrivePath returns the drive path for an app. diff --git a/controller/internal/backup/offbox.go b/controller/internal/backup/offbox.go index 527757d..5821eb8 100644 --- a/controller/internal/backup/offbox.go +++ b/controller/internal/backup/offbox.go @@ -304,6 +304,45 @@ func (m *Manager) offboxBaseArgs(t *settings.OffboxTarget) ([]string, []string) return args, env } +// offboxLockRe matches restic's "already locked" error (both the exclusive and shared forms). +var offboxLockRe = regexp.MustCompile(`repository is already locked`) + +// unlockStale runs `restic unlock` (stale-only) — cheap pre-run hygiene that removes any lock restic can +// itself prove dead/old. Non-fatal (logged at debug). Called before every offbox run/restore. +func (m *Manager) unlockStale(ctx context.Context, base, env []string) { + uctx, cancel := context.WithTimeout(ctx, offboxProbeTimeout) + defer cancel() + if out, err := m.runner()(uctx, env, append(append([]string{}, base...), "unlock")...); err != nil { + m.logger.Printf("[DEBUG] [offbox] pre-run unlock (stale-only) non-fatal: %v: %s", err, truncate(out)) + } +} + +// resticStep runs one restic step (backup/prune/restore) under the offbox single-flight guarantee and +// self-heals the C2 crash lock. On a lock error it escalates to `unlock --remove-all` and retries ONCE, +// because THIS controller is the repo's ONLY legitimate writer — per-customer sub-account isolation gives +// one repo one writer, and the in-process single-flight mutex (held by every caller of this method) proves +// no sibling operation is live. Plain `restic unlock` is stale-ONLY and does NOT clear a crash lock: the +// recreated container has a new hostname, so restic can't verify the dead PID and won't treat the lock as +// stale for ~30 min (the overnight-campaign C2 finding — `unlock --remove-all` is required). A second lock +// failure surfaces the error (never loops). BOUNDARY: a DR-cloned SECOND controller writing the same repo +// would defeat the single-writer premise — that is operator-supervised territory (see README), out of scope. +func (m *Manager) resticStep(ctx context.Context, env, base []string, label string, args ...string) ([]byte, error) { + full := append(append([]string{}, base...), args...) + out, err := m.runner()(ctx, env, full...) + if err == nil || !offboxLockRe.Match(out) { + return out, err + } + m.logger.Printf("[WARN] [offbox] cleared a stale exclusive lock left by a previous crash (single-writer repo) before %s; retrying once", label) + uctx, cancel := context.WithTimeout(ctx, offboxProbeTimeout) + if uout, uerr := m.runner()(uctx, env, append(append([]string{}, base...), "unlock", "--remove-all")...); uerr != nil { + cancel() + m.logger.Printf("[WARN] [offbox] unlock --remove-all failed: %v: %s", uerr, truncate(uout)) + return out, err // surface the original lock error (never loop) + } + cancel() + return m.runner()(ctx, env, full...) // retry exactly ONCE +} + // ensureOffboxRepo makes sure the SFTP repo exists: probe `cat config`; if absent, `init` (idempotent — // a present repo is reused, never re-init). A connect failure surfaces here (fast, via ConnectTimeout). func (m *Manager) ensureOffboxRepo(ctx context.Context, base, env []string) error { @@ -508,6 +547,9 @@ func (m *Manager) runOffboxInternal(ctx context.Context, apps, base, env []strin if rerr := m.ensureOffboxRepo(ctx, base, env); rerr != nil { return 0, nil, rerr // fail fast (dead NAS surfaces here) } + // Pre-run hygiene: clear any lock restic can prove stale before we start (cheap; the --remove-all + // crash-lock escalation lives in resticStep for the locks restic can't self-detect). + m.unlockStale(ctx, base, env) var firstErr error for _, stack := range apps { src, ok := m.discoverOffboxUnit(stack) @@ -517,8 +559,7 @@ func (m *Manager) runOffboxInternal(ctx context.Context, apps, base, env []strin continue } bctx, cancel := context.WithTimeout(ctx, offboxBackupTimeout) - args := append(append([]string{}, base...), "backup", "--tag", "felhom-offbox", "--tag", stack, src) - out, berr := m.runner()(bctx, env, args...) + out, berr := m.resticStep(bctx, env, base, "backup:"+stack, "backup", "--tag", "felhom-offbox", "--tag", stack, src) cancel() if berr != nil { m.logger.Printf("[ERROR] [offbox] backup %s failed: %v: %s", stack, berr, truncate(out)) @@ -534,10 +575,11 @@ func (m *Manager) runOffboxInternal(ctx context.Context, apps, base, env []strin return backedUp, missing, firstErr } // Retention: keep a sane window, prune the rest. Repo-wide (grouped by host+paths by default). + // prune takes an EXCLUSIVE lock — the exact step whose crash left the C2 stale lock — so it goes + // through resticStep for the --remove-all self-heal too. fctx, cancel := context.WithTimeout(ctx, offboxBackupTimeout) defer cancel() - fargs := append(append([]string{}, base...), "forget", "--keep-daily", "7", "--keep-weekly", "4", "--keep-monthly", "6", "--prune") - if out, ferr := m.runner()(fctx, env, fargs...); ferr != nil { + if out, ferr := m.resticStep(fctx, env, base, "prune", "forget", "--keep-daily", "7", "--keep-weekly", "4", "--keep-monthly", "6", "--prune"); ferr != nil { // A prune failure is non-fatal to the backup itself (data is safe) — log, don't fail the run. m.logger.Printf("[WARN] [offbox] forget --prune failed (backups are safe): %v: %s", ferr, truncate(out)) } @@ -673,8 +715,8 @@ func (m *Manager) RestoreOffbox(ctx context.Context, stackName, destDir string) base, env := m.offboxBaseArgs(t) rctx, cancel := context.WithTimeout(ctx, offboxBackupTimeout) defer cancel() - args := append(append([]string{}, base...), "restore", "latest", "--tag", stackName, "--target", destDir) - out, err := m.runner()(rctx, env, args...) + m.unlockStale(rctx, base, env) // pre-restore hygiene (crash-lock self-heal is in resticStep) + out, err := m.resticStep(rctx, env, base, "restore:"+stackName, "restore", "latest", "--tag", stackName, "--target", destDir) if err != nil { return fmt.Errorf("offbox restore %s: %w: %s", stackName, err, truncate(out)) } diff --git a/controller/internal/backup/offbox_test.go b/controller/internal/backup/offbox_test.go index 6227124..7e5e235 100644 --- a/controller/internal/backup/offbox_test.go +++ b/controller/internal/backup/offbox_test.go @@ -459,6 +459,129 @@ func TestApplyOffsiteTarget_PreservesEscrowAndStatusOnReapply(t *testing.T) { } } +// --- C2 stale-lock self-heal + C1 crash-truthful status (overnight campaign findings) --- + +const resticLockErr = "unable to create lock in backend: repository is already locked exclusively by PID 33174 on demo-felhom by root" + +// lockRunner records calls and returns the lock error for `backup` the first `lockTimes` times it's called. +type lockRunner struct { + backups, prunes, unlockStale, unlockRemoveAll int + lockTimes int + seq []string +} + +func (r *lockRunner) run(_ context.Context, _ []string, args ...string) ([]byte, error) { + switch { + case contains(args, "cat") && contains(args, "config"): + return []byte(`{}`), nil + case contains(args, "unlock") && contains(args, "--remove-all"): + r.unlockRemoveAll++ + r.seq = append(r.seq, "unlock-all") + return nil, nil + case contains(args, "unlock"): + r.unlockStale++ + r.seq = append(r.seq, "unlock-stale") + return nil, nil + case contains(args, "backup"): + r.backups++ + r.seq = append(r.seq, "backup") + if r.backups <= r.lockTimes { + return []byte(resticLockErr), errors.New("exit status 1") + } + return nil, nil + case contains(args, "forget"): + r.prunes++ + r.seq = append(r.seq, "prune") + return nil, nil + case contains(args, "snapshots"): + return []byte(`[{"id":"a"}]`), nil + case contains(args, "stats"): + return []byte(`{"total_size":1}`), nil + } + return nil, nil +} + +func newLockManager(t *testing.T, lr *lockRunner) (*Manager, *settings.Settings) { + t.Helper() + m, sett := newOffboxManager(t) + nsRoot := m.AppNamespaceRoot("rallly") + if err := os.MkdirAll(RecoveryUnitPath(nsRoot, "rallly"), 0o755); err != nil { + t.Fatal(err) + } + _ = sett.SetAppOffbox("rallly", true) + m.SetOffboxRunner(lr.run) + return m, sett +} + +// Scenario A — a lock error on backup → unlock --remove-all → retry succeeds → run ok. +func TestOffbox_LockSelfHealsAndRetries(t *testing.T) { + lr := &lockRunner{lockTimes: 1} + m, sett := newLockManager(t, lr) + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatalf("a self-healable lock must recover, got %v", err) + } + if lr.unlockRemoveAll != 1 { + t.Fatalf("exactly one unlock --remove-all expected, got %d", lr.unlockRemoveAll) + } + if lr.backups != 2 { + t.Fatalf("backup must be retried once after the unlock (2 calls), got %d", lr.backups) + } + if lr.unlockStale < 1 { + t.Fatal("the pre-run stale unlock must have been issued (Scenario C)") + } + if got := sett.GetOffboxTarget().LastStatus; got != "ok" { + t.Fatalf("after self-heal the run must be ok, got %q", got) + } +} + +// Scenario B — the lock error persists (twice) → exactly one --remove-all + one retry → error surfaced, +// no unlock loop. +func TestOffbox_LockPersistsSurfacesErrorNoLoop(t *testing.T) { + lr := &lockRunner{lockTimes: 2} + m, sett := newLockManager(t, lr) + if err := m.RunOffboxBackup(context.Background()); err == nil { + t.Fatal("a persistent lock must surface an error") + } + if lr.unlockRemoveAll != 1 { + t.Fatalf("must NOT loop unlocks — exactly 1 --remove-all, got %d", lr.unlockRemoveAll) + } + if lr.backups != 2 { + t.Fatalf("backup attempted exactly twice (original + one retry), got %d", lr.backups) + } + if got := sett.GetOffboxTarget().LastStatus; got != "error" { + t.Fatalf("status must be error, got %q", got) + } +} + +// Scenario E — no lock error → the escalation NEVER fires (unlock --remove-all count stays 0). +func TestOffbox_NoLockNoRemoveAll(t *testing.T) { + lr := &lockRunner{lockTimes: 0} + m, _ := newLockManager(t, lr) + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatal(err) + } + if lr.unlockRemoveAll != 0 { + t.Fatalf("--remove-all must NEVER fire without a lock error, got %d", lr.unlockRemoveAll) + } + if lr.unlockStale < 1 { + t.Fatal("the pre-run stale unlock still runs on a clean run") + } +} + +// Scenario D (C1) — a Manager built while settings say LastStatus="running" (crash mid-run) flips it to a +// truthful error with the Hungarian interrupted message. +func TestOffbox_CrashedRunStatusReconciled(t *testing.T) { + _, sett := newOffboxManager(t) + _ = sett.UpdateOffboxStatus(func(o *settings.OffboxTarget) { o.LastStatus = "running" }) + // a fresh Manager over the same settings = a restart after a crash + m2 := NewManager(&config.Config{}, sett, log.New(os.Stderr, "", 0)) + _ = m2 + got := sett.GetOffboxTarget() + if got.LastStatus != "error" || !strings.Contains(got.LastError, "megszakadt futás") { + t.Fatalf("a crash-interrupted run must become a truthful error, got status=%q err=%q", got.LastStatus, got.LastError) + } +} + // TestOffbox_SingleFlight: an off-box run while another backup holds m.running skips (no runner call). func TestOffbox_SingleFlight(t *testing.T) { m, _ := newOffboxManager(t)