[Bugfix] vParquetX: race conditions by ruslan-mikhailov · Pull Request #6773 · grafana/tempo

ruslan-mikhailov · 2026-03-25T12:09:07Z

What this PR does: fixes race conditions found by TestWalBlockRaceConditionCheck ran with -race -count=10

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copilot

Pull request overview

This PR addresses data races in the vParquet WAL block implementations (vparquet3/4/5), aiming to make concurrent reads (search/fetch/iterate) safe while writes/flushes are happening.

Changes:

Add mutex protection around flushedSize / unflushedSize updates and DataLength() reads.
Avoid iterating b.flushed directly by routing Iterator() / FindTraceByID() through readFlushes().
Add a new concurrent “race condition check” test for WAL blocks in vparquet3/4/5.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tempodb/encoding/vparquet5/wal_block.go	Synchronizes size accounting and iteration over flushed WAL pages.
tempodb/encoding/vparquet4/wal_block.go	Same synchronization changes for vparquet4.
tempodb/encoding/vparquet3/wal_block.go	Same synchronization changes for vparquet3.
tempodb/encoding/vparquet5/wal_block_test.go	Adds a concurrent stress test intended to reproduce races under `-race`.
tempodb/encoding/vparquet4/wal_block_test.go	Adds the same concurrent race-check test for vparquet4.
tempodb/encoding/vparquet3/wal_block_test.go	Adds the same concurrent race-check test for vparquet3.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

ruslan-mikhailov · 2026-03-25T14:00:09Z

@@ -355,7 +355,9 @@ func (b *walBlock) AppendTrace(id common.ID, trace *tempopb.Trace, start, end ui
 	b.meta.ObjectAdded(start, end)


I need to add to changelog

mdisibio · 2026-03-25T14:01:39Z

 	b.meta.ObjectAdded(start, end)
 	b.ids.Set(id, int64(b.ids.Len())) // Next row number

+	b.mtx.Lock()


nit: Since this is at the end of the method, we could defer the unlock and stylistically I think that reads better.

ruslan-mikhailov · 2026-03-25T14:45:58Z

+ rebase to resolve conflicts in changelog

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Copilot · 2026-03-25T14:53:15Z

+		"Iterator":      func() { _, _ = w.Iterator() },
+		"DataLength":    func() { _ = w.DataLength() },


The race-check test calls w.Iterator() repeatedly but never closes the returned iterator. walBlock.Iterator() opens parquet files (via rowIterator()), so not closing can leak file descriptors and make this test flaky (or stop exercising races once it hits OS limits). Make the reader close the iterator each time (even if you ignore the error).

Suggested change

"Iterator": func() { _, _ = w.Iterator() },

"DataLength": func() { _ = w.DataLength() },

"Iterator": func() {

it, _ := w.Iterator()

if it != nil {

_ = it.Close()

}

},

"DataLength": func() { _ = w.DataLength() },

Copilot · 2026-03-25T14:53:15Z

+		"Fetch":      func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
+		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },


The Fetch / FetchSpans reader functions drop the response without closing resp.Results. Those iterators keep page files open until Close() is called, so this can leak file descriptors across the loop and make the race test flaky. Close Results (preferably in a defer) when the call succeeds.

Suggested change

"Fetch": func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },

"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },

"Fetch": func() {

resp, err := w.Fetch(ctx, traceql.FetchSpansRequest{}, opts)

if err != nil || resp == nil || resp.Results == nil {

return

}

_ = resp.Results.Close()

},

"FetchSpans": func() {

resp, err := w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts)

if err != nil || resp == nil || resp.Results == nil {

return

}

_ = resp.Results.Close()

},

Copilot · 2026-03-25T14:53:15Z

+	readers := map[string]func(){
+		"FindTraceByID": func() { _, _ = w.FindTraceByID(ctx, id, opts) },
+		"Search":        func() { _, _ = w.Search(ctx, &tempopb.SearchRequest{}, opts) },
+		"Iterator":      func() { _, _ = w.Iterator() },


The race-check test calls w.Iterator() repeatedly but never closes the returned iterator. walBlock.Iterator() opens parquet files (via rowIterator()), so not closing can leak file descriptors and make this test flaky (or stop exercising races once it hits OS limits). Make the reader close the iterator each time (even if you ignore the error).

Suggested change

"Iterator": func() { _, _ = w.Iterator() },

"Iterator": func() { it, _ := w.Iterator(); if it != nil { _ = it.Close() } },

Copilot · 2026-03-25T14:53:16Z

+		"Fetch":      func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
+		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },


The Fetch / FetchSpans reader functions drop the response without closing resp.Results. Those iterators keep page files open until Close() is called, so this can leak file descriptors across the loop and make the race test flaky. Close Results (preferably in a defer) when the call succeeds.

Suggested change

"Fetch": func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },

"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },

"Fetch": func() {

resp, _ := w.Fetch(ctx, traceql.FetchSpansRequest{}, opts)

if resp != nil && resp.Results != nil {

defer resp.Results.Close()

}

},

"FetchSpans": func() {

resp, _ := w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts)

if resp != nil && resp.Results != nil {

defer resp.Results.Close()

}

},

Copilot · 2026-03-25T14:53:16Z

+		"Iterator":      func() { _, _ = w.Iterator() },
+		"DataLength":    func() { _ = w.DataLength() },


The race-check test calls w.Iterator() repeatedly but never closes the returned iterator. walBlock.Iterator() opens parquet files (via rowIterator()), so not closing can leak file descriptors and make this test flaky (or stop exercising races once it hits OS limits). Make the reader close the iterator each time (even if you ignore the error).

Suggested change

"Iterator": func() { _, _ = w.Iterator() },

"DataLength": func() { _ = w.DataLength() },

"Iterator": func() {

it, err := w.Iterator()

if err != nil {

return

}

_ = it.Close()

},

"DataLength": func() { _ = w.DataLength() },

Copilot · 2026-03-25T14:53:16Z

+		"Fetch":      func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
+		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },


The Fetch / FetchSpans reader functions drop the response without closing resp.Results. Those iterators keep page files open until Close() is called, so this can leak file descriptors across the loop and make the race test flaky. Close Results (preferably in a defer) when the call succeeds.

Suggested change

"Fetch": func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },

"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },

"Fetch": func() {

resp, err := w.Fetch(ctx, traceql.FetchSpansRequest{}, opts)

if err != nil {

return

}

defer resp.Results.Close()

},

"FetchSpans": func() {

resp, err := w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts)

if err != nil {

return

}

defer resp.Results.Close()

},

Copilot AI review requested due to automatic review settings March 25, 2026 12:09

Copilot started reviewing on behalf of ruslan-mikhailov March 25, 2026 12:09 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

ruslan-mikhailov force-pushed the bugfix/vparquetx-race-conditions branch from 87de5ed to cd681bb Compare March 25, 2026 12:45

Copilot AI review requested due to automatic review settings March 25, 2026 13:02

ruslan-mikhailov force-pushed the bugfix/vparquetx-race-conditions branch from cd681bb to 5b78063 Compare March 25, 2026 13:02

Copilot started reviewing on behalf of ruslan-mikhailov March 25, 2026 13:02 View session

ruslan-mikhailov force-pushed the bugfix/vparquetx-race-conditions branch from 5b78063 to de6cafa Compare March 25, 2026 13:06

Copilot AI reviewed Mar 25, 2026

View reviewed changes

ruslan-mikhailov marked this pull request as ready for review March 25, 2026 13:23

ruslan-mikhailov requested review from carles-grafana, electron0zero, ie-pham, javiermolinar, mapno, mattdurham, mdisibio, oleg-kozlyuk-grafana, stoewer, yvrhdn and zalegrala as code owners March 25, 2026 13:23

ruslan-mikhailov commented Mar 25, 2026

View reviewed changes

mdisibio reviewed Mar 25, 2026

View reviewed changes

ruslan-mikhailov force-pushed the bugfix/vparquetx-race-conditions branch from de6cafa to 1220425 Compare March 25, 2026 14:42

ruslan-mikhailov added 5 commits March 25, 2026 15:44

vparquet5: test for race condition

dfc2a4e

vparquet5: race condition bugfix

b2d54bc

vparquetX: test for race condition

cfafaae

vparquetX: race condition bugfix

d384f2f

Changelog

b2456d6

Copilot AI review requested due to automatic review settings March 25, 2026 14:45

ruslan-mikhailov force-pushed the bugfix/vparquetx-race-conditions branch from 1220425 to b2456d6 Compare March 25, 2026 14:45

Copilot started reviewing on behalf of ruslan-mikhailov March 25, 2026 14:46 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

mdisibio approved these changes Mar 25, 2026

View reviewed changes

ruslan-mikhailov merged commit 8d030c8 into grafana:main Mar 26, 2026
31 checks passed

ruslan-mikhailov deleted the bugfix/vparquetx-race-conditions branch March 26, 2026 09:09

		@@ -355,7 +355,9 @@ func (b walBlock) AppendTrace(id common.ID, trace tempopb.Trace, start, end ui
		b.meta.ObjectAdded(start, end)

		"Iterator": func() { _, _ = w.Iterator() },
		"DataLength": func() { _ = w.DataLength() },

		"Fetch": func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },

-		"Fetch":      func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
-		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },
+		"Fetch": func() {
+			resp, err := w.Fetch(ctx, traceql.FetchSpansRequest{}, opts)
+			if err != nil || resp == nil || resp.Results == nil {
+				return
+			}
+			_ = resp.Results.Close()
+		},
+		"FetchSpans": func() {
+			resp, err := w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts)
+			if err != nil || resp == nil || resp.Results == nil {
+				return
+			}
+			_ = resp.Results.Close()
+		},

	"Iterator": func() { _, _ = w.Iterator() },
	"Iterator": func() { it, _ := w.Iterator(); if it != nil { _ = it.Close() } },

-		"Fetch":      func() { _, _ = w.Fetch(ctx, traceql.FetchSpansRequest{}, opts) },
-		"FetchSpans": func() { _, _ = w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts) },
+		"Fetch": func() {
+			resp, _ := w.Fetch(ctx, traceql.FetchSpansRequest{}, opts)
+			if resp != nil && resp.Results != nil {
+				defer resp.Results.Close()
+			}
+		},
+		"FetchSpans": func() {
+			resp, _ := w.FetchSpans(ctx, traceql.FetchSpansRequest{}, opts)
+			if resp != nil && resp.Results != nil {
+				defer resp.Results.Close()
+			}
+		},

Conversation

ruslan-mikhailov commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ruslan-mikhailov Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

ruslan-mikhailov Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mdisibio Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

ruslan-mikhailov Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruslan-mikhailov commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants