diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index fcb1d3e1f..d1179a723 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -10670,3 +10670,14 @@ reasoning and real remediation in `docs/qualification/PATROL_ASSISTANT_CUSTOMER_JOURNEY.md`. The repeatable browser proof is `scripts/check-patrol-assistant-journey.mjs`. A passing scripted response does not establish a useful customer outcome or model qualification. + +### Report email shares the canonical SMTP retry verdict + +Attachment delivery through `internal/notifications/email_enhanced.go` uses +notifications-owned `sendEmailWithOptions`; reporting must not introduce a +separate retry classification. That transport stops on authentication, +configuration, or rejection failures and preserves temporary-failure retries. +Its returned failure retains the structured cause and actual attempt count; +this does not change report API schemas or prove recipient acceptance. +`internal/notifications/email_retry_class_test.go` verifies the shared transport +failure boundary with in-memory SMTP replies, not end-to-end report delivery. diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index e02bed3a8..a18ef9ae8 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -581,6 +581,21 @@ 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`. +The SMTP transport's inner retry loop must apply that same classifier after +an unsuccessful send, before sleeping or attempting another connection. +Permanent authentication, configuration, and rejection failures return on that +attempt; transient failures retain up to `MaxRetries + 1` transport attempts. +The returned error preserves the structured cause and reports the actual +attempt count, not the configured maximum. This applies equally to ordinary, +threaded, and attachment email through `sendEmailWithOptions`. + +`internal/notifications/email_retry_class_test.go` exercises the real sender +with in-memory SMTP handshake failures: permanent 550/535/554/501 replies stop +at one attempt, while transient 421 retains three configured attempts even +when its prose mentions authentication. It checks classification and the +reported attempt count. This is transport-only proof, not queue persistence, +post-DATA acceptance, or an installed recipient receipt. + Dead-lettering early must not lose the notification: `RetryTerminalFailures` remains the operator's recovery path, returning eligible retained terminal failures to the queue with a fresh budget once the credentials or configuration diff --git a/internal/notifications/email_enhanced.go b/internal/notifications/email_enhanced.go index d71045a2c..ffc644fc3 100644 --- a/internal/notifications/email_enhanced.go +++ b/internal/notifications/email_enhanced.go @@ -474,10 +474,10 @@ func NewEnhancedEmailManager(config EmailProviderConfig) *EnhancedEmailManager { // SendEmailWithRetry sends email with retry logic // Note: When used with the persistent queue, retry behavior is layered: -// - Transport retries (this function): up to MaxRetries attempts with RetryDelay between +// - Transport retries (this function): up to MaxRetries+1 attempts with RetryDelay between // - Queue retries: up to MaxAttempts (default 3) with exponential backoff -// Total attempts = MaxRetries * MaxAttempts (e.g., 3 * 3 = 9 SMTP calls for a single notification) -// This ensures delivery even during transient failures at either layer. +// Total attempts = (MaxRetries+1) * MaxAttempts for transient failures. +// Deterministic failures stop at the first attempt in both layers. func (e *EnhancedEmailManager) SendEmailWithRetry(subject, htmlBody, textBody string) error { return e.sendEmailWithOptions(subject, htmlBody, textBody, nil, "") } @@ -528,6 +528,10 @@ func (e *EnhancedEmailManager) sendEmailWithOptions(subject, htmlBody, textBody Int("attempt", attempt). Str("provider", e.config.Provider). Msg("email send attempt failed") + + if !ClassifyNotificationFailureError(err).Retryable() { + return fmt.Errorf("email failed after %d attempts: %w", attempt+1, err) + } } return fmt.Errorf("email failed after %d attempts: %w", e.config.MaxRetries+1, lastErr) diff --git a/internal/notifications/email_retry_class_test.go b/internal/notifications/email_retry_class_test.go new file mode 100644 index 000000000..a3e49d75b --- /dev/null +++ b/internal/notifications/email_retry_class_test.go @@ -0,0 +1,64 @@ +package notifications + +import ( + "fmt" + "net" + "strings" + "sync" + "testing" + "time" +) + +// Exercise the SMTP sender, not just the classifier: its inner retry loop must +// respect structured replies before the outer queue ever sees the error. +func TestEmailRetryRespectsSMTPFailureClass(t *testing.T) { + for _, tc := range []struct { + name string + code int + message string + wantClass NotificationFailureClass + wantAttempts int + }{ + {"rejected despite temporary prose", 550, "temporary timeout", NotificationFailureRejected, 1}, + {"authentication", 535, "credentials invalid", NotificationFailureAuthentication, 1}, + {"transaction rejection", 554, "transaction rejected", NotificationFailureRejected, 1}, + {"configuration", 501, "invalid parameters", NotificationFailureConfiguration, 1}, + {"transient despite authentication prose", 421, "authentication service unavailable", NotificationFailureServerError, 3}, + } { + t.Run(tc.name, func(t *testing.T) { + var servers sync.WaitGroup + originalDial := smtpDialTimeout + t.Cleanup(func() { smtpDialTimeout = originalDial; servers.Wait() }) + attempts := 0 + smtpDialTimeout = func(string, string, time.Duration) (net.Conn, error) { + attempts++ + client, server := net.Pipe() + servers.Add(1) + go func() { + defer servers.Done() + defer server.Close() + _ = server.SetDeadline(time.Now().Add(5 * time.Second)) + _, _ = fmt.Fprintf(server, "%d %s\r\n", tc.code, tc.message) + }() + return client, nil + } + manager := NewEnhancedEmailManager(EmailProviderConfig{ + EmailConfig: EmailConfig{SMTPHost: "smtp.example.test", SMTPPort: 25, From: "pulse@example.test", To: []string{"recipient@example.test"}}, + MaxRetries: 2, + }) + err := manager.SendEmailWithRetry("test", "
test
", "test") + if err == nil { + t.Fatal("expected SMTP failure") + } + if got := ClassifyNotificationFailureError(err); got != tc.wantClass { + t.Errorf("class = %q, want %q", got, tc.wantClass) + } + if !strings.Contains(err.Error(), fmt.Sprintf("email failed after %d attempts:", tc.wantAttempts)) { + t.Errorf("incorrect attempt count in error: %v", err) + } + if attempts != tc.wantAttempts { + t.Errorf("SMTP attempts = %d, want %d", attempts, tc.wantAttempts) + } + }) + } +}