[Mooncake] Guards on the partial-hash hit retry - #52273
Draft
xinli-sw wants to merge 2 commits into
Draft
Conversation
…ndaries Replaces the local fix reverted in the previous commit. vllm-project#50359 is upstream's answer to the same defect and states it exactly: "A longer stored key proves that one object exists at that endpoint; it does not imply that Mooncake also contains independently addressable objects at every shorter hash boundary." Our -704 is that, reached through DCP. Core's fine-grained lookup deliberately extends a hit into the first non-full block -- locally a block is usable as a prefix, remotely an object is not -- and the EAGLE path then subtracts exactly one hash unit, landing mid-block by construction. Off DCP the gap is empty because the attention block equals hash_block_size; scaling it by dcp opens a 96-unit interior, and every hit landing there names a key nobody wrote. Measured on B300 c8 DCP=8: 2,757,664 OBJECT_NOT_FOUND, all on the one scaled group. Preferred over the reverted local fix, which disabled partial hash hits under DCP outright. That reached the same boundary in one step but gave up fine-grained hits everywhere, including the no-spec arm that works today at a 29.1% external hit rate. vllm-project#50359 keeps them and steps back only when the exact key is absent, off the already-populated exists set, so no extra store RPC. ONE ADAPTATION. Upstream unpacks self.attention_groups[0] as a 3-tuple; here SpecGroup also carries manager_cls and use_eagle, so that raises ValueError on the first lookup. Attribute access instead, which is arity-independent. git apply reported the patch clean -- this is a runtime mismatch, not a textual one. The added test asserts a stronger property than upstream's: the reported hit must be an exact object boundary for EVERY group, not only attention_groups[0], over dcp in {1,2,4,8}. Upstream validates one group because off DCP the coarse group is Mamba, which is tail-only; that assumption is what DCP inverts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit ada5e31) Co-authored-by: mispa-ms <81828223+mispa-ms@users.noreply.github.com>
Reviewing the carried patch on this branch found one path where it is safe only by accident and one where it crashes. SAFE BY ACCIDENT. find_longest_cache_hit is also called from load_mask, whose pool is built with exists=None -- a sentinel that makes get_cached_block answer "present" to anything. The retry therefore never fires there. Nothing states that. If it did fire, load_mask would return a mask for a shorter length while _handle_request keeps using the original token_len, so the trailing chunks process_tokens yields would index past the end of the mask and be skipped, leaving those blocks uninitialized in the local KV pool. That is silent, unlike the -704 this patch exists to fix. Now explicit: ExternalCachedBlockPool carries tracks_existence, and the retry declines to run against a pool that has no truth to check. CRASH. Upstream asserts isinstance(spec, FullAttentionSpec) on attention_groups[0]. That group is only attention-first by a sort; a Mamba-only layout puts a MambaSpec there, and partial hash hits are enabled by a Mamba group alone, so the assert is reachable and fires at the first lookup. Nothing to revalidate in that case anyway -- the defect needs an attention group coarser than hash granularity, and a recurrent group is tail-only -- so return True. Both guards are ours, not upstream's, and belong in a comment on vllm-project#50359. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7b97d35) Co-authored-by: mispa-ms <81828223+mispa-ms@users.noreply.github.com>
xinli-sw
force-pushed
the
k3-mooncake-partial-hit-retry-guards
branch
from
August 14, 2026 13:57
d241e11 to
a7f22dc
Compare
xinli-sw
changed the title
xinli-sw
force-pushed
the
k3-mooncake-partial-hit-retry-guards
branch
from
August 14, 2026 15:30
a7f22dc to
d241e11
Compare
xinli-sw
changed the title
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
2 participants
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.
Stacks on #52272.