[KVConnector][P2P] Configurable unbound-store timeout and one-RTT rejection of a late fetch by liranschour · Pull Request #53453 · vllm-project/vllm

liranschour · 2026-08-23T09:44:37Z

Purpose

Fixes the two problems reported in #53128 for the p2p secondary offloading tier.

A producer parks a request's stored KV blocks until the consumer's FetchMsg binds them to a
session, and reaps them after 60 s so they stop pinning primary-tier CPU slots.

The 60 s ceiling was not configurable. Deployments whose prefills legitimately run
longer than that had no recourse. This adds an unbound_store_timeout_s tier config key,
defaulting to the existing 60 s, shaped like the tier's other keys (host, port,
backends, num_threads) — i.e. per-tier, documented, and validated:
```
--kv-transfer-config '{"kv_connector":"OffloadingConnector","kv_role":"kv_both",
  "kv_connector_extra_config":{"spec_name":"TieringOffloadingSpec",
    "secondary_tiers":[{"type":"p2p","unbound_store_timeout_s":180}]}}'
```
A fetch arriving after the reap was never rejected — this is the user-visible damage.
_poll_once bound the id to the session and ServerRole parked demand that no
submit_store could ever satisfy, so the consumer burned its full
_LOAD_TIMEOUT_S (30 s) plus an abort round trip before falling back to local prefill.
Reaped ids now go into a time-pruned _reaped_stores map and _poll_once finalizes the
round on the spot, so the peer receives TransferDoneMsg(success=False) on the same tick
and recomputes immediately. No protocol change and no new session API — this reuses the
existing finish_request → _finalize_outbound terminal path.

Where the reap clock starts (first vs. last stored chunk) is deliberately left alone: the
reporter measured only ~6 s of spread when idle and ~13 s under load, which is not what makes
the failure painful.

As a side effect this removes a real leak. The reap path's self._failed_req_ids.add(kid) was
documented as making "a late inbound FetchMsg short-circuit to a clean rejection", but no
code on the fetch path ever read that set, so the claim never held; and because the producer
request had already run on_request_finished (the only discard site), the id was never
removed and the set grew for the lifetime of the process.

Not a duplicate

fix(kv_offload): don't discard parked KV after a fixed 60s #53235 adds an unbound_store_timeout_s knob via an undocumented kwargs.pop and
nothing else. The knob alone leaves the 30 s consumer stall in place, which is the actual
regression; its PR body also misdescribes the change ("keep it until it is safely
bound/consumed" — it parameterises the reap, it does not remove it). This PR carries both
halves of the fix and shapes the knob as documented, validated tier config.
[Bugfix][KVOffload] P2P tier declares REQUEST_LEVEL on the producer leg #52912 is referenced by the issue but addresses a different area.

Test plan

venv/bin/pytest tests/v1/kv_offload/tiering/p2p/test_manager.py -v   # 69 passed
venv/bin/pytest tests/v1/kv_offload/tiering/p2p/ -v                 # 207 passed
pre-commit run --files vllm/v1/kv_offload/tiering/p2p/manager.py \
  tests/v1/kv_offload/tiering/p2p/test_manager.py \
  docs/features/kv_offloading_usage.md                              # all hooks pass

New test test_late_fetch_after_reap_fails_immediately asserts the path, not just the
outcome: the consumer's load job completes with success=False, no AbortFetchMsg ever
crosses the wire (proving it came from TransferDoneMsg, not the timeout-then-abort path),
and elapsed wall clock is far below _LOAD_TIMEOUT_S. Verified that the test fails on the
unpatched manager — the consumer gets no result at all within the bounded poll count.

The existing test_unbound_store_reaped_after_timeout was updated for the _failed_req_ids
→ _reaped_stores move.

No model eval: the change affects only failure-path timing and a config default, never
generated output.

AI assistance

AI assistance (Claude Code) was used to write this change. I reviewed every changed line and
ran the test commands above myself.

Closes #53128

🤖 Generated with Claude Code

…tch in one RTT The P2P secondary tier parks a producer's stored blocks until the consumer's FetchMsg binds them to a session, and reaps them after 60 s so they stop pinning primary-tier CPU slots. Two problems, both reported in vllm-project#53128: 1. The 60 s ceiling is not configurable, so deployments whose prefills legitimately run longer have no recourse. Add an `unbound_store_timeout_s` tier config key (per-tier, like `host`/`port`/`backends`/`num_threads`), defaulting to the existing 60 s. 2. A fetch arriving after the reap was never rejected: `_poll_once` bound the id to the session and ServerRole parked demand no submit_store could ever satisfy, so the consumer burned its full 30 s load timeout plus an abort round trip before falling back to local prefill. Track reaped ids in a time-pruned `_reaped_stores` map and finalize the round on the spot, so the peer gets TransferDoneMsg(success=False) on the same tick. This replaces the reap path's `_failed_req_ids.add(kid)`, which claimed to make "a late inbound FetchMsg short-circuit to a clean rejection" but was read by no fetch-path code, and — since the producer request had already run on_request_finished, the only discard site — grew for the process lifetime. Signed-off-by: Liran Schour <lirans@il.ibm.com>

claude

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 · 2026-08-23T09:45:12Z

Documentation preview: https://vllm--53453.org.readthedocs.build/en/53453/

bnellnm

LGTM!

orozery · 2026-08-31T08:21:58Z

+                    # own load timeout expires. The id is deliberately left
+                    # unbound so any later submit_store keeps taking the
+                    # unbound path.
+                    session.finish_request(kv_request_id)


Claude:
submit_store never consults _reaped_stores, so a producer whose prefill outruns the timeout keeps parking new batches under the reaped id.
Fix: also drain/fail self._unbound_stores.pop(kv_request_id, ()) in the reject branch, and short-circuit submit_store on a reaped id.

orozery · 2026-08-31T08:24:22Z

+            ValueError: If ``unbound_store_timeout_s`` is not positive.
        """
        super().__init__(offloading_spec, primary_kv_view, tier_type)
+        if unbound_store_timeout_s <= 0:


Probably worth converting to float first (with a try).

orozery · 2026-08-31T08:25:37Z

+            ValueError: If ``unbound_store_timeout_s`` is not positive.
        """
        super().__init__(offloading_spec, primary_kv_view, tier_type)
+        if unbound_store_timeout_s <= 0:


Claude: consider also rejecting values below _STORE_TIMEOUT_S.

No need. These are 2 unrelated timeouts.

The reap path records the id in `_reaped_stores` and deliberately leaves it unbound so a late FetchMsg is rejected rather than served. But `submit_store` never consulted that map, so a producer whose prefill outruns `unbound_store_timeout_s` kept parking fresh batches under the reaped id: blocks nobody can fetch, pinning primary-tier CPU slots for another full timeout, once per reap cycle. Consult `_reaped_stores` in `submit_store` and fail the job immediately — the decoder falls back to local recompute either way. The fetch-path reject branch now also drains and fails anything still parked under the reaped id, so a re-parked batch can't outlive its rejection. Signed-off-by: Liran Schour <lirans@il.ibm.com>

Signed-off-by: Liran Schour <lirans@il.ibm.com>

The value is splatted into __init__ verbatim from the user's kv_connector_extra_config JSON (SecondaryTierFactory.create_secondary_tier passes **config), so the float annotation is enforced by nothing. Checking the raw value first meant a quoted "180" -- what deploy tooling that templates the JSON commonly produces -- died on a TypeError from the comparison, even though float() on the next line would have accepted it. Unconvertible values fared no better: the error named no config key. Coerce first and funnel both failure modes into one ValueError that names the key and echoes the offending value. The check is inverted to `not timeout_s > 0` so NaN is rejected as well; it passes `<= 0` and then disables reaping outright, since no deadline comparison against it is True. Also drop the stale claim that this deadline must exceed the per-store one "so the store-timeout path fires first for individual jobs", which implied the two can be pending on the same job and race. submit_store routes a batch straight to the session when its id is already bound and parks it otherwise, and binding pops every parked batch out of _unbound_stores, so a job is only ever under one of the two deadlines. Signed-off-by: Liran Schour <lirans@il.ibm.com>

orozery · 2026-09-14T12:41:40Z

/ci run

github-actions · 2026-09-14T12:41:48Z

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

/ci run starts upstream CI; /amd-ci run starts AMD CI only.
/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 · 2026-09-14T12:41:56Z

✅ Triggered Buildkite CI #88816 for commit ee7945625dad.

…ection of a late fetch (vllm-project#53453) Signed-off-by: Liran Schour <lirans@il.ibm.com>

liranschour requested review from ApostaC and orozery as code owners August 23, 2026 09:44

claude Bot reviewed Aug 23, 2026

View reviewed changes

mergify Bot added the documentation Improvements or additions to documentation label Aug 23, 2026

liranschour mentioned this pull request Aug 23, 2026

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

Closed

bnellnm approved these changes Aug 26, 2026

View reviewed changes

orozery reviewed Aug 31, 2026

View reviewed changes

jperezdealgaba mentioned this pull request Sep 11, 2026

[Security] Retain P2P client round_seq after idle prune #56471

Open

liranschour added 3 commits September 14, 2026 12:47

Merge branch 'main' into fix/53128-p2p-unbound-store-timeout

8c613ec

Signed-off-by: Liran Schour <lirans@il.ibm.com>

liranschour requested a review from orozery September 14, 2026 12:13

orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 14, 2026

orozery approved these changes Sep 15, 2026

View reviewed changes

orozery merged commit 17bc3e1 into vllm-project:main Sep 15, 2026
120 of 121 checks passed

ItsRoy69 pushed a commit to ItsRoy69/vllm that referenced this pull request Sep 15, 2026

[KVConnector][P2P] Configurable unbound-store timeout and one-RTT rej…

1f8512c

…ection of a late fetch (vllm-project#53453) Signed-off-by: Liran Schour <lirans@il.ibm.com>

Navigation Menu

Uh oh!

Uh oh!

[KVConnector][P2P] Configurable unbound-store timeout and one-RTT rejection of a late fetch - #53453

[KVConnector][P2P] Configurable unbound-store timeout and one-RTT rejection of a late fetch#53453
orozery merged 4 commits into
vllm-project:mainfrom
liranschour:fix/53128-p2p-unbound-store-timeout

liranschour commented Aug 23, 2026

Uh oh!

claude Bot left a comment

Uh oh!

mergify Bot commented Aug 23, 2026

Uh oh!

bnellnm left a comment

Uh oh!

orozery Aug 31, 2026

Uh oh!

liranschour Sep 14, 2026

Uh oh!

orozery Aug 31, 2026

Uh oh!

liranschour Sep 14, 2026

Uh oh!

orozery Aug 31, 2026

Uh oh!

liranschour Sep 14, 2026

Uh oh!

orozery commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

liranschour commented Aug 23, 2026

Purpose

Not a duplicate

Test plan

AI assistance

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

mergify Bot commented Aug 23, 2026

Uh oh!

bnellnm left a comment

Choose a reason for hiding this comment

Uh oh!

orozery Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

liranschour Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

orozery Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

liranschour Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

orozery Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

liranschour Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

orozery commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants