fix(kvevent): purge pod prefixes on AllBlocksCleared (#2287) - #2385
Conversation
Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request implements the handleAllBlocksCleared method in the event handler, which was previously a no-op. When an AllBlocksCleared event is received, the handler now retrieves the sync indexer and calls RemovePrefix to purge the pod's prefix entries, preventing routing to pods with cold caches. Additionally, comprehensive unit tests have been added to verify the correct behavior, including error handling and propagation. There are no review comments to address.
There was a problem hiding this comment.
Pull request overview
This PR fixes KV-event handling so that when a vLLM pod clears its entire prefix cache (emitting AllBlocksCleared), AIBrix purges that pod’s entries from the cross-pod prefix index via SyncIndexer.RemovePrefix, preventing the prefix router from continuing to route matching prefixes to a now-cold pod.
Changes:
- Implement
handleAllBlocksClearedto callRemovePrefix(ctx, modelName, loraID, podKey)with temporary indexer errors swallowed and non-temporary errors propagated. - Update unit tests to assert
RemovePrefixis invoked exactly once onAllBlocksCleared, and to cover temporary vs. processing error behavior. - Extend the mock sync indexer used by tests to record
RemovePrefixcalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/kvevent/handler.go | Implements AllBlocksCleared handling by purging the pod’s prefix entries from the sync index. |
| pkg/kvevent/handler_test.go | Adds/updates tests and mocks to verify RemovePrefix behavior for AllBlocksCleared including error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Saw the note on #2287 to wait on #2290, so I have marked this as draft. This PR is only the part-1 event handling: acting on AllBlocksCleared, which vLLM emits today via the existing kv-events path. It does not touch the sleep-mode control plane or the metric reconcile backstop, so it should be independent of the #2290 RFC. Happy to keep it as draft and sequence it after #2290 if you prefer, or land it early as a standalone correctness fix for the no-op handler. Your call on the sequencing. |
CarolWinddd
left a comment
There was a problem hiding this comment.
Code reviewed and no issue found. Implementation logic, error handling, test cases align with existing code conventions.
Description
handleAllBlocksClearedwas a no-op. When a vLLM engine wipes its entire prefix cache (/reset_prefix_cache, an OOM-driven reset, or/sleepwithlevel>=1) it enqueuesAllBlocksCleared, but AIBrix ignored it: the pod staysRunningand/healthstill returns 200, so the pod-unsubscribe path that normally callsRemovePrefixnever fires. The pod's entries linger in the cross-pod prefix index, and because the prefix router prefers the pod with the most matching blocks, it keeps routing prefix traffic to the pod whose cache was just cleared, turning each match into a cold miss.This wires
handleAllBlocksClearedto purge the pod from the index viaRemovePrefix(ctx, modelName, loraID, podKey)(already on theSyncIndexerinterface), mirroringhandleBlockRemoved: temporary errors fetching the indexer are swallowed, real failures propagate.Scope: this is part 1 of #2287 (act on the event). As the issue notes, events alone are not a complete fix, vLLM only flushes queued KV events from inside a scheduler step, so an idle/sleeping engine may never emit this, and ZMQ delivery is lossy. The metric-driven reconcile backstop (purge against scraped
vllm:engine_sleep_state) is left as a follow-up and called out in a code comment.Related issue
Part 1 of #2287.
Tests
TestHandleAllBlocksClearedEventto assertRemovePrefixis called exactly once, scoped to the handler's model/lora/pod.TestHandleAllBlocksClearedTemporaryError(temporary indexer error is swallowed, no purge) andTestHandleAllBlocksClearedProcessingError(RemovePrefix failure propagates).RemovePrefixcalls.Verified locally (the package is behind the
zmqbuild tag):