From f7232553478aeeb1b815d94b9374af0601b3993a Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 06:10:00 +0100 Subject: [PATCH] test(notifications): bound repeated rate-limit retries and history Incoming webhook destinations can repeatedly return 429. Cover exhaustion for explicit and default retry budgets so response hints cannot reset the attempt ceiling or misreport failed delivery history. Verify each attempt preserves payload and event identity. Correct the transport comment's off-by-one ceiling and remove its delivery guarantee; runtime behaviour is unchanged. Change-source: pulse-maintainer --- .../v6/internal/subsystems/notifications.md | 20 +++++++++ internal/notifications/webhook_enhanced.go | 9 ++-- .../notifications/webhook_enhanced_test.go | 44 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 6760b98d8..22ebb76e1 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -154,6 +154,26 @@ verifies three requests and the two-second exponential wait before the final request. This is transport timing proof, not durable queue retry, installed provider acceptance or recipient receipt. +### Webhook transport retry exhaustion + +The enhanced retry sender makes one initial HTTP attempt plus `RetryCount` +retries; non-positive counts select `WebhookDefaultRetries`. Repeated valid +zero-delay 429 hints do not reset that budget. When all attempts fail, the +sender returns the total attempt count and records one failed webhook history +entry with the final status and the number of retries, not one entry per HTTP +attempt. Every attempt retains the event ID and payload. A queue delivery that +uses this sender has a transport ceiling of effective retry count plus one; +layering up to `MaxAttempts` queue deliveries multiplies that ceiling, but does +not guarantee destination receipt. Permanent errors may stop retries earlier. + +`TestSendWebhookWithRetry_RateLimitExhaustion` in +`internal/notifications/webhook_enhanced_test.go` verifies configured counts +1 and 2 and default selection for 0 and -1 against an owned HTTP fixture that +always returns 429 with `Retry-After: 0`. It checks request identity and body, +attempt exhaustion, and the single failed history record's status, retry +count, payload size and error text. This is transport-budget and local history +proof only, not queue persistence, provider receipt or installed qualification. + Notification-management HTTP production and its unit/contract proof now live together under `internal/api/alerting/`. Router-level scope and integration tests remain in `internal/api`, while the compatibility aliases there keep the diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index 0917468ed..3c8d79a3c 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -302,10 +302,11 @@ func (n *NotificationManager) shouldSendWebhook(webhook EnhancedWebhookConfig, a // sendWebhookWithRetry implements exponential backoff retry with enhanced error tracking // Note: When used with the persistent queue, retry behavior is layered: -// - Transport retries (this function): up to RetryCount attempts with exponential backoff -// - Queue retries: up to MaxAttempts (default 3) with exponential backoff -// Total attempts = RetryCount * MaxAttempts (e.g., 3 * 3 = 9 HTTP calls for a single notification) -// This ensures delivery even during transient failures at either layer. +// - Transport: one initial attempt plus RetryCount retries (non-positive counts use WebhookDefaultRetries) +// - Queue: up to MaxAttempts delivery attempts with exponential backoff +// The HTTP attempt ceiling is (effective RetryCount + 1) * MaxAttempts when +// each queue delivery uses this transport. Permanent errors can stop earlier; +// exhausting either retry budget does not guarantee delivery. func (n *NotificationManager) sendWebhookWithRetry(webhook EnhancedWebhookConfig, payload []byte, eventID string) error { maxRetries := webhook.RetryCount if maxRetries <= 0 { diff --git a/internal/notifications/webhook_enhanced_test.go b/internal/notifications/webhook_enhanced_test.go index 68a2ce0c9..48f2396f0 100644 --- a/internal/notifications/webhook_enhanced_test.go +++ b/internal/notifications/webhook_enhanced_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "net/http" + "sync/atomic" "testing" "time" @@ -292,6 +293,49 @@ func TestParseRetryAfterBackoff(t *testing.T) { } } +// Even repeated valid zero-delay hints must not reset the transport retry budget. +func TestSendWebhookWithRetry_RateLimitExhaustion(t *testing.T) { + for _, retries := range []int{1, 2, 0, -1} { + t.Run(fmt.Sprint(retries), func(t *testing.T) { + nm := NewNotificationManager("http://pulse.local") + t.Cleanup(nm.Stop) + require.NoError(t, nm.UpdateAllowedPrivateCIDRs("127.0.0.1")) + var attempts atomic.Int32 + payload := []byte(`{"test":true}`) + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + assert.Equal(t, "limited:alert", r.Header.Get("X-Pulse-Event-ID")) + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + assert.Equal(t, payload, body) + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + webhook := EnhancedWebhookConfig{ + WebhookConfig: WebhookConfig{Name: "Limited webhook", URL: server.URL}, + RetryEnabled: true, + RetryCount: retries, + } + wantRetries := retries + if wantRetries <= 0 { + wantRetries = WebhookDefaultRetries + } + err := nm.sendWebhookWithRetry(webhook, payload, "limited:alert") + require.Error(t, err) + assert.Contains(t, err.Error(), fmt.Sprintf("after %d attempts", wantRetries+1)) + assert.Equal(t, int32(wantRetries+1), attempts.Load()) + history := nm.GetWebhookHistory() + require.Len(t, history, 1, "one failed delivery, not one history entry per HTTP attempt") + assert.False(t, history[0].Success) + assert.Equal(t, http.StatusTooManyRequests, history[0].StatusCode) + assert.Equal(t, wantRetries, history[0].RetryAttempts) + assert.Equal(t, len(payload), history[0].PayloadSize) + assert.Contains(t, history[0].ErrorMessage, "HTTP 429") + }) + } +} + func TestSendWebhookWithRetry_429RetryAfter(t *testing.T) { nm := NewNotificationManager("http://pulse.local") _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1")