Bound the check_server health probe so connection_timeout can fire

[behroozazarkhalili]

The bug

check_server cannot honour its own documented contract. It polls /health in a while True: loop, and the check that enforces total_timeout sits only inside the except requests.exceptions.RequestException branch:

while True:
    try:
        response = requests.get(url)          # no timeout
    except requests.exceptions.RequestException as exc:
        elapsed_time = time.time() - start_time
        if elapsed_time >= total_timeout:
            raise ConnectionError(...)

A server that refuses the connection raises immediately, so the deadline works. A server that accepts the connection and then stalls never raises at all, so the loop never reaches its own deadline test and blocks forever. VLLMClient's connection_timeout documents "If the server is not up after the timeout, a ConnectionError is raised", and against a stalled server that promise could not be kept. This is the failure mode the parameter exists for.

Measured on a socket that accepts and never answers: the bare call was still blocked 5.0s into a 2.0s budget, while the same call with timeout=1.0 raised ReadTimeout in 1.0s.

The fix

One argument: timeout=retry_interval. That bounds each probe so the except branch can fire and enforce total_timeout, and it adds no new API surface, since the loop already sleeps retry_interval between attempts. A probe that outlives one full interval is late by the method's own measure. A refused connection is unaffected: it raised straight away before and still does.

Two follow-ups from review. The loop slept up to retry_interval and then probed again without looking at the clock, so a sleep that landed on the deadline was followed by a full-length probe that could accept a server which came up after the budget (measured: a 0.05 s budget accepted a 200 at 0.251 s). The wait now checks the deadline once more after sleeping. And retry_interval must be positive: requests rejects a zero timeout with a ValueError from inside the loop, and a negative value skipped the sleep.

The async GRPO and async distillation clients keep their own wait_for_server_ready. It probed with a fixed poll_interval_s timeout, slept the full interval, and checked the deadline only afterwards, so it could overshoot server_timeout by up to one poll interval plus one probe. Both copies now use the monotonic clock, bound every probe and every sleep by what is left of the deadline, and stop before a probe that would start past it. They stay identical apart from the config name.

Tests

Six hermetic cases in TestCheckServerHealthProbe, needing neither a GPU nor a running vLLM server, so they run in the normal lane rather than the slow one:

• test_stalled_server_raises_rather_than_blocking points check_server at a socket that accepts and never answers, and requires it to give up inside the budget.
• test_unavailable_server_raises_rather_than_looping serves 503, the answer vLLM gives while it loads, and requires the same deadline to fire on a non-200 status.
• test_healthy_server_still_accepted is the control: an ordinary 200 must still be accepted.
• test_server_that_becomes_ready_is_accepted_on_retry serves 503 then 200 and requires the second request to be accepted (the server counts two requests).
• test_no_probe_starts_after_the_deadline serves 503, then a 200 that only arrives after the budget, and requires ConnectionError: the one-shot mutant that passed the first three cases fails this one and the retry case.
• test_non_positive_retry_interval_is_rejected requires ValueError for retry_interval=0.0.

tests/experimental/test_async_vllm_client.py runs both async clients against a stalled socket and a 503 server with a poll interval far larger than the deadline, and bounds the wait by the deadline. The previous clients fail all four cases; the aligned ones pass.

Red and green, both arms run:

arm	stalled-server test	healthy-server control
without the fix	hangs, killed at 90s (rc 124)	passes
with the fix	passes	passes

The control passes in both arms, so the pair is not vacuously green. The file collects 43 tests; the eight that run without a GPU pass in CI on the current head. ruff check and ruff format --check at the pinned 0.13.3 and doc-builder at the pinned 0ab9ea03 with --max_len 119 all pass on both files.

Scope

This is the half of #6973 that is a plain defect, and it needs no design decision. The other half, whether the remaining 10 VLLMClient call sites should get control-plane and data-plane timeout defaults, is still an open question on that issue, because a blanket timeout would break long rollouts on /v1/completions. I have deliberately left those sites alone.

One limit stays: requests applies retry_interval to the connect and to each read separately, so a server that keeps sending bytes can hold one attempt past it. The docstring says so.

Refs #6973

---

Note

Low Risk
Scoped to vLLM client startup health polling; improves timeout behavior without changing generation or weight-update HTTP calls.

Overview
VLLMClient.check_server now honors total_timeout when /health hangs or returns a non-ready response (e.g. vLLM 503 while loading), not only when the TCP connection is refused.

Each probe uses a requests.get timeout capped by retry_interval and remaining budget; deadlines use time.monotonic(); failed attempts (exceptions or non-200) share one deadline check; retry sleep is clamped so the loop cannot overshoot the limit. Docstrings and the ConnectionError message were updated accordingly.

Adds fast, hermetic TestCheckServerHealthProbe cases (stalled accept, 503, healthy 200) without a running vLLM server.

^{Reviewed by Cursor Bugbot for commit 89a6a8f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[Jokasa7]

Thanks for adding a bounded probe and a hermetic stalled-connection test. I checked the exact method from this head against the other unhealthy-server outcome that requests can return normally: an HTTP response whose status is not 200. That path still bypasses the only elapsed-time check, so the documented total deadline remains ineffective for a server that is reachable but reports that it is not ready.

Using the exact check_server() body with a deterministic requests.get stub that returned 503, total_timeout=0.10, and retry_interval=0.02, the call was still running after 0.375 seconds. It returned only after the stub was changed to 200. Applying the deadline to every unsuccessful attempt and adding a 503 regression would make this PR cover the full health-probe contract it describes.

[Jokasa7]

[P1] This still never applies total_timeout when the server responds normally with a non-200 status. The elapsed-time check remains inside except RequestException, so a vLLM instance that repeatedly returns 503 loops and sleeps forever even though it is not up. On this exact head, a deterministic 503 response with total_timeout=0.10 and retry_interval=0.02 was still running after 0.375 seconds and returned only after the response changed to 200. Please check the remaining deadline after every unsuccessful probe (and avoid a final request/sleep extending past it), then add a 503 regression alongside the stalled-socket case.

[behroozazarkhalili]

Confirmed, and the scope is wider than 503. Fixed in fe8d46e.

Reproducing your case against a local server on the previous head, total_timeout=0.10 and retry_interval=0.02 was still running after 5.0s. A server answering 404 hangs identically, so the defect is any non-200 status rather than 503 specifically: the else branch fell through to the retry sleep without ever consulting the deadline. The check now sits where the raising and non-raising branches converge, so both are bounded.

Your parenthetical about the final request and sleep extending past the deadline was also correct, and it was the larger error. A single probe was bounded only by retry_interval, so total_timeout=0.1 with retry_interval=5.0 against a stalled socket raised at 5.008s, overshooting by 4.908s. Both the request and the sleep are now clamped to the remaining budget. The clamp is conditional rather than a bare min, because total_timeout defaults to 0.0: an unconditional clamp would pass timeout=0.0 on the default path and break every healthy connection. A non-positive remainder therefore still gets one full attempt, which is what 0.0 has always meant, and the docstring now states that instead of implying a hard zero-second bound.

Two more changes came out of checking the guards rather than the behaviour:

• The tests did not actually cover either clamp. Deleting the sleep clamp or the request clamp left all three passing, because the < 5.0 bound was too loose to notice. The bound is now derived from the class constants (RETRY_INTERVAL / 2 with TOTAL_TIMEOUT = 0.1, RETRY_INTERVAL = 5.0), and three mutants are each killed by the test that targets them: dropping the sleep clamp, dropping the request clamp, and moving the deadline check back inside except.
• Deadline arithmetic moved from time.time() to time.monotonic(), so a system clock adjustment cannot shift the deadline.

Two related problems are real but deliberately not in this PR:

• total_timeout=float("nan") never satisfies elapsed >= total_timeout and loops forever, and retry_interval <= 0 raises ValueError rather than ConnectionError. Both reproduce. AGENTS.md asks contributors not to add defensive code or handle cases that do not exist today, and no caller passes either value, so validating them here would work against that guidance. Happy to add it if a maintainer prefers.
• A requests timeout bounds inter-byte inactivity, not total duration, so a response delivered slowly enough can outlast the deadline regardless of the clamps. Closing that needs a different transport-level mechanism and is a larger change than this bugfix.

Regression coverage: test_unavailable_server_raises_rather_than_looping for the 503 path, alongside the existing stalled-socket and healthy-server cases. Local run is 8 passed, 35 skipped.