Repository navigation
[Bugfix][KV Offload][P2P] Fault a peer whose source WRITE stalls - #64251
Conversation
|
Documentation preview: https://vllm--64251.org.readthedocs.build/en/64251/ |
mergify
Bot
added
documentation
Part of vllm-project#63602 (gap 2, source half). Stacked on the gap 3 retention change. A transfer NIXL keeps reporting as PROC/PEND was never bounded on the source: poll() treats it as "wait", the abort drain holds it and its AbortAck forever, and nothing faults the peer. write_blocks() is gated only on peer_fault, so the source kept posting WRITEs to a wedged peer and accumulated stuck transfers and pins. ServerRole now stamps each transfer's submit time. Once one outlives VLLM_P2P_TRANSFER_STALL_TIMEOUT_S (default 20 s, 0 disables), the peer is faulted through the existing vllm-project#60374 path: no new WRITEs, DisconnectMsg so the consumer fails its loads, reap with the data-plane reconnect backoff. It releases nothing; the reap retains the stalled transfer and its pins until the transport reports it stopped. A stall fault skips the 10 s peer-fault drain, since the stalled transfer would never drain. Signed-off-by: Liran Schour <lirans@il.ibm.com>
6e57bb4 to
ffaa825
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
orozery
left a comment
orozery
left a comment
There was a problem hiding this comment.
Left some unverified Claude comments.
| elif self.peer_fault is None: | ||
| self._check_stalled_transfers() |
There was a problem hiding this comment.
Claude:
The check runs before the done/failed loops, so it faults the peer over transfers this very poll already resolved — move it after the drain.
There was a problem hiding this comment.
Moved the stall check after the done/failed drain.
| now = time.monotonic() | ||
| oldest = min(x.submitted_at for x in self._inflight.values()) | ||
| age = now - oldest |
There was a problem hiding this comment.
Claude:
Age-since-submit includes time nobody polled, so one scheduler stall past the timeout faults every session that has an in-flight WRITE; measure progress between polls instead.
There was a problem hiding this comment.
Thanks. I think measuring age since submit is the right call. The reorder for comment 1 (e260881) takes care of most of the scheduler-stall case, and the rest is safe.
What the reorder covers. The stall check now runs after the done/failed drain. Say the scheduler doesn't poll for longer than the timeout. Every WRITE that finished during the gap comes back in that next poll() and is drained as done before _check_stalled_transfers looks at the oldest in-flight transfer. Missed polls don't hold transfers up either: UCX runs its own progress threads, so WRITEs keep going while nobody polls. So a scheduler stall no longer faults sessions whose WRITEs actually finished. The fault fires only if a WRITE is still PROC/PEND, by a fresh state read, more than 20s after submit. On a healthy lane that's milliseconds of work, so we treat it as a real stall wherever the time went.
Why per-poll progress doesn't help. NIXL only gives us a transfer's state, not how many bytes have moved. Between two polls a pending WRITE looks the same whether it is moving or wedged. "Progress between polls" would just mean "still PROC/PEND". That's what the check already tests, and it uses fresh state.
Why the fault is safe. The stall check never releases anything on a timer; it only marks the peer faulted. Every release after that goes through a fresh state read:
- Teardown:
close()cancels withmode="wait". That checkscheck_xfer_stateper transfer and releases only DONE/ERR. A WRITE still PROC/PEND is retained, with its source pins, not freed. - Hold cap:
_poll_retainedcallspoll_retired()(another state read) before its 30s cap force-releases anything. Only transfers still running after that read are released. - Abort drain and store timeout: both cancel with
mode="wait"too.
The only release that skips the state check is the shutdown drain's mode="immediate" fallback, and it runs only after its own wait-and-poll loop has timed out.
So even a fault after a long gap can't release a WRITE that might still be landing. At worst, a session whose WRITE is still pending after the gap gets torn down. That's what the stall timeout is for.
There was a problem hiding this comment.
I'd rather keep the age-since-submit measure. There are two kinds of polling gap:
- The scheduler thread is busy but the process runs. NIXL's progress thread and the UCX worker threads (
num_threads=4) keep the WRITE moving without us polling. After the reorder in e260881, anything that completed during the gap is drained before the stall check. A transfer still PROC/PEND 20s after submit had 20s of real progress time, so faulting it is correct. - The whole process is frozen (SIGSTOP, VM pause). We can't tell this apart from a stall, and we don't need to: the session is lost either way. The peer's ZMQ heartbeat (10s) drops us and the consumer's load timeout aborts the fetch. Clamping the gap would keep a session alive that the peer has already reaped, and only delay our own teardown.
In both cases the fault releases nothing. Teardown and the hold cap re-read transfer state before releasing anything, so there's no correctness risk in calling the transfer stalled.
…tions The stall check ran before the done/failed loops, so a transfer the transport had just reported complete was still in _inflight and could fault the peer. Run it after the drain. Addresses review comment on vllm-project#64251. Signed-off-by: Liran Schour <lirans@il.ibm.com>
|
/ci run |
|
✅ @liranschour, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #98271 for commit ℹ️ This PR is 6 commits behind upstream |
|
/ci run |
|
✅ Triggered Buildkite CI #98292 for commit ℹ️ This PR is 3 commits behind upstream |
Retire queued fetches locally, release producer storage without closing newer work, and preserve post-disconnect abort acknowledgements after rebasing on vllm-project#64251. Co-authored-by: OpenAI Codex Signed-off-by: jiuyueQ <631905507@qq.com>
Retire queued fetches locally, release producer storage without closing newer work, and preserve post-disconnect abort acknowledgements after rebasing on vllm-project#64251. Co-authored-by: OpenAI Codex Signed-off-by: jiuyueQ <631905507@qq.com>
Overview
Part of #63602 (gap 2, source half). Builds on #64250 (merged), which retains the stalled transfer across the teardown this fault triggers. One commit on main (
ffaa825b33).A source WRITE that NIXL keeps reporting as
PROC/PENDpastVLLM_P2P_TRANSFER_STALL_TIMEOUT_S(default 20 s) now faults its peer. The fault releases nothing.Claims
write_blocks()kept posting new WRITEs to the wedged peer.Validation
The issue's source-side property: a WRITE still pending at 300 s must have faulted its peer, while keeping the transfer and its store job held.
The issue body's
test_gap2_pending_transfer.pystill fails, for an unrelated reason. It hard-codes the consumer failing at exactly 40 s, but since #63781 the consumer fails at 35 s (30 s load + 5 s abort-ack). The check above is the same scenario without that hard-coded timing.New tests, run with this PR's tests on #64250's code:
(The two that pass guard the no-fault cases: under the deadline, and the deadline disabled with 0.)
With this PR:
CPU-only, and CI runs it in
misc.yaml. The 20 s default is not yet measured on GPU. It is far above the ~131 ms p99 tiering-lock hold we measured on H200 at c256, and above healthy transfer times seen so far. Before this leaves draft, I'd like to measure transfer duration by size under load from NIXL telemetry, and run a real stall (rc_gdare-enabled, as in #63499).Details
Root cause.
NixlTransport.poll()treatsPROC/PENDas "wait" forever._drain_abortholds a stuck transfer and its AbortAck forever, and only logs after 10 s.write_blocksis gated only onpeer_fault, so the source keeps feeding the wedged peer.In the GPU run in #63602, 36 WRITEs never completed, and pulls from a healthy peer dropped (679/847).
Change.
_InflightXfer.submitted_atis set on submit.ServerRole.collect_results()checks for stalls when the transport reports no fault. Once the oldest in-flight transfer is past the timeout, it setspeer_fault = "transfer stalled for Ns"andtransfer_stalled._escalate_peer_faultthenDisconnectMsg(the consumer fails its loads), reap, and the data-plane reconnect backoff._escalate_peer_faultskips its drain wait whentransfer_stalledis set.Detection lives in
ServerRole, notNixlTransport. It sits next to the_STORE_TIMEOUT_Slogic it mirrors, it doesn't depend on which transport is used, and the session reproducers exercise it directly.Why it depends on #64250. Without retention, the teardown this fault triggers would run
cancel(mode="immediate")and unpin the stalled transfer's blocks. That is the very release the deadline must not cause.New env var
VLLM_P2P_TRANSFER_STALL_TIMEOUT_Sis a float, default 20, and 0 disables it. It is documented invllm/envs.pyand in the P2P table indocs/usage/security.md, next to the otherVLLM_P2P_*limits.Complexity: about 40 non-test lines.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI. (GPU measurement of the default pending; see Validation.)
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
🤖 Generated with Claude Code