Merge pull request #1879 from rcourtman/claude/notification-retry-class-gate

Stop retrying notification failures that cannot succeed

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.

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
+	}
+}

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)
+			}
+		})
+	}
+}

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