fix: De-flake several flaky unit and integration tests by jnewbigin · Pull Request #23295 · grafana/loki

jnewbigin · 2026-07-16T01:02:20Z

What

Several unit tests fail intermittently on low-powered CI runners. I reproduced them
locally by compiling each package's test binary and running it under
golang.org/x/tools/cmd/stress with an oversubscribed CPU, a reduced GOMAXPROCS, and a
-race pass to mimic a slow, contended runner. This PR fixes the ones that reproduced.

Note: the loser.Tree mid-iteration duplicate fix that was originally part of this
PR has been split out into #23300 (Loki) and grafana/dskit#1021 (upstream), so it is no
longer included here. This PR is now the remaining flaky-test fixes plus the gcp retry
classification fix.

Fixes

test(canary) — NewComparator starts a background run() loop that these tests
never stopped. Its metric-test ticker could fire after the test began, running
metricTest on a comparator built with a nil reader (nil-pointer dereference) or
concurrently mutating the package-level metric gauges read by TestMetricTest (data
race). Stop the comparator in each test.
test(distributor) — TestPartitionWatcher_PicksUpChanges's Eventually only
checked that ShuffleSharder() returned a different object than the initial one, which
a freshly built sharder still holding the pre-update partitions satisfies. Assert on the
partition contents so the test waits for the KV update to actually propagate.
test(indexshipper) — the table manager's background loop runs an upload on startup,
and with a zero DBRetainPeriod the following cleanup drops indexes from the in-memory
set as soon as they are uploaded, racing the test's additions. Set a retain period.
test(cfg) — Test_DynamicUnmarshal created a temp config file with os.CreateTemp
but never removed or closed it, leaking a file and descriptor on every run. Use
t.TempDir and close the file.
fix(gcp) — TestTCPErrs asserted that a transport timeout is retryable via
http.Client.Timeout, but Go surfaces that timeout inconsistently under load (a
"Client.Timeout" error or a bare "context deadline exceeded"), so
IsStorageTimeoutErr classified it non-retryable and the test flaked. Drive the case
with the transport's ResponseHeaderTimeout (a deterministic server-side timeout) and
treat its error ("timeout awaiting response headers") as retryable — it wraps a
context deadline but, unlike a caller cancellation, unambiguously signals server
slowness. The fake server's sleeps are now interruptible so a long stall doesn't block
teardown.
test(integration) — TestLabelAccessTestCases's metrics-range-query case ran
count_over_time(...[2h]) as a range query and summed the value of every sample in the
returned matrix. Because the 2h lookback window is much larger than the range step, the
recently pushed lines fall inside several consecutive steps' windows, so each step
reports the full count and the summed total is a timing-dependent multiple of the real
line count (observed as exactly 2×). Take the largest per-step sum instead, which equals
the true count regardless of how many steps' windows overlap.

Verified

The unit-test fixes were hammered under a local stress harness (thousands of runs, plus
-race) and no longer reproduce; the integration fix was confirmed with repeated runs of
TestLabelAccessTestCases. All touched packages pass normally.

NewComparator starts a background run() loop that these tests never stopped. Its metric-test ticker could fire after the test had begun, running metricTest on a comparator constructed with a nil reader (a nil-pointer dereference) or concurrently mutating the package-level metric gauges read by TestMetricTest (a data race). Stop the comparator so the loop is torn down deterministically.

…her test The Eventually condition only checked that ShuffleSharder() returned a different object than the initial one, which is satisfied by a freshly built sharder that still holds the pre-update partitions. Assert on the partition contents so the test waits for the KV update to actually propagate before checking the result.

Based on grafana/loki#23295 from jnewbigin Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

Advance the current winner at the start of Push(). Add a test that would have caught the issue. Based on grafana/loki#23295 from jnewbigin Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

bboreham · 2026-07-16T11:01:39Z

+	if len(t.nodes) > 0 && t.nodes[0].index != -1 && t.nodes[t.nodes[0].index].index != -1 {
+		t.moveNext(t.nodes[0].index)
+	}


This fix should be in a different PR; it's a completely different category of change to the test fixes.

bboreham · 2026-07-16T11:02:42Z

+		// uploaded, so ForEach could miss just-added indexes. Retain them for
+		// the duration of the test.
+		DBRetainPeriod: time.Hour,


In what way does "one hour" equate to "the duration of the test"?

bboreham · 2026-07-16T11:03:49Z

+	// done unblocks the sleeps below at teardown, so a sleep set longer than the
+	// client's timeout (to reliably win the timeout race under load) does not
+	// stall server.Close().
+	done := make(chan struct{})


This could be t.Context() ?

bboreham · 2026-07-16T11:06:19Z

 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			server := fakeSleepingServer(t, tc.responseSleep, tc.connectSleep, tc.closeOnNew, tc.closeOnActive)
 			ctx, cancelFunc := context.WithTimeout(context.Background(), tc.clientTimeout)


why is this one called clientTimeout when it's given to the context?

bboreham · 2026-07-16T11:06:42Z

+				transport.ResponseHeaderTimeout = tc.responseHeaderTimeout
+			}
 			client.Transport = transport
 			client.Timeout = tc.connectTimeout


Why is this one called connectTimeout? Docstring for this field is "The timeout includes connection time, any redirects, and reading the response body."

bboreham · 2026-07-16T11:12:02Z

