From a86341daff2fb4f8656409a05f82541ea5453408 Mon Sep 17 00:00:00 2001 From: Christoph Maser Date: Thu, 16 Jul 2026 19:24:01 +0200 Subject: [PATCH 1/5] feat(notify): generic handling of 429 in retrier add generic handling of 429 in retrier. This will allow to remove any handling of 429 in the individual notifiers and instead rely on the retrier to handle it (TBD in follow-up PRs). Signed-off-by: Christoph Maser --- notify/notify_test.go | 109 ++++++++++++++++++++++++ notify/retry_stage.go | 49 +++++++++-- notify/util.go | 78 ++++++++++++++++- notify/util_test.go | 189 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 416 insertions(+), 9 deletions(-) diff --git a/notify/notify_test.go b/notify/notify_test.go index e536223375..465cfcb795 100644 --- a/notify/notify_test.go +++ b/notify/notify_test.go @@ -621,6 +621,115 @@ func TestRetryStageWithContextCanceled(t *testing.T) { require.NotNil(t, resctx) } +func TestRetryStageHonorsRetryAfter(t *testing.T) { + attempts := 0 + i := Integration{ + name: "test", + notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { + attempts++ + if attempts < 4 { + err := NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) + err.RetryAfter = 10 * time.Millisecond + return true, err + } + return false, nil + }), + rs: sendResolved(false), + } + r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) + + alerts := []*types.Alert{{ + Alert: model.Alert{ + EndsAt: time.Now().Add(time.Hour), + }, + }} + + // The default exponential backoff starts at 500ms after the first immediate + // attempt, so 4 attempts can only complete within this timeout when + // Retry-After is actually honored. + ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) + defer cancel() + ctx = WithFiringAlerts(ctx, []uint64{0}) + + start := time.Now() + _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) + elapsed := time.Since(start) + require.NoError(t, err) + require.Equal(t, 4, attempts) + require.GreaterOrEqual(t, elapsed, 30*time.Millisecond) + require.Less(t, elapsed, 350*time.Millisecond) +} + +func TestRetryStageRecalculatesBackoffAfterRetryAfter(t *testing.T) { + attempts := 0 + i := Integration{ + name: "test", + notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { + attempts++ + switch attempts { + case 1: + err := NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) + err.RetryAfter = 10 * time.Millisecond + return true, err + case 2: + return true, errors.New("temporary failure") + default: + return false, nil + } + }), + rs: sendResolved(false), + } + r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) + + alerts := []*types.Alert{{ + Alert: model.Alert{ + EndsAt: time.Now().Add(time.Hour), + }, + }} + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + ctx = WithFiringAlerts(ctx, []uint64{0}) + + _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) + require.Error(t, err) + require.Contains(t, err.Error(), "notify retry canceled after 2 attempts") + require.Equal(t, 2, attempts) +} + +func TestRetryStageWithoutRetryAfterUsesExponentialBackoff(t *testing.T) { + attempts := 0 + i := Integration{ + name: "test", + notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { + attempts++ + if attempts < 4 { + return true, NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) + } + return false, nil + }), + rs: sendResolved(false), + } + r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) + + alerts := []*types.Alert{{ + Alert: model.Alert{ + EndsAt: time.Now().Add(time.Hour), + }, + }} + + // Without Retry-After we should follow the default backoff, whose first + // interval after the initial attempt is far larger than this timeout. + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + ctx = WithFiringAlerts(ctx, []uint64{0}) + + _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) + require.Error(t, err) + require.Contains(t, err.Error(), "notify retry canceled after 1 attempts") + require.Equal(t, 1, attempts) +} + func TestRetryStageNoResolved(t *testing.T) { sent := []*alert.Alert{} i := Integration{ diff --git a/notify/retry_stage.go b/notify/retry_stage.go index 7c71f9c7d5..644c7eff46 100644 --- a/notify/retry_stage.go +++ b/notify/retry_stage.go @@ -116,8 +116,18 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A // the ticker retries indefinitely until the context is canceled. b := backoff.NewExponentialBackOff() - tick := backoff.NewTicker(b) - defer tick.Stop() + stopTimer := func(timer *time.Timer) { + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + } + + // Fire immediately for the first attempt. + attemptTimer := time.NewTimer(0) + defer stopTimer(attemptTimer) var ( i = 0 @@ -151,7 +161,7 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A } select { - case <-tick.C: + case <-attemptTimer.C: now := time.Now() retry, err := r.integration.Notify(ctx, sent...) i++ @@ -164,12 +174,35 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A return ctx, alerts, fmt.Errorf("%s/%s: notify retry canceled due to unrecoverable error after %d attempts: %w", r.groupName, r.integration.String(), i, err) } if ctx.Err() == nil { - if iErr == nil || err.Error() != iErr.Error() { - // Log the error if the context isn't done and the error isn't the same as before. - l.Warn("Notify attempt failed, will retry later", "attempts", i, "err", err) + nextDelay := b.NextBackOff() + + // Defensive: NextBackOff only returns Stop when MaxElapsedTime > 0, + // which we don't set, but guard against future config changes. + if nextDelay == backoff.Stop { + return ctx, nil, fmt.Errorf("%s/%s: notify retry stopped after %d attempts: %w", r.groupName, r.integration.String(), i, err) + } + + var e *ErrorWithReason + if errors.As(err, &e) && e.Reason == RateLimitedReason && e.RetryAfter > 0 { + nextDelay = e.RetryAfter + l.Warn("Notify attempt failed, honoring Retry-After", "attempts", i, "retry_after", e.RetryAfter, "err", err) + } else { + // Subtract the attempt duration so the next attempt fires at + // approximately attempt_start + backoff, matching the behavior + // of the previous backoff.Ticker (which started counting from + // when the tick was consumed, not when the attempt finished). + nextDelay -= dur + if nextDelay < 0 { + nextDelay = 0 + } + if iErr == nil || err.Error() != iErr.Error() { + // Log if context isn't done and the error differs from last time. + l.Warn("Notify attempt failed, will retry later", "attempts", i, "err", err) + } } - // Save this error to be able to return the last seen error by an - // integration upon context timeout. + + attemptTimer.Reset(nextDelay) + // Save the error to return the last seen error on context timeout. iErr = err } } else { diff --git a/notify/util.go b/notify/util.go index fe4c9ea508..08e7c1e1f9 100644 --- a/notify/util.go +++ b/notify/util.go @@ -23,7 +23,9 @@ import ( "net/http" "net/url" "slices" + "strconv" "strings" + "time" commoncfg "github.com/prometheus/common/config" "github.com/prometheus/common/version" @@ -239,6 +241,28 @@ type Retrier struct { RetryCodes []int } +// parseRetryAfter parses the Retry-After header value, which can be either +// a delay in seconds (integer) or an HTTP-date. Returns zero if absent or unparseable. +func parseRetryAfter(h http.Header) time.Duration { + val := h.Get("Retry-After") + if val == "" { + return 0 + } + // Try integer seconds first. + if secs, err := strconv.Atoi(val); err == nil { + return time.Duration(secs) * time.Second + } + // Try HTTP-date format. + if t, err := http.ParseTime(val); err == nil { + d := time.Until(t) + if d < 0 { + return 0 + } + return d + } + return 0 +} + // Check returns a boolean indicating whether the request should be retried // and an optional error if the request has failed. If body is not nil, it will // be included in the error message. @@ -264,10 +288,62 @@ func (r *Retrier) Check(statusCode int, body io.Reader) (bool, error) { return retry, errors.New(s) } +// CheckResponse returns a boolean indicating whether the request should be +// retried and an optional ErrorWithReason if the request has failed. +// Unlike Check, it accepts the full *http.Response so it can parse the +// Retry-After header on 429 responses and attach it to the returned error. +func (r *Retrier) CheckResponse(resp *http.Response) (bool, error) { + if resp == nil { + return false, NewErrorWithReason(DefaultReason, errors.New("nil HTTP response")) + } + + // 2xx responses are always successful. + if resp.StatusCode/100 == 2 { + return false, nil + } + + s := fmt.Sprintf("unexpected status code %v", resp.StatusCode) + var details string + if r.CustomDetailsFunc != nil { + details = r.CustomDetailsFunc(resp.StatusCode, resp.Body) + } else { + details = readAll(resp.Body) + } + if details != "" { + s = fmt.Sprintf("%s: %s", s, details) + } + + // Codes in RetryCodes are retriable regardless of class, except 429 + // which is handled separately below to attach Retry-After. + if slices.Contains(r.RetryCodes, resp.StatusCode) && resp.StatusCode != http.StatusTooManyRequests { + return true, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) + } + + if resp.StatusCode == http.StatusTooManyRequests { + e := NewErrorWithReason(RateLimitedReason, errors.New(s)) + if d := parseRetryAfter(resp.Header); d > 0 { + e.RetryAfter = d + } + return true, e + } + + if resp.StatusCode/100 == 4 { + return false, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) + } + + // 5xx responses are always retried. + if resp.StatusCode/100 == 5 { + return true, NewErrorWithReason(ServerErrorReason, errors.New(s)) + } + + return false, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) +} + type ErrorWithReason struct { Err error - Reason Reason + Reason Reason + RetryAfter time.Duration } func NewErrorWithReason(reason Reason, err error) *ErrorWithReason { diff --git a/notify/util_test.go b/notify/util_test.go index 2c2d4922e8..d7e2d18749 100644 --- a/notify/util_test.go +++ b/notify/util_test.go @@ -24,6 +24,7 @@ import ( "reflect" "runtime" "testing" + "time" "github.com/prometheus/common/model" "github.com/prometheus/common/promslog" @@ -273,3 +274,191 @@ func TestGetFailureReasonFromStatusCode(t *testing.T) { }) } } + +func TestCheckResponse(t *testing.T) { + for _, tc := range []struct { + name string + retrier Retrier + response *http.Response + retry bool + expectedErr string + reason Reason + }{ + { + name: "2xx success", + response: &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString("ok")), + }, + retry: false, + }, + { + name: "204 no content", + response: &http.Response{ + StatusCode: http.StatusNoContent, + Body: io.NopCloser(bytes.NewBuffer(nil)), + }, + retry: false, + }, + { + name: "400 bad request", + response: &http.Response{ + StatusCode: http.StatusBadRequest, + Body: io.NopCloser(bytes.NewBufferString("invalid request")), + }, + retry: false, + expectedErr: "unexpected status code 400: invalid request", + reason: ClientErrorReason, + }, + { + name: "401 unauthorized", + response: &http.Response{ + StatusCode: http.StatusUnauthorized, + Body: io.NopCloser(bytes.NewBuffer(nil)), + }, + retry: false, + expectedErr: "unexpected status code 401", + reason: AuthErrorReason, + }, + { + name: "429 without Retry-After", + response: &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewBufferString("too many requests")), + }, + retry: true, + expectedErr: "unexpected status code 429: too many requests", + reason: RateLimitedReason, + }, + { + name: "429 in RetryCodes uses RateLimitedReason", + retrier: Retrier{RetryCodes: []int{http.StatusTooManyRequests}}, + response: &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewBufferString("too many requests")), + }, + retry: true, + expectedErr: "unexpected status code 429: too many requests", + reason: RateLimitedReason, + }, + { + name: "503 service unavailable", + response: &http.Response{ + StatusCode: http.StatusServiceUnavailable, + Body: io.NopCloser(bytes.NewBufferString("retry later")), + }, + retry: true, + expectedErr: "unexpected status code 503: retry later", + reason: ServerErrorReason, + }, + { + name: "502 bad gateway with broken body", + response: &http.Response{ + StatusCode: http.StatusBadGateway, + Body: io.NopCloser(&brokenReader{}), + }, + retry: true, + expectedErr: "unexpected status code 502", + reason: ServerErrorReason, + }, + { + name: "non-retryable code in RetryCodes (e.g. 409)", + retrier: Retrier{RetryCodes: []int{http.StatusConflict}}, + response: &http.Response{ + StatusCode: http.StatusConflict, + Body: io.NopCloser(bytes.NewBufferString("conflict")), + }, + retry: true, + expectedErr: "unexpected status code 409: conflict", + reason: ClientErrorReason, + }, + { + name: "nil response", + response: nil, + retry: false, + expectedErr: "nil HTTP response", + reason: DefaultReason, + }, + } { + t.Run(tc.name, func(t *testing.T) { + retry, err := tc.retrier.CheckResponse(tc.response) + require.Equal(t, tc.retry, retry) + if tc.expectedErr == "" { + require.NoError(t, err) + return + } + require.EqualError(t, err, tc.expectedErr) + var e *ErrorWithReason + require.ErrorAs(t, err, &e) + require.Equal(t, tc.reason, e.Reason) + }) + } +} + +func TestCheckResponseRetryAfterPropagation(t *testing.T) { + for _, tc := range []struct { + name string + retryAfterHeader string + useHTTPDate bool + expectedRetryAfter time.Duration + expectExactRetryAfter bool + }{ + { + name: "integer seconds", + retryAfterHeader: "7", + expectedRetryAfter: 7 * time.Second, + expectExactRetryAfter: true, + }, + { + name: "zero seconds is treated as no Retry-After", + retryAfterHeader: "0", + expectedRetryAfter: 0, + expectExactRetryAfter: true, + }, + { + name: "HTTP-date in the future", + useHTTPDate: true, + expectedRetryAfter: 2 * time.Second, + }, + { + name: "absent header means zero RetryAfter", + retryAfterHeader: "", + expectedRetryAfter: 0, + expectExactRetryAfter: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + header := make(http.Header) + if tc.useHTTPDate { + header.Set("Retry-After", time.Now().Add(tc.expectedRetryAfter).UTC().Format(http.TimeFormat)) + } else if tc.retryAfterHeader != "" { + header.Set("Retry-After", tc.retryAfterHeader) + } + + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: header, + Body: io.NopCloser(bytes.NewBufferString("too many requests")), + } + + retry, err := (&Retrier{}).CheckResponse(resp) + require.True(t, retry) + require.Error(t, err) + + var e *ErrorWithReason + require.ErrorAs(t, err, &e) + require.Equal(t, RateLimitedReason, e.Reason) + + if tc.expectExactRetryAfter { + require.Equal(t, tc.expectedRetryAfter, e.RetryAfter) + return + } + // HTTP-date parsing depends on wall-clock timing; assert a positive + // value close to the requested duration. + require.Greater(t, e.RetryAfter, time.Duration(0)) + require.InDelta(t, tc.expectedRetryAfter.Seconds(), e.RetryAfter.Seconds(), 1.0) + }) + } +} From 9acf2e4f88e816eb97000ebdcad9b2e59c7a6f87 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Thu, 27 Aug 2026 21:10:09 +0200 Subject: [PATCH 2/5] notify: limit new handler to parseRetryAfter A generic handler is desirable, but we don't want to achieve it without `CheckResponse` or `ErrorWithReason`. Follow-up work should expand the current handler though. Signed-off-by: Solomon Jacobs --- notify/notify_test.go | 109 ------------------------ notify/retry_stage.go | 49 ++--------- notify/util.go | 58 +------------ notify/util_test.go | 189 ------------------------------------------ 4 files changed, 11 insertions(+), 394 deletions(-) diff --git a/notify/notify_test.go b/notify/notify_test.go index 465cfcb795..e536223375 100644 --- a/notify/notify_test.go +++ b/notify/notify_test.go @@ -621,115 +621,6 @@ func TestRetryStageWithContextCanceled(t *testing.T) { require.NotNil(t, resctx) } -func TestRetryStageHonorsRetryAfter(t *testing.T) { - attempts := 0 - i := Integration{ - name: "test", - notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { - attempts++ - if attempts < 4 { - err := NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) - err.RetryAfter = 10 * time.Millisecond - return true, err - } - return false, nil - }), - rs: sendResolved(false), - } - r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) - - alerts := []*types.Alert{{ - Alert: model.Alert{ - EndsAt: time.Now().Add(time.Hour), - }, - }} - - // The default exponential backoff starts at 500ms after the first immediate - // attempt, so 4 attempts can only complete within this timeout when - // Retry-After is actually honored. - ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) - defer cancel() - ctx = WithFiringAlerts(ctx, []uint64{0}) - - start := time.Now() - _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) - elapsed := time.Since(start) - require.NoError(t, err) - require.Equal(t, 4, attempts) - require.GreaterOrEqual(t, elapsed, 30*time.Millisecond) - require.Less(t, elapsed, 350*time.Millisecond) -} - -func TestRetryStageRecalculatesBackoffAfterRetryAfter(t *testing.T) { - attempts := 0 - i := Integration{ - name: "test", - notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { - attempts++ - switch attempts { - case 1: - err := NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) - err.RetryAfter = 10 * time.Millisecond - return true, err - case 2: - return true, errors.New("temporary failure") - default: - return false, nil - } - }), - rs: sendResolved(false), - } - r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) - - alerts := []*types.Alert{{ - Alert: model.Alert{ - EndsAt: time.Now().Add(time.Hour), - }, - }} - - ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) - defer cancel() - ctx = WithFiringAlerts(ctx, []uint64{0}) - - _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) - require.Error(t, err) - require.Contains(t, err.Error(), "notify retry canceled after 2 attempts") - require.Equal(t, 2, attempts) -} - -func TestRetryStageWithoutRetryAfterUsesExponentialBackoff(t *testing.T) { - attempts := 0 - i := Integration{ - name: "test", - notifier: notifierFunc(func(ctx context.Context, alerts ...*types.Alert) (bool, error) { - attempts++ - if attempts < 4 { - return true, NewErrorWithReason(RateLimitedReason, errors.New("received 429 Too Many Requests")) - } - return false, nil - }), - rs: sendResolved(false), - } - r := NewRetryStage(i, "", NewMetrics(prometheus.NewRegistry(), featurecontrol.NoopFlags{}), eventrecorder.NopRecorder()) - - alerts := []*types.Alert{{ - Alert: model.Alert{ - EndsAt: time.Now().Add(time.Hour), - }, - }} - - // Without Retry-After we should follow the default backoff, whose first - // interval after the initial attempt is far larger than this timeout. - ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) - defer cancel() - ctx = WithFiringAlerts(ctx, []uint64{0}) - - _, _, err := r.Exec(ctx, promslog.NewNopLogger(), alerts...) - require.Error(t, err) - require.Contains(t, err.Error(), "notify retry canceled after 1 attempts") - require.Equal(t, 1, attempts) -} - func TestRetryStageNoResolved(t *testing.T) { sent := []*alert.Alert{} i := Integration{ diff --git a/notify/retry_stage.go b/notify/retry_stage.go index 644c7eff46..7c71f9c7d5 100644 --- a/notify/retry_stage.go +++ b/notify/retry_stage.go @@ -116,18 +116,8 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A // the ticker retries indefinitely until the context is canceled. b := backoff.NewExponentialBackOff() - stopTimer := func(timer *time.Timer) { - if !timer.Stop() { - select { - case <-timer.C: - default: - } - } - } - - // Fire immediately for the first attempt. - attemptTimer := time.NewTimer(0) - defer stopTimer(attemptTimer) + tick := backoff.NewTicker(b) + defer tick.Stop() var ( i = 0 @@ -161,7 +151,7 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A } select { - case <-attemptTimer.C: + case <-tick.C: now := time.Now() retry, err := r.integration.Notify(ctx, sent...) i++ @@ -174,35 +164,12 @@ func (r RetryStage) exec(ctx context.Context, l *slog.Logger, alerts ...*alert.A return ctx, alerts, fmt.Errorf("%s/%s: notify retry canceled due to unrecoverable error after %d attempts: %w", r.groupName, r.integration.String(), i, err) } if ctx.Err() == nil { - nextDelay := b.NextBackOff() - - // Defensive: NextBackOff only returns Stop when MaxElapsedTime > 0, - // which we don't set, but guard against future config changes. - if nextDelay == backoff.Stop { - return ctx, nil, fmt.Errorf("%s/%s: notify retry stopped after %d attempts: %w", r.groupName, r.integration.String(), i, err) - } - - var e *ErrorWithReason - if errors.As(err, &e) && e.Reason == RateLimitedReason && e.RetryAfter > 0 { - nextDelay = e.RetryAfter - l.Warn("Notify attempt failed, honoring Retry-After", "attempts", i, "retry_after", e.RetryAfter, "err", err) - } else { - // Subtract the attempt duration so the next attempt fires at - // approximately attempt_start + backoff, matching the behavior - // of the previous backoff.Ticker (which started counting from - // when the tick was consumed, not when the attempt finished). - nextDelay -= dur - if nextDelay < 0 { - nextDelay = 0 - } - if iErr == nil || err.Error() != iErr.Error() { - // Log if context isn't done and the error differs from last time. - l.Warn("Notify attempt failed, will retry later", "attempts", i, "err", err) - } + if iErr == nil || err.Error() != iErr.Error() { + // Log the error if the context isn't done and the error isn't the same as before. + l.Warn("Notify attempt failed, will retry later", "attempts", i, "err", err) } - - attemptTimer.Reset(nextDelay) - // Save the error to return the last seen error on context timeout. + // Save this error to be able to return the last seen error by an + // integration upon context timeout. iErr = err } } else { diff --git a/notify/util.go b/notify/util.go index 08e7c1e1f9..305fa23563 100644 --- a/notify/util.go +++ b/notify/util.go @@ -241,9 +241,9 @@ type Retrier struct { RetryCodes []int } -// parseRetryAfter parses the Retry-After header value, which can be either +// ParseRetryAfter parses the Retry-After header value, which can be either // a delay in seconds (integer) or an HTTP-date. Returns zero if absent or unparseable. -func parseRetryAfter(h http.Header) time.Duration { +func ParseRetryAfter(h http.Header) time.Duration { val := h.Get("Retry-After") if val == "" { return 0 @@ -288,62 +288,10 @@ func (r *Retrier) Check(statusCode int, body io.Reader) (bool, error) { return retry, errors.New(s) } -// CheckResponse returns a boolean indicating whether the request should be -// retried and an optional ErrorWithReason if the request has failed. -// Unlike Check, it accepts the full *http.Response so it can parse the -// Retry-After header on 429 responses and attach it to the returned error. -func (r *Retrier) CheckResponse(resp *http.Response) (bool, error) { - if resp == nil { - return false, NewErrorWithReason(DefaultReason, errors.New("nil HTTP response")) - } - - // 2xx responses are always successful. - if resp.StatusCode/100 == 2 { - return false, nil - } - - s := fmt.Sprintf("unexpected status code %v", resp.StatusCode) - var details string - if r.CustomDetailsFunc != nil { - details = r.CustomDetailsFunc(resp.StatusCode, resp.Body) - } else { - details = readAll(resp.Body) - } - if details != "" { - s = fmt.Sprintf("%s: %s", s, details) - } - - // Codes in RetryCodes are retriable regardless of class, except 429 - // which is handled separately below to attach Retry-After. - if slices.Contains(r.RetryCodes, resp.StatusCode) && resp.StatusCode != http.StatusTooManyRequests { - return true, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) - } - - if resp.StatusCode == http.StatusTooManyRequests { - e := NewErrorWithReason(RateLimitedReason, errors.New(s)) - if d := parseRetryAfter(resp.Header); d > 0 { - e.RetryAfter = d - } - return true, e - } - - if resp.StatusCode/100 == 4 { - return false, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) - } - - // 5xx responses are always retried. - if resp.StatusCode/100 == 5 { - return true, NewErrorWithReason(ServerErrorReason, errors.New(s)) - } - - return false, NewErrorWithReason(GetFailureReasonFromStatusCode(resp.StatusCode), errors.New(s)) -} - type ErrorWithReason struct { Err error - Reason Reason - RetryAfter time.Duration + Reason Reason } func NewErrorWithReason(reason Reason, err error) *ErrorWithReason { diff --git a/notify/util_test.go b/notify/util_test.go index d7e2d18749..2c2d4922e8 100644 --- a/notify/util_test.go +++ b/notify/util_test.go @@ -24,7 +24,6 @@ import ( "reflect" "runtime" "testing" - "time" "github.com/prometheus/common/model" "github.com/prometheus/common/promslog" @@ -274,191 +273,3 @@ func TestGetFailureReasonFromStatusCode(t *testing.T) { }) } } - -func TestCheckResponse(t *testing.T) { - for _, tc := range []struct { - name string - retrier Retrier - response *http.Response - retry bool - expectedErr string - reason Reason - }{ - { - name: "2xx success", - response: &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(bytes.NewBufferString("ok")), - }, - retry: false, - }, - { - name: "204 no content", - response: &http.Response{ - StatusCode: http.StatusNoContent, - Body: io.NopCloser(bytes.NewBuffer(nil)), - }, - retry: false, - }, - { - name: "400 bad request", - response: &http.Response{ - StatusCode: http.StatusBadRequest, - Body: io.NopCloser(bytes.NewBufferString("invalid request")), - }, - retry: false, - expectedErr: "unexpected status code 400: invalid request", - reason: ClientErrorReason, - }, - { - name: "401 unauthorized", - response: &http.Response{ - StatusCode: http.StatusUnauthorized, - Body: io.NopCloser(bytes.NewBuffer(nil)), - }, - retry: false, - expectedErr: "unexpected status code 401", - reason: AuthErrorReason, - }, - { - name: "429 without Retry-After", - response: &http.Response{ - StatusCode: http.StatusTooManyRequests, - Header: make(http.Header), - Body: io.NopCloser(bytes.NewBufferString("too many requests")), - }, - retry: true, - expectedErr: "unexpected status code 429: too many requests", - reason: RateLimitedReason, - }, - { - name: "429 in RetryCodes uses RateLimitedReason", - retrier: Retrier{RetryCodes: []int{http.StatusTooManyRequests}}, - response: &http.Response{ - StatusCode: http.StatusTooManyRequests, - Header: make(http.Header), - Body: io.NopCloser(bytes.NewBufferString("too many requests")), - }, - retry: true, - expectedErr: "unexpected status code 429: too many requests", - reason: RateLimitedReason, - }, - { - name: "503 service unavailable", - response: &http.Response{ - StatusCode: http.StatusServiceUnavailable, - Body: io.NopCloser(bytes.NewBufferString("retry later")), - }, - retry: true, - expectedErr: "unexpected status code 503: retry later", - reason: ServerErrorReason, - }, - { - name: "502 bad gateway with broken body", - response: &http.Response{ - StatusCode: http.StatusBadGateway, - Body: io.NopCloser(&brokenReader{}), - }, - retry: true, - expectedErr: "unexpected status code 502", - reason: ServerErrorReason, - }, - { - name: "non-retryable code in RetryCodes (e.g. 409)", - retrier: Retrier{RetryCodes: []int{http.StatusConflict}}, - response: &http.Response{ - StatusCode: http.StatusConflict, - Body: io.NopCloser(bytes.NewBufferString("conflict")), - }, - retry: true, - expectedErr: "unexpected status code 409: conflict", - reason: ClientErrorReason, - }, - { - name: "nil response", - response: nil, - retry: false, - expectedErr: "nil HTTP response", - reason: DefaultReason, - }, - } { - t.Run(tc.name, func(t *testing.T) { - retry, err := tc.retrier.CheckResponse(tc.response) - require.Equal(t, tc.retry, retry) - if tc.expectedErr == "" { - require.NoError(t, err) - return - } - require.EqualError(t, err, tc.expectedErr) - var e *ErrorWithReason - require.ErrorAs(t, err, &e) - require.Equal(t, tc.reason, e.Reason) - }) - } -} - -func TestCheckResponseRetryAfterPropagation(t *testing.T) { - for _, tc := range []struct { - name string - retryAfterHeader string - useHTTPDate bool - expectedRetryAfter time.Duration - expectExactRetryAfter bool - }{ - { - name: "integer seconds", - retryAfterHeader: "7", - expectedRetryAfter: 7 * time.Second, - expectExactRetryAfter: true, - }, - { - name: "zero seconds is treated as no Retry-After", - retryAfterHeader: "0", - expectedRetryAfter: 0, - expectExactRetryAfter: true, - }, - { - name: "HTTP-date in the future", - useHTTPDate: true, - expectedRetryAfter: 2 * time.Second, - }, - { - name: "absent header means zero RetryAfter", - retryAfterHeader: "", - expectedRetryAfter: 0, - expectExactRetryAfter: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - header := make(http.Header) - if tc.useHTTPDate { - header.Set("Retry-After", time.Now().Add(tc.expectedRetryAfter).UTC().Format(http.TimeFormat)) - } else if tc.retryAfterHeader != "" { - header.Set("Retry-After", tc.retryAfterHeader) - } - - resp := &http.Response{ - StatusCode: http.StatusTooManyRequests, - Header: header, - Body: io.NopCloser(bytes.NewBufferString("too many requests")), - } - - retry, err := (&Retrier{}).CheckResponse(resp) - require.True(t, retry) - require.Error(t, err) - - var e *ErrorWithReason - require.ErrorAs(t, err, &e) - require.Equal(t, RateLimitedReason, e.Reason) - - if tc.expectExactRetryAfter { - require.Equal(t, tc.expectedRetryAfter, e.RetryAfter) - return - } - // HTTP-date parsing depends on wall-clock timing; assert a positive - // value close to the requested duration. - require.Greater(t, e.RetryAfter, time.Duration(0)) - require.InDelta(t, tc.expectedRetryAfter.Seconds(), e.RetryAfter.Seconds(), 1.0) - }) - } -} From c60ca7bdf16a9d498118267997518378bef7c299 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Thu, 10 Sep 2026 22:23:50 +0200 Subject: [PATCH 3/5] notify: clamp negative Retry-After delays to zero The delay-seconds grammar in RFC 9110 is 1*DIGIT, so a signed value is malformed rather than a delay and belongs in the same bucket as any other unparseable value. Signed-off-by: Solomon Jacobs --- notify/util.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/notify/util.go b/notify/util.go index 305fa23563..d85de696f5 100644 --- a/notify/util.go +++ b/notify/util.go @@ -250,15 +250,12 @@ func ParseRetryAfter(h http.Header) time.Duration { } // Try integer seconds first. if secs, err := strconv.Atoi(val); err == nil { - return time.Duration(secs) * time.Second + // The grammar is 1*DIGIT, so a negative is malformed rather than a delay. + return max(0, time.Duration(secs)*time.Second) } // Try HTTP-date format. if t, err := http.ParseTime(val); err == nil { - d := time.Until(t) - if d < 0 { - return 0 - } - return d + return max(0, time.Until(t)) } return 0 } From 8d7db2829c2ae2d3a5d818226f1787a2cec67562 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Thu, 10 Sep 2026 22:12:41 +0200 Subject: [PATCH 4/5] notify: take into account clock skew on server Signed-off-by: Solomon Jacobs --- notify/util.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/notify/util.go b/notify/util.go index d85de696f5..38d58ffc68 100644 --- a/notify/util.go +++ b/notify/util.go @@ -243,7 +243,7 @@ type Retrier struct { // ParseRetryAfter parses the Retry-After header value, which can be either // a delay in seconds (integer) or an HTTP-date. Returns zero if absent or unparseable. -func ParseRetryAfter(h http.Header) time.Duration { +func ParseRetryAfter(h http.Header, received time.Time) time.Duration { val := h.Get("Retry-After") if val == "" { return 0 @@ -255,7 +255,12 @@ func ParseRetryAfter(h http.Header) time.Duration { } // Try HTTP-date format. if t, err := http.ParseTime(val); err == nil { - return max(0, time.Until(t)) + // The date is on the server's clock, so time.Until would apply our skew. + now := received + if serverNow, err := http.ParseTime(h.Get("Date")); err == nil { + now = serverNow + } + return max(0, t.Sub(now)) } return 0 } From b81f7b603b28005d3102a550bff8bfde08647548 Mon Sep 17 00:00:00 2001 From: Solomon Jacobs Date: Thu, 10 Sep 2026 22:21:15 +0200 Subject: [PATCH 5/5] notify: unify slack Retry-After parsing onto notify.ParseRetryAfter Signed-off-by: Solomon Jacobs --- notify/slack/slack.go | 18 ++----------- notify/slack/slack_test.go | 20 -------------- notify/util_test.go | 55 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/notify/slack/slack.go b/notify/slack/slack.go index 64964db4ea..adfe53acdd 100644 --- a/notify/slack/slack.go +++ b/notify/slack/slack.go @@ -22,7 +22,6 @@ import ( "log/slog" "net/http" "os" - "strconv" "strings" "time" @@ -198,6 +197,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) } resp, err := n.postJSONFunc(ctx, n.client, u, &buf) + received := time.Now() if err != nil { if ctx.Err() != nil { err = fmt.Errorf("%w: %w", err, context.Cause(ctx)) @@ -211,7 +211,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) retry, err := n.retrier.Check(resp.StatusCode, resp.Body) if err != nil { if resp.StatusCode == http.StatusTooManyRequests { - if d := parseRetryAfter(resp.Header.Get("Retry-After")); d > 0 { + if d := notify.ParseRetryAfter(resp.Header, received); d > 0 { n.logger.Warn("Rate limited by Slack, waiting before retry", "retry_after_secs", d.Seconds()) select { case <-time.After(d): @@ -257,20 +257,6 @@ func (n *Notifier) slackResponseHandler(resp *http.Response, store *nflog.Store) return false, nil } -// parseRetryAfter parses the Retry-After header value as integer seconds -// and returns the corresponding duration. Returns 0 if the value is empty, -// not a valid integer, or non-positive. -func parseRetryAfter(val string) time.Duration { - if val == "" { - return 0 - } - seconds, err := strconv.Atoi(val) - if err != nil || seconds <= 0 { - return 0 - } - return time.Duration(seconds) * time.Second -} - // checkTextResponseError classifies plaintext responses from Slack. // A plaintext (non-JSON) response is successful if it's a string "ok". // This is typically a response for an Incoming Webhook diff --git a/notify/slack/slack_test.go b/notify/slack/slack_test.go index ba0e793907..076d1d7ca7 100644 --- a/notify/slack/slack_test.go +++ b/notify/slack/slack_test.go @@ -361,26 +361,6 @@ func TestSlackMessageField(t *testing.T) { } } -func TestParseRetryAfter(t *testing.T) { - tests := []struct { - name string - value string - expected time.Duration - }{ - {name: "valid integer", value: "30", expected: 30 * time.Second}, - {name: "empty string", value: "", expected: 0}, - {name: "non-integer", value: "abc", expected: 0}, - {name: "negative", value: "-5", expected: 0}, - {name: "zero", value: "0", expected: 0}, - {name: "float value", value: "1.5", expected: 0}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.expected, parseRetryAfter(tt.value)) - }) - } -} - func TestNotifier_Notify_RetryAfterSleep(t *testing.T) { apiurl, _ := url.Parse("https://slack.com/post.Message") notifier, err := New( diff --git a/notify/util_test.go b/notify/util_test.go index 2c2d4922e8..372894a2f3 100644 --- a/notify/util_test.go +++ b/notify/util_test.go @@ -24,6 +24,7 @@ import ( "reflect" "runtime" "testing" + "time" "github.com/prometheus/common/model" "github.com/prometheus/common/promslog" @@ -273,3 +274,57 @@ func TestGetFailureReasonFromStatusCode(t *testing.T) { }) } } + +func TestParseRetryAfter(t *testing.T) { + received := time.Date(2026, 9, 10, 12, 0, 0, 0, time.UTC) + // A client whose clock trails the server's must still get the delay right. + serverNow := received.Add(time.Minute) + + header := func(retryAfter, date string) http.Header { + h := http.Header{} + if retryAfter != "" { + h.Set("Retry-After", retryAfter) + } + if date != "" { + h.Set("Date", date) + } + return h + } + + for _, tc := range []struct { + name string + header http.Header + expected time.Duration + }{ + {name: "absent", header: header("", ""), expected: 0}, + {name: "valid integer", header: header("30", ""), expected: 30 * time.Second}, + {name: "non-integer", header: header("abc", ""), expected: 0}, + {name: "negative", header: header("-5", ""), expected: 0}, + {name: "zero", header: header("0", ""), expected: 0}, + {name: "float value", header: header("1.5", ""), expected: 0}, + { + name: "date against server clock", + header: header(serverNow.Add(30*time.Second).Format(http.TimeFormat), serverNow.Format(http.TimeFormat)), + expected: 30 * time.Second, + }, + { + name: "date without Date header", + header: header(received.Add(30*time.Second).Format(http.TimeFormat), ""), + expected: 30 * time.Second, + }, + { + name: "date with unparseable Date header", + header: header(received.Add(30*time.Second).Format(http.TimeFormat), "not a date"), + expected: 30 * time.Second, + }, + { + name: "date already elapsed", + header: header(serverNow.Add(-30*time.Second).Format(http.TimeFormat), serverNow.Format(http.TimeFormat)), + expected: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, ParseRetryAfter(tc.header, received)) + }) + } +}