Repository navigation
[Bugfix][Duplex] Give the first session open a cold-start timeout budget - #9107
Conversation
…get (vllm-project#9072) The duplex websocket smoke test failed on a freshly started engine: the first open exceeded the 10 s control timeout while the engine was still initializing per-session runner state, the late RPC result was dropped by the result router, and the client saw a spurious 'duplex open timed out'. Open now has its own 60 s budget (_DEFAULT_OPEN_TIMEOUT_S) in both the engine and the frontend handle, while close/resume/touch keep the 10 s control budget that is right for control ops on a live session. Explicit timeouts passed by callers are unchanged. Signed-off-by: kunkunblueberry <1833921874@qq.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Self review:
|
|
This PR appears to belong to: docs/design/module/observability.md, docs/design/module/entrypoints.md. Module owners: @NickCao @alex-jw-brooks @linyueqian Routing: @NickCao via module of the changed files, module named in the PR description, CODEOWNERS; @alex-jw-brooks via module of the changed files, module named in the PR description; @linyueqian via module of the changed files, module named in the PR description @kunkunblueberry, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
linyueqian
left a comment
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed 3b6c4b7. Giving session open its own 60 s budget while live-session control ops keep 10 s fixes the cold-start failure in #9072 at the default, so every caller that does not pass a timeout benefits. Note that #9082 and #9084 also set open budgets (30 s at the serving call site and in the inline client); if they land as written they override this default, so it would help to agree on one value across the three.
Static review only; I did not run the tests locally. Main CUDA passes at 3b6c4b7 (build 17884).
…s set Forwarding handshake_timeout_s unconditionally meant its 30 s default was always passed to DuplexOmni.open_session(), so every InlineDuplexClient overrode the engine's open default -- the 60 s cold-start budget vllm-project#9107 adds (10 s today). A caller that never asked for a budget still shortened the open. The parameter is now `float | None = None`: it reaches open_session() only when the caller sets it, and the base handshake wait keeps its own default. A test asserts the default case sends no timeout at all. Signed-off-by: Jose Alexis Roman Martinez <alexis_romtz@hotmail.com>
…est_error `duplex_omni_engine.py` raises `DuplexSessionError(code="timeout")` when the open or an in-session control RPC runs out of budget. The table had no bare `timeout` entry, so `realtime_error_type()` fell back to `invalid_request_error`: a server-side timeout was reported to the client as a malformed request, and a correct retry policy backs off permanently on a condition that is transient. `config_timeout` is deliberately left as a client error: that one is the caller's own config budget, not the server failing to answer. The explicit 30 s open budget this branch also carried is dropped. vllm-project#9107 gave `open_session` its own 60 s default (`_DEFAULT_OPEN_TIMEOUT_S`), so passing 30 s from `serving.py` would override it for every websocket session; `main` now calls `open_session(session_payload)` and inherits the engine default, per @linyueqian's review. The fix in this PR is the error mapping only. The regressions ride with the fix, not in a follow-up: `test_error_event_maps_codes_to_openai_error_types_and_echoes_client_event_id` pins the table entry and the bucket as it leaves through `ErrorEvent.to_realtime()`, which is the shape a client actually reads. Both assertions fail when the production line is reverted. Focused run, exact assertions added to that test, executed against the real `vllm_omni.protocol.{duplex,realtime}` modules (`vllm` has no macOS wheel, so the pure-Python module unit is exercised directly instead of through pytest): - base `c704a28` (upstream/main): FAIL - `tabla[timeout] = None`, `wire.error.type = 'invalid_request_error'` (2 of 4 assertions). - head: PASS - 4/4. Not run: `pytest tests/engine/duplex/` needs a CUDA host and `vllm` installed. That execution gap remains; this commit does not claim it as coverage. Signed-off-by: Jose Alexis Roman Martinez <alexis_romtz@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…get (vllm-project#9107) Signed-off-by: kunkunblueberry <1833921874@qq.com> (cherry picked from commit e1b99a8) Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Make the MiniCPM-o duplex websocket smoke test stop failing with
duplex open timed out for session ...when it opens the first session on a freshly started engine (#9072).2. What is the highest-leverage variable for that outcome?
The timeout budget applied to the
opencontrol RPC.3. How will this work move that variable?
Open gets its own 60 s budget instead of sharing the 10 s control budget. The first open on a cold engine also initializes the session runner and the first stage reservation, which legitimately takes longer than any later control op; close/resume/touch keep the 10 s budget.
4. Whose customer or user needs does this address, and what are those needs?
Duplex users and CI. A 10 s budget turns normal cold-start latency into a spurious
invalid_request_error/timeoutthat the client cannot distinguish from a real failure, and the late engine result is then dropped.Purpose
Before: the first duplex open on a cold engine could exceed 10 s; the RPC waiter timed out, the engine's eventual open result was dropped by
rpc_result_router, and the client received a retryable timeout error.After: open waits up to 60 s by default in both the engine (
_DEFAULT_OPEN_TIMEOUT_S) and the frontend handle. Control ops on a live session are unchanged, and callers that pass an explicit timeout are unaffected.No user guide/example change: the client API is unchanged.
Test Plan
vLLM Version: 0.31.0
vLLM-Omni Commit:
3b6c4b7c2pytest tests/engine/test_duplex_omni_engine.py -q(CPU)_DEFAULT_OPEN_TIMEOUT_S, close must keep_DEFAULT_CONTROL_TIMEOUT_S.Test Result
Not run: the MiniCPM-o duplex e2e smoke (
test_duplex_websocket_protocol_smoke[three-stage-single-gpu]), which needs a 3-stage server on H100.Related
Fixes #9072