Skip to content

fix(tests): de-flake serve tests by isolating ports and failing fast - #2298

Open
bhimrazy wants to merge 6 commits into
Lightning-AI:mainfrom
bhimrazy:fix/flaky-serve-tests
Open

fix(tests): de-flake serve tests by isolating ports and failing fast#2298
bhimrazy wants to merge 6 commits into
Lightning-AI:mainfrom
bhimrazy:fix/flaky-serve-tests

Conversation

@bhimrazy

@bhimrazy bhimrazy commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes the recurring flake in tests/test_serve.py, most often seen on test_serve_with_generate_strategy[tensor_parallel]:

AssertionError: Server did not respond as expected. 
Error: HTTPConnectionPool(host='127.0.0.1', port=8000):
Max retries exceeded ... Connection refused
image

Each test now serves on its own OS-assigned free port, so servers can no longer collide:

  • Waiting stops as soon as the server process exits, and reports its exit code plus a tail of the server log instead of a bare connection error — e.g. Server exited with code 2 before it was ready. right away, rather than a connection refused after 30s.
  • Teardown always runs through a context manager and waits for the whole process tree to be gone before the next test starts.
  • Server output goes to a file rather than an unread pipe, which can fill up and block a chatty server (NCCL_DEBUG=INFO on CI) before it binds.
  • The boilerplate duplicated across all seven tests collapses into two helpers.
Root cause

All seven tests bound the hardcoded port 8000, and teardown never waited for the server to actually exit — kill_process_tree only sends the signals, and the thread wrapping the non-blocking Popen was joined instead of the process. Three compounding problems:

  • Port still held. A server that has not finished shutting down makes the next one exit at bind_socket() via uvicorn's silent sys.exit(1), which the test can only observe as a connection refused.
  • Leaked servers. When an assertion fails, the kill_process_tree call below it is never reached, so the server survives for the rest of the session and keeps port 8000. tensor_parallel runs last, so it inherits the debris.
  • No diagnostics. Nothing checked whether the server was still alive, so a crash at second one looked exactly like a slow start.

The serve tests all bound the hardcoded port 8000 and killed the server without
waiting for the process tree to exit, so a server that was still shutting down
made the next one exit at bind time with 'Address already in use'. The test then
polled a dead server for 30s and reported a bare connection error with no log.
A failing test leaked its server entirely, because the assertion fired before
the line that killed it.

- serve on a free port per test
- stop polling as soon as the server process exits, and report its exit code
  and log tail instead of a generic connection error
- always kill the server, and wait for its process tree to exit
- log to a file rather than to a pipe that nothing reads during startup
- allow 120s for startup, which is cheap now that a dead server fails fast
- drop the thread wrapper around the non-blocking Popen and share the
  checkpoint and server setup between the tests
Keep reporting the actual exception text (e.g. connection refused) when the
server never answers, instead of a generic status placeholder, matching what
the old _wait_and_check_response reported.
@bhimrazy bhimrazy changed the title fix(tests): stop flaky serve tests from sharing port 8000 fix(tests): de-flake serve tests by isolating ports and failing fast Aug 14, 2026
- probe the free port on all interfaces, matching the server's 0.0.0.0 bind,
  so a port taken elsewhere is not reported as free
- report the status code when the server answers with something other than 200
- explain the teardown wait by what it actually protects now that ports are
  unique: GPU memory and lingering workers
- trim the comments down to the non-obvious mechanics
- drop text=True, which does nothing when stdout is a file and no pipe is read
@bhimrazy
bhimrazy marked this pull request as ready for review August 21, 2026 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant