Repository navigation
Conversation
KastanDay
requested review from
ByronHsu,
HaiShaw,
ShangmingCai,
Ying1123,
hnyls2002,
iforgetmyname,
merrymercy,
ping1jing2,
sogalin and
xiezhq-hermann
as code owners
July 27, 2026 18:59
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
KastanDay
force-pushed
the
kastan/mooncake-transfer-ownership-barrier
branch
from
July 27, 2026 19:10
d7bb2e3 to
d3f10fb
Compare
KastanDay
force-pushed
the
kastan/mooncake-transfer-ownership-barrier
branch
from
July 28, 2026 16:51
a9a5465 to
7a1b41b
Compare
KastanDay
force-pushed
the
kastan/mooncake-transfer-ownership-barrier
branch
from
August 10, 2026 16:11
7a1b41b to
3d1af6c
Compare
…ame, adopt handler-owned writer counts
3 of 5 tasks
… counting them as drained
ShangmingCai
left a comment
Collaborator
There was a problem hiding this comment.
Hi @KastanDay , thank you for the digging and the PR, but I think this PR is quite complicated since it introduces many behavior changes. I will explore a more lightweight and universal fix for all transfer backends in the following week.
This was referenced Aug 16, 2026
ShangmingCai
pushed a commit
to ShangmingCai/sglang
that referenced
this pull request
Aug 17, 2026
…in-ack Blocker from review: the drain-ack fires when _staging_outstanding[room] == 0, which assumes send_kvcache leaves no write in flight when it returns. That holds for the default synchronous batch_transfer_sync path, and for the custom-mem-pool executor path on success (it awaits every layer future). But on the failure branch that path returned on the first failing future and only called future.cancel() -- a no-op for an already-running future -- so sibling layer writes could still be landing on the decode pages after the ack (the same lingering-write hazard sgl-project#32564 calls out). A premature ack there = the corruption this PR prevents. Route the three executor loops through _await_transfer_futures(): when deferred release is enabled it cancels the not-yet-started futures but still drains the running ones before returning, so _staging_outstanding == 0 provably means no write is still touching the decode pages. With the feature off it keeps the original early-return-on-first-failure behavior byte-for-byte. Scope: covers the KV-page write paths (send_kvcache / _dcp / _slice); send_aux is synchronous. The mamba/state-component path (maybe_send_extra) is not routed through this yet -- noted as a follow-up for hybrid models. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 of 4 tasks
Author
|
Mostly resolved now. |
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.
The bug
An abort or timeout during a prefill→decode transfer frees the request's KV pages back to the allocator, but it does not cancel the transfer. The next request can then be allocated those same pages and have them overwritten by the earlier, still-in-flight write, causing the next request to hold the first request's KV, with no error raised anywhere.
@ShangmingCai already anticipated this in #27372, which prevents the prefill starting new work after an abort and this PR implements what he mentioned as a TODO, which is handling transfers that are already in flight.
The bug's window is narrow: the transfer has to still be in flight and the pages have to be reallocated before it lands. But in our setup, we often abort requests that are too slow, so I believe we are hitting this scenario. The only symptom is a slightly wrong answer to our users.
No cost on the happy-path: no extra collective and no wait unless a request has failed.
Only ~995 lines of implementation code, the rest is tests and doc strings.
Reproduction script
This script reproduces the bug on current
main(CPU only, no GPU):Reporting a terminal state is what lets the allocator hand those pages out, so
Failedat [T3] is the bug. Asserting the ordering makes it deterministic. With this PR, [T3] reportsTransferringand terminal arrives only after the write returns.The core change
No new states and no new message types.
KVPollkeeps its five states (Failed,Bootstrapping,WaitingForInput,Transferring,Success); what changes is which one the scheduler is told, and when:Failedimmediately. A terminal state is what authorizes the allocator to hand the request's pages out — so pages could be reused while a transfer was still writing to them.Transferring— a state the scheduler already knows to wait on.Failedarrives only with proof. (Skipped entirely when nothing failed, so the happy path is unchanged.)Correspondingly,
ABORT_ACKchanges meaning: from "I heard your abort" to "your pages are safe to reuse." Prefill now sends it only after that room's transfer work has drained, and it must echo a per-request nonce.Each bootstrap room gets a lease-counted lifetime. Transfer work holds a lease; a failed request closes the room; pages are released once it is closed with zero leases and, on decode, once every peer has acknowledged.
Required for the barrier to hold
Fixed these three paths from bypassing the abort:
send_kvcachereturns on the first failing future, butFuture.cancel()is a no-op once a future is running — so siblings keep writing while the worker reports the chunk done.bootstrap_roomis recycled, so a queued chunk can lease the wrong request's lifetime.Tests
STRICTpolicy), 192/192 immediate victim reuse probes succeeded after 192 in-flight donor aborts. There were zero transport, semantic, hard, or critical failures; zero foreign canaries; a decode transfer-queue peak of 20; and zero model restarts. Raw summary, matrix, telemetry, and the admission-control disclosure.71 tests named by property. Every fix has a regression test verified to fail with that fix reverted.
unit/disaggregation72 → 143 passed;unit/managers+unit/mem_cacheunchanged apart from additions.Related PRs
Glad for this to land as a completion of #27372. @AsterWang identified this race first in #24580 but I think my solution is a bit safer.
CI States
Latest PR Test (Base): ❌ Run #31566610945
Latest PR Test (Extra): ✅ Run #31620191653