+		return strings.Contains(err.Error(), "Client.Timeout") ||
+			strings.Contains(err.Error(), "timeout awaiting response headers")


This is a change to non-test behaviour and needs to be listed as such in the PR description and changelog.

bboreham · 2026-07-16T11:13:36Z

 	require.Eventually(t, func() bool {
-		return watcher.ShuffleSharder() != initial
+		ss := watcher.ShuffleSharder()
+		return ss != initial && slices.Equal(ss.partitions, []int32{2})


Why do we still need to check ss != initial ? Wouldn't that be {1} ?

It would indeed. Will fix

bboreham · 2026-07-16T11:14:19Z

+	// Wait until the watcher reflects the new ring. Checking identity alone is
+	// racy: ShuffleSharder() can return a fresh object that still holds the
+	// pre-update partitions, so wait for the partitions themselves to change.


This comment belongs in the commit description not the code; it is mostly about what the old racy code used to do.

bboreham · 2026-07-16T11:17:32Z

+// reports the full count. Summing every sample would therefore multiply the true
+// count by the (timing-dependent) number of overlapping steps. The step whose


Again this is mostly about the old code that we can't see here, so belongs in the commit description not here.

bboreham · 2026-07-16T11:38:37Z

duplicate and misordered entries in tail output under load

I see how duplicates occur, but how would they get misordered?

The table manager's background loop runs an upload on startup, and with a zero DBRetainPeriod the following cleanup drops indexes from the in-memory set as soon as they are uploaded. That raced the test's additions, so ForEach could miss just-added indexes. Set a retain period so uploaded indexes are kept for the duration of the test.

The test created a temp config file with os.CreateTemp but never removed or closed it, leaking a file and file descriptor on every invocation. Use t.TempDir so the file is removed automatically and close it after writing.

TestTCPErrs asserted that a transport timeout is retryable by relying on http.Client.Timeout, but Go surfaces that timeout inconsistently under load -- sometimes as a "Client.Timeout" error and sometimes as a bare "context deadline exceeded" -- so IsStorageTimeoutErr classified it as non-retryable and the test flaked. Drive the case with the transport's ResponseHeaderTimeout instead, which is a deterministic server-side timeout, and treat its error ("timeout awaiting response headers") as retryable in IsStorageTimeoutErr: it wraps a context deadline but, unlike a caller cancellation, unambiguously signals server slowness. Also make the fake server's sleeps interruptible at teardown so a long stall doesn't block server.Close().

The metrics-range-query case runs count_over_time(...[2h]) as a range query and summed the value of every sample in the returned matrix. Because the 2h lookback window is much larger than the range step, the recently pushed lines fall inside several consecutive steps' windows, so each of those steps reports the full count and the summed total is a timing-dependent multiple of the real line count (it was observed as exactly 2x). Take the largest per-step sum instead, which equals the true count regardless of how many steps' windows overlap the data.

jnewbigin · 2026-07-17T01:32:49Z

how would they get misordered?

on re-init, the stale winner is re-selected immediately and emitted at the current output position, before the not-yet-consumed smaller-ordered entries from other leaves. Producing the sequence:
1,2,5,3,4,5 <- the first 5 is the misordered duplicate

jnewbigin added 2 commits July 16, 2026 10:45

jnewbigin requested a review from a team as a code owner July 16, 2026 01:02

jnewbigin changed the title ~~fix: De-flake unit tests and fix a loser-tree mid-iteration duplicate bug~~ fix: De-flake unit + integration tests and fix a loser-tree mid-iteration bug Jul 16, 2026

bboreham added a commit to grafana/dskit that referenced this pull request Jul 16, 2026

fix(loser): calling Push could result in seeing the same element twice

d03d5b0

Based on grafana/loki#23295 from jnewbigin Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

bboreham mentioned this pull request Jul 16, 2026

fix(loser): calling Push could result in seeing the same element twice grafana/dskit#1021

Open

1 task

bboreham requested changes Jul 16, 2026

View reviewed changes

bboreham mentioned this pull request Jul 16, 2026

fix(log tailer): Fix occasional duplicate entries #23300

Open

3 tasks

jnewbigin added 4 commits July 17, 2026 10:24

test(cfg): Clean up temp config file in Test_DynamicUnmarshal

14dfc81

The test created a temp config file with os.CreateTemp but never removed or closed it, leaking a file and file descriptor on every invocation. Use t.TempDir so the file is removed automatically and close it after writing.

jnewbigin force-pushed the jnewbigin/flakey-tests branch from 05b1edc to 5c6e8cc Compare July 17, 2026 00:42

jnewbigin changed the title ~~fix: De-flake unit + integration tests and fix a loser-tree mid-iteration bug~~ fix: De-flake unit + integration tests Jul 17, 2026

jnewbigin changed the title ~~fix: De-flake unit + integration tests~~ fix: De-flake several flaky unit and integration tests Jul 17, 2026

		return strings.Contains(err.Error(), "Client.Timeout") \|\|
		strings.Contains(err.Error(), "timeout awaiting response headers")

		// reports the full count. Summing every sample would therefore multiply the true
		// count by the (timing-dependent) number of overlapping steps. The step whose

Uh oh!

Conversation

jnewbigin commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Fixes

Verified

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bboreham commented Jul 16, 2026

Uh oh!

jnewbigin commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jnewbigin commented Jul 16, 2026 •

edited

Loading

jnewbigin commented Jul 17, 2026 •

edited

Loading