From f21e7caed1f39aa5ba5b1cdc5362ac6c831e5b73 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 3 Aug 2026 13:54:02 +0200 Subject: [PATCH] =?UTF-8?q?hub=20v0.90.1=20=E2=80=94=20the=20digest's=20pe?= =?UTF-8?q?r-app=20lines=20stop=20repeating=20the=20filesystem=20figures?= =?UTF-8?q?=20(R-182)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by reading the first REAL digest, not by design. Every app row ended with the same usage clause the mail already prints once on its own Filesystem line. On a two-app box that is untidy; down a list of a dozen it is the same forty characters twelve times, pushing the part that DIFFERS off a phone screen at 07:00 — the only moment this mail has to work. The reserve's refusal message is authored for a single-app alert where naming the filesystem is right, so the message is unchanged; the digest trims the duplicate when rendering. trimRepeatedUsage removes ONLY an exact "— :" suffix, so an unrelated reason is untouched and a reason that is nothing but the usage clause is left alone rather than emptied. Also updates TestRecoveryUnitCaptureFailed_NeverReachesTheCustomer, which required the OPERATOR to be emailed a per-app capture failure. That was correct when the event was the only signal and is wrong now that it is the record and the digest is the notification. Its customer-safety claim is unchanged and is why the test still exists; the operator assertion is inverted with the reasoning written in place, and R-158's guarantee is shown to have MOVED, not weakened. --- hub/internal/notify/backup_run_digest_test.go | 35 ++++++++++++++++++ .../notify/recovery_unit_dispatch_test.go | 36 ++++++++++++------- hub/internal/notify/templates.go | 26 +++++++++++++- manifests/hub.yaml | 2 +- 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/hub/internal/notify/backup_run_digest_test.go b/hub/internal/notify/backup_run_digest_test.go index ea5729a..fe01497 100644 --- a/hub/internal/notify/backup_run_digest_test.go +++ b/hub/internal/notify/backup_run_digest_test.go @@ -264,3 +264,38 @@ func TestPerAppFailure_IsRecordedButNotMailed(t *testing.T) { "anywhere is the measured defect of 2026-08-03", len(seen), len(apps), seen) } } + +// The per-app reason must not repeat the filesystem figures the digest already prints once. Reviewed +// as copy against the first real digest, not designed in the abstract. +func TestDigestEmail_ReasonDoesNotRepeatTheUsageLine(t *testing.T) { + reason := "refused: below the reserve (reserve: 97% used or 1.0 GiB free) — /mnt/sys_drive: 65.0/68.7 GB used (95%), 0.2 GB free" + details := `{"run_id":"r","run_kind":"nightly","failed":1,"attempted":2,"target_path":"/mnt/sys_drive",` + + `"used_gb":65,"avail_gb":0.2,"total_gb":68.7,"used_percent":95,"space_known":true,` + + `"apps":[{"app":"opengist","leg":"whole app","reason":"` + reason + `"}]}` + _, body := FormatOperatorEmail("demo-hp", "backup_run_failures", "error", "1 of 2 failed", details) + + // The figures appear ONCE, on the Filesystem line — not again on every app row. + if strings.Count(body, "65.0/68.7 GB used") != 1 { + t.Fatalf("the usage clause appears %d times; it must appear once, on its own line — repeated "+ + "down a list of a dozen apps it pushes the part that DIFFERS off a phone screen:\n%s", + strings.Count(body, "65.0/68.7 GB used"), body) + } + // But the reason itself survives — trimming must not eat the diagnosis. + if !strings.Contains(body, "below the reserve") { + t.Fatalf("the reason was trimmed away entirely:\n%s", body) + } +} + +// A reason naming a DIFFERENT path, or none, must be left completely alone. +func TestTrimRepeatedUsage_LeavesUnrelatedReasonsAlone(t *testing.T) { + for _, c := range []struct{ reason, target string }{ + {"pg_dump: connection refused", "/mnt/sys_drive"}, + {"tar failed — /mnt/other: 1/2 GB used (50%), 1 GB free", "/mnt/sys_drive"}, + {"boom", ""}, + {"", "/mnt/sys_drive"}, + } { + if got := trimRepeatedUsage(c.reason, c.target); got != c.reason { + t.Errorf("reason %q (target %q) was altered to %q", c.reason, c.target, got) + } + } +} diff --git a/hub/internal/notify/recovery_unit_dispatch_test.go b/hub/internal/notify/recovery_unit_dispatch_test.go index 0dd4573..e70fd4a 100644 --- a/hub/internal/notify/recovery_unit_dispatch_test.go +++ b/hub/internal/notify/recovery_unit_dispatch_test.go @@ -39,33 +39,45 @@ func TestRecoveryUnitCaptureFailed_NeverReachesTheCustomer(t *testing.T) { } } - // The operator must still get it: the register mutes the customer channel, not the signal. - gotOperator := false + // R-182 CHANGED WHAT THIS ASSERTS, DELIBERATELY, AND THE OLD ASSERTION IS WORTH KEEPING IN VIEW. + // + // Until 2026-08-03 this test required the OPERATOR to be e-mailed here, on the grounds that "the + // alert is the whole point of R-158". That was right when this event was the only signal, and it + // is wrong now: measured, nine of these arrived at the hub and two were mailed, because the + // operator cooldown key carries no app identifier — so as an alarm it told the operator about one + // app and threw the rest away. + // + // The type is now RECORD-ONLY: written down every time, never mailed. R-158's guarantee — the + // operator learns WHICH app failed and WHY — is not weakened, it MOVED: the per-run digest + // `backup_run_failures` carries every failed app in one mail, and is pinned by + // backup_run_digest_test.go. The customer safety claim below is untouched and is the reason this + // test still exists. for _, to := range rec.to { if to == "operator@felhom.eu" { - gotOperator = true + t.Fatal("the operator was e-mailed a PER-APP capture failure — this type is the record " + + "now, not the alarm. One mail per failing app on a full disk is a dozen mails, which " + + "is the volume problem the operator ruled against; the digest is the notification") } } - if !gotOperator { - t.Fatal("the operator was not notified of a recovery-unit capture failure — the alert is the " + - "whole point of R-158 and it went nowhere") - } - // The skip must be VISIBLE. An absent log row is equally consistent with "correctly skipped" and - // "the dispatcher never ran" — the positive observable is the row itself (standing rule 3). + // The RECORD must exist, always. It is what makes the digest trustworthy: if the digest is ever + // lost, delayed or suppressed, the failures are still individually written down. An absent row is + // equally consistent with "correctly not mailed" and "the dispatcher never ran" — the positive + // observable is the row itself (standing rule 3). logs, err := st.GetRecentNotifications("c1", 20) if err != nil { t.Fatalf("GetRecentNotifications: %v", err) } found := false for _, l := range logs { - if l.Channel == "customer" && l.Status == "skipped" && strings.Contains(l.ErrorMessage, "operator_only") { + if l.EventType == "recovery_unit_capture_failed" && l.Status == "recorded" && + strings.Contains(l.ErrorMessage, "record-only") { found = true } } if !found { - t.Fatalf("the customer skip is not logged as skipped/operator_only — it is indistinguishable "+ - "from a delivery that never happened; got %d row(s)", len(logs)) + t.Fatalf("the per-app failure left no 'recorded' row — a failure that is neither mailed nor "+ + "written down is exactly the 2026-08-03 defect, rebuilt; got %d row(s)", len(logs)) } } diff --git a/hub/internal/notify/templates.go b/hub/internal/notify/templates.go index 38fe3a1..e413e83 100644 --- a/hub/internal/notify/templates.go +++ b/hub/internal/notify/templates.go @@ -361,7 +361,7 @@ func renderBackupRunFailures(customerID, detailsJSON string) (string, string, bo var b strings.Builder fmt.Fprintf(&b, "\n\nFAILED: %d of %d apps attempted in this %s run.\n\n", d.Failed, d.Attempted, kind) for _, a := range d.Apps { - reason := a.Reason + reason := trimRepeatedUsage(a.Reason, d.TargetPath) if reason == "" { reason = "(no reason recorded)" } @@ -382,3 +382,27 @@ func renderBackupRunFailures(customerID, detailsJSON string) (string, string, bo b.WriteString("whether or not this mail was sent.") return b.String(), subject, true } + +// trimRepeatedUsage strips the trailing "— /path: X/Y GB used (Z%), W GB free" clause from a per-app +// reason, because the digest prints those figures ONCE for the whole run on its own line. +// +// This is a copy fix, and it was made after reading the first real digest rather than from the +// design. The reserve's refusal message is authored for a single-app alert, where naming the +// filesystem is exactly right; repeated down a list of a dozen apps it is the same forty characters +// twelve times, and it pushes the part that differs off the right-hand edge of a phone screen at +// 07:00 — which is the only moment this mail has to work. +// +// It trims ONLY an exact "— :" suffix, so a reason that mentions a different path, or +// none, is left completely alone. A reason that is nothing but the usage clause is left alone too: +// removing everything would turn a bad line into an empty one. +func trimRepeatedUsage(reason, targetPath string) string { + if reason == "" || targetPath == "" { + return reason + } + marker := " — " + targetPath + ":" + i := strings.LastIndex(reason, marker) + if i <= 0 { + return reason + } + return strings.TrimSpace(reason[:i]) +} diff --git a/manifests/hub.yaml b/manifests/hub.yaml index 573fc2b..224f9a3 100644 --- a/manifests/hub.yaml +++ b/manifests/hub.yaml @@ -125,7 +125,7 @@ spec: spec: containers: - name: hub - image: gitea.dooplex.hu/admin/felhom-hub:0.90.0 + image: gitea.dooplex.hu/admin/felhom-hub:0.90.1 ports: - containerPort: 8080 name: http