fix(notifications): classify webhook HTTP retries canonically

This commit is contained in:
rcourtman 2026-03-27 12:15:17 +00:00
parent c03ec1e74d
commit ff8b64d3ef
4 changed files with 61 additions and 22 deletions

View file

@ -105,6 +105,11 @@ Enhanced webhook test/live delivery must follow that same ownership model:
into the canonical webhook render and transport path instead of maintaining a
parallel URL-rendering, enrichment, Telegram URL sanitization, or single-send
HTTP stack.
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.
`internal/api/notifications.go` and
`frontend-modern/src/api/notifications.ts` are shared boundaries with

View file

@ -4,6 +4,7 @@ import (
"bytes"
"fmt"
"net/http"
"regexp"
"strconv"
"strings"
"text/template"
@ -13,6 +14,8 @@ import (
"github.com/rs/zerolog/log"
)
var webhookHTTPStatusPattern = regexp.MustCompile(`\b(?:status|http)\s+(\d{3})\b`)
// EnhancedWebhookConfig extends WebhookConfig with template support
type EnhancedWebhookConfig struct {
WebhookConfig
@ -495,28 +498,15 @@ func isRetryableWebhookError(err error) bool {
return true
}
// HTTP status codes that should be retried
if strings.Contains(errStr, "status 408") || // Request Timeout
strings.Contains(errStr, "status 421") || // Misdirected Request
strings.Contains(errStr, "status 423") || // Locked
strings.Contains(errStr, "status 425") || // Too Early
strings.Contains(errStr, "status 429") || // Rate limited
strings.Contains(errStr, "status 502") || // Bad Gateway
strings.Contains(errStr, "status 503") || // Service Unavailable
strings.Contains(errStr, "status 504") { // Gateway Timeout
return true
}
// 5xx server errors are generally retryable
for i := 500; i <= 599; i++ {
if strings.Contains(errStr, fmt.Sprintf("status %d", i)) {
if statusCode, ok := webhookErrorStatusCode(err); ok {
switch statusCode {
case http.StatusRequestTimeout, http.StatusMisdirectedRequest, http.StatusLocked, http.StatusTooEarly, http.StatusTooManyRequests:
return true
}
}
// 4xx client errors are generally not retryable
for i := 400; i <= 499; i++ {
if strings.Contains(errStr, fmt.Sprintf("status %d", i)) {
if statusCode >= 500 && statusCode <= 599 {
return true
}
if statusCode >= 400 && statusCode <= 499 {
return false
}
}
@ -525,6 +515,23 @@ func isRetryableWebhookError(err error) bool {
return true
}
func webhookErrorStatusCode(err error) (int, bool) {
if err == nil {
return 0, false
}
match := webhookHTTPStatusPattern.FindStringSubmatch(strings.ToLower(err.Error()))
if len(match) != 2 {
return 0, false
}
statusCode, parseErr := strconv.Atoi(match[1])
if parseErr != nil {
return 0, false
}
return statusCode, true
}
// executeEnhancedWebhookRequest sends a single enhanced webhook request and returns response metadata.
func (n *NotificationManager) executeEnhancedWebhookRequest(webhook EnhancedWebhookConfig, payload []byte, timeout time.Duration, userAgent string) (*webhookHTTPResult, error) {
canonical := canonicalWebhookConfigForEnhanced(webhook)

View file

@ -455,7 +455,7 @@ func TestSendWebhookWithRetry_StopsOnNonRetryableErrorAfterRetryable(t *testing.
err := nm.sendWebhookWithRetry(webhook, []byte(`{"test":true}`))
assert.Error(t, err)
assert.Contains(t, err.Error(), "status 400")
assert.Contains(t, err.Error(), "HTTP 400")
assert.Contains(t, err.Error(), "after 2 attempts")
assert.Equal(t, 2, attempts)
}
@ -510,7 +510,7 @@ func TestSendWebhookWithRetry_PreservesFailedStatusCodeInHistory(t *testing.T) {
err := nm.sendWebhookWithRetry(webhook, []byte(`{"test":true}`))
assert.Error(t, err)
assert.Contains(t, err.Error(), "status 400")
assert.Contains(t, err.Error(), "HTTP 400")
history := nm.GetWebhookHistory()
if assert.Len(t, history, 1) {
@ -738,9 +738,11 @@ func TestIsRetryableWebhookErrorEnhanced(t *testing.T) {
{"timeout", true},
{"connection refused", true},
{"status 429", true},
{"http 429", true},
{"status 502", true},
{"status 404", false},
{"status 400", false},
{"webhook returned HTTP 400: Bad Request", false},
}
for _, tt := range tests {

View file

@ -49,6 +49,11 @@ func TestIsRetryableWebhookError(t *testing.T) {
err: errors.New("webhook returned status 500: Internal Server Error"),
expected: true,
},
{
name: "http 500 internal server error",
err: errors.New("webhook returned HTTP 500: Internal Server Error"),
expected: true,
},
{
name: "status 502 bad gateway",
err: errors.New("webhook returned status 502: Bad Gateway"),
@ -105,12 +110,22 @@ func TestIsRetryableWebhookError(t *testing.T) {
err: errors.New("webhook returned status 429: Too Many Requests"),
expected: true,
},
{
name: "http 429 too many requests",
err: errors.New("webhook returned HTTP 429: Too Many Requests"),
expected: true,
},
// HTTP 4xx client errors - should NOT be retryable
{
name: "status 400 bad request",
err: errors.New("webhook returned status 400: Bad Request"),
expected: false,
},
{
name: "http 400 bad request",
err: errors.New("webhook returned HTTP 400: Bad Request"),
expected: false,
},
{
name: "status 401 unauthorized",
err: errors.New("webhook returned status 401: Unauthorized"),
@ -215,11 +230,21 @@ func TestIsRetryableWebhookError(t *testing.T) {
err: errors.New("HTTP error: status 503"),
expected: true,
},
{
name: "http code in message format",
err: errors.New("webhook returned HTTP 503"),
expected: true,
},
{
name: "status code only",
err: errors.New("status 401"),
expected: false,
},
{
name: "http code only",
err: errors.New("HTTP 401"),
expected: false,
},
// Boundary tests for status code ranges
{
name: "status 499 last 4xx",