Skip to content

[Bugfix][KV Offload] Do not store sliding-window chunks with null blocks - #63652

Merged
orozery merged 2 commits into
vllm-project:mainfrom
pepmach:fix/offload-swa-partial-chunk
Oct 26, 2026
Merged

orozery merged 2 commits into
vllm-project:mainfrom
pepmach:fix/offload-swa-partial-chunk

Conversation

@pepmach

@pepmach pepmach commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Fixes #63653.

With offloaded chunks of several GPU blocks, OffloadingConnector could store a sliding-window chunk that contained a null block. A later CPU hit then loaded the never-written slot as KV, and the request's output changed with no error.

Claims

  • A sliding-window chunk is stored only when every block that the store mask marks reachable is present, so a CPU hit no longer loads a slot that no store wrote.
  • One-block chunks and full-attention groups behave as before. A one-block chunk with a null block was already skipped, and full-attention block tables hold no null blocks.

Validation

Two new tests:

  • test_sliding_window_chunk_with_null_block_is_not_stored in tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py: scheduler level, sync and async scheduling.
  • test_cpu_offloading_sliding_window_partial_chunk in tests/v1/kv_connector/unit/test_offloading_connector.py: LLM level, CUDA, google/gemma-3-1b-it with blocks_per_chunk=8 and store_threshold=2. Request B's GPU hit ends mid-chunk, which leaves null sliding-window blocks inside chunk 0, and B stores that chunk. The test asserts that this hit length is reached, so the case is exercised, and that a request hitting the chunk on the CPU tier produces the same output as a cold run.

Scheduler-level test without the fix (main 24324ef0), failing at the store check for request B:

$ pytest --tb=short -q "tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py::test_sliding_window_chunk_with_null_block_is_not_stored"
tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py:2299: in test_sliding_window_chunk_with_null_block_is_not_stored
tests/v1/kv_connector/unit/offloading_connector/utils.py:680: in run
E   AssertionError
FAILED ...::test_sliding_window_chunk_with_null_block_is_not_stored[True]
FAILED ...::test_sliding_window_chunk_with_null_block_is_not_stored[False]
2 failed in 1.11s

Line 2299 is runner.run(decoded_tokens=[EOS_TOKEN_ID], expected_stored=((0, 0), (0, 1))): on main, B also stores sliding-window block 1 without block 0. With the fix, 2 passed in 1.29s.

LLM-level test: not run on a GPU, because I don't have one. CI runs it on CUDA. I ran its helper _sliding_window_partial_chunk_test on CPU against a tiny random Gemma-3 (512-token window, GPU block 32, 8 blocks per chunk). The offload copy was done by an out-of-tree spec that runs gpu_worker.py's transfer code with memmove in place of the CUDA kernel (described in the linked issue).

  • On main it fails at assert out_c.outputs[0].token_ids == cold_c.outputs[0].token_ids: C's greedy tokens change, and C loaded 114,688 bytes that no store wrote.
  • With the fix it passes, and C loads nothing.

End to end on CPU with the same spec, comparing C's output against a run without offloading:

Run C's CPU hit Loaded bytes no store wrote C's logprobs vs no offloading
main, 64-token window, 2 blocks per chunk 64 tokens 16,384 off by 0.11 to 0.15 at every step
main, chunk-aligned control 64 tokens 0 identical
main, 512-token window, 8 blocks per chunk 512 tokens 114,688 (7 blocks) off by 0.18 to 0.39; 3 of the top-5 first tokens change
this PR, both shapes none (recomputed) 0 identical

Model evaluation: the fix changes only which chunks get stored, so an affected request recomputes a chunk instead of loading it. An accuracy benchmark wouldn't isolate the bug, because the bug needs a specific hit-then-evict pattern. The runs above construct that pattern and compare against a run without offloading. The LLM-level test does the same in CI against a cold run on google/gemma-3-1b-it.

Suites, run with pytest -q <suite> on CPU (macOS arm64, VLLM_TARGET_DEVICE=cpu) at 24324ef0:

Suite main this PR
tests/v1/kv_connector/unit/offloading_connector/ 628 passed, 2 skipped 630 passed, 2 skipped
tests/v1/kv_connector/unit/test_offloading_connector.py 2 failed, 3 skipped 2 failed, 4 skipped (the same 2 failures)
tests/v1/core 1059 passed, 1 failed, 4 errors the same counts and the same failing tests
tests/v1/kv_offload (without cpu/test_gpu_worker.py) 718 passed, 54 failed, 10 skipped the same counts and the same failing tests

