cli/command: fix flaky TestInitializeFromClientHangs - #7134
Conversation
Signed-off-by: Hamir <hirehamir@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix is sound. Raising clientInitTimeout to 500 ms gives ~5× headroom over the measured 40–90 ms scheduler stalls under CPU pressure. The buffered receivedReqCh channel and the new releaseHandlerCh ensure the handler never drops a notification and the server shuts down cleanly. Moving the assert.Check call from the spawned goroutine to the main test goroutine eliminates a prior data-race against t. The separate 10 s backstop waitTimeout correctly decouples test-harness liveness from the client-init deadline being tested.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix is well-structured: raising the client init timeout from 1ms to 500ms gives ample headroom against scheduler stalls under CPU pressure, a separate 10s backstop avoids conflating the test's own waiting with the deadline under test, the buffered channel allows the handler to record arrival non-blockingly, and releaseHandlerCh cleanly unblocks any pending handlers at cleanup. The assertion ordering (wait for init completion first, then check the server was reached) correctly reflects that a successful timeout-driven return implies the client attempted a connection. No issues found.
There was a problem hiding this comment.
Pull request overview
This PR fixes flakiness in TestInitializeFromClientHangs by removing a timing-sensitive synchronization between the test goroutine and the fake server handler, and by using more realistic timeouts under CPU scheduler stalls.
Changes:
- Increase the client init timeout from 1ms to 500ms to avoid premature deadlines under CPU pressure while still validating the init timeout behavior.
- Replace the unbuffered “handler reached” signal (which required a receiver to be ready) with a buffered, non-blocking signal.
- Add a separate 10s backstop timeout for test waits and use
t.Cleanupto reliably unblock the handler and close the server.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
You're welcome! |
- What I did
addresses #6003
- How I did it
The test set the client's init timeout to 1 millisecond, then asserted that a request actually reached the fake server.
Under CPU pressure the client goroutine can stall for tens of milliseconds, so the 1 millisecond deadline expires before the request is even written, and the handler never runs.
I raised that timeout to ~500 milliseconds, which is about ~5x the worst stall I measured (40 - 90 milliseconds).
The assertion isn't weakened: the fake server never responds, so the client returning at all still proves the deadline works.
The test's own waits now get a separate 10s backstop instead of sharing the client's 1s context.
Trade-off: the test now waits out the deadline it's testing, so it takes ~500 milliseconds instead of ~2 milliseconds, and
cli/commandgoes from ~0.07s to ~0.57s.- How to verify it
The flake needs CPU pressure to show up: