Repository navigation
[Bugfix][TransferEngine] Bound the handshake daemon's pending-notify queue - #4453
Conversation
…queue The handshake daemon accepts notify frames from any peer that can reach the RPC port, and receivePeerNotify appended every one of them to an unbounded in-memory vector. A trivial flood grows the host's RSS without limit (kvcache-ai#4445). Cap the queue at MC_HANDSHAKE_MAX_NOTIFY_ENTRIES (default 1024, max 1M) and evict the oldest undrained entry once full, so a flood degrades pending notifications instead of the process. Warn at most once per 1000 evictions to keep the flood from turning into log spam. Adds a socket-level regression test that floods cap + 64 frames and verifies the oldest 64 are evicted while the survivors keep arrival order. Fixes kvcache-ai#4445 Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
The flood loop's bare read() can return after the frame header alone. Closing then resets the socket while the daemon is still writing the payload, and its next write() kills the test process with SIGPIPE, as seen on the CTest leg. Drain the frame with readString instead, which also asserts the daemon answered with a Notify reply. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
CTest leg is green again after 74f04e7 (43m run, pass), so the SIGPIPE kill is confirmed fixed in CI as well. The two remaining red legs are infrastructure, not this change:
Nothing in this PR touches the wheel build path; the change is confined to the handshake daemon test and the bounded notify queue. |
|
Unfreezing this now that both blockers have landed: #4440 pins the wheel-test torch (the remaining test-wheel red) and #4513 restored the build. Merged current main (cd049ca); the earlier red legs traced to those two repo-wide gaps, not this 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. Fresh CI is running. |
|
Everything is green on the merged head now, including the four tent-ci variants and both wheel test legs. The transfer_metadata suite also passed locally against a live HTTP metadata server after the merge. @alogfans with your approval this can go in. One sequencing note: #4451 touches the same two files (common.h and transfer_metadata_test.cpp), so once this lands I will rebase #4451 onto the new main and re-run its suite before asking for review there. |
Description
The handshake daemon accepts notify frames from any peer that can reach the RPC port, and
TransferMetadata::receivePeerNotifyappended every one of them to an unbounded in-memory vector. The PoC in #4445 grows the process RSS from 50 MB to 2.6 GB in 20 seconds.This caps the pending-notify queue at
MC_HANDSHAKE_MAX_NOTIFY_ENTRIES(default 1024, accepted range 1 to 1M, invalid values fall back to the default with a warning). Once the queue is full, the oldest undrained entry is evicted to make room for the new one: under a flood you lose old pending notifications instead of the whole process. Evictions log at most once per 1000 to keep the flood from turning into log spam.Evict-oldest rather than reject-new because under an actual flood the recent signals are the useful ones, and a drained queue behaves identically either way.
Fixes #4445
Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
Added
HandshakeNotifyTest.BoundedQueueEvictsOldestUnderFloodintransfer_metadata_test.cpp. It stands up a realTransferMetadataserver in P2P handshake mode on a loopback port, writes cap + 64 notify frames over real sockets (reading each reply to keep frame ordering deterministic), then drains withgetNotifiesand asserts exactly cap entries survive, the oldest 64 were evicted, and the survivors keep arrival order.Local host is macOS, so the full CTest binary was not built here. Verified locally:
clang++ -std=c++20 -fsyntax-onlyon bothtransfer_metadata.cppand the test file (gtest include tree plus a shim for the glibc-onlybits/stdint-uintn.h)./scripts/code_format.sh --staged --checkclean with clang-format 20.1.8pre-commit run --fileson all three touched files, all hooks passThe new test relies on CI for the actual run; the eviction path it exercises is the same code path the flood PoC in #4445 hits.
Test results:
Checklist
./scripts/code_format.shAI Assistance Disclosure
Prepared with AI assistance (Kimi Code). The submitter reviewed every changed line and can defend the change end to end. Verification actually run is listed above: source-level syntax checks, clang-format on the changed lines, and pre-commit; the gtest binary itself was not executed locally.