Repository navigation
Bound the check_server health probe so connection_timeout can fire - #6977
behroozazarkhalili wants to merge 9 commits into
Conversation
check_server polls /health with a bare requests.get, and its deadline test lives only inside the except RequestException branch. A server that accepts the connection and then stalls never raises, so the loop never reaches that test and blocks forever. The connection_timeout parameter documents that a ConnectionError is raised once the budget is spent, and against a stalled server that promise could not be kept. Passing timeout=retry_interval bounds each probe without adding a knob: the loop already sleeps that long between attempts, so a probe outliving one interval is late by the method's own measure. A refused connection behaves as before, since it raised straight away already. Adds two hermetic regression tests. One points check_server at a socket that accepts and never answers, and requires it to give up inside the budget. The other is the control: a healthy server must still be accepted. Without the fix the first test hangs rather than failing, which is the bug. Refs #6973
|
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
left a comment
Jokasa7
left a comment
There was a problem hiding this comment.
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.
| response = requests.get(url) | ||
| # The probe must be bounded: a server that accepts the connection and then stalls would otherwise | ||
| # block here forever, and `total_timeout` is only checked in the except branch below. | ||
| response = requests.get(url, timeout=retry_interval) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.0bound was too loose to notice. The bound is now derived from the class constants (RETRY_INTERVAL / 2withTOTAL_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 insideexcept. - Deadline arithmetic moved from
time.time()totime.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 satisfieselapsed >= total_timeoutand loops forever, andretry_interval <= 0raisesValueErrorrather thanConnectionError. Both reproduce.AGENTS.mdasks 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
requeststimeout 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.
…eady server The elapsed-time check sat inside `except RequestException`, so it ran only when the probe raised. A vLLM server that accepts the connection and answers HTTP 503 while it loads weights returns normally, so the deadline was never consulted and the loop retried forever. Against a local 503 server, `total_timeout=0.1` was still running after 5 seconds; 404 reproduces it identically, so the defect is any non-200 status rather than 503 specifically. Checking the deadline where the raising and non-raising branches converge bounds both. Two further bounds follow from the same contract: one probe is capped by the remaining budget, because `retry_interval=5.0` against a stalled socket overshot a 0.1s deadline by 4.9s, and the retry sleep is capped the same way. Deadline arithmetic moves to `time.monotonic()` so a system clock adjustment cannot shift it. `total_timeout` defaults to 0.0, meaning one attempt and then give up. That attempt stays bounded by `retry_interval` rather than by the deadline, and the docstring now says so.
…lign the async clients `check_server` slept up to `retry_interval`, then probed again without checking the deadline. When the sleep landed on the deadline the next probe ran for a full `retry_interval` and accepted a server that 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. `retry_interval` must be positive: `requests` rejects a zero timeout with a ValueError from inside the loop, and a negative value would have skipped the sleep. The docstring states the remaining limit, that `requests` applies the timeout to the connect and to each read separately. The async GRPO and async distillation clients keep their own `wait_for_server_ready`, which 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 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. The two copies stay identical apart from the config name. Tests: the health-probe cases gain a retry case (503 then 200, two requests seen), a late-200 case that must be refused, and a rejected non-positive interval; the one-shot mutant that passed the previous cases now fails three. A new experimental test file exercises both async clients against a stalled socket and a 503 server and bounds the wait by the deadline.
|
Have you actually hit this in a real training run ? |
|
does vllm, in some setting accepts the connection and then never replies? |
|
No. #6973 came from reading |
|
Closing this one myself. It came from reading the code, not from a failure in a real training run, and per the discussion on #6862 that isn't the bar for this repo. If someone hits this in practice, the branch is still here and I'm happy to reopen it with a reproduction. |
3 participants
Server readiness checks could hang on a stalled connection or start another probe after the deadline. I bounded health probes, added deadline checks and retry interval validation, and aligned both async clients' probes and sleeps with the remaining budget. Tests against stalled, unavailable, and healthy local servers pass, including cases that fail or hang without the fix.
Refs #6973.
Note
Medium Risk
Changes only health-check polling in vLLM clients, but fixes behavior trainers rely on at startup; mis-tuned timeouts could fail connects that previously hung indefinitely.
Overview
Fixes vLLM client startup waits that could block past
connection_timeout/server_timeoutwhen/healthaccepts TCP but never responds, returns 503 while loading, or when retry/poll sleeps overshot the deadline.VLLMClient.check_servernow caps each/healthrequest withtimeout=min(retry_interval, remaining budget), treats non-200 (including 503) as “not up” when enforcingtotal_timeout, clamps inter-retry sleep to the remaining budget, re-checks the deadline before the next probe, usestime.monotonic()for deadlines, and rejects non-positiveretry_interval.The same deadline semantics are applied to
wait_for_server_readyon the async GRPO and async distillationVLLMClientcopies (bounded probes and sleeps, monotonic clock).Tests add hermetic
TestCheckServerHealthProbecases (stalled socket, persistent 503, healthy/retry paths, no probe after deadline, invalid interval) and parametrized async-client readiness tests—no live vLLM or GPU required.Reviewed by Cursor Bugbot for commit 53dcd3a. Bugbot is set up for automated code reviews on this repo. Configure here.