[Juhyun-Kim-Memphis]

Description

The failure mode

ClientMonitorFunc expires a client whose heartbeat deadline has passed, and expiry
is irreversible: the client's segments are unmounted and every key they held is
erased from the index. The monitor decides this from the absence of heartbeats,
which looks exactly the same whether the clients died or the master stopped serving
Ping.

So any stall in the master longer than client_ttl (default 10 s) makes it expire
every client at once and destroy the whole cluster's cache index. A transient
control-plane stall becomes permanent, cluster-wide data loss.

We hit this in production and reproduced all of it. Ping's body is a shared-lock
read of ok_client_ plus a push onto a lock-free queue, so an exclusive
client_mutex_ holder is the one thing that can stop the handler running at all —
and because coro_rpc runs a non-coroutine handler inline on the io thread, io
threads blocked on that lock stop reading pings off their connections entirely. We
measured master_ping_requests_total completely flat for nine consecutive seconds
while one such hold ran.

Why this is an operational problem, not just a rare edge case

The trigger sits on the rollout path. ReMountSegment is the only way into
ok_client_, so every store client start goes through it — a deploy, a rolling
update, a scale-out, an eviction, a node reboot, a DaemonSet update. The operations
run most often are the ones that can wipe the cluster's cache.

And the penalty is charged in exactly the currency operators are trying to save.
The loss is the cache, and re-warming a cache is far more expensive than the pod
boot that preceded it, so a fleet roll turns into a roll followed by a long cold
period. The joining node's own cache also comes online late, because its segments
are not usable until its remount returns. The rational response is to avoid
restarting store pods at all — which is the opposite of fast, frequent, safe
deploys, and it blocks the ordinary capacity work of scaling a cell out and back in.

For scale: on our rig, the metadata scan in ReMountSegment took 0.72 s at 1M
keys, 9.76 s at 14M and 13.2 s at 18M, against a 10 s client_ttl — a threshold at
roughly 14.2M keys. One of our production deployments holds 14.6M, so it sits above
the threshold in steady state; a single store pod restart there expires all of its
clients.

#3533 removed the largest scan on that path, and it helped. But the exposure is
structural rather than tied to that one scan: any exclusive client_mutex_ hold
that outlasts client_ttl reproduces the same irreversible outcome. There is also
an inconsistency between the two defaults — client_ttl is 10 s while the client's
own RPC wait is coro_rpc's 30 s default, so the master gives up on a client well
before that client gives up on the master.

What this PR adds is the property that makes the join path safe to keep optimizing:
a regression there becomes a bounded deferral instead of a cluster-wide cache loss.

What the change does

A circuit breaker in ClientMonitorFunc. When a tick's expiry candidates cannot be
distinguished from the master's own stall, it holds them back — a true no-op, with
no unmount prepared and no sweep started — bounded by
client_mass_expiry_grace_sec. Recovery needs no machinery: the next queue drain
refreshes the deadlines and the episode closes itself.

The judgement is a pure function, EvaluateClientMassExpiry, over one tick's
readings, which is what lets its boundary cases be a table-driven test with no
sleeping.

It judges the cause, not the symptom. The primary condition is that an exclusive
client_mutex_ hold overlapped [last_ping_batch_at, now], the stretch over which
this master has no heartbeat of its own to show. Every site taking that lock
exclusively records its hold window into relaxed atomics; the monitor's own expiry
branch deliberately does not, so a previous tick's expiry cannot come back as
evidence that the master was stalling.

Two things about that window are load-bearing:

• A completed hold counts only while no heartbeat has been popped since it
  released. Once one has, the master is demonstrably serving Ping again, that
  hold is refuted as an explanation, and a candidate still overdue is genuinely
  gone — so an ordinary single-client death is not deferred.
• The edge follows the heartbeat stream and not the wall clock. After a hold
  releases, the master can stay starved for many more ticks; through that no
  heartbeat is popped, so the released hold stays evidence. A window of "holds
  within the last tick or two" would age out mid-stall and erase the cache.

A secondary condition covers a total stall with no hold recorded: the master's own
silence, gated on there being at least two tracked clients — with exactly one
client, hearing nothing says only that that client stopped, which is the client's
own problem and must be acted on at once.

We tried symptom-based rules first — "too many clients expired in one tick" — and
they were defeated twice by the same thing: a deadline is stamped by the tick that
popped that client's heartbeat, so a stall's expiries spread across consecutive
ticks and each tick's share falls under any threshold. That is why the rule is
cause-based, and it is worth not simplifying back.

The behaviour change a reviewer should weigh

A genuine simultaneous death of every client is deferred up to the grace bound
before being expired. This is inherent, not an oversight: a fleet that has vanished
and a master that has totally stalled produce identical heartbeat streams — none —
so no rule can separate them. The cost is bounded and observable
(master_client_expiry_deferred_total, and a LOG(ERROR) when the grace is
exceeded), and it buys the difference between a bounded delay and an unrecoverable
cache wipe.

Two gaps stay open and are stated in the code:

1. A single-client deployment starved by something other than a client_mutex_
   hold — all io threads busy on shard locks, say — records no hold, and one tracked
   client is not silence evidence, so that deployment's keys still go. Closing it
   needs a liveness signal off the data path.
2. The whole-fleet case above.

The guard is on by default and client_mass_expiry_guard=false restores the
previous behaviour exactly.

Verification

