Skip to content

[Security] Reject unresolved multimodal inputs before execution - #62152

Open
jperezdealgaba wants to merge 2 commits into
vllm-project:mainfrom
jperezdealgaba:fix/reject-unresolved-mm-input
Open

jperezdealgaba wants to merge 2 commits into
vllm-project:mainfrom
jperezdealgaba:fix/reject-unresolved-mm-input

Conversation

@jperezdealgaba

@jperezdealgaba jperezdealgaba commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Reject unresolved multimodal features before worker admission, including when
prefix, encoder, or external encoder caches are warm. Layout-only requests to
/inference/v1/generate return HTTP 400 before submission to the engine.

Claims

  • Validate every feature once in EngineCore.preprocess_add_request, after
    receiver-cache lookup; unresolved items raise MultiModalCacheMissError.
  • Preserve valid receiver-cache hits and existing EC metadata inputs.
  • Remove the scheduler guards, repeated cache queries, and zero-token returns.
  • Return HTTP 400 for scale-out features with neither payload nor metadata,
    including streaming requests. Render with mm_payload: "tensors" to include
    the payload.

Validation

  • VLLM_TARGET_DEVICE=cpu PYTHONPATH=. HF_HUB_OFFLINE=1 .venv/bin/python -m pytest tests/v1/core/test_scheduler.py tests/entrypoints/scale_out/token_in_token_out/test_generate_stream.py tests/entrypoints/scale_out/token_in_token_out/test_mm_serde.py -q --tb=short: 307 passed.
  • Admission tests cover LLaVA and Qwen2-VL with cold cache, future chunk input,
    encoder hit, EC hit, prefix hit, and receiver hit. The Qwen encoder-hit test
    fails without the new guard because admission does not raise the typed error.
  • HTTP tests exercise the actual FastAPI route with whole-field and per-item
    null payloads, both streaming and non-streaming.
  • Changed-file pre-commit hooks and git diff --check: passed.
  • Repository-wide pre-commit: unrelated macOS failures for Linux-only APIs in
    mypy and /var versus /private/var in the non-root entrypoint wrapper.
  • GPU end-to-end: pending. Added
    tests/entrypoints/scale_out/token_in_token_out/test_serving_multimodal_tokens.py::test_layout_only_features_return_bad_request_and_server_recovers
    to the existing Buildkite entrypoints suite. It verifies layout-only render
    output receives HTTP 400 and subsequent rendering with tensors generates
    successfully on Qwen3-VL. This test has not been run locally.
  • No model evaluation was run: successful-request model outputs and sampling
    are unchanged; the change rejects unresolved inputs.

Details

Encoder outputs and cached prefix tokens do not supply the feature data used
by M-RoPE setup during worker admission. Allowing a null item through on those
cache hits still reaches model assertions. Receiver-cache resolution must run
first, then every unresolved feature must be rejected before scheduling.

The HTTP path now rejects layout-only features before calling the engine.
For direct engine callers, the guard reuses the existing typed cache-miss path.
General generation cache-miss recovery is handled separately in #61656; this
revision does not duplicate its output-processor or endpoint retry changes.
The explicit continuation contract in #60526 is separate from ordinary
multimodal admission. Both related diffs were checked before this revision.

Public /generate callers must now supply payload data or the existing EC
metadata contract for each feature. Hashes and placeholder positions alone are
insufficient. Internal receiver-cache references still resolve normally.

AI assistance was used to develop and review this revision. The human submitter
reviewed the changes and approved pushing with GPU end-to-end validation pending.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill.
  • AI assistance was used during the creation of this PR.
  • Design Fit: Admission validation replaces per-step scheduler checks.
  • Testing and Validation: 307 CPU tests pass; GPU end-to-end validation remains pending.
  • Code Quality and Style: Changed-file checks pass; docs describe the HTTP input contract.
  • Pull Request Contents: Includes root cause, compatibility, related work, and validation limits.

Rebase validation — 2026-10-27

