Repository navigation
Conversation
3 of 5 tasks
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
KastanDay
force-pushed
the
kastan/upstream-unified-abort-cleanup
branch
from
August 10, 2026 16:11
d828be3 to
ac49149
Compare
Owner
Author
|
Note, we're not using |
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.
Note, we're not using
UnifiedRadixCachecurrently, so skip.TLDR: L3/HiCache storage prefetch abort path in UnifiedRadixCache
Motivation
UnifiedRadixCache.release_aborted_request()could take different cleanup paths on different attention ranks for the same request. One rank may already have allocated host pages and started storage I/O while another still sees a pending prefetch, or noongoing_prefetchentry at all.Before this change, the active rank entered the transfer barrier while the pending/missing rank returned early. That creates two related hazards:
The sidecar-length portion of the old fork PR was intentionally removed because equivalent coverage is already present on current SGLang
mainvia upstream PR sgl-project#31443. This replacement contains only the still-unique abort-cleanup fix.Modifications
MAXacross attention groups before any rank returns.This is intentionally one PR: the state reduction and the barrier participation rule are one atomic distributed invariant. Splitting them would leave an intermediate revision that still deadlocks or releases ownership inconsistently.
Reproduction
The regression tests model two ranks without timing sleeps:
The important cases are:
On the old implementation, the first two cases return before the barrier and fail the asserted collective contract. With this patch, every case joins exactly once and ownership is released only by the rank that owns it.
Experimental validation
The original integrated candidate was exercised in an isolated GLM 5.2 staging deployment on three H200 nodes in both 2-prefill/1-decode and 1-prefill/2-decode topologies. Real
stream=truedisconnects reached active KV transfer and producedKVTransferError: Aborted by AbortReq; bounded victim requests were 8/8 correct, with no foreign canary and no engine restart. This is integrated-stack evidence, not a claim of per-commit isolation.For this current-main rewrite:
python3 -m py_compilepassed for the implementation and new test file.git diff --checkpassed.tritonimport path; repository CI should execute the registered CPU tests in the supported environment.Relationship to other work
UnifiedRadixCache.Performance
The change adds one small attention-group reduction on the abort path only. The normal prefetch and decode paths are unchanged.
Checklist
maingit diff --checkCI States
Latest PR Test (Base): ❌ Run #31407663783
Latest PR Test (Extra): ❌ Run #31407663386