mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-09-10 02:25:56 +00:00
fix(notifications): prioritise HTTP rejection over retry body hints
A provider 403 response mentioning an authentication timeout was treated as a transient network failure and sent again with unchanged credentials. Classify explicit HTTP status before diagnostic text, retaining existing transient status and network fallback behaviour. Add a status/body matrix and a queue-free loopback regression that verifies one request and terminal delivery history. Change-source: pulse-maintainer
This commit is contained in:
@@ -247,7 +247,18 @@ That same ownership includes webhook retry classification. The canonical
|
||||
retry gate in `webhook_enhanced.go` must parse provider failures from both
|
||||
`status 429`-style and `HTTP 429`-style error strings before it decides
|
||||
whether to retry, so a non-retryable `HTTP 400` result cannot be retried just
|
||||
because the transport changed its error wording.
|
||||
because the transport changed its error wording. Explicit HTTP status also
|
||||
wins over network-like diagnostic text in the response body: a terminal 403
|
||||
mentioning "authentication timeout" must stop transport retries, retain the
|
||||
403 in delivery history, and record zero retries when rejected on the first
|
||||
attempt. The existing transient HTTP exceptions and retryable fallback for
|
||||
network or unclassified failures remain unchanged.
|
||||
`TestIsRetryableWebhookError_StatusOverridesBody` and
|
||||
`TestWebhookRetryRejectsForbiddenTimeoutBody` in
|
||||
`internal/notifications/webhook_retry_test.go` pin this precedence with a
|
||||
status/body matrix and a queue-free loopback receiver that checks request
|
||||
count and delivery history. These are synthetic transport proofs, not installed
|
||||
recipient receipts or a change to persistent-queue retry policy.
|
||||
That same notification transport boundary also owns outbound Apprise HTTP URL
|
||||
normalization. Server URLs must be validated as absolute HTTP(S) endpoints
|
||||
without userinfo before request construction, and the `/notify` plus optional
|
||||
|
||||
@@ -495,17 +495,9 @@ func parseRetryAfterBackoff(retryAfter string, now time.Time) (time.Duration, bo
|
||||
|
||||
// isRetryableWebhookError determines if a webhook error should trigger a retry
|
||||
func isRetryableWebhookError(err error) bool {
|
||||
errStr := strings.ToLower(err.Error())
|
||||
|
||||
// Network-related errors that should be retried
|
||||
if strings.Contains(errStr, "timeout") ||
|
||||
strings.Contains(errStr, "connection refused") ||
|
||||
strings.Contains(errStr, "connection reset") ||
|
||||
strings.Contains(errStr, "no such host") ||
|
||||
strings.Contains(errStr, "network unreachable") {
|
||||
return true
|
||||
}
|
||||
|
||||
// An explicit HTTP rejection takes precedence over diagnostic body text:
|
||||
// a 403 mentioning "timeout" is not a transport timeout. Errors without
|
||||
// a recognised status (including network failures) remain retryable below.
|
||||
if statusCode, ok := webhookErrorStatusCode(err); ok {
|
||||
switch statusCode {
|
||||
case http.StatusRequestTimeout, http.StatusMisdirectedRequest, http.StatusLocked, http.StatusTooEarly, http.StatusTooManyRequests:
|
||||
|
||||
@@ -3,6 +3,8 @@ package notifications
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -320,3 +322,52 @@ func TestIsRetryableWebhookError_NetworkPatterns(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Provider diagnostics must not turn a terminal HTTP rejection into a network retry.
|
||||
func TestIsRetryableWebhookError_StatusOverridesBody(t *testing.T) {
|
||||
for _, code := range []int{400, 401, 403, 404, 422} {
|
||||
for _, body := range []string{"authentication timeout", "connection refused", "connection reset", "no such host", "network unreachable"} {
|
||||
t.Run(fmt.Sprintf("%d/%s", code, body), func(t *testing.T) {
|
||||
err := fmt.Errorf("webhook returned HTTP %d: %s", code, body)
|
||||
if isRetryableWebhookError(err) {
|
||||
t.Fatalf("terminal HTTP %d was retried because of diagnostic %q", code, body)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Exercise the HTTP transport without starting a persistent queue or workers.
|
||||
func TestWebhookRetryRejectsForbiddenTimeoutBody(t *testing.T) {
|
||||
var attempts atomic.Int32
|
||||
server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts.Add(1)
|
||||
w.Header().Set("Retry-After", "0")
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
fmt.Fprint(w, "authentication timeout: replace credentials")
|
||||
}))
|
||||
defer server.Close()
|
||||
nm := &NotificationManager{}
|
||||
if err := nm.UpdateAllowedPrivateCIDRs("127.0.0.1"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
nm.webhookClient = nm.createSecureWebhookClient(WebhookTimeout)
|
||||
defer nm.webhookClient.CloseIdleConnections()
|
||||
err := nm.sendWebhookWithRetry(EnhancedWebhookConfig{
|
||||
WebhookConfig: WebhookConfig{Name: "synthetic rejection", URL: server.URL},
|
||||
RetryEnabled: true, RetryCount: 1,
|
||||
}, []byte(`{"test":true}`), "synthetic:alert")
|
||||
if err == nil {
|
||||
t.Fatal("expected forbidden delivery failure")
|
||||
}
|
||||
if got := attempts.Load(); got != 1 {
|
||||
t.Errorf("HTTP attempts = %d, want 1", got)
|
||||
}
|
||||
history := nm.GetWebhookHistory()
|
||||
if len(history) != 1 {
|
||||
t.Fatalf("history length = %d, want 1", len(history))
|
||||
}
|
||||
if history[0].StatusCode != http.StatusForbidden || history[0].Success || history[0].RetryAttempts != 0 {
|
||||
t.Errorf("history lost terminal rejection: status=%d success=%v retries=%d", history[0].StatusCode, history[0].Success, history[0].RetryAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user