Skip to content

[Frontend] Validate JSON body and request_ids in /inference/v1/abort_requests - #63302

Merged
vllm-bot merged 13 commits into
vllm-project:mainfrom
zydo:validate-scale-out-abort-requests
Nov 2, 2026
Merged

vllm-bot merged 13 commits into
vllm-project:mainfrom
zydo:validate-scale-out-abort-requests

Conversation

@zydo

@zydo zydo commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Validate the body of /inference/v1/abort_requests (and /abort_requests) in vllm/entrypoints/scale_out/token_in_token_out/api_router.py with a pydantic model, so malformed payloads return 400 Bad Request instead of raising an unhandled 500 Internal Server Error.

Claims

  • Adds AbortRequestsRequest (request_ids: str | list[str]) to protocol.py; the annotation defines the contract, so non-object JSON bodies and non-str/list[str] request_ids are rejected by the model rather than by hand-written checks.
  • Non-object JSON payloads (such as the array ["req-1"]) now return the standard 400 ErrorResponse instead of crashing with AttributeError: 'list' object has no attribute 'get'.
  • Clients that send no Content-Type header keep working on the legacy /abort_requests path.
  • No test is added: the request_ids contract holds by definition from the model annotation, so an endpoint test would only re-test pydantic (per review feedback).

Validation

  • Reproducer (without fix vs with fix):
    • Without fix: POST /inference/v1/abort_requests with JSON body ["req-1"] raises AttributeError: 'list' object has no attribute 'get' (500 Internal Server Error).
    • With fix: returns 400 Bad Request, while {"request_ids": "req-1"} and {"request_ids": ["req-1", "req-2"]} return 200 OK.
  • Behavior verified locally with a temporary TestClient test asserting 400 for ["req-1"], {}, {"request_ids": 123} and {"request_ids": [1]}, 200 for valid bodies, and 200 for a body sent with no Content-Type. That test was removed before merge per review, since the model enforces the contract by definition.
  • Test commands run:
    .venv/bin/python -m pytest tests/entrypoints/scale_out/token_in_token_out/test_protocol.py -q
    .venv/bin/pre-commit run --files vllm/entrypoints/scale_out/token_in_token_out/api_router.py vllm/entrypoints/scale_out/token_in_token_out/protocol.py
  • Results: 35 passed; ruff-check, ruff-format, mypy, and the remaining pre-commit hooks all passed.
  • Model evaluation: N/A (input validation on an HTTP endpoint; does not alter model outputs or accuracy).

Details

  • Root cause: abort_requests called body = await raw_request.json() and immediately accessed body.get("request_ids"). Because raw_request.json() accepts any top-level JSON value (arrays, numbers, booleans, strings, null), non-dict bodies raised an unhandled AttributeError, and non-string request_ids values were forwarded straight to engine_client(raw_request).abort(request_ids).
  • Tradeoff — manual parse instead of a FastAPI body parameter: the endpoint still parses the JSON itself and calls AbortRequestsRequest.model_validate(body) rather than declaring the model as a body parameter. vLLM requires FastAPI >= 0.133, where strict_content_type defaults to True, so a body parameter would reject clients that send no Content-Type — the legacy /abort_requests breakage this change avoids. Pydantic errors are converted to RequestValidationError, so the response format matches other endpoints. The limitation is that the model does not appear in the OpenAPI schema for this route.
  • Duplicate-PR check: searched open PRs via gh pr list --repo vllm-project/vllm --state open --search "abort_requests token_in_token_out". Open PR [Bugfix][Frontend] Keep a reference to the background abort task in disagg api_router #45249 retains a background task reference for asyncio.create_task in abort_requests and does not validate the request payload or request_ids types.
  • AI assistance: AI assistance was used to identify the unvalidated payload path and prepare this PR; all changes were reviewed and verified locally.

Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

…requests

Signed-off-by: zydo <zydo.git@gmail.com>

@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.

@mergify mergify Bot added the frontend label Oct 23, 2026
zydo added 3 commits October 27, 2026 12:14
Replace the hand-written isinstance checks with an AbortRequestsRequest
model so invalid bodies get the standard 400 ErrorResponse. The body is
still parsed manually so clients without a Content-Type header keep
working on the legacy /abort_requests path.

Signed-off-by: zydo <zydo.git@gmail.com>
zydo added 4 commits November 1, 2026 20:22
The AbortRequestsRequest annotation already rejects non-object bodies and
non-str/list[str] request_ids, so asserting those 400s only re-tests
pydantic. Keep a focused test for the manual body parse, which accepts
clients that send no Content-Type header.

Signed-off-by: zydo <zydo.git@gmail.com>
The AbortRequestsRequest annotation enforces the request_ids contract by
definition, so the endpoint test added nothing beyond re-testing pydantic.

Signed-off-by: zydo <zydo.git@gmail.com>

@DarkLight1337 DarkLight1337 left a 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.

Thanks

@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 2, 2026
@DarkLight1337

Copy link
Copy Markdown
Member

/ci run

@DarkLight1337
DarkLight1337 enabled auto-merge (squash) November 2, 2026 06:36
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ @zydo, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #99091 for commit 5386e28ef343.

@mergify

mergify Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Hi @zydo, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

@DarkLight1337

Copy link
Copy Markdown
Member

Please rebase from main to fix unrelated pre-commit issues

vllm-project#64357 and vllm-project#63440 each added a process_weights_after_loading to
MuseSpark1xForCausalLM, so the second definition silently replaced the
first and broke pre-commit on main (ruff F811, mypy no-redef). Run both
steps in one method: materialize the residual-path constants first, then
attach the monokernel, which snapshots the final weights.

Signed-off-by: zydo <zydo.git@gmail.com>
auto-merge was automatically disabled November 2, 2026 06:50

Head branch was pushed to by a user without write access

@zydo zydo changed the title [Frontend] Validate JSON body and request_ids in /inference/v1/abort_requests [Frontend] Validate JSON body and request_ids in /inference/v1/abort_requests; fix Muse Spark pre-commit Nov 2, 2026
@DarkLight1337 DarkLight1337 changed the title [Frontend] Validate JSON body and request_ids in /inference/v1/abort_requests; fix Muse Spark pre-commit [Frontend] Validate JSON body and request_ids in /inference/v1/abort_requests Nov 2, 2026
@DarkLight1337
DarkLight1337 enabled auto-merge (squash) November 2, 2026 06:52
@DarkLight1337

Copy link
Copy Markdown
Member

/ci run

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #99097 for commit 839f29bc57bc.

zydo added 2 commits November 2, 2026 00:04
get_rope caches RotaryEmbedding instances by shape and dtype, not
device. test_muse_spark_rocm_shared_rope.py runs earlier in the same
pytest process and builds a CPU cache with the exact key the mono tiny
model uses (head_dim 64, max_position 4096, theta 5e5, fp32), so the
tiny model's target layers reused it and the fused QK-norm/RoPE kernel
rejected the CPU tensor in all 26 stock-path tests. Clear the cache
before building the tiny model and after tearing it down.

Signed-off-by: zydo <zydo.git@gmail.com>
auto-merge was automatically disabled November 2, 2026 08:04

Head branch was pushed to by a user without write access

@zydo
zydo requested a review from AndreasKaratzas as a code owner November 2, 2026 08:04
This reverts commit c8128f4 per review: keep this PR scoped to the
abort_requests change.

Signed-off-by: zydo <zydo.git@gmail.com>
@DarkLight1337

Copy link
Copy Markdown
Member

/ci run

@DarkLight1337
DarkLight1337 enabled auto-merge (squash) November 2, 2026 09:11
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #99132 for commit 49c4432fda5e.

@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 @zydo

@hickeyma

hickeyma commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

❌ @hickeyma, Only reviewers with write access can use CI commands before CI is delegated to the PR author.

@DarkLight1337

Copy link
Copy Markdown
Member

Force merging

@vllm-bot
vllm-bot merged commit e92bac3 into vllm-project:main Nov 2, 2026
71 of 74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants