From 0ff1d3c8834797fb6f328d0a80408a574533d56e Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 30 Jun 2026 14:18:45 +0200 Subject: [PATCH] hub v0.24.0: dispatcher routes critical severity (+ nil-prefs crash guard) ProcessEvent routed only warning/error; a critical-severity event was silently dropped. Now routes warning/error/critical, logs unrecognized severities, and guards a nil GetNotificationPrefs (which would panic/crash the hub). host_disk_critical emits its natural critical severity. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01HxLA1mZurFq9kt8hneFeCs --- hub/CHANGELOG.md | 21 ++++++ hub/internal/monitor/host_disk.go | 6 +- hub/internal/monitor/host_disk_test.go | 4 +- hub/internal/notify/dispatcher.go | 41 ++++++++--- hub/internal/notify/dispatcher_test.go | 99 ++++++++++++++++++++++++++ hub/internal/notify/templates.go | 2 +- 6 files changed, 159 insertions(+), 14 deletions(-) create mode 100644 hub/internal/notify/dispatcher_test.go diff --git a/hub/CHANGELOG.md b/hub/CHANGELOG.md index 7ddf48a..f86c4ae 100644 --- a/hub/CHANGELOG.md +++ b/hub/CHANGELOG.md @@ -1,5 +1,26 @@ # Felhom Hub — Changelog +## v0.24.0 — dispatcher routes `critical` severity (+ nil-prefs crash guard) (2026-06-30) + +NAS Part A2's "Part 0": close the dispatcher's silent drop of `critical`-severity events. + +- **`internal/notify/dispatcher.go` `ProcessEvent`:** the severity gate was `severity != "warning" && + severity != "error"` → a `critical` event was **silently dropped** (never emailed). Now routes + **warning / error / critical** (`severityNotifies`); `info` stays an intentional non-notify; any + **unrecognized** severity is **logged** (`[WARN] Dispatcher: unrecognized severity …`), never silently + dropped. Verified safe first: no controller event emits `critical` (all are info/warning/error) and the + hub's only would-be `critical` emitter is `host_disk` — so no surprise alert volume. +- **`internal/monitor/host_disk.go`:** `host_disk_critical` now emits its **natural `critical` severity** + (was forced to `error` to survive the old gate); `FormatOperatorEmail` styles `critical` 🔴 like `error`. +- **Latent crash guard:** `processCustomer` dereferenced `GetNotificationPrefs`, which returns `(nil, nil)` + for a customer with no notification row — an event for such a customer would **panic the dispatcher + goroutine and crash the hub**. Now guards `prefs == nil` before use. +- **Seam:** `sendEmailFn` field (defaults to the Resend sender) so routing is unit-tested without real HTTP. +- Tests: `severityNotifies` (warning/error/critical notify; info/unknown don't) + **companion red-proof** + (the pre-fix `warning||error` predicate drops `critical`); ProcessEvent routes `critical` to the operator; + an unknown severity is logged not dropped; `info` is silent and not mis-logged. `go build/vet/test ./...` + green. + ## v0.23.0 — host root-disk pressure monitoring + alert (2026-06-30) Closes the silent-failure gap behind the felhom-pve incident: a Proxmox host root fs filling up (vzdump diff --git a/hub/internal/monitor/host_disk.go b/hub/internal/monitor/host_disk.go index ba7423d..8d55f6a 100644 --- a/hub/internal/monitor/host_disk.go +++ b/hub/internal/monitor/host_disk.go @@ -167,9 +167,9 @@ func (dc *HostDiskChecker) emit(row store.HostDiskRow, oldBand, newBand string) switch newBand { case bandCritical: eventType = "host_disk_critical" - // NB: the dispatcher only routes severity "warning"/"error" — a "critical" severity would be - // silently dropped — so the critical BAND maps to severity "error" (and the operator email's 🔴). - severity = "error" + // Natural "critical" severity (hub v0.24.0 routes it; the operator email styles it 🔴). Before + // v0.24.0 the dispatcher silently dropped "critical", so this had to be "error" — now it is honest. + severity = "critical" message = fmt.Sprintf("Host %s: root filesystem CRITICALLY full at %.0f%% (threshold %.0f%%) — PVE/logging/agent writes may start failing; free space (e.g. old vzdump backups) immediately", row.HostID, row.DiskPercent, dc.crit) case bandWarning: eventType = "host_disk_warning" diff --git a/hub/internal/monitor/host_disk_test.go b/hub/internal/monitor/host_disk_test.go index 57535c6..339c8cd 100644 --- a/hub/internal/monitor/host_disk_test.go +++ b/hub/internal/monitor/host_disk_test.go @@ -117,8 +117,8 @@ func TestHostDiskChecker_Severity(t *testing.T) { dc.Check() saveDiskReport(t, st, 97) dc.Check() - if len(sev) != 2 || sev[0] != "warning" || sev[1] != "error" { - t.Fatalf("severities = %v, want [warning error] (critical band must be 'error' so the dispatcher routes it)", sev) + if len(sev) != 2 || sev[0] != "warning" || sev[1] != "critical" { + t.Fatalf("severities = %v, want [warning critical] (hub v0.24.0 routes the natural critical severity)", sev) } } diff --git a/hub/internal/notify/dispatcher.go b/hub/internal/notify/dispatcher.go index f99fa95..5f3e455 100644 --- a/hub/internal/notify/dispatcher.go +++ b/hub/internal/notify/dispatcher.go @@ -27,11 +27,15 @@ type Dispatcher struct { mu sync.Mutex opCooldowns map[string]time.Time // "customerID:eventType" → last operator notify custCooldowns map[string]time.Time // "customerID:eventType" → last customer notify + + // sendEmailFn is the email sender, seam-injected so tests exercise routing without real HTTP. + // Defaults to (*Dispatcher).sendEmail (Resend) in NewDispatcher. + sendEmailFn func(to, subject, textBody string) error } // NewDispatcher creates a new notification dispatcher. func NewDispatcher(s *store.Store, resendAPIKey, fromEmail, operatorEmail string, operatorOn bool, logger *log.Logger) *Dispatcher { - return &Dispatcher{ + d := &Dispatcher{ store: s, resendAPIKey: resendAPIKey, fromEmail: fromEmail, @@ -42,6 +46,20 @@ func NewDispatcher(s *store.Store, resendAPIKey, fromEmail, operatorEmail string opCooldowns: make(map[string]time.Time), custCooldowns: make(map[string]time.Time), } + d.sendEmailFn = d.sendEmail + return d +} + +// severityNotifies reports whether a severity triggers email notifications. warning / error / critical +// notify; everything else (info, recovery/status, or an unrecognized value) does not. Pure → unit-tested. +// (Before v0.24.0 a "critical" severity was silently dropped here — the host_disk-class bug.) +func severityNotifies(severity string) bool { + switch severity { + case "warning", "error", "critical": + return true + default: + return false + } } // ProcessEvent evaluates an event and sends notifications as appropriate. @@ -57,8 +75,13 @@ func (d *Dispatcher) ProcessEvent(customerID, eventType, severity, message, deta return } - // Only warning and error severity trigger notifications - if severity != "warning" && severity != "error" { + // warning / error / critical trigger notifications. "info" is an intentional non-notify (status/ + // recovery events). Anything else is UNRECOGNIZED — log it (don't silently drop), so a bad severity + // surfaces instead of vanishing (the felhom-pve-class lesson: a critical event must never be lost). + if !severityNotifies(severity) { + if severity != "info" { + d.logger.Printf("[WARN] Dispatcher: unrecognized severity %q for %s/%s — not routing", severity, customerID, eventType) + } return } @@ -79,7 +102,7 @@ func (d *Dispatcher) sendTestEmail(customerID string) { subject := "[Felhom] Teszt értesítés" body := "Kedves Ügyfél!\n\nEz egy teszt értesítés a Felhom monitoring rendszerből.\nAz értesítések megfelelően működnek.\n\nÜdvözlettel,\nFelhom.eu monitoring" - if err := d.sendEmail(prefs.Email, subject, body); err != nil { + if err := d.sendEmailFn(prefs.Email, subject, body); err != nil { d.logger.Printf("[ERROR] Test email to %s failed: %v", prefs.Email, err) d.store.LogNotification(customerID, "test", "info", "Teszt értesítés", "failed", err.Error(), "customer") return @@ -104,7 +127,7 @@ func (d *Dispatcher) processOperator(customerID, eventType, severity, message, d subject, body := FormatOperatorEmail(customerID, eventType, severity, message, detailsJSON) - if err := d.sendEmail(d.operatorEmail, subject, body); err != nil { + if err := d.sendEmailFn(d.operatorEmail, subject, body); err != nil { d.logger.Printf("[ERROR] Operator email failed for %s/%s: %v", customerID, eventType, err) d.store.LogNotification(customerID, eventType, severity, message, "failed", err.Error(), "operator") return @@ -119,9 +142,11 @@ func (d *Dispatcher) processCustomer(customerID, eventType, severity, message, d return } - // Load preferences + // Load preferences. GetNotificationPrefs returns (nil, nil) for a customer with no notification row — + // guard the nil BEFORE dereferencing (else an event for such a customer panics the dispatcher + // goroutine and crashes the hub). No prefs / no email → no customer notification. prefs, err := d.store.GetNotificationPrefs(customerID) - if err != nil || prefs.Email == "" { + if err != nil || prefs == nil || prefs.Email == "" { return } @@ -148,7 +173,7 @@ func (d *Dispatcher) processCustomer(customerID, eventType, severity, message, d subject, body := FormatCustomerEmail(customerID, eventType, severity, message, detailsJSON) - if err := d.sendEmail(prefs.Email, subject, body); err != nil { + if err := d.sendEmailFn(prefs.Email, subject, body); err != nil { d.logger.Printf("[ERROR] Customer email failed for %s/%s: %v", customerID, eventType, err) d.store.LogNotification(customerID, eventType, severity, message, "failed", err.Error(), "customer") return diff --git a/hub/internal/notify/dispatcher_test.go b/hub/internal/notify/dispatcher_test.go new file mode 100644 index 0000000..cffb88d --- /dev/null +++ b/hub/internal/notify/dispatcher_test.go @@ -0,0 +1,99 @@ +package notify + +import ( + "bytes" + "io" + "log" + "path/filepath" + "strings" + "sync" + "testing" + + "gitea.dooplex.hu/admin/felhom-hub/internal/store" + _ "modernc.org/sqlite" +) + +func newDispStore(t *testing.T) *store.Store { + t.Helper() + st, err := store.New(filepath.Join(t.TempDir(), "d.db"), log.New(io.Discard, "", 0)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { st.Close() }) + st.SaveCustomerConfig(&store.CustomerConfig{CustomerID: "c1", APIKey: "k", RetrievalPassword: "p"}) + return st +} + +// TestSeverityNotifies is the Part-0 contract + companion red-proof: warning/error/critical notify; info +// and unknown values do not. The companion models the PRE-FIX predicate (warning||error) that silently +// dropped "critical" — the bug this fixes. +func TestSeverityNotifies(t *testing.T) { + for _, s := range []string{"warning", "error", "critical"} { + if !severityNotifies(s) { + t.Errorf("%q must notify", s) + } + } + for _, s := range []string{"info", "", "frobnicate", "debug"} { + if severityNotifies(s) { + t.Errorf("%q must NOT notify", s) + } + } + // COMPANION RED-PROOF: the pre-fix predicate dropped "critical". The fix routes it. + preFix := func(sev string) bool { return sev == "warning" || sev == "error" } + if preFix("critical") { + t.Fatal("control: the pre-fix predicate should not match critical") + } + if !severityNotifies("critical") { + t.Fatal("the fix MUST route critical where the old (warning||error) predicate dropped it") + } +} + +// TestProcessEvent_CriticalRoutes: a critical-severity event reaches the operator channel (Scenario E). +func TestProcessEvent_CriticalRoutes(t *testing.T) { + st := newDispStore(t) + d := NewDispatcher(st, "test-key", "from@felhom.eu", "op@felhom.eu", true, log.New(io.Discard, "", 0)) + var mu sync.Mutex + var sent []string + d.sendEmailFn = func(to, _, _ string) error { mu.Lock(); defer mu.Unlock(); sent = append(sent, to); return nil } + + d.ProcessEvent("c1", "host_disk_critical", "critical", "root full", "{}", "hub") + if len(sent) != 1 || sent[0] != "op@felhom.eu" { + t.Fatalf("critical must route to the operator, sent=%v", sent) + } +} + +// TestProcessEvent_UnknownSeverityLogged: an unrecognized severity is LOGGED (not silently dropped) and +// not routed. +func TestProcessEvent_UnknownSeverityLogged(t *testing.T) { + st := newDispStore(t) + var buf bytes.Buffer + d := NewDispatcher(st, "test-key", "from", "op@felhom.eu", true, log.New(&buf, "", 0)) + sent := 0 + d.sendEmailFn = func(_, _, _ string) error { sent++; return nil } + + d.ProcessEvent("c1", "weird_event", "frobnicate", "msg", "{}", "hub") + if sent != 0 { + t.Fatal("an unknown severity must not route") + } + if !strings.Contains(buf.String(), "unrecognized severity") { + t.Fatalf("an unknown severity must be logged, got: %q", buf.String()) + } +} + +// TestProcessEvent_InfoSilent: "info" is an intentional non-notify — not routed AND not logged as +// unrecognized (it's a known, deliberate non-alert). +func TestProcessEvent_InfoSilent(t *testing.T) { + st := newDispStore(t) + var buf bytes.Buffer + d := NewDispatcher(st, "test-key", "from", "op@felhom.eu", true, log.New(&buf, "", 0)) + sent := 0 + d.sendEmailFn = func(_, _, _ string) error { sent++; return nil } + + d.ProcessEvent("c1", "controller_started", "info", "msg", "{}", "hub") + if sent != 0 { + t.Fatal("info must not route") + } + if strings.Contains(buf.String(), "unrecognized") { + t.Fatal("info is an intentional non-notify — it must NOT be logged as unrecognized") + } +} diff --git a/hub/internal/notify/templates.go b/hub/internal/notify/templates.go index 6b90b68..0594173 100644 --- a/hub/internal/notify/templates.go +++ b/hub/internal/notify/templates.go @@ -23,7 +23,7 @@ func init() { // FormatOperatorEmail returns (subject, textBody) for the operator channel. func FormatOperatorEmail(customerID, eventType, severity, message, detailsJSON string) (string, string) { icon := "⚠️" - if severity == "error" { + if severity == "error" || severity == "critical" { icon = "🔴" }