From 80b652579bf509c7c8ac4036eb2fbe4f8f200e52 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 12:44:39 +0000 Subject: [PATCH 1/3] feat(errors): add shared error categorization and BatchResult primitives Introduces internal/shared/errors and internal/shared/batch as the foundation for issue #371. Errors can now be tagged with Fatal / Operational / Business categories that propagate through the standard fmt.Errorf %w chain, and IsRetryable centralizes the retry decision. Wires the new primitives into the reminder pipeline: - Notification domain sentinels are categorized in errors.go. - ProcessReminders returns (*batch.BatchResult, error) so callers can see per-item outcomes instead of inferring partial success from logs. - retryWithBackoff short-circuits on non-retryable errors and no longer trips the circuit breaker on Business/Fatal failures. - cmd/reminder logs the structured BatchResult counts. Out of scope for this PR (deferred to follow-up issues): wrapping cleanup in encryption/storage/message domains, metrics + dashboards, and rollout of the retry helper to other domains. Refs #371 --- app/cmd/reminder/reminder.go | 9 +- .../domains/notification/domain/errors.go | 86 ++++++++----- .../notification/domain/reminder_service.go | 85 ++++++++----- .../domain/reminder_service_test.go | 115 ++++++++++++++++-- .../domains/notification/domain/service.go | 3 +- .../notification/ports/primary/service.go | 9 +- .../reminder_flow_integration_test.go | 11 +- app/internal/shared/batch/result.go | 62 ++++++++++ app/internal/shared/batch/result_test.go | 61 ++++++++++ app/internal/shared/errors/category.go | 97 +++++++++++++++ app/internal/shared/errors/category_test.go | 69 +++++++++++ 11 files changed, 529 insertions(+), 78 deletions(-) create mode 100644 app/internal/shared/batch/result.go create mode 100644 app/internal/shared/batch/result_test.go create mode 100644 app/internal/shared/errors/category.go create mode 100644 app/internal/shared/errors/category_test.go diff --git a/app/cmd/reminder/reminder.go b/app/cmd/reminder/reminder.go index d07e03e83..b7807f0f4 100644 --- a/app/cmd/reminder/reminder.go +++ b/app/cmd/reminder/reminder.go @@ -184,15 +184,22 @@ PASSWORDEXCHANGE_REMINDER_INTERVAL: Hours between reminders (1-720, default: 24) // Process reminders ctx := context.Background() - if err := reminderService.ProcessReminders(ctx, reminderConfig); err != nil { + result, err := reminderService.ProcessReminders(ctx, reminderConfig) + if err != nil { logging.Error(). Err(err). + Int("processedCount", result.SuccessCount). + Int("errorCount", result.FailureCount). + Bool("operationalFailures", result.HasOperationalFailures()). Str("operation", "process_reminders"). Msg("Failed to process reminders") return } logging.Info(). + Int("processedCount", result.SuccessCount). + Int("errorCount", result.FailureCount). + Int("totalProcessed", result.TotalProcessed). Str("operation", "processing_completed"). Msg("Reminder email processing completed") diff --git a/app/internal/domains/notification/domain/errors.go b/app/internal/domains/notification/domain/errors.go index 19a84c012..c460de57f 100644 --- a/app/internal/domains/notification/domain/errors.go +++ b/app/internal/domains/notification/domain/errors.go @@ -1,35 +1,61 @@ package domain -import "errors" +import ( + "errors" -var ( - // ErrInvalidEmailAddress indicates the email address is malformed - ErrInvalidEmailAddress = errors.New("invalid email address") - - // ErrEmailSendFailed indicates the email sending operation failed - ErrEmailSendFailed = errors.New("failed to send email") - - // ErrQueueConnectionFailed indicates queue connection failed - ErrQueueConnectionFailed = errors.New("failed to connect to message queue") - - // ErrQueueConsumeFailed indicates queue consumption failed - ErrQueueConsumeFailed = errors.New("failed to consume from message queue") - - // ErrTemplateRenderFailed indicates template rendering failed - ErrTemplateRenderFailed = errors.New("failed to render notification template") - - // ErrTemplateNotFound indicates the requested template was not found - ErrTemplateNotFound = errors.New("notification template not found") - - // ErrInvalidNotificationRequest indicates the notification request is invalid - ErrInvalidNotificationRequest = errors.New("invalid notification request") - - // ErrSMTPAuthFailed indicates SMTP authentication failed - ErrSMTPAuthFailed = errors.New("SMTP authentication failed") - - // ErrMessageUnmarshalFailed indicates message unmarshaling failed - ErrMessageUnmarshalFailed = errors.New("failed to unmarshal queue message") + sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" +) - // ErrEmptyMessageBody indicates the message body is empty - ErrEmptyMessageBody = errors.New("message body is empty") +var ( + // ErrInvalidEmailAddress indicates the email address is malformed. + // Business: bad input, not retryable. + ErrInvalidEmailAddress = sherr.WithCategory( + errors.New("invalid email address"), sherr.CategoryBusiness) + + // ErrEmailSendFailed indicates the email sending operation failed. + // Operational: typically transient SMTP issue. + ErrEmailSendFailed = sherr.WithCategory( + errors.New("failed to send email"), sherr.CategoryOperational) + + // ErrQueueConnectionFailed indicates queue connection failed. + // Operational: transient broker availability. + ErrQueueConnectionFailed = sherr.WithCategory( + errors.New("failed to connect to message queue"), sherr.CategoryOperational) + + // ErrQueueConsumeFailed indicates queue consumption failed. + // Operational: transient broker issue. + ErrQueueConsumeFailed = sherr.WithCategory( + errors.New("failed to consume from message queue"), sherr.CategoryOperational) + + // ErrTemplateRenderFailed indicates template rendering failed. + // Operational: transient I/O reading a template file. + ErrTemplateRenderFailed = sherr.WithCategory( + errors.New("failed to render notification template"), sherr.CategoryOperational) + + // ErrTemplateNotFound indicates the requested template was not found. + // Fatal: missing template is a deploy/config problem; retrying will not help. + ErrTemplateNotFound = sherr.WithCategory( + errors.New("notification template not found"), sherr.CategoryFatal) + + // ErrInvalidNotificationRequest indicates the notification request is invalid. + // Business: validation failure, not retryable. + ErrInvalidNotificationRequest = sherr.WithCategory( + errors.New("invalid notification request"), sherr.CategoryBusiness) + + // ErrSMTPAuthFailed indicates SMTP authentication failed. + // Operational: kept retryable because credential rotation often resolves + // transiently; if it persists, alerting via repeated failures is preferred + // over fail-fast here. + ErrSMTPAuthFailed = sherr.WithCategory( + errors.New("SMTP authentication failed"), sherr.CategoryOperational) + + // ErrMessageUnmarshalFailed indicates message unmarshaling failed. + // Business: malformed input is not a retryable condition. + ErrMessageUnmarshalFailed = sherr.WithCategory( + errors.New("failed to unmarshal queue message"), sherr.CategoryBusiness) + + // ErrEmptyMessageBody indicates the message body is empty. + // Business: validation failure. + ErrEmptyMessageBody = sherr.WithCategory( + errors.New("message body is empty"), sherr.CategoryBusiness) ) diff --git a/app/internal/domains/notification/domain/reminder_service.go b/app/internal/domains/notification/domain/reminder_service.go index 1ca453a68..91118ff08 100644 --- a/app/internal/domains/notification/domain/reminder_service.go +++ b/app/internal/domains/notification/domain/reminder_service.go @@ -4,18 +4,27 @@ import ( "context" "errors" "fmt" + "strconv" "time" "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/ports/secondary" + "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" + sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" ) // Error constants for reminder processing var ( - ErrInvalidCheckAfterHours = errors.New("checkAfterHours must be between 1 and 8760 hours") - ErrInvalidMaxReminders = errors.New("maxReminders must be between 1 and 10") - ErrInvalidReminderInterval = errors.New("reminderInterval must be between 1 and 720 hours") - ErrCircuitBreakerOpen = errors.New("circuit breaker is open") - ErrMaxRetriesExceeded = errors.New("maximum retries exceeded") + ErrInvalidCheckAfterHours = sherr.WithCategory( + errors.New("checkAfterHours must be between 1 and 8760 hours"), sherr.CategoryBusiness) + ErrInvalidMaxReminders = sherr.WithCategory( + errors.New("maxReminders must be between 1 and 10"), sherr.CategoryBusiness) + ErrInvalidReminderInterval = sherr.WithCategory( + errors.New("reminderInterval must be between 1 and 720 hours"), sherr.CategoryBusiness) + // ErrCircuitBreakerOpen is operational: the breaker will close again once + // downstream recovers, and retrying the same item is pointless until then. + ErrCircuitBreakerOpen = sherr.WithCategory( + errors.New("circuit breaker is open"), sherr.CategoryOperational) + ErrMaxRetriesExceeded = errors.New("maximum retries exceeded") ) // Validation constants for reminder configuration @@ -100,11 +109,17 @@ func NewReminderService( } } -// ProcessReminders finds and processes messages eligible for reminder emails -func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig ReminderConfig) error { +// ProcessReminders finds and processes messages eligible for reminder emails. +// It returns a BatchResult describing per-item outcomes alongside any +// top-level error (config validation, fetch failure). When at least one +// message was processed successfully the error is nil even if other items +// failed — callers inspect the BatchResult to report partial success. +func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig ReminderConfig) (*batch.BatchResult, error) { + result := batch.NewBatchResult() + // Check context cancellation early if err := ctx.Err(); err != nil { - return err + return result, err } // Validate configuration @@ -116,12 +131,12 @@ func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig R Int("maxReminders", reminderConfig.MaxReminders). Int("reminderInterval", reminderConfig.Interval). Msg("Invalid reminder configuration") - return err + return result, err } if !reminderConfig.Enabled { r.logger.Info().Bool("enabled", false).Msg("Reminder system is disabled") - return nil + return result, nil } r.logger.Info(). @@ -145,21 +160,21 @@ func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig R return err }, "get_unviewed_messages") if err != nil { - return fmt.Errorf("failed to get unviewed messages: %w", err) + return result, fmt.Errorf("failed to get unviewed messages: %w", err) } r.logger.Info().Int("count", len(messages)).Msg("Found messages eligible for reminders") if len(messages) == 0 { r.logger.Info().Msg("No messages found requiring reminders") - return nil + return result, nil } // Process each message with individual error recovery // Strategy: Continue processing other messages even if some fail (graceful degradation) - processedCount := 0 - errorCount := 0 for _, message := range messages { + itemID := strconv.Itoa(message.MessageID) + // Create reminder request reminderReq := ReminderRequest{ MessageID: message.MessageID, @@ -174,46 +189,47 @@ func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig R return r.ProcessMessageReminder(ctx, reminderReq) }, fmt.Sprintf("process_message_%d", message.MessageID)) if err != nil { - errorCount++ + result.RecordFailure(itemID, err) r.logger.Error(). Err(err). Int("messageID", message.MessageID). Str("email", message.RecipientEmail). Int("daysOld", message.DaysOld). + Str("category", sherr.CategoryOf(err).String()). Str("operation", "process_reminder"). Msg("Failed to process reminder for message after all retry attempts") continue // Continue processing other messages } - processedCount++ + result.RecordSuccess(itemID) } r.logger.Info(). Int("totalMessages", len(messages)). - Int("processedCount", processedCount). - Int("errorCount", errorCount). + Int("processedCount", result.SuccessCount). + Int("errorCount", result.FailureCount). Msg("Reminder processing completed") // Implement graceful degradation: return success if at least some messages were processed // This allows partial success rather than all-or-nothing failure - if processedCount > 0 { + if result.SuccessCount > 0 { r.logger.Info(). - Int("processedCount", processedCount). - Int("errorCount", errorCount). - Float64("successRate", float64(processedCount)/float64(len(messages))*100). + Int("processedCount", result.SuccessCount). + Int("errorCount", result.FailureCount). + Float64("successRate", float64(result.SuccessCount)/float64(len(messages))*100). Msg("Reminder processing completed with partial success") - return nil + return result, nil } // If no messages were processed and we had errors, this indicates a more serious issue - if errorCount > 0 { + if result.FailureCount > 0 { r.logger.Error(). - Int("errorCount", errorCount). + Int("errorCount", result.FailureCount). Int("totalMessages", len(messages)). Msg("Failed to process any reminder messages") - return fmt.Errorf("failed to process any of %d reminder messages", len(messages)) + return result, fmt.Errorf("failed to process any of %d reminder messages", len(messages)) } - return nil + return result, nil } // ProcessMessageReminder sends a reminder email for a specific message @@ -341,6 +357,19 @@ func (r *ReminderService) retryWithBackoff(ctx context.Context, operation func() } lastErr = err + + // Fail fast on non-retryable errors (Business/Fatal/Unknown). The + // circuit breaker only tracks transient infrastructure problems, so + // don't penalize it for a user-supplied invalid input. + if !sherr.IsRetryable(err) && sherr.CategoryOf(err) != sherr.CategoryUnknown { + r.logger.Debug(). + Err(err). + Str("operation", operationName). + Str("category", sherr.CategoryOf(err).String()). + Msg("Operation failed with non-retryable error; skipping retry") + return err + } + r.circuitBreaker.RecordFailure() // Don't retry on last attempt @@ -375,7 +404,7 @@ func (r *ReminderService) retryWithBackoff(ctx context.Context, operation func() Int("maxRetries", MaxRetries). Msg("Operation failed after all retry attempts") - return fmt.Errorf("%w: %s failed after %d attempts: %v", ErrMaxRetriesExceeded, operationName, MaxRetries, lastErr) + return fmt.Errorf("%w: %s failed after %d attempts: %w", ErrMaxRetriesExceeded, operationName, MaxRetries, lastErr) } // validateReminderConfig validates all reminder configuration parameters diff --git a/app/internal/domains/notification/domain/reminder_service_test.go b/app/internal/domains/notification/domain/reminder_service_test.go index fd56fdfbe..e90eafae9 100644 --- a/app/internal/domains/notification/domain/reminder_service_test.go +++ b/app/internal/domains/notification/domain/reminder_service_test.go @@ -253,7 +253,7 @@ func TestProcessReminders_DisabledConfig_ReturnsEarly(t *testing.T) { } // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.NoError(t, err) @@ -277,7 +277,7 @@ func TestProcessReminders_InvalidConfig_ReturnsError(t *testing.T) { } // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.Error(t, err) @@ -303,7 +303,7 @@ func TestProcessReminders_NoMessages_ReturnsSuccess(t *testing.T) { mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return([]*UnviewedMessage{}, nil) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.NoError(t, err) @@ -329,7 +329,7 @@ func TestProcessReminders_StorageError_ReturnsError(t *testing.T) { mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return(nil, storageError) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.Error(t, err) @@ -371,7 +371,7 @@ func TestProcessReminders_SuccessfulProcessing_ReturnsSuccess(t *testing.T) { mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")).Return(nil) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.NoError(t, err) @@ -396,7 +396,7 @@ func TestProcessReminders_ContextCancelled_ReturnsError(t *testing.T) { } // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.Error(t, err) @@ -423,7 +423,7 @@ func TestProcessReminders_StorageTimeout_HandledGracefully(t *testing.T) { mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return(nil, context.DeadlineExceeded) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.Error(t, err) @@ -559,7 +559,7 @@ func TestProcessReminders_LoggingFailure_ContinuesProcessing(t *testing.T) { mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")).Return(nil) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert // Should succeed because at least one message was processed successfully @@ -587,7 +587,7 @@ func TestProcessReminders_CircuitBreakerOpen_StopsProcessing(t *testing.T) { service.circuitBreaker.lastFailureTime = time.Now() // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.Error(t, err) @@ -659,7 +659,7 @@ func TestProcessReminders_MixedResults_ContinuesProcessing(t *testing.T) { })).Return(nil) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert // Should succeed overall despite individual failures @@ -783,7 +783,7 @@ func TestProcessReminders_ReminderInterval_RespectedCorrectly(t *testing.T) { mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")).Return(nil) // Act - err := service.ProcessReminders(ctx, config) + _, err := service.ProcessReminders(ctx, config) // Assert assert.NoError(t, err) @@ -824,3 +824,96 @@ func TestProcessMessageReminder_ValidRequest_Success(t *testing.T) { mockStorageRepo.AssertExpectations(t) mockNotificationPublisher.AssertExpectations(t) } + +// TestProcessReminders_ReturnsBatchResultWithCounts verifies the BatchResult +// contains accurate per-item counters after a mixed-success batch. +func TestProcessReminders_ReturnsBatchResultWithCounts(t *testing.T) { + mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation := createTestMocks() + service := NewReminderService(mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation) + + ctx := context.Background() + config := ReminderConfig{Enabled: true, CheckAfterHours: 24, MaxReminders: 3, Interval: 24} + + messages := []*UnviewedMessage{ + {MessageID: 1, UniqueID: "u1", RecipientEmail: "a@example.com", DaysOld: 2, Created: time.Now().Add(-48 * time.Hour)}, + {MessageID: 2, UniqueID: "u2", RecipientEmail: "b@example.com", DaysOld: 2, Created: time.Now().Add(-48 * time.Hour)}, + } + mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return(messages, nil) + mockStorageRepo.On("GetReminderHistory", ctx, 1).Return([]*ReminderLogEntry{}, nil) + mockStorageRepo.On("GetReminderHistory", ctx, 2).Return([]*ReminderLogEntry{}, nil) + mockStorageRepo.On("LogReminderSent", ctx, 1, "a@example.com").Return(nil) + mockStorageRepo.On("LogReminderSent", ctx, 2, "b@example.com").Return(nil) + mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")).Return(nil) + + result, err := service.ProcessReminders(ctx, config) + + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Equal(t, 2, result.TotalProcessed) + assert.Equal(t, 2, result.SuccessCount) + assert.Equal(t, 0, result.FailureCount) + assert.False(t, result.HasFailures()) +} + +// TestProcessReminders_NonRetryableError_FailsFast verifies that a Business +// error from the publisher is NOT retried — publisher should be called once. +func TestProcessReminders_NonRetryableError_FailsFast(t *testing.T) { + mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation := createTestMocks() + service := NewReminderService(mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation) + + ctx := context.Background() + config := ReminderConfig{Enabled: true, CheckAfterHours: 24, MaxReminders: 3, Interval: 24} + + messages := []*UnviewedMessage{ + {MessageID: 1, UniqueID: "u1", RecipientEmail: "a@example.com", DaysOld: 2, Created: time.Now().Add(-48 * time.Hour)}, + } + mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return(messages, nil) + mockStorageRepo.On("GetReminderHistory", ctx, 1).Return([]*ReminderLogEntry{}, nil) + // ErrInvalidNotificationRequest is categorized Business — must not retry. + mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")). + Return(ErrInvalidNotificationRequest).Once() + + result, err := service.ProcessReminders(ctx, config) + + // All messages failed → top-level error returned. + assert.Error(t, err) + assert.NotNil(t, result) + assert.Equal(t, 1, result.FailureCount) + assert.Equal(t, 0, result.SuccessCount) + mockNotificationPublisher.AssertExpectations(t) + // Critical assertion: publisher invoked exactly once, no retry. + mockNotificationPublisher.AssertNumberOfCalls(t, "PublishNotification", 1) +} + +// TestProcessReminders_OperationalError_RetriesAndRecords verifies that a +// retryable error is retried, and on persistent failure the BatchError +// carries the Operational category derived from the wrapped sentinel. +func TestProcessReminders_OperationalError_RetriesAndRecords(t *testing.T) { + mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation := createTestMocks() + service := NewReminderService(mockStorageRepo, mockNotificationPublisher, mockLogger, mockConfig, mockValidation) + + ctx := context.Background() + config := ReminderConfig{Enabled: true, CheckAfterHours: 24, MaxReminders: 3, Interval: 24} + + messages := []*UnviewedMessage{ + {MessageID: 1, UniqueID: "u1", RecipientEmail: "a@example.com", DaysOld: 2, Created: time.Now().Add(-48 * time.Hour)}, + } + mockStorageRepo.On("GetUnviewedMessagesForReminders", ctx, 24, 3, 24).Return(messages, nil) + mockStorageRepo.On("GetReminderHistory", ctx, 1).Return([]*ReminderLogEntry{}, nil) + // ErrEmailSendFailed is categorized Operational — should be retried. + mockNotificationPublisher.On("PublishNotification", ctx, mock.AnythingOfType("NotificationRequest")). + Return(ErrEmailSendFailed) + + result, err := service.ProcessReminders(ctx, config) + + assert.Error(t, err) // all attempts failed + assert.NotNil(t, result) + assert.Equal(t, 1, result.FailureCount) + assert.Len(t, result.Errors, 1) + assert.True(t, result.HasOperationalFailures(), + "persistent retryable publisher failure should surface as an operational batch failure") + // Publisher should have been invoked MaxRetries times (full retry exhaustion). + mockNotificationPublisher.AssertNumberOfCalls(t, "PublishNotification", MaxRetries) + // Silence unused-import warning if errors pkg is otherwise unused. + _ = errors.New +} diff --git a/app/internal/domains/notification/domain/service.go b/app/internal/domains/notification/domain/service.go index 2cdf0414b..54848107d 100644 --- a/app/internal/domains/notification/domain/service.go +++ b/app/internal/domains/notification/domain/service.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/ports/secondary" + "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" ) // NotificationService provides notification operations @@ -175,7 +176,7 @@ func (s *NotificationService) validateNotificationRequest(req NotificationReques } // ProcessReminders finds and processes messages eligible for reminder emails -func (s *NotificationService) ProcessReminders(ctx context.Context, config ReminderConfig) error { +func (s *NotificationService) ProcessReminders(ctx context.Context, config ReminderConfig) (*batch.BatchResult, error) { return s.reminderService.ProcessReminders(ctx, config) } diff --git a/app/internal/domains/notification/ports/primary/service.go b/app/internal/domains/notification/ports/primary/service.go index 10b2f0189..7e5db5492 100644 --- a/app/internal/domains/notification/ports/primary/service.go +++ b/app/internal/domains/notification/ports/primary/service.go @@ -2,8 +2,9 @@ package primary import ( "context" - + "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/domain" + "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" ) // NotificationServicePort defines the primary port for notification operations @@ -20,8 +21,10 @@ type NotificationServicePort interface { // ReminderServicePort defines the primary port for reminder operations type ReminderServicePort interface { - // ProcessReminders finds and processes messages eligible for reminder emails - ProcessReminders(ctx context.Context, config domain.ReminderConfig) error + // ProcessReminders finds and processes messages eligible for reminder emails. + // Returns a BatchResult describing per-item outcomes plus a top-level error + // for fail-fast conditions (validation, fetch failure, total failure). + ProcessReminders(ctx context.Context, config domain.ReminderConfig) (*batch.BatchResult, error) // ProcessMessageReminder sends a reminder email for a specific message ProcessMessageReminder(ctx context.Context, req domain.ReminderRequest) error diff --git a/app/internal/integration/reminder/reminder_flow_integration_test.go b/app/internal/integration/reminder/reminder_flow_integration_test.go index e210753b8..710c3f0a0 100644 --- a/app/internal/integration/reminder/reminder_flow_integration_test.go +++ b/app/internal/integration/reminder/reminder_flow_integration_test.go @@ -119,7 +119,8 @@ func TestIntegration_ReminderPipeline_EndToEnd(t *testing.T) { reminderCfg := notificationDomain.ReminderConfig{Enabled: true, CheckAfterHours: 24, MaxReminders: 3, Interval: 24} // First run: eligible message reminded, too-recent skipped. - require.NoError(t, svc.ProcessReminders(context.Background(), reminderCfg)) + _, err := svc.ProcessReminders(context.Background(), reminderCfg) + require.NoError(t, err) recipients := publisher.recipients() assert.True(t, recipients["wanted@example.com"], "eligible recipient should receive a reminder") @@ -143,18 +144,20 @@ func TestIntegration_ReminderPipeline_IncrementsOnSecondRun(t *testing.T) { svc, _ := newPipeline(dbCfg) reminderCfg := notificationDomain.ReminderConfig{Enabled: true, CheckAfterHours: 24, MaxReminders: 3, Interval: 1} - require.NoError(t, svc.ProcessReminders(context.Background(), reminderCfg)) + _, err := svc.ProcessReminders(context.Background(), reminderCfg) + require.NoError(t, err) count, _ := reminderRow(t, db, "pipeline-repeat") require.Equal(t, 1, count) // Age the existing reminder so the interval has elapsed for the second run. - _, err := db.Exec(`UPDATE email_reminders er + _, err = db.Exec(`UPDATE email_reminders er JOIN messages m ON m.messageid = er.message_id SET er.last_reminder_sent = NOW() - INTERVAL 5 HOUR WHERE m.uniqueid = ?`, "pipeline-repeat") require.NoError(t, err) - require.NoError(t, svc.ProcessReminders(context.Background(), reminderCfg)) + _, err = svc.ProcessReminders(context.Background(), reminderCfg) + require.NoError(t, err) count, _ = reminderRow(t, db, "pipeline-repeat") assert.Equal(t, 2, count, "second eligible run should increment the reminder count") } diff --git a/app/internal/shared/batch/result.go b/app/internal/shared/batch/result.go new file mode 100644 index 000000000..fbf3fbdf9 --- /dev/null +++ b/app/internal/shared/batch/result.go @@ -0,0 +1,62 @@ +// Package batch provides a shared result type for batch operations that may +// experience partial failures. Callers record per-item outcomes and inspect +// the aggregate for reporting and exit-code decisions. +package batch + +import ( + sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" +) + +// BatchError captures one failed item in a batch along with its categorized +// error, derived from the wrapped sentinel chain via sherr.Category. +type BatchError struct { + ItemID string + Err error + Category sherr.Category +} + +// BatchResult aggregates the outcome of a batch operation. +type BatchResult struct { + TotalProcessed int + SuccessCount int + FailureCount int + Errors []BatchError +} + +// NewBatchResult returns an empty BatchResult ready for use. +func NewBatchResult() *BatchResult { + return &BatchResult{} +} + +// RecordSuccess increments the success counter for the given item. +func (r *BatchResult) RecordSuccess(itemID string) { + r.TotalProcessed++ + r.SuccessCount++ +} + +// RecordFailure appends a categorized failure for the given item. +func (r *BatchResult) RecordFailure(itemID string, err error) { + r.TotalProcessed++ + r.FailureCount++ + r.Errors = append(r.Errors, BatchError{ + ItemID: itemID, + Err: err, + Category: sherr.CategoryOf(err), + }) +} + +// HasFailures reports whether any item failed. +func (r *BatchResult) HasFailures() bool { + return r.FailureCount > 0 +} + +// HasOperationalFailures reports whether any failure was retryable/operational, +// which is the signal monitoring should treat as a system-health concern. +func (r *BatchResult) HasOperationalFailures() bool { + for _, e := range r.Errors { + if e.Category == sherr.CategoryOperational { + return true + } + } + return false +} diff --git a/app/internal/shared/batch/result_test.go b/app/internal/shared/batch/result_test.go new file mode 100644 index 000000000..fa52864d1 --- /dev/null +++ b/app/internal/shared/batch/result_test.go @@ -0,0 +1,61 @@ +package batch + +import ( + stderrors "errors" + "fmt" + "testing" + + sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" + "github.com/stretchr/testify/assert" +) + +var errSentinel = sherr.WithCategory(stderrors.New("transient"), sherr.CategoryOperational) +var errBusiness = sherr.WithCategory(stderrors.New("bad input"), sherr.CategoryBusiness) + +func TestNewBatchResult_Empty(t *testing.T) { + r := NewBatchResult() + assert.Equal(t, 0, r.TotalProcessed) + assert.Equal(t, 0, r.SuccessCount) + assert.Equal(t, 0, r.FailureCount) + assert.False(t, r.HasFailures()) + assert.False(t, r.HasOperationalFailures()) +} + +func TestRecordSuccess_IncrementsCounters(t *testing.T) { + r := NewBatchResult() + r.RecordSuccess("item-1") + r.RecordSuccess("item-2") + assert.Equal(t, 2, r.TotalProcessed) + assert.Equal(t, 2, r.SuccessCount) + assert.Equal(t, 0, r.FailureCount) +} + +func TestRecordFailure_AppendsCategorizedError(t *testing.T) { + r := NewBatchResult() + wrapped := fmt.Errorf("context: %w", errSentinel) + r.RecordFailure("item-1", wrapped) + + assert.Equal(t, 1, r.TotalProcessed) + assert.Equal(t, 1, r.FailureCount) + assert.Equal(t, 0, r.SuccessCount) + assert.Len(t, r.Errors, 1) + assert.Equal(t, "item-1", r.Errors[0].ItemID) + assert.Equal(t, sherr.CategoryOperational, r.Errors[0].Category) + assert.True(t, stderrors.Is(r.Errors[0].Err, errSentinel)) +} + +func TestHasFailures(t *testing.T) { + r := NewBatchResult() + r.RecordSuccess("ok") + assert.False(t, r.HasFailures()) + r.RecordFailure("bad", errBusiness) + assert.True(t, r.HasFailures()) +} + +func TestHasOperationalFailures(t *testing.T) { + r := NewBatchResult() + r.RecordFailure("a", errBusiness) + assert.False(t, r.HasOperationalFailures()) + r.RecordFailure("b", errSentinel) + assert.True(t, r.HasOperationalFailures()) +} diff --git a/app/internal/shared/errors/category.go b/app/internal/shared/errors/category.go new file mode 100644 index 000000000..6a8e97df4 --- /dev/null +++ b/app/internal/shared/errors/category.go @@ -0,0 +1,97 @@ +// Package errors provides shared error categorization primitives used across +// Password Exchange domains. It defines three error categories (Fatal, +// Operational, Business) and helpers that walk the standard fmt.Errorf %w chain +// via errors.As, so domain code can keep using sentinel errors and %w wrapping +// while gaining a single retry/handling vocabulary. +package errors + +import ( + stderrors "errors" +) + +// Category classifies an error for retry and handling decisions. +type Category int + +const ( + // CategoryUnknown is the zero value, used when no category is attached. + CategoryUnknown Category = iota + // CategoryFatal indicates an unrecoverable condition (misconfiguration, + // programmer error, missing dependency). Callers should not retry. + CategoryFatal + // CategoryOperational indicates a transient infrastructure failure + // (network, queue, SMTP timeout). Callers may retry. + CategoryOperational + // CategoryBusiness indicates a domain rule violation or invalid input + // (validation, authorization). Callers should not retry; it is not a + // system failure. + CategoryBusiness +) + +// String returns a stable, lowercase identifier for the category, suitable +// for structured log fields and metrics labels. +func (c Category) String() string { + switch c { + case CategoryFatal: + return "fatal" + case CategoryOperational: + return "operational" + case CategoryBusiness: + return "business" + default: + return "unknown" + } +} + +// Categorized is implemented by errors that carry a Category. +type Categorized interface { + error + Category() Category +} + +type categoryError struct { + err error + category Category +} + +func (e *categoryError) Error() string { return e.err.Error() } +func (e *categoryError) Unwrap() error { return e.err } +func (e *categoryError) Category() Category { return e.category } + +// WithCategory returns err annotated with the given category. The returned +// error implements Unwrap so errors.Is and errors.As continue to work against +// the wrapped error. Passing a nil err returns nil. +func WithCategory(err error, c Category) error { + if err == nil { + return nil + } + return &categoryError{err: err, category: c} +} + +// CategoryOf walks the error chain via errors.As and returns the first +// attached Category. Returns CategoryUnknown if none is found or err is nil. +func CategoryOf(err error) Category { + if err == nil { + return CategoryUnknown + } + var c Categorized + if stderrors.As(err, &c) { + return c.Category() + } + return CategoryUnknown +} + +// IsRetryable reports whether err should be retried. Only operational errors +// are retryable. +func IsRetryable(err error) bool { + return CategoryOf(err) == CategoryOperational +} + +// IsFatal reports whether err is categorized as fatal. +func IsFatal(err error) bool { + return CategoryOf(err) == CategoryFatal +} + +// IsBusiness reports whether err is categorized as a business/validation error. +func IsBusiness(err error) bool { + return CategoryOf(err) == CategoryBusiness +} diff --git a/app/internal/shared/errors/category_test.go b/app/internal/shared/errors/category_test.go new file mode 100644 index 000000000..670e48ccf --- /dev/null +++ b/app/internal/shared/errors/category_test.go @@ -0,0 +1,69 @@ +package errors + +import ( + stderrors "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +var errSentinel = stderrors.New("sentinel") + +func TestWithCategory_NilReturnsNil(t *testing.T) { + assert.Nil(t, WithCategory(nil, CategoryOperational)) +} + +func TestWithCategory_PreservesErrorsIs(t *testing.T) { + wrapped := WithCategory(errSentinel, CategoryOperational) + assert.True(t, stderrors.Is(wrapped, errSentinel)) +} + +func TestWithCategory_PreservesErrorMessage(t *testing.T) { + wrapped := WithCategory(errSentinel, CategoryFatal) + assert.Equal(t, "sentinel", wrapped.Error()) +} + +func TestCategoryOf_DirectlyWrapped(t *testing.T) { + wrapped := WithCategory(errSentinel, CategoryOperational) + assert.Equal(t, CategoryOperational, CategoryOf(wrapped)) +} + +func TestCategoryOf_WalksFmtErrorfChain(t *testing.T) { + wrapped := WithCategory(errSentinel, CategoryOperational) + outer := fmt.Errorf("outer context: %w", wrapped) + doubled := fmt.Errorf("more: %w", outer) + assert.Equal(t, CategoryOperational, CategoryOf(doubled)) +} + +func TestCategoryOf_NilReturnsUnknown(t *testing.T) { + assert.Equal(t, CategoryUnknown, CategoryOf(nil)) +} + +func TestCategoryOf_UncategorizedReturnsUnknown(t *testing.T) { + assert.Equal(t, CategoryUnknown, CategoryOf(errSentinel)) +} + +func TestIsRetryable_OnlyOperational(t *testing.T) { + cases := []struct { + cat Category + want bool + }{ + {CategoryUnknown, false}, + {CategoryFatal, false}, + {CategoryOperational, true}, + {CategoryBusiness, false}, + } + for _, tc := range cases { + err := WithCategory(errSentinel, tc.cat) + assert.Equal(t, tc.want, IsRetryable(err), "cat=%d", tc.cat) + } + assert.False(t, IsRetryable(nil)) +} + +func TestIsFatalAndIsBusiness(t *testing.T) { + assert.True(t, IsFatal(WithCategory(errSentinel, CategoryFatal))) + assert.False(t, IsFatal(WithCategory(errSentinel, CategoryOperational))) + assert.True(t, IsBusiness(WithCategory(errSentinel, CategoryBusiness))) + assert.False(t, IsBusiness(WithCategory(errSentinel, CategoryFatal))) +} From 39376b56504597cc390753755ff7016608c830c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 12:49:41 +0000 Subject: [PATCH 2/3] docs(claude): document shared error categorization and BatchResult --- CLAUDE.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 936121b25..a176f0825 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -147,9 +147,27 @@ protoc --proto_path=protos \ - `config/`: Viper-based config loading - `logging/`: Slog logger setup - `validation/`: Input validation utilities +- `errors/`: Error categorization primitives (`Category`, `WithCategory`, `CategoryOf`, `IsRetryable`) +- `batch/`: `BatchResult` for reporting partial success/failure across items `app/pkg/clients/` contains the email notification client (separate from gRPC secondary adapters in domain layer). +### Error Handling Patterns + +The shared `errors` package defines three categories that propagate through standard `fmt.Errorf %w` chains via `errors.As`: +- `CategoryFatal`: misconfiguration / missing dependency — do not retry +- `CategoryOperational`: transient infra (network, queue, SMTP) — retryable +- `CategoryBusiness`: validation / domain rule — do not retry, not a system failure + +Categorize domain sentinels at declaration so retry helpers and callers share one vocabulary: +```go +ErrEmailSendFailed = sherr.WithCategory( + errors.New("failed to send email"), sherr.CategoryOperational) +``` +`WithCategory` implements `Unwrap` so existing `errors.Is` checks keep working. Always wrap with `%w` (not `%v`) so the category survives the chain. + +For batch operations, use `batch.BatchResult` (`RecordSuccess` / `RecordFailure` / `HasOperationalFailures`) instead of ad-hoc counters. The reminder pipeline (`internal/domains/notification/domain/reminder_service.go::ProcessReminders`) is the reference implementation. Retry loops should gate on `sherr.IsRetryable(err)` to fail fast on Business/Fatal errors. + ## Hexagonal Architecture Patterns ### Domain Layer (`domain/`) From 131a2c22e389c84911e6cdef00a52fabaa0da1b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 13:17:49 +0000 Subject: [PATCH 3/3] refactor(notification): keep BatchResult local; drop shared error categorization Reverts the premature shared/errors and shared/batch packages from the previous commit. There is only one batch processor in the app today (reminder cronjob), so categorization machinery and a generic BatchResult are abstractions without a second consumer. - Moves BatchResult into notification/domain/batch_result.go with a HasInfraFailures() check used by the cronjob for alerting. - Replaces the Category type / IsRetryable helper with a small list of non-retryable sentinels checked via errors.Is in retryWithBackoff. - Reverts notification sentinels in errors.go to plain errors.New. - Updates CLAUDE.md to describe the local pattern rather than a shared one. Promote to a shared package only when a second consumer appears. The behavior (fail-fast on validation errors, structured per-item result for the cronjob) is preserved. --- CLAUDE.md | 18 +--- app/cmd/reminder/reminder.go | 2 +- .../notification/domain/batch_result.go | 98 +++++++++++++++++++ .../domains/notification/domain/errors.go | 48 +++------ .../notification/domain/reminder_service.go | 32 +++--- .../domain/reminder_service_test.go | 4 +- .../domains/notification/domain/service.go | 3 +- .../notification/ports/primary/service.go | 11 +-- app/internal/shared/batch/result.go | 62 ------------ app/internal/shared/batch/result_test.go | 61 ------------ app/internal/shared/errors/category.go | 97 ------------------ app/internal/shared/errors/category_test.go | 69 ------------- 12 files changed, 131 insertions(+), 374 deletions(-) create mode 100644 app/internal/domains/notification/domain/batch_result.go delete mode 100644 app/internal/shared/batch/result.go delete mode 100644 app/internal/shared/batch/result_test.go delete mode 100644 app/internal/shared/errors/category.go delete mode 100644 app/internal/shared/errors/category_test.go diff --git a/CLAUDE.md b/CLAUDE.md index a176f0825..31d32c8d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -147,26 +147,12 @@ protoc --proto_path=protos \ - `config/`: Viper-based config loading - `logging/`: Slog logger setup - `validation/`: Input validation utilities -- `errors/`: Error categorization primitives (`Category`, `WithCategory`, `CategoryOf`, `IsRetryable`) -- `batch/`: `BatchResult` for reporting partial success/failure across items `app/pkg/clients/` contains the email notification client (separate from gRPC secondary adapters in domain layer). -### Error Handling Patterns +### Reminder Batch Reporting -The shared `errors` package defines three categories that propagate through standard `fmt.Errorf %w` chains via `errors.As`: -- `CategoryFatal`: misconfiguration / missing dependency — do not retry -- `CategoryOperational`: transient infra (network, queue, SMTP) — retryable -- `CategoryBusiness`: validation / domain rule — do not retry, not a system failure - -Categorize domain sentinels at declaration so retry helpers and callers share one vocabulary: -```go -ErrEmailSendFailed = sherr.WithCategory( - errors.New("failed to send email"), sherr.CategoryOperational) -``` -`WithCategory` implements `Unwrap` so existing `errors.Is` checks keep working. Always wrap with `%w` (not `%v`) so the category survives the chain. - -For batch operations, use `batch.BatchResult` (`RecordSuccess` / `RecordFailure` / `HasOperationalFailures`) instead of ad-hoc counters. The reminder pipeline (`internal/domains/notification/domain/reminder_service.go::ProcessReminders`) is the reference implementation. Retry loops should gate on `sherr.IsRetryable(err)` to fail fast on Business/Fatal errors. +The reminder pipeline is the only batch processor in the app. `ProcessReminders` (`internal/domains/notification/domain/reminder_service.go`) returns a `*BatchResult` describing per-item outcomes; the cronjob uses `HasInfraFailures()` to distinguish transient infrastructure problems from invalid-input failures for alerting. The retry loop short-circuits on a small list of non-retryable sentinels (validation errors, missing template) so retries aren't wasted on errors guaranteed to keep failing. Both lists live in `batch_result.go`; promote to a shared package only when a second consumer needs the same pattern. ## Hexagonal Architecture Patterns diff --git a/app/cmd/reminder/reminder.go b/app/cmd/reminder/reminder.go index b7807f0f4..9586e8454 100644 --- a/app/cmd/reminder/reminder.go +++ b/app/cmd/reminder/reminder.go @@ -190,7 +190,7 @@ PASSWORDEXCHANGE_REMINDER_INTERVAL: Hours between reminders (1-720, default: 24) Err(err). Int("processedCount", result.SuccessCount). Int("errorCount", result.FailureCount). - Bool("operationalFailures", result.HasOperationalFailures()). + Bool("infraFailures", result.HasInfraFailures()). Str("operation", "process_reminders"). Msg("Failed to process reminders") return diff --git a/app/internal/domains/notification/domain/batch_result.go b/app/internal/domains/notification/domain/batch_result.go new file mode 100644 index 000000000..e6bcd6c92 --- /dev/null +++ b/app/internal/domains/notification/domain/batch_result.go @@ -0,0 +1,98 @@ +// Package domain – reminder batch reporting. +// +// BatchResult records per-item outcomes from ProcessReminders so callers can +// distinguish total failure from partial success, and infrastructure failures +// from invalid-input failures. Lives in this package because the reminder +// pipeline is the only batch processor in the app today; promote to a shared +// package only when a second consumer appears. +package domain + +import stderrors "errors" + +// BatchError captures one failed item in a reminder batch. +type BatchError struct { + ItemID string + Err error +} + +// BatchResult aggregates the outcome of a reminder batch. +type BatchResult struct { + TotalProcessed int + SuccessCount int + FailureCount int + Errors []BatchError +} + +// NewBatchResult returns an empty BatchResult. +func NewBatchResult() *BatchResult { return &BatchResult{} } + +// RecordSuccess increments the success counter. +func (r *BatchResult) RecordSuccess(itemID string) { + r.TotalProcessed++ + r.SuccessCount++ +} + +// RecordFailure appends a failure entry. +func (r *BatchResult) RecordFailure(itemID string, err error) { + r.TotalProcessed++ + r.FailureCount++ + r.Errors = append(r.Errors, BatchError{ItemID: itemID, Err: err}) +} + +// HasFailures reports whether any item failed. +func (r *BatchResult) HasFailures() bool { return r.FailureCount > 0 } + +// HasInfraFailures reports whether any failure looks like a transient +// infrastructure problem (queue, SMTP, storage). Useful for alerting: +// validation failures are not a system health concern, infra failures are. +func (r *BatchResult) HasInfraFailures() bool { + for _, e := range r.Errors { + if isInfraError(e.Err) { + return true + } + } + return false +} + +// nonRetryableSentinels are domain errors where retrying is guaranteed to +// keep failing — validation problems and missing-config conditions. Anything +// not in this list is treated as transient and eligible for retry. +var nonRetryableSentinels = []error{ + ErrInvalidEmailAddress, + ErrInvalidNotificationRequest, + ErrMessageUnmarshalFailed, + ErrEmptyMessageBody, + ErrTemplateNotFound, + ErrInvalidCheckAfterHours, + ErrInvalidMaxReminders, + ErrInvalidReminderInterval, +} + +// isRetryable reports whether retryWithBackoff should attempt err again. +func isRetryable(err error) bool { + for _, s := range nonRetryableSentinels { + if stderrors.Is(err, s) { + return false + } + } + return true +} + +// infraSentinels are domain errors that indicate transient infrastructure +// problems worth surfacing to monitoring. +var infraSentinels = []error{ + ErrEmailSendFailed, + ErrQueueConnectionFailed, + ErrQueueConsumeFailed, + ErrTemplateRenderFailed, + ErrSMTPAuthFailed, +} + +func isInfraError(err error) bool { + for _, s := range infraSentinels { + if stderrors.Is(err, s) { + return true + } + } + return false +} diff --git a/app/internal/domains/notification/domain/errors.go b/app/internal/domains/notification/domain/errors.go index c460de57f..74fdc9260 100644 --- a/app/internal/domains/notification/domain/errors.go +++ b/app/internal/domains/notification/domain/errors.go @@ -1,61 +1,35 @@ package domain -import ( - "errors" - - sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" -) +import "errors" var ( // ErrInvalidEmailAddress indicates the email address is malformed. - // Business: bad input, not retryable. - ErrInvalidEmailAddress = sherr.WithCategory( - errors.New("invalid email address"), sherr.CategoryBusiness) + ErrInvalidEmailAddress = errors.New("invalid email address") // ErrEmailSendFailed indicates the email sending operation failed. - // Operational: typically transient SMTP issue. - ErrEmailSendFailed = sherr.WithCategory( - errors.New("failed to send email"), sherr.CategoryOperational) + ErrEmailSendFailed = errors.New("failed to send email") // ErrQueueConnectionFailed indicates queue connection failed. - // Operational: transient broker availability. - ErrQueueConnectionFailed = sherr.WithCategory( - errors.New("failed to connect to message queue"), sherr.CategoryOperational) + ErrQueueConnectionFailed = errors.New("failed to connect to message queue") // ErrQueueConsumeFailed indicates queue consumption failed. - // Operational: transient broker issue. - ErrQueueConsumeFailed = sherr.WithCategory( - errors.New("failed to consume from message queue"), sherr.CategoryOperational) + ErrQueueConsumeFailed = errors.New("failed to consume from message queue") // ErrTemplateRenderFailed indicates template rendering failed. - // Operational: transient I/O reading a template file. - ErrTemplateRenderFailed = sherr.WithCategory( - errors.New("failed to render notification template"), sherr.CategoryOperational) + ErrTemplateRenderFailed = errors.New("failed to render notification template") // ErrTemplateNotFound indicates the requested template was not found. - // Fatal: missing template is a deploy/config problem; retrying will not help. - ErrTemplateNotFound = sherr.WithCategory( - errors.New("notification template not found"), sherr.CategoryFatal) + ErrTemplateNotFound = errors.New("notification template not found") // ErrInvalidNotificationRequest indicates the notification request is invalid. - // Business: validation failure, not retryable. - ErrInvalidNotificationRequest = sherr.WithCategory( - errors.New("invalid notification request"), sherr.CategoryBusiness) + ErrInvalidNotificationRequest = errors.New("invalid notification request") // ErrSMTPAuthFailed indicates SMTP authentication failed. - // Operational: kept retryable because credential rotation often resolves - // transiently; if it persists, alerting via repeated failures is preferred - // over fail-fast here. - ErrSMTPAuthFailed = sherr.WithCategory( - errors.New("SMTP authentication failed"), sherr.CategoryOperational) + ErrSMTPAuthFailed = errors.New("SMTP authentication failed") // ErrMessageUnmarshalFailed indicates message unmarshaling failed. - // Business: malformed input is not a retryable condition. - ErrMessageUnmarshalFailed = sherr.WithCategory( - errors.New("failed to unmarshal queue message"), sherr.CategoryBusiness) + ErrMessageUnmarshalFailed = errors.New("failed to unmarshal queue message") // ErrEmptyMessageBody indicates the message body is empty. - // Business: validation failure. - ErrEmptyMessageBody = sherr.WithCategory( - errors.New("message body is empty"), sherr.CategoryBusiness) + ErrEmptyMessageBody = errors.New("message body is empty") ) diff --git a/app/internal/domains/notification/domain/reminder_service.go b/app/internal/domains/notification/domain/reminder_service.go index 91118ff08..8b6760191 100644 --- a/app/internal/domains/notification/domain/reminder_service.go +++ b/app/internal/domains/notification/domain/reminder_service.go @@ -8,23 +8,15 @@ import ( "time" "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/ports/secondary" - "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" - sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" ) // Error constants for reminder processing var ( - ErrInvalidCheckAfterHours = sherr.WithCategory( - errors.New("checkAfterHours must be between 1 and 8760 hours"), sherr.CategoryBusiness) - ErrInvalidMaxReminders = sherr.WithCategory( - errors.New("maxReminders must be between 1 and 10"), sherr.CategoryBusiness) - ErrInvalidReminderInterval = sherr.WithCategory( - errors.New("reminderInterval must be between 1 and 720 hours"), sherr.CategoryBusiness) - // ErrCircuitBreakerOpen is operational: the breaker will close again once - // downstream recovers, and retrying the same item is pointless until then. - ErrCircuitBreakerOpen = sherr.WithCategory( - errors.New("circuit breaker is open"), sherr.CategoryOperational) - ErrMaxRetriesExceeded = errors.New("maximum retries exceeded") + ErrInvalidCheckAfterHours = errors.New("checkAfterHours must be between 1 and 8760 hours") + ErrInvalidMaxReminders = errors.New("maxReminders must be between 1 and 10") + ErrInvalidReminderInterval = errors.New("reminderInterval must be between 1 and 720 hours") + ErrCircuitBreakerOpen = errors.New("circuit breaker is open") + ErrMaxRetriesExceeded = errors.New("maximum retries exceeded") ) // Validation constants for reminder configuration @@ -114,8 +106,8 @@ func NewReminderService( // top-level error (config validation, fetch failure). When at least one // message was processed successfully the error is nil even if other items // failed — callers inspect the BatchResult to report partial success. -func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig ReminderConfig) (*batch.BatchResult, error) { - result := batch.NewBatchResult() +func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig ReminderConfig) (*BatchResult, error) { + result := NewBatchResult() // Check context cancellation early if err := ctx.Err(); err != nil { @@ -195,7 +187,6 @@ func (r *ReminderService) ProcessReminders(ctx context.Context, reminderConfig R Int("messageID", message.MessageID). Str("email", message.RecipientEmail). Int("daysOld", message.DaysOld). - Str("category", sherr.CategoryOf(err).String()). Str("operation", "process_reminder"). Msg("Failed to process reminder for message after all retry attempts") continue // Continue processing other messages @@ -358,14 +349,13 @@ func (r *ReminderService) retryWithBackoff(ctx context.Context, operation func() lastErr = err - // Fail fast on non-retryable errors (Business/Fatal/Unknown). The - // circuit breaker only tracks transient infrastructure problems, so - // don't penalize it for a user-supplied invalid input. - if !sherr.IsRetryable(err) && sherr.CategoryOf(err) != sherr.CategoryUnknown { + // Fail fast on validation / missing-config errors. Retrying them is + // guaranteed to keep failing, and we don't want to trip the circuit + // breaker on user-supplied invalid input. + if !isRetryable(err) { r.logger.Debug(). Err(err). Str("operation", operationName). - Str("category", sherr.CategoryOf(err).String()). Msg("Operation failed with non-retryable error; skipping retry") return err } diff --git a/app/internal/domains/notification/domain/reminder_service_test.go b/app/internal/domains/notification/domain/reminder_service_test.go index e90eafae9..b61fdd358 100644 --- a/app/internal/domains/notification/domain/reminder_service_test.go +++ b/app/internal/domains/notification/domain/reminder_service_test.go @@ -910,8 +910,8 @@ func TestProcessReminders_OperationalError_RetriesAndRecords(t *testing.T) { assert.NotNil(t, result) assert.Equal(t, 1, result.FailureCount) assert.Len(t, result.Errors, 1) - assert.True(t, result.HasOperationalFailures(), - "persistent retryable publisher failure should surface as an operational batch failure") + assert.True(t, result.HasInfraFailures(), + "persistent retryable publisher failure should surface as an infra batch failure") // Publisher should have been invoked MaxRetries times (full retry exhaustion). mockNotificationPublisher.AssertNumberOfCalls(t, "PublishNotification", MaxRetries) // Silence unused-import warning if errors pkg is otherwise unused. diff --git a/app/internal/domains/notification/domain/service.go b/app/internal/domains/notification/domain/service.go index 54848107d..b453f07ae 100644 --- a/app/internal/domains/notification/domain/service.go +++ b/app/internal/domains/notification/domain/service.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/ports/secondary" - "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" ) // NotificationService provides notification operations @@ -176,7 +175,7 @@ func (s *NotificationService) validateNotificationRequest(req NotificationReques } // ProcessReminders finds and processes messages eligible for reminder emails -func (s *NotificationService) ProcessReminders(ctx context.Context, config ReminderConfig) (*batch.BatchResult, error) { +func (s *NotificationService) ProcessReminders(ctx context.Context, config ReminderConfig) (*BatchResult, error) { return s.reminderService.ProcessReminders(ctx, config) } diff --git a/app/internal/domains/notification/ports/primary/service.go b/app/internal/domains/notification/ports/primary/service.go index 7e5db5492..283e5e697 100644 --- a/app/internal/domains/notification/ports/primary/service.go +++ b/app/internal/domains/notification/ports/primary/service.go @@ -4,17 +4,16 @@ import ( "context" "github.com/Anthony-Bible/password-exchange/app/internal/domains/notification/domain" - "github.com/Anthony-Bible/password-exchange/app/internal/shared/batch" ) // NotificationServicePort defines the primary port for notification operations type NotificationServicePort interface { // SendNotification sends a notification using the configured sender SendNotification(ctx context.Context, req domain.NotificationRequest) (*domain.NotificationResponse, error) - + // StartMessageProcessing starts consuming messages from the queue and processing them StartMessageProcessing(ctx context.Context, queueConn domain.QueueConnection, concurrency int) error - + // Close closes any open connections Close() error } @@ -24,8 +23,8 @@ type ReminderServicePort interface { // ProcessReminders finds and processes messages eligible for reminder emails. // Returns a BatchResult describing per-item outcomes plus a top-level error // for fail-fast conditions (validation, fetch failure, total failure). - ProcessReminders(ctx context.Context, config domain.ReminderConfig) (*batch.BatchResult, error) - + ProcessReminders(ctx context.Context, config domain.ReminderConfig) (*domain.BatchResult, error) + // ProcessMessageReminder sends a reminder email for a specific message ProcessMessageReminder(ctx context.Context, req domain.ReminderRequest) error -} \ No newline at end of file +} diff --git a/app/internal/shared/batch/result.go b/app/internal/shared/batch/result.go deleted file mode 100644 index fbf3fbdf9..000000000 --- a/app/internal/shared/batch/result.go +++ /dev/null @@ -1,62 +0,0 @@ -// Package batch provides a shared result type for batch operations that may -// experience partial failures. Callers record per-item outcomes and inspect -// the aggregate for reporting and exit-code decisions. -package batch - -import ( - sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" -) - -// BatchError captures one failed item in a batch along with its categorized -// error, derived from the wrapped sentinel chain via sherr.Category. -type BatchError struct { - ItemID string - Err error - Category sherr.Category -} - -// BatchResult aggregates the outcome of a batch operation. -type BatchResult struct { - TotalProcessed int - SuccessCount int - FailureCount int - Errors []BatchError -} - -// NewBatchResult returns an empty BatchResult ready for use. -func NewBatchResult() *BatchResult { - return &BatchResult{} -} - -// RecordSuccess increments the success counter for the given item. -func (r *BatchResult) RecordSuccess(itemID string) { - r.TotalProcessed++ - r.SuccessCount++ -} - -// RecordFailure appends a categorized failure for the given item. -func (r *BatchResult) RecordFailure(itemID string, err error) { - r.TotalProcessed++ - r.FailureCount++ - r.Errors = append(r.Errors, BatchError{ - ItemID: itemID, - Err: err, - Category: sherr.CategoryOf(err), - }) -} - -// HasFailures reports whether any item failed. -func (r *BatchResult) HasFailures() bool { - return r.FailureCount > 0 -} - -// HasOperationalFailures reports whether any failure was retryable/operational, -// which is the signal monitoring should treat as a system-health concern. -func (r *BatchResult) HasOperationalFailures() bool { - for _, e := range r.Errors { - if e.Category == sherr.CategoryOperational { - return true - } - } - return false -} diff --git a/app/internal/shared/batch/result_test.go b/app/internal/shared/batch/result_test.go deleted file mode 100644 index fa52864d1..000000000 --- a/app/internal/shared/batch/result_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package batch - -import ( - stderrors "errors" - "fmt" - "testing" - - sherr "github.com/Anthony-Bible/password-exchange/app/internal/shared/errors" - "github.com/stretchr/testify/assert" -) - -var errSentinel = sherr.WithCategory(stderrors.New("transient"), sherr.CategoryOperational) -var errBusiness = sherr.WithCategory(stderrors.New("bad input"), sherr.CategoryBusiness) - -func TestNewBatchResult_Empty(t *testing.T) { - r := NewBatchResult() - assert.Equal(t, 0, r.TotalProcessed) - assert.Equal(t, 0, r.SuccessCount) - assert.Equal(t, 0, r.FailureCount) - assert.False(t, r.HasFailures()) - assert.False(t, r.HasOperationalFailures()) -} - -func TestRecordSuccess_IncrementsCounters(t *testing.T) { - r := NewBatchResult() - r.RecordSuccess("item-1") - r.RecordSuccess("item-2") - assert.Equal(t, 2, r.TotalProcessed) - assert.Equal(t, 2, r.SuccessCount) - assert.Equal(t, 0, r.FailureCount) -} - -func TestRecordFailure_AppendsCategorizedError(t *testing.T) { - r := NewBatchResult() - wrapped := fmt.Errorf("context: %w", errSentinel) - r.RecordFailure("item-1", wrapped) - - assert.Equal(t, 1, r.TotalProcessed) - assert.Equal(t, 1, r.FailureCount) - assert.Equal(t, 0, r.SuccessCount) - assert.Len(t, r.Errors, 1) - assert.Equal(t, "item-1", r.Errors[0].ItemID) - assert.Equal(t, sherr.CategoryOperational, r.Errors[0].Category) - assert.True(t, stderrors.Is(r.Errors[0].Err, errSentinel)) -} - -func TestHasFailures(t *testing.T) { - r := NewBatchResult() - r.RecordSuccess("ok") - assert.False(t, r.HasFailures()) - r.RecordFailure("bad", errBusiness) - assert.True(t, r.HasFailures()) -} - -func TestHasOperationalFailures(t *testing.T) { - r := NewBatchResult() - r.RecordFailure("a", errBusiness) - assert.False(t, r.HasOperationalFailures()) - r.RecordFailure("b", errSentinel) - assert.True(t, r.HasOperationalFailures()) -} diff --git a/app/internal/shared/errors/category.go b/app/internal/shared/errors/category.go deleted file mode 100644 index 6a8e97df4..000000000 --- a/app/internal/shared/errors/category.go +++ /dev/null @@ -1,97 +0,0 @@ -// Package errors provides shared error categorization primitives used across -// Password Exchange domains. It defines three error categories (Fatal, -// Operational, Business) and helpers that walk the standard fmt.Errorf %w chain -// via errors.As, so domain code can keep using sentinel errors and %w wrapping -// while gaining a single retry/handling vocabulary. -package errors - -import ( - stderrors "errors" -) - -// Category classifies an error for retry and handling decisions. -type Category int - -const ( - // CategoryUnknown is the zero value, used when no category is attached. - CategoryUnknown Category = iota - // CategoryFatal indicates an unrecoverable condition (misconfiguration, - // programmer error, missing dependency). Callers should not retry. - CategoryFatal - // CategoryOperational indicates a transient infrastructure failure - // (network, queue, SMTP timeout). Callers may retry. - CategoryOperational - // CategoryBusiness indicates a domain rule violation or invalid input - // (validation, authorization). Callers should not retry; it is not a - // system failure. - CategoryBusiness -) - -// String returns a stable, lowercase identifier for the category, suitable -// for structured log fields and metrics labels. -func (c Category) String() string { - switch c { - case CategoryFatal: - return "fatal" - case CategoryOperational: - return "operational" - case CategoryBusiness: - return "business" - default: - return "unknown" - } -} - -// Categorized is implemented by errors that carry a Category. -type Categorized interface { - error - Category() Category -} - -type categoryError struct { - err error - category Category -} - -func (e *categoryError) Error() string { return e.err.Error() } -func (e *categoryError) Unwrap() error { return e.err } -func (e *categoryError) Category() Category { return e.category } - -// WithCategory returns err annotated with the given category. The returned -// error implements Unwrap so errors.Is and errors.As continue to work against -// the wrapped error. Passing a nil err returns nil. -func WithCategory(err error, c Category) error { - if err == nil { - return nil - } - return &categoryError{err: err, category: c} -} - -// CategoryOf walks the error chain via errors.As and returns the first -// attached Category. Returns CategoryUnknown if none is found or err is nil. -func CategoryOf(err error) Category { - if err == nil { - return CategoryUnknown - } - var c Categorized - if stderrors.As(err, &c) { - return c.Category() - } - return CategoryUnknown -} - -// IsRetryable reports whether err should be retried. Only operational errors -// are retryable. -func IsRetryable(err error) bool { - return CategoryOf(err) == CategoryOperational -} - -// IsFatal reports whether err is categorized as fatal. -func IsFatal(err error) bool { - return CategoryOf(err) == CategoryFatal -} - -// IsBusiness reports whether err is categorized as a business/validation error. -func IsBusiness(err error) bool { - return CategoryOf(err) == CategoryBusiness -} diff --git a/app/internal/shared/errors/category_test.go b/app/internal/shared/errors/category_test.go deleted file mode 100644 index 670e48ccf..000000000 --- a/app/internal/shared/errors/category_test.go +++ /dev/null @@ -1,69 +0,0 @@ -package errors - -import ( - stderrors "errors" - "fmt" - "testing" - - "github.com/stretchr/testify/assert" -) - -var errSentinel = stderrors.New("sentinel") - -func TestWithCategory_NilReturnsNil(t *testing.T) { - assert.Nil(t, WithCategory(nil, CategoryOperational)) -} - -func TestWithCategory_PreservesErrorsIs(t *testing.T) { - wrapped := WithCategory(errSentinel, CategoryOperational) - assert.True(t, stderrors.Is(wrapped, errSentinel)) -} - -func TestWithCategory_PreservesErrorMessage(t *testing.T) { - wrapped := WithCategory(errSentinel, CategoryFatal) - assert.Equal(t, "sentinel", wrapped.Error()) -} - -func TestCategoryOf_DirectlyWrapped(t *testing.T) { - wrapped := WithCategory(errSentinel, CategoryOperational) - assert.Equal(t, CategoryOperational, CategoryOf(wrapped)) -} - -func TestCategoryOf_WalksFmtErrorfChain(t *testing.T) { - wrapped := WithCategory(errSentinel, CategoryOperational) - outer := fmt.Errorf("outer context: %w", wrapped) - doubled := fmt.Errorf("more: %w", outer) - assert.Equal(t, CategoryOperational, CategoryOf(doubled)) -} - -func TestCategoryOf_NilReturnsUnknown(t *testing.T) { - assert.Equal(t, CategoryUnknown, CategoryOf(nil)) -} - -func TestCategoryOf_UncategorizedReturnsUnknown(t *testing.T) { - assert.Equal(t, CategoryUnknown, CategoryOf(errSentinel)) -} - -func TestIsRetryable_OnlyOperational(t *testing.T) { - cases := []struct { - cat Category - want bool - }{ - {CategoryUnknown, false}, - {CategoryFatal, false}, - {CategoryOperational, true}, - {CategoryBusiness, false}, - } - for _, tc := range cases { - err := WithCategory(errSentinel, tc.cat) - assert.Equal(t, tc.want, IsRetryable(err), "cat=%d", tc.cat) - } - assert.False(t, IsRetryable(nil)) -} - -func TestIsFatalAndIsBusiness(t *testing.T) { - assert.True(t, IsFatal(WithCategory(errSentinel, CategoryFatal))) - assert.False(t, IsFatal(WithCategory(errSentinel, CategoryOperational))) - assert.True(t, IsBusiness(WithCategory(errSentinel, CategoryBusiness))) - assert.False(t, IsBusiness(WithCategory(errSentinel, CategoryFatal))) -}