Wait for the server to accept connections in start_server - #287
Draft
evnchn wants to merge 1 commit into
Draft
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drafted by Claude Code on evnchn's behalf.
Fork staging PR — for review before anything goes upstream.
Motivation
Screen.start_server()returns before the webserver is accepting connections. 12 call sites across 5 test files callscreen.start_server()and then immediately issue an HTTP request, so every one of them races an unsynchronised window.Observed in the merge queue (run 30927252265, py3.13) in
tests/test_sub_pages.py::test_http_404_on_initial_request:The method had no readiness synchronisation at all — it started a thread and returned:
open()tolerates this because it retriesselenium.get()against a deadline. Direct-HTTP callers have no such protection; they just happen to win the race because client-side connection setup usually outlasts uvicorn's bind.Measured window (instrumented locally, 8 runs):
start_server()returned after 0.2–1.5 ms, but the port only accepted connections after 4–87 ms — an unguarded gap of 3.4–86.4 ms on every call, worst on the first (cold) run. That is the window a loaded CI runner loses.Implementation
Wait for the port to accept connections before returning, bounded by a
timeoutparameter:This reuses the existing
helpers.is_port_open()and mirrors the deadline/liveness idiom already inopen(). All 12 call sites are fixed with no changes to the tests themselves.Port readiness is used rather than
uvicorn.Server.starteddeliberately:Server.instanceis a class attribute that survives between tests, so a stale instance from a previous test could reportstarted is Trueand make the wait a silent no-op. TCP readiness is also the exact condition the failing callers need.Both branches of
start_server()are covered — therunpybranch serves on the same port becauseui_run.pyforcesport = NICEGUI_SCREEN_TEST_PORTunder pytest.Verification
Measured window, before and after (same instrument, 8 runs each):
start_server()returnsThe 0.2–0.5 ms residual is the measuring probe's own latency, not a real gap.
Regression test (
tests/test_screen.py, new) — deterministic, and seen to fail without the fix:No regressions: the 5 files containing all 12
start_server()call sites pass, andexamples/pytests(which exercises therunpybranch) passes 10/10.Gates: pre-commit all Passed · mypy clean over 245 source files · pylint 10.00/10.
The 12 exposed call sites
grep -rn -A 2 "screen.start_server()" tests/ | grep -cE "httpx\.(get|post)"→ 12, across:tests/test_sub_pages.py(the one that actually failed in CI)tests/test_status_code.pytests/test_page.pytests/test_timer.pytests/test_event.pyEach does
start_server()followed immediately by an HTTP request. They are all exposed to the same race; only one has been unlucky in the queue so far.Second-lineage review (Codex)
8/10, no MUST-FIX, verdict "fit to put in front of a maintainer". It confirmed the
Server.instance.startedstaleness reasoning, agreed the dead-thread check cannot false-positive (is_alive()is true fromstart()untilrun()exits), and found no caller depending on the old non-blocking behaviour.Its one finding — that the timeout error message should state the bound actually used — is applied above (
within {timeout} s).Progress
start_server()keeps its signature (thetimeoutparameter is optional) and only becomes stricter about when it returns.