Repository navigation
[Bugfix][TransferEngine] Return EPIPE from handshake replies instead of dying on SIGPIPE - #4463
Conversation
…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).
|
The two red legs are the repo-wide gap tracked in #4439, not this PR: |
|
[P1] writeFullySocket retries EAGAIN indefinitely. With #4451's SO_SNDTIMEO, a peer that stops reading still traps the handshake daemon in this loop, undoing that fix. Preserve EAGAIN/EWOULDBLOCK timeout handling here and run the stalled-peer regression with both changes applied. |
…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.
|
Good catch, done in c14bf0b. Ran the stalled-peer regression on a tree with both changes applied (macOS can't build the engine natively, so this was a standalone harness over |
Description
Fixes #4454.
The handshake daemon answers peers it has not vetted. When a peer resets the connection while the daemon is mid-reply,
writeString()ended in a plainwrite()on the dead socket and the resulting SIGPIPE killed the whole process, host application included.This adds
writeFullySocket()next towriteFully(): same retry loop, but the bytes go out viasend()withMSG_NOSIGNALon Linux, and withSO_NOSIGPIPEset on the socket on macOS (which has noMSG_NOSIGNAL). A reset peer now surfaces as a plainEPIPEerror return, and the daemon logs and moves on.writeFully()is unchanged because its remaining callers write to regular files, wheresend()would fail withENOTSOCK.writeString()switches over; every one of its call sites is an accepted handshake socket intransfer_metadata_plugin.cpp.Two tests pin the behavior on a
socketpair: bytes arrive intact on a healthy connection, and a closed peer yields-1/EPIPEwith the process still alive. Without the suppression the second test never reaches its assert, because the process dies from SIGPIPE (exit status 141, reproduced locally with the same write pattern against a plainwrite()).Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
The full CTest target needs Linux (libnuma, ibverbs), so locally I compiled
mooncake-transfer-engine/tests/common_test.cppdirectly against gtest on macOS arm64 and ran the suite. The Linux CTest run is covered by CI.Test commands:
c++ -std=gnu++20 -arch arm64 tests/common_test.cpp -lglog -lgtest -lgtest_main ./common_test ./common_test --gtest_filter='WriteFullySocketTest.*'Test results:
WriteFullySocketTestcases pass)EPIPEand the process survives)Checklist
./scripts/code_format.shAI Assistance Disclosure
Prepared with AI assistance (Kimi Code): it drafted the patch and the tests. I reviewed every changed line and ran the verification described above myself.