R-359 + R-397: the off-site store gets checked, and the advertised check becomes real

Nothing ever verified that the off-site copies are still readable. The whole-guest tier has verify jobs; the tier holding the customer's documents and photos had none -- the complete set of restic verbs this controller used contained no `check`. We would have found out at restore time, with a customer waiting. On 2026-08-21 a deliberately damaged pack was caught at once by plain `restic check`; we had never run it. R-397: NotifyIntegrityOK/NotifyIntegrityFailed existed with no caller, the hub allowlists both event types and carries the Hungarian text for both, the settings checkbox exists, and the debug button posts to /api/debug/backup/ integrity. Everything was built except the part that runs. SIXTH instance of that shape in this project. THE HAZARD SHAPES THE WHOLE DESIGN. resticStep self-heals a crash lock by running `unlock --remove-all` and retrying, and its own comment records why that is safe: every caller holds the in-process single-flight mutex, so any lock it meets is stale. A check that did not take that flag could meet a LIVE prune's lock from this same box, remove it, and retry over the top of it. So the check TAKES THE FLAG and SKIPS rather than waits -- waiting would pin the nightly backup behind it, and a skip costs nothing because due-ness makes tomorrow try again. TestR359_SkipsWhenRunningFlagHeld asserts the NON-EFFECTS: restic never invoked, `unlock` never in any argv. Its red-proof prints the real thing -- restic running `check` while the flag was held. DUE-NESS, NOT A WEEKDAY. Daily job, weekly behaviour: "is the last successful check older than 7 days?" not "is it Sunday?". R-341 is exactly the other shape, a dated check quietly missed and never caught up. No Weekly primitive added. THREE OUTCOMES, NOT TWO. Skipped, Unreachable and failed are different facts. "I could not look" is not "I looked and it is broken" -- R-339 already owns reachability, and a second alarm for the same fact trains the operator to discount the one alarm that means the backups are damaged. A timeout is unreachable, never damage. A failure advances due-ness (a broken store must not be re-checked nightly); a skip and an unreachable store do not. Success is severity `info`, which severityNotifies DROPS -- it mails NOBODY, by design. A weekly success e-mail is how people stop reading their alerts. The customer gets a SENTENCE; restic's words go to the log, truncated (R-379: 615 bytes of raw database text reached a customer once). read-data-subset ships OFF and a malformed value is refused at read time rather than handed to restic, where one typo would fail the whole check. Published on OffboxReportStatus, NOT on report.BackupReport's IntegrityOK -- those were retired by R-331 YESTERDAY and TestBackupReport_DeadFieldsStayZero still passes unmodified. Also: the monitoring page stopped promising a Sunday job that never existed, and the debug button got its dispatch case. PART 0 WAS NOT BUILT, AND R-398 WAS MY OWN MISTAKE. The seam it asked for already exists: offboxRunner/SetOffboxRunner/m.runner() has been injectable since the off-site tier shipped, and other tests drive restic-backed paths through it. A resticStepFn seam would have been WORSE here -- it would replace the `unlock --remove-all` escalation and hide it from the assertions that must see it. R-358's AST ordering test is converted to a real execution test instead, which immediately surfaced something the AST walk could not: unlockStale legitimately runs before the restore. Four red-proofs, each printing the pre-fix behaviour. Green gate: 28 packages, rc 0. All 12 controller gates OK.
2026-08-30 21:03:29 +02:00
parent e64c84aef8
commit 0d52a42c17
13 changed files with 1365 additions and 52 deletions
@@ -0,0 +1,261 @@
+package backup
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"log"
+	"strings"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
+)
+
+// ── R-359 — the off-site store was never checked ─────────────────────────────────────────────────
+//
+// The whole-guest tier has verify jobs; the tier holding the customer's documents and photos had none.
+// The complete set of restic verbs this controller used contained no `check` — verified 2026-08-30.
+//
+// These drive the REAL CheckOffboxIntegrity through the EXISTING `offboxRunner` seam, which has been
+// injectable since the off-site tier shipped. (R-398 claimed otherwise and was my own mistake; the
+// seam sees every argv, including the `unlock --remove-all` escalation a `resticStepFn` would have
+// hidden — which is exactly what the lock-safety tests must observe.)
+
+// errFake is a plain non-nil error for seam replies; the classifier reads the OUTPUT, not the error
+// type, so a synthetic error is faithful here.
+var errFake = errors.New("restic exited non-zero")
+
+// integrityCapture records every restic invocation so both the effects and the NON-effects are
+// assertable. `argvs` is the whole point: a test that only checks the verdict cannot tell a check that
+// ran from one that did not.
+type integrityCapture struct {
+	argvs  [][]string
+	reply  func(args []string) ([]byte, error)
+	logBuf *bytes.Buffer
+}
+
+func (c *integrityCapture) runner() offboxRunner {
+	return func(_ context.Context, _ []string, args ...string) ([]byte, error) {
+		c.argvs = append(c.argvs, append([]string{}, args...))
+		if c.reply != nil {
+			return c.reply(args)
+		}
+		return nil, nil
+	}
+}
+
+func (c *integrityCapture) sawVerb(verb string) bool {
+	for _, a := range c.argvs {
+		for _, x := range a {
+			if x == verb {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func (c *integrityCapture) checkArgv() []string {
+	for _, a := range c.argvs {
+		for _, x := range a {
+			if x == "check" {
+				return a
+			}
+		}
+	}
+	return nil
+}
+
+// newIntegrityManager builds a manager with a configured off-site target and a captured runner.
+func newIntegrityManager(t *testing.T, reply func(args []string) ([]byte, error)) (*Manager, *integrityCapture) {
+	t.Helper()
+	m, _ := newOffboxManager(t)
+	cap := &integrityCapture{reply: reply, logBuf: &bytes.Buffer{}}
+	m.logger = log.New(cap.logBuf, "", 0)
+	m.SetOffboxRunner(cap.runner())
+	return m, cap
+}
+
+// okRepo answers `cat config` so ensureOffboxRepo passes, then defers to `then` for everything else.
+func okRepo(then func(args []string) ([]byte, error)) func(args []string) ([]byte, error) {
+	return func(args []string) ([]byte, error) {
+		for _, a := range args {
+			if a == "config" {
+				return []byte(`{"version":2}`), nil
+			}
+		}
+		if then != nil {
+			return then(args)
+		}
+		return nil, nil
+	}
+}
+
+func TestR359_HealthyRepoReportsOK(t *testing.T) {
+	m, cap := newIntegrityManager(t, okRepo(nil))
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if !res.OK || res.Skipped || res.Unreachable {
+		t.Fatalf("a healthy repo did not report OK: %+v", res)
+	}
+	if cap.checkArgv() == nil {
+		t.Fatal("`restic check` was never invoked — the check did not check anything")
+	}
+}
+
+func TestR359_RepositoryErrorReportsFailure(t *testing.T) {
+	// restic's own words from the 2026-08-21 damaged-pack drill.
+	const damaged = "pack 5b1f2c3d: not found in index\nrepository contains errors"
+	m, _ := newIntegrityManager(t, okRepo(func(args []string) ([]byte, error) {
+		return []byte(damaged), errFake
+	}))
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if res.OK {
+		t.Fatal("a repository restic said contains errors was reported as OK — this is the defect the " +
+			"whole feature exists to prevent")
+	}
+	if res.Unreachable {
+		t.Fatal("readable-and-damaged was misclassified as unreachable — those are different facts, " +
+			"and only one of them means the customer's backups are broken")
+	}
+	if !strings.Contains(res.Output, "not found in index") {
+		t.Errorf("restic's own words must reach the LOG so the operator can diagnose; got %q", res.Output)
+	}
+}
+
+func TestR359_UnreachableIsNotAnIntegrityFailure(t *testing.T) {
+	// The repo cannot even be opened. "I could not look" is not "I looked and it is broken".
+	m, _ := newIntegrityManager(t, func(args []string) ([]byte, error) {
+		return []byte("ssh: connect to host nas.local port 22: Connection refused"), errors.New("exit 1")
+	})
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if res.OK {
+		t.Fatal("an unreachable repository was reported as a passing check")
+	}
+	if !res.Unreachable {
+		t.Fatal("an unreachable repository was reported as DAMAGE — that would alarm the customer that " +
+			"their backups are corrupt when nothing was ever looked at, and R-339 already owns reachability")
+	}
+}
+
+func TestR359_TimeoutIsNotDamage(t *testing.T) {
+	m, _ := newIntegrityManager(t, okRepo(func(args []string) ([]byte, error) {
+		return nil, context.DeadlineExceeded
+	}))
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // an already-dead context: the check cannot finish
+	res := m.CheckOffboxIntegrity(ctx)
+
+	if res.OK {
+		t.Fatal("a check that never finished reported OK")
+	}
+	if !res.Unreachable {
+		t.Fatalf("a check that timed out was reported as damage: %+v — it saw nothing, so it may not "+
+			"claim the store is broken", res)
+	}
+}
+
+func TestR359_StructureCheckPassesNoReadDataFlag(t *testing.T) {
+	m, cap := newIntegrityManager(t, okRepo(nil))
+	m.CheckOffboxIntegrity(context.Background())
+
+	argv := cap.checkArgv()
+	if argv == nil {
+		t.Fatal("no check ran")
+	}
+	for _, a := range argv {
+		if strings.HasPrefix(a, "--read-data") {
+			t.Fatalf("the DEFAULT check downloaded pack data (%q) — that is a bandwidth cost nobody "+
+				"chose, and R-399 exists precisely so it is not chosen here", a)
+		}
+	}
+}
+
+func TestR359_ReadDataSubsetIsPassedWhenConfigured(t *testing.T) {
+	m, cap := newIntegrityManager(t, okRepo(nil))
+	m.cfg.Monitoring.Integrity.ReadDataSubset = "5%"
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if res.ReadDataSubset != "5%" {
+		t.Errorf("result did not record the depth it ran at: %+v", res)
+	}
+	var found bool
+	for _, a := range cap.checkArgv() {
+		if a == "--read-data-subset=5%" {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("the configured subset did not reach restic; argv=%v", cap.checkArgv())
+	}
+}
+
+func TestR359_MalformedReadDataSubsetIsTreatedAsOff(t *testing.T) {
+	m, cap := newIntegrityManager(t, okRepo(nil))
+	m.cfg.Monitoring.Integrity.ReadDataSubset = "banana"
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	for _, a := range cap.checkArgv() {
+		if strings.HasPrefix(a, "--read-data") {
+			t.Fatalf("a malformed value was handed to restic (%q) — restic rejects it and the WHOLE "+
+				"check fails, so one typo silently stops the store being verified at all", a)
+		}
+	}
+	if res.ReadDataSubset != "" {
+		t.Errorf("a refused value was still recorded as the depth: %q", res.ReadDataSubset)
+	}
+	if !strings.Contains(cap.logBuf.String(), "WARN") {
+		t.Error("a refused config value must say so — silence makes a typo indistinguishable from a " +
+			"deliberate structure-only setting")
+	}
+}
+
+func TestR359_MessageNeverCarriesResticOutputOrCredentials(t *testing.T) {
+	// R-379: 615 bytes of raw database text reached a customer once. And offboxBaseArgs builds the repo
+	// as `sftp:<user>@<host>:<path>`, so a raw passthrough leaks the credential shape too.
+	const secretish = "sftp:felhom@nas.local:/srv/repo pack 5b1f2c3d corrupt"
+	m, _ := newIntegrityManager(t, okRepo(func(args []string) ([]byte, error) {
+		return []byte(secretish), errFake
+	}))
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if res.OK {
+		t.Fatal("fixture wrong: this should be a failure")
+	}
+	// The customer sentence is a CONSTANT and contains none of it. Asserted here rather than only in
+	// the web package because this is where the output is captured.
+	for _, bad := range []string{"sftp:", "nas.local", "5b1f2c3d", "felhom@"} {
+		if strings.Contains(integrityFailedCustomerSentence, bad) {
+			t.Fatalf("the customer-facing failure sentence carries %q", bad)
+		}
+	}
+	// ...while the operator's log DOES get it, or the fault cannot be diagnosed without a rebuild.
+	if !strings.Contains(res.Output, "5b1f2c3d") {
+		t.Error("restic's output did not reach the result for the log")
+	}
+}
+
+// integrityFailedCustomerSentence mirrors the constant in cmd/controller. Duplicated deliberately and
+// narrowly: this package cannot import main, and the property under test is that the SENTENCE carries
+// no machine detail — a property of the words themselves.
+const integrityFailedCustomerSentence = "A távoli mentés ellenőrzése hibát talált a tárolóban. A mentések egy része sérült lehet. Ne törölj semmit, és vedd fel velünk a kapcsolatot."
+
+func TestR359_NoTargetConfiguredIsASilentSkip(t *testing.T) {
+	m, cap := newIntegrityManager(t, okRepo(nil))
+	if err := m.settings.SetOffboxTarget(&settings.OffboxTarget{Enabled: false}); err != nil {
+		t.Fatal(err)
+	}
+	res := m.CheckOffboxIntegrity(context.Background())
+
+	if !res.Skipped {
+		t.Fatalf("a box with no off-site tier did not skip: %+v", res)
+	}
+	if len(cap.argvs) != 0 {
+		t.Fatalf("restic ran on a box with no off-site target: %v", cap.argvs)
+	}
+	if res.OK {
+		t.Fatal("a skip was reported as a passing check — nothing was checked")
+	}
+}