[Bugfix]: prevent crash when sampled tokens exceed max_model_len

[nicole-lihui]

Purpose

closed #25120

• cut off tokens beyond max_model_len so it doesn’t assert.

Test Plan

Test Result

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request addresses a bug that caused a crash when the number of sampled tokens exceeded the maximum model length. The fix correctly replaces an assertion with logic to truncate the sampled tokens, ensuring they fit within max_model_len. The implementation is sound and handles edge cases, such as when a sequence has already reached its maximum length. This change effectively resolves the reported issue.

[vadimkantorov]

Typically, when truncating the output, vllm would set the reason/cause in the output object

But in any case, it was extremely weird that this assert blew up

[nicole-lihui]

I noticed that #20291
changed one line in the scheduler from:

self.max_model_len - request.num_computed_tokens

to:

self.max_model_len - 1 - request.num_computed_tokens

I haven’t directly identified the root cause of the overflow, but I did notice that the scheduler handles num_new_tokens inconsistently between running and waiting requests, which is very likely related.

@WoosukKwon Could you share some suggestions on this?

[nicole-lihui]

@yannicks1 Looks like we’re fixing the same issue — I guessed another possible cause. I’m new to vLLM, so any feedback would be super helpful.

[yannicks1]

hi @nicole-lihui, I found that "[Optimization] Cache sampled token ids in model runner" #20291 actually introduced this bug and addressed it accordingly in that part of the code. IMO that is safer and less intrusive than touching the scheduler as in your PR. I also provide a unit test to detect such an inconsistency in the future.

[tdoublep]

@nicole-lihui Could you perhaps review #24446? To me, the fix looks simpler and the PR also includes a test case.