Repository navigation
[Bugfix] Reject mounted routes whose handler is never wired - #8169
apex-mochen wants to merge 4 commits into
Conversation
|
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, semantic router, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, semantic router, CODEOWNERS @apex-mochen, 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. |
|
Author self-review, as requested. What the diff changes (3 files, +75/-1)
Edge cases checked
Cross-module impact
Review-type checks before submitting
Not run The GPU server-startup / perf job that produced the original log. Local evidence is CPU-only: 66 passed in the guards file (64 before), 5 passed in the policy file, |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot routing recordAssigned Strict under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
In serving modes that never construct a tokenization handler (pure diffusion and duplex), upstream build_app() still mounts POST /tokenize and POST /detokenize, and those handlers used to crash with AttributeError and HTTP 500. After omni_init_app_state(), the worker now reuses shutdown_unsupported_routes() to replace both routes with the existing 400 BadRequestError envelope when app.state.serving_tokenization is missing or None. Multi-stage servers that do wire ServingTokenization are left unchanged, so clients keep using the upstream handlers.
Change flow
flowchart TD
A["[EXISTING] Client POST /tokenize or /detokenize"]:::existing
B["[CHANGED] omni_run_server_worker after omni_init_app_state"]:::changed
C["[NEW] _shutdown_tokenization_routes_if_unwired"]:::new
D["[CHANGED] OmniServingCapability TOKENIZE and DETOKENIZE"]:::changed
E["[EXISTING] shutdown_unsupported_routes 400 BadRequestError"]:::existing
F["[EXISTING] Wired ServingTokenization handler"]:::existing
A --> B
B --> C
C -->|"handler is None"| D
D --> E
C -->|"handler wired"| F
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
48868d4012d6(2026-09-26T20:27:31.765076+00:00): required check(s) blocking:buildkite/vllm-omni(missing), and -5 more.
No actionable findings.
|
Author self-review of the updated commit What the diff changes. Patch Coverage Review — is every added line exercised?
Two branches deliberately have no new test:
Edge Case Review — normal / None / absent / boundary / exception
Cross-Provider / Backend Impact Review
|
061967a to
bb3aae8
Compare
bb3aae8 to
ca033de
Compare
|
Rebased onto The conflict was confined to one file and was purely mechanical: both sides added a single import line to
New head: |
ca033de to
8634dc0
Compare
|
Rebased onto The only conflict was the import block in No source changes: |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
This PR stops pure-diffusion and duplex servers from returning HTTP 500 on upstream routes that stay mounted while their app.state handlers are never wired (/tokenize, /detokenize, completions, responses retrieve/cancel, messages). After normal app-state init and any model-declared endpoint_restrictions, it derives additional restrictions from missing/None handlers, applies them only when the route is actually mounted, and reuses the existing shutdown_unsupported_routes() 400 envelope. Wired deployments and unmounted surfaces stay unchanged; a pipeline-declared reason still wins over the generic unwired reason.
Change flow
flowchart TD
A["[EXISTING] omni_run_server_worker<br/>after omni_init_app_state"]:::existing
B["[EXISTING] shutdown_unsupported_routes<br/>model endpoint_restrictions"]:::existing
C["[NEW] _shutdown_routes_without_handlers"]:::new
D["[CHANGED] endpoint_policy<br/>UNWIRED_HANDLER_ROUTES + unwired_endpoint_restrictions"]:::changed
E["[NEW] route_is_mounted"]:::new
F["[EXISTING] shutdown_unsupported_routes<br/>replace mounted unwired handlers"]:::existing
G["[CHANGED] unwired probes: AttributeError/500 → 400 BadRequestError"]:::changed
A --> B --> C
C --> D
C --> E
D --> F
E --> F
F --> G
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
8634dc0bb8de(2026-09-30T22:40:55.249371+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
No actionable findings.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
|
Hi @hsliuustc0106 — polite follow-up on this one. You applied the What it does: If this isn't the right area for you, could you point me to whoever owns omni's frontend wiring? The four sibling wiring PRs (#8209, #8212, #8242, #8244) are in the same state — happy to consolidate them if that's easier to review. Thanks! |
| "The Responses API is unavailable because this deployment does not initialize a responses handler.", | ||
| ), | ||
| ( | ||
| OmniServingCapability.MESSAGES, |
There was a problem hiding this comment.
[P2] Cover the sibling Messages and tokenizer-info routes
The upstream vLLM 0.30.0 routers also mount POST /v1/messages/count_tokens and, with --enable-tokenizer-info-endpoint, GET /tokenizer_info. Both depend on the same handlers checked here, but neither is included in this table. On pure-diffusion or duplex servers, a valid count-tokens request still reaches the missing/None anthropic_serving_messages or the fallback serving_tokenization.create_error_response(), and tokenizer-info directly calls serving_tokenization.get_tokenizer_info() on None; these paths still produce HTTP 500. Please add both route targets to the restrictions and cover them using the real upstream routers. The new tests register synthetic routes, so they cannot catch omissions from the mounted upstream surface.
There was a problem hiding this comment.
Thanks @hsliuustc0106. On 10-09 I said I'd handle this in a separate PR. Since it uses the same table, I've added it to this PR in 026e0e06 instead.
POST /v1/messages/count_tokensnow goes through the guard keyed onanthropic_serving_messages. When the Messages handler is missing, upstream builds its error response fromserving_tokenization, which pure diffusion leaves unset and duplex sets toNone. That is why the request crashed.GET /tokenizer_infonow goes through the guard keyed onserving_tokenization.
Tests use upstream's real routers. The new tests mount vllm.entrypoints.anthropic.api_router and vllm.entrypoints.serve.tokenize.api_router (with --enable-tokenizer-info-endpoint) the same way build_app does. They cover /v1/messages, /v1/messages/count_tokens, /tokenize, /detokenize and /tokenizer_info, each with the handler absent (pure diffusion) and set to None (duplex).
- Each case first checks that the unguarded upstream route raises
AttributeErroron the missing handler. - It then checks that the guarded route returns 400.
- A separate test checks that every guarded path and method is actually mounted by those upstream routers, so an upstream rename would fail the test.
Verification (local, CPU):
test_api_server_guards.py: 86 passed on the branch, and 93 passed on a merge with currentmain(910b8f07).- Negative control: with only
endpoint_policy.pyreverted, exactly the 5 new count-tokens/tokenizer-info cases fail (81 passed). - ruff 0.14.10 check and format are clean.
One more route I found but did not change here. I sent a request to every upstream route build_app mounts on pure-diffusion and duplex state. POST /invocations has the same problem: it dispatches by request body, so a completions-shaped body reaches the missing openai_serving_completion and returns 500. Pure diffusion does wire a chat handler, so a whole-route 400 like the others would also reject requests that work. It needs a per-request check, which I'd rather propose as a separate change. Happy to do it here if you prefer.
8634dc0 to
9212539
Compare
Re-applies the vllm-project#8169 tokenize route wiring fixes on top of f9b97d2. Signed-off-by: apex-mochen <2756823972@qq.com>
Omni ReviewBot: supersededThe CI failure noted on |
|
Hi @hsliuustc0106, thanks for the review! Re: P2 sibling Messages/tokenizer-info routes — good catch. This PR currently guards the routes that were already wired with handlers; extending the guard to |
ruff check (pre-commit) flags the unused import and rewrites the line; the all-files pre-commit run therefore fails at head 9212539. Remove the unused symbol so the isort/ruff check passes unchanged. Signed-off-by: apex-mochen <2756823972@qq.com>
|
Hi maintainers — heads-up on the current The run ( Evidence this is not a code problem:
Looks like a CI-infra issue (e.g. hook-env cache restore failure). Could you re-run the check? Happy to dig further if the log shows anything else. |
Omni ReviewBot: supersededThe CI failure noted on |
The pre-commit job fails on this head: ruff (pinned 0.14.10 in .pre-commit-config.yaml) reports F401 for realtime_connection in the api-server guards test, and the hook runs with --fix, so CI rejects the head as soon as the linter rewrites it. The module is unused there: the guards test reaches realtime behaviour through api_server.* and the /v1/realtime route strings. Signed-off-by: apex-mochen <2756823972@qq.com>
|
Correction to my note above: this was not a CI-infra issue, and I was wrong to ask for a re-run. The real cause is in this PR's own diff. Running the ruff version this repo pins ( That unused import came in with the rebase commit Fix is pushed in Sorry for the noise. I should have reproduced the pinned linter locally before suggesting the failure was infrastructure. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; no fallback: the fallb). |
1 similar comment
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; no fallback: the fallb). |
Upstream also mounts POST /v1/messages/count_tokens and, with --enable-tokenizer-info-endpoint, GET /tokenizer_info. Count-tokens falls back to serving_tokenization for its error response when the Messages handler is missing, and tokenizer-info calls serving_tokenization directly, so both still answered HTTP 500 on pure-diffusion and duplex servers. Add both to the unwired-handler table and cover the Messages and tokenize routes with upstream's own routers instead of synthetic ones. Signed-off-by: apex-mochen <2756823972@qq.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Omni ReviewBot: no human activity for 7 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-15. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot: no human activity for 14 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-15. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
Closing this stale bugfix PR to keep the queue focused on core feature work. It is not a technical rejection: the branch and review history remain available, and it can be reopened if maintainers want it after the core work lands. |
Fixes #8139.
Problem
omni_run_server_worker()assembles the app with upstreambuild_app(args, supported_tasks). Forsupported_tasks=("generate",)that mounts the whole generate family —/v1/completions,/v1/responses(plus retrieve and cancel),/v1/messages— on top of the always-mounted/tokenizeand/detokenizeroutes.A serving mode only initializes the handlers it can serve. Pure diffusion and duplex deliberately leave
app.state.openai_serving_completion,openai_serving_responses,anthropic_serving_messagesandserving_tokenizationunset orNone(_DIFFUSION_MUST_BE_NONEfreezes this for diffusion; duplex nulls its handlers in a loop), and no route-level null check covers them:/v1/completions,/v1/responses(+ retrieve/cancel),/v1/messages,/tokenize,/detokenize→AttributeError→ HTTP 500./v1/messagesis doubly affected: it null-checksanthropic_serving_messages, but then builds its error throughserving_tokenization.create_error_response, which diffusion sets toNone— so even its existing check cannot produce the intended error.#8139 reported this for
/tokenizefrom a Cosmos3 diffusion startup log. The same root cause reaches three more route families, so the fix here is stated once for the class rather than four times per route.Reproduction (CPU, no GPU needed)
Assemble the real app the way
omni_run_server_worker()does — upstreambuild_app(args, ("generate",))plus the realomni_init_app_state()for a pure-diffusion engine — then probe it withTestClient:POST /v1/completionsAttributeErrorBadRequestErrorPOST /v1/responsesAttributeErrorBadRequestErrorGET /v1/responses/{id}AttributeErrorBadRequestErrorPOST /v1/responses/{id}/cancelAttributeErrorBadRequestErrorPOST /v1/messagesAttributeErrorBadRequestErrorPOST /tokenizeAttributeErrorBadRequestErrorPOST /detokenizeAttributeErrorBadRequestErrorGET /v1/models,/load,/version(controls)Fix
Reuse the mechanism this repo already has for exactly this job —
shutdown_unsupported_routes(), added by #4762 for/v1/completionsand/v1/images/edits— and derive its input fromapp.stateinstead of hard-coding routes:config/endpoint_policy.py— register the newly reachable capabilities (RESPONSES,RESPONSES_RETRIEVE,RESPONSES_CANCEL,MESSAGES) next to the existingCOMPLETIONS, and replace the two-branch special case with oneUNWIRED_HANDLER_ROUTEStable mapping each capability to theapp.stateattribute that serves it, plusunwired_endpoint_restrictions(state, already_restricted=...).entrypoints/serve/utils/routes.py—route_is_mounted(app, path, methods), so a restriction is only applied to a route that is actually mounted.entrypoints/openai/api_server.py—_shutdown_routes_without_handlers(app, already_restricted=...)replaces the previous tokenize-only helper. One call site, unchanged position, right afteromni_init_app_state()and the existingshutdown_unsupported_routes(app, engine_client.endpoint_restrictions)call.Three invariants the helper keeps, each with a test:
26 -> 26).route_is_mounted()is checked first, so this never grows the surface — a path the app did not mount still answers 404 rather than gaining a new 400.engine_client.endpoint_restrictionsare skipped, so a model that declares a restriction keeps the reason that pipeline declared.Why not set the state attribute to
None, or install stub handlers?Setting the attribute to
Nonegives 501 for completions/responses (upstream null checks) but still 500 for/v1/messages, which needs aServingTokenizationobject to build its error. Installing stub handlers contradicts_DIFFUSION_MUST_BE_NONEintests/entrypoints/openai_api/test_api_server_guards.py, a contract this repo deliberately froze. Route rejection is the only one of the three that fixes the whole class, and it reuses the path the repo already chose for the other unavailable endpoints.Why 400 here rather than 404
Omni's own routes answer 404 for an unwired handler ("The model does not support …"). These seven routes are different: they are upstream routes, and the repo already has a mechanism for "this route is mounted but not available to this deployment" —
shutdown_unsupported_routes(), which answers 400. The same route can already be made unavailable that way today: a pipeline that declares it inendpoint_restrictions(e.g.qwen3_omni/pipeline.py) makes/v1/completionsanswer 400 on that deployment. Routing the unwired case through the same function keeps one answer per route regardless of why it is unavailable, and keeps this change to deriving the function's input rather than changing what it emits. If you would rather these answer 404 to match omni's own routes, that is a one-line change inbuild_rejection_handler()and I will happily make it.Scope: why the table lists exactly these seven routes
The table is only worth having if it is complete. I enumerated the real route surface an omni server builds — upstream
register_api_routers()plus the per-task factories (generate,speech_to_text,pooling) plus omni's own router — and checked every route that is not in the table against theapp.stateattribute it reads:/v1/chat/completions,/v1/chat/completions/batch,/v1/audio/speech,/v1/audio/speech/batch,/v1/audio/generate/cohere/v2/chat,/cohere/v2/chat/rendergetattr(..., None)and answers 501; additionally gated behindVLLM_ENABLE_COHERE_API/v1/load_lora_adapter,/v1/unload_lora_adapteropenai_serving_models, which every omni mode wires; additionally gated behindVLLM_ALLOW_RUNTIME_LORA_UPDATING/v1/embeddings,/v1/score,/v1/rerank,/v1/pooling/*POOLING_TASKStest on both sides/v1/audio/transcriptions,/v1/audio/translationstranscription; no omni pipeline declares that task/generative_scoringNotImplementedError, documented as a 404 in current vLLM main). Omni's defect is different in kind: it never initializesstate.serving_generative_scoringin any mode, so the attribute read raises before that path is reached. That needs the attribute wired, not the route rejected, so it is deliberately not in this table.Test Plan
CPU-only, no GPU needed.
Test Result
tests/entrypoints/openai_api/test_api_server_guards.py: 74 passed (64 on upstream main; the first revision of this PR added 2 of them, this one replaces those two with 10)tests/config/test_endpoint_policy.py+tests/entrypoints/openai_api/test_duplex_api_server.py+test_video_pipeline_capability.py: 134 passedruff checkandruff format --checkon all four changed files: cleanvllm_omni/; line endings unchanged (0 CR bytes in all four files, same as upstream)Regression check (A/B with the patch physically removed). The 15 test files that import the modules this PR touches were run twice, once with the patch and once after replacing the three source files and the test file with the
ad025deforiginals (the replacement was asserted on md5 before the second run):The two FAILED sets are identical — no test passes only before this change and none fails only after it. The delta of 10 is exactly the new tests. The 9 failures are environment-only and reproduce at the base too:
ModuleNotFoundError: No module named 'cache_dit'via the Cosmos3 model registry (test_config_factory.py,test_image_server.py,test_video_server.py), i.e. an optional dependency missing from this CPU box, not a behaviour change.Not run: the GPU server-startup / perf job that produced the original log in #8139 — no GPU available here. The path is exercised end-to-end against the real upstream routers and
TestClient, but the Cosmos3 diffusion run should be repeated in CI to confirm the startup log is clean there.vLLM Version: 0.30.0 (the version in the report)
vLLM-Omni Commit: ad025de
AI assistance: used Claude Code to diagnose the failure and draft the patch and the regression tests. I reviewed every change, ran the checks reported above myself, and can explain the design and how it interacts with the existing endpoint-policy mechanism.
Negative control
A/B on the 15 test files that exercise the touched paths, with the three source
files restored to their pre-patch contents by file-level replacement (not
git stash, which would be a silent no-op here) and then re-patched: both sidesfail exactly the same 9 tests — all of them the pre-existing
cache_ditenvironment failures — and the FAILED sets are identical, i.e. no regression and
no new failure. The head adds 10 new assertions in
tests/entrypoints/openai_api/test_api_server_guards.py(64 → 74 passed).Alternatives considered
The narrow first revision — reject only
/tokenizeand/detokenize, which iswhat #8139 reported — was replaced rather than shipped: assembling the real app
with
build_app(..., ("generate",))plus the realomni_init_app_stateshowsseven routes answering 500 on a pure-diffusion server, so the fix is driven by the
state each route actually needs. Per-route hard-coding was rejected because it
would need editing every time upstream adds a route.
Compatibility
No route is added, removed, or renamed, and no schema changes: only routes that
already answered 500 start answering the loader's existing "unsupported" envelope
(the same JSON fields and the reason the pipeline itself declared). Wired routes
are untouched — an invariant with its own test — and a route that was never
mounted does not appear.
AI assistance
AI assistance (Claude Code) was used to draft the route table, the regression
tests, and this description. I reviewed every change, ran every command reported
here locally, and can explain each invariant above without the tool.