[Router][Bugfix] Return 400 for malformed request payloads

[lfsun02]

Summary

This PR returns HTTP 400 responses for invalid client request payloads instead of allowing parsing errors to propagate as HTTP 500 responses.

Specifically, it:

• returns HTTP 400 for malformed or undecodable JSON request bodies;
• returns HTTP 400 when the top-level JSON value is not an object;
• returns HTTP 400 when the transcription form data is invalid;
• adds regression tests for these request-validation cases.

Testing

• pre-commit check and unittests green
• Local HTTP verification:
  • malformed JSON returns 400
  • non-object JSON returns 400
  • invalid transcription form data returns 400

---

• Make sure the code changes pass the pre-commit checks.
• Sign-off your commit by using -s when doing git commit
• Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].

Detailed Checklist (Click to Expand)

Thank you for your contribution to production-stack! Before submitting the pull request, please ensure the PR meets the following criteria. This helps us maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Please try to classify PRs for easy understanding of the type of changes. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

• [Bugfix] for bug fixes.
• [CI/Build] for build or continuous integration improvements.
• [Doc] for documentation fixes and improvements.
• [Feat] for new features in the cluster (e.g., autoscaling, disaggregated prefill, etc.).
• [Router] for changes to the vllm_router (e.g., routing algorithm, router observability, etc.).
• [Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

• Pass all linter checks. Please use pre-commit to format your code. See README.md for installation.
• The code need to be well-documented to ensure future contributors can easily understand the code.
• Please include sufficient tests to ensure the change is stay correct and robust. This includes both unit tests and integration tests.

DCO and Signed-off-by

When contributing changes to this project, you must agree to the DCO. Commits must include a Signed-off-by: header which certifies agreement with the terms of the DCO.

Using -s with git commit will automatically add this header.

What to Expect for the Reviews

We aim to address all PRs in a timely manner. If no one reviews your PR within 5 days, please @-mention one of YuhanLiu11
, Shaoting-Feng or ApostaC.

[gemini-code-assist]

Code Review

This pull request improves request validation by handling JSON and Unicode decoding errors, ensuring the request body is a JSON object, and catching Type/Value errors during transcription form parsing. Unit tests are also added to verify these changes. The review feedback correctly identifies a potential unhandled AttributeError when parsing the stream field if a non-string value is provided, which would result in a 500 error. It is recommended to catch AttributeError and add a corresponding regression test as suggested.

[gemini-code-assist]

If the client sends a non-string value (such as an uploaded file) for the stream field, calling .lower() on it will raise an AttributeError. Since AttributeError is not caught by the current except (TypeError, ValueError) block, this will propagate as an HTTP 500 Internal Server Error.

We should catch AttributeError as well and return a 400 Bad Request response with an appropriate error message.

    except (TypeError, ValueError, AttributeError) as e:
        error_msg = (
            "Invalid request: 'temperature' must be a number."
            if isinstance(e, (TypeError, ValueError))
            else "Invalid request: invalid form data."
        )
        return JSONResponse(
            status_code=400,
            content={"error": error_msg},
        )

[gemini-code-assist]

It would be beneficial to add a regression test to verify that the router correctly rejects invalid types for the stream parameter (which raises an AttributeError during parsing) with an HTTP 400 instead of crashing with an HTTP 500.

@pytest.mark.asyncio
async def test_transcription_rejects_non_numeric_temperature():
    request = SimpleNamespace(
        form=AsyncMock(
            return_value={
                "file": object(),
                "model": "whisper-model",
                "temperature": "not-a-number",
            }
        )
    )

    response = await route_general_transcriptions(
        request, "/v1/audio/transcriptions", BackgroundTasks()
    )

    assert response.status_code == 400
    assert json.loads(response.body)["error"]


@pytest.mark.asyncio
async def test_transcription_rejects_invalid_stream_type():
    request = SimpleNamespace(
        form=AsyncMock(
            return_value={
                "file": object(),
                "model": "whisper-model",
                "stream": object(),  # This will raise AttributeError on .lower()
            }
        )
    )

    response = await route_general_transcriptions(
        request, "/v1/audio/transcriptions", BackgroundTasks()
    )

    assert response.status_code == 400
    assert json.loads(response.body)["error"]

[ruizhang0101]

otherwise LGTM

[ruizhang0101]

Could you return request id as well?

[ruizhang0101]

Not only temperature will have these error here. The request.form might also raise that. Consider using a broader scope message.

[ruizhang0101]

LGTM

[lfsun02]

looks like there are some timeout issues in checks. Could you take a look when you have a chance? Thanks! @ruizhang0101

[ErenAta16]

Direction is right, and the isinstance(request_json, dict) guard is the part I would have most expected to be forgotten. It catches every non-object JSON document, which the downstream code would otherwise index into:

b'[]'    -> list      b'5'     -> int
b'"x"'   -> str       b'null'  -> NoneType
b'{}'    -> dict      (the only one that proceeds)

Threading request_id into the transcription path so the 400 carries X-Request-Id is a good catch too, since a client correlating by that header would otherwise lose exactly the failed requests.

The except clause is one exception short. json.loads does not only raise JSONDecodeError and UnicodeDecodeError. A deeply nested document exhausts the interpreter stack, and RecursionError is not a subclass of either:

depth  1000 : parsed ok        (2 KB payload)
depth 10000 : RecursionError   (20 KB payload)
depth 100000: RecursionError   (200 KB payload)

issubclass(RecursionError, (json.JSONDecodeError, UnicodeDecodeError))  ->  False

So b"[" * 10000 + b"]" * 10000, a 20 KB body that any client can send, still lands as a 500 after this PR rather than the 400 the PR is introducing. That is not a new problem, but it sits squarely inside what this change is trying to fix, and it is cheap to trigger, so I would fold it in rather than leave a second PR's worth of the same bug behind.

Widening to except (json.JSONDecodeError, UnicodeDecodeError, RecursionError) covers it. except ValueError would too, since JSONDecodeError subclasses it, but that also swallows things you probably want to see, so the explicit tuple reads better.

Worth noting the message can stay the same. From the client's point of view a body the parser cannot handle is a malformed body either way, and "must be valid JSON" is accurate for both.

Two smaller things on the tests, since a test file is the substance of the diff here:

The malformed-payload cases would be stronger with a nesting case alongside the bad-syntax ones, and it would fail today, which is the property that keeps it honest.

_json_request builds a SimpleNamespace rather than a real Request. That is fine for these paths, but it means the test cannot catch a regression where the handler starts touching an attribute the namespace does not define. Worth a comment saying so, otherwise the next person extends the handler and gets an AttributeError from the test rather than a meaningful failure.

[ruizhang0101]

LGTM

[lfsun02]

LGTM

looks like the gatekeeper does not do deduplication on workflow runs of the same check, and treats previous cancelled runs as failed even when new runs pass.

https://github.com/vllm-project/production-stack/actions/runs/30885907838/job/92122841550?pr=1017