Skip to content

fix(sampling): reject oversized top_k, logprobs, and n before they kill the server - #42779

Closed
cipheraxat wants to merge 41 commits into
sgl-project:mainfrom
cipheraxat:fix/sampling-upper-bounds
Closed

cipheraxat wants to merge 41 commits into
sgl-project:mainfrom
cipheraxat:fix/sampling-upper-bounds

Conversation

@cipheraxat

@cipheraxat cipheraxat commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

SamplingParams.verify range-checks almost every sampling field, but it never checks an upper bound on top_k, and it does not check n at all. top_logprobs_num is not a SamplingParams field, so it skips verify entirely. The values then reach torch inside the scheduler, the event loop dies, and the server stops answering /health.

A /v1/completions request with max_tokens: 4 does this:

  • "top_k": 2147483648 overflows the int32 top_ks tensor (value cannot be converted to type int32 without overflow)
  • "logprobs": 2147483648 is passed to torch.topk past the vocab row (selected index k out of range)
  • "n": 2147483648 copies the prompt inside normalize_batch_and_arguments, which runs before verify, so the request times out and the server stops answering

The sibling fields already 400. min_new_tokens, a negative top_k, and a non-finite frequency_penalty all come back from verify without killing the process.

Tracked in #41482.

Reproduced with:

python -m sglang.launch_server --model-path Qwen/Qwen3-0.6B --port 30000
python repro_sglang_sampling_upper_bound.py --base-url http://127.0.0.1:30000 --variant top_k
# one variant per fresh server: top_k, logprobs, n

Modifications

Upper bounds in sampling_params.py, io_struct.py, and tokenizer_manager.py:

  • top_k must be -1 (whole vocabulary, stored as TOP_K_ALL = 1 << 30) or in [1, vocab_size]. TOP_K_ALL still fits in the int32 top_ks tensor. Both Rust verify copies use the same check, and a negative top_k is rejected before the u64 cast.
  • n for normal sampling must be in [1, 128]. The check runs in _handle_parallel_sampling before any list replication, and again in verify. An explicit JSON null still means one sample, same as leaving n out.
  • Beam search does not copy the prompt, so when beam_width > 1 the 128 cap is skipped and fan-out stays 1. n must still be an integer >= 1: 0 would return no sequences, and a negative n would slice from the end. n above beam_width is still rejected later by the beam coordinator.
  • top_logprobs_num must be an integer in [0, vocab_size], checked when a generate request is tokenized. k == vocab_size is allowed, so a reranker asking for 50 still passes. The Rust frontend checks the same bound in scheduler validation, which is the first place that knows the vocab size.

Deliberately unchanged:

  • top_k=-1 still disables the limit
  • OpenAI's chat cap of 20 on top_logprobs (the bound is the vocab size)
  • Rust parallel sampling, which already rejects n != 1
  • The beam coordinator's existing n <= beam_width check

Tests

python3 -m pytest -q \
  test/registered/unit/sampling/test_sampling_params.py \
  test/registered/unit/managers/test_io_struct.py

Covers top_k at vocab_size and at TOP_K_ALL, top_k past the vocab and past int32, n at 1 and 128, n of 0 and above 128, beam n above 128 that does not copy the prompt, beam n of 0 and -1, beam n above beam_width that still passes verify, a JSON null n that stays one prompt, and a tokenized generate request whose top_logprobs_num is past the vocab.

Rust: verify accepts top_k == vocab and rejects top_k == vocab + 1 in both copies. top_logprobs_num_is_bounded_by_vocab calls validate() directly: 1001 against a vocab of 1000 returns the validation error, and k == vocab_size is accepted.

This environment has no Triton and no cargo, so the pytest files were not collected here and the Rust tests were not executed. The SamplingParams bounds were checked by importing that module directly.

Accuracy Tests

N/A: request validation only. Values that were already legal stay legal. n above 128 now returns 400. A beam request does not copy the prompt, and a beam n below 1 returns 400. n above beam_width is still the coordinator's error.

Speed Tests and Profiling

N/A. The new checks are a few integer comparisons on the request path.

Checklist

  • Format your code according to the Format code with pre-commit. Applied the ruff and rustfmt wraps from the lint run.
  • Add unit tests according to the Run and add unit tests.
  • Update documentation: sampling_params.mdx now states the top_k, top_logprobs_num, and n ranges.
  • Provide accuracy and speed benchmark results: N/A (see above).
  • Follow the SGLang code style guidance.

CI States

Latest PR Test (Base): ❌ Missing run-ci label -- add it to run CI tests.
Latest PR Test (Extra): ❌ Blocked -- run-ci is required first.
Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.

…ll the server

A huge top_k overflows the int32 tensor, a huge logprobs width blows up torch.topk, and a huge n copies the prompt before verify runs.
cipheraxat and others added 3 commits October 6, 2026 21:48
The crash fix only needs the fan-out cap. Beam search already does not copy the prompt, so it skips that cap instead of growing a second n check.
Beam search skips the fan-out cap, but n is later used as a slice. Zero or a negative n must 400 before it can drop sequences.
@cipheraxat

Copy link
Copy Markdown
Contributor Author

/tag-and-rerun-ci

cipheraxat and others added 3 commits October 6, 2026 22:26
…res (sgl-project#42663)

Co-authored-by: Kan Wu <kan.wu@radixark.ai>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…memory (sgl-project#42754)

Co-authored-by: merrymercy <merrymercy@users.noreply.github.com>
Co-authored-by: xiezhq-hermann <xiezhq-hermann@users.noreply.github.com>
Comment thread python/sglang/srt/managers/io_struct.py Outdated
n = self._coerce_parallel_n(self.sampling_params.get("n", 1))
else: # isinstance(self.sampling_params, list):
self.parallel_sample_num = self.sampling_params[0].get("n", 1)
n = self._coerce_parallel_n(self.sampling_params[0].get("n", 1))

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.

why validate only the first request?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Every request in the batch is checked now, with its own beam_width. A beam item at the front no longer lets a later one skip the cap.


# Build return object
if isinstance(obj, GenerateReqInput):
check_top_logprobs_num(obj.top_logprobs_num, self.model_config.vocab_size)

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.

doesn't belong here, should be adjacent to token_ids_logprob validation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved it next to the token_ids_logprob check.

Comment on lines +51 to +73
def check_n(n: Any) -> None:
"""Reject a parallel-sample count that would replicate the prompt."""
if type(n) is not int or not 1 <= n <= MAX_N:
raise ValueError(f"n must be an integer in [1, {MAX_N}], got {n}.")


def check_beam_n(n: Any) -> None:
"""Reject a non-positive beam n.

Beam search does not replicate the prompt, so n is not capped at MAX_N.
It is stored as num_return and used as a slice, so 0 drops every sequence
and a negative n drops from the end.
"""
if type(n) is not int or n < 1:
raise ValueError(f"n must be an integer >= 1, got {n}.")


def check_top_logprobs_num(value: Any, vocab_size: int) -> None:
"""Reject a top-logprobs width that torch.topk cannot serve for this vocab."""
if type(value) is not int or not 0 <= value <= vocab_size:
raise ValueError(
f"top_logprobs_num must be an integer in [0, vocab_size({vocab_size})], "
f"got {value}."

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.

there are 5 validators for this field in this pr (5 python, 2 rust), do we need so many?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was too many. There is one check_n now. It runs before the prompt is copied, and again in verify. Rust already rejects n != 1, so I left that as is.

Comment thread python/sglang/srt/managers/io_struct.py Outdated
return
elif isinstance(self.sampling_params, dict):
self.parallel_sample_num = self.sampling_params.get("n", 1)
n = self._coerce_parallel_n(self.sampling_params.get("n", 1))

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.

please extract sampling_params.get("n", 1) once instead of nesting it in every call

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. n is read once per request, then the null case is handled

hnyls2002 and others added 10 commits October 6, 2026 13:11
…egaMoE MMA type (sgl-project#42022)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…DA image (sgl-project#42612)

Co-authored-by: Mohammad Angkad <mohammad.angkad@radixark.ai>
)

Co-authored-by: merrymercy <merrymercy@users.noreply.github.com>
Co-authored-by: hanming-lu <hanming-lu@users.noreply.github.com>
…gl-project#42651)

Co-authored-by: merrymercy <merrymercy@users.noreply.github.com>
Co-authored-by: 842974287 <842974287@users.noreply.github.com>
Co-authored-by: merrymercy <merrymercy@users.noreply.github.com>
Co-authored-by: charlotte12l <charlotte12l@users.noreply.github.com>
…ct#41765)

Co-authored-by: Po-Han Huang (NVIDIA) <53919306+nvpohanh@users.noreply.github.com>
…oject#42625)

Co-authored-by: Cheng Wan <54331508+ch-wan@users.noreply.github.com>
…roject#39659)

Signed-off-by: jain-ria <riajain@NVIDIA.com>
Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amd blackwell SM100/SM120 deepseek dependencies Pull requests that update a dependency file diffusion SGLang Diffusion documentation Improvements or additions to documentation hicache Hierarchical Caching for SGLang jit-kernel memory-pool npu quant LLM Quantization rust-renderer unified-radix-cache

Projects

None yet

Development

Successfully merging this pull request may close these issues.