From c65ba29bf7b56f2d2069500c747d147b59ae66be Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:47:30 +0100 Subject: [PATCH] fix(notifications): retain queue retries for transient HTTP rejections Webhook transport already retries HTTP 421, 423 and 425, but shared classification marked them terminal and discarded the remaining queue retry budget. Align these exceptions with retryable connectivity or receiver failure classes. A synthetic 400-599 transport matrix reproduces the three mismatches and verifies permanent rejections remain terminal without starting storage workers. Change-source: pulse-maintainer --- .../v6/internal/subsystems/notifications.md | 15 +++++++++ internal/notifications/failure_class.go | 7 +++- internal/notifications/failure_class_test.go | 3 ++ internal/notifications/webhook_retry_test.go | 32 +++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index a2c57128e..c971e68c6 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -877,3 +877,18 @@ proves a truncated 403 stops after one attempt with terminal history, while a truncated 503 can retry to 204 with its event identity intact. No queue or storage workers are started by these tests; this is not installed delivery acceptance or a change to retry budgets. + +### HTTP retry classification agrees across delivery layers + +HTTP 421 retains a connectivity class; 423 and 425 retain a server-error class +for temporary receiver conditions. These are already retryable exceptions in +the webhook transport, and must not become terminal rejections when a wrapped +transport error reaches the queue. Authentication, configuration and other +permanent HTTP rejections still stop early; attempt limits remain unchanged. + +`TestWebhookHTTPRetryPolicyMatchesQueueClassification` exercises every status +400–599 through a synthetic HTTP transport with misleading diagnostic text, +wraps its returned error and checks both transport retry policy and the shared +class predicate used by the queue. `TestClassFromHTTPStatus` pins the reason +classes. This proves the classification boundary without starting queue/storage +workers; it does not establish installed receipt or queue scheduling execution. diff --git a/internal/notifications/failure_class.go b/internal/notifications/failure_class.go index 9995a9d21..db2c339bd 100644 --- a/internal/notifications/failure_class.go +++ b/internal/notifications/failure_class.go @@ -71,8 +71,13 @@ func ClassFromHTTPStatus(status int) NotificationFailureClass { return NotificationFailureAuthentication case 402: return NotificationFailureConfiguration - case 408: + case 408, 421: return NotificationFailureConnectivity + case 423, 425: + // The webhook transport already retries locked/too-early responses. + // These temporary receiver conditions must retain the queue's retry + // budget too, rather than becoming terminal request rejections. + return NotificationFailureServerError case 429: return NotificationFailureRateLimited } diff --git a/internal/notifications/failure_class_test.go b/internal/notifications/failure_class_test.go index a2ba78da2..740e206a4 100644 --- a/internal/notifications/failure_class_test.go +++ b/internal/notifications/failure_class_test.go @@ -18,6 +18,9 @@ func TestClassFromHTTPStatus(t *testing.T) { 407: NotificationFailureAuthentication, 402: NotificationFailureConfiguration, 408: NotificationFailureConnectivity, + 421: NotificationFailureConnectivity, + 423: NotificationFailureServerError, + 425: NotificationFailureServerError, 429: NotificationFailureRateLimited, 400: NotificationFailureRejected, 404: NotificationFailureRejected, diff --git a/internal/notifications/webhook_retry_test.go b/internal/notifications/webhook_retry_test.go index e2adcece6..7c7351238 100644 --- a/internal/notifications/webhook_retry_test.go +++ b/internal/notifications/webhook_retry_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "net/http" + "strings" "sync/atomic" "testing" ) @@ -495,3 +496,34 @@ func TestWebhookRetryRateLimitThenTerminalRejection(t *testing.T) { t.Errorf("history lost final rejection or retry accounting: %+v", history[0]) } } + +// Queue retry policy consumes the classified, wrapped transport error, not the +// HTTP response. Keep both retry layers consistent for every rejection status. +func TestWebhookHTTPRetryPolicyMatchesQueueClassification(t *testing.T) { + for code := 400; code <= 599; code++ { + t.Run(fmt.Sprint(code), func(t *testing.T) { + nm := &NotificationManager{webhookClient: &http.Client{ + Transport: confidentialityTransport(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: code, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("unauthorized timeout rate limit")), + }, nil + }), + }} + _, err := nm.executeWebhookRequest(WebhookConfig{URL: "https://example.test/hook"}, + []byte("{}"), webhookRequestOptions{}) + if err == nil { + t.Fatal("expected HTTP rejection") + } + err = fmt.Errorf("webhook delivery exhausted: %w", err) + wantRetry := code >= 500 || code == 408 || code == 421 || code == 423 || code == 425 || code == 429 + if got := isRetryableWebhookError(err); got != wantRetry { + t.Errorf("transport retryable = %v, want %v", got, wantRetry) + } + class := ClassifyNotificationFailureError(err) + if got := class.Retryable(); got != wantRetry { + t.Errorf("queue class %s retryable = %v, want %v", class, got, wantRetry) + } + }) + } +}