fix(service,queue): eliminate data races, add retry backoff, tenant scoping

- Copy discipline for notifications: the store, the queue, workers, and API callers each own clones; no notification object is shared across goroutines (races previously flagged by -race between workers mutating Status/RetryCount and handlers JSON-encoding the same pointer). - Retry progress derives from QueueMessage.Attempt so it survives requeues; exponential backoff (1s base, 30s cap) honors the documented queue.retry_backoff setting instead of hammering failing providers in a tight loop; shutdown abandons pending backoff waits cleanly. - Stop() cancels a service-lifetime context so idle workers blocked in Dequeue exit immediately instead of waiting out their poll timeout. - LocalQueue no longer holds its mutex while sending on the queue channel (Enqueue/Nack) — with a full buffer this deadlocked the entire worker pool, since draining requires the same mutex. - Tenant scoping: notifications are stamped with the caller's ClientID; non-admin clients can only read/cancel/retry their own (cross-tenant access reports not-found to avoid leaking existence). - Sentinel errors ErrNotificationNotFound/ErrNotificationAlreadySent. - New race, backoff, and tenant-scoping test suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 09:15:57 -07:00
parent 90287d5da0
commit 21990f2533
6 changed files with 924 additions and 91 deletions
@@ -17,6 +17,21 @@ type AccountResolver interface {
 	GetDefaultAccount(notifierType domain.NotificationType) string
 }

+const (
+	// defaultRetryBaseDelay is the base delay used for exponential retry
+	// backoff: delay = defaultRetryBaseDelay * 2^(RetryCount-1).
+	defaultRetryBaseDelay = time.Second
+
+	// maxRetryBackoffDelay caps the exponential backoff delay so a
+	// persistently failing notification doesn't wait arbitrarily long between
+	// attempts.
+	maxRetryBackoffDelay = 30 * time.Second
+
+	// adminRole grants access to all tenants' notifications regardless of
+	// ClientID.
+	adminRole = "admin"
+)
+
 // NotificationService implements the domain.NotificationService interface
 type NotificationService struct {
 	factory                domain.NotifierFactory
@@ -27,12 +42,15 @@ type NotificationService struct {
 	mu                     sync.RWMutex
 	workerCount            int
 	stopChan               chan struct{}
+	runCancel              context.CancelFunc
 	wg                     sync.WaitGroup
 	logger                 *logging.Logger
 	retentionConfig        config.NotificationRetentionConfig
 	cleanupStopChan        chan struct{}
 	ttlDuration            time.Duration
 	checkFrequencyDuration time.Duration
+	retryBackoff           string
+	retryBaseDelay         time.Duration
 }

 // NewNotificationService creates a new notification service
@@ -51,9 +69,19 @@ func NewNotificationService(factory domain.NotifierFactory, queue domain.Queue,
 		stopChan:        make(chan struct{}),
 		logger:          logger,
 		cleanupStopChan: make(chan struct{}),
+		retryBaseDelay:  defaultRetryBaseDelay,
 	}
 }

