R-893: hold the app after ANY failure once the definition or a volume moved; hold persisted before the stop; run_job done says it ran, not what it found (security review)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS

controller/internal/backup/offbox_reconstitute.go

@@ -501,6 +501,23 @@ func (m *Manager) captureLiveDefinition(stack string) (*liveDefinition, error) {
 // held with HoldReasonRestoreMixed. The operator is notified with no rollback error — that is how the
 // notification tells this case from R-379's double failure.
 func (m *Manager) holdAppAfterMixedRestore(stack string, replayErr error, live *liveDefinition) {
+	// The hold is PERSISTED FIRST (security review 2026-10-08): a controller that dies between the stop and
+	// the hold would otherwise restart the app from the R-166 marker with nothing refusing it. When the hold
+	// cannot be persisted, the app-stop marker is KEPT, so nothing reads the window as finished.
+	held := false
+	if m.settings == nil {
+		m.logger.Printf("[ERROR] [offbox] %s: cannot persist the restore hold — no settings wired; the app is stopped but NOTHING will refuse a restart", stack)
+	} else {
+		h := settings.RestoreHold{Stack: stack, At: time.Now().UTC().Format(time.RFC3339), Reason: settings.HoldReasonRestoreMixed}
+		if replayErr != nil {
+			h.ReplayError = replayErr.Error()
+		}
+		if err := m.settings.SetRestoreHold(h); err != nil {
+			m.logger.Printf("[ERROR] [offbox] %s: persisting the restore hold FAILED: %v — the app is stopped and the app-stop marker is kept", stack, err)
+		} else {
+			held = true
+		}
+	}
 	if live != nil {
 		if err := m.stackProvider.RecreateStackDefinitionFromUnit(stack, live.dir, live.env); err != nil {
 			m.logger.Printf("[ERROR] [offbox] %s: writing the live definition back FAILED: %v — the app is held at the snapshot's definition", stack, err)
@@ -509,21 +526,10 @@ func (m *Manager) holdAppAfterMixedRestore(stack string, replayErr error, live *
 		}
 	}
 	if err := m.stackProvider.StopStack(stack); err != nil {
-		m.logger.Printf("[WARN] [offbox] %s: stopping the database service before the hold failed: %v", stack, err)
+		m.logger.Printf("[WARN] [offbox] %s: stopping the app before the hold failed: %v", stack, err)
 	}
-	m.logger.Printf("[ERROR] [offbox] %s: database rolled back, but files/volumes/version had already moved — HOLDING the app stopped for support (R-893); replay error was: %v", stack, replayErr)
-	if m.settings == nil {
-		m.logger.Printf("[ERROR] [offbox] %s: cannot persist the restore hold — no settings wired; the app is stopped but NOTHING will refuse a restart", stack)
-		return
-	}
-	h := settings.RestoreHold{Stack: stack, At: time.Now().UTC().Format(time.RFC3339), Reason: settings.HoldReasonRestoreMixed}
-	if replayErr != nil {
-		h.ReplayError = replayErr.Error()
-	}
-	if err := m.settings.SetRestoreHold(h); err != nil {
-		m.logger.Printf("[ERROR] [offbox] %s: persisting the restore hold FAILED: %v — the app is stopped and unguarded", stack, err)
-	}
-	if m.appStop != nil {
+	m.logger.Printf("[ERROR] [offbox] %s: the restore stopped half-way after files/volumes/version had moved — HOLDING the app stopped for support (R-893); error was: %v", stack, replayErr)
+	if held && m.appStop != nil {
 		m.appStop.End()
 	}
 	if m.restoreHoldNotify != nil {
@@ -935,8 +941,13 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack
 		}
 		n, cErr := copier(pl.src, pl.dst)
 		if cErr != nil {
-			// Best-effort bring-up: leaving the app stopped after a partial copy would turn a failed
-			// restore into an outage.
+			// R-893 option C (security review 2026-10-08): once the snapshot's (possibly older) definition
+			// is written, a start would run that version on the live data — HOLD the app instead.
+			if defineFromSnapshot {
+				m.holdAppAfterMixedRestore(stack, cErr, liveDef)
+				return res, util.MsgError("err.backup.restore_failed_held_mixed", stack)
+			}
+			// Nothing moved yet: best-effort bring-up — a failed restore must not also be an outage.
 			if sErr := restartStack(); sErr != nil {
 				m.logger.Printf("[WARN] [offbox] %s: restart after failed placement also failed: %v", stack, sErr)
 			}
@@ -961,8 +972,16 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack
 	nVols, vErr := volReplay(stack, filepath.Join(scratchUnit, "volume-dumps"))
 	res.VolumesReplayed = nVols
 	if vErr != nil {
-		// A partial replay must never read as a completion. Bring the app back up rather than leaving
-		// an outage, then surface it — the same shape the file leg above uses.
+		// A partial replay must never read as a completion. R-893 (security review 2026-10-08): a volume
+		// replay that failed may have removed or half-replaced a volume, and the definition and files may
+		// already be the snapshot's — HOLD the app rather than start it on a mix.
+		// R-893 option C (security review 2026-10-08): a volume already replaced, or the snapshot's
+		// definition written → HOLD (the volume detail reaches the operator through the hold notice).
+		if defineFromSnapshot || nVols > 0 {
+			m.holdAppAfterMixedRestore(stack, vErr, liveDef)
+			return res, util.MsgError("err.backup.restore_failed_held_mixed", stack)
+		}
+		// Nothing replaced yet: bring the app back up rather than leaving an outage, then surface it.
 		if sErr := restartStack(); sErr != nil {
 			m.logger.Printf("[WARN] [offbox] %s: restart after failed volume replay also failed: %v", stack, sErr)
 		}
@@ -977,6 +996,11 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack
 	// under ON_ERROR_STOP=1 (H4). Starting only the database service closes that window entirely.
 	if hasDB {
 		if err := m.stackProvider.StartStackServices(stack, dbServices); err != nil {
+			// R-893 option C (security review 2026-10-08): volumes or the definition already moved → HOLD.
+			if defineFromSnapshot || res.VolumesReplayed > 0 {
+				m.holdAppAfterMixedRestore(stack, err, liveDef)
+				return res, util.MsgError("err.backup.restore_failed_held_mixed", stack)
+			}
 			// Best-effort bring-up: a failed restore must not also be an outage.
 			if sErr := restartStack(); sErr != nil {
 				m.logger.Printf("[WARN] [offbox] %s: full start after failed DB-only start also failed: %v", stack, sErr)
@@ -1018,6 +1042,8 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack
 			// files and volumes (and maybe its older version) under the newer database. Write the
 			// live definition back, stop the database service again, and HOLD the app for support.
 			// Pinned by r893_mixed_restore_test.go; the plain case keeps TestR379_ScenarioA.
+			// Placed FILES alone do not hold (option C as ruled): that mix is what „put back exactly as it
+			// was" (option A, the next slice) removes.
 			if defineFromSnapshot || res.VolumesReplayed > 0 {
 				m.holdAppAfterMixedRestore(stack, iErr, liveDef)
 				return res, util.MsgError("err.backup.db_restore_failed_held_mixed", stack)

controller/internal/backup/r354_volume_replay_test.go

@@ -190,24 +190,33 @@ func TestR354_ScenarioD_LiveRecoveryUnitIsNeverWritten(t *testing.T) {
 }
 
 // TestR354_ScenarioE_PartialReplayIsAFailure — a volume replay that fails must never read as a
-// completion, and must name what failed.
+// completion, and must name what failed (to the operator).
+//
+// UPDATED 2026-10-08 (R-893 option C, `09` §3 decision 192): one volume HAS been replaced, so a start
+// would run the app on a mix of the snapshot's volume and the live rest. The app is now HELD stopped
+// for support instead of brought up; the volume that did not come back is named in the operator's
+// hold notice (the household reads the plain hold sentence).
 func TestR354_ScenarioE_PartialReplayIsReportedAsFailure(t *testing.T) {
 	m, prov, _ := reconFixture(t, "20260719T060000Z", "2026-07-19T06:00:00Z", pgDump(1))
 	seedScratchVolumes(t, m, "immich", "immich_a.tar", "immich_b.tar")
 	m.volumeReplayFrom = func(_, _ string) (int, error) {
 		return 1, fmt.Errorf("failed to restore 1 volume(s): [immich_b]")
 	}
+	var noticed error
+	m.SetRestoreHoldNotify(func(_ string, replayErr, _ error) { noticed = replayErr })
 
 	res, err := m.ReconstituteFromOffsite(context.Background(), "immich", false)
 	if err == nil {
 		t.Fatal("a partial volume replay must be reported as a failure, not a completion")
 	}
-	if !strings.Contains(err.Error(), "immich_b") {
-		t.Errorf("the failure must name the volume that did not come back; got %q", err.Error())
+	if noticed == nil || !strings.Contains(noticed.Error(), "immich_b") {
+		t.Errorf("the operator's hold notice must name the volume that did not come back; got %v", noticed)
 	}
-	// Best-effort bring-up: a failed restore must not also be an outage.
-	if !prov.fullStarted {
-		t.Error("the app was left stopped after a failed volume replay")
+	if prov.fullStarted {
+		t.Error("the app was STARTED on a partly replaced set of volumes — it must be held (R-893)")
+	}
+	if held, _ := m.RestoreHoldFor("immich"); !held {
+		t.Error("no restore hold was left after a partial volume replay")
 	}
 	// The count of what DID come back is still carried, so the report can say "1 of 2".
 	if res.VolumesReplayed != 1 {

controller/internal/backup/r893_mixed_restore_test.go

@@ -31,7 +31,15 @@ import (
 // r893Provider records EVERY definition write (vtReconProvider keeps only the last).
 type r893Provider struct {
 	*vtReconProvider
-	defs [][]string
+	defs   [][]string
+	onStop func()
+}
+
+func (p *r893Provider) StopStack(name string) error {
+	if p.onStop != nil {
+		p.onStop()
+	}
+	return p.vtReconProvider.StopStack(name)
 }
 
 func (p *r893Provider) RecreateStackDefinitionFromUnit(name, composeDir string, env map[string]string) error {
@@ -133,3 +141,45 @@ func TestR893_AReplacedVolumeThenAFailedReplayHoldsTheApp(t *testing.T) {
 		t.Fatalf("no version changed, yet a definition was written: %v", vp.defs)
 	}
 }
+
+// Security review 2026-10-08 (G1): the hold covers EVERY failure after the snapshot's definition is written, not only a
+// failed replay. Here the DB-only start fails after a version change: before the fix the app was started at the
+// snapshot's (older) definition on the live database. Now the live definition is written back and the app is held.
+// RED-PROOF: restore `restartStack()` as the only action in the StartStackServices failure branch → started → FAILS.
+func TestR893_AVersionChangeThenAFailedDBStartHoldsTheApp(t *testing.T) {
+	m, vp, rolled, notified := r893Fixture(t, true, 0)
+	vp.startSvcErr = context.DeadlineExceeded
+	_, err := m.ReconstituteFromOffsite(context.Background(), "immich", false)
+	if err == nil {
+		t.Fatal("a failed DB-only start must be surfaced")
+	}
+	if vp.fullStarted {
+		t.Fatalf("the app was STARTED at the snapshot's definition on the live data — calls %v", vp.calls)
+	}
+	if held, _ := m.RestoreHoldFor("immich"); !held {
+		t.Fatal("no hold was left after a failed DB-only start that followed a version change")
+	}
+	if len(vp.defs) != 2 || !strings.Contains(strings.Join(vp.defs[1], " "), "postgres:18-alpine") {
+		t.Fatalf("definition writes %v — want the snapshot's, then the LIVE one written back", vp.defs)
+	}
+	if *rolled != 0 || *notified != 1 {
+		t.Errorf("rollback ran %d (want 0: nothing was replayed), notified %d (want 1)", *rolled, *notified)
+	}
+}
+
+// Security review 2026-10-08 (G3): the hold is persisted BEFORE the app is stopped, so a controller that dies in
+// between cannot restart the app from its app-stop marker with nothing refusing it. The provider's StopStack asserts
+// the hold is already on disk at the moment it is called.
+// RED-PROOF: move SetRestoreHold after StopStack in holdAppAfterMixedRestore → FAILS.
+func TestR893_HoldIsPersistedBeforeTheStop(t *testing.T) {
+	m, vp, _, _ := r893Fixture(t, true, 0)
+	seenAtStop := []bool{}
+	vp.onStop = func() {
+		held, _ := m.RestoreHoldFor("immich")
+		seenAtStop = append(seenAtStop, held)
+	}
+	_, _ = m.ReconstituteFromOffsite(context.Background(), "immich", false)
+	if len(seenAtStop) == 0 || !seenAtStop[len(seenAtStop)-1] {
+		t.Fatalf("the last stop ran before the hold was persisted (held at each stop: %v)", seenAtStop)
+	}
+}