Repository navigation
[Bugfix][TransferEngine] Keep a stalled peer from wedging the handshake daemon - #4451
Conversation
The handshake daemon serves each accepted connection inline on the listener thread, and the response path had no send timeout: the accepted socket only got SO_RCVTIMEO, and writeFully() spins on EAGAIN, which a blocking socket only reports once SO_SNDTIMEO fires. A peer that requests the Metadata descriptor and never reads the response wedges the daemon's writer indefinitely, freezing all control-plane traffic until the engine restarts (kvcache-ai#4443). Set SO_SNDTIMEO (5s, symmetric with the existing SO_RCVTIMEO) on the accepted socket, and treat EAGAIN/EWOULDBLOCK in writeFully() as a failed write returning the short count, mirroring readFully(). All existing callers use blocking sockets without SO_SNDTIMEO, so their behavior is unchanged; with the timeout set, a stalled response now fails within seconds and the daemon keeps serving other peers.
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
The two red test-wheel legs are not from this PR. This is the repo-wide gap tracked in #4439: |
…ying forever A blocking socket only reports EAGAIN once SO_SNDTIMEO fires (kvcache-ai#4451), so retrying it here would spin the handshake daemon indefinitely and undo that fix. Mirror the writeFully handling: retry EINTR, fail the write on EAGAIN/EWOULDBLOCK.
…of dying on SIGPIPE (#4463) * [Bugfix][TransferEngine] Return EPIPE from handshake replies instead of dying on SIGPIPE The handshake daemon answers peers it did not vet. If the peer resets the connection while the daemon is mid-reply, writeString() reached a plain write() on the dead socket and the resulting SIGPIPE killed the whole process, host application included. Add writeFullySocket() next to writeFully(): same loop, but sends with MSG_NOSIGNAL on Linux and sets SO_NOSIGPIPE on the socket on macOS, so a reset peer surfaces as a plain EPIPE error. writeFully() stays as is because its remaining callers write to regular files, where send() would fail with ENOTSOCK. writeString() switches over; every one of its call sites is an accepted handshake socket. Tests cover both ends: bytes arrive intact on a healthy socketpair, and a closed peer yields -1/EPIPE with the process still alive (exit status 141 without the suppression). * [Bugfix][TransferEngine] Fail socket writes on EAGAIN instead of retrying forever A blocking socket only reports EAGAIN once SO_SNDTIMEO fires (#4451), so retrying it here would spin the handshake daemon indefinitely and undo that fix. Mirror the writeFully handling: retry EINTR, fail the write on EAGAIN/EWOULDBLOCK.
|
Merged current main (f78e3a4) now that #4513 and #4440 have landed, so the build legs pick up the Environ fix and the wheel-test legs pick up the torch pin. The red this branch sat on was those two repo-wide gaps, not the change. Rebuilt in the local docker toolchain and reran transfer_metadata_test (26 cases) against a live HTTP metadata server; green on the merged tree. |
|
Sequencing note for reviewers: #4453 (approved, CI fully green) overlaps this branch in common.h and transfer_metadata_test.cpp. To keep the conflict surface single-sided, I plan to let #4453 land first, then rebase this branch onto the new main and re-run transfer_metadata_test locally before pushing. Nothing needed from reviewers here until that rebase is done; the current head is green as it stands. |
Description
Fixes #4443.
The P2P handshake daemon serves every accepted connection inline on the single listener thread, and the response path had no send timeout:
SO_RCVTIMEOwriteFully()retries onEAGAIN, which a blocking socket only reports onceSO_SNDTIMEOfiresA peer that requests the Metadata descriptor and then never reads the response leaves the daemon's writer blocked in the kernel indefinitely. All control-plane traffic (RDMA QP setup, segment metadata exchange, notify, probe) freezes until the engine process restarts, while the process stays alive and looks healthy.
Changes:
SO_SNDTIMEO(5s, symmetric with the existingSO_RCVTIMEO) on the accepted socket instartDaemon(). A peer that keeps reading always makes progress and never trips it; only a stalled send buffer hits the timeout.writeFully(), treatEAGAIN/EWOULDBLOCKas a timed-out write and return the short count instead of spinning, mirroringreadFully(). Every current caller uses a blocking socket withoutSO_SNDTIMEO(the daemon connection, the client connection restored to blocking after the non-blocking connect indoConnect(), and test socketpairs), so none of them can observeEAGAINtoday and their behavior is unchanged.With both halves in place, a stalled response write fails within about 5s, the daemon closes that connection, and later peers are served normally.
Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
New regression test
HandshakeFrameTest.WriteToSilentPeerFailsWithinSendTimeout: a socketpair with a 4KB send buffer andSO_SNDTIMEO=1s, the peer never reads, and an 8MB Metadata frame must fail withERR_SOCKETwithin 30s instead of hanging.Socket write timed out: expected 8388608 bytes, actual 8064 bytes).writeFully(): the same test never finishes (killed at 15s), confirming it reproduces the wedge.Test commands:
cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DUSE_CUDA=OFF -DUSE_ETCD=OFF -DUSE_REDIS=OFF -DWITH_STORE=OFF -DWITH_STORE_RUST=OFF -DUSE_HTTP=ON cmake --build build --target transfer_metadata_test -j ./build/mooncake-transfer-engine/tests/transfer_metadata_testTest results:
transfer_metadata_test, including the fullHandshakeFrameTestsuite. (TheTransferMetadataTestfixture needs the etcd storage plugin, which this build config does not provide; that failure path is untouched by this change.)Checklist
./scripts/code_format.sh(via the repo's pre-commit clang-format hook)AI Assistance Disclosure
Kimi Code drafted the change; verification steps above were actually run as described, and the submitter has reviewed every changed line.