Skip to content

[Bugfix][Spec Decode] Don't pad a resumed decode request past max_model_len - #53812

Closed
yifjiang wants to merge 1 commit into
vllm-project:mainfrom
yifjiang:fix-spec-pad-max-model-len
Closed

yifjiang wants to merge 1 commit into
vllm-project:mainfrom
yifjiang:fix-spec-pad-max-model-len

Conversation

@yifjiang

@yifjiang yifjiang commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Purpose

Fixes an engine-killing assertion when speculative decoding meets the context limit:

RuntimeError: Sampled token IDs exceed the max model length.
              Total number of tokens: 262145 > max_model_len: 262144

Seen twice in one day on a long-context production deployment (max_model_len 262144, MTP with
num_speculative_tokens: 1, prefix caching + CPU KV offload), under two different configurations.

before/after

Root cause

Two paths size a decode step, and they disagree by one token.

The running path reserves room for the token the step will sample —
scheduler.py#L593-L600:

num_new_tokens = min(
    num_new_tokens,
    self.max_model_len - request.num_computed_tokens - self.num_sampled_tokens_per_step,
)

The waiting path, padding a resumed decode request out to spec width for a full cudagraph,
does not — scheduler.py#L967-L974:

num_new_tokens = 1 + self.num_spec_tokens
if (num_new_tokens > request_token_budget
        or num_computed_tokens + num_new_tokens > self.max_model_len):   # permits ==
    break
pad_spec_decode = True

pad_spec_decode schedules real spec slots
(#L1161-L1164), so the step samples up to
1 + K tokens and the assertion in _bookkeeping_sync fires
(gpu_model_runner.py#L3903-L3909).

With C = num_computed_tokens and K = num_spec_tokens:

guard admits :  C + 1 + K <= max_model_len
runner needs :  C + 2 + K <= max_model_len

One apart, so it fires at exactly C == max_model_len - 1 - K and always overruns by one.

Reaching it needs speculative decoding, plus a resumed request holding a single uncomputed token
(preempted and brought back with its prefix recovered — routine with prefix caching or a KV-offload
tier), sitting at that one position.

Fix

Reserve num_sampled_tokens_per_step here too, so both paths agree.

On failure, fall back to an un-padded step rather than break. Unlike the token-budget case, this
condition never clears on a later step — num_computed_tokens cannot advance while the request is
unscheduled — so a bare guard fix would convert the crash into an indefinitely starved request. The
cost is one cudagraph miss on the final step of a request that is about to stop on length.

Test

test_resumed_decode_padded_to_spec_width_respects_max_model_len, parametrized over
num_spec_tokens ∈ {1, 3}. Scheduler-only, no model execution. Places the request at the derived
boundary and asserts both that the step cannot overrun and that it is still scheduled.

Measured before/after (max_model_len 2048):

            C     scheduled  spec   end_idx
before  K=1 2046      2        1     2049  > 2048   FAIL
before  K=3 2044      4        3     2049  > 2048   FAIL
after   K=1 2046      1        0     2048  = 2048   pass
after   K=3 2044      1        0     2046  < 2048   pass

Existing tests/v1/core/test_scheduler.py: 129 passed. Run on v0.26.0, where this hunk is identical
to main; I have not run main's full suite locally.

Separate from #52807 (offloading connector) — different subsystem, different failure.

@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 bug Something isn't working scheduler labels Aug 26, 2026
@yifjiang
yifjiang force-pushed the fix-spec-pad-max-model-len branch from 5b5ffcf to 8082193 Compare August 26, 2026 00:49
…el_len

The waiting path pads a resumed decode request out to 1 + num_spec_tokens input
positions to preserve a full cudagraph, guarded by

    num_computed_tokens + num_new_tokens > self.max_model_len

which permits num_computed_tokens + num_new_tokens == max_model_len. The step
then samples up to 1 + num_spec_tokens tokens, so _bookkeeping_sync's

    assert end_idx <= self.max_model_len

fails by exactly one and takes down the engine:

    RuntimeError: Sampled token IDs exceed the max model length.
                  Total number of tokens: 262145 > max_model_len: 262144

The running-request path already reserves that slot via
`- self.num_sampled_tokens_per_step`, so the two paths disagree by one token.
Writing C for num_computed_tokens and K for num_spec_tokens:

    guard admits :  C + 1 + K <= max_model_len
    runner needs :  C + 2 + K <= max_model_len

so the fault fires at exactly C == max_model_len - 1 - K.

Reserve the sampled token here too. On failure fall back to an un-padded step
rather than `break`: unlike the token-budget case this condition does not clear
on a later step, because num_computed_tokens cannot advance while the request is
unscheduled, so breaking would starve the request instead of letting it emit its
final token and stop on length. The cost is one cudagraph miss on the last step
of a request that is about to finish.

Reaching it needs speculative decoding plus a resumed request holding a single
uncomputed token at that exact position, which prefix caching or a KV-offload
tier makes routine on long-context serving.

Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com>
@aaron-seq

Copy link
Copy Markdown

In case you have not seen it, njhill opened #53962 for the same bug and says there that it replaces this PR and #50342. It has the ready label and CI running. Your diagnosis held up when I traced it, the two paths really do disagree by one token, so this was the right read. Might be worth checking that the un padded fallback in his version does what you wanted before this one gets closed.

@mergify

mergify Bot commented Aug 27, 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, @yifjiang.

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

@yifjiang

yifjiang commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing — superseded by #53962, which merged on 2026-08-27 and carries the same fix.

Confirmed on main: the padding guard now reserves the sampled token before padding, and falls back to an un-padded step rather than breaking out of the waiting loop.

Thanks @njhill for picking it up, and for folding in the create_scheduler(max_model_len=...)ModelConfig fix — that trap cost me a bogus repro before I noticed the scheduler reads model_config.max_model_len, and fixing the helper is better than the runtime workaround I had.

One coverage note in case it is useful later: #53962's regression test asserts the async-scheduling path (negative num_scheduled_tokensnp.repeat). The same padding also trips _bookkeeping_sync's assert end_idx <= self.max_model_len on the sync path, which is the crash this PR was opened for, and that path is currently untested. I have a parametrized test over num_spec_tokens ∈ {1, 3} that places the request at max_model_len - 1 - K and asserts both no-overrun and not-starved; it failed on pre-fix main with 2049 > 2048. Happy to send it as a small test-only follow-up if you'd like it.

@yifjiang yifjiang closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs-rebase scheduler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants