[RFC]: [Store] The client heartbeat path must share no lock or thread with data-plane operations

### Changes proposed

**Make the client heartbeat path share no contended resource with data-plane operations.** Today a `Ping` waits for `client_mutex_` (held exclusively by every client-state writer), for the calling client's liveness-record mutex (held for the whole lifetime of a serving/retaining guard), and for a free RPC io thread (shared with every long handler). Any of the three lets a slow control-plane operation stop a live client from being observed, and the master then moves healthy clients to `SUSPECTED` and, after the suspicion TTL, to `OFFLINE` — which offboards their segments and erases the keys on them. This RFC proposes two small, independent PRs that remove the lock dependency (PR-A, +85/−8 production lines) and the thread dependency (PR-B, +114/−4 production lines), and states the invariant they establish so that future changes cannot silently reintroduce it.

> **Invariant.** The liveness path — from a `Ping` arriving at the master until its observation is recorded, and the monitor deciding a transition — never waits for a resource that a data-plane operation can hold for a data-dependent time.

### 1. Why this needs a structural fix

In production, one store pod restarting on a master holding 13.9M keys made every one of the 12 live clients expire at once and erased the index from 13.9M keys to 0.43M. We reproduced the chain on a rig and described it in #3937: the joining client's `ReMountSegment` walked all metadata shards under exclusive `client_mutex_`, no `Ping` could be served, and the monitor expired everyone. The trigger — a store client (re)joining — is on the rollout path, so the operations run most often (deploy, roll, scale-out, eviction, node reboot) are the ones that can wipe the cache.

#3936 proposed a circuit breaker for the resulting mass expiry. As @ykwd noted there, that is a mitigation: it withholds the consequence and leaves the cause. #2991 (RFC #2953) made the consequence recoverable — a heartbeat gap now yields `SUSPECTED` before `OFFLINE` — but the cause is unchanged, and the number of places that can trigger it has grown:

| exclusive `client_mutex_` holder on `main` (d9e4dcbc) | note |
| -- | -- |
| `MountSegment` | exclusive since #2991 (previously did not take the lock) |
| `ReMountSegment` | includes the 1024-shard walk when standby-restored memory is kept alive (HA promotion) |
| `ProcessClientOffboardingJob` | new in #2991 |
| `MountLocalDiskSegment` | |
| `RestoreFromStandbyState` | whole restore |
| `ClientMonitorFunc` retirement, `UpdateClientHostId`, `ResetStateAfterFailedRestoreAttempt`, `RebuildClientLivenessAfterSnapshotRestore` | |

and `Ping` now records the heartbeat itself inside that lock (`record->Observe()` under `client_mutex_` shared), where it previously pushed onto a lock-free queue.

How the invariant was lost, in four steps that were each reasonable on their own:
- #501 introduced `ok_client_`, the status lookup in `Ping` and exclusive `client_mutex_` in `ReMountSegment` — HA-only, bounded work.
- #845 removed the HA gate, so every deployment runs this path.
- #2826 put the standby validation and the full shard walk inside that exclusive section.
- #2991 moved heartbeat recording under the lock and added exclusive holders.

Nothing in the code says "`client_mutex_` is on the heartbeat path, so keep its critical sections short", so no review could catch any of these. Narrowing `ReMountSegment`'s lock would restore that promise once; this RFC removes the need for the promise.

### 2. Design

