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.

Note: the TestTCPErrs (gcp) de-flake has also been dropped from this PR.
De-flaking it deterministically required changing IsStorageTimeoutErr to retry the
"timeout awaiting response headers" error — a non-test behaviour change that does not
belong in a test-only de-flake PR — so it is tracked separately in #23350. This PR is
now test-only changes.

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.
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: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: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

…contents Drop the redundant ShuffleSharder() identity check and the accompanying code comment.

Reword to describe why the retain period is set rather than implying one hour exceeds the expected test duration.

…Errs" This reverts commit 1b6e460. De-flaking TestTCPErrs deterministically required changing IsStorageTimeoutErr to retry the "timeout awaiting response headers" error -- a non-test behaviour change that does not belong in this test-only de-flake PR. Drop it here; the retry classification can be handled separately.

Drop the narration of the previous sum-every-sample approach from the doc comment; that explanation already lives in the de-flake commit. Keep only a short description of what maxStepValue returns.

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

Merged

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

jnewbigin added 4 commits July 20, 2026 10:43

test(distributor): Simplify partition watcher assertion to partition …

c7db864

…contents Drop the redundant ShuffleSharder() identity check and the accompanying code comment.

test(indexshipper): Clarify DBRetainPeriod comment

269b8e4

Reword to describe why the retain period is set rather than implying one hour exceeds the expected test duration.

test(integration): Trim maxStepValue comment to current behaviour

ff1bed0

Drop the narration of the previous sum-every-sample approach from the doc comment; that explanation already lives in the de-flake commit. Keep only a short description of what maxStepValue returns.

jnewbigin mentioned this pull request Jul 20, 2026

TestTCPErrs is flaky under load; de-flaking requires an IsStorageTimeoutErr behaviour change #23350

Open

jnewbigin requested a review from bboreham July 20, 2026 01:20

bboreham approved these changes Jul 20, 2026

View reviewed changes

bboreham merged commit 562a762 into main Jul 20, 2026
86 checks passed

bboreham deleted the jnewbigin/flakey-tests branch July 20, 2026 08:38

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!

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!

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!

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