Repository navigation
[Store] Serve Ping on a separate RPC server that clients discover on Connect - #4183
Closed
Juhyun-Kim-Memphis wants to merge 2 commits into
Closed
Juhyun-Kim-Memphis wants to merge 2 commits into
Juhyun-Kim-Memphis wants to merge 2 commits into
Conversation
…Connect A non-coroutine RPC handler runs on the io thread that read it. When every main io thread is inside a long handler -- a synchronous ClearInvalidHandles after an unmount, a large batch operation -- a Ping waiting on any of those connections is not even read, so a live client stops being observed and the master moves it to SUSPECTED and then OFFLINE. No lock is involved; the heartbeat simply shares its threads with the data plane. Add --heartbeat_rpc_port (default 0, non-HA mode). When set, the master starts a second coro_rpc_server with one io thread that serves only Ping and ServiceReady, and advertises its port through a new GetHeartbeatRpcPort RPC. MasterClient asks for that port on Connect and sends Ping there once the server answers ServiceReady. Rollout needs no coordination: the main server keeps serving Ping, so a client that predates this change, a master that does not advertise a port, or an advertised server that is unreachable all leave Ping on the main server. This is the design of kvcache-ai#3541 on the P2P branch, applied to main's Ping, with the port discovered from the master instead of configured on every client.
This was referenced Sep 17, 2026
… is idle Raising rpc_thread_num does not keep a Ping from waiting. coro_rpc binds each accepted connection to one io thread, round-robin, and runs a non-coroutine handler inline on that thread, so a Ping is read only by the thread that owns its connection, however many others are idle. The new test runs the main server with two io threads, holds the thread of one connection, and checks that of two Pings sent on the other two connections exactly one is answered until the hold ends, and both after.
This was referenced Sep 21, 2026
Merged
Juhyun-Kim-Memphis
added a commit
to vessl-ai/Mooncake
that referenced
this pull request
Sep 23, 2026
… (default off) Merges upstream kvcache-ai#4183 at head 0386371 ("[Store] Serve Ping on a separate RPC server that clients discover on Connect"), two commits: 3593169 and 0386371. The master gains --heartbeat_rpc_port (default 0 = off) and a GetHeartbeatRpcPort RPC; a client probes it on Connect and sends Ping to the separate server when the master advertises one. The flag stays off in this fork: without upstream kvcache-ai#4180, Ping still takes client_mutex_, so a single heartbeat thread would stall every client at once. Upstream: kvcache-ai#4183
Juhyun-Kim-Memphis
added a commit
to vessl-ai/Mooncake
that referenced
this pull request
Sep 23, 2026
… (default off) (#21) [INF-522] Carry upstream kvcache-ai#4183: separate heartbeat RPC server (default off)
8 of 32 tasks
9 of 32 tasks
12 of 32 tasks
10 of 11 tasks
Contributor
Author
|
Closing as superseded by #4601, which landed on 10-16 and takes the worker-pool alternative from §5 of RFC #4181: every non- |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Pingcan be served by a separate RPC server whose only thread never runs a data-plane handler, and clients find it by themselves. This is the thread half of RFC #4181. The lock half is #4180, which removesclient_mutex_and the guards from thePingpath but does not touch RPC threading. Production code: +114/−4 in six files. Default off.Depends on #4180. With a single heartbeat thread, any lock wait inside
Pingstalls every heartbeat at once. Without the lock half, one healthy client wentOFFLINEin the RFC's join-storm run. Do not enable--heartbeat_rpc_porton a master without #4180.Why
coro_rpc gives each io thread its own
io_context, binds an accepted connection to one of them for its lifetime, and runs non-coroutine handlers inline on the thread that read the request. APingis therefore read only by the io thread that owns its connection. If that one thread is inside a long handler, thePingwaits for it, even when every other io thread is idle, so raisingrpc_thread_numdoes not remove the wait, and removing locks fromPingcannot help because nothing is waiting on a lock.On
main,UnmountSegmentruns its O(keys)ClearInvalidHandlessweep inline when HA, snapshot or CXL is enabled. On a non-HA rig with--enable_snapshot, 18M keys and 12 concurrent unmounts, healthy clients' heartbeats stopped for 19.8 s onmainand the master marked 6 of themSUSPECTED. EveryPingthat took longer than 2 s completed within 8 ms of anUnmountSegmentreturning. With this PR the worst gap was 1.0 s and nothing was suspected. A run with only the lock half (#4182, now closed in favour of #4180) showed 29.1 s; the lock half does not act here, and the spread most likely comes from which io thread a heartbeat's connection happened to share with the deepest unmount queue. Each figure is a single run (RFC #4181 §4).What changes
--heartbeat_rpc_port(default 0; non-HA mode) starts a secondcoro_rpc_serverwith one io thread that serves onlyPingandServiceReady. Eachcoro_rpc_serverowns its ownio_context_pool, so the two servers share no io thread. A newGetHeartbeatRpcPortRPC on the main server returns the port (0 when disabled). In HA mode the flag logs a warning and is ignored.MasterClient::Connectasks for the port, probesServiceReadyon it, and then sendsPingthere. The heartbeat address gets its own connection pool, so aPingalso no longer shares connections with the client's own data-plane RPCs. Other RPCs are unchanged.Scope
snapshot_mutex_, which is where a heartbeat server would matter most; HA support (the supervisor's server lifecycle) is the follow-up listed in the RFC.Pinghandler on a worker pool instead; this PR will follow that answer.Rollout needs no coordination
The main server keeps serving
Ping. A client that predates this change, a master that does not advertise a port, and an advertised server that does not answer all leavePingon the main server. This is the design of #3541 on the P2P branch applied tomain'sPing, with the port discovered from the master instead of configured on every client, which removes the client/master mismatch cases #3541 had to add error codes for. (A new client connecting to an older master logs oneRPC call failedline perConnectfor the unknown RPC, then behaves as before.) A deployment that remaps ports between the client and the master (for example a Kubernetes Service whoseportdiffers from itstargetPort) must expose the heartbeat port under the same number; otherwise clients stay on the main server.Module
mooncake-store)Type of Change
How Has This Been Tested?
Integration test (new)
heartbeat_rpc_server_test, with real servers and a realMasterClient:PingIsServedWhileTheMainIoThreadIsBusy: with the heartbeat server,Pingreturns within 2 s while the main server's only io thread is held by a blocking handler.PingWaitsForTheMainIoThreadWithoutTheServer: the control. Without it,Pingdoes not return within 2 s.PingFallsBackToTheMainServerWhenUnreachable: an advertised port nobody listens on leavesPingon the main server.PingWaitsForABusyIoThreadEvenWhenAnotherIsIdle: the main server has two io threads and one of them is held. Of two connections opened before the hold, thePingon the one that shares the held thread does not return within 2 s, while the other does. Raisingrpc_thread_numleaves the wait in place for the connections on a busy thread.Results: the three original tests 3/3, repeated 5× with no flake;
master_service_test58/58 (Release, GCC 13, Ubuntu 24.04). The new test: a standalone copy with the same fixture and the pinned yalantinglibs passes 50/50, and the same assertion fails with one or three io threads, so it does check the placement. On the real target it passes in this PR's CI (CTest unit tests).Rig. Measurements for both dimensions, alone and together, are in RFC #4181 §4.
Checklist
./scripts/code_format.sh(clang-format 20, changed lines)AI Assistance Disclosure
Claude (Anthropic) helped analyze the threading, write the change and its tests, and run the verification. The submitter reviewed every changed line.