v0.110.0: offbox stale-lock self-heal (campaign C2) + crash-truthful status (C1)

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6

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.

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))
 	}

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)