From 550b2d80a175cc66ab4e7ac269b7ec350e101611 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:14:32 +0100 Subject: [PATCH] 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 --- .../v6/internal/subsystems/notifications.md | 17 ++++ internal/notifications/notifications.go | 9 ++- internal/notifications/webhook_retry_test.go | 81 +++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 0736831a2..a2c57128e 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -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. diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 49d879617..726b5106a 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -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(). diff --git a/internal/notifications/webhook_retry_test.go b/internal/notifications/webhook_retry_test.go index 0ad4000e6..e2adcece6 100644 --- a/internal/notifications/webhook_retry_test.go +++ b/internal/notifications/webhook_retry_test.go @@ -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