Repository navigation
Conversation
| def _prepare_fetch( | ||
| self, op: _PendingResidencyOp, key: BlockKey | ||
| ) -> list[_TransferRef] | None: | ||
| if key in op.results: |
There was a problem hiding this comment.
When fetch starts, keys already pending invalidation are recorded as failures in op.results. Without this guard, _prepare_fetch could find their still-resident data and overwrite that failure with a successful cache hit.
| self._resume_capacity_waiters() | ||
| return True | ||
|
|
||
| def _drop(self, key: BlockKey) -> None: |
There was a problem hiding this comment.
Existing G2 removal is tied to capacity eviction or retirement on release. Move needs targeted removal after previously admitted operations finish and no_evict protection is checked, without cancelling earlier work. This provides that final cleanup; its reclamation bookkeeping overlaps the existing paths and could be consolidated.
90e2a68 to
778cdb3
Compare
PeaBrane
left a comment
There was a problem hiding this comment.
Thanks for this. The move/invalidation design looks solid overall: reader admission against pending removal, no_evict and layout handling, lock ordering and shutdown all read well. A few notes inline: a possible lost-copy window at the deadline boundary, a fetch-failure regression under move hints, an API-shape question about fetch() inheriting move from the hint, and one softer, unreproduced generation/self-source concern.
These came from an AI-assisted read of the code at 778cdb3 and were checked against the source; none were reproduced at runtime.
| op.source_keys, | ||
| op.local_sources, | ||
| success=op.success, | ||
| moved_keys=op.moved_keys() if op.success and op.keys_to_move else (), |
There was a problem hiding this comment.
Possible lost copy at the deadline boundary. Removal is queued here when the source sees its write succeed. The destination, though, accepts a local-fill completion only while now < self.deadline (remote_ops.py around L227-231), and the source's deadline is received_at + remaining (remote_fw_dram.py around L769), so it lands roughly one control-message latency after the destination's. A write that completes in that gap succeeds at the source (copy removed) but is discarded at the destination, leaving neither side with a G2 copy. The destination's _expire_pending_ops running before the target op is applied in poll_completed seems to widen the window.
Suggestion: gate source removal on the destination's commit, for example by reusing the removal-only _StartWrite already sent for cached keys in moves.py complete() once the destination's fill commits; or, at minimum, make the source deadline strictly earlier than the destination's. A test that advances the target clock past its deadline between the source's DONE and the target's poll would cover it; the current test pair pins the target clock to 0.0 in test_kvcr_move.py.
There was a problem hiding this comment.
Good catch my earlier code review also caught this one. The update removes source blocks after destination confirmation.
There was a problem hiding this comment.
Updating my earlier reply: after a discussion internally, we switched back to source-owned removal after successful native writes. Source copies remain claimed and pending removal through completion, avoiding the unpin/redeposit gap before a later destination confirmation.
Move destinations now use abandon_timeout_ms for both transfer completion and pending local fills, while the source retains its normal operation_timeout_ms. Each worker requires the abandonment allowance to be at least twice its own operation timeout. write_done indicates that the native writes into the destination buffers completed successfully. The longer destination allowance gives it additional time to process completion and commit the local fill. This reduces the deadline-boundary failure window, although a sufficiently delayed destination can still time out after the source copy has been removed.
| remote = self._core._remote_fw_dram | ||
| hint = remote._request_hints.get(request_id) | ||
| adapter = remote._key_adapter | ||
| if hint is None or not hint.source or hint.failed or adapter is None: |
There was a problem hiding this comment.
Request poisoning under move advice: once the request's hint is marked failed, whether after any failed deliver() (_fail_request_hint) or a conflicting second hint (replace(..., failed=True, move=move)), move stays true. fetch() then enters begin(), gets False here, and the whole fetch completes FAILED, including keys that are local G2 hits. Before this change a failed hint only disabled remote fills.
Would it make sense for hint-inherited moves (as opposed to explicit move()) to fall back to a plain copy/local fill when the hint has failed, keeping the hard failure only for explicit move()?
There was a problem hiding this comment.
Fixed. If the move hint is failed or conflicting, it then behaves like any fetch with a failed copy hint: local cache hits are served, remote keys fail. We also stopped overwriting the stored hint's mode with the mode of the hint that got rejected.
| # Register move intent before fetch can complete on a local hit. | ||
| deadline = self._operation_deadline() | ||
| hint = self._remote_fw_dram._request_hints.get(request_id) | ||
| if move or (hint is not None and hint.move): |
There was a problem hiding this comment.
fetch() becomes a move whenever the request's hint says mode="move", while deliver() stays a copy. For integrations that use both in one request this splits semantics. For example, the SGLang HiCache backend (sgl-project/sglang#45647) delivers ordinary KV pages with deliver() but confirms hybrid trailing-state checkpoints with fetch(), so a move hint there would move the checkpoints while only copying the KV pages.
Consider making move opt-in per call (explicit move(), or a move= argument on fetch()) instead of inheriting it from the hint. That seems cheaper to settle now than after integrations depend on it.
There was a problem hiding this comment.
For this one, in the update we kept the hint mode as the trigger, but narrowed it. Only keys whose hashes are in a move hint get moved; any other key in the same fetch is a normal copy, and deliver always copies.
We didn't add a per-call move() because ultimately we want the router to decide move vs. copy, as expressed in the hint. For HiCache that means delivered pages are copied, and checkpoints fetched with fetch() are moved only if the router actually listed their hashes in a move hint.
| ): | ||
| continue | ||
| keys.append(key) | ||
| self._core._invalidation_handler.submit(keys, key_layout=request.key_layout) |
There was a problem hiding this comment.
Possibly worth a look, but this needs human judgment and I haven't reproduced it. The removal intent here (and on the all-cached path in remote_fw_dram.py around L759-763) acts on whatever G2 copy is current when the message arrives, with no residency generation. If a cached-hit intent were delayed until after the key had been removed and freshly re-deposited, it could remove the new copy.
Separately, I didn't see a check that the hint's source isn't this node's own endpoint. With self-referencing advice, a node could drop its own copy once the fetch handle is released. Dynamo's source selection excludes a worker's own attachment, so this may be unreachable in practice. If you think either is real, a generation check and a self-source check look cheap.
51275d2 to
3dc6ba8
Compare
Let fetch honor copy/move mode in request-scoped kv.fetch hints. Capture usable source advice and matching full keys at admission; move hints always require hash matching. Hint replacement does not change admitted work, failed advice preserves local hits, and deliver remains a copy. Carry move intent in the initial transfer request. The source admits moves on its owner thread, withdraws eligible G2 keys, and keeps its transfer claims through native completion. Successful writes permit removal after earlier operations drain; failed writes reopen only keys the move owns. Complete destination cache hits send an initial one-way invalidation. Reject source keys with incomplete G2 copies or unfinished deposits before closing admission, allowing those deposits to finish normally. Use the existing abandonment budget for destination move completion while retaining the source's normal transfer timeout. Exceptionally delayed destinations can still fail after source removal. No post-copy confirmation is required, and destination completion does not wait for source cleanup. Preserve no_evict protection and incompatible source layouts. Framework and G3 copies are untouched. Existing inventory hooks report actual source removals and newly complete destination copies. Rejecting a key relinquishes the move's removal ownership before later retries or callbacks. Validation: 920 unit tests passed, 2 environment-dependent skips; lint and formatting passed. Regression coverage includes delayed completion, partial/cached blocks, retention, failures, and overlapping invalidations. Signed-off-by: Weiyang Wang <wafrank@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Hi Rudy, thanks for putting this together and for the thorough validation. I wanted to flag a direction change we’ve been discussing, since your work depends on PR #154. We’re planning to make move hints always request-independent, so #154 may change fairly significantly. Our thinking is that a request-scoped hint labeled Before we settle on that, do you see value in retaining request-scoped move hints? I’d really appreciate your perspective on any use cases or benefits we might be overlooking. Thanks! |
|
Thanks for flagging this early. From our side, nearly every move we need is triggered by routing a request to a cold worker, so we'd lean toward keeping request-scoped move as the primary shape:
A request-independent Things we learned building the SGLang side (ziqifan617/sglang#9) that KVCR could absorb to make request-scoped move simpler for every framework:
Happy to share our test results or the native repro if useful. Totally open to other views; this is from one integration's perspective. |
|
Thanks so much, this is helpful. I see the benefit of reusing the request’s transfer and completion path. I wanted to clarify what we each mean by “polling.” Looking at current main and #154, When you say the caller never has to poll, do you mean no additional framework-side move-completion polling on the source, and no waiting for source cleanup, because the framework already handles fetch/delivery completion? On the race point, my understanding is that the delayed-invalidation/reverse-delivery issue can occur with the current request-scoped path too. Is your concern that a separate move introduces another operation that must be ordered or coordinated with the request’s read? Does these descriptions match your understanding, or is there an important constraint I’m missing? Thanks again for working through this with us! :P @PeaBrane Edit: updated PR at #181 which contains prefetch and move as request-independent hints. Adding request-scoped move is relatively straightforward on top of the infrastructure as well. |
Summary
Add move semantics to request-scoped
kv.fetchhints. The destination fetches or reuses its cached data, while the source removes eligible KVCR-owned DRAM (G2) copies. Destination completion does not wait for source cleanup.Changes
deliver()remains a copy.Behavior
deposit(no_evict=True)and preserve incompatible source layouts. Framework-owned memory and G3 copies are untouched.abandon_timeout_msallowance while sources keep their normal transfer timeout. A destination delayed beyond that allowance can still fail after source removal.Validation
Native checks predate the final ownership fix, which is covered by focused unit regressions. Full vLLM/Dynamo serving was not rerun for this revision.