Flaky pkg/compactor tests: sub-second Poll budgets and polling a different signal than the assertion

[CharlieTLe]

Summary

pkg/compactor tests flake in the test (amd64) / test (arm64) CI jobs (make test, i.e.
go test -race). Across eight PRs opened on 2026-08-19 the package failed 5 times, each time
with a different test, and every failure passed on rerun without a code change.

There are two distinct mechanisms, both of which are test-synchronisation bugs rather than product
bugs. Neither is a new regression — the older one dates to #1942 (2020).

This is the same class as #7565, which fixed two of these this cycle by polling on a better signal,
and is adjacent to #7607 / #7617. Filing separately because the specific tests and lines below are
not covered by those.

Observed failures

All on runs against master (a330338) plus unrelated changes. Notably #7792 touches only
pkg/util/validation and pkg/ruler, and #7790/#7791 are strict subsets of it that both passed —
so nothing in these PRs is implicated.

PR	Test	Assertion	Package runtime
#7788	TestPartitionCompactor_ShouldSkipOutOrOrderBlocks	compactor_paritioning_test.go:1027 counter 0 vs expected 1	209s
#7788	TestCompactor_ShouldIterateOverUsersAndRunCompaction	compactor_test.go:530: expected 1, got 0	327s
#7792	TestCompactor_ShouldIncrementCompactionErrorIfFailedToCompactASingleTenant	compactor_test.go:395: expected 1, got 0	—
#7792	TestCompactor_ShouldIterateOverUsersAndRunCompaction	compactor_test.go:530: expected 1, got 0	323s
#7792	TestCompactor_ShouldIncrementCompactionErrorIfFailedToCompactASingleTenant	compactor_test.go:395: expected 1, got 0	293s

Locally the package passes 3/3 with -race in isolation, including all of the above. It only
fails as part of ./..., where the package itself takes 209–327s under -race — i.e. the runner is
heavily contended.

Mechanism 1 — a one-second Poll budget for a full compaction run

compactor_test.go:395 and :530 both wait for a compaction run to finish with a 1 second budget:

// Wait until a run has completed.
cortex_testutil.Poll(t, time.Second, 1.0, func() any {
    return prom_testutil.ToFloat64(c.CompactionRunsCompleted)
})

expected 1, got 0 is this Poll timing out. One second is not a meaningful budget for starting the
compactor service, discovering users, and completing a run on a contended runner under -race.

The budgets in this package are inconsistent — of 41 Poll calls:

Budget	Count
20*time.Second	17
time.Second / 1*time.Second	12
120*time.Second	3
60*time.Second	2
others (5s, 10s, 240s, 5000ms)	7

The 1s form originates in #1942 (2020-01-21, "Introduce TSDB blocks compactor") and was copied into
TestCompactor_ShouldIncrementCompactionErrorIfFailedToCompactASingleTenant by #4094 (2021-04-21).
git blame misleadingly points at #7005, which only rewrote interface{} → any.

Mechanism 2 — polling one signal, then asserting on another

compactor_paritioning_test.go:1019-1031 polls for the marker file and then immediately asserts
on the counter:

cortex_testutil.Poll(t, 20*time.Second, true, func() any {
    if _, err := os.Stat(path.Join(dir, "no-compact-mark.json")); err == nil {
        return true
    }
    return false
})

assert.NoError(t, prom_testutil.GatherAndCompare(registry, strings.NewReader(`
        cortex_compactor_blocks_marked_for_no_compaction_total 1
    `), "cortex_compactor_blocks_marked_for_no_compaction_total"))

The counter is incremented separately from the file write, so the file can exist before the counter is
bumped. The 20s budget is fine here; the bug is that the polled signal is not the asserted one. This is
exactly what #7565 fixed elsewhere in this package.

Proposed fixes

1. Mechanism 1 — raise the 12 sub-second budgets to the package's prevailing 20*time.Second.
   Poll returns as soon as the condition holds, so a larger budget costs nothing on a healthy run
   and only buys headroom on a loaded one.
2. Mechanism 2 — poll on the value actually asserted (the counter), and keep the file check as a
   precondition rather than the sole gate.
3. Optionally, add a lint or review check against sub-second Poll budgets in service-lifecycle tests,
   since the pattern keeps getting copied into new tests.

Happy to send a PR for 1 and 2 if that's welcome.

TestCompactor_ShouldIterateOverUsersAndRunCompaction — #7788, run 32320647761

go test -tags "netgo slicelabels" -timeout 30m -race -count 1 ./...
...
--- FAIL: TestCompactor_ShouldIterateOverUsersAndRunCompaction (1.06s)
    compactor_test.go:530: expected 1, got 0
FAIL
FAIL	github.com/cortexproject/cortex/pkg/compactor	327.440s

TestPartitionCompactor_ShouldSkipOutOrOrderBlocks — #7788, run 32320647761

--- FAIL: TestPartitionCompactor_ShouldSkipOutOrOrderBlocks (1.22s)
    compactor_paritioning_test.go:1027:
        	Error Trace:	/__w/cortex/cortex/pkg/compactor/compactor_paritioning_test.go:1027
        	Error:      	Received unexpected error:
        	            	 # HELP cortex_compactor_blocks_marked_for_no_compaction_total Total number of blocks marked for no compact during a compaction run.
        	            	 # TYPE cortex_compactor_blocks_marked_for_no_compaction_total counter
        	            	-cortex_compactor_blocks_marked_for_no_compaction_total 0
        	            	+cortex_compactor_blocks_marked_for_no_compaction_total 1
        	            	 
        	Test:       	TestPartitionCompactor_ShouldSkipOutOrOrderBlocks
FAIL
FAIL	github.com/cortexproject/cortex/pkg/compactor	209.875s

TestCompactor_ShouldIncrementCompactionErrorIfFailedToCompactASingleTenant — #7792, run 32320635278

    compactor_test.go:395: expected 1, got 0
--- FAIL: TestCompactor_ShouldIncrementCompactionErrorIfFailedToCompactASingleTenant (1.14s)
FAIL	github.com/cortexproject/cortex/pkg/compactor	293.744s

[CharlieTLe]

Fix in #7796. Writing it turned up two refinements to the analysis above, both worth recording here.

The dominant term in mechanism 1 is jitter, not run duration. Compactor.running() sleeps for up
to 10% of CompactionInterval before the initial compaction:

// Insert jitter right before compaction starts to avoid multiple starting compactor to be in sync
case <-time.After(time.Duration(rand.Int63n(int64(float64(c.compactorCfg.CompactionInterval) * 0.1)))):
}
c.compactUsers(ctx)

With prepareConfig()'s 5s interval that is up to 500ms, so half the one-second budget can be gone
before compaction starts. That is a sharper explanation than "one second is not a meaningful budget
for a run".

It also invalidates one of the fixes I suggested: raising CompactionInterval would make this worse,
because the jitter is derived from it. Ignore that idea.

Raising the budget to 20s is safe, for a non-obvious reason. Several of these tests assert exact
state after the Poll (AssertNumberOfCalls(t, "Plan", 2), exact log matching, exact counter totals),
so I expected a longer budget to risk a second compaction run perturbing them. It doesn't:
running() starts the interval ticker only after the initial compactUsers returns, so runs never
overlap and the counter holds its post-run-1 value for a full interval (>=5s); and Poll returns on
first match, with granularity d/100 = 200ms at 20s. The tests still stop the compactor well before
run 2 — they just tolerate a slow run 1.

So the 1s budget was not arbitrary; it was implicitly relying on the run being fast. Making the
tolerance explicit is the fix.

Proposal 3 above (a lint against sub-second Poll budgets) is not in #7796 — happy to follow up
separately if it's wanted.