[Store] Drain Client TE paths before tearDown (#3909) - #3969
Conversation
Teardown of a read-side client after consecutive RPC timeouts released the client pool while request coroutines were still suspended in it, and ylt only documents send_request as thread-safe, so the resumed coroutine touched freed state and segfaulted in asio's epoll_reactor (kvcache-ai#3909). Two layers, both verified against current main: - A shared RpcDrainGuard (mooncake-common) stops admitting new calls once draining and makes the destructor wait for in-flight ones, with a 30s bound that logs loudly rather than silently proceeding. Wired into MasterClient, DummyClient and ClientRequester entry points. - RpcClientPool now hands out pools from a process-wide registry keyed by address instead of freeing them on teardown or address flaps. ylt pools own background reconnect coroutines that reference pool storage, so no amount of request draining makes pool destruction safe mid-retry; pools are one-per-master-address and deliberately process-lifetime. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
MasterClient rpc_drain alone still UAF when Get is in TransferRead during close. Add Client::api_drain_ and RealClient::client_op_drain_ so tearDown waits for in-flight API/TE work before freeing TE.
8e75a63 to
033524c
Compare
Review acceleratorCI: format failure was a single extra blank line after Suggested review order (keeps wall-clock down):
cc @he-yufeng (stacked on your #3943) · reviewers already requested: @ykwd @stmatengss @XucSh @YiXR — happy to address comments quickly. |
Co-authored-by: Cursor <cursoragent@cursor.com>
CTest red on prior tip — not a unit-test regressionJob annotation on run
Failed step was Build project and unit tests (~7.5m into Pushed empty |
Co-authored-by: Cursor <cursoragent@cursor.com>
Gentle review nudgeTip Still OPEN · MERGEABLE · no review yet (~1.5d). Would appreciate a pass when you have a slot:
cc @ykwd @stmatengss @XucSh @YiXR @he-yufeng — happy to address comments same day. |
|
Thanks for putting this follow-up together. I understand that #3969 is intended to be stacked on top of #3943, and that its first commit currently contains the #3943 changes because the official stacked-PR workflow was not used. After looking through the incremental changes in #3969, I noticed two points that seem specific to this PR and may be worth addressing:
The pool-registry lifetime and configuration questions seem more naturally tied to the base change in #3943. I plan to wait for #3943 to be reviewed and merged first, then review #3969 again against that final base with a more detailed pass. Thanks again for working on this; the additional TE-side drain appears useful, and I would be happy to continue the review once these points are clarified. |
Resolve MasterClient conflicts: keep RpcDrainGuard alongside HA control/probe pools; drain at invoke_rpc_with_client_pool entry; keep teardown drain test and main HA timeout tests.
…uard batch APIs On RealClient tearDown, fail with RPC_TIMEOUT and keep TE/Client if client_op or API drain times out instead of logging UAF risk and continuing. Client dtor waits until idle after a timed-out drain. Wrap BatchGet / BatchPut / BatchGetOffloadObject with api_drain_ ScopedCall. Add RpcDrainGuard regression that a timed-out drain keeps blocking new calls.
|
@Icedcoco Follow-up on your two points is in tip
Happy to take a detailed pass when you have a slot — #3943 can still land independently; this tip is already mergeable vs current main. |
3 participants
Summary
Follow-up to #3943 for Mooncake #3909 (short
MC_RPC_TIMEOUT_MS+ concurrentget×close→ asio SIGSEGV / exit 139).rpc_drain_) + process-wide pool registryTransferReadafter Master returnsTip commit (review focus)
Client::api_drain_+DrainInflightOperations();ScopedCallon Get/Put pathsRealClient::client_op_drain_around fullget_buffer(incl. observe)tearDownAll_internal: drain client ops →DrainInflightOperations→ then unregister/reset TEBranch = rebased
#3943+ TE-drain + clang-format fix. Prefer review with/after #3943; happy to squash/retarget once it lands.Test plan
repro_mooncake_3909.py/MC_RPC_TIMEOUT_MS=20: unpatched & [Store] Drain in-flight RPCs and keep pools alive across client teardown #3943-only → 139; +TE drain → 0 (15 cycles)real_client.cpp)Refs: #3909 · #3943