Skip to content

[Bugfix][KV Connector] Preserve async load reservations - #48433

Closed
GirasoleY wants to merge 3 commits into
vllm-project:mainfrom
GirasoleY:fix/async-kv-reservation
Closed

GirasoleY wants to merge 3 commits into
vllm-project:mainfrom
GirasoleY:fix/async-kv-reservation

Conversation

@GirasoleY

Copy link
Copy Markdown
Contributor

Summary

Async KV loads reserve the blocks they still need to finish, but the scheduler only honored that reservation when admitting another async load. Ordinary local admissions and subsequent running chunks could consume the reserved headroom. The full-sequence admission check also ignored reserved_blocks.

This change:

  • applies in-flight prefill reservations to both waiting and running allocations;
  • excludes the request that owns the reservation when it resumes;
  • includes reserved blocks in the full-sequence fit check; and
  • adds a regression test where a local request would otherwise consume an async load's remaining capacity.

The reservation is only nonzero while async loads or chunked prefills are in flight, so normal scheduling is unchanged.

Why this is not a duplicate

I searched open PRs for async KV reservation, scheduler deadlock, and reserved_blocks.

Validation

Focused lifecycle test file:

.venv/bin/python -m pytest \
  tests/v1/kv_connector/unit/test_remote_prefill_lifecycle.py -q
10 passed, 17 warnings in 6.82s

Repository hooks on the modified files:

pre-commit run --files \
  vllm/v1/core/kv_cache_manager.py \
  vllm/v1/core/sched/scheduler.py \
  tests/v1/kv_connector/unit/test_remote_prefill_lifecycle.py

All hooks passed.

I also ran a local two-GPU DSv4-Pro-shaped stress test with DP=2, EP=2, DeepGEMM MegaMoE, dummy weights, a deliberately small KV pool, one delayed async external-KV load, and heavily imbalanced requests. All 8 requests completed without a collective stall. On the original target branch, the same-config A/B reduced peak logical KV usage from 94.2% to 87.2% and completion time from 28.91s to 20.82s. Current upstream main with this patch completed in 20.78s with 87.8% peak KV usage.

This test validates the reservation-pressure behavior; it does not claim that every production collective hang has the same root cause.

AI assistance

OpenAI Codex was used to investigate the scheduler path, draft the patch and regression test, and run the validation. I reviewed every changed line and am responsible for the change.

Apply in-flight prefill block reservations to local admissions and running allocations, and include them in the full-sequence fit gate. This prevents ordinary work from consuming capacity promised to a parked async KV load.

Co-authored-by: OpenAI Codex <codex@openai.com>

Signed-off-by: Jingyi Yang <girasoleyang@gmail.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 v1 bug Something isn't working kv-connector labels Jul 12, 2026
@GirasoleY
GirasoleY marked this pull request as draft July 12, 2026 21:02
Signed-off-by: Yifan Qiao <yifanqiao@inferact.ai>
@ivanium
ivanium force-pushed the fix/async-kv-reservation branch from e545ec7 to 6480df6 Compare July 12, 2026 23:12
@ivanium
ivanium marked this pull request as ready for review July 12, 2026 23:27

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

LGTM. Thanks for the fix!

@ivanium ivanium added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 12, 2026
@ivanium
ivanium enabled auto-merge (squash) July 12, 2026 23:29
@ivanium
ivanium disabled auto-merge July 12, 2026 23:37
Signed-off-by: Yifan Qiao <yifanqiao@inferact.ai>
@ivanium

ivanium commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Actually I may need to test this further so hold up temporarily

@ivanium ivanium removed the ready ONLY add when PR is ready to merge/full CI is needed label Jul 13, 2026

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

Actually there might be some bugs. I'll investigate further

Comment on lines +413 to +415
if required_blocks > (
self.block_pool.get_num_free_blocks() - reserved_blocks
):

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.

This applies to all requests and seems to be buggy and cause stuck in some of my tests.

@njhill

njhill commented Jul 13, 2026

Copy link
Copy Markdown
Member

Thanks @GirasoleY! Actually the current behavior is intentional - it optimistically allows use of the "reserved" blocks, with the assumption that more will usually be freed by the time they are actually needed (or else we resort to preemption).

We need to enforce the reserved blocks for other async loads to avoid deadlocks.

See this comment in the original PR:

Note: we apply this throttle to async kv loads only, rather than chunked-prefill requests in general, since the latter are naturally throttled by batch capacity and are also already eligible for preemption.

I'm not saying we definitely shouldn't make this change but It would be good to understand the motivation.

@mergify

mergify Bot commented Sep 17, 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, @GirasoleY.

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 Sep 17, 2026
@GirasoleY GirasoleY closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants