v0.191.0 — warn before the wall comes down (R-167, R-158, R-174)

R-167: new internal/fillwatch warns the CUSTOMER before a filesystem fills. It emits the PRE-EXISTING disk_warning/disk_critical pair, which was allowlisted, copy'd, default-enabled and checkbox'd with no producer in any repo — the sixth "built but never wired" instance here. Two threshold terms (85% or 5 GiB free; critical 95%/2 GiB) because a percentage alone lies at both ends of this fleet's size range. Edge-triggered on escalation only, state persisted, hysteresis dead zone at 75%/7 GiB pinned by a test. A nil usage read is never a warning and never clears one. Per filesystem, never per app. Daily 03:30, before the nightly app-data legs. R-158: new unitNotify seam fires per app when a Tier-1 recovery-unit capture fails, loop continuing, carrying the target filesystem's used/free bytes. Operator-tier (recovery_unit_capture_failed) — deliberately NOT backup_failed, which is customer-enabled and would email the customer about a failure they cannot act on. D-c overrides R-158's own proposal here. R-174: the app-stop guard no longer starts apps onto MISSING drives — a regression in v0.189.0 code, found by review and closed the same session. SetStarter got the raw stack manager, whose StartStack has no drive gate, and Recover runs at startup. R-171 one path over. bootDriveGate could not be reused whole (its holder #2 is the guard's own marker, and holders #1/#2 read vars assigned after Recover runs), so holder #3 is extracted into a shared driveStartGate with a test pinning the delegation. ErrStartRefused splits a refusal from a failure: both keep the marker, only Failed alarms, because routing a deliberate hold into NotifyBackupFailed is the same false alarm. Tests 1157 -> 1184. All red-proofs demonstrated failing and restored.
2026-08-02 23:18:51 +02:00
parent 95eb5c2c1a
commit cf48214f6c
12 changed files with 1785 additions and 22 deletions
@@ -0,0 +1,229 @@
+package backup
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"log"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// R-174 — the app-stop guard's crash recovery must not start an app onto a MISSING drive.
+//
+// The defect these pin, found by review on 2026-08-02 in code shipped 2026-08-01 (v0.189.0):
+// `appStopGuard.SetStarter(stackMgr)` handed Recover the raw stack manager, whose `StartStack` has
+// no drive gate. Recover runs at STARTUP — exactly when an external drive may not have come back —
+// so a backup that stopped an app, followed by a power cut and a drive that did not remount, ended
+// with the app started onto a missing drive. R-171 one path over.
+//
+// THE SEAM UNDER TEST IS THE STARTER, not the gate: `internal/backup` must not import `stacks` or
+// `settings`, so the production gate lives in `cmd/controller`. What is pinned here is the contract
+// between them — that a starter returning ErrStartRefused produces a REFUSAL (marker kept, no alarm)
+// and not a FAILURE. The production wiring itself is pinned by TestMainWiresGatedAppStopStarter.
+
+// gatingStarter is a starter whose gate refuses a named set of apps, in the shape the production
+// `gatedAppStopStarter` uses: refuse BEFORE calling through, and wrap ErrStartRefused with a reason.
+type gatingStarter struct {
+	inner   *fakeStarter
+	refuse  map[string]string // app → reason
+	refused []string
+}
+
+func (s *gatingStarter) StartStack(name string) error {
+	if why, ok := s.refuse[name]; ok {
+		s.refused = append(s.refused, name)
+		return fmt.Errorf("%w: %s", ErrStartRefused, why)
+	}
+	return s.inner.StartStack(name)
+}
+
+func newGatedGuard(t *testing.T, dir string, refuse map[string]string) (*AppStopGuard, *gatingStarter) {
+	t.Helper()
+	s := &gatingStarter{inner: &fakeStarter{}, refuse: refuse}
+	g := NewAppStopGuard(filepath.Join(dir, "appstop-state.json"), log.New(io.Discard, "", 0))
+	g.SetStarter(s)
+	return g, s
+}
+
+// --- Scenario A — the guard does not start an app onto a missing drive ---------------------------
+
+func TestRecover_DriveAbsent_RefusesTheStartAndKEEPSTheMarker(t *testing.T) {
+	dir := t.TempDir()
+
+	// process 1: a volume dump stops immich, then the box loses power. No End(), no defer.
+	g1, _ := newGatedGuard(t, dir, nil)
+	if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil {
+		t.Fatalf("Begin: %v", err)
+	}
+	// <power cut> — and immich's drive does NOT come back.
+
+	// process 2: a fresh controller starts. The drive is absent.
+	g2, starter := newGatedGuard(t, dir, map[string]string{
+		"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint",
+	})
+	res := g2.Recover()
+
+	if len(starter.inner.starts) != 0 {
+		t.Fatalf("started %v — the app was started onto a MISSING drive, which is the whole defect",
+			starter.inner.starts)
+	}
+	if res == nil {
+		t.Fatal("Recover returned nil — the refusal is invisible to the caller, so nothing can report it")
+	}
+	if len(res.Refused) != 1 || res.Refused[0] != "immich" {
+		t.Fatalf("refused=%v, want [immich]", res.Refused)
+	}
+	if len(res.Failed) != 0 {
+		t.Fatalf("failed=%v — a deliberate hold was recorded as a FAILURE. That bucket reaches "+
+			"NotifyBackupFailed, which is customer-enabled by default, so the customer would be "+
+			"emailed \"A biztonsági mentés sikertelen!\" about an app nothing is wrong with (R-171's "+
+			"false-alarm shape one path over)", res.Failed)
+	}
+	if !markerExists(t, dir) {
+		t.Fatal("the marker was CLEARED after a refused start — the operation is genuinely " +
+			"unfinished, and clearing it erases the only durable record that immich is owed a restart")
+	}
+	// The refusal must name the app AND the reason, or an operator cannot act on it.
+	if d := res.Detail(); !strings.Contains(d, "held_by_drive") || !strings.Contains(d, "immich") {
+		t.Fatalf("detail %q does not name the held app", d)
+	}
+	if msg := res.Message(); !strings.Contains(msg, "HELD") || !strings.Contains(msg, "drive") {
+		t.Fatalf("operator message %q does not say the app is held by an absent drive", msg)
+	}
+}
+
+// A refusal-only recovery MUST NOT alarm. This is the assertion that keeps the fix from being the
+// bug it fixes: the drive gate doing its job is not a backup failure.
+func TestRecover_RefusalOnly_IsNotAlarming(t *testing.T) {
+	dir := t.TempDir()
+	g1, _ := newGatedGuard(t, dir, nil)
+	if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil {
+		t.Fatal(err)
+	}
+	g2, _ := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"})
+	res := g2.Recover()
+
+	if res.Alarming() {
+		t.Fatal("a recovery that only REFUSED starts reports as alarming — main.go would push it " +
+			"through NotifyBackupFailed and email the customer about a working drive gate")
+	}
+}
+
+// A genuine failure alongside a refusal still alarms, and the two stay in different buckets.
+func TestRecover_FailureAlongsideRefusal_StillAlarmsAndKeepsThemApart(t *testing.T) {
+	dir := t.TempDir()
+	g1, _ := newGatedGuard(t, dir, nil)
+	if err := g1.Begin("volume-dump:batch", ReasonVolumeDump, []string{"immich", "nextcloud", "homebox"}); err != nil {
+		t.Fatal(err)
+	}
+
+	g2, starter := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"})
+	starter.inner.failWith = map[string]error{"nextcloud": errors.New("compose up: no such image")}
+	res := g2.Recover()
+
+	if len(res.Refused) != 1 || res.Refused[0] != "immich" {
+		t.Fatalf("refused=%v, want [immich]", res.Refused)
+	}
+	if len(res.Failed) != 1 || res.Failed[0] != "nextcloud" {
+		t.Fatalf("failed=%v, want [nextcloud]", res.Failed)
+	}
+	if len(res.Restarted) != 1 || res.Restarted[0] != "homebox" {
+		t.Fatalf("restarted=%v, want [homebox] — neither a refusal nor a failure may abort the loop",
+			res.Restarted)
+	}
+	if !res.Alarming() {
+		t.Fatal("a genuine restart FAILURE alongside a refusal no longer alarms — the refusal " +
+			"swallowed a real fault")
+	}
+	if !markerExists(t, dir) {
+		t.Fatal("the marker was cleared with work still owed")
+	}
+	// The message must not let the held app inflate the failure count.
+	msg := res.Message()
+	if !strings.Contains(msg, "1 of 2 app(s) could NOT be restarted") {
+		t.Fatalf("operator message %q miscounts: the held app must not be counted as a failure", msg)
+	}
+	if !strings.Contains(msg, "not counted as failures") {
+		t.Fatalf("operator message %q does not disclose the held app at all", msg)
+	}
+}
+
+// --- Scenario B — a live drive still recovers normally, byte-identical to before -----------------
+
+func TestRecover_DriveLive_RecoversExactlyAsBefore(t *testing.T) {
+	dir := t.TempDir()
+	g1, _ := newGatedGuard(t, dir, nil)
+	if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich", "nextcloud"}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Nothing refused — the gate says yes for both.
+	g2, starter := newGatedGuard(t, dir, nil)
+	res := g2.Recover()
+
+	if len(starter.inner.starts) != 2 {
+		t.Fatalf("started %v, want both apps — the new gate refused a LEGITIMATE recovery",
+			starter.inner.starts)
+	}
+	if len(res.Refused) != 0 || len(res.Failed) != 0 {
+		t.Fatalf("refused=%v failed=%v, want neither on a live drive", res.Refused, res.Failed)
+	}
+	if len(res.Restarted) != 2 {
+		t.Fatalf("restarted=%v, want both", res.Restarted)
+	}
+	if markerExists(t, dir) {
+		t.Fatal("the marker survived a fully successful recovery — the next boot would restart the apps again")
+	}
+	if !res.Alarming() {
+		t.Fatal("a successful recovery no longer reports to the operator — the interrupted operation " +
+			"itself is what §2.4 wants reported, and it went silent")
+	}
+}
+
+// The next startup, with the drive back, completes the recovery and clears the marker. This is what
+// makes "keep the marker" a recovery rather than a leak.
+func TestRecover_HeldAppIsRestartedOnceTheDriveReturns(t *testing.T) {
+	dir := t.TempDir()
+	g1, _ := newGatedGuard(t, dir, nil)
+	if err := g1.Begin("volume-dump:immich", ReasonVolumeDump, []string{"immich"}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Boot 1 — drive absent: refused, marker kept.
+	g2, _ := newGatedGuard(t, dir, map[string]string{"immich": "drive /mnt/felhom-drives/hdd_1 is not a live mountpoint"})
+	if res := g2.Recover(); len(res.Refused) != 1 {
+		t.Fatalf("boot 1 refused=%v, want [immich]", res.Refused)
+	}
+	if !markerExists(t, dir) {
+		t.Fatal("boot 1 cleared the marker — boot 2 has nothing to act on and immich stays down forever")
+	}
+
+	// Boot 2 — the drive is back.
+	g3, starter := newGatedGuard(t, dir, nil)
+	res := g3.Recover()
+	if len(starter.inner.starts) != 1 || starter.inner.starts[0] != "immich" {
+		t.Fatalf("boot 2 started %v, want [immich] — the held app was never picked up again",
+			starter.inner.starts)
+	}
+	if len(res.Restarted) != 1 {
+		t.Fatalf("boot 2 restarted=%v, want [immich]", res.Restarted)
+	}
+	if markerExists(t, dir) {
+		t.Fatal("boot 2 kept the marker after a fully successful recovery")
+	}
+}
+
+// ErrStartRefused must be matched with errors.Is, i.e. it survives wrapping. A starter that returns
+// a bare string reason would land in Failed and alarm — the exact collapse this type prevents.
+func TestErrStartRefused_SurvivesWrapping(t *testing.T) {
+	err := fmt.Errorf("%w: drive /mnt/felhom-drives/hdd_1 is not a live mountpoint", ErrStartRefused)
+	if !errors.Is(err, ErrStartRefused) {
+		t.Fatal("a wrapped ErrStartRefused is no longer matched by errors.Is — every refusal would " +
+			"be recorded as a restart failure and alarm the customer")
+	}
+	if errors.Is(errors.New("compose up: no such image"), ErrStartRefused) {
+		t.Fatal("an ordinary restart failure matches ErrStartRefused — real faults would go silent")
+	}
+}