Repository navigation
[Bugfix][KV Offload][P2P] Stop DEALER monitor before closing its reader - #63207
Conversation
|
Validated Setup: three single-GPU engines (Qwen3-0.6B) on nightly
"Frozen" matches your description: once B goes away, A's ROUTER receives no messages from any peer, A to C pulls drop to near 0, and every lookup ends at the 30 s deadline for the rest of the run (about 190 s). With the PR, no A to C lookup hits the deadline, p50 0.05-0.26 s. On this setup the stall path never wedged, even on the nightly. It was the removal paths (force delete, graceful delete, kill, roll) that froze every time. These runs don't isolate which of the two commits is responsible: I didn't run either alone, and the logs don't show whether a message was still unsent in the DEALER queue at close, which is the stall path's precondition. With the freeze gone, a second failure shows up: over RDMA, a peer that stalled or was replaced was not usable again during the run. Either registration fails (the UCX |
vMaroon
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| self._sockets.dealer.close() | ||
| # A lingering DEALER outlives close() and keeps reconnecting to an | ||
| # unresponsive peer to deliver its queued messages. | ||
| self._sockets.dealer.close(linger=0) |
There was a problem hiding this comment.
Suggestion: use a short bounded linger instead of linger=0
linger=0 also drops the DisconnectMsg that the send-then-close callers rely on. In _accept_new_peers, the reconnect-backoff branch does:
# Tell the peer, so its lookups fall back now rather than
# at the handshake deadline.
with contextlib.suppress(Exception):
conn.send({TYPE_KEY: DisconnectMsg.TYPE})
conn.close()That conn is the inbound DEALER opened in the same poll(), so its TCP connect hasn't finished when close() runs. With linger=0 the queued message is discarded, so the rejected peer waits out VLLM_P2P_HANDSHAKE_TIMEOUT_S (30 s), which is what that comment is there to prevent. session.close() sends the same message the same way, so it's exposed when the session is new or the peer is slow.
I measured this with two ZmqTransports on one host (pyzmq 27.1.0, libzmq 4.3.5). Each trial sends DisconnectMsg and then calls close(), and counts whether the peer's ROUTER receives it within 1–3 s:
close() variant |
right after connect() |
on an established connection |
|---|---|---|
| main (default linger) | 30/30 | 30/30 |
this PR (linger=0) |
0/30 | 30/30 |
disable_monitor() + linger=500 |
30/30 | — |
With disable_monitor() + close(linger=500), both new tests pass 3/3 locally. I didn't use the default infinite linger: disable_monitor() with the default linger hangs test_close_with_unsent_message_keeps_transport_usable in 3/3 runs, so some limit is needed.
So I'd suggest something like:
# Bounded so a final DisconnectMsg still flushes, including on a connect
# still in progress, without leaving a DEALER reconnecting to a dead peer.
self._sockets.dealer.close(linger=_CLOSE_LINGER_MS)with _CLOSE_LINGER_MS = 500 next to the heartbeat constants. close() still returns immediately; only the background teardown waits, for at most 500 ms.
There was a problem hiding this comment.
Agreed, linger=0 drops it: a message sent right after connect() and then close() arrives 0/30 on 478d86b21d and 30/30 on main. #63708 already sets LINGER to _DEALER_LINGER_MS on every DEALER, so I rebased onto it and dropped the close(linger=0) change; the PR is now only the disable_monitor() call and the two tests. The 500 ms linger alone doesn't stop the wedge: on main 51d2a85594 both new tests fail 3/3. With disable_monitor() they pass 3/3, the final-message check is 30/30, and the P2P suite is 326 passed.
liranschour
left a comment
liranschour
left a comment
There was a problem hiding this comment.
Looks good. I think that we need to close with linger=500 (wait for 500 ms before closing).
Look at the inline comment below
libzmq sends monitor events with a blocking send from its only I/O thread. ZmqConnection.close() closed the monitor's reader while the DEALER could still raise events: EVENT_CLOSED when a connect still in progress is torn down (a departed pod or lost node), and handshake or reconnect events while the DEALER lingers to flush queued messages. The send then never returns and every P2P control socket in the engine stops until restart. Stop the DEALER's monitor before closing the reader. The DEALER keeps its bounded LINGER, so a final DisconnectMsg still reaches the peer. The close tests keep a wedged transport referenced: garbage collection would terminate its context, which blocks forever, so a regression hung the test run instead of failing. Signed-off-by: Maroon Ayoub <mayoub@redhat.com>
478d86b to
b6c8ba4
Compare
|
This PR is still needed on top of #63708: at Re-ran the GPU fault cases against
So the bounded linger alone does not stop the freeze on GPUs. On |
There was a problem hiding this comment.
LGTM. With #63708's 500 ms linger on main, this is now just disable_monitor(), and that's the part that fixes the wedge.
Checked at b6c8ba4bd6 on current main (pyzmq 27.1.0, libzmq 4.3.5):
test_zmq_transport.py: 22 passed, 3/3 runs.- The two new tests against main's
zmq.py(500 ms linger, nodisable_monitor()): both fail, 3/3. DisconnectMsgsent thenclose()right afterconnect(): 30/30 delivered (0/30 at the old head withlinger=0); 30/30 on an established connection.tests/v1/kv_offload/tiering/p2p: 327 passed.
My earlier linger=0 thread is addressed by the rebase onto #63708; feel free to resolve it.
|
/ci run |
|
✅ @vMaroon, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #97923 for commit |
Overview
ZmqConnection.close()closes the monitor's reader while the DEALER can still raise monitor events. libzmq sends those events with a blocking send from its only I/O thread, so the send never returns and every P2P control socket in the engine stops until restart; P2P lookups to all peers then wait out their 30 s deadline before falling back to local prefill. Two paths raise such an event: a DEALER kept alive by itsLINGERafter its peer stalls, and a DEALER whose TCP connect is still in progress when it is closed (a peer address with no host behind it, as after a pod leaves or a node is lost). This stops the DEALER's monitor before closing its reader. The DEALER keeps the bounded 500 msLINGERfrom #63708, so a finalDisconnectMsgstill reaches the peer.Found while qualifying the P2P tier for a GLM-5.3 P/D deployment. Rebased on #63708, which bounds the DEALER
LINGERat 500 ms; the wedge reproduces on main with that linger.Claims
DisconnectMsgsent right beforeclose()still reaches the peer, including on a connect still in progress.Validation
Unit tests. Each closes one connection, then checks that a new connection on the same transport still delivers a message. A wedged transport is kept referenced until interpreter exit, since terminating its context blocks forever, so a regression fails the test instead of hanging the run.
test_close_with_unsent_message_keeps_transport_usable: a message is queued to a listener that never answers the ZMTP greeting.test_close_while_connecting_keeps_transport_usable: the TCP connect is still in progress. A listener with a full accept queue stands in for an address with no host, since Linux drops SYNs to it; the test skips when the kernel completes the connect anyway.vLLM arm64 CPU CI image (
074c83a09a, pyzmq 27.1.0, libzmq 4.3.5) with each arm'svllm/v1/kv_offloadand this branch's tests mounted, 3 runs per arm:51d2a85594(LINGER500 ms)b6c8ba4bd6close_with_unsent_messageclose_while_connectingFinal message on close (same image, two transports, 30 trials: send a message right after
connect(), thenclose(); count trials where the peer receives it within 3 s). First measured by @liranschour in review.51d2a85594478d86b21d(disable_monitor()withLINGER0)b6c8ba4bd6Minimal pyzmq repro (pyzmq 27.2.0, libzmq 4.3.5, in the deployment image). One context with a ROUTER and a DEALER monitored with vLLM's fatal-event mask and heartbeat options; the DEALER connects, then is closed; afterwards a raw TCP client checks that the ROUTER still sends its ZMTP greeting.
close(linger=0)disable_monitor(), reader closed,close(linger=0)(478d86b21d)close(linger=0)Deployment check (GLM-5.3 P/D, 4 prefill pods with the P2P tier, libzmq 4.3.5).
handshake timed outagainst a departed pod's IP was followed 8 s later by its peers loggingcontrol connection lost; from then on its P2P port accepted TCP but never sent the ZMTP greeting. One request naming a departed IP reproduced it on the fourth pod.478d86b21d(disable_monitor()withLINGER0): all 4 pods at once named a departed IP, 2 rounds. 8/8 requests completed by local prefill (43-51 s, 74-76 s for two cold first requests), all 4 pods kept sending the greeting, and a pull ring right after pulled 29,696 of about 30.4K prompt tokens on 8/8 requests.Two-process P2P harness (CPU, measured on
42b418aca8, which hadLINGER0 and nodisable_monitor(); on head the stall path is covered bydisable_monitor(), asclose_with_unsent_messageshows). Peer A runs a realP2PSecondaryTierManagerandZmqTransport, opens a request to peer B every 200 ms and pollslookup()until it resolves; peer B answers every lookup with MISS (NIXL data plane faked as in the unit tests). B isSIGSTOPed at 5 s andSIGCONTed at 65 s; the run ends at 160 s. 6 runs per arm:SIGCONT42b418aca8The harness ran on the 10-17 main plus #51504, #56836, #57139, #58168 and #60374, all merged since. The remaining delay after
SIGCONTis a session opened during the stall waiting outVLLM_P2P_HANDSHAKE_TIMEOUT_S(30 s); with it set to 5 s, 3 runs recover in 1.7 s to 4.7 s.Details
Mechanism in libzmq 4.3.5 (
src/socket_base.cpp,src/stream_connecter_base.cpp):socket_base_t::event()takes_monitor_syncand callsmonitor_event(), which sends each event to the monitor's PAIR socket with a blockingzmq_msg_send(noZMQ_DONTWAIT). These calls run on the I/O thread.ctx.term()at shutdown hangs.zmq_closehands a DEALER with queued messages to the reaper, which keeps it reconnecting for theLINGERperiod; each reconnect or handshake failure raises an event. The 500 msLINGERfrom [Bugfix][KV Offload][P2P] Never block in ZmqConnection.send() #63708 bounds that window, and an event inside it still blocks the I/O thread (close_with_unsent_messagefails on main).stream_connecter_base_t::process_term()callsclose(), which closes the socket and raisesEVENT_CLOSED(in vLLM's mask) before the DEALER's monitor is stopped. This happens with any linger.disable_monitor()iszmq_socket_monitor(s, NULL, 0), which runsstop_monitor()under_monitor_sync: it closes the internal PAIR and clears_monitor_events, so no event is sent after the reader closes. With the boundedLINGER, a dead session's DEALER stops reconnecting to its peer after 500 ms.disable_monitor()waits for_monitor_sync, which the I/O thread holds only while sending an event. That send completes: the reader is still open, and its pipe (default HWM) holds far more than the few fatal events a dead connection raises beforeZmqTransport.poll()closes it.Not in scope: a request naming a peer that never answers still waits the 30 s handshake deadline (
VLLM_P2P_HANDSHAKE_TIMEOUT_S) before prefilling locally, and nothing backs off from such a peer across requests.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.
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.