v0.190.0 — the boot settle window, both gates on intent, and R-171

R-171 (a regression v0.189.0 introduced, CONFIRMED on hardware before any fix was written). Replacing isBootOrphan's container-count term with recorded intent made a drive-gate-stopped app read as a boot orphan: the gate stops apps with `compose down` (zero containers) and never touches desired_state, because it is not the customer. Observed on 9201 with the drive held unmounted — the sweep found and started it, burned both attempts, and handed it to the dead-app alarm. The write hazard did not materialise (the unbound mountpoint is host-root-owned and the guest is unprivileged) but that protection is accidental and untested. New consumer-side seam bootrecon.StartGate, fail-safe (cannot determine ⇒ do not start), wired in main.go. The rule is not new: the API's startGatedByMissingDrive already refuses this; the sweep bypassed it. R-157 mechanism A. The sweep looked once at T+5s, deriving candidates from a fleet docker was still restoring — three of six hard resets. Now a settle-then- sweep window: sample every 5s, settled after 3 identical samples, sweep ONCE at the end; ends on settled or a 50s budget, and the log says which. The budget is 50s because settle+budget+one retry must stay under the 90s dead-app grace — a test rejected 60s at 95s. A window that overruns emits a LATE RECOVERY warn rather than the grace being widened to hide it. Widening the window made two more holders reachable, so the one gate covers all three: an absent drive, a quiesce, and an in-flight app-data operation — reusing quiesce.SuppressedStacks() and a new read-only AppStopGuard.HeldStacks(). R-170. shouldRecreateOnBoot now reads desired_state with the identical three-way table; absent keeps the old hasContainers behaviour exactly. Its comment argued for the container count and was rewritten. presentStable is untouched. The two gates' agreement is pinned from both sides against one fixture table. 27/27 packages green; 6 red-proofs observed FAIL then restored.
2026-08-02 19:56:20 +02:00
parent 3446609420
commit 582135f861
13 changed files with 1272 additions and 43 deletions
@@ -41,6 +41,45 @@ type StackProvider interface {
 	RefreshStatus() error
 }

+// StartGate answers the one question this package must ask before starting anything: **may this app
+// be started right now?** Declared consumer-side, in the style of StackProvider, so `bootrecon`
+// still imports `stacks` alone and knows nothing about settings, the agent, quiesce or the web layer.
+//
+// It is ONE seam rather than three because the three reasons a boot orphan must NOT be started share
+// a shape — something else is deliberately holding this app — and differ only in the reason string:
+//
+//	the drive-absent gate stopped it     → its drive is not live (R-171, below)
+//	a quiesce is holding it for a backup → the quiesce loop restarts its own stacks
+//	an app-data operation stopped it     → the app-stop guard's own Recover owns it
+//
+// A widened boot window (R-157 mechanism A) is what makes the last two reachable at all: the old
+// T+5 s single sweep never overlapped them.
+//
+// R-171 — WHY THIS EXISTS, and it is a regression this package caused. Until v0.189.0 the sweep
+// required a stack to still HAVE containers, and an app the drive-absent gate had stopped has zero,
+// so such apps were skipped by accident. v0.189.0 replaced that term with the customer's recorded
+// intent — correctly — and the drive gate does NOT change `desired_state` (it is not the customer),
+// so a gate-stopped app now reads as `running` + zero containers, i.e. a boot orphan. Observed live
+// on 2026-08-02: the sweep found and started an app whose drive was unmounted, burned both attempts,
+// and handed it to the dead-app alarm — a false alarm about an app the drive gate is deliberately
+// holding (audits/DIAG-bootrecon-drive-absent-2026-08-02.md).
+//
+// The rule itself is not new and is not invented here: the API's own start path already refuses this
+// (`startGatedByMissingDrive`, internal/api/router.go) with a Hungarian message to the customer. The
+// sweep simply bypassed it by calling Manager.StartStack directly. This seam gives the sweep the
+// same question to ask.
+//
+// CONTRACT — the answer is fail-safe by design (§8.4): an implementation that CANNOT DETERMINE
+// whether the drive is live must return false, not true. Not starting is recoverable — the drive
+// gate's `Return` branch restarts the app when the drive comes back, and the dead-app alarm reports
+// it meanwhile. Starting on an absent drive is not recoverable by anything automatic: compose
+// creates the bind sources wherever the mountpoint currently points, which is the guest rootfs.
+type StartGate interface {
+	// MayStart reports whether the named stack may be started. The reason is for the log line and is
+	// only read when may is false.
+	MayStart(stackName string) (may bool, reason string)
+}
+
 const (
 	// DefaultAttempts is the total number of start attempts per boot (not per app per retry-forever).
 	DefaultAttempts = 2
@@ -58,6 +97,13 @@ type Reconciler struct {

 	// sleep is the inter-attempt wait; injectable so tests never spend 30 real seconds.
 	sleep func(context.Context, time.Duration)
+
+	// startGate (R-171) refuses to start an app something else is deliberately holding. nil = NOT
+	// WIRED, which means "this caller has no such concept" and is permissive — the test fixtures'
+	// case. It is NOT the same as "cannot determine", which the gate itself answers with false (see
+	// StartGate's contract). Production MUST wire it; TestMainWiresBootDriveGate walks main.go's AST
+	// for the call, because an unwired seam here is silently the pre-v0.190.0 behaviour.
+	startGate StartGate
 }

 // Result is the outcome, returned for logging/testing (the hub learns about failures only through
@@ -67,6 +113,23 @@ type Result struct {
 	Recovered  []string // running again by the end
 	StillDown  []string // still down after the last attempt — the alarm's problem now
 	Attempts   int      // attempts actually made (0 when there was nothing to do)
+	// HeldByDrive (R-171) are apps that ARE boot orphans by intent but which something else is
+	// deliberately holding (an absent drive, a quiesce, an app-data operation), so they were not
+	// started. Reported separately from StillDown because they are not a fault this sweep failed to
+	// fix — the holder owns their recovery. Collapsing the two would put a deliberately-held app in
+	// the same bucket as a broken one, which is the false alarm R-171 removes.
+	HeldByDrive []string
+}
+
+// SetDriveGate wires the R-171 start refusal. INIT-ONLY — call once, before Run.
+func (r *Reconciler) SetDriveGate(g StartGate) { r.startGate = g }
+
+// mayStart asks the gate, or allows when none is wired (see the startGate field comment).
+func (r *Reconciler) mayStart(stackName string) (bool, string) {
+	if r.startGate == nil {
+		return true, ""
+	}
+	return r.startGate.MayStart(stackName)
 }

 // New builds a Reconciler with the shipped defaults.
@@ -109,9 +172,9 @@ func sleepCtx(ctx context.Context, d time.Duration) {
 // customer stopped this" and left alone. The safety goal was right and still holds. The SIGNAL was
 // wrong, because zero containers has at least three causes and the count cannot tell them apart:
 //
-//	  a deliberate Stop   → must stay down
-//	  a power cut mid-compose, or an interrupted deploy → must come back
-//	  a backup that stopped the app and died before restarting it → must come back
+//	a deliberate Stop   → must stay down
+//	a power cut mid-compose, or an interrupted deploy → must come back
+//	a backup that stopped the app and died before restarting it → must come back
 //
 // Two of those three were silently unrecoverable: the app simply stayed gone until a human noticed.
 // The count was never capable of separating them, so the fix is not a better inference — it is to
@@ -162,16 +225,33 @@ func (r *Reconciler) Run(ctx context.Context) Result {

 	pending := map[string]bool{}
 	for _, s := range r.stacks.GetStacks() {
-		if isBootOrphan(s) {
-			pending[s.Name] = true
-			res.Candidates = append(res.Candidates, s.Name)
+		if !isBootOrphan(s) {
+			continue
 		}
+		// R-171: intent says this app should be running and it is not — but if something else is
+		// deliberately holding it (absent drive, quiesce, an app-data operation), starting it is the
+		// wrong repair. Refuse, loudly, and let the holder own it.
+		if live, reason := r.mayStart(s.Name); !live {
+			res.HeldByDrive = append(res.HeldByDrive, s.Name)
+			r.logger.Printf("[INFO] [bootrecon] %q is a boot orphan by intent but is HELD (%s) — NOT starting it; whatever is holding it owns its recovery",
+				s.Name, reason)
+			continue
+		}
+		pending[s.Name] = true
+		res.Candidates = append(res.Candidates, s.Name)
 	}
 	sortStrings(res.Candidates)
+	sortStrings(res.HeldByDrive)

 	if len(pending) == 0 {
 		// The healthy path must be observable — "no alarms" and "never ran" have to be
-		// distinguishable in a log (the v0.91.2 lesson).
+		// distinguishable in a log (the v0.91.2 lesson). "Nothing to start" and "everything I found
+		// is held by an absent drive" must be distinguishable too, or the held case reads as healthy.
+		if len(res.HeldByDrive) > 0 {
+			r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: nothing to start — %d app(s) held (absent drive / quiesce / app-data operation): %v",
+				len(res.HeldByDrive), res.HeldByDrive)
+			return res
+		}
 		r.logger.Printf("[INFO] [bootrecon] Boot reconciliation: no boot-orphaned apps (nothing to start)")
 		return res
 	}
@@ -0,0 +1,217 @@
+package bootrecon
+
+import (
+	"context"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/stacks"
+)
+
+// R-171 — the sweep must not start an app whose data drive is absent.
+//
+// This is a REGRESSION TEST for a defect this package caused: v0.189.0 replaced the container-count
+// term with recorded intent, and a drive-gate-stopped app reads as `running` + zero containers, i.e.
+// a boot orphan. Confirmed live on 2026-08-02 (the sweep started it, burned both attempts, and
+// handed it to the dead-app alarm).
+
+// fakeDriveGate answers a scripted liveness verdict per app.
+type fakeDriveGate struct {
+	dead   map[string]string // app → reason it is not live
+	asked  []string
+	unsure map[string]bool // app → the gate cannot determine (must be treated as NOT live)
+}
+
+func (g *fakeDriveGate) MayStart(name string) (bool, string) {
+	g.asked = append(g.asked, name)
+	if r, ok := g.dead[name]; ok {
+		return false, r
+	}
+	if g.unsure[name] {
+		return false, "cannot determine"
+	}
+	return true, ""
+}
+
+// --- Group G — Scenario A / Part 0's finding, as a test ------------------------------------------
+
+func TestReconcile_DriveAbsentApp_IsNeverStarted(t *testing.T) {
+	// The exact live shape: the drive gate stopped it (zero containers), it is still recorded
+	// `running` because the gate is not the customer, and its drive is gone.
+	//
+	// RED-PROOF: delete the `if live, reason := r.driveLive(...)` block from Run and this test fails
+	// with a start count of 1 — which is precisely what was observed on the box before the fix.
+	// Demonstrated in REPORT.md §4.
+	app := withDesired(vanished("calibre-web"), stacks.DesiredStateRunning)
+	f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp}
+	r, _ := newTestReconciler(f)
+	gate := &fakeDriveGate{dead: map[string]string{"calibre-web": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"}}
+	r.SetDriveGate(gate)
+
+	res := r.Run(context.Background())
+
+	if n := f.starts["calibre-web"]; n != 0 {
+		t.Fatalf("an app whose data drive is ABSENT was started %d time(s) — compose would create its "+
+			"bind sources on the guest rootfs, which is the hazard the drive gate exists to prevent", n)
+	}
+	if len(res.Candidates) != 0 {
+		t.Fatalf("a drive-held app was listed as a start candidate: %v", res.Candidates)
+	}
+	if len(res.HeldByDrive) != 1 || res.HeldByDrive[0] != "calibre-web" {
+		t.Fatalf("HeldByDrive = %v, want [calibre-web] — a held app must be reported, not silently dropped", res.HeldByDrive)
+	}
+	if len(res.StillDown) != 0 {
+		t.Fatalf("a deliberately-held app was reported as StillDown %v — that is the dead-app alarm's "+
+			"bucket, and putting it there is the false alarm this fix removes", res.StillDown)
+	}
+	if res.Attempts != 0 {
+		t.Fatalf("attempts=%d, want 0 — nothing should have been attempted", res.Attempts)
+	}
+}
+
+func TestReconcile_UndeterminableDrive_IsNotStarted(t *testing.T) {
+	// §8.4's fail-safe direction. "Cannot determine" must behave exactly like "absent": not starting
+	// is recoverable (the drive gate's Return branch owns it); starting on an absent drive is not.
+	app := withDesired(vanished("immich"), stacks.DesiredStateRunning)
+	f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp}
+	r, _ := newTestReconciler(f)
+	r.SetDriveGate(&fakeDriveGate{unsure: map[string]bool{"immich": true}})
+
+	res := r.Run(context.Background())
+
+	if len(f.starts) != 0 {
+		t.Fatalf("an app whose drive liveness could NOT be determined was started: %v — the fail-safe "+
+			"direction is to refuse", f.starts)
+	}
+	if len(res.HeldByDrive) != 1 {
+		t.Fatalf("HeldByDrive = %v, want the undeterminable app held", res.HeldByDrive)
+	}
+}
+
+func TestReconcile_LiveDriveApp_IsStillRecovered(t *testing.T) {
+	// The gate must not become a blanket refusal — an app on a LIVE drive is still the R-157 case
+	// and must still be recovered. Without this, a "fix" that returns false always would pass the
+	// test above and silently disable the whole feature.
+	app := withDesired(vanished("bookstack"), stacks.DesiredStateRunning)
+	f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp}
+	r, _ := newTestReconciler(f)
+	gate := &fakeDriveGate{}
+	r.SetDriveGate(gate)
+
+	res := r.Run(context.Background())
+
+	if f.starts["bookstack"] == 0 {
+		t.Fatal("an app on a LIVE drive was not recovered — the drive gate must refuse absent drives, not all of them")
+	}
+	if len(res.HeldByDrive) != 0 {
+		t.Fatalf("an app on a live drive was reported held: %v", res.HeldByDrive)
+	}
+	if len(gate.asked) != 1 || gate.asked[0] != "bookstack" {
+		t.Fatalf("the gate was asked %v, want exactly [bookstack] — one question per candidate", gate.asked)
+	}
+}
+
+func TestReconcile_DriveGateIsOnlyAskedAboutOrphans(t *testing.T) {
+	// A running app and a customer-stopped app are not candidates, so the gate must never be asked
+	// about them. Asking is not merely wasteful: the production gate reads app.yaml off disk per
+	// call, and a stopped app's drive being absent is not a fault anyone should hear about.
+	running := withDesired(stacks.Stack{
+		Name: "docmost", Deployed: true, State: stacks.StateRunning,
+		Containers: []stacks.ContainerInfo{{Name: "docmost", State: stacks.StateRunning}},
+	}, stacks.DesiredStateRunning)
+	stopped := withDesired(vanished("nextcloud"), stacks.DesiredStateStopped)
+	orphan := withDesired(vanished("immich"), stacks.DesiredStateRunning)
+
+	f := &fakeStacks{list: []stacks.Stack{running, stopped, orphan}, onStart: comesUp}
+	r, _ := newTestReconciler(f)
+	gate := &fakeDriveGate{}
+	r.SetDriveGate(gate)
+	r.Run(context.Background())
+
+	if len(gate.asked) != 1 || gate.asked[0] != "immich" {
+		t.Fatalf("the drive gate was asked about %v, want exactly [immich] — only boot orphans", gate.asked)
+	}
+}
+
+func TestReconcile_NoDriveGateWired_IsPermissive(t *testing.T) {
+	// nil gate = "this caller has no drive concept" (the fixtures' case), NOT "cannot determine".
+	// Production wiring is pinned separately by TestMainWiresBootDriveGate — an unwired gate here
+	// would silently be the pre-v0.190.0 behaviour, which is why that AST test exists.
+	app := withDesired(vanished("immich"), stacks.DesiredStateRunning)
+	f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp}
+	r, _ := newTestReconciler(f)
+
+	r.Run(context.Background())
+
+	if f.starts["immich"] == 0 {
+		t.Fatal("with no drive gate wired the sweep must behave as before — the nil case is permissive")
+	}
+}
+
+// --- Scenario F — the two boot gates agree -------------------------------------------------------
+
+func TestBothBootGatesAgreeOnIntent(t *testing.T) {
+	// R-170 + R-166: isBootOrphan and shouldRecreateOnBoot answer the SAME question — did the
+	// customer want this running? — and until v0.190.0 they answered it with different signals.
+	//
+	// shouldRecreateOnBoot lives in internal/web and cannot be called from here without an import
+	// cycle, so this test pins THIS side of the agreement and its sibling
+	// TestShouldRecreateOnBoot_AgreesWithBootrecon (internal/web) pins the other, against the same
+	// fixture table. Both must be updated together if the table changes.
+	cases := []struct {
+		desired    string
+		containers int
+		wantWanted bool // "the customer wanted this running"
+	}{
+		{stacks.DesiredStateStopped, 0, false},
+		{stacks.DesiredStateStopped, 2, false},
+		{stacks.DesiredStateRunning, 0, true},
+		{stacks.DesiredStateRunning, 2, true},
+		{stacks.DesiredStateUnknown, 0, false}, // legacy: zero containers ⇒ treated as stopped
+		{stacks.DesiredStateUnknown, 2, true},  // legacy: containers present ⇒ treated as wanted
+	}
+	for _, c := range cases {
+		s := stacks.Stack{
+			Name: "app", Deployed: true, State: stacks.StateExited,
+			Containers: make([]stacks.ContainerInfo, c.containers),
+			AppConfig:  &stacks.AppConfig{Deployed: true, DesiredState: c.desired},
+		}
+		if got := isBootOrphan(s); got != c.wantWanted {
+			t.Fatalf("isBootOrphan(desired=%q containers=%d) = %v, want %v — the two boot gates must "+
+				"answer the intent question identically", c.desired, c.containers, got, c.wantWanted)
+		}
+	}
+}
+
+// --- Group F / §8.2 — every holder the widened window can now overlap ----------------------------
+
+func TestReconcile_HeldByAnyHolder_IsNeverStarted(t *testing.T) {
+	// §8.2's table, one case per row that the gate is responsible for. The reasons differ; the
+	// required behaviour is identical, which is why they share one seam.
+	//
+	// The first two rows only became reachable when R-157 mechanism A widened the boot window — the
+	// old T+5 s single sweep never overlapped a quiesce or a running app-data operation. Widening the
+	// window without these would have traded a fixed bug for two new ones.
+	for _, reason := range []string{
+		"a whole-guest backup (quiesce) is holding it — the quiesce loop restarts its own stacks",
+		"an app-data operation is holding it — the app-stop guard restarts it when the operation ends",
+		"drive /mnt/felhom-drives/hdd_1 is not a live mountpoint",
+	} {
+		app := withDesired(vanished("immich"), stacks.DesiredStateRunning)
+		f := &fakeStacks{list: []stacks.Stack{app}, onStart: comesUp}
+		r, _ := newTestReconciler(f)
+		r.SetDriveGate(&fakeDriveGate{dead: map[string]string{"immich": reason}})
+
+		res := r.Run(context.Background())
+
+		if len(f.starts) != 0 {
+			t.Fatalf("held by %q but started anyway: %v", reason, f.starts)
+		}
+		if len(res.HeldByDrive) != 1 {
+			t.Fatalf("held by %q but not reported as held: %+v", reason, res)
+		}
+		if len(res.StillDown) != 0 {
+			t.Fatalf("held by %q and reported as StillDown %v — that is the dead-app alarm's bucket",
+				reason, res.StillDown)
+		}
+	}
+}