From 0be33b61f702852ebbf06e595ac52be3ca15a71b Mon Sep 17 00:00:00 2001 From: Daniel Blando Date: Tue, 20 Oct 2026 19:19:42 +0000 Subject: [PATCH 1/2] Ruler: stop retrying staleness markers that can never be accepted Signed-off-by: Daniel Blando --- CHANGELOG.md | 1 + pkg/ruler/compat.go | 35 +++++++++++++++ pkg/ruler/compat_test.go | 92 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe8e1f25f4..c44c5a331f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * [BUGFIX] Update prometheus alertmanager version to v0.34.1 to apply upstream inhibit rule bug fixes. #7877 * [BUGFIX] Tenant Federation: Fix regex tenant federation not working in single binary mode, and failing with `multiple org IDs present` when a matched tenant ID contains regex metacharacters and also matches other tenants as a regex. #7869 * [BUGFIX] Query Frontend: Fix the data selection time window computed for an instant query sent without a `time` parameter. The default evaluation time was passed to `util.ParseTimeParam` in milliseconds, but that helper formats the default and parses it as seconds, so the value overflowed `UnixNano()` and wrapped to a timestamp decades away from `now`. This made the `data_select_min_time` and `data_select_max_time` query stats wrong, and stopped the `time_window` query attribute of query priority and query rejection from matching such queries correctly. #7880 +* [BUGFIX] Ruler: Stop retrying staleness markers that can never be accepted. A rule moved between rule groups left the original group re-appending staleness markers for its series on every evaluation until the ruler restarted. #7888 ## 1.22.0 in progress * [CHANGE] Ruler: Remove the deprecated `-ruler.evaluation-delay-duration` flag and its `ruler_evaluation_delay_duration` per-tenant limit. Use `-ruler.query-offset` / `ruler_query_offset`, which no longer takes the higher of the two values. Cortex decodes the runtime config strictly, so a leftover `ruler_evaluation_delay_duration` override makes the runtime config fail to load: Cortex **exits at startup** (`module failed`, `module=runtime-config`), and on an already-running process every reload fails, pinning the last good overrides and dropping `cortex_runtime_config_last_reload_successful` to 0. Run `grep -r ruler_evaluation_delay_duration` over your runtime configs before upgrading. #7792 diff --git a/pkg/ruler/compat.go b/pkg/ruler/compat.go index d78a9636a60..7107c84e572 100644 --- a/pkg/ruler/compat.go +++ b/pkg/ruler/compat.go @@ -16,6 +16,7 @@ import ( "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/metadata" + "github.com/prometheus/prometheus/model/value" "github.com/prometheus/prometheus/notifier" "github.com/prometheus/prometheus/promql" "github.com/prometheus/prometheus/rules" @@ -97,6 +98,10 @@ func (a *PusherAppender) AppendExemplar(_ storage.SeriesRef, _ labels.Labels, _ func (a *PusherAppender) Commit() error { a.totalWrites.Inc() + // Computed before the samples are handed to ToWriteRequest, which takes + // ownership of them. + onlyStaleMarkers := a.onlyStaleMarkers() + req := cortexpb.ToWriteRequest(a.labels, a.samples, nil, nil, cortexpb.RULE) req.AddHistogramTimeSeries(a.histogramLabels, a.histograms) @@ -112,6 +117,19 @@ func (a *PusherAppender) Commit() error { // Don't report errors that ended with 4xx HTTP status code (series limits, duplicate samples, out of order, etc.) if resp, ok := httpgrpc.HTTPResponseFromError(err); !ok || resp.Code/100 == 5 { a.failedWrites.Inc() + } else if onlyStaleMarkers { + // A 4xx means the rejected samples will never be accepted, and that the + // rest of the batch was written. That is not the all-or-nothing rollback + // a non-nil Commit promises, and callers that retry on Commit failure + // take it literally: rules.Group.cleanupStaleSeries keeps its staleSeries + // list until Commit succeeds and re-appends the whole list on every + // evaluation, so a marker that can never be accepted is retried until the + // ruler restarts. + // + // Report success when the transaction is only staleness markers, so that + // the caller drops them. Transactions carrying rule results keep returning + // the error: the ruler reports it as rule health and does not retry it. + err = nil } } @@ -122,6 +140,23 @@ func (a *PusherAppender) Commit() error { return err } +// onlyStaleMarkers reports whether every sample in the transaction is a staleness +// marker. Prometheus writes those for series a rule no longer produces, as +// best-effort bookkeeping rather than tenant data, and rules.Group.cleanupStaleSeries +// builds a transaction containing nothing else. A transaction that also carries +// rule results is not one of them. +func (a *PusherAppender) onlyStaleMarkers() bool { + if len(a.samples) == 0 || len(a.histograms) > 0 { + return false + } + for _, s := range a.samples { + if !value.IsStaleNaN(s.Value) { + return false + } + } + return true +} + func (a *PusherAppender) UpdateMetadata(_ storage.SeriesRef, _ labels.Labels, _ metadata.Metadata) (storage.SeriesRef, error) { return 0, errors.New("update metadata unsupported") } diff --git a/pkg/ruler/compat_test.go b/pkg/ruler/compat_test.go index bd0ed3f9068..b62eafcb43c 100644 --- a/pkg/ruler/compat_test.go +++ b/pkg/ruler/compat_test.go @@ -312,6 +312,98 @@ func TestPusherErrors(t *testing.T) { } } +func TestPusherAppender_Commit_StaleMarkers(t *testing.T) { + staleNaN := math.Float64frombits(value.StaleNaN) + + for name, tc := range map[string]struct { + samples []float64 + withHistogram bool + returnedError error + expectedError error + expectedFailures int + }{ + "only staleness markers, 4xx: reported as success so the caller stops retrying": { + samples: []float64{staleNaN, staleNaN}, + returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + expectedError: nil, + }, + "only staleness markers, 429: also terminal": { + samples: []float64{staleNaN}, + returnedError: httpgrpc.Errorf(http.StatusTooManyRequests, "test error"), + expectedError: nil, + }, + "only staleness markers, 5xx: still an error, retrying is correct": { + samples: []float64{staleNaN}, + returnedError: httpgrpc.Errorf(http.StatusInternalServerError, "test error"), + expectedError: httpgrpc.Errorf(http.StatusInternalServerError, "test error"), + expectedFailures: 1, + }, + "only staleness markers, non-HTTP error: still an error": { + samples: []float64{staleNaN}, + returnedError: errors.New("test error"), + expectedError: errors.New("test error"), + expectedFailures: 1, + }, + "only staleness markers, no error": { + samples: []float64{staleNaN}, + returnedError: nil, + expectedError: nil, + }, + "markers alongside a rule result, 4xx: still an error, the ruler reports rule health from it": { + samples: []float64{staleNaN, 123456}, + returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + expectedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + }, + "rule results only, 4xx: still an error": { + samples: []float64{123456}, + returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + expectedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + }, + "staleness marker with a histogram, 4xx: still an error, cleanupStaleSeries never appends histograms": { + samples: []float64{staleNaN}, + withHistogram: true, + returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + expectedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + }, + "empty transaction, 4xx: still an error": { + returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + expectedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), + }, + } { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + pusher := &fakePusher{err: tc.returnedError, response: &cortexpb.WriteResponse{}} + + writes := prometheus.NewCounter(prometheus.CounterOpts{}) + failures := prometheus.NewCounter(prometheus.CounterOpts{}) + + pa := NewPusherAppendable(pusher, "user-1", &ruleLimits{}, writes, failures) + + lbls, err := parser.ParseMetric("foo_bar") + require.NoError(t, err) + + a := pa.Appender(ctx) + for _, v := range tc.samples { + _, err = a.Append(0, lbls, int64(model.Now()), v) + require.NoError(t, err) + } + if tc.withHistogram { + _, err = a.AppendHistogram(0, lbls, int64(model.Now()), tsdbutil.GenerateTestHistogram(1), nil) + require.NoError(t, err) + } + + if tc.expectedError == nil { + require.NoError(t, a.Commit()) + } else { + require.Equal(t, tc.expectedError, a.Commit()) + } + + require.Equal(t, 1, int(testutil.ToFloat64(writes))) + require.Equal(t, tc.expectedFailures, int(testutil.ToFloat64(failures))) + }) + } +} + func TestMetricsQueryFuncErrors(t *testing.T) { for name, tc := range map[string]struct { returnedError error From fb5a55eaae8309cfab14709e2bd3ee0cbcb16267 Mon Sep 17 00:00:00 2001 From: Daniel Blando Date: Wed, 21 Oct 2026 18:10:40 +0000 Subject: [PATCH 2/2] Keep retrying staleness markers on 429 Signed-off-by: Daniel Blando --- pkg/ruler/compat.go | 27 +++++++++------------------ pkg/ruler/compat_test.go | 4 ++-- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/pkg/ruler/compat.go b/pkg/ruler/compat.go index 7107c84e572..a6dd2a1f76d 100644 --- a/pkg/ruler/compat.go +++ b/pkg/ruler/compat.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/http" "net/url" "time" @@ -98,8 +99,7 @@ func (a *PusherAppender) AppendExemplar(_ storage.SeriesRef, _ labels.Labels, _ func (a *PusherAppender) Commit() error { a.totalWrites.Inc() - // Computed before the samples are handed to ToWriteRequest, which takes - // ownership of them. + // Computed before ToWriteRequest takes ownership of the samples. onlyStaleMarkers := a.onlyStaleMarkers() req := cortexpb.ToWriteRequest(a.labels, a.samples, nil, nil, cortexpb.RULE) @@ -117,18 +117,12 @@ func (a *PusherAppender) Commit() error { // Don't report errors that ended with 4xx HTTP status code (series limits, duplicate samples, out of order, etc.) if resp, ok := httpgrpc.HTTPResponseFromError(err); !ok || resp.Code/100 == 5 { a.failedWrites.Inc() - } else if onlyStaleMarkers { - // A 4xx means the rejected samples will never be accepted, and that the - // rest of the batch was written. That is not the all-or-nothing rollback - // a non-nil Commit promises, and callers that retry on Commit failure - // take it literally: rules.Group.cleanupStaleSeries keeps its staleSeries - // list until Commit succeeds and re-appends the whole list on every - // evaluation, so a marker that can never be accepted is retried until the - // ruler restarts. - // - // Report success when the transaction is only staleness markers, so that - // the caller drops them. Transactions carrying rule results keep returning - // the error: the ruler reports it as rule health and does not retry it. + } else if onlyStaleMarkers && resp.Code != http.StatusTooManyRequests { + // cleanupStaleSeries keeps its staleSeries list until Commit succeeds, so a + // permanent rejection makes it re-append the whole list on every evaluation + // until the ruler restarts. Report success so it drops them instead. Markers + // only: with rule results the ruler surfaces the error as rule health. 429 is + // excluded, the rate limiter rejects the whole request and is transient. err = nil } } @@ -141,10 +135,7 @@ func (a *PusherAppender) Commit() error { } // onlyStaleMarkers reports whether every sample in the transaction is a staleness -// marker. Prometheus writes those for series a rule no longer produces, as -// best-effort bookkeeping rather than tenant data, and rules.Group.cleanupStaleSeries -// builds a transaction containing nothing else. A transaction that also carries -// rule results is not one of them. +// marker, as in a transaction built by rules.Group.cleanupStaleSeries. func (a *PusherAppender) onlyStaleMarkers() bool { if len(a.samples) == 0 || len(a.histograms) > 0 { return false diff --git a/pkg/ruler/compat_test.go b/pkg/ruler/compat_test.go index b62eafcb43c..0fd5d258bad 100644 --- a/pkg/ruler/compat_test.go +++ b/pkg/ruler/compat_test.go @@ -327,10 +327,10 @@ func TestPusherAppender_Commit_StaleMarkers(t *testing.T) { returnedError: httpgrpc.Errorf(http.StatusBadRequest, "test error"), expectedError: nil, }, - "only staleness markers, 429: also terminal": { + "only staleness markers, 429: still an error, the rate limiter rejected the whole request and the limit is transient": { samples: []float64{staleNaN}, returnedError: httpgrpc.Errorf(http.StatusTooManyRequests, "test error"), - expectedError: nil, + expectedError: httpgrpc.Errorf(http.StatusTooManyRequests, "test error"), }, "only staleness markers, 5xx: still an error, retrying is correct": { samples: []float64{staleNaN},