**PR-A — lock dimension (#4182).**
1. Every exclusive `client_mutex_` section that changes `client_liveness_records_` or `ok_client_` publishes an immutable `ClientView` of the two on scope exit (an RAII helper declared right after the lock, so it runs before unlock on every return path). `Ping` reads that view with `std::atomic_load_explicit` — the same facility `replica.h` and `allocator.h` already use for liveness records — instead of taking `client_mutex_`. Writers are unchanged.
2. `ClientLivenessRecord::ObserveHeartbeat` publishes the heartbeat time atomically and only *try*-locks `transition_mutex_`. If a guard holds it, `Evaluate` still counts the heartbeat (an `ACTIVE` client is not suspected; a `SUSPECTED` client does not go `OFFLINE`), and the next heartbeat that gets the mutex recovers the state exactly as `Observe` does. Only `Ping` uses it; mount/register paths keep `ObserveAndRun`.

Semantics are preserved:
- The status `Ping` returns was already stale by the time it reached the client; `view_version_` in the same response is read without the lock today.
- A client not yet in the view gets `NEED_REMOUNT` once and calls the idempotent `ReMountSegment` it already retries every ping; the view is published before `ReMountSegment` returns, so this only happens to a Ping that raced the remount.
- A heartbeat that lands on a record the view no longer contains is lost, which the next ping (1 s later) repairs; an `OFFLINE` record still rejects it.
- Guard invariants are untouched: `ObserveHeartbeat` never changes state without `transition_mutex_`, and it only ever moves `SUSPECTED` to `ACTIVE`.

**PR-B — thread dimension (#4183).** A non-coroutine handler runs on the io thread that read it, so when every main io thread is inside a long handler a `Ping` is not even read. `--heartbeat_rpc_port` (default 0, non-HA) starts a second `coro_rpc_server` with one io thread that serves only `Ping` and `ServiceReady`, advertised through a new `GetHeartbeatRpcPort` RPC; `MasterClient::Connect` discovers it and sends `Ping` there once it answers. The main server keeps serving `Ping`, so an old client, an old master, or an unreachable heartbeat server all fall back to today's behaviour with no coordination. This is the design of #3541 on the P2P branch, applied to `main`'s `Ping`, with the port discovered rather than configured on every client (which removes the mismatch failure modes #3541 had to add error codes for).

### 3. Proof that the invariant holds (with both PRs)

A test can only show that a failure did not occur; it cannot show that it cannot. So the claim is argued from the code, and each step is pinned by a test.

**3.1 Locks on the heartbeat path.** Every acquisition reachable from `WrappedMasterService::Ping`:

| # | acquisition on the heartbeat path (PR-A + PR-B) | kind | can a data-plane operation make it wait? |
| -- | -- | -- | -- |
| 1 | `client_mutex_` | — | **not taken** (was: shared, behind every exclusive holder listed in §1) |
| 2 | `snapshot_mutex_`, metadata shard locks, segment manager lock | — | not taken (never were) |
| 3 | `ClientLivenessRecord::transition_mutex_` | `try_lock` only | **never waits** (was: blocking; held by `ServingGuard`/`RetainingGuard` for the guarded operation, and by `ReMountSegment` for the whole remount) |
| 4 | `std::atomic_load_explicit(&client_view_)` | libstdc++ shared_ptr lock pool | no: every critical section in that pool is a single `shared_ptr` copy or swap (`client_view_`, `replica.h`, `allocator.h`), and none takes another lock inside it |
| 5 | `latest_heartbeat_rep_` | `std::atomic` CAS | no lock |
| 6 | `MasterMetricManager::inc_ping_requests`, `client_liveness_recovered` | ylt `counter_t`/`gauge_t` | no lock: `thread_local_value::inc/dec` are `fetch_add`/`fetch_sub` on a per-thread `std::atomic` slot (`ylt/metric/thread_local_value.hpp`) |
| 7 | glog, only when a line is written (`VLOG(1)` per Ping when `-v>=1`; `LOG(INFO)` on recovery) | glog internal mutex + stderr write | only through blocking log I/O, which stalls every thread in the process and is outside the scope of this invariant |

Everything else `Ping` touches is immutable after publication (`ClientView`) or immutable after construction (`view_version_`).

**3.2 Threads.** | server | io threads | handlers registered |
| -- | -- | -- |
| main `coro_rpc_server` | `rpc_thread_num` (its own `io_context_pool`) | every master RPC, including `Ping` for clients that do not use the heartbeat server |
| heartbeat `coro_rpc_server` (`--heartbeat_rpc_port`) | 1 (its own `io_context_pool`) | `Ping`, `ServiceReady` only |

Each `coro_rpc_server` constructs its own `io_context_pool` from its `thread_num` (`coro_rpc_server.hpp`: member `pool_`, `pool_(thread_num)`, `acceptor->set_io_threads_pool(&pool_)`), and `io_context_pool::run()` starts one `std::thread` per `io_context`, so the two servers share no io thread. A `Ping` on the heartbeat server is therefore read and executed by a thread that never runs a data-plane handler, and by 3.1 it never waits for one either. `ServiceReady` returns a constant string.

**3.3 The monitor.** `ClientMonitorFunc` captures `now` before it takes anything, and every wait after that (`client_mutex_` shared to copy the record list, a record's `transition_mutex_` inside `EvaluateAndRetire`) only delays a decision taken against that earlier `now`. A transition therefore requires `now - max(last_liveness_at, latest_heartbeat_at) >= ttl`, i.e. that no heartbeat was recorded for the client within `ttl` before `now`. By 3.1 and 3.2, recording a received heartbeat never waits for the data plane. The remaining causes are outside the master's data plane: the client, the network, CPU starvation of the whole process, and blocking log I/O.

### 4. Measurement

Rig: one master (8 RPC threads, 7-CPU / 40 GiB cgroup), 18M keys, 12 clients pinging every second on their own threads, 16 load threads running `BatchExistKey`, default TTLs (active 10 s, suspicion 20 s). The same benchmark binary is used for every variant; against a master that does not advertise a heartbeat server it behaves as an unmodified client. Numbers are for the 12 clients that were healthy throughout ("originals"), during the load phase.

- **S1 — join storm.** 12 store clients join at once. Every remount runs the post-promotion shard walk; a rig-only environment variable forces the `any_standby_kept_alive` branch so this HA path can be exercised without a failover (not part of either PR).
- **S2 — unmount storm.** 12 clients unmount at once on a master with `--enable_snapshot`, so `UnmountSegment` runs `ClearInvalidHandles` synchronously on the RPC thread (as it does with HA, snapshot or CXL enabled).
- **S3 — a client really dies** (both PRs). Checks that detection is unchanged.

| scenario | variant | worst heartbeat gap (originals) | originals → SUSPECTED | originals → OFFLINE | keys at end |
| -- | -- | -- | -- | -- | -- |
| S2 unmount storm | main | 19.8 s | 6 | 0 | 18M |
| | PR-A only | 29.1 s | 12 | 0 | 18M |
| | PR-B only | 1.0 s | 0 | 0 | 18M |
| | PR-A + PR-B | 1.0 s | 0 | 0 | 18M |
| S1 join storm | main | 213.6 s | 30 | 0 | 18M |
| | PR-A only | 91.0 s | 28 | 0 | 18M |
| | PR-B only | 196.7 s | 14 | **1** | **16.5M** |
| | PR-A + PR-B | 1.0 s | 0 | 0 | 18M |
| S3 dead client | PR-A + PR-B | — | the dead client only, 9.9 s after its last ping | the dead client only, 30.0 s after | its keys only |

What the table shows:
- **Neither PR is sufficient alone, and together they remove every false transition.** PR-A alone changes nothing measurable: a heartbeat that no thread reads cannot be recorded. PR-B alone fixes the thread-only case (S2) but not S1, where its single heartbeat thread waits for `client_mutex_` behind each remount; one healthy client went `OFFLINE` and 1.5M keys were erased.
- **#2991's recovery is doing real work on `main`**, which is why S1 on `main` shows dozens of `SUSPECTED` transitions but no data loss in this run. Each of those transitions still removes a healthy client from every serving path for the time it is suspected, and the margin to `OFFLINE` is only the suspicion TTL (S1-B crossed it).
- Detection of a client that really died is unchanged (S3).

### 5. Alternatives considered
- **Narrow `ReMountSegment`'s exclusive section.** Fixes one holder; the other six remain, and the next holder repeats the regression (§1).
- **Circuit breaker (#3936).** Withholds the consequence of a stall the master cannot rule out as its own; the cause remains, and the breaker's evidence has to be taught every new kind of stall. With this RFC it becomes an optional safety net whose evidence can be generalized to "no heartbeat processed for any of ≥2 clients in a tick".
- **Make long handlers coroutines on a worker pool.** Correct in principle, but requires enumerating every long handler and misses the next one; a heartbeat server needs no enumeration.
- **Only PR-B.** The heartbeat thread still waits for `client_mutex_` and for guarded records; S1 shows a healthy client going `OFFLINE` and its keys erased (§4).
- **Only PR-A.** A heartbeat that no thread reads cannot be recorded; S1 and S2 are unchanged (§4).

### 6. Out of scope / follow-ups
- HA mode for `--heartbeat_rpc_port` (the supervisor's server lifecycle).
- Clients need no configuration: the heartbeat port is discovered on `Connect`.

### 7. Questions for maintainers
1. Is a second RPC port acceptable for non-HA masters, or would you rather serve `Ping` from a dedicated executor inside the existing server? The invariant only needs the heartbeat to run on threads no data-plane handler can occupy.
2. Should the heartbeat server be on by default in a later release, once HA support lands?
3. With both PRs merged, do you still want the breaker from #3936 as a safety net, with its evidence generalized as in §5?

### AI assistance disclosure
Claude (Anthropic) helped analyze the code paths, write both PRs and their tests, drive the rig measurements and draft this RFC. The submitter reviewed every changed line and the measurement data.

### Before submitting a new issue...

- [x] Make sure you already searched for relevant issues and read the [documentation](https://kvcache-ai.github.io/Mooncake/)

Start at WrappedMasterService::Ping and ClientLivenessRecord, then read the related server setup in coro_rpc_server.hpp and the liveness patterns in replica.h and allocator.h. Review PRs #4182 and #4183 and their tests; done means the heartbeat path does not wait on data-plane locks or threads, while real client failures still transition normally.

Written by the indexing model from the issue text.