Skip to content

[CI] Harden FilereadWorkerPool thread-count flake - #4226

Open
quantz8a wants to merge 1 commit into
kvcache-ai:mainfrom
quantz8a:ci-flake-fileread-threadcount
Open

quantz8a wants to merge 1 commit into
kvcache-ai:mainfrom
quantz8a:ci-flake-fileread-threadcount

Conversation

@quantz8a

Copy link
Copy Markdown

Summary

  • FilereadWorkerPoolTest.AcceptsTypedTrailingWhitespaceAndCaches flakes on GHA when glog helper threads move /proc/self/task count around the baseline window (seen as baseline+2 expected 4, actual 6, then join wait timeout).
  • Settle thread count after InitGoogleLogging, poll for worker start (not instant EXPECT_EQ), and stretch join wait 1s → 5s.

Test plan

  • CTest fileread_worker_pool_test green on GHA
  • No change to FilereadWorkerPool production code

Settle /proc/self/task before asserting and wait longer for
worker start/join; glog helper threads were racing the baseline.

@he-yufeng he-yufeng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the filer of #4213: this fixes the flake at its mechanism rather than its margin, which is the better answer than the one my issue suggested.

The failure data (two consecutive CTest hits on PR #3806, both at exactly 1000 ms on the first teardown wait) read as "the 1 s join window is too tight", but the actual driver is upstream of the window: the baseline was read while glog helper threads were still spawning, so baseline itself was a moving target. StableProcessThreadCount settles it first, and the instant EXPECT_EQ(…, baseline + 2) becoming a polled WaitForProcessThreadCount(baseline + 2) matches the fact that worker spawn is asynchronous. The stretched 5 s join then covers the slow-teardown tail. This is also the shape I asked for as a follow-up in #4218 (poll a condition instead of stretching a fixed sleep) — good to see it land properly here.

One scope note to keep the two flakes from drifting apart in style: #4218 took the margin route for the EvictObject lease wait. If this helper shape proves out, porting the same poll-until idiom there later would retire the whole family consistently.

Verified by reading the diff against the test's current structure; a single green CTest run can't prove a flake dead, so the honest check is the next week of runs staying quiet on this test.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants