Skip to content

fix(kv_offload): don't discard parked KV after a fixed 60s - #53235

Closed
SayHelloToWorld wants to merge 1 commit into
vllm-project:mainfrom
SayHelloToWorld:fix/issue-53128
Closed

SayHelloToWorld wants to merge 1 commit into
vllm-project:mainfrom
SayHelloToWorld:fix/issue-53128

Conversation

@SayHelloToWorld

Copy link
Copy Markdown
Contributor

Fixes #53128

  • Root cause: _UNBOUND_STORE_TIMEOUT_S = 60.0 counts from submit_store and discards parked KV when no FetchMsg has bound the kv_request_id yet, so a later consumer recomputes the prompt.
  • Fix: stop discarding parked KV on a fixed timer; keep it until it is safely bound/consumed.

…0s _UNBOUND_STORE_TIMEOUT_S ignored deployment queueing\n- Fix: allow configuring via tier option (defaults to 60s)\n- Tests: added minimal constructor test asserting the configured value\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SayHelloToWorld
SayHelloToWorld marked this pull request as ready for review August 21, 2026 08:04

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

@liranschour

Copy link
Copy Markdown
Contributor

Thanks for picking this up. I've opened #53453, which overlaps with this one, so rather than duplicate the work let me explain the difference.

Two notes on this PR:

  1. The description says "stop discarding parked KV on a fixed timer; keep it until it is safely bound/consumed", but the code keeps the timer and only makes it configurable. Worth aligning the two.

  2. The knob on its own leaves the part that actually hurts in place. Once the blocks are reaped, a FetchMsg arriving for that kv_request_id is still bound to the session, and ServerRole parks demand that no submit_store will ever satisfy — so the consumer waits out its full _LOAD_TIMEOUT_S (30 s) plus an abort round trip before falling back to local prefill. Raising the timeout narrows the window, but whenever you do hit it you still pay the 30 s stall.

#53453 carries both halves: unbound_store_timeout_s as documented and validated tier config (matching the tier's existing host/port/backends/num_threads style), plus a time-pruned reaped-id map so _poll_once finalizes the round on the spot — the consumer gets TransferDoneMsg(success=False) on the next tick and recomputes immediately, with no protocol change.

Review there would be welcome.

@SayHelloToWorld

Copy link
Copy Markdown
Contributor Author

Closing: issue #53128 was already addressed by open PR #53453, so this PR is no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug][KVOffload] PD Multi Tier discards parked KV after a fixed 60s, so a queued consumer recomputes

2 participants