From c84b0acdb4bd32bbd6f471cb20e05eccdfa10ef3 Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Thu, 23 Apr 2026 09:48:12 +0200 Subject: [PATCH] test(tsdb): add OOO error coverage for ST zero sample appends (#18554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(tsdb): add OOO error coverage for ST zero sample appends Add unit tests exercising the out-of-order error paths in AppendSTZeroSample, AppendHistogramSTZeroSample (AppenderV1), and the best-effort ST injection in AppenderV2.Append. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: György Krajcsovits * make format Signed-off-by: György Krajcsovits * test(tsdb): add TestHeadAppenderV2_BestEffortSTZeroSample_OOO The three OOO cases added to TestHeadAppenderV2_Append_EnableSTAsZeroSample use a single appender so headChunks is nil at append time; the zero sample enters the batch and is rejected silently in commitFloats, never reaching the error-handling branch at line 374 of bestEffortAppendSTZeroSample. Add a dedicated test that commits the first sample before appending the second. This makes headChunks non-nil, so appendFloat/appendHistogram/ appendFloatHistogram returns ErrOutOfOrderSample at append time and the branch at line 374 is actually executed. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: György Krajcsovits --------- Signed-off-by: György Krajcsovits Co-authored-by: Claude Sonnet 4.6 --- tsdb/head_append_v2_test.go | 103 ++++++++++++++++++++++++++++++++++++ tsdb/head_test.go | 82 ++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+) diff --git a/tsdb/head_append_v2_test.go b/tsdb/head_append_v2_test.go index 76b86fee45..b98ee5c10d 100644 --- a/tsdb/head_append_v2_test.go +++ b/tsdb/head_append_v2_test.go @@ -4436,6 +4436,59 @@ func TestHeadAppenderV2_Append_EnableSTAsZeroSample(t *testing.T) { } }(), }, + { + name: "ST is out of order/float", + appendableSamples: []appendableSamples{ + {ts: 200, fSample: 10}, + {ts: 300, fSample: 10, st: 100}, + }, + // ST results ErrOutOfOrderSample, but ST append is best effort, so + // ST should be ignored, but sample appended. + expectedSamples: func() []chunks.Sample { + return []chunks.Sample{ + sample{t: 200, f: 10}, + sample{t: 300, f: 10}, + } + }(), + }, + { + name: "ST is out of order/histogram", + appendableSamples: []appendableSamples{ + {ts: 200, h: testHistogram}, + {ts: 300, h: testHistogram, st: 100}, + }, + // ST results ErrOutOfOrderSample, but ST append is best effort, so + // ST should be ignored, but sample appended. + expectedSamples: func() []chunks.Sample { + // NOTE: Without ST, on query, first histogram sample will get + // CounterReset adjusted to UnknownCounterReset. + firstSample := testHistogram.Copy() + firstSample.CounterResetHint = histogram.UnknownCounterReset + return []chunks.Sample{ + sample{t: 200, h: firstSample}, + sample{t: 300, h: testHistogram}, + } + }(), + }, + { + name: "ST is out of order/floathistogram", + appendableSamples: []appendableSamples{ + {ts: 200, fh: testFloatHistogram}, + {ts: 300, fh: testFloatHistogram, st: 100}, + }, + // ST results ErrOutOfOrderSample, but ST append is best effort, so + // ST should be ignored, but sample appended. + expectedSamples: func() []chunks.Sample { + // NOTE: Without ST, on query, first histogram sample will get + // CounterReset adjusted to UnknownCounterReset. + firstSample := testFloatHistogram.Copy() + firstSample.CounterResetHint = histogram.UnknownCounterReset + return []chunks.Sample{ + sample{t: 200, fh: firstSample}, + sample{t: 300, fh: testFloatHistogram}, + } + }(), + }, } { t.Run(tc.name, func(t *testing.T) { opts := newTestHeadDefaultOptions(DefaultBlockDuration, false) @@ -4462,6 +4515,56 @@ func TestHeadAppenderV2_Append_EnableSTAsZeroSample(t *testing.T) { } } +func TestHeadAppenderV2_BestEffortSTZeroSample_OOO(t *testing.T) { + testHistogram := tsdbutil.GenerateTestHistogram(1) + testHistogram.CounterResetHint = histogram.NotCounterReset + testFloatHistogram := tsdbutil.GenerateTestFloatHistogram(1) + testFloatHistogram.CounterResetHint = histogram.NotCounterReset + + for _, tc := range []struct { + name string + v float64 + h *histogram.Histogram + fh *histogram.FloatHistogram + }{ + {name: "float", v: 10}, + {name: "histogram", h: testHistogram}, + {name: "floathistogram", fh: testFloatHistogram}, + } { + t.Run(tc.name, func(t *testing.T) { + opts := newTestHeadDefaultOptions(DefaultBlockDuration, false) + opts.EnableSTAsZeroSample = true + head, _ := newTestHeadWithOptions(t, compression.None, opts) + + lbls := labels.FromStrings("foo", "bar") + + // First appender: commit a sample at ts=200 to establish headMaxt and + // populate headChunks on the series. + app := head.AppenderV2(context.Background()) + _, err := app.Append(0, lbls, 0, 200, tc.v, tc.h, tc.fh, storage.AOptions{}) + require.NoError(t, err) + require.NoError(t, app.Commit()) + + // Second appender: st=100 is OOO (headMaxt=200, OOO disabled). Because the + // series now has committed data, bestEffortAppendSTZeroSample calls + // appendFloat/appendHistogram/appendFloatHistogram with fastRejectOOO=true, + // gets ErrOutOfOrderSample, and silently ignores it. The main sample must + // still be appended successfully. + app = head.AppenderV2(context.Background()) + _, err = app.Append(0, lbls, 100, 300, tc.v, tc.h, tc.fh, storage.AOptions{}) + require.NoError(t, err) + require.NoError(t, app.Commit()) + + q, err := NewBlockQuerier(head, math.MinInt64, math.MaxInt64) + require.NoError(t, err) + result := query(t, q, labels.MustNewMatcher(labels.MatchEqual, "foo", "bar")) + // Only the two main samples should be present; the OOO zero sample at ts=100 + // must have been dropped. + require.Len(t, result[`{foo="bar"}`], 2) + }) + } +} + // Regression test for data race https://github.com/prometheus/prometheus/issues/15139. func TestHeadAppenderV2_Append_HistogramAndCommitConcurrency(t *testing.T) { h := tsdbutil.GenerateTestHistogram(1) diff --git a/tsdb/head_test.go b/tsdb/head_test.go index 0bbdc4c12c..d4279185fe 100644 --- a/tsdb/head_test.go +++ b/tsdb/head_test.go @@ -7172,6 +7172,22 @@ func TestHeadAppender_AppendHistogramSTZeroSample(t *testing.T) { }, expectedError: storage.ErrDuplicateSampleForTimestamp, }, + { + name: "integer histogram ST is out of order", + appendableSamples: []appendableSamples{ + {ts: 200, h: tsdbutil.GenerateTestHistogram(1)}, + {ts: 300, h: tsdbutil.GenerateTestHistogram(1), st: 100}, + }, + expectedError: storage.ErrOutOfOrderST, + }, + { + name: "float histogram ST is out of order", + appendableSamples: []appendableSamples{ + {ts: 200, fh: tsdbutil.GenerateTestFloatHistogram(1)}, + {ts: 300, fh: tsdbutil.GenerateTestFloatHistogram(1), st: 100}, + }, + expectedError: storage.ErrOutOfOrderST, + }, } { t.Run(tc.name, func(t *testing.T) { h, _ := newTestHead(t, DefaultBlockDuration, compression.None, false) @@ -7195,6 +7211,72 @@ func TestHeadAppender_AppendHistogramSTZeroSample(t *testing.T) { } } +func TestHeadAppender_AppendSTZeroSample(t *testing.T) { + type appendableSample struct { + ts int64 + fSample float64 + st int64 + } + for _, tc := range []struct { + name string + oooEnabled bool + appendableSamples []appendableSample + expectedError error + }{ + { + name: "ST lower than minValidTime returns ErrOutOfBounds", + appendableSamples: []appendableSample{ + {ts: 100, fSample: 1.0, st: -1}, + }, + expectedError: storage.ErrOutOfBounds, + }, + { + name: "ST duplicates an existing sample", + appendableSamples: []appendableSample{ + {ts: 100, fSample: 1.0}, + {ts: 200, fSample: 2.0, st: 100}, + }, + expectedError: storage.ErrDuplicateSampleForTimestamp, + }, + { + name: "ST is out of order when OOO is disabled", + appendableSamples: []appendableSample{ + {ts: 200, fSample: 2.0}, + {ts: 300, fSample: 3.0, st: 100}, + }, + expectedError: storage.ErrOutOfOrderSample, + }, + { + name: "ST is out of order when OOO is enabled", + oooEnabled: true, + appendableSamples: []appendableSample{ + {ts: 200, fSample: 2.0}, + {ts: 300, fSample: 3.0, st: 100}, + }, + expectedError: storage.ErrOutOfOrderST, + }, + } { + t.Run(tc.name, func(t *testing.T) { + h, _ := newTestHead(t, DefaultBlockDuration, compression.None, tc.oooEnabled) + + lbls := labels.FromStrings("foo", "bar") + + var ref storage.SeriesRef + for _, sample := range tc.appendableSamples { + a := h.Appender(context.Background()) + var err error + if sample.st != 0 { + ref, err = a.AppendSTZeroSample(ref, lbls, sample.ts, sample.st) + require.ErrorIs(t, err, tc.expectedError) + } + ref, err = a.Append(ref, lbls, sample.ts, sample.fSample) + require.NoError(t, err) + require.NoError(t, a.Commit()) + } + }) + } +} + func TestHeadCompactableDoesNotCompactEmptyHead(t *testing.T) { // Use a chunk range of 1 here so that if we attempted to determine if the head // was compactable using default values for min and max times, `Head.compactable()`