From 47b8664f0b38db54d1ecd6790640e0a3d7879836 Mon Sep 17 00:00:00 2001 From: Richard Courtman Date: Thu, 3 Sep 2026 23:42:50 +0100 Subject: [PATCH] Classify notification failures from senders, not error prose Terminal notification failures are 36% of resolved delivery outcomes fleet-wide (126,337 dead-lettered against 224,692 delivered in the week to 2026-09-03, over 6,668 clean installs), and the category breakdown could not say why: unknown was the modal bucket at 31,218. The class was being derived by substring-matching the Go error message. That fails in two ways. Any failure whose text carries none of the ~50 recognised tokens falls through to unknown, which is most of what SMTP produces: net/smtp reports the server's verdict as a reply code, and only 535 was ever matched, so a 550 relay refusal and a 451 temporary failure both recorded as unknown. Worse, the text being matched includes the destination's own response body, so a third party can choose the reason code Pulse records and shows the operator - a 500 whose body contains "rate limit" was recorded as rate_limited rather than server_error. Senders now declare the class where they already know it, and the classifier reads Go's own error types before it reads any prose: *textproto.Error for SMTP reply codes, x509 and tls for certificate failures, net.DNSError and timeouts for connectivity. HTTP status codes set the class at the five sites that build a status error, so the response body is preserved for the operator's audit row but can no longer influence the classification. Prose matching remains only as the last resort for paths that declare nothing. SMTP 5xx is deliberately not mapped the way HTTP 5xx is: a 550 is the destination refusing the message, not the destination breaking, so only the transient 4xx replies count as server_error. Registers the wider finding as a coverage gap. The 36% is concentration, not breadth - 72 installs that delivered nothing at all in seven days account for half of all terminal failures, and Pulse neither backs off nor tells those operators the destination has never once succeeded. --- docs/release-control/v6/internal/status.json | 38 +++ .../v6/internal/subsystems/notifications.md | 27 ++ internal/notifications/email_enhanced.go | 8 +- internal/notifications/failure_class.go | 172 +++++++++++++ internal/notifications/failure_class_test.go | 242 ++++++++++++++++++ internal/notifications/notifications.go | 20 +- internal/notifications/queue.go | 36 ++- 7 files changed, 522 insertions(+), 21 deletions(-) create mode 100644 internal/notifications/failure_class.go create mode 100644 internal/notifications/failure_class_test.go diff --git a/docs/release-control/v6/internal/status.json b/docs/release-control/v6/internal/status.json index 5c0036325..70abebaeb 100644 --- a/docs/release-control/v6/internal/status.json +++ b/docs/release-control/v6/internal/status.json @@ -10128,6 +10128,44 @@ "kind": "file" } ] + }, + { + "id": "notification-delivery-terminal-failure-diagnosis", + "summary": "Terminal notification failures are 36% of resolved delivery outcomes fleet-wide (126,337 dead-lettered against 224,692 delivered, week to 2026-09-03, 6,668 clean installs), and the failure breakdown could not explain why. Two causes, one fixed here. First, the delivery failure class was inferred from Go error prose, so any failure whose text carried no recognised token fell to unknown (31,218, the modal bucket) and the destination's own response body could steer the class, letting a 500 whose body says 'rate limit' be recorded as rate_limited; senders now declare the class and SMTP reply codes, DNS, x509 and timeout errors classify structurally. Second, the shape is concentration, not breadth: only 400 of 6,668 installs report any failure, the top 10 carry 73,276 (58%), the median failing install has 13, and 72 installs that delivered nothing at all in seven days account for 62,615 failures (50%). Pulse retries a permanently broken destination three times per notification forever and never tells the operator the destination has never once succeeded, so the worst install burned 20,312 dead letters against 36 deliveries. Still open: back off or disable a destination with no successful delivery, surface that state on the destinations surface, and add server_error guidance to the delivery health card, which currently falls through to the unknown copy for a class the client already emits.", + "owner": "project-owner", + "status": "triaged", + "recorded_at": "2026-09-03", + "lane_ids": [ + "L6" + ], + "subsystem_ids": [ + "alerts", + "notifications" + ], + "proposed_resolution": "lane-expansion", + "coverage_impact": 3, + "evidence": [ + { + "repo": "pulse", + "path": "frontend-modern/src/utils/alertDestinationsPresentation.ts", + "kind": "file" + }, + { + "repo": "pulse", + "path": "internal/notifications/failure_class.go", + "kind": "file" + }, + { + "repo": "pulse", + "path": "internal/notifications/queue.go", + "kind": "file" + }, + { + "repo": "pulse-pro", + "path": "docs/TELEMETRY_SIGNALS.md", + "kind": "file" + } + ] } ], "candidate_lanes": [ diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 6bfe2aaad..9a01d6ca2 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -52,6 +52,7 @@ or displayed a notification. 9. `internal/notifications/tag_routing.go` 10. `internal/notifications/delivery_health.go` 11. `internal/notifications/deadman_config.go` +12. `internal/notifications/failure_class.go` ## Shared Boundaries @@ -525,3 +526,29 @@ content and destination identity never leave Pulse. `internal/notifications/queue_test.go` pins the local classification order and the terminal-only retry/dead-letter accounting boundary. + +### Delivery failure class is declared by the sender, never read from a response + +The failure class is authoritative where the sender knows it and heuristic only +where nobody does. `internal/notifications/failure_class.go` owns that order: +a class declared by the sender through `NotificationFailureError` wins, then +Go's own error types decide (`*textproto.Error` carries the SMTP reply code, +x509 and TLS errors mean `tls`, `net.DNSError` and timeouts mean +`connectivity`), and only then is the error message text consulted. + +Two rules follow and are not optional. A destination's response body must never +determine its own failure class: every site that builds an error from an HTTP +status classifies from the status code, so the body is retained for the +operator's audit row but cannot make a 500 that says "rate limit" record as +`rate_limited`. And an SMTP reply code is not read like an HTTP status: a 5xx +reply is the destination refusing the message (`rejected`, or `authentication` +for 530/534/535/538 and `configuration` for the 500-504 syntax replies), while +only the transient 4xx replies are `server_error`. + +`RecordAuditError` is the canonical audit entry point because it is the one +that preserves a declared class; `RecordAudit` re-derives from text and is +retained only for callers that never had the error value. + +`internal/notifications/failure_class_test.go` pins the precedence order, the +SMTP reply-code mapping, and the rule that response-body text cannot steer the +recorded class. diff --git a/internal/notifications/email_enhanced.go b/internal/notifications/email_enhanced.go index e4c66f1dd..d71045a2c 100644 --- a/internal/notifications/email_enhanced.go +++ b/internal/notifications/email_enhanced.go @@ -55,7 +55,7 @@ func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) { case "password:": return []byte(a.password), nil default: - return nil, fmt.Errorf("unexpected LOGIN prompt: %s", fromServer) + return nil, FailfWithClass(NotificationFailureAuthentication, "unexpected LOGIN prompt: %s", fromServer) } } @@ -75,7 +75,7 @@ func (a *plainAuth) Start(server *smtp.ServerInfo) (string, []byte, error) { func (a *plainAuth) Next(fromServer []byte, more bool) ([]byte, error) { if more { - return nil, fmt.Errorf("unexpected server challenge") + return nil, FailfWithClass(NotificationFailureAuthentication, "unexpected server challenge") } return nil, nil } @@ -95,14 +95,14 @@ type resolvedEmailAddresses struct { func resolveEmailAddress(fieldName, value string) (*mail.Address, error) { addr, err := mail.ParseAddress(strings.TrimSpace(value)) if err != nil { - return nil, fmt.Errorf("invalid %s address %q: %w", fieldName, value, err) + return nil, FailfWithClass(NotificationFailureConfiguration, "invalid %s address %q: %w", fieldName, value, err) } return addr, nil } func resolveRecipientAddresses(values []string) ([]*mail.Address, error) { if len(values) == 0 { - return nil, fmt.Errorf("at least one recipient address is required") + return nil, FailfWithClass(NotificationFailureConfiguration, "at least one recipient address is required") } resolved := make([]*mail.Address, 0, len(values)) diff --git a/internal/notifications/failure_class.go b/internal/notifications/failure_class.go new file mode 100644 index 000000000..80d62ec5d --- /dev/null +++ b/internal/notifications/failure_class.go @@ -0,0 +1,172 @@ +package notifications + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net" + "net/textproto" + "syscall" +) + +// Delivery failure classification is authoritative where the sender knows the +// answer and heuristic only where it does not. +// +// The original classifier read the class out of the error prose. That is wrong +// in two ways that both showed up in fleet telemetry. It cannot see failures +// whose text carries no recognised token, which is why `unknown` became the +// modal bucket; and the prose it reads includes the destination's own response +// body, so a third party can steer the reason code by putting "rate limit" or +// "unauthorized" in the body of a 500. A declared class removes both: the +// sender states what happened, and the text is only consulted when nobody did. + +// NotificationFailureError carries the delivery failure class its sender +// already knew, so the class never has to be inferred from the message. +type NotificationFailureError struct { + Class NotificationFailureClass + Err error +} + +func (e *NotificationFailureError) Error() string { + if e == nil { + return "" + } + if e.Err == nil { + return string(e.Class) + } + return e.Err.Error() +} + +func (e *NotificationFailureError) Unwrap() error { + if e == nil { + return nil + } + return e.Err +} + +// FailWithClass labels err with an authoritative delivery failure class. +// A nil err yields nil so callers can wrap unconditionally. +func FailWithClass(class NotificationFailureClass, err error) error { + if err == nil { + return nil + } + return &NotificationFailureError{Class: class, Err: err} +} + +// FailfWithClass builds a labelled failure from a format string. The format +// arguments may include destination response text; that text is preserved for +// the operator-facing audit row but can no longer influence the class. +func FailfWithClass(class NotificationFailureClass, format string, args ...any) error { + return &NotificationFailureError{Class: class, Err: fmt.Errorf(format, args...)} +} + +// ClassFromHTTPStatus maps an HTTP response status to its delivery class. +// Status codes are the destination's own structured verdict, so they outrank +// anything the response body happens to say. +func ClassFromHTTPStatus(status int) NotificationFailureClass { + switch status { + case 401, 403, 407: + return NotificationFailureAuthentication + case 402: + return NotificationFailureConfiguration + case 408: + return NotificationFailureConnectivity + case 429: + return NotificationFailureRateLimited + } + switch { + case status >= 500 && status <= 599: + return NotificationFailureServerError + case status >= 400 && status <= 499: + return NotificationFailureRejected + default: + return NotificationFailureUnknown + } +} + +// ClassFromSMTPCode maps an SMTP reply code to its delivery class. +// +// SMTP 5xx is not the HTTP 5xx meaning: a 550 is the destination refusing the +// message, not the destination breaking. Only the transient 4xx replies are a +// server-side fault, and the 5xx range splits between our credentials, our +// command, and the recipient. +func ClassFromSMTPCode(code int) NotificationFailureClass { + switch code { + case 530, 534, 535, 538: + return NotificationFailureAuthentication + case 500, 501, 502, 503, 504: + // Syntax, bad sequence, or an unimplemented command: the session Pulse + // built does not match what this server accepts. + return NotificationFailureConfiguration + } + switch { + case code >= 400 && code <= 499: + // Transient negative completion. The server responded and asked for + // this to be tried again later. + return NotificationFailureServerError + case code >= 500 && code <= 599: + return NotificationFailureRejected + default: + return NotificationFailureUnknown + } +} + +// ClassifyNotificationFailureError determines the delivery failure class for a +// send error. It prefers what the sender declared, then what Go's own error +// types prove, and only then falls back to reading the message text. +func ClassifyNotificationFailureError(err error) NotificationFailureClass { + if err == nil { + return NotificationFailureUnknown + } + + var declared *NotificationFailureError + if errors.As(err, &declared) && declared.Class != "" { + return declared.Class + } + + // An SMTP reply code is the mail server's structured verdict. net/smtp + // surfaces it as *textproto.Error however deeply it is wrapped. + var protoErr *textproto.Error + if errors.As(err, &protoErr) { + return ClassFromSMTPCode(protoErr.Code) + } + + // Certificate and handshake failures, before the generic network checks: + // a verification failure is also reachable through a dial. + var certVerifyErr *tls.CertificateVerificationError + var unknownAuthorityErr x509.UnknownAuthorityError + var hostnameErr x509.HostnameError + var certInvalidErr x509.CertificateInvalidError + var recordHeaderErr tls.RecordHeaderError + if errors.As(err, &certVerifyErr) || + errors.As(err, &unknownAuthorityErr) || + errors.As(err, &hostnameErr) || + errors.As(err, &certInvalidErr) || + errors.As(err, &recordHeaderErr) { + return NotificationFailureTLS + } + + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return NotificationFailureConnectivity + } + + if errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, syscall.ECONNREFUSED) || + errors.Is(err, syscall.ECONNRESET) || + errors.Is(err, syscall.EHOSTUNREACH) || + errors.Is(err, syscall.ENETUNREACH) || + errors.Is(err, syscall.ETIMEDOUT) || + errors.Is(err, syscall.EPIPE) { + return NotificationFailureConnectivity + } + + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return NotificationFailureConnectivity + } + + return ClassifyNotificationFailure(err.Error()) +} diff --git a/internal/notifications/failure_class_test.go b/internal/notifications/failure_class_test.go new file mode 100644 index 000000000..3afd9e69c --- /dev/null +++ b/internal/notifications/failure_class_test.go @@ -0,0 +1,242 @@ +package notifications + +import ( + "context" + "crypto/x509" + "errors" + "fmt" + "net" + "net/textproto" + "testing" + "time" +) + +func TestClassFromHTTPStatus(t *testing.T) { + cases := map[int]NotificationFailureClass{ + 401: NotificationFailureAuthentication, + 403: NotificationFailureAuthentication, + 407: NotificationFailureAuthentication, + 402: NotificationFailureConfiguration, + 408: NotificationFailureConnectivity, + 429: NotificationFailureRateLimited, + 400: NotificationFailureRejected, + 404: NotificationFailureRejected, + 422: NotificationFailureRejected, + 500: NotificationFailureServerError, + 502: NotificationFailureServerError, + 503: NotificationFailureServerError, + 200: NotificationFailureUnknown, + } + for status, want := range cases { + if got := ClassFromHTTPStatus(status); got != want { + t.Errorf("ClassFromHTTPStatus(%d) = %q, want %q", status, got, want) + } + } +} + +func TestClassFromSMTPCode(t *testing.T) { + // SMTP 5xx is a refusal, not a server fault: only the transient 4xx range + // means the destination broke. + cases := map[int]NotificationFailureClass{ + 421: NotificationFailureServerError, + 450: NotificationFailureServerError, + 451: NotificationFailureServerError, + 452: NotificationFailureServerError, + 530: NotificationFailureAuthentication, + 535: NotificationFailureAuthentication, + 538: NotificationFailureAuthentication, + 500: NotificationFailureConfiguration, + 501: NotificationFailureConfiguration, + 503: NotificationFailureConfiguration, + 550: NotificationFailureRejected, + 552: NotificationFailureRejected, + 554: NotificationFailureRejected, + 250: NotificationFailureUnknown, + } + for code, want := range cases { + if got := ClassFromSMTPCode(code); got != want { + t.Errorf("ClassFromSMTPCode(%d) = %q, want %q", code, got, want) + } + } +} + +// A destination controls its own response body. It must not be able to choose +// the reason code Pulse records and shows the operator. +func TestClassifyNotificationFailureError_ResponseBodyCannotSteerClass(t *testing.T) { + cases := []struct { + name string + status int + body string + want NotificationFailureClass + }{ + {"server error body claiming rate limit", 500, `{"error":"rate limit exceeded"}`, NotificationFailureServerError}, + {"server error body claiming unauthorized", 503, `unauthorized`, NotificationFailureServerError}, + {"rejection body mentioning certificate", 404, `no certificate route found`, NotificationFailureRejected}, + {"rejection body mentioning connection refused", 400, `upstream connection refused`, NotificationFailureRejected}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := FailfWithClass( + ClassFromHTTPStatus(tc.status), + "webhook returned HTTP %d: %s", tc.status, tc.body, + ) + if got := ClassifyNotificationFailureError(err); got != tc.want { + t.Errorf("class = %q, want %q", got, tc.want) + } + // The prose classifier is the one that gets this wrong; that is + // precisely why the declared class has to win. + if prose := ClassifyNotificationFailure(err.Error()); prose == tc.want { + t.Logf("prose classifier happened to agree for %q", tc.name) + } + }) + } +} + +func TestClassifyNotificationFailureError_DeclaredClassSurvivesWrapping(t *testing.T) { + base := FailWithClass(NotificationFailureConfiguration, errors.New("no Apprise targets configured for CLI delivery")) + wrapped := fmt.Errorf("apprise CLI send failed: %w", base) + if got := ClassifyNotificationFailureError(wrapped); got != NotificationFailureConfiguration { + t.Fatalf("class = %q, want %q", got, NotificationFailureConfiguration) + } +} + +func TestClassifyNotificationFailureError_SMTPReplyCode(t *testing.T) { + // net/smtp surfaces the server's reply as *textproto.Error. Before this + // path existed every one of these landed in the unknown bucket. + cases := []struct { + code int + msg string + want NotificationFailureClass + }{ + {550, "5.7.1 Relay access denied", NotificationFailureRejected}, + {535, "5.7.8 Authentication credentials invalid", NotificationFailureAuthentication}, + {451, "4.3.0 Temporary local problem", NotificationFailureServerError}, + {501, "5.5.4 Syntax error in parameters", NotificationFailureConfiguration}, + } + for _, tc := range cases { + err := fmt.Errorf("failed to send email: %w", &textproto.Error{Code: tc.code, Msg: tc.msg}) + if got := ClassifyNotificationFailureError(err); got != tc.want { + t.Errorf("SMTP %d: class = %q, want %q", tc.code, got, tc.want) + } + } +} + +func TestClassifyNotificationFailureError_StructuralNetworkErrors(t *testing.T) { + cases := []struct { + name string + err error + want NotificationFailureClass + }{ + { + name: "dns failure", + err: fmt.Errorf("post webhook: %w", &net.DNSError{Err: "server misbehaving", Name: "hooks.example"}), + want: NotificationFailureConnectivity, + }, + { + name: "context deadline", + err: fmt.Errorf("post webhook: %w", context.DeadlineExceeded), + want: NotificationFailureConnectivity, + }, + { + name: "unknown certificate authority", + err: fmt.Errorf("post webhook: %w", x509.UnknownAuthorityError{}), + want: NotificationFailureTLS, + }, + { + name: "certificate hostname mismatch", + err: fmt.Errorf("post webhook: %w", x509.HostnameError{Host: "hooks.example"}), + want: NotificationFailureTLS, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ClassifyNotificationFailureError(tc.err); got != tc.want { + t.Errorf("class = %q, want %q", got, tc.want) + } + }) + } +} + +func TestClassifyNotificationFailureError_FallsBackToProse(t *testing.T) { + err := errors.New("webhook returned HTTP 429 Too Many Requests") + if got := ClassifyNotificationFailureError(err); got != NotificationFailureRateLimited { + t.Fatalf("class = %q, want %q", got, NotificationFailureRateLimited) + } +} + +func TestClassifyNotificationFailureError_NilIsUnknown(t *testing.T) { + if got := ClassifyNotificationFailureError(nil); got != NotificationFailureUnknown { + t.Fatalf("class = %q, want %q", got, NotificationFailureUnknown) + } +} + +func TestFailWithClassPreservesMessageAndUnwrap(t *testing.T) { + cause := errors.New("dial tcp 10.0.0.1:465: connect: connection refused") + err := FailWithClass(NotificationFailureConnectivity, cause) + if err.Error() != cause.Error() { + t.Errorf("Error() = %q, want %q", err.Error(), cause.Error()) + } + if !errors.Is(err, cause) { + t.Error("wrapped error does not unwrap to its cause") + } + if FailWithClass(NotificationFailureConnectivity, nil) != nil { + t.Error("FailWithClass(nil) should be nil") + } +} + +// The declared class has to survive all the way into the audit row that feeds +// both the operator's delivery health card and the telemetry counters. +func TestRecordAuditErrorPersistsDeclaredClass(t *testing.T) { + nq, err := NewNotificationQueue(t.TempDir()) + if err != nil { + t.Fatalf("NewNotificationQueue: %v", err) + } + defer func() { _ = nq.Stop() }() + + now := time.Now().UTC() + entries := []*QueuedNotification{ + {ID: "body-says-rate-limit", Type: "webhook", Status: QueueStatusDLQ, Attempts: 3, Config: []byte(`{}`), CreatedAt: now}, + {ID: "smtp-refusal", Type: "email", Status: QueueStatusDLQ, Attempts: 3, Config: []byte(`{}`), CreatedAt: now}, + } + for _, entry := range entries { + status := entry.Status + entry.Status = QueueStatusPending + if err := nq.Enqueue(entry); err != nil { + t.Fatalf("enqueue %s: %v", entry.ID, err) + } + entry.Status = status + } + + // A 500 whose body says "rate limit" is a server error, not rate limiting. + bodySteered := FailfWithClass( + ClassFromHTTPStatus(500), + "webhook returned HTTP %d: %s", 500, `{"error":"rate limit exceeded"}`, + ) + if err := nq.RecordAuditError(entries[0], false, bodySteered); err != nil { + t.Fatalf("record body-steered audit: %v", err) + } + smtpRefusal := fmt.Errorf("failed to send email: %w", &textproto.Error{Code: 550, Msg: "5.7.1 Relay access denied"}) + if err := nq.RecordAuditError(entries[1], false, smtpRefusal); err != nil { + t.Fatalf("record smtp audit: %v", err) + } + + stats, err := nq.GetTelemetryStats(now.Add(-time.Hour)) + if err != nil { + t.Fatalf("GetTelemetryStats: %v", err) + } + if stats.Failures != 2 { + t.Fatalf("failures = %d, want 2", stats.Failures) + } + if stats.FailureClasses.ServerError != 1 { + t.Errorf("server_error = %d, want 1", stats.FailureClasses.ServerError) + } + if stats.FailureClasses.Rejected != 1 { + t.Errorf("rejected = %d, want 1 (SMTP 550 is a refusal)", stats.FailureClasses.Rejected) + } + if stats.FailureClasses.RateLimited != 0 { + t.Errorf("rate_limited = %d, want 0: the response body must not set the class", stats.FailureClasses.RateLimited) + } + if stats.FailureClasses.Unknown != 0 { + t.Errorf("unknown = %d, want 0", stats.FailureClasses.Unknown) + } +} diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 79d494a22..4721a784a 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -1695,7 +1695,7 @@ func configJSONForNotificationDeliveryJob(job notificationDeliveryJob) ([]byte, return json.Marshal(*job.WebhookConfig) case "apprise": if job.AppriseConfig == nil { - return nil, fmt.Errorf("missing apprise config") + return nil, FailfWithClass(NotificationFailureConfiguration, "missing apprise config") } return json.Marshal(*job.AppriseConfig) default: @@ -1853,7 +1853,7 @@ func (n *NotificationManager) deliverNotificationJob(job notificationDeliveryJob return n.sendGroupedWebhook(*job.WebhookConfig, job.Alerts) case "apprise": if job.AppriseConfig == nil { - return fmt.Errorf("missing apprise config") + return FailfWithClass(NotificationFailureConfiguration, "missing apprise config") } if job.Event == eventResolved { return n.sendResolvedApprise(*job.AppriseConfig, job.Alerts, job.ResolvedAt) @@ -1919,7 +1919,7 @@ func (n *NotificationManager) sendGroupedApprise(config AppriseConfig, alertList cfg := NormalizeAppriseConfig(config) if !cfg.Enabled { - return fmt.Errorf("apprise not enabled") + return FailfWithClass(NotificationFailureConfiguration, "apprise not enabled") } title, body, notifyType := buildApprisePayload(alertList, n.publicURL) @@ -2089,7 +2089,7 @@ func resolveAppriseNotificationType(alertList []*alerts.Alert) string { func (n *NotificationManager) sendAppriseViaCLI(cfg AppriseConfig, title, body string) error { if len(cfg.Targets) == 0 { - return fmt.Errorf("no Apprise targets configured for CLI delivery") + return FailfWithClass(NotificationFailureConfiguration, "no Apprise targets configured for CLI delivery") } ctx, cancel := context.WithTimeout(context.Background(), time.Duration(cfg.TimeoutSeconds)*time.Second) @@ -2216,9 +2216,9 @@ func (n *NotificationManager) sendAppriseViaHTTP(cfg AppriseConfig, title, body, if resp.StatusCode < 200 || resp.StatusCode >= 300 { if len(respBody) > 0 { - return fmt.Errorf("apprise server returned HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(respBody))) + return FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "apprise server returned HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(respBody))) } - return fmt.Errorf("apprise server returned HTTP %d", resp.StatusCode) + return FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "apprise server returned HTTP %d", resp.StatusCode) } if len(respBody) > 0 { @@ -2239,7 +2239,7 @@ func (n *NotificationManager) sendResolvedApprise(config AppriseConfig, alertLis cfg := NormalizeAppriseConfig(config) if !cfg.Enabled { - return fmt.Errorf("apprise not enabled") + return FailfWithClass(NotificationFailureConfiguration, "apprise not enabled") } title, _, body := buildResolvedNotificationContent(alertList, resolvedAt, n.publicURL) @@ -2813,7 +2813,7 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale Int("status", resp.StatusCode). Str("response", respBody.String()). Msg("resolved ntfy webhook returned non-success status") - return fmt.Errorf("ntfy webhook returned HTTP %d: %s", resp.StatusCode, respBody.String()) + return FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "ntfy webhook returned HTTP %d: %s", resp.StatusCode, respBody.String()) } // checkWebhookRateLimit checks if a webhook can be sent based on rate limits @@ -2981,7 +2981,7 @@ func (n *NotificationManager) executeWebhookRequest(webhook WebhookConfig, paylo } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return result, fmt.Errorf("webhook returned HTTP %d: %s", resp.StatusCode, result.body) + return result, FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "webhook returned HTTP %d: %s", resp.StatusCode, result.body) } return result, nil @@ -3049,7 +3049,7 @@ func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData Int("status", result.statusCode). Str("response", result.body). Msg("webhook returned non-success status") - return fmt.Errorf("webhook returned HTTP %d: %s", result.statusCode, result.body) + return FailfWithClass(ClassFromHTTPStatus(result.statusCode), "webhook returned HTTP %d: %s", result.statusCode, result.body) } } diff --git a/internal/notifications/queue.go b/internal/notifications/queue.go index 062011321..735cc92d7 100644 --- a/internal/notifications/queue.go +++ b/internal/notifications/queue.go @@ -1456,8 +1456,34 @@ func (nq *NotificationQueue) notifyDeliveryHealthChanged() { } } -// RecordAudit records a notification delivery attempt in the audit log +// RecordAudit records a notification delivery attempt in the audit log, +// deriving the failure class from the error text. Prefer RecordAuditError, +// which keeps the class the sender declared. func (nq *NotificationQueue) RecordAudit(notif *QueuedNotification, success bool, errorMsg string) error { + failureClass := NotificationFailureClass("") + if !success { + failureClass = ClassifyNotificationFailure(errorMsg) + } + return nq.recordAudit(notif, success, errorMsg, failureClass) +} + +// RecordAuditError records a delivery attempt from the send error itself, so a +// class the sender declared reaches the audit row instead of being re-derived +// from the message. This is what keeps a destination's response body out of the +// classification decision. +func (nq *NotificationQueue) RecordAuditError(notif *QueuedNotification, success bool, sendErr error) error { + errorMsg := "" + failureClass := NotificationFailureClass("") + if !success { + if sendErr != nil { + errorMsg = sendErr.Error() + } + failureClass = ClassifyNotificationFailureError(sendErr) + } + return nq.recordAudit(notif, success, errorMsg, failureClass) +} + +func (nq *NotificationQueue) recordAudit(notif *QueuedNotification, success bool, errorMsg string, failureClass NotificationFailureClass) error { nq.mu.Lock() defer nq.mu.Unlock() @@ -1477,10 +1503,6 @@ func (nq *NotificationQueue) RecordAudit(notif *QueuedNotification, success bool VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ` - failureClass := "" - if !success { - failureClass = string(ClassifyNotificationFailure(errorMsg)) - } _, err = nq.db.Exec(query, notif.ID, notif.Type, @@ -1492,7 +1514,7 @@ func (nq *NotificationQueue) RecordAudit(notif *QueuedNotification, success bool notif.Attempts, success, errorMsg, - failureClass, + string(failureClass), strings.TrimSpace(notif.DestinationID), notif.PayloadBytes, time.Now().Unix(), @@ -1853,7 +1875,7 @@ func (nq *NotificationQueue) processNotification(notif *QueuedNotification) { } else { notif.Links = persistedLinks } - if auditErr := nq.RecordAudit(notif, success, errorMsg); auditErr != nil { + if auditErr := nq.RecordAuditError(notif, success, err); auditErr != nil { log.Error(). Err(auditErr). Str("component", "notification_queue").