From 12e833d3d0173470146b3a853197d1f30f375c2e Mon Sep 17 00:00:00 2001 From: Richard Courtman Date: Thu, 3 Sep 2026 23:53:42 +0100 Subject: [PATCH] Stop retrying notification failures that cannot succeed 72 installs delivered no notification at all in the week to 2026-09-03 and between them burned 196,562 attempts for 62,618 dead letters. Their categorised failures are 79% authentication (11,693), configuration (9,554) and rejected (5,954) against 930 connectivity: deterministic verdicts about the request, not conditions that clear. The queue retried each of them the full ladder anyway, because the retry decision never consulted the failure class. Those three classes now dead-letter on the attempt that produced them. The same payload, sent again to the same destination with the same credentials, gets the same answer; spending two more attempts on it only delays the dead letter the operator needs to act on. Connectivity, rate limiting, server errors and unclassified failures keep the full ladder. TLS deliberately stays on the retrying side. A handshake can fail transiently during a rotation, and one wasted ladder is cheaper than dropping a recoverable notification. This is the same call webhook delivery already made for HTTP 4xx in isRetryableWebhookError, now generalised to every destination type and owned in one place. It was not safely expressible before the class became sender-declared rather than guessed from error prose. Nothing is lost by giving up sooner: RetryTerminalFailures still returns retained terminal failures to the queue with a fresh budget once the operator fixes the credentials or the configuration, and dead-letter rows now record the failure class and whether the cause was an exhausted ladder or a non-retryable class. Note for telemetry reads: notification_attempts_7d will fall sharply in the blackout cohort while notification_failures_7d is unchanged, because the same terminal failures now cost one attempt instead of three. That is the intended effect and not a drop in notification volume. --- .../v6/internal/subsystems/notifications.md | 27 +++++ internal/notifications/failure_class.go | 27 +++++ internal/notifications/failure_class_test.go | 111 ++++++++++++++++++ internal/notifications/queue.go | 18 ++- 4 files changed, 180 insertions(+), 3 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 9a01d6ca2..2cc850585 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -552,3 +552,30 @@ 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. + +### The retry ladder is gated on the failure class + +Delivery retries are for conditions that can clear. `authentication`, +`configuration`, and `rejected` are verdicts about the request itself: the same +payload, sent again to the same destination with the same credentials, gets the +same answer. Those three dead-letter on the attempt that produced them, without +consuming the remaining ladder. `connectivity`, `rate_limited`, `server_error`, +`tls`, and an unclassified failure keep the full ladder, because nothing about +them proves a later attempt fails. TLS is deliberately on the retrying side: a +handshake can fail transiently during rotation, and the cost of one wasted +ladder is lower than the cost of dropping a recoverable notification. + +`NotificationFailureClass.Retryable` is the single owner of that split. Neither +the queue nor any destination type may keep its own list. This generalises to +every destination the decision webhook delivery already made for HTTP 4xx in +`isRetryableWebhookError`. + +Dead-lettering early must not lose the notification: `RetryTerminalFailures` +remains the operator's recovery path, returning retained terminal failures to +the queue with a fresh budget once the credentials or configuration are fixed. +A dead-letter row records `failureClass` and a `deadLetterReason` of +`failure_class_not_retryable` or `max_retries_exhausted` so the two are +distinguishable in local logs. + +`internal/notifications/failure_class_test.go` pins the retryable split and +that a deterministic failure dead-letters on its first attempt. diff --git a/internal/notifications/failure_class.go b/internal/notifications/failure_class.go index 80d62ec5d..9995a9d21 100644 --- a/internal/notifications/failure_class.go +++ b/internal/notifications/failure_class.go @@ -170,3 +170,30 @@ func ClassifyNotificationFailureError(err error) NotificationFailureClass { return ClassifyNotificationFailure(err.Error()) } + +// Retryable reports whether another delivery attempt could plausibly succeed. +// +// Authentication, configuration and rejection are verdicts about the request +// itself: the same payload, sent again to the same destination with the same +// credentials, gets the same answer. Retrying them spends attempts, delays the +// dead letter the operator needs to see, and learns nothing. Connectivity, +// rate limiting and server errors describe conditions that clear on their own, +// and an unclassified failure is retried because nothing proves it will not +// succeed. +// +// This generalises to every destination type the decision webhook delivery +// already made for HTTP 4xx in isRetryableWebhookError. +// +// A dead letter is not the end of the road: once the operator fixes the +// credentials or the configuration, RetryTerminalFailures returns retained +// terminal failures to the queue with a fresh budget. +func (class NotificationFailureClass) Retryable() bool { + switch class { + case NotificationFailureAuthentication, + NotificationFailureConfiguration, + NotificationFailureRejected: + return false + default: + return true + } +} diff --git a/internal/notifications/failure_class_test.go b/internal/notifications/failure_class_test.go index 3afd9e69c..a2ba78da2 100644 --- a/internal/notifications/failure_class_test.go +++ b/internal/notifications/failure_class_test.go @@ -240,3 +240,114 @@ func TestRecordAuditErrorPersistsDeclaredClass(t *testing.T) { t.Errorf("unknown = %d, want 0", stats.FailureClasses.Unknown) } } + +func TestFailureClassRetryable(t *testing.T) { + cases := map[NotificationFailureClass]bool{ + NotificationFailureAuthentication: false, + NotificationFailureConfiguration: false, + NotificationFailureRejected: false, + NotificationFailureConnectivity: true, + NotificationFailureRateLimited: true, + NotificationFailureServerError: true, + NotificationFailureTLS: true, + NotificationFailureUnknown: true, + NotificationFailureClass(""): true, + } + for class, want := range cases { + if got := class.Retryable(); got != want { + t.Errorf("%q.Retryable() = %v, want %v", class, got, want) + } + } +} + +// A deterministic failure must dead-letter on the first attempt rather than +// spend the whole ladder re-asking a question that already has an answer. +func TestProcessNotificationDeadLettersDeterministicFailureImmediately(t *testing.T) { + cases := []struct { + name string + sendErr error + wantStatus NotificationQueueStatus + wantCallCount int + }{ + { + name: "authentication does not retry", + sendErr: FailWithClass(NotificationFailureAuthentication, errors.New("smtp 535 authentication failed")), + wantStatus: QueueStatusDLQ, + wantCallCount: 1, + }, + { + name: "configuration does not retry", + sendErr: FailWithClass(NotificationFailureConfiguration, errors.New("no Apprise targets configured for CLI delivery")), + wantStatus: QueueStatusDLQ, + wantCallCount: 1, + }, + { + name: "rejection does not retry", + sendErr: FailfWithClass(ClassFromHTTPStatus(422), "webhook returned HTTP 422: unprocessable"), + wantStatus: QueueStatusDLQ, + wantCallCount: 1, + }, + { + name: "connectivity still retries", + sendErr: FailWithClass(NotificationFailureConnectivity, errors.New("dial tcp: connection refused")), + wantStatus: QueueStatusPending, + wantCallCount: 1, + }, + { + name: "server error still retries", + sendErr: FailfWithClass(ClassFromHTTPStatus(503), "webhook returned HTTP 503: unavailable"), + wantStatus: QueueStatusPending, + wantCallCount: 1, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + nq, err := NewNotificationQueue(t.TempDir()) + if err != nil { + t.Fatalf("NewNotificationQueue: %v", err) + } + defer func() { _ = nq.Stop() }() + + futureRetry := time.Now().Add(time.Hour) + notif := &QueuedNotification{ + ID: "deterministic-failure", + Type: "webhook", + Status: QueueStatusPending, + MaxAttempts: 3, + Config: []byte(`{}`), + NextRetryAt: &futureRetry, + } + if err := nq.Enqueue(notif); err != nil { + t.Fatalf("enqueue: %v", err) + } + + calls := 0 + nq.SetProcessor(func(*QueuedNotification) error { + calls++ + return tc.sendErr + }) + + nq.processNotification(notif) + + if calls != tc.wantCallCount { + t.Errorf("processor calls = %d, want %d", calls, tc.wantCallCount) + } + + if notif.Status != tc.wantStatus { + t.Errorf("status = %q, want %q", notif.Status, tc.wantStatus) + } + if notif.Attempts != 1 { + t.Errorf("attempts = %d, want 1 (one delivery attempt was made)", notif.Attempts) + } + + stats, err := nq.GetQueueStats() + if err != nil { + t.Fatalf("GetQueueStats: %v", err) + } + if stats[string(tc.wantStatus)] != 1 { + t.Errorf("persisted queue stats = %#v, want one row in %q", stats, tc.wantStatus) + } + }) + } +} diff --git a/internal/notifications/queue.go b/internal/notifications/queue.go index 735cc92d7..ef311465c 100644 --- a/internal/notifications/queue.go +++ b/internal/notifications/queue.go @@ -1767,8 +1767,10 @@ func (nq *NotificationQueue) processNotification(notif *QueuedNotification) { success := err == nil errorMsg := "" + failureClass := NotificationFailureClass("") if err != nil { errorMsg = err.Error() + failureClass = ClassifyNotificationFailureError(err) } if success { @@ -1801,8 +1803,12 @@ func (nq *NotificationQueue) processNotification(notif *QueuedNotification) { Int("maxAttempts", notif.MaxAttempts). Msg("Notification sent successfully") } else { - // Check if we should retry or move to DLQ - if notif.Attempts >= notif.MaxAttempts { + // Check if we should retry or move to DLQ. A deterministic failure + // class is dead-lettered on the spot: retrying an authentication, + // configuration, or rejection verdict cannot change the answer, and + // only delays the dead letter the operator needs to act on. + retryable := failureClass.Retryable() + if notif.Attempts >= notif.MaxAttempts || !retryable { // Move to DLQ if dlqErr := nq.MoveToDLQ(notif.ID, errorMsg); dlqErr != nil { log.Error(). @@ -1823,6 +1829,10 @@ func (nq *NotificationQueue) processNotification(notif *QueuedNotification) { operationaltrust.NotificationDeadLetter, completedAt, ) + deadLetterReason := "max_retries_exhausted" + if !retryable { + deadLetterReason = "failure_class_not_retryable" + } log.Warn(). Str("component", "notification_queue"). Str("action", "move_to_dlq"). @@ -1830,8 +1840,10 @@ func (nq *NotificationQueue) processNotification(notif *QueuedNotification) { Str("type", notif.Type). Int("attempts", notif.Attempts). Int("maxAttempts", notif.MaxAttempts). + Str("failureClass", string(failureClass)). + Str("deadLetterReason", deadLetterReason). Str("error", errorMsg). - Msg("notification moved to DLQ after max retries") + Msg("notification moved to DLQ") } } else { // Schedule retry