VLLMClient sets no request timeout, so a stalled vLLM server hangs training indefinitely

[behroozazarkhalili]

Summary

VLLMClient issues every HTTP request without a timeout, so a vLLM server that accepts a connection and then stalls hangs the training process indefinitely rather than failing. The retry policy that looks like it covers this cannot fire, because without a timeout a stalled read never raises the exception the retry is watching for.

Where

trl/generation/vllm_client.py. None of the ten call sites passes timeout=, and _get/_post forward **kwargs without supplying a default:

def _get(self, url: str, **kwargs) -> dict:
    response = self.session.get(url, **kwargs)      # :246
    ...
def _post(self, url: str, **kwargs) -> dict:
    response = self.session.post(url, **kwargs)     # :252

The ten sites: :243 /v1/models, :297 get_world_size, :325 render, :419 and :655 /v1/completions, :445 and :572 generate/chat, :756 start_weight_update, :760 finish_weight_update, :826 reset_prefix_cache. check_server at :273 also calls requests.get(url) bare.

requests defaults to timeout=None, which blocks forever.

Why the retry does not save it

The session mounts a retry strategy at :216:

retry_strategy = Retry(
    total=5,
    connect=5,
    read=5,   # retry failures while reading the response after the connection was successfully established
    ...

read=5 retries a read that fails. A read with no timeout does not fail, it blocks, so this branch is unreachable for the stall case. The comment above it says the strategy exists to keep "an otherwise healthy training run" alive through transient failures, which is exactly the case it silently does not cover.

Impact

Any server-side stall after the TCP connection is established. A wedged engine, a GPU fault on the server, a hung weight transfer, or a network path that blackholes established connections all present the same way: the trainer sits in recv with no error, no log line, and no progress. On multi-rank runs the other ranks then hit their NCCL timeout, which is what the symptom is usually reported as. #3433 is one instance of that shape, though its own root cause was a TRL-side worker that no longer exists.

The design question

A blanket timeout is wrong: /v1/completions legitimately runs for minutes on long rollouts, and a fixed ceiling would break real training. The split that looks right is by endpoint class:

class	endpoints	suggested
control plane	/v1/models, get_world_size, reset_prefix_cache, start_weight_update, finish_weight_update, check_server	short timeout, seconds
data plane	/v1/completions and the chat/generate paths	no timeout, or a long one the caller sets

That keeps generation unbounded while making the fast control calls fail loudly. Whether the data-plane timeout should exist at all, and what the control-plane default should be, are calls I would rather have from a maintainer than guess.

Happy to open a PR once the shape is agreed.

Versions

Observed on main at 6630e17a against vLLM 0.22.0.

[behroozazarkhalili]

Opened #6977 for the half of this that needs no design decision.

check_server cannot honour connection_timeout against a stalled server, because its deadline check sits only inside the except RequestException branch and a bare requests.get on a stalled connection never raises. Measured: still blocked 5.0s into a 2.0s budget. The fix passes timeout=retry_interval, reusing the interval the loop already sleeps rather than adding a knob, and comes with two hermetic tests. Without the fix the stalled-server test hangs instead of failing, which is the bug.

The design question in the original report is untouched and still open: whether the other 10 call sites should get control-plane and data-plane defaults, and what those defaults should be. A blanket timeout would break long rollouts on /v1/completions, so I have not guessed at it.