From eefcbb56e070ba8a11f19a86048de829c67839b7 Mon Sep 17 00:00:00 2001 From: Ben Clive Date: Wed, 22 May 2024 11:45:45 +0100 Subject: [PATCH 1/2] fix: Fix panic on requesting out-of-order Pattern samples --- pkg/pattern/drain/chunk.go | 7 ++++++- pkg/pattern/drain/chunk_test.go | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/pattern/drain/chunk.go b/pkg/pattern/drain/chunk.go index 9b1e34e2e3a19..4c43da3245986 100644 --- a/pkg/pattern/drain/chunk.go +++ b/pkg/pattern/drain/chunk.go @@ -51,7 +51,7 @@ func (c Chunk) ForRange(start, end, step model.Time) []logproto.PatternSample { } first := c.Samples[0].Timestamp last := c.Samples[len(c.Samples)-1].Timestamp - if start >= end || first >= end || last < start { + if last < first || start >= end || first >= end || last < start { return nil } var lo int @@ -114,6 +114,11 @@ func (c *Chunks) Add(ts model.Time) { Timestamp: t, Value: 1, }) + if len(last.Samples) > 1 && last.Samples[len(last.Samples)-2].Timestamp > t { + sort.SliceStable(last.Samples, func(i, j int) bool { + return last.Samples[i].Timestamp < last.Samples[j].Timestamp + }) + } } func (c Chunks) Iterator(pattern string, from, through, step model.Time) iter.Iterator { diff --git a/pkg/pattern/drain/chunk_test.go b/pkg/pattern/drain/chunk_test.go index 4863a6629729a..db7a42be6a571 100644 --- a/pkg/pattern/drain/chunk_test.go +++ b/pkg/pattern/drain/chunk_test.go @@ -21,6 +21,10 @@ func TestAdd(t *testing.T) { cks.Add(model.TimeFromUnixNano(time.Hour.Nanoseconds()) + TimeResolution + 1) require.Equal(t, 2, len(cks)) require.Equal(t, 1, len(cks[1].Samples)) + cks.Add(model.TimeFromUnixNano(time.Hour.Nanoseconds()) - TimeResolution) + require.Equal(t, 2, len(cks)) + require.Equal(t, 2, len(cks[1].Samples)) + require.Lessf(t, cks[1].Samples[0].Timestamp, cks[1].Samples[1].Timestamp, "Samples should be sorted if inserted out of order") } func TestIterator(t *testing.T) { @@ -52,6 +56,7 @@ func TestForRange(t *testing.T) { c *Chunk start model.Time end model.Time + step model.Time expected []logproto.PatternSample }{ { @@ -180,6 +185,16 @@ func TestForRange(t *testing.T) { {Timestamp: 4, Value: 10}, }, }, + { + name: "Out-of-order samples generate nil result", + c: &Chunk{Samples: []logproto.PatternSample{ + {Timestamp: 5, Value: 2}, + {Timestamp: 3, Value: 2}, + }}, + start: 4, + end: 6, + expected: nil, + }, } for _, tc := range testCases { From 31750fae9491f62009fa626afd0ce7b53a4a85f1 Mon Sep 17 00:00:00 2001 From: Ben Clive Date: Wed, 22 May 2024 13:40:33 +0100 Subject: [PATCH 2/2] Reject out-of-order samples instead of sorting them --- pkg/pattern/drain/chunk.go | 15 +++++++++------ pkg/pattern/drain/chunk_test.go | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/pattern/drain/chunk.go b/pkg/pattern/drain/chunk.go index 4c43da3245986..1333299467585 100644 --- a/pkg/pattern/drain/chunk.go +++ b/pkg/pattern/drain/chunk.go @@ -51,7 +51,7 @@ func (c Chunk) ForRange(start, end, step model.Time) []logproto.PatternSample { } first := c.Samples[0].Timestamp last := c.Samples[len(c.Samples)-1].Timestamp - if last < first || start >= end || first >= end || last < start { + if start >= end || first >= end || last < start { return nil } var lo int @@ -66,6 +66,11 @@ func (c Chunk) ForRange(start, end, step model.Time) []logproto.PatternSample { return c.Samples[i].Timestamp >= end }) } + + if c.Samples[lo].Timestamp > c.Samples[hi-1].Timestamp { + return nil + } + if step == TimeResolution { return c.Samples[lo:hi] } @@ -110,15 +115,13 @@ func (c *Chunks) Add(ts model.Time) { *c = append(*c, newChunk(t)) return } + if ts.Before(last.Samples[len(last.Samples)-1].Timestamp) { + return + } last.Samples = append(last.Samples, logproto.PatternSample{ Timestamp: t, Value: 1, }) - if len(last.Samples) > 1 && last.Samples[len(last.Samples)-2].Timestamp > t { - sort.SliceStable(last.Samples, func(i, j int) bool { - return last.Samples[i].Timestamp < last.Samples[j].Timestamp - }) - } } func (c Chunks) Iterator(pattern string, from, through, step model.Time) iter.Iterator { diff --git a/pkg/pattern/drain/chunk_test.go b/pkg/pattern/drain/chunk_test.go index db7a42be6a571..17429da594e19 100644 --- a/pkg/pattern/drain/chunk_test.go +++ b/pkg/pattern/drain/chunk_test.go @@ -23,8 +23,7 @@ func TestAdd(t *testing.T) { require.Equal(t, 1, len(cks[1].Samples)) cks.Add(model.TimeFromUnixNano(time.Hour.Nanoseconds()) - TimeResolution) require.Equal(t, 2, len(cks)) - require.Equal(t, 2, len(cks[1].Samples)) - require.Lessf(t, cks[1].Samples[0].Timestamp, cks[1].Samples[1].Timestamp, "Samples should be sorted if inserted out of order") + require.Equalf(t, 1, len(cks[1].Samples), "Older samples should not be added if they arrive out of order") } func TestIterator(t *testing.T) { @@ -195,6 +194,18 @@ func TestForRange(t *testing.T) { end: 6, expected: nil, }, + { + name: "Internally out-of-order samples generate nil result", + c: &Chunk{Samples: []logproto.PatternSample{ + {Timestamp: 1, Value: 2}, + {Timestamp: 5, Value: 2}, + {Timestamp: 3, Value: 2}, + {Timestamp: 7, Value: 2}, + }}, + start: 2, + end: 6, + expected: nil, + }, } for _, tc := range testCases {