diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index dd1d5e452..4e670311e 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -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 diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index c72bfa74c..7a5c2ec2d 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -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) diff --git a/internal/notifications/webhook_enhanced_test.go b/internal/notifications/webhook_enhanced_test.go index 97ffb4288..9ff5c4c45 100644 --- a/internal/notifications/webhook_enhanced_test.go +++ b/internal/notifications/webhook_enhanced_test.go @@ -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 { diff --git a/internal/notifications/webhook_retry_test.go b/internal/notifications/webhook_retry_test.go index 400bdc118..588ad54ea 100644 --- a/internal/notifications/webhook_retry_test.go +++ b/internal/notifications/webhook_retry_test.go @@ -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",