mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-09-10 02:25:56 +00:00
fix(notifications): retain HTTP verdict on interrupted response bodies
A truncated diagnostic body currently erases a received HTTP rejection, causing terminal failures such as 403 to consume extra delivery attempts and report a connectivity class. Preserve the HTTP verdict and wrapped read error so existing retry rules still apply. Transport-only regressions reproduce the extra attempt, verify transient recovery and retain successful-response read-error behaviour. Change-source: pulse-maintainer
This commit is contained in:
@@ -860,3 +860,20 @@ This boundary uses the configuration persistence API to save choices, not the
|
||||
HTTP or browser save path. It does not start a new operating-system process,
|
||||
drive an alert lifecycle or establish recipient delivery. Those installed
|
||||
acceptance obligations remain separate from constructor restoration coverage.
|
||||
|
||||
### Interrupted webhook rejection bodies retain the HTTP verdict
|
||||
|
||||
When webhook response headers establish a non-2xx status, a subsequent body
|
||||
read failure retains that HTTP status in the error and its authoritative
|
||||
notification failure class. Terminal rejections must not become connectivity
|
||||
retries merely because the diagnostic body is truncated. Response headers,
|
||||
including Retry-After, and the underlying read error remain available; partial
|
||||
body text is not added to the error. Existing handling of 2xx body read errors
|
||||
is unchanged: it remains a read failure, not proof of recipient receipt.
|
||||
|
||||
Transport-only `TestWebhookTruncatedResponseClassification` covers 200, 401,
|
||||
403, 422, 429 and 503 with interrupted bodies. `TestWebhookRetryTruncatedResponse`
|
||||
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.
|
||||
|
||||
@@ -2957,7 +2957,14 @@ func (n *NotificationManager) executeWebhookRequest(webhook WebhookConfig, paylo
|
||||
var respBody bytes.Buffer
|
||||
bytesRead, err := respBody.ReadFrom(limitedReader)
|
||||
if err != nil {
|
||||
return &webhookHTTPResult{statusCode: resp.StatusCode, headers: resp.Header.Clone()}, fmt.Errorf("failed to read webhook response: %w", err)
|
||||
result := &webhookHTTPResult{statusCode: resp.StatusCode, headers: resp.Header.Clone()}
|
||||
// Headers already establish a rejection even when its diagnostic body
|
||||
// is interrupted. Preserve that verdict for transport and queue retries,
|
||||
// rather than turning a terminal HTTP failure into a connectivity retry.
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return result, FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "webhook returned HTTP %d: failed to read webhook response: %w", resp.StatusCode, err)
|
||||
}
|
||||
return result, fmt.Errorf("failed to read webhook response: %w", err)
|
||||
}
|
||||
if bytesRead >= WebhookMaxResponseSize {
|
||||
log.Warn().
|
||||
|
||||
@@ -3,11 +3,92 @@ package notifications
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// A body interrupted after the response headers must not erase the server's
|
||||
// rejection. These fixtures use only the HTTP transport, without queue workers.
|
||||
func TestWebhookTruncatedResponseClassification(t *testing.T) {
|
||||
for _, code := range []int{200, 401, 403, 422, 429, 503} {
|
||||
t.Run(fmt.Sprint(code), func(t *testing.T) {
|
||||
server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("Content-Length", "100")
|
||||
w.Header().Set("Retry-After", "0")
|
||||
w.WriteHeader(code)
|
||||
fmt.Fprint(w, "short")
|
||||
}))
|
||||
defer server.Close()
|
||||
nm := createTestNotificationManager(t)
|
||||
nm.webhookClient = nm.createSecureWebhookClient(WebhookTimeout)
|
||||
defer nm.webhookClient.CloseIdleConnections()
|
||||
resp, err := nm.executeEnhancedWebhookRequest(EnhancedWebhookConfig{
|
||||
WebhookConfig: WebhookConfig{URL: server.URL},
|
||||
}, []byte(`{}`), WebhookTimeout, "test", "synthetic:alert")
|
||||
if !errors.Is(err, io.ErrUnexpectedEOF) {
|
||||
t.Fatalf("read error not retained: %v", err)
|
||||
}
|
||||
if resp == nil || resp.statusCode != code || resp.headers.Get("Retry-After") != "0" {
|
||||
t.Fatalf("response metadata lost: %+v", resp)
|
||||
}
|
||||
if code != 200 {
|
||||
if got := ClassifyNotificationFailureError(err); got != ClassFromHTTPStatus(code) {
|
||||
t.Errorf("class = %s, want %s", got, ClassFromHTTPStatus(code))
|
||||
}
|
||||
}
|
||||
wantRetry := code == 200 || code == 429 || code == 503
|
||||
if got := isRetryableWebhookError(err); got != wantRetry {
|
||||
t.Errorf("retryable = %v, want %v: %v", got, wantRetry, err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestWebhookRetryTruncatedResponse(t *testing.T) {
|
||||
for _, code := range []int{403, 503} {
|
||||
t.Run(fmt.Sprint(code), func(t *testing.T) {
|
||||
var attempts atomic.Int32
|
||||
server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Header.Get("X-Pulse-Event-ID") != "synthetic:alert" {
|
||||
t.Error("lost event identity")
|
||||
}
|
||||
if attempts.Add(1) > 1 && code == 503 {
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
return
|
||||
}
|
||||
w.Header().Set("Content-Length", "100")
|
||||
w.Header().Set("Retry-After", "0")
|
||||
w.WriteHeader(code)
|
||||
fmt.Fprint(w, "short")
|
||||
}))
|
||||
defer server.Close()
|
||||
nm := createTestNotificationManager(t)
|
||||
nm.webhookClient = nm.createSecureWebhookClient(WebhookTimeout)
|
||||
defer nm.webhookClient.CloseIdleConnections()
|
||||
err := nm.sendWebhookWithRetry(EnhancedWebhookConfig{
|
||||
WebhookConfig: WebhookConfig{URL: server.URL}, RetryCount: 1,
|
||||
}, []byte(`{}`), "synthetic:alert")
|
||||
wantAttempts, wantStatus := int32(1), code
|
||||
wantSuccess := code == 503
|
||||
if wantSuccess {
|
||||
wantAttempts, wantStatus = 2, http.StatusNoContent
|
||||
}
|
||||
if (err == nil) != wantSuccess || attempts.Load() != wantAttempts {
|
||||
t.Errorf("attempts=%d error=%v, want attempts=%d success=%v", attempts.Load(), err, wantAttempts, wantSuccess)
|
||||
}
|
||||
history := nm.GetWebhookHistory()
|
||||
if len(history) != 1 {
|
||||
t.Fatalf("history length = %d, want 1", len(history))
|
||||
}
|
||||
if h := history[0]; h.StatusCode != wantStatus || h.Success != wantSuccess || h.RetryAttempts != int(wantAttempts-1) {
|
||||
t.Errorf("history status=%d success=%v retries=%d", h.StatusCode, h.Success, h.RetryAttempts)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsRetryableWebhookError(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
Reference in New Issue
Block a user