Skip to content

[Bugfix][KV Offload][P2P] Never block in ZmqConnection.send() - #63708

Merged
orozery merged 4 commits into
vllm-project:mainfrom
liranschour:p2p-zmq-nonblocking-send
Oct 26, 2026
Merged

orozery merged 4 commits into
vllm-project:mainfrom
liranschour:p2p-zmq-nonblocking-send

Conversation

@liranschour

@liranschour liranschour commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Part of #63602 (gap 1).

ControlConnection.send() is documented as non-blocking (control/base.py: "ControlConnection.send() must not block"), but ZmqConnection.send() called dealer.send(data) without NOBLOCK, at libzmq's default SNDHWM and with no SNDTIMEO. Once the queue toward a peer that stops reading fills, the send blocks. Every caller holds the tiering manager lock: the tier-poll thread holds it for a whole round, and OffloadingConnectorScheduler._acquire_step() waits on it with no timeout. A blocked send therefore stalls healthy-peer servicing and the engine's next scheduler step.

This PR:

  • Sends with zmq.NOBLOCK at an explicit SNDHWM (1000, unchanged from the libzmq default but no longer implicit) and raises ConnectionError when the queue is full. P2PSession._do_send already marks the connection dead on any send error, so a stalled peer is reaped and its pending work fails over to local prefill. [Bugfix][KV Offload][P2P] Mark the session dead when its ConnectAck send fails #63664 routes the ConnectAck send through the same path.
  • Bounds DEALER LINGER to 500 ms. While writing the test I found that, with the default (-1), closing a DEALER that still has messages queued for an unreachable peer makes Context.destroy(linger=0) hang forever. The context-level linger only applies to sockets that are still open. Before this change that path was rare, since a full queue blocked the sender first; now that queued-then-dead is the normal failure path, it has to be bounded.
  • Leaves ZMQ_IMMEDIATE at its default of 0. With IMMEDIATE=1, a NOBLOCK send fails before the TCP connect completes, which would break the ConnectMsg sent right after connect().

On the issue's policy question (fail now vs. retry on later polls): this fails now. Control messages are small, so a full queue means the peer went 1000 messages without reading while its TCP connection stayed up. That isn't short-lived backpressure, and a retry outbox would need its own bound plus ordering against the pre-ack queue.

Not a duplicate: the only open PRs referencing #63602 are #63628 (gap 4) and #63664 (gap 5). #63207 changes the DEALER monitor teardown, not send().

Test plan

New TestZmqTransportConnectivity::test_send_raises_instead_of_blocking_when_queue_full: a real DEALER connects to a port with no listener, with _SNDHWM patched to 2. send() must raise ConnectionError within 1 s, and transport.close() must return.

venv/bin/pytest tests/v1/kv_offload/tiering/p2p/test_zmq_transport.py::TestZmqTransportConnectivity::test_send_raises_instead_of_blocking_when_queue_full
venv/bin/pytest tests/v1/kv_offload/tiering/p2p
pre-commit run --files vllm/v1/kv_offload/tiering/p2p/control/zmq.py tests/v1/kv_offload/tiering/p2p/test_zmq_transport.py

Test results

  • New test with the old blocking send(): hangs in zmq/sugar/socket.py:send, called from ZmqConnection.send (faulthandler dump at 20 s).
  • New test with this change, but without the linger bound: send() raises, then the test hangs in ZmqTransport.close() → Context.destroy().
  • With this change: passes.
  • Full P2P suite (tests/v1/kv_offload/tiering/p2p): 312 passed, CPU-only (pyzmq 27.1.0, libzmq 4.3.5).
  • pre-commit: passed.

No model evaluation: this changes only control-plane failure handling, not outputs.

AI assistance (Claude Code) was used to write this change. I reviewed every line and ran the tests above.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

ControlConnection.send() is documented as non-blocking, but the ZMQ
implementation called dealer.send() without NOBLOCK, so it blocked once
the queue toward a peer that stopped reading was full. Callers hold the
tiering manager lock, so a blocked send also stalled the scheduler.

Send with NOBLOCK at an explicit SNDHWM and raise ConnectionError when
the queue is full; P2PSession._do_send already marks the session dead
on a send error. Bound DEALER linger as well: with the default (-1), a
message queued for an unreachable peer made Context.destroy() hang.

Part of vllm-project#63602 (gap 1).

Signed-off-by: Liran Schour <lirans@il.ibm.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added the bug Something isn't working label Oct 26, 2026
@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 26, 2026
@orozery

orozery commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ @liranschour, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #97291 for commit 8a890b4be877.

ℹ️ This PR is 1 commit behind upstream main. Merged upstream main into this branch as 8a890b4be877 before starting CI. Pull the branch before pushing more changes.

@orozery
orozery merged commit 51d2a85 into vllm-project:main Oct 26, 2026
78 checks passed
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…roject#63708)

Signed-off-by: Liran Schour <lirans@il.ibm.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…roject#63708)

Signed-off-by: Liran Schour <lirans@il.ibm.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants