Skip to content

indexheader: replace fixed sleep with require.Eventually in reader pool idle test - #8978

Draft
nikolauspschuetz wants to merge 1 commit into
thanos-io:mainfrom
nikolauspschuetz:fix/reader-pool-test-flakiness
Draft

indexheader: replace fixed sleep with require.Eventually in reader pool idle test#8978
nikolauspschuetz wants to merge 1 commit into
thanos-io:mainfrom
nikolauspschuetz:fix/reader-pool-test-flakiness

Conversation

@nikolauspschuetz

Copy link
Copy Markdown
  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

TestReaderPool_ShouldCloseIdleLazyReaders waited for the pool's background
reaper to unload an idle reader with a fixed time.Sleep(idleTimeout * 2) (2s)
and then asserted unloadCount == 1.

The reaper goroutine ticks every idleTimeout/10 and unloads a reader once it
has been idle for idleTimeout, so the unload is an asynchronous event. This
replaces the blind sleep with require.Eventually (testify is already imported
in this file), polling unloadCount until it reaches 1 with a generous 5×
idleTimeout cap and a short tick. The surrounding loadCount/isTracking
assertions are unchanged — they are stable at the moment the unload is observed.

Net effect: both a speedup (resolves in ~1s instead of a fixed 2s of dead
time) and a de-flake (a loaded CI runner where the reaper tick slips past
the fixed 2s window no longer fails a hard timing assertion). Test-only, no
production code touched.

Verification

Ran the target test with the race detector, repeated to shake out flakiness:

go test -race -run TestReaderPool_ShouldCloseIdleLazyReaders -count=50 ./pkg/block/indexheader/
ok  github.com/thanos-io/thanos/pkg/block/indexheader  113.217s

50/50 passes, no data races. Timing comparison (-count=10, no race, same box):
old fixed-sleep 58.4s vs. poll 54.9s — the per-iteration sleep saving shows
through even though block setup dominates each iteration. gofmt clean.


for the record: authored with AI assistance, reviewed and verified by me.

…ol idle test

TestReaderPool_ShouldCloseIdleLazyReaders slept a fixed idleTimeout*2 (2s)
before asserting the pool's background reaper had unloaded the idle reader.
The reaper runs every idleTimeout/10 and unloads once a reader has been idle
for idleTimeout, so on a loaded runner the fixed sleep can race the reaper and
is dead time otherwise. Poll for the unload with require.Eventually instead:
deterministic and faster.

Signed-off-by: Nikolaus Schuetz <nikolauspschuetz@gmail.com>
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.54%. Comparing base (b453e37) to head (a327d92).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8978   +/-   ##
=======================================
  Coverage   64.54%   64.54%           
=======================================
  Files         289      289           
  Lines       37359    37359           
=======================================
  Hits        24115    24115           
  Misses      11151    11151           
  Partials     2093     2093           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant