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