mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-09-10 02:25:56 +00:00
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
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user