fix(vllm): report an exhausted KV pool as a scheduling miss, not a crash - #453
Merged
Merged
Conversation
vLLM's own block pool raises from `get_new_blocks()` when the free count is short, and that raise is unreachable by construction: the count is process-local and authoritative, so an allocation that passed the check cannot fail. kvcached replaces that count with one derived from device-wide free memory, which is shared with every colocated engine. It is a snapshot, not a reservation, and a peer can take the last physical pages before they are claimed. The invariant guard becomes a reachable runtime path. vLLM already knows how to answer "not right now": `allocate_slots()` returns None, the scheduler preempts a running request and retries next step, and the preemption itself releases physical pages back to the shared pool. What it cannot answer is an exception -- `schedule()` installs no handler, and EngineCore's own handler wraps only `execute_model` -- so the exception terminates the engine and every in-flight request with it. Raise a distinct `KVCachePoolExhausted` for this case and translate it to None in `KVCacheManager.allocate_slots`. Nothing else is caught: asking the pool for more blocks than it just reported free is a defect in the caller, and hiding it as a request that is silently never scheduled would be worse than the crash. This completes a path #301 and #430 each covered one window of. Availability is sampled three times -- by the pool, by `alloc()`, and by `alloc_page()` -- and a peer can drain between any two. #301 handled the first gap by evicting and retrying; #430 handled the second by rolling back instead of leaking. Both then converged on the same unhandled exception.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the vLLM integration so a genuinely exhausted shared physical KV pool is surfaced as a scheduler allocation miss (via allocate_slots() -> None) rather than crashing EngineCore, while preserving fail-loud behavior for real contract violations.
Changes:
- Introduces
KVCachePoolExhausted(ValueError)and raises it fromElasticBlockPool.get_new_blocks()when physical allocation fails. - Adds
KVCacheManagerAllocateSlotsPatchto catchKVCachePoolExhaustedinKVCacheManager.allocate_slotsand returnNone(scheduler miss). - Adds regression coverage (new focused vLLM test + prefix-cache test) and wires the new test into the CPU manifest.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
kvcached/utils.py |
Adds a distinct exception type (KVCachePoolExhausted) for physical-pool exhaustion. |
kvcached/integration/vllm/patches.py |
Raises KVCachePoolExhausted from the block pool and translates it to a scheduler miss in allocate_slots(). |
kvcached/integration/vllm/autopatch.py |
Registers the new vLLM patch in the autopatch sequence. |
tests/test_vllm_pool_exhaustion.py |
Adds unit tests asserting exhaustion becomes a scheduling miss and other ValueErrors still raise. |
tests/test_prefix_cache.py |
Extends the pool factory for injection and asserts the pool raises the translated exception type. |
tests/manifests/cpu.txt |
Includes the new vLLM pool exhaustion test in the CPU test manifest. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Availability is checked, then the blocks are claimed, and a colocated engine can drain the shared physical pool in between. #301 and #430 each closed one such window. When the memory genuinely is not there, kvcached currently raises out of
get_new_blocks()and takes EngineCore down with it.vLLM already has a "not right now" path:
allocate_slots()returnsNone, the scheduler preempts a running request and retries next step — which itself releases pages back to the pool. This PR reuses it.Changes
KVCachePoolExhausted(ValueError)replaces the bareValueErrorat the one existing raise site inElasticBlockPool.get_new_blocks().KVCacheManagerAllocateSlotsPatchtranslates it toNoneinKVCacheManager.allocate_slots.Tests
3 CPU tests, one per way this can break:
allocate_slotsreturnsNoneon pool exhaustion — the fix itself.allocate_slotsstill raises on a plainValueErrorfrom the pool — guards against widening theexcepttoValueError, which would turn a caller bug into a request that is silently never scheduled.get_new_blocks()raisesKVCachePoolExhausted— guards against narrowing it back to a bareValueError, which would leave the patch catching nothing and restore the crash without failing any other test.