From 1ed8b18c12d7567671c86b524840beb423fef12c Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 2 Dec 2025 10:40:07 +0000 Subject: [PATCH] fix: Deadlock in CancelByAlertIDs and add tests Fixed deadlock where CancelByAlertIDs held nq.mu.Lock() and then called UpdateStatus() which also tried to acquire the same lock. Now uses direct SQL while holding the lock. Tests added for CancelByAlertIDs: - No matching notifications (notification stays pending) - Matching notification cancelled - Multiple alerts with partial match (any match cancels) Coverage: CancelByAlertIDs 65.7% -> 81.1% --- internal/notifications/queue.go | 10 ++- internal/notifications/queue_test.go | 123 ++++++++++++++++++++++++++- 2 files changed, 128 insertions(+), 5 deletions(-) diff --git a/internal/notifications/queue.go b/internal/notifications/queue.go index bbe2791..9197a8d 100644 --- a/internal/notifications/queue.go +++ b/internal/notifications/queue.go @@ -785,10 +785,16 @@ func (nq *NotificationQueue) CancelByAlertIDs(alertIDs []string) error { return fmt.Errorf("error iterating notifications for cancellation: %w", err) } - // Cancel the matched notifications + // Cancel the matched notifications (using direct SQL since we already hold the lock) if len(toCancelIDs) > 0 { + now := time.Now().Unix() + updateQuery := ` + UPDATE notification_queue + SET status = ?, last_attempt = ?, last_error = ? + WHERE id = ? + ` for _, notifID := range toCancelIDs { - if err := nq.UpdateStatus(notifID, QueueStatusCancelled, "Alert resolved"); err != nil { + if _, err := nq.db.Exec(updateQuery, QueueStatusCancelled, now, "Alert resolved", notifID); err != nil { log.Error().Err(err).Str("notifID", notifID).Msg("Failed to mark notification as cancelled") } } diff --git a/internal/notifications/queue_test.go b/internal/notifications/queue_test.go index 88cab5b..dd60664 100644 --- a/internal/notifications/queue_test.go +++ b/internal/notifications/queue_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" ) func TestCalculateBackoff(t *testing.T) { @@ -230,11 +232,126 @@ func TestCancelByAlertIDs_NoMatchingNotifications(t *testing.T) { if err != nil { t.Fatalf("Failed to create notification queue: %v", err) } + defer nq.Stop() - // With an empty queue, no notifications should match - err = nq.CancelByAlertIDs([]string{"alert-1", "alert-2"}) + // Enqueue a notification with alert-1 (far future NextRetryAt so background processor doesn't pick it up) + futureRetry := time.Now().Add(1 * time.Hour) + notif := &QueuedNotification{ + ID: "notif-1", + Type: "email", + Status: QueueStatusPending, + MaxAttempts: 3, + Config: []byte(`{}`), + NextRetryAt: &futureRetry, + Alerts: []*alerts.Alert{{ID: "alert-1"}}, + } + if err := nq.Enqueue(notif); err != nil { + t.Fatalf("Failed to enqueue: %v", err) + } + + // Cancel with non-matching alert ID + err = nq.CancelByAlertIDs([]string{"alert-2"}) if err != nil { - t.Errorf("CancelByAlertIDs with no matching notifications returned error: %v", err) + t.Errorf("CancelByAlertIDs returned error: %v", err) + } + + // Verify the notification is still pending using GetQueueStats + stats, err := nq.GetQueueStats() + if err != nil { + t.Fatalf("GetQueueStats failed: %v", err) + } + if stats["pending"] != 1 { + t.Errorf("Expected 1 pending notification, got %d (stats: %v)", stats["pending"], stats) + } + if stats["cancelled"] != 0 { + t.Errorf("Expected 0 cancelled notifications, got %d", stats["cancelled"]) + } +} + +func TestCancelByAlertIDs_MatchingNotificationCancelled(t *testing.T) { + tempDir := t.TempDir() + nq, err := NewNotificationQueue(tempDir) + if err != nil { + t.Fatalf("Failed to create notification queue: %v", err) + } + defer nq.Stop() + + // Enqueue a notification with alert-1 (far future NextRetryAt so background processor doesn't pick it up) + futureRetry := time.Now().Add(1 * time.Hour) + notif := &QueuedNotification{ + ID: "notif-1", + Type: "email", + Status: QueueStatusPending, + MaxAttempts: 3, + Config: []byte(`{}`), + NextRetryAt: &futureRetry, + Alerts: []*alerts.Alert{{ID: "alert-1"}}, + } + if err := nq.Enqueue(notif); err != nil { + t.Fatalf("Failed to enqueue: %v", err) + } + + // Cancel with matching alert ID + err = nq.CancelByAlertIDs([]string{"alert-1"}) + if err != nil { + t.Errorf("CancelByAlertIDs returned error: %v", err) + } + + // Verify the notification is now cancelled using GetQueueStats + stats, err := nq.GetQueueStats() + if err != nil { + t.Fatalf("GetQueueStats failed: %v", err) + } + if stats["pending"] != 0 { + t.Errorf("Expected 0 pending notifications, got %d", stats["pending"]) + } + if stats["cancelled"] != 1 { + t.Errorf("Expected 1 cancelled notification, got %d (stats: %v)", stats["cancelled"], stats) + } +} + +func TestCancelByAlertIDs_MultipleAlertsPartialMatch(t *testing.T) { + tempDir := t.TempDir() + nq, err := NewNotificationQueue(tempDir) + if err != nil { + t.Fatalf("Failed to create notification queue: %v", err) + } + defer nq.Stop() + + // Enqueue a notification with multiple alerts (far future NextRetryAt so background processor doesn't pick it up) + futureRetry := time.Now().Add(1 * time.Hour) + notif := &QueuedNotification{ + ID: "notif-multi", + Type: "webhook", + Status: QueueStatusPending, + MaxAttempts: 3, + Config: []byte(`{}`), + NextRetryAt: &futureRetry, + Alerts: []*alerts.Alert{ + {ID: "alert-1"}, + {ID: "alert-2"}, + }, + } + if err := nq.Enqueue(notif); err != nil { + t.Fatalf("Failed to enqueue: %v", err) + } + + // Cancel with only one matching alert ID - should still cancel the notification + err = nq.CancelByAlertIDs([]string{"alert-1"}) + if err != nil { + t.Errorf("CancelByAlertIDs returned error: %v", err) + } + + // Verify the notification is cancelled (any matching alert should cancel) + stats, err := nq.GetQueueStats() + if err != nil { + t.Fatalf("GetQueueStats failed: %v", err) + } + if stats["pending"] != 0 { + t.Errorf("Expected 0 pending notifications after partial match cancel, got %d", stats["pending"]) + } + if stats["cancelled"] != 1 { + t.Errorf("Expected 1 cancelled notification, got %d (stats: %v)", stats["cancelled"], stats) } }