Rebased onto upstream main, preserving the original fix and current surrounding behavior. The duplicate audit found no complete merged or open replacement for this PR’s remaining control.

Preserved upstream scheduler tests. The broad run had 365 passes and four failures; the same four fail on clean upstream because of unavailable model/configuration support in this CPU environment.

Validation on macOS CPU: 365 passed; four unrelated failures also reproduced on clean upstream were excluded from the final run. All applicable changed-file pre-commit checks and git diff --check passed.

VLLM_TARGET_DEVICE=cpu VLLM_PLUGINS='' PYTHONPATH=. HF_HUB_OFFLINE=1 .venv/bin/python -m pytest tests/v1/core/test_scheduler.py tests/entrypoints/scale_out/token_in_token_out/test_generate_stream.py tests/entrypoints/scale_out/token_in_token_out/test_mm_serde.py -k 'not streaming_lookahead_hashes_follow_recomputed_kv and not async_scheduling_pp_allows_rescheduling_with_output_placeholders' -q --tb=short -p no:cacheprovider

Native GPU/RDMA execution and model evaluations were not run during this rebase; required CI and human review remain pending. AI assistance (OpenAI Codex) was used for the conflict resolution and validation.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@jperezdealgaba

Copy link
Copy Markdown
Contributor Author

@WoosukKwon @njhill @robertgshaw2-redhat would you mind reviewing the fix for it?

@hickeyma hickeyma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jperezdealgaba for picking this up. The guard covers the basic case but I don't think it closes the issue yet.

The cache hit paths (prefix cache, encoder cache, EC connector) still let a data=None item reach the worker. On M-RoPE models like Qwen2-VL, the worker reads mm_feature.data in add_request whatever the cache state is. Those paths therefore still take the engine down. Inline comments have the details.

The client also still gets a 500. I'd rather this come back as a 4xx or reuse the retryable multimodal_cache_miss error.

Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread vllm/v1/core/sched/scheduler.py Resolved
Comment thread tests/v1/core/test_scheduler.py Resolved
Comment thread tests/v1/core/test_scheduler.py Resolved
@mergify

mergify Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Documentation preview: https://vllm--62152.org.readthedocs.build/en/62152/

@mergify mergify Bot added documentation Improvements or additions to documentation frontend labels Oct 20, 2026

@hickeyma hickeyma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jperezdealgaba

Comment thread vllm/v1/engine/core.py
request.mm_features
)

if request.mm_features:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why the previous if branch is not sufficient via code comment?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

@mergify

mergify Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jperezdealgaba.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 22, 2026
jperezdealgaba and others added 2 commits October 27, 2026 22:10
Co-authored-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Juan Pérez de Algaba <jperezde@redhat.com>
Co-authored-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Juan Pérez de Algaba <jperezde@redhat.com>
Co-authored-by: OpenAI Codex
@jperezdealgaba
jperezdealgaba force-pushed the fix/reject-unresolved-mm-input branch from ed1f088 to 975b1f9 Compare October 27, 2026 21:19
@mergify mergify Bot removed the needs-rebase label Oct 27, 2026
@mergify

mergify Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jperezdealgaba.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@hickeyma hickeyma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jperezdealgaba. A few things before this can merge:

  • Please answer @DarkLight1337's question on core.py. The receiver cache only exists on the engine for LRU, so a one line comment saying that should do it.
  • test_admission_requires_resolved_multimodal_data can be shortened. The check never reads scheduler or cache state which means one rejected case and one receiver hit cover it. tests/v1/engine/test_engine_core.py looks like a better home also.
  • The mm_payload field descriptions in the chat, completion, responses and anthropic protocol.py files still say a null kwargs_data means every item is cached.

One question on EC prefill. LLaVA renders mm_metadata: null and as far as I can tell the documented prefill flow in renderer.md (drop kwargs_data, send ec_transfer_params) now gets a 400 on models like that. Is that intended? If it is, can you document it? If not, could reject only when uses_mrope is set.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation frontend needs-rebase scheduler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants