fix(vllm): make MP retrieve failures recompute safely - #8
Conversation
A retrieve the healthy MP server answers with result=False (missing or evicted keys, block-id underflow, mid-copy exception) was logged and then reported finished, i.e. ACKed to vLLM as a successful load. The request then decodes over never-written GPU blocks and the phantom blocks enter the shared prefix cache, so the corruption propagates to later requests. Record the failed op's block ids in error_block_ids so that get_block_ids_with_load_errors() feeds invalid_block_ids and the scheduler recomputes the span. A retrieve future whose result() raises is contained on the same failure path instead of propagating out of get_finished and killing the engine step. LMCACHE_FAULT_INJECT_RETRIEVE (test-only, default off) deterministically flips healthy retrieve results to failures to exercise the recompute path end to end. Refs LMCache#2898, LMCache#3388
GetRetrieveMetadata sliced the tracker's allocated block ids without checking that they cover [start_token_idx, end_token_idx). slice_block_ids_per_group validates chunk alignment only and its plain Python slice truncates silently, so a tracker primed with LMCache hit counts on a scheduling pass whose allocation never materialised emitted a retrieve op with too few block ids. The MP server fails closed on the short list, wasting a doomed round-trip, and the failure lands on the success-ACK path unless it is surfaced. Check per-engine-group coverage before emitting the op and return None on a shortfall so the request recomputes cleanly. Clamping was rejected: a clamped retrieve completes a load vLLM does not expect and re-creates the desync downstream.
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Problem
The MP connector could acknowledge a failed retrieve as complete without telling the vLLM scheduler that the destination blocks were never populated. It could also emit a retrieve whose allocated block-id list did not cover the requested token range. Both cases allow decode to continue over missing KV data.
Change
falseand a raising future as failed retrievesget_finishedinstead of terminating the engine stepClamping is deliberately avoided: it would acknowledge a shorter load than the scheduler expects. Whole-span invalidation is conservative because the MP result is currently one boolean rather than a per-block result.
The original Florian Bernd commits and authorship are preserved. The original runtime random fault-injection environment hook was removed; deterministic unit tests exercise the same paths without adding production behavior.
Validation
LMCache #7 provides the lower-level RPC exception transport used by real remote handler failures, but this PR remains independently reviewable against the release base.