Repository navigation
[Scheduler] Fix final chunked-prefill abort commit race - #34153
jeremyzhang866 wants to merge 6 commits into
Conversation
|
@cctry @hnyls2002 Could you please take a look when you have time? This PR fixes the final chunked-prefill abort/result-commit race in the regular overlap scheduler and adds a deterministic scripted-runtime regression. The change is intentionally scoped to the non-PD path; local unit, grammar/spec, formatting, registration, and MLX end-to-end race tests pass. |
3022150 to
b1da9c2
Compare
|
/tag-run-ci-label |
|
@cctry @hnyls2002 The updated head |
b1da9c2 to
a7380b2
Compare
|
@xiezhq-hermann Could you please take a look at the current head |
a7380b2 to
04af626
Compare
|
@cctry @xiezhq-hermann A gentle follow-up on the current head |
04af626 to
b178911
Compare
|
@cctry @xiezhq-hermann Update: the branch has now been rebased onto |
b178911 to
a370243
Compare
|
@cctry @xiezhq-hermann Final update after syncing with current |
a370243 to
5cc00a8
Compare
5cc00a8 to
141a3e2
Compare
|
@cctry @xiezhq-hermann Update: this PR is now rebased onto current |
JulianZJN
left a comment
There was a problem hiding this comment.
I went through the current head against the regular overlap loop and the prefill result processor.
The two protections are at the right commit boundaries:
- while
result_queueholds the delayed final-prefill result, a prefill request withto_finishis excluded before the prefill-to-decode merge, so it cannot take an optimistic decode step; process_batch_result_prefillconsumes the pending finish with an accepted length of zero before attaching the sampled token or token-derived state.
The mixed-batch exception also looks necessary: requests already in decoding_reqs retain the existing partial-output semantics. The packed-result handling is important as well—the aborted segment still advances hidden-state and logprob cursors, and the two-request test would catch a slice shift into the following live request.
I do not see a code-level blocker in this scope. The remaining validation gap is the hardware path. The original symptom was reproduced under CUDA overlap on RTX 4090, while the deterministic end-to-end regression reported here ran through MLX; the official CPU/CUDA/AMD shards are still gated on run-ci. Before merge, I would want the current head exercised by the gated one-GPU shard or the original CUDA reproducer, retaining the assertions on zero post-abort tokens, no decode_batch_idx advance, and complete KV/request-pool reclamation.
|
@JustinTong0323 Could you please take a look when you have time? This PR complements #35255: that PR makes a deferred abort follow a request after it leaves the chunked-prefill slot, while this one prevents a delayed final-prefill result from committing a token or entering optimistic decode after the abort has won. The current head |
JulianZJN
left a comment
There was a problem hiding this comment.
Follow-up to my earlier review: the CUDA validation gap is now closed for the scope claimed by this PR.
I independently ran a matched red/green validation on one RTX 4090 D against the exact current head 141a3e22e0c6f03882d640174323086596a38c8b:
- unpatched base
7605529bdf98ded2815e48c7054b1bc4533256c9: the first CUDA run reproduced the target failure,chunked-prefill result appended a token after abort: [38297]; - exact PR head: the same deterministic scripted CUDA regression passed 10/10 independent server/scheduler lifecycles;
- clean synthetic merge onto
mainateac91ac362f1ea9caf383cabfc2a57bf7e07b367: the focused CPU set passed (11 passed,2 subtests passed) and the CUDA regression passed 5/5. Currentmain(d9f6861359) is only one docs-only commit beyond that tested revision.
The scripted oracle is substantive: it checks zero post-abort output tokens, no decode_batch_idx or KV committed-watermark advance, request-pool and KV release, restoration of reclaimable KV capacity, an empty result queue, and a fully idle scheduler after drain. The packed hidden-state/logprob cursor tests also pass.
The older #34112 attachment was not runnable as a standalone artifact because its backends.sglang.* dependency was absent. That does not weaken the causal result for the current #34149 race: the same test fails on the exact base at the intended delayed final-prefill boundary and passes on the patch.
Scope remains TP=1 / DP=1 / PP=1 regular overlap scheduling on CUDA; PD-prefill and multi-rank runtime validation are not claimed. Within that scope, I no longer see a code or hardware-validation blocker and approve this change.
The required pr-test-finish check is still red only because call-gate / pr-gate rejected the missing run-ci label and all substantive shards were skipped. @cctry @xiezhq-hermann @hnyls2002, could one of you trigger the standard tag-and-rerun CI command for this head?
|
@JustinTong0323, quick follow-up while #35255 is in active CI: I rechecked current #35255 handles deferred abort delivery after a request leaves the chunked-prefill slot; this PR protects the downstream result-commit and optimistic-decode boundaries after that abort wins. The exact head |
|
@xiezhq-hermann @cctry Could you please take another look at #34153 when convenient? I rechecked current #35255 is complementary rather than a replacement: it fixes deferred abort delivery after the request leaves the chunked-prefill slot, while #34153 protects the downstream result-commit boundary after that abort wins. The exact head |
141a3e2 to
30ae497
Compare
|
@hnyls2002 Could you please review the updated head I rebased the single commit onto current The scripted race oracle is registered for CUDA/AMD. Current main now runs the local Mac path with overlap disabled, so I am not treating the old hardware result as validation of this new SHA. #35255 remains open and complementary. Could you review the scheduler/result-commit scope and add |
4947a91 to
f74c17a
Compare
JulianZJN
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased current head f74c17adf0e8.
The two production protections remain at the correct boundaries for regular overlap scheduling: pending-finish prefill requests are excluded before optimistic decode while a delayed result is queued, and the result processor consumes the pending finish without committing the sampled token or token-derived state. The new beam-search branch is not a blocker for this logic: beam requests delegate finish/abort ownership to BeamCoordinator.commit_prefill(), which checks req.to_finish and aborts the group before committing staged selection.
For the claimed non-beam TP1/DP1/PP1 scope, I do not see a new code-level blocker after the rebase. My matched CUDA red/green run was against the earlier exact head 141a3e22e0c6, so this approval does not represent exact-SHA GPU validation of f74c17adf0e8; the current official run-ci shard is still required.
|
@cctry @xiezhq-hermann Could you please take one more look at the rebased current head The Scheduler/result-commit protections remain unchanged in scope, and the exact head has been re-reviewed with no new code-level blocker identified. Lint is green, but the official CPU/GPU shards are still skipped because the PR has no |
f74c17a to
e6acb48
Compare
|
Update on the current head:
@cctry @xiezhq-hermann @hnyls2002, could you review the rebased head and, if the scope looks right, trigger the required |
|
@cctry Since you merged #35255, could you please take a look at #34153 when convenient? The two PRs cover adjacent abort boundaries: #35255 handles deferred abort delivery after a request leaves the chunked-prefill slot, while #34153 protects the delayed final-prefill result from committing a token or entering optimistic decode after the abort wins. #34153 is now rebased onto the latest |
|
Update on #34153: I checked the branch against the current The patch remains focused on the delayed final-prefill result boundary: when abort wins, the result must not commit a token or enter optimistic decode. This is complementary to #35255, which handles deferred abort delivery after the request leaves the chunked-prefill slot. After the rebase I will report the new head and rerun the available static/targeted checks. Could a maintainer please review the rebased head and, if the scope still looks right, add the |
0f46b6e to
bcf7e2e
Compare
f0e1881 to
b3dc038
Compare
|
Rebased onto current |
|
@hnyls2002 Could you take a look at this PR when you have a chance? Thanks! |
|
@hnyls2002 Could you take a look at this PR when you have a chance? Thanks! |
Motivation
Fixes #34149.
Related prior report: #34112.
While testing chunked-prefill cancellation on latest
main, we independently reproduced the same user-visible symptom reported in #34112: a cancelled request can expose one token and then finish withabort.The #34112 attachment used SGLang v0.5.9. Its negative batch-level output IDs were old FutureMap relay handles and are not the failure criterion or target of this PR. Current
mainno longer uses thatScheduleBatch.output_idsrepresentation.Our deterministic current-main reproduction isolates a final-prefill overlap race. An abort can arrive after the final chunked-prefill
EXTENDis launched but before its delayed result is committed. At that pointscheduler.chunked_reqis already clear,Req.output_idsis empty, and the result is pending inresult_queue.Root cause
There were two commit boundaries to protect:
abort_request()recordsreq.to_finish = FINISH_ABORT(), but the prefill result processor previously appended the sampled token and token-derived metadata beforeupdate_finish_state()consumed the pending finish.decode_batch_idx/ the KV committed watermark, and perform an optimistic decode step and its associated allocation before the result is dropped (which may reserve multiple slots or pages under speculative decoding).Filtering only in the output streamer is too late: request output, reasoning state, logprobs, hidden states, sampling metadata, grammar state, and KV accounting may already be mutated.
Modifications
new_accepted_len=0and reuse normal finished-request cleanup.result_queuecontains a delayed result.Regression coverage
The scripted test deterministically waits for this window:
EXTENDis launched;scheduler.chunked_req is None;Req.output_idsis empty;It then aborts and verifies:
FINISH_ABORT;Additional CPU tests cover both sides of the scheduler gate (pending queue vs. already-processed result), mixed prefill/decode membership, and a two-request packed-logprob case where the aborted segment advances the cursor without attaching metadata to the aborted request.
Accuracy Tests
Cancellation control flow only; normal token generation is unchanged.
Validated on top of
maincommit7605529bdf98ded2815e48c7054b1bc4533256c9:Also passed:
git diff --check.The packed-logprob test was mutation-checked: it fails both if the aborted request attaches returned logprobs and if its packed cursor contribution is skipped.
Speed Tests and Profiling
Not applicable. The new checks are scheduler control-flow predicates; the result-drop branch runs only for a request that is already finishing.
Topology notes
The race is not caused by TP, DP, or PP. TP/DP use the same per-request result state. PP and MLX process their pending result before the relevant
last_batchis reused, so the queue-gated admission change is a no-op there while the result-drop protection remains valid. PD-prefill is intentionally out of scope because it has a separate transfer and result lifecycle.The registered end-to-end regression is TP=1 / DP=1 / PP=1. Multi-rank topology behavior was source-reviewed but not claimed as a multi-GPU runtime validation.
Checklist
main.CI States
Latest PR Test (Base): ❌ Run #37111470582
Latest PR Test (Extra): ❌ Run #37111470433
Latest PR Test (AMD ROCm 10): ❌ Run #37111470553