Skip to content

[Bugfix][KV Offload] Reset a resumed streaming session's block table in OffloadingConnector and SimpleCPUOffloadConnector - #60810

Merged
orozery merged 16 commits into
vllm-project:mainfrom
albertoperdomo2:fix/offload-streaming-session-block-ids
Oct 25, 2026
Merged

orozery merged 16 commits into
vllm-project:mainfrom
albertoperdomo2:fix/offload-streaming-session-block-ids

Conversation

@albertoperdomo2

@albertoperdomo2 albertoperdomo2 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Overview

OffloadingConnector and SimpleCPUOffloadConnector corrupts the CPU KV cache for streaming-input sessions. A session resumed for its next input chunk arrives as NewRequestData with its full block table, but yield_req_data flagged it as a delta, so the connector appended it to the table it already held.

Claims

  • A resumed streaming session's new chunks are offloaded from their own GPU blocks, so requests reusing that prefix from the CPU tier get correct KV.
  • yield_req_data reports is_full_table (True for new requests) instead of preempted; the connector resets on it. NIXL and SimpleCPUOffload ignore the flag, so their behavior is unchanged.

Validation

  • test_streaming_session_stores_new_chunks (sync and async): the session's second chunk stores blocks (0, 1, 2) on main (earlier chunks' blocks) and (2, 3, 4) with this PR.
  • pytest tests/v1/kv_connector/unit/offloading_connector/ tests/v1/simple_kv_offload/: all pass.
  • E2E ran, and after a three-chunk streaming session, a greedy request on its 192-token prefix is served from the CPU tier and compared with a full recompute.
    • main: the outputs differ (the CPU-tier output loops on 5, 276, 183, 9).
    • this PR: all 32 tokens match.

Details

_build_store_jobs pairs chunk j's key with block_ids[j], so the duplicated table ([1, 2, 3, 1, 2, 3, 4, 5, 6] vs [1, 2, 3, 4, 5, 6]) stored earlier chunks' KV under later chunks' keys. For every other request the table is already empty when the reset runs, so it is a no-op for them.

SimpleCPUOffloadConnector duplicates the table the same way. It doesn't corrupt KV (it keys by block hash), but it skips offloading the session's new blocks. That will be a follow-up.

AI assistance (codex) was used; I reviewed every changed line and ran the validation above.


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 (tests).

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

Signed-off-by: Alberto Perdomo <aperdomo@redhat.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.

@mergify mergify Bot added bug Something isn't working kv-connector labels Oct 9, 2026
@albertoperdomo2 albertoperdomo2 changed the title [Bugfix][KV Offload] Reset a resumed streaming session's block table in OffloadingConnector [Bugfix][KV Offload] Reset a resumed streaming session's block table in OffloadingConnector and SimpleCPUOffloadConnector Oct 14, 2026
@albertoperdomo2

Copy link
Copy Markdown
Contributor Author

cc: @orozery @ApostaC

@albertoperdomo2

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

❌ @albertoperdomo2, A reviewer with write access must run /ci run, approve the PR, or add the ready label first.

@orozery orozery left a comment

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.

LGTM!
Thanks for the fix @albertoperdomo2 !
@ivanium PTAL for the simple cpu changes.

@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 21, 2026
@github-actions

Copy link
Copy Markdown

✅ @albertoperdomo2, 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.

@ivanium ivanium left a comment

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.

Overall good; left a small question.

Comment thread vllm/v1/simple_kv_offload/manager.py Outdated
if new_block_id_groups[g] is not None:
if is_full_table:
state.block_ids = tuple([] for _ in range(num_groups))
state.num_stored_blocks = [0] * num_groups

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.

Small Q: Should we preserve num_stored_blocks here and only replace block_ids? Or maybe I missed sth

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.

nice catch, yes, you are right: streaming continuation preserves the prefix, so num_stored_blocks remains valid (new requests initialize it to zero and preemption resets it above). I’ll remove this reset and only replace block_ids 👍🏽
cc: @orozery

Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
@orozery

orozery commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #97104 for commit fe973a776985.

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

@orozery
orozery merged commit 921ed7f into vllm-project:main Oct 25, 2026
105 checks passed
@albertoperdomo2
albertoperdomo2 deleted the fix/offload-streaming-session-block-ids branch October 25, 2026 08:03
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…in OffloadingConnector and SimpleCPUOffloadConnector (vllm-project#60810)

Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.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
…in OffloadingConnector and SimpleCPUOffloadConnector (vllm-project#60810)

Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
bantianyue pushed a commit to bantianyue/vllm that referenced this pull request Oct 28, 2026
…in OffloadingConnector and SimpleCPUOffloadConnector (vllm-project#60810)

Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
sohom-cs added a commit to sohom-cs/vllm that referenced this pull request Oct 29, 2026
…e flag

Chunked local attention blocks hold token positions, like full and
sliding-window attention, so select them by position too: the block that
straddles a chunk boundary was not copied again. A UniformTypeKVCacheSpecs
group already takes this path, because generate_scheduler_kv_cache_config
gives the scheduler one of its layer specs.

Since vllm-project#60810, yield_req_data reports a new request, and so a preempted
request that comes back as new, with is_full_table=True. The extra reset
on preempted_req_ids is redundant: drop it and name the flag after what it
means. Skip the last-chunk SSM copy for "align" SSM groups, whose block
table is not append-only; they keep main's per-step copy. Reword two stale
comments.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.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.

5 participants