Skip to content

fix(sglang): return sidecar request refusals to the client - #15285

Merged
KrishnanPrash merged 1 commit into
mainfrom
kprashanth/dis-2975-sglang-sidecar-public-message
Sep 28, 2026
Merged

KrishnanPrash merged 1 commit into
mainfrom
kprashanth/dis-2975-sglang-sidecar-public-message

Conversation

@KrishnanPrash

@KrishnanPrash KrishnanPrash commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Overview:

The SGLang sidecar refuses some requests before it calls the engine, for example an image request or n=2. The client then gets 400 {"message":"Invalid request"}, and the real reason is only in the worker log. Since #14396, the HTTP body shows DynamoError::public_message() and falls back to "Invalid request" when the error has no public message. The sidecar sets only the private message. This PR gives the fixed request-validation texts of the sidecar gRPC request builder a public message.

Repro (stock SGLang, one SGLang sidecar worker):

curl -s localhost:8000/v1/chat/completions -H 'Content-Type: application/json' -d '{
  "model": "Qwen/Qwen3-VL-2B-Instruct", "max_tokens": 16,
  "messages": [{"role": "user", "content": [
    {"type": "image_url", "image_url": {"url": "data:image/jpeg;base64,<JPEG>"}},
    {"type": "text", "text": "Describe this image."}]}]}'

Before:

{"message":"Invalid request","type":"Bad Request","code":400}

After:

{"message":"multimodal payloads are not supported by SGLang's native Generate RPC","type":"Bad Request","code":400}

The same change applies to n=2 ("n must be 1 for the SGLang sidecar") and to the other request checks.

Details:

lib/sidecar/sglang/src/client.rs adds invalid_request(&'static str). The helper sets the same fixed text as the message and as the public message. The parameter type is &'static str, so client input and runtime values cannot become public through this helper.

lib/sidecar/sglang/src/protocol.rs uses the helper for its fixed request-validation texts. These are the checks in validate_request and the range checks for max_tokens, min_tokens, logprobs, the data-parallel rank, token ids, and stop-token ids. The token-id texts no longer include the client value. Errors that carry endpoints, bootstrap ports and rooms, engine state, or engine text stay private.

The prompt_embeds check now runs before the empty-token check. A prompt_embeds request arrives with empty token_ids, so the old order gave the wrong reason. The thinking-budget text now names the client field thinking_token_budget. This PR does not add or remove a refusal.

The tests check the public text for three refusals. They also check that a disaggregation handoff error has no public message.

Out of scope: the vLLM and TRT-LLM sidecars have the same gap through invalid_argument in lib/sidecar/common/src/error.rs. The refusals on the native HTTP path of the SGLang sidecar (native_http.rs) also keep a private message.

Related Issues

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Request validation errors now provide consistent, clearer messages, including for unsupported prompt embeddings and out-of-range stop tokens.
    • Requests that include both prompt embeddings and empty token IDs now report the unsupported-embeddings error.
    • The thinking token budget error now identifies the alternate name for the setting.
    • Missing decode rendezvous parameters no longer expose a public error message.

Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
@KrishnanPrash
KrishnanPrash requested a review from a team as a code owner September 26, 2026 23:19
@github-actions github-actions Bot added the fix label Sep 26, 2026
@coderabbitai

coderabbitai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 74fd9490-f739-4e1a-8a57-8b1f937b4c5c

📥 Commits

Reviewing files that changed from the base of the PR and between 81a9871 and afec15e.

📒 Files selected for processing (2)
  • lib/sidecar/sglang/src/client.rs
  • lib/sidecar/sglang/src/protocol.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

SGLang request-conversion and validation errors now use a shared InvalidArgument helper. Prompt embeddings are checked before empty token IDs. Tests verify selected public error messages and that a missing decode rendezvous parameter error has no public message.

Changes

SGLang request errors

Layer / File(s) Summary
Invalid request error helper
lib/sidecar/sglang/src/client.rs
Added a crate-visible helper that sets the InvalidArgument error type and uses the provided text for both internal and public messages.
Request conversion and validation errors
lib/sidecar/sglang/src/protocol.rs
Updated conversion and validation failures to use the helper. Prompt embeddings are checked before empty token IDs, the thinking-token-budget error names both parameter forms, and tests check selected public error messages.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to afec1

The SGLang request-error change appears mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: returning SGLang sidecar request-refusal messages to clients.
Description check ✅ Passed The description explains the problem, behavior before and after the change, implementation details, tests, and scope. The Related Issues section identifies DIS-2975, although it does not use the templ…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

@KrishnanPrash
KrishnanPrash merged commit f020aea into main Sep 28, 2026
115 checks passed
@KrishnanPrash
KrishnanPrash deleted the kprashanth/dis-2975-sglang-sidecar-public-message branch September 28, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants