Repository navigation
Conversation
200lz
requested review from
ByronHsu,
Duyi-Wang,
HaiShaw,
ShangmingCai,
hnyls2002 and
sogalin
as code owners
August 27, 2026 14:51
Contributor
Author
|
Hi @ShangmingCai @YAMY1234 — this restores the deferred-release accounting invariant from #35360 / 30a50a2. I included a current-main negative control: an unresolved count of 1 is erased by a later Failed-room skip, producing a false ABORT_ACK; the patched path leaves the count at 1 and does not ACK. The PR is split so the per-chunk accounting fix is independently reviewable from the NIXL ERR-handle retention change. Thanks for taking a look. |
This was referenced Sep 5, 2026
This was referenced Sep 5, 2026
This branch has not been deployed
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.
Motivation
#35360 made
_staging_outstandingthe Prefill-side drain condition fordeferred Decode-side KV release: chunks are counted at dequeue so that
Failedwithoutstanding == 0means there is no remaining transfer workthat can still write.
The follow-up fix in
30a50a2relies on the same invariant for NIXL errors:when the NIXL DONE barrier exits on
ERR, the transfer group remains counted,so
ABORT_ACKis withheld and Decode falls back to the release timeout ratherthan treating the room as drained.
The current Failed-room skip path can defeat that invariant.
If another chunk for the same room was already queued, it is dequeued after
the room becomes
Failed. The worker first counts that chunk, then the skippath removes the entire room-level
_staging_outstandingentry withpop().That also erases the deliberately retained count from the earlier errored
transfer group.
A later deferred-ACK check can therefore observe
outstanding == 0and emitABORT_ACKeven though the earlier NIXL transfer group has not been provenquiescent.
A deterministic negative control reproduces this on upstream
main:This PR restores the intended outstanding-count invariant.
Changes
The PR is intentionally split into two independently reviewable commits.
1. Preserve unresolved accounting on the Failed-room skip
fix(pd): preserve unresolved count when skipping failed NIXL chunksThe Failed-room skip now removes only the skipped chunk's own contribution
instead of clearing the entire room-level count.
This preserves any earlier unresolved group's outstanding state and keeps
ABORT_ACKblocked.2. Retain unresolved handles after NIXL transfer errors
fix(pd): retain unresolved NIXL handles after transfer errorsWhen a handle reports
ERR, the worker performs one complete poll sweep,retains errored and otherwise unresolved handles, does not retain already-DONE
handles, marks the room Failed, and returns to the worker loop without waiting
indefinitely for a sibling that may remain
PROC.The failed group's outstanding count remains positive, so it cannot become
eligible for deferred
ABORT_ACK.The existing all-DONE path is unchanged.
Mooncake
I also checked the analogous Failed-room skip in Mooncake.
The NIXL-specific failure sequence fixed here does not apply to Mooncake's
normal path:
queue and processed serially;
batch_transfer_sync;process a later same-room Failed skip.
Mooncake is therefore not modified by this PR.
Why unresolved NIXL handles are retained
This PR deliberately does not call
release_xfer_handle()on errored orunresolved handles.
The current NIXL UCX release path does not provide SGLang with a reliable
"abort incomplete" result that could be used as a quiescence proof. Retaining
unresolved ownership is therefore the conservative behavior here.
Transport-backed reclamation for permanently unresolved handles is left as
follow-up work.
Tests
All added regressions are deterministic CPU tests; no GPU, RDMA device, or
installed NIXL runtime is required.
Negative control on upstream main:
Patched branch:
Also verified:
The new regression file is registered with:
No
runner_configs.ymlchange is required.Not covered / follow-ups
validated that those paths must preserve the same per-chunk accounting
invariant; the [PD] Align defensive protocol behavior across Mooncake, NIXL, and Mori #35281-specific compatibility change is intentionally not
included here because it is not on
main.Checklist
Related: #35360, #35049, #35281,
30a50a2CI States
Latest PR Test (Base): ❌ Run #33084550496
Latest PR Test (Extra): ❌ Run #33084550468
Latest PR Test (AMD ROCm 7.2): ❌ Run #33084552980