From 4573fe6fcad2c58b30709137ca1691a96bed6736 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 02:57:53 +0100 Subject: [PATCH] fix(notifications): preserve backoff after Retry-After override A zero Retry-After on a rate-limited response reset the exponential schedule to zero, causing subsequent transient failures to consume retries immediately. Apply the response delay only to its own wait and retain the normal schedule for later failures. Local HTTP regressions cover both a subsequent 503 and a headerless 429; focused retry tests pass with the race detector over three repetitions. Change-source: pulse-maintainer --- .../v6/internal/subsystems/notifications.md | 15 ++++++++ internal/notifications/webhook_enhanced.go | 3 +- .../notifications/webhook_enhanced_test.go | 38 +++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 290aa0369..a8ac05bad 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -139,6 +139,21 @@ negative overflow cases alongside ordinary seconds, dates, whitespace and invalid inputs. This is pure parser proof: it does not establish elapsed HTTP retry timing, queue persistence, provider acceptance or recipient receipt. +For HTTP 429 transport retries, a valid `Retry-After` overrides only the wait +following that response. It must not replace the independent exponential +backoff schedule. In particular, a zero-delay response cannot cause later +headerless 429 or 503 failures to exhaust their remaining retries immediately. +The schedule continues doubling up to `WebhookMaxBackoff`; retry budgets, +classification, parsing and the existing header-delay cap remain unchanged. +This does not extend header handling to 503 responses. + +`TestSendWebhookWithRetry_ZeroRetryAfterPreservesLaterBackoff` in +`internal/notifications/webhook_enhanced_test.go` sends a zero-delay 429, +then a headerless 429 or 503, then success through a local HTTP fixture. It +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. + 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 48d346eab..0917468ed 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -330,7 +330,8 @@ func (n *NotificationManager) sendWebhookWithRetry(webhook EnhancedWebhookConfig Dur("retryAfter", customBackoff). Msg("using Retry-After header for backoff") time.Sleep(customBackoff) - backoff = customBackoff // Use this for next iteration + // The header overrides this wait only. Preserve the exponential + // schedule so a zero delay cannot erase later failure backoff. usedBackoff = true } else { log.Debug(). diff --git a/internal/notifications/webhook_enhanced_test.go b/internal/notifications/webhook_enhanced_test.go index d47f4f482..68a2ce0c9 100644 --- a/internal/notifications/webhook_enhanced_test.go +++ b/internal/notifications/webhook_enhanced_test.go @@ -844,3 +844,41 @@ func TestIsRetryableWebhookErrorEnhanced(t *testing.T) { }) } } + +// A response-specific zero delay must not erase backoff for later failures. +func TestSendWebhookWithRetry_ZeroRetryAfterPreservesLaterBackoff(t *testing.T) { + for _, status := range []int{http.StatusServiceUnavailable, http.StatusTooManyRequests} { + t.Run(fmt.Sprint(status), func(t *testing.T) { + nm := NewNotificationManager("http://pulse.local") + t.Cleanup(nm.Stop) + require.NoError(t, nm.UpdateAllowedPrivateCIDRs("127.0.0.1")) + attemptTimes := make(chan time.Time, 3) + attempts := 0 + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + attemptTimes <- time.Now() + switch attempts { + case 1: + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + case 2: + w.WriteHeader(status) + default: + w.WriteHeader(http.StatusOK) + } + })) + defer server.Close() + webhook := EnhancedWebhookConfig{ + WebhookConfig: WebhookConfig{Name: "Backoff regression", URL: server.URL}, + RetryEnabled: true, + RetryCount: 2, + } + require.NoError(t, nm.sendWebhookWithRetry(webhook, []byte("{}"), "backoff:alert")) + require.Len(t, attemptTimes, 3) + <-attemptTimes + second, third := <-attemptTimes, <-attemptTimes + assert.GreaterOrEqual(t, third.Sub(second), 2*WebhookInitialBackoff, + "later failure must retain exponential backoff after a zero Retry-After") + }) + } +}