hub v0.107.0: the hub rewrote a severity and said nothing (R-387); golden 0.223.0
gates / gates (push) Successful in 17s
gates / gates (push) Successful in 17s
One handler, two fields, opposite discipline. An unknown event_type is rejected with a loud 400. An unknown severity was rewritten to "info" without a word - and severityNotifies drops "info" before BOTH legs, so the event was stored, answered 200, and mailed to nobody. Two shipped features went out that way: DiskAlertKind.Severity emitted "warn" until controller v0.215.0, app_start_failed until v0.223.0. Measured on the live hub DB today: 91 app_start_failed events stored all-time, ZERO notification_log rows before this session - not one, on any channel. The mechanism built to catch this class was structurally blind to it: the dispatcher's `unrecognized severity` line cannot execute for anything arriving over the API, because the coercion one line earlier guarantees the value it looks for cannot arrive. The coercion STAYS - a rejected event is a lost event, and losing an alarm is worse than mis-routing one. Only the silence is fixed: a WARN naming the customer, the event type and the rejected value. The dispatcher branch is KEPT, not deleted as dead, and the reason is evidence rather than caution: cmd/hub/main.go wires dispatcher.ProcessEvent DIRECTLY as the monitor.EventNotifyFunc for the staleness, host-staleness and offsite-box checkers, which never pass through the handler. For those it is the only severity guard there is. All 90 severity literals in internal/monitor are already valid, so the guard is silent because the producers are correct. Test count 702 -> 709. Red-proof seen failing: delete the WARN line and the coercion test fails with "the hub rewrote a severity and said nothing". Golden 0.223.0 baked and published (sha 9eaf39ac3921...), round-trip HTTP 206. Vouching is the operator's act and was not done here.
This commit is contained in:
@@ -2118,10 +2118,32 @@ func (h *Handler) handleEvent(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
// Validate/default severity (exact-match lowercase; unknown values coerce to info)
|
||||
// Validate/default severity (exact-match lowercase; unknown values coerce to info).
|
||||
//
|
||||
// R-387 — THE COERCION STAYS. THE SILENCE DOES NOT.
|
||||
//
|
||||
// Note the asymmetry two blocks up: an unknown event TYPE is rejected with a loud 400, while an
|
||||
// unknown SEVERITY was rewritten without a word. The silent one is the one that hid a real defect
|
||||
// for the whole life of two features — `DiskAlertKind.Severity` emitted "warn" until controller
|
||||
// v0.215.0, and `app_start_failed` emitted it until v0.223.0. Both were stored, both were
|
||||
// coerced here to "info", and "info" is dropped by severityNotifies — so both were mailed to
|
||||
// NOBODY, on either leg, while every POST returned 200.
|
||||
//
|
||||
// The dispatcher has a line whose job is exactly this (`unrecognized severity %q`), and it can
|
||||
// never execute, because this block guarantees the value it looks for cannot reach it. **The
|
||||
// mechanism built to detect this class was structurally blind to it.**
|
||||
//
|
||||
// WHY NOT A 400. A rejected event is a LOST event, and losing an alarm is worse than mis-routing
|
||||
// one — the same reasoning that made this a coercion in the first place. The producer is our own
|
||||
// controller, so the fix belongs at the emitter; this line is how the emitter's mistake becomes
|
||||
// VISIBLE the first time it happens instead of never.
|
||||
switch payload.Severity {
|
||||
case "info", "warning", "error", "critical":
|
||||
default:
|
||||
h.logger.Printf("[WARN] [api] Event from %s: severity %q is not in {info,warning,error,critical} "+
|
||||
"— coercing to \"info\", which severityNotifies DROPS, so this %s alert will reach NOBODY. "+
|
||||
"Fix the emitting controller; this event is stored but not routed.",
|
||||
payload.CustomerID, payload.Severity, payload.EventType)
|
||||
payload.Severity = "info"
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,133 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"io"
|
||||
"log"
|
||||
"net/http/httptest"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-hub/internal/store"
|
||||
)
|
||||
|
||||
// R-387 — the ingest handler rewrote an unknown severity WITHOUT SAYING SO, and the guard built to
|
||||
// catch that class sat downstream of the rewrite, permanently blind to it.
|
||||
//
|
||||
// THE ASYMMETRY. Two fields, same handler, opposite discipline: an unknown event TYPE is rejected
|
||||
// with a loud 400; an unknown SEVERITY was silently coerced to "info" — after which severityNotifies
|
||||
// drops it and NEITHER leg runs. Two shipped features (`DiskAlertKind.Severity` until controller
|
||||
// v0.215.0, `app_start_failed` until v0.223.0) emitted "warn" and were mailed to nobody, while every
|
||||
// POST returned 200 and every dashboard showed the alert.
|
||||
//
|
||||
// THE LAYER. The guard sits at INGEST, because that is the last point at which the offending value
|
||||
// still exists — by design it ceases to exist one line later, which is exactly why nothing downstream
|
||||
// could ever detect it.
|
||||
//
|
||||
// WHY NOT A 400. A rejected event is a LOST event, and losing an alarm is worse than mis-routing one.
|
||||
// The coercion is deliberate and stays; only the silence is fixed.
|
||||
//
|
||||
// RED-PROOF (observed, see REPORT.md): delete the h.logger.Printf from the default branch and
|
||||
// TestR387_UnknownSeverityIsCoercedAndAnnounced fails with `the hub rewrote a severity and said
|
||||
// nothing`.
|
||||
|
||||
func severityTestHandler(t *testing.T) (*Handler, *store.Store, *bytes.Buffer) {
|
||||
t.Helper()
|
||||
var buf bytes.Buffer
|
||||
path := filepath.Join(t.TempDir(), "test.db")
|
||||
st, err := store.New(path, log.New(io.Discard, "", 0))
|
||||
if err != nil {
|
||||
t.Fatalf("store.New: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { st.Close() })
|
||||
if err := st.SaveCustomerConfig(&store.CustomerConfig{
|
||||
CustomerID: "c1", APIKey: "k1", RetrievalPassword: "p",
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
h := New(st, globalKey, "", "", nil, log.New(&buf, "", 0))
|
||||
return h, st, &buf
|
||||
}
|
||||
|
||||
func postEvent(t *testing.T, h *Handler, eventType, severity string) *httptest.ResponseRecorder {
|
||||
t.Helper()
|
||||
body, _ := json.Marshal(map[string]any{
|
||||
"customer_id": "c1", "event_type": eventType, "severity": severity,
|
||||
"message": "test message",
|
||||
})
|
||||
return do(h, "POST", "/event", "k1", string(body))
|
||||
}
|
||||
|
||||
func TestR387_UnknownSeverityIsCoercedAndAnnounced(t *testing.T) {
|
||||
h, st, logs := severityTestHandler(t)
|
||||
|
||||
rr := postEvent(t, h, "backup_failed", "warn")
|
||||
|
||||
// The event must NOT be lost — that is the whole reason this is a coercion and not a 400.
|
||||
if rr.Code >= 400 {
|
||||
t.Fatalf("HTTP %d — an unknown severity must never LOSE the event; a rejected alarm is worse "+
|
||||
"than a mis-routed one", rr.Code)
|
||||
}
|
||||
|
||||
// The STORED effect: coerced to info, exactly as before.
|
||||
events, err := st.GetRecentEvents("c1", 10)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(events) != 1 {
|
||||
t.Fatalf("stored %d events, want 1 — the event was lost", len(events))
|
||||
}
|
||||
if events[0].Severity != "info" {
|
||||
t.Errorf("stored severity = %q, want \"info\" — the coercion behaviour must not change", events[0].Severity)
|
||||
}
|
||||
|
||||
// THE FIX: it is no longer silent.
|
||||
out := logs.String()
|
||||
if !strings.Contains(out, "WARN") {
|
||||
t.Fatalf("the hub rewrote a severity and said nothing — this is R-387, and it is how two "+
|
||||
"features shipped undeliverable for months. log=%q", out)
|
||||
}
|
||||
for _, want := range []string{`"warn"`, "c1", "backup_failed"} {
|
||||
if !strings.Contains(out, want) {
|
||||
t.Errorf("the log line does not name %q, so nobody can act on it: %q", want, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A VALID severity must stay silent — a guard that fires on the normal path is one people learn to
|
||||
// ignore, which is the same failure one door over.
|
||||
func TestR387_ValidSeveritiesAreSilent(t *testing.T) {
|
||||
for _, sev := range []string{"info", "warning", "error", "critical"} {
|
||||
h, _, logs := severityTestHandler(t)
|
||||
if rr := postEvent(t, h, "backup_failed", sev); rr.Code >= 400 {
|
||||
t.Fatalf("severity %q: HTTP %d", sev, rr.Code)
|
||||
}
|
||||
if strings.Contains(logs.String(), "not in {info,warning,error,critical}") {
|
||||
t.Errorf("severity %q produced a coercion warning: %q", sev, logs.String())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The asymmetry this finding is about, pinned so it stays deliberate: an unknown TYPE is still
|
||||
// rejected loudly, an unknown SEVERITY is still accepted-and-logged. Two different answers, both on
|
||||
// purpose, and now both visible.
|
||||
func TestR387_UnknownTypeStillRejectedUnknownSeverityStillAccepted(t *testing.T) {
|
||||
h, st, _ := severityTestHandler(t)
|
||||
|
||||
if rr := postEvent(t, h, "no_such_event_type", "warning"); rr.Code != 400 {
|
||||
t.Errorf("unknown event_type: HTTP %d, want 400 — an unlisted type must stay a loud refusal", rr.Code)
|
||||
}
|
||||
if rr := postEvent(t, h, "backup_failed", "nonsense"); rr.Code >= 400 {
|
||||
t.Errorf("unknown severity: HTTP %d, want <400 — it must be accepted and logged, never lost", rr.Code)
|
||||
}
|
||||
events, err := st.GetRecentEvents("c1", 10)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(events) != 1 {
|
||||
t.Errorf("stored %d events, want exactly 1 (the bad-severity one; the bad-type one must not "+
|
||||
"be stored)", len(events))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user