The failures on both sides are environment limits on this Mac:

  • the two tiering tests download a gated model;
  • in tests/v1/core, the hybrid test model needs causal_conv1d_weight_pack, which the CPU build lacks, and one test's block size is unsupported;
  • the kv_offload failures are cpu/test_shared_offload_region.py, which needs /dev/shm.

pre-commit run --files on the changed files passes, including ruff 0.14.0, typos, mypy and the Buildkite test-wiring check.

The branch merges cleanly into main 237d2ffd. On that merge, the scheduler-level test, the LLM-level helper on CPU and the offloading_connector suite (632 passed, 2 skipped) all pass.

Details

Root cause, with lines at 24324ef0:

  • A GPU prefix-cache hit leaves sliding-window blocks before the window null (vllm/v1/core/single_type_kv_cache_manager.py:1004-1037). A sliding-window free also zeroes pending store positions (offloading/scheduler.py:1503-1522).
  • With the default prefix_cache_retention_interval=0, a GPU hit lands on an earlier prompt's replay boundary. That boundary is block-aligned but not necessarily chunk-aligned, so the null run can end mid-chunk.
  • _build_store_jobs selected chunks by their last block only (offloading/scheduler.py:1880-1891, 1917), then dropped null blocks from the store spec (1995).
  • The worker wrote part of the chunk (vllm/v1/kv_offload/cpu/gpu_worker.py:635-654), while complete_store marked all of it ready (vllm/v1/kv_offload/cpu/manager.py:370-387).

The fix adds one check to key selection. After the existing reachability test, a chunk is skipped if any block the store mask marks reachable is null. With no mask, any null block skips it. No GPU block exists to copy for that position, so the chunk can't be completed. A later request that would have hit this chunk computes it instead: it loses a partial CPU hit and avoids loading stale KV. Null blocks that the mask marks unreachable are still allowed. A request whose window excludes them holds null blocks there too, and its load skips those slots.

Test changes:

  • RequestRunner rejected blocks_per_chunk > 1 whenever custom KV cache groups were passed. It now accepts them when every group uses the runner's block size.
  • _wait_for_prefix_cache_reset takes reset_connector, so the LLM-level test can clear the CPU tier before its cold run.

Not a duplicate: as of 2026-10-26, no open or closed issue or PR reports this bug. I ran 20 keyword searches over issues and PRs and checked the 171 open PRs that touch the offloading connector or kv_offload/. The closest are these two:

AI assistance: Claude (Anthropic) found this bug with a randomized checker over the offloading connector. It also wrote the reproductions, the CPU end-to-end harness, both tests and the fix, and ran the suites above. I reviewed every changed line and reran the tests and the CPU end-to-end check. The commit carries a Co-authored-by trailer.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

With blocks_per_chunk > 1, a sliding-window group can have a null block
inside a chunk whose last block is real: a GPU prefix-cache hit leaves the
blocks before the window null, and a sliding-window free zeroes pending
store positions. _build_store_jobs checked only each chunk's last block,
stored the chunk without the null block's slot, and the whole chunk became
a CPU hit. A later request whose window covers that slot loaded KV that was
never written.

Skip a chunk when any block that the store mask counts as reachable is
null. With no mask, every block counts.

Add a scheduler-level test, and an LLM-level test (Gemma-3, 8 blocks per
chunk, store_threshold=2) that checks a request hitting the CPU tier at
such a chunk produces the same output as a cold run.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Stan <273962421+pepmach@users.noreply.github.com>

@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 for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream 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 bug Something isn't working kv-connector labels Oct 26, 2026
@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 26, 2026
@orozery

orozery commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ @pepmach, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #97248 for commit ff0ab90f39ee.

ℹ️ This PR is 37 commits behind upstream main. Merged upstream main into this branch as ff0ab90f39ee before starting CI. Pull the branch before pushing more changes.

@orozery
orozery merged commit 1d3f05a into vllm-project:main Oct 26, 2026
42 checks passed
AndreasKaratzas added a commit to AndreasKaratzas/vllm that referenced this pull request Oct 26, 2026
Conflict in test_offloading_connector.py: keep main's new
test_cpu_offloading_sliding_window_partial_chunk (vllm-project#63652) CUDA-only, as
main has it (not yet run on ROCm), and this branch's CUDA-or-ROCm gate on
test_cpu_offloading_metrics.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <AndreasKaratzas@users.noreply.github.com>
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…cks (vllm-project#63652)

Signed-off-by: Stan <273962421+pepmach@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…cks (vllm-project#63652)

Signed-off-by: Stan <273962421+pepmach@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working kv-connector ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OffloadingConnector loads never-written KV for sliding-window layers when offloaded chunks span several GPU blocks

3 participants