Reproduced and gated on a 13-client deployment with the master under the same 8-CPU
and 60 GiB limits it runs with in production.

• The stall, at 18M keys. A joining client's remount held the lock for 13.8 s
  against a 10 s TTL. With the breaker: zero expiries, key count held at
  18,000,000, active clients never fell below the 12 long-lived ones, and the
  episode closed itself three ticks later when the hold released. Without it, all
  13 expire in one tick and the sweep erases the keys.
• No false positive on an ordinary death. One client killed while the others
  keep pinging expires after one TTL (10.66 s), with zero deferral ticks. Under a
  pinned window instead of last_ping_batch_at, the same case was deferred for the
  full 60 s grace — which is what motivated that edge.
• Single-client deployment, both directions. A lone death expires 1.4 s after
  SIGKILL; a genuine stall defers on hold evidence alone and preserves its keys.

Unit tests on this branch, built and run against 8715bb7c: master_service_test
80/80, master_service_config_test 9/9, the breaker's own 11 cases
(7 wall-clock, 4 pure) 11/11, and master_service_group_test 13/13, which
is the other consumer of the fixture header these tests extend. The tests were also
checked for discriminating power rather than just for passing — deleting the primary
condition fails only the stall and tick-split cases, and pinning the window's left
edge fails only the ordinary-death case.

Happy to split the breaker and its tests differently, or to trim the reasoning at
ClientMonitorFunc, if that suits review better — the comments are there mainly so
the rule is not later simplified back into a count of expiring clients, which does
not work.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Bug fix

How Has This Been Tested?

Built against 8715bb7c and run in a container with the master's production
resource limits (8 CPU, 60 GiB).

Test commands:

cmake -B build -DBUILD_UNIT_TESTS=ON
ninja -C build master_service_test master_service_config_test \
                master_service_group_test mooncake_master
./build/mooncake-store/tests/master_service_test
./build/mooncake-store/tests/master_service_config_test
./build/mooncake-store/tests/master_service_group_test
# the breaker's own cases
./build/mooncake-store/tests/master_service_test \
    --gtest_filter='*ClientMassExpiry*:*ClientExpiry*'

Test results:

• Unit tests pass — master_service_test 80/80 (207 s),
  master_service_config_test 9/9, master_service_group_test 13/13,
  breaker subset 11/11 (46 s). Zero warnings in the build.
• Integration tests pass (if applicable)
• Manual testing done — the three gates described above, on a 13-client
  deployment against a master under production limits: the 18M-key stall,
  the ordinary single-client death, and both single-client directions.

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh — clang-format
  20.1.0 reports zero replacements on every file touched
• I have run pre-commit on the files changed in this PR and all hooks pass —
  not run; happy to do so if the hooks are expected to be green before review
• I have updated the documentation (if applicable)
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue — not filed. The diff is
  1637 insertions, of which 985 are tests and the test fixture and ~660 are
  production code, a large part of that the reasoning comments at
  ClientMonitorFunc. Please say if you would rather have an RFC issue first
  and I will open one and hold this PR behind it.

AI Assistance Disclosure

• AI tools were used (specify below)

Claude Code was used throughout, under human direction: to reproduce the failure on
a rig, to implement the breaker and its tests, and to run the verification above.
Two earlier symptom-based designs it produced were rejected during review after
being shown defeatable, which is why the rule is cause-based. The human submitter
has reviewed every line and is responsible for defending the change.

[ykwd]

Thanks for identifying and raising this issue. This is indeed a serious problem that we should address.

That said, the current proposal feels more like a temporary mitigation. @Aionw will also help take a closer look at this issue so we can work toward a more robust solution.

[Juhyun-Kim-Memphis]

@ykwd thanks — agreed that this PR is a mitigation. We went after the cause and filed it as #4181, with two small PRs:

• [Store] Let Ping record a heartbeat without waiting for client_mutex_ or its own liveness record #4182 — Ping no longer waits for client_mutex_ or for its own client's liveness-record mutex (+85/−8).
• [Store] Serve Ping on a separate RPC server that clients discover on Connect #4183 — Ping can be served by a separate one-thread RPC server that clients discover on Connect (+114/−4, default off; the design of feat(store): dedicated heartbeat RPC server to prevent head-of-line blocking #3541, applied to main).

On a rig with 18M keys, a burst of 12 joining clients (running the post-promotion shard walk) made healthy clients SUSPECTED on main 30 times; with #4183 alone one went OFFLINE and 1.5M keys were erased; with both PRs there were no transitions and the worst heartbeat gap was 1.0 s. The RFC also argues from the code that, with both PRs, the heartbeat path shares no lock or thread with data-plane work, instead of relying on each lock holder staying short.

With that in place, this breaker is no longer the fix. If you still want it as a safety net, its evidence should be generalized to "no heartbeat processed for any of two or more clients in a tick", as discussed in #4181 §5. Happy to rework or close this PR, whichever you prefer. @Aionw

[Aionw]

Thanks for the explanation. I recently opened a PR #4180 that introduces a new abstraction, ClientSessionManager, to manage client liveness in one place — we may be able to use it to fix the issues found here as well.

[Juhyun-Kim-Memphis]

Closing. As noted above, this withholds the consequence of a heartbeat stall rather than removing its cause, and #4180 does not adopt it. The cause is now being addressed directly under RFC #4181: #4180 takes Ping off the data-plane locks, and #4183 takes it off the data-plane threads.