kvcache-ai / kvcache-ai/Mooncake

[Bug][Store]: Lease-expired client cleanup blocks all master RPC for minutes under HA + SSD Offload

Open
#3,940 4 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
6.6k
Forks
1.2k
Avg merge
3d 5h
Merged PRs (30d)
312

Description

### Bug Report

**Summary**

When a store's lease expires (client process stalls but doesn't crash — misses pings until lease TTL), the master's `ClientMonitorThreadFunc` synchronously runs `ClearInvalidHandles` → `ClearStaleHandles`: an O(total keys) full sweep of all shards, holding each shard's write lock. Because the sweep visits **every** key (not just the expired client's), cleaning a client that owns a large replica set (tens of millions, a fraction of total keys) takes minutes. During that time **all** RPCs — including `Ping` and `FetchTasks` — are blocked (`Total=0.00` in admin metrics), so the master appears hung even though the process is alive and `view_version` is unchanged.

> Reproduced on a deployment with SSD Offload enabled and **HA on, OpLog off**. So the bottleneck below is **not** OpLog/etcd throughput — it holds with pure in-memory deletion.

**Symptoms (observed in production)**

- ~7-minute "unresponsive" window (10:24:25 → 10:31:20).
- Admin metrics keep printing every 10s, `role=leader`, `view_version` unchanged (no HA re-election) → the master process is alive.
- During the window `PutStart/Get/Ping/FetchTasks` all show `Total=0.00`; `Clients` 8→7→8 (self-recovered at 10:31:46).
- `Mem Storage` pinned at 4.41 TB / 5.47 TB (80.7%) — unchanged, because the cleanup targets `local_disk`/SSD replicas, not memory.
- `SSD Storage` dropped 5.32 TB (37.47 → 32.15 TB); `Keys` dropped ~33.7M (234.1M → 200.5M).
- All requests from the inference engine (sglang) were blocked — get/put throughput was 0 for 7 minutes.
- All admin-metric counters (`Eviction/Discard/Promotion/Snapshot`) stayed 0 / unchanged, because `ClearStaleHandles` doesn't update any of them — making the work nearly invisible during diagnosis.

**Root cause analysis** — three compounding factors:

1. **The lease-expire path is unconditionally synchronous.** `ClientMonitorThreadFunc` calls `ClearInvalidHandles(alive_clients)` directly ([`master_service.cpp:11576`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L11576)), unlike the normal unmount path which can go async via `replica_cleanup_worker_.Schedule()` ([`master_service.cpp:2858`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L2858)). The lease-expire path doesn't even consult `enable_async_segment_cleanup_`.

2. **`ClearStaleHandles` holds each shard's write lock for the entire shard sweep** ([`master_service.cpp:2729-2798`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L2729)). `MetadataShardAccessorRW shard(this, i)` scopes the whole shard loop; `PutStart`/`Get`/`Ping` all need a read lock on the shard → all blocked for the sweep duration.

3. **`ClearStaleHandles` is O(total keys), not O(stale keys).** The sweep iterates **every** key in **every** shard/tenant and calls `BuildStaleHandleCleanupPlan` on each ([`master_service.cpp:2738`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L2738)), which walks that key's replicas and runs `is_stale` — and `has_stale_local_disk_client` does an `alive_clients.find()` hash lookup per replica ([`replica.h:465`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/include/replica.h#L465)). So even though only ~33.7M of 234M keys were stale, **all 234M keys are visited and judged**. Stale keys then go through `EraseMetadata` ([`master_service.cpp:2252`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L2252)), which per key does several hash lookups/erasures (offloading_tasks, processing_keys, replication_tasks, soft-pin index). The whole pass is a large, cache-miss-bound in-memory traversal. This is independent of OpLog: with `enable_oplog_=false` the deletion path is pure in-memory `EraseMetadata` (no etcd write), yet the sweep still took ~7 min. OpLog, when enabled, adds a per-replica `Reserve`+`Commit` on top ([`master_service.cpp:2205-2226`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/master_service.cpp#L2205)); `Commit` is non-blocking but `Reserve` returns `TASK_PENDING_LIMIT_EXCEEDED` at 1024 in-flight slots ([`ordered_oplog_writer.cpp:189-191`](https://github.com/kvcache-ai/Mooncake/blob/main/mooncake-store/src/ha/oplog/ordered_oplog_writer.cpp#L189)), which can only make things slower.

**Proposed approaches**

- **A:** Make `ClearStaleHandles` release the shard write lock periodically (time-sliced, e.g. every ~50ms) and resume from a key cursor (`lower_bound(last_key)`). While the lock is released, other RPCs may insert, delete, or modify keys in that shard — on resume, those keys are re-judged against `is_stale`: newly inserted keys don't belong to the expired client and are skipped, already-deleted keys are skipped by `lower_bound`, so releasing the lock mid-sweep is consistent. When OpLog is enabled, `ClearStaleHandles` only enqueues the OpLog entry + `mark_removed` and the actual `EraseMetadata` runs in the durable callback, which likewise supports releasing the lock mid-sweep. This keeps the master responsive regardless of cleanup volume — the "hung" symptom is fixed even if cleanup still takes minutes.
- **B:** Maintain a client→replica reverse index so lease-expire cleanup scans only that client's replicas (O(its replicas) vs O(total keys)). This directly attacks factor 3 — turning a 234M-key sweep into a 33.7M-key one. Larger change; suggest as a follow-up.
- **C (only when OpLog is enabled):** Increase `max_entries_per_batch` (1024 → larger) and/or batch `ClearStaleHandles`' OpLog submissions. Doesn't apply when OpLog is off (as in this report); when on, combine with A.

**Discussion**

- Is there a correctness reason `enable_async_segment_cleanup_` is disabled under HA, or is it conservative? If it can be lifted, the lease-expire path could use `replica_cleanup_worker_` directly.
- Which approach is preferred? Or if there's a better one, please share.
- Any concerns with the time-sliced lock release in `ClearStaleHandles` (cursor validity, intermediate consistency)?

### Before submitting...

- [ ] Ensure you searched for relevant issues and read the [documentation]

Contributor guide

Open the contributing guide

Research direction

Start with ClientMonitorThreadFunc and ClearStaleHandles in mooncake-store/src/master_service.cpp, then read BuildStaleHandleCleanupPlan, EraseMetadata, and has_stale_local_disk_client in replica.h. Compare the lease-expire path at master_service.cpp:11576 with the normal unmount cleanup at :2858 and examine the locking around :2729-2798. Done means lease-expiry cleanup no longer blocks Ping, FetchTasks, PutStart, or Get for minutes while stale handles are removed.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, distributed-systems, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.