[GH-ISSUE #4418] Deadlock risk between snapshot restore and segment mount (segment_mutex_ / client_mutex_ ordering) #1114

New issue

Open

opened 2026-10-29 21:23:02 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-29 21:23:02 +00:00

Owner

Originally created by @quantz8a on GitHub (Sep 30, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4418

Summary

MasterService::ApplySnapshotState() reverses the lock order used by MountSegment() / UnmountSegment(), which can deadlock when snapshot restore races with a client mount or unmount.

Affected path

ApplySnapshotState() obtains an exclusive lock on SegmentManager::segment_mutex_ via segment_manager_.getSegmentAccess() (mooncake-store/src/master_service.cpp:11232).
While still holding that lock it calls:
- UnmountSegment() (:11238), and
- Ping(client_id) (:11248).
Both of those paths acquire client_mutex_:
- Ping() does std::shared_lock<std::shared_mutex> lock(client_mutex_) (:8405).
- UnmountSegment() does std::shared_lock<std::shared_mutex> client_lock(client_mutex_) (:3127) before re-acquiring segment_mutex_ (:3133).

Reverse order in the other callers

MountSegment() first takes client_mutex_ exclusively (:900) and then, inside its liveness callback, takes snapshot_mutex_ and segment_mutex_ via segment_manager_.getSegmentAccess() (:909-:913).
UnmountSegment() takes client_mutex_ before segment_mutex_ as shown above.

So the code has both:

segment_mutex_ (unique) -> client_mutex_ (shared) in ApplySnapshotState().
client_mutex_ (unique) -> segment_mutex_ (unique) in MountSegment().

Deadlock scenario

Thread A is in ApplySnapshotState(): it holds segment_mutex_ exclusively and blocks trying to take client_mutex_ (via Ping).
Thread B is in MountSegment(): it holds client_mutex_ exclusively and blocks trying to take segment_mutex_ (via getSegmentAccess()).

A and B wait on each other, forming a classic ABBA deadlock. The same inconsistent ordering exists for the snapshot-restore unmount path.

Suggested fix

Do not call UnmountSegment() or Ping() while holding the snapshot-restore segment_mutex_ hold. Collect the unready/all segment lists under segment_mutex_, release the lock, then unmount/ping/metric-update outside the lock. See the accompanying PR.

/cc @kvcache-ai/mooncake-core

Originally created by @quantz8a on GitHub (Sep 30, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4418 ## Summary `MasterService::ApplySnapshotState()` reverses the lock order used by `MountSegment()` / `UnmountSegment()`, which can deadlock when snapshot restore races with a client mount or unmount. ## Affected path - `ApplySnapshotState()` obtains an exclusive lock on `SegmentManager::segment_mutex_` via `segment_manager_.getSegmentAccess()` (`mooncake-store/src/master_service.cpp:11232`). - While still holding that lock it calls: - `UnmountSegment()` (`:11238`), and - `Ping(client_id)` (`:11248`). - Both of those paths acquire `client_mutex_`: - `Ping()` does `std::shared_lock<std::shared_mutex> lock(client_mutex_)` (`:8405`). - `UnmountSegment()` does `std::shared_lock<std::shared_mutex> client_lock(client_mutex_)` (`:3127`) before re-acquiring `segment_mutex_` (`:3133`). ## Reverse order in the other callers - `MountSegment()` first takes `client_mutex_` exclusively (`:900`) and then, inside its liveness callback, takes `snapshot_mutex_` and `segment_mutex_` via `segment_manager_.getSegmentAccess()` (`:909-:913`). - `UnmountSegment()` takes `client_mutex_` before `segment_mutex_` as shown above. So the code has both: 1. `segment_mutex_ (unique) -> client_mutex_ (shared)` in `ApplySnapshotState()`. 2. `client_mutex_ (unique) -> segment_mutex_ (unique)` in `MountSegment()`. ## Deadlock scenario - Thread A is in `ApplySnapshotState()`: it holds `segment_mutex_` exclusively and blocks trying to take `client_mutex_` (via `Ping`). - Thread B is in `MountSegment()`: it holds `client_mutex_` exclusively and blocks trying to take `segment_mutex_` (via `getSegmentAccess()`). A and B wait on each other, forming a classic ABBA deadlock. The same inconsistent ordering exists for the snapshot-restore unmount path. ## Suggested fix Do not call `UnmountSegment()` or `Ping()` while holding the snapshot-restore `segment_mutex_` hold. Collect the unready/all segment lists under `segment_mutex_`, release the lock, then unmount/ping/metric-update outside the lock. See the accompanying PR. /cc @kvcache-ai/mooncake-core

bsakaguchi commented

2026-10-29 21:23:02 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Sep 30, 2026):

Thanks for opening this issue, @quantz8a!

Field	Value
Issue	#4418
GitHub user ID	`3152505`
Reporter	@quantz8a

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Sep 30, 2026): Thanks for opening this issue, @quantz8a! | Field | Value | |-------|-------| | **Issue** | #4418 | | **GitHub user ID** | `3152505` | | **Reporter** | @quantz8a | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:23:02 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 2, 2026):

Verified both deadlock faces on current main (ApplySnapshotState at master_service.cpp:11142):

Self-deadlock on the unready path. The function holds ScopedSegmentAccess (unique on segment_mutex_) and calls UnmountSegment for each unready segment; UnmountSegment itself enters ScopedSegmentAccess at :3133. std::shared_mutex is non-recursive, so a second unique lock on the same thread is UB and blocks in practice. Any restore that actually has unready segments wedges the master right there.
ABBA on the ping path. Still holding segment_mutex_, it calls Ping, which takes client_mutex_ shared (:8405). MountSegment takes client_mutex_ exclusive first (:900) and then enters getSegmentAccess() in its liveness callback. Restore racing a client mount has each side waiting on the other.

So this is two distinct defects in one function, and the collect-then-act shape the issue suggests covers both.

One overlap worth settling before review effort gets duplicated: #4022 (Sep 11) already implements the same collect-under-lock, act-outside restructure for the unready path, with a test, and has been sitting without comments for three weeks. #4419 is the minimal version of the same idea. Same root cause, same fix shape; one of them should absorb the other. There is also prior art in #3152 (closed unmerged), which attacked the adjacent snapshot_mutex_ ordering in this same function, in case the ordering argument there is still useful.

Context for prioritization: my open #3806 rewrites the repair side of this restore path. Whichever of #4022/#4419 lands, I will rebase and adapt #3806 to the new locking, so merging one of them does not strand the tolerance work.

@he-yufeng commented on GitHub (Oct 2, 2026): Verified both deadlock faces on current main (`ApplySnapshotState` at master_service.cpp:11142): 1. Self-deadlock on the unready path. The function holds `ScopedSegmentAccess` (unique on `segment_mutex_`) and calls `UnmountSegment` for each unready segment; `UnmountSegment` itself enters `ScopedSegmentAccess` at :3133. `std::shared_mutex` is non-recursive, so a second unique lock on the same thread is UB and blocks in practice. Any restore that actually has unready segments wedges the master right there. 2. ABBA on the ping path. Still holding `segment_mutex_`, it calls `Ping`, which takes `client_mutex_` shared (:8405). `MountSegment` takes `client_mutex_` exclusive first (:900) and then enters `getSegmentAccess()` in its liveness callback. Restore racing a client mount has each side waiting on the other. So this is two distinct defects in one function, and the collect-then-act shape the issue suggests covers both. One overlap worth settling before review effort gets duplicated: #4022 (Sep 11) already implements the same collect-under-lock, act-outside restructure for the unready path, with a test, and has been sitting without comments for three weeks. #4419 is the minimal version of the same idea. Same root cause, same fix shape; one of them should absorb the other. There is also prior art in #3152 (closed unmerged), which attacked the adjacent `snapshot_mutex_` ordering in this same function, in case the ordering argument there is still useful. Context for prioritization: my open #3806 rewrites the repair side of this restore path. Whichever of #4022/#4419 lands, I will rebase and adapt #3806 to the new locking, so merging one of them does not strand the tolerance work.

bsakaguchi referenced this issue

2026-10-29 21:32:10 +00:00

[PR #1114] [MERGED] change cmake #2020

bsakaguchi referenced this issue

2026-10-29 21:33:38 +00:00

[PR #1370] [CLOSED] [TE] feat: zmq communicator based on coro_rpc #2204