+// WithRetryBackoff sets the retry backoff strategy used when requeueing a
+// notification after a retryable send failure. Recognized values are
+// "exponential" (delay = 1s * 2^(RetryCount-1), capped at 30s) and "none"
+// (requeue immediately). An empty string is treated as "exponential", matching
+// the documented default for queue.retry_backoff.
+func (s *NotificationService) WithRetryBackoff(mode string) {
+	s.retryBackoff = mode
+}
+
 // WithRetentionConfig sets the notification retention configuration
 func (s *NotificationService) WithRetentionConfig(cfg config.NotificationRetentionConfig) error {
 	s.retentionConfig = cfg
@@ -77,15 +105,20 @@ func (s *NotificationService) WithRetentionConfig(cfg config.NotificationRetenti

 // Start starts the worker pool and cleanup goroutine
 func (s *NotificationService) Start(ctx context.Context) error {
+	// Derive a service-lifetime context so Stop() can interrupt workers
+	// blocked in Dequeue instead of waiting out their poll timeout.
+	runCtx, cancel := context.WithCancel(ctx)
+	s.runCancel = cancel
+
 	for i := 0; i < s.workerCount; i++ {
 		s.wg.Add(1)
-		go s.worker(ctx, i)
+		go s.worker(runCtx, i)
 	}

 	// Start cleanup goroutine if retention is enabled
 	if s.retentionConfig.Enabled && s.checkFrequencyDuration > 0 {
 		s.wg.Add(1)
-		go s.cleanupLoop(ctx)
+		go s.cleanupLoop(runCtx)
 	}

 	return nil
@@ -95,6 +128,9 @@ func (s *NotificationService) Start(ctx context.Context) error {
 func (s *NotificationService) Stop() error {
 	close(s.stopChan)
 	close(s.cleanupStopChan)
+	if s.runCancel != nil {
+		s.runCancel()
+	}
 	s.wg.Wait()
 	return s.queue.Close()
 }
@@ -218,7 +254,19 @@ func (s *NotificationService) worker(ctx context.Context, id int) {

 // processNotification sends a notification and handles the result
 func (s *NotificationService) processNotification(ctx context.Context, msg *domain.QueueMessage) {
-	notification := msg.Notification
+	// Work on our own clone of the queued notification. msg.Notification is
+	// owned by the queue (which mutates its Status for its own bookkeeping);
+	// cloning here means our mutations below never race with the queue's or
+	// with clones already handed out by GetNotification/ListNotifications.
+	notification := msg.Notification.Clone()
+
+	// Derive retry progress from msg.Attempt (incremented by the queue on
+	// every dequeue): the queue's copy never sees the counter on our clone,
+	// so the message itself is the source of truth across requeues.
+	notification.RetryCount = msg.Attempt - 1
+	if notification.RetryCount < 0 {
+		notification.RetryCount = 0
+	}

 	s.logger.Debugf("Processing notification - id=%s, type=%s, recipients=%d",
 		notification.ID, notification.Type, len(notification.Recipients))
@@ -257,7 +305,10 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma
 			notification.Status = domain.StatusRetrying
 			s.logger.Warnf("Notification send failed, will retry - id=%s, type=%s, account=%s, attempt=%d/%d, error=%s",
 				notification.ID, notification.Type, account, notification.RetryCount, notification.MaxRetries, notification.LastError)
-			s.queue.Nack(ctx, msg.ID, true) // Requeue
+			// Hand the retry goroutine its own clone: the worker still
+			// publishes this notification via updateNotification below, and
+			// a shutdown-time abandonRetry must not write to the same object.
+			s.scheduleRetry(ctx, msg, notification.Clone())
 		} else {
 			notification.Status = domain.StatusFailed
 			s.logger.Errorf("Notification send failed permanently - id=%s, type=%s, account=%s, recipients=%v, attempts=%d, error=%s",
@@ -276,6 +327,76 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma
 	s.updateNotification(notification)
 }

+// scheduleRetry requeues a failed notification, optionally delaying the
+// requeue according to the configured backoff strategy. When a delay applies,
+// the wait happens on a goroutine tracked by the service WaitGroup so Stop()
+// blocks until it finishes - guaranteeing the queue is never closed while a
+// requeue for it is still pending.
+func (s *NotificationService) scheduleRetry(ctx context.Context, msg *domain.QueueMessage, notification *domain.Notification) {
+	delay := s.retryDelay(notification.RetryCount)
+	if delay <= 0 {
+		s.queue.Nack(ctx, msg.ID, true) // Requeue immediately
+		return
+	}
+
+	s.wg.Add(1)
+	go func() {
+		defer s.wg.Done()
+
+		timer := time.NewTimer(delay)
+		defer timer.Stop()
+
+		select {
+		case <-timer.C:
+			s.queue.Nack(ctx, msg.ID, true) // Requeue after backoff
+		case <-ctx.Done():
+			s.abandonRetry(msg, notification)
+		case <-s.stopChan:
+			s.abandonRetry(msg, notification)
+		}
+	}()
+}
+
+// abandonRetry marks a notification as permanently failed without requeueing
+// it. It is used when the service shuts down while a backoff delay for a
+// retry is still pending, so the notification isn't left stuck in "retrying"
+// forever and no goroutine lingers past shutdown.
+func (s *NotificationService) abandonRetry(msg *domain.QueueMessage, notification *domain.Notification) {
+	s.queue.Nack(context.Background(), msg.ID, false) // Don't requeue
+	notification.Status = domain.StatusFailed
+	s.updateNotification(notification)
+}
+
+// retryDelay computes how long to wait before requeueing a notification that
+// failed on attempt retryCount, per the configured backoff strategy. "none"
+// means requeue immediately (delay of 0); anything else - including the
+// empty string, the documented default - uses exponential backoff: base 1s *
+// 2^(retryCount-1), capped at maxRetryBackoffDelay.
+func (s *NotificationService) retryDelay(retryCount int) time.Duration {
+	if s.retryBackoff == "none" {
+		return 0
+	}
+
+	base := s.retryBaseDelay
+	if base <= 0 {
+		base = defaultRetryBaseDelay
+	}
+
+	if retryCount < 1 {
+		retryCount = 1
+	}
+	shift := retryCount - 1
+	if shift > 20 { // guard against overflow for pathological retry counts
+		shift = 20
+	}
+
+	delay := base * time.Duration(int64(1)<<uint(shift))
+	if delay <= 0 || delay > maxRetryBackoffDelay {
+		delay = maxRetryBackoffDelay
+	}
+	return delay
+}
+
 // Send queues a notification for delivery
 func (s *NotificationService) Send(ctx context.Context, notification *domain.Notification) (*domain.NotificationResult, error) {
 	// Enforce RBAC authorization if configured
@@ -288,15 +409,30 @@ func (s *NotificationService) Send(ctx context.Context, notification *domain.Not
 		}, err
 	}

-	// Store the notification
+	// Stamp tenant ownership from the auth context, if present. Absent auth
+	// context (auth disabled) leaves ClientID empty, preserving current
+	// behavior.
+	if authCtx, ok := auth.GetAuthContext(ctx); ok && authCtx != nil {
+		notification.ClientID = authCtx.ClientID
+	}
+
+	// Mark queued and store BEFORE enqueueing: a worker can pick the message
+	// up immediately, and a post-enqueue status write here would overwrite
+	// the worker's Sent/Failed transition.
+	notification.Status = domain.StatusQueued
 	s.storeNotification(notification)

-	// Enqueue for processing
-	if err := s.queue.Enqueue(ctx, notification); err != nil {
+	// Enqueue a clone: the queue mutates Status on its copy for its own
+	// bookkeeping and workers clone again on dequeue, so no notification
+	// object is ever shared between the queue, the store, and callers.
+	if err := s.queue.Enqueue(ctx, notification.Clone()); err != nil {
+		notification.Status = domain.StatusFailed
+		notification.LastError = fmt.Sprintf("failed to enqueue: %v", err)
+		s.updateNotification(notification)
 		return &domain.NotificationResult{
 			NotificationID: notification.ID,
 			Success:        false,
-			Error:          fmt.Sprintf("failed to enqueue: %v", err),
+			Error:          notification.LastError,
 			SentAt:         time.Now(),
 		}, err
 	}
@@ -320,13 +456,23 @@ func (s *NotificationService) SendBatch(ctx context.Context, notifications []*do
 		}
 	}

-	// Store all notifications
-	for _, notification := range notifications {
-		s.storeNotification(notification)
+	// Stamp tenant ownership from the auth context, if present.
+	if authCtx, ok := auth.GetAuthContext(ctx); ok && authCtx != nil {
+		for _, notification := range notifications {
+			notification.ClientID = authCtx.ClientID
+		}
 	}

-	// Enqueue batch
-	if err := s.queue.EnqueueBatch(ctx, notifications); err != nil {
+	// Mark queued and store BEFORE enqueueing (see Send), then hand the
+	// queue clones so it never shares notification objects with the store
+	// or callers.
+	queued := make([]*domain.Notification, 0, len(notifications))
+	for _, notification := range notifications {
+		notification.Status = domain.StatusQueued
+		s.storeNotification(notification)
+		queued = append(queued, notification.Clone())
+	}
+	if err := s.queue.EnqueueBatch(ctx, queued); err != nil {
 		return nil, fmt.Errorf("failed to enqueue batch: %w", err)
 	}

@@ -343,20 +489,25 @@ func (s *NotificationService) SendBatch(ctx context.Context, notifications []*do
 	return results, nil
 }

-// GetNotification retrieves a notification by ID
+// GetNotification retrieves a notification by ID. If an auth context is
+// present and the caller lacks the admin role, a notification belonging to a
+// different tenant is reported as not found rather than leaking its
+// existence.
 func (s *NotificationService) GetNotification(ctx context.Context, id string) (*domain.Notification, error) {
 	s.mu.RLock()
-	defer s.mu.RUnlock()
-
 	notification, exists := s.notifications[id]
-	if !exists {
-		return nil, fmt.Errorf("notification not found: %s", id)
+	s.mu.RUnlock()
+
+	if !exists || !s.tenantCanAccess(ctx, notification) {
+		return nil, fmt.Errorf("%w: %s", domain.ErrNotificationNotFound, id)
 	}

-	return notification, nil
+	return notification.Clone(), nil
 }

-// ListNotifications retrieves notifications matching the filter
+// ListNotifications retrieves notifications matching the filter, scoped to
+// the caller's tenant unless they have the admin role or no auth context is
+// present.
 func (s *NotificationService) ListNotifications(ctx context.Context, filter *domain.NotificationFilter) ([]*domain.Notification, error) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
@@ -365,8 +516,11 @@ func (s *NotificationService) ListNotifications(ctx context.Context, filter *dom
 	var results []*domain.Notification

 	for _, notification := range s.notifications {
+		if !s.tenantCanAccess(ctx, notification) {
+			continue
+		}
 		if s.matchesFilter(notification, filter) {
-			results = append(results, notification)
+			results = append(results, notification.Clone())
 		}
 	}

@@ -382,27 +536,32 @@ func (s *NotificationService) ListNotifications(ctx context.Context, filter *dom
 	return results, nil
 }

-// CancelNotification cancels a pending notification
+// CancelNotification cancels a pending notification. A notification belonging
+// to a different tenant is reported as not found, matching GetNotification.
 func (s *NotificationService) CancelNotification(ctx context.Context, id string) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()

 	notification, exists := s.notifications[id]
-	if !exists {
-		return fmt.Errorf("notification not found: %s", id)
+	if !exists || !s.tenantCanAccess(ctx, notification) {
+		return fmt.Errorf("%w: %s", domain.ErrNotificationNotFound, id)
 	}

 	if notification.Status == domain.StatusSent {
-		return fmt.Errorf("notification already sent")
+		return domain.ErrNotificationAlreadySent
 	}

-	notification.Status = domain.StatusFailed
-	notification.LastError = "cancelled by user"
+	updated := notification.Clone()
+	updated.Status = domain.StatusFailed
+	updated.LastError = "cancelled by user"
+	s.notifications[id] = updated

 	return nil
 }

-// RetryNotification retries a failed notification
+// RetryNotification retries a failed notification. Cross-tenant access is
+// rejected the same way as GetNotification (via the same not-found error),
+// since RetryNotification is built on top of it.
 func (s *NotificationService) RetryNotification(ctx context.Context, id string) (*domain.NotificationResult, error) {
 	notification, err := s.GetNotification(ctx, id)
 	if err != nil {
@@ -415,7 +574,7 @@ func (s *NotificationService) RetryNotification(ctx context.Context, id string)
 			Success:        false,
 			Error:          "notification already sent",
 			SentAt:         time.Now(),
-		}, fmt.Errorf("notification already sent")
+		}, domain.ErrNotificationAlreadySent
 	}

 	// Reset retry count and status
@@ -426,7 +585,8 @@ func (s *NotificationService) RetryNotification(ctx context.Context, id string)
 	return s.Send(ctx, notification)
 }

-// GetStats returns notification statistics
+// GetStats returns notification statistics, scoped to the caller's tenant
+// unless they have the admin role or no auth context is present.
 func (s *NotificationService) GetStats(ctx context.Context) (*domain.NotificationStats, error) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
@@ -437,6 +597,10 @@ func (s *NotificationService) GetStats(ctx context.Context) (*domain.Notificatio
 	}

 	for _, notification := range s.notifications {
+		if !s.tenantCanAccess(ctx, notification) {
+			continue
+		}
+
 		switch notification.Status {
 		case domain.StatusSent:
 			stats.TotalSent++
@@ -510,18 +674,55 @@ func (s *NotificationService) GetNotifiers(ctx context.Context) (*domain.Notifie
 	}, nil
 }

-// storeNotification stores a notification in memory
+// storeNotification stores a clone of the notification in memory. Storing a
+// clone (rather than the caller's pointer) ensures the map never aliases a
+// notification that the caller, the queue, or a worker may still mutate.
 func (s *NotificationService) storeNotification(notification *domain.Notification) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.notifications[notification.ID] = notification
+	s.notifications[notification.ID] = notification.Clone()
 }

-// updateNotification updates a notification in memory
+// updateNotification updates a notification in memory with a clone of the
+// given notification, for the same reason as storeNotification.
 func (s *NotificationService) updateNotification(notification *domain.Notification) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.notifications[notification.ID] = notification
+	s.notifications[notification.ID] = notification.Clone()
+}
+
+// tenantCanAccess reports whether the caller identified by ctx is allowed to
+// see the given notification. Behavior:
+//   - No auth context present (auth disabled): always allowed, preserving
+//     pre-multi-tenant behavior.
+//   - Auth context present with the admin role: always allowed.
+//   - Auth context present without the admin role: allowed only if the
+//     notification's ClientID matches the caller's.
+func (s *NotificationService) tenantCanAccess(ctx context.Context, notification *domain.Notification) bool {
+	if notification == nil {
+		return false
+	}
+
+	authCtx, ok := auth.GetAuthContext(ctx)
+	if !ok || authCtx == nil {
+		return true
+	}
+
+	if hasRole(authCtx.Roles, adminRole) {
+		return true
+	}
+
+	return notification.ClientID == authCtx.ClientID
+}
+
+// hasRole reports whether role is present in roles.
+func hasRole(roles []string, role string) bool {
+	for _, r := range roles {
+		if r == role {
+			return true
+		}
+	}
+	return false
 }

 // checkAuthorization verifies that the caller is authorized to send to the given notifier/account.