[GH-ISSUE #4459] [RFC][Store]: Settle segment.name cardinality and consolidate derived-index ownership in segment lifecycle #1126

New issue

Closed

opened 2026-10-29 21:23:09 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

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

Owner

Originally created by @LinyuWang on GitHub (Oct 3, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4459

[RFC][Store]: Settle `segment.name` cardinality and consolidate derived-index ownership in segment lifecycle

Summary

The segment lifecycle code in mooncake-store/src/segment.cpp maintains several derived indexes over mounted_segments_ (client_segments_, client_by_name_, segment_id_by_name_, segments_by_host_, plus AllocatorManager). These indexes currently encode two contradictory assumptions about the cardinality of segment.name:

name is a unique key → client_by_name_ and segment_id_by_name_ are name → single value maps (include/segment.h:481-483)
name is a grouping key → AllocatorManager (name → list of allocators) and segments_by_host_ (name → set<UUID>, include/segment.h:23) are 1:N

The write path never enforces either assumption, and the delete paths compensate locally with conditional logic. This RFC proposes to settle the cardinality question and give the derived indexes a single point of ownership.

Problem

1. Duplicate names are accepted, but not consistently supported

On the client side, every MountSegmentAndGetId() call generates a fresh random segment.id, while segment.name = local_hostname_ is constant per client (client_service.cpp:3529-3530). A client mounting N non-overlapping buffers therefore produces N segments with the same name and different ids. Master-side dedup checks only segment.id (segment.cpp:228), so all of them mount successfully.

Consequences:

client_by_name_[name] = client_id and segment_id_by_name_[name] = segment.id are silently overwritten (last-wins) (segment.cpp:277-278).
IsSegmentAllocatable(name) / GetSegmentStatusByName(name) answer for the latest mounted id only (segment.cpp:1090-1115).
Consumers of a name disagree about what the name denotes.

2. Order-dependent erase semantics in `CommitUnmountSegment`

if (segment_id_by_name_it->second == segment_id) {
    segment_manager_->segment_id_by_name_.erase(segment_id_by_name_it);
    segment_manager_->client_by_name_.erase(segment_name);
}

(segment.cpp:503-510)

The == segment_id guard protects the remount-vs-late-unmount race: Prepare and Commit run under separate lock scopes, so a same-name remount (new id) can interleave between them, and the guard keeps the stale Commit from stealing the new registration's entries. However:

It conflates two questions: "does the name still point at the segment being removed?" (id-level) with "should we forget this name's owner?" (name-level). With more than one segment per name, unmounting the latest-mounted segment first erases client_by_name_[name] while other same-name segments are still alive and allocatable: ExistsSegmentName returns false, GetOwnerClientId returns nullopt (SSD free-ratio ranking silently degrades to the default), and IsSegmentAllocatable returns false for a healthy segment. Unmounting in the opposite order happens to stay correct — i.e. the name-level view depends on unmount order.
There is no survivor fallback: when the guarded entry is erased, the id of a surviving same-name segment is not restored to segment_id_by_name_.
The fix is asymmetric across paths: the memory path has this guard; the NoF path does not — the same race there is reported in #4211.

3. Redundant, idempotency-based index maintenance

RemoveHostSegment is called from PrepareUnmountSegment, PrepareGracefulUnmountSegment, CommitUnmountSegment, and the NoF unmount path; AddHostSegment from four mount/remount/restore paths. Every new lifecycle path must remember to keep five derived structures mutually consistent, and the safety net is idempotent re-execution rather than single ownership. This is the class of debt that produced both #4211 and the asymmetry above.

Proposed directions (pick one)

A. Admit 1:N. Change client_by_name_ / segment_id_by_name_ to name → set<UUID>; Commit erases the id and cascades client_by_name_ only when the set becomes empty; consumers adopt any-OK semantics. This matches what AllocatorManager and segments_by_host_ already do, and supports multi-NUMA / mixed-media mounts by one client.

B. Enforce uniqueness. Reject a second same-name mount (return SEGMENT_ALREADY_EXISTS, or require explicitly distinct names such as ip:port#tag); erase paths become unconditional; document segment.name as a unique key.

Either direction removes the conditional erase, the circular id→name→id lookup, and the unmount-order dependence.

Relation to existing work

#4211 — the same late-unmount race on the NoF path; an instance of the problem this RFC addresses structurally.
#3360 (SegmentPool refactor) — its MountedRegionCatalog plans "client, name, host, and status indexes"; the name-cardinality decision is a prerequisite design input for that catalog, and the catalog is the natural home for single-ownership index maintenance.
#3560 (distinguish NEW/REMOUNT/STALE registration intent) — complementary; explicit per-segment lifecycle intent interacts with what name uniqueness must mean during remount.

Discussion questions

Is "one client mounts multiple same-name segments" an intended, supported case (multi-NUMA, DRAM+SSD tiering), or an artifact of the client API?
If intended → direction A; if not → direction B. Which behavior do existing deployments rely on?
Should this be folded into #3360 as an explicit design requirement for MountedRegionCatalog, or tracked independently?

Before submitting a new issue...

Make sure you already searched for relevant issues and read the documentation

Originally created by @LinyuWang on GitHub (Oct 3, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4459 # [RFC][Store]: Settle `segment.name` cardinality and consolidate derived-index ownership in segment lifecycle ## Summary The segment lifecycle code in `mooncake-store/src/segment.cpp` maintains several derived indexes over `mounted_segments_` (`client_segments_`, `client_by_name_`, `segment_id_by_name_`, `segments_by_host_`, plus `AllocatorManager`). These indexes currently encode **two contradictory assumptions** about the cardinality of `segment.name`: - **name is a unique key** → `client_by_name_` and `segment_id_by_name_` are `name → single value` maps (`include/segment.h:481-483`) - **name is a grouping key** → `AllocatorManager` (name → list of allocators) and `segments_by_host_` (name → `set<UUID>`, `include/segment.h:23`) are 1:N The write path never enforces either assumption, and the delete paths compensate locally with conditional logic. This RFC proposes to settle the cardinality question and give the derived indexes a single point of ownership. ## Problem ### 1. Duplicate names are accepted, but not consistently supported On the client side, every `MountSegmentAndGetId()` call generates a fresh random `segment.id`, while `segment.name = local_hostname_` is constant per client (`client_service.cpp:3529-3530`). A client mounting N non-overlapping buffers therefore produces N segments with the same name and different ids. Master-side dedup checks only `segment.id` (`segment.cpp:228`), so all of them mount successfully. Consequences: - `client_by_name_[name] = client_id` and `segment_id_by_name_[name] = segment.id` are silently overwritten (last-wins) (`segment.cpp:277-278`). - `IsSegmentAllocatable(name)` / `GetSegmentStatusByName(name)` answer for the *latest mounted* id only (`segment.cpp:1090-1115`). - Consumers of a name disagree about what the name denotes. ### 2. Order-dependent erase semantics in `CommitUnmountSegment` ```cpp if (segment_id_by_name_it->second == segment_id) { segment_manager_->segment_id_by_name_.erase(segment_id_by_name_it); segment_manager_->client_by_name_.erase(segment_name); } ``` (`segment.cpp:503-510`) The `== segment_id` guard protects the remount-vs-late-unmount race: `Prepare` and `Commit` run under separate lock scopes, so a same-name remount (new id) can interleave between them, and the guard keeps the stale Commit from stealing the new registration's entries. However: - **It conflates two questions**: "does the name still point at the segment being removed?" (id-level) with "should we forget this name's owner?" (name-level). With more than one segment per name, unmounting the *latest-mounted* segment first erases `client_by_name_[name]` **while other same-name segments are still alive and allocatable**: `ExistsSegmentName` returns false, `GetOwnerClientId` returns nullopt (SSD free-ratio ranking silently degrades to the default), and `IsSegmentAllocatable` returns false for a healthy segment. Unmounting in the opposite order happens to stay correct — i.e. the name-level view depends on unmount order. - **There is no survivor fallback**: when the guarded entry is erased, the id of a surviving same-name segment is not restored to `segment_id_by_name_`. - **The fix is asymmetric across paths**: the memory path has this guard; the NoF path does not — the same race there is reported in #4211. ### 3. Redundant, idempotency-based index maintenance `RemoveHostSegment` is called from `PrepareUnmountSegment`, `PrepareGracefulUnmountSegment`, `CommitUnmountSegment`, and the NoF unmount path; `AddHostSegment` from four mount/remount/restore paths. Every new lifecycle path must remember to keep five derived structures mutually consistent, and the safety net is idempotent re-execution rather than single ownership. This is the class of debt that produced both #4211 and the asymmetry above. ## Proposed directions (pick one) **A. Admit 1:N.** Change `client_by_name_` / `segment_id_by_name_` to `name → set<UUID>`; Commit erases the id and cascades `client_by_name_` only when the set becomes empty; consumers adopt any-OK semantics. This matches what `AllocatorManager` and `segments_by_host_` already do, and supports multi-NUMA / mixed-media mounts by one client. **B. Enforce uniqueness.** Reject a second same-name mount (return `SEGMENT_ALREADY_EXISTS`, or require explicitly distinct names such as `ip:port#tag`); erase paths become unconditional; document `segment.name` as a unique key. Either direction removes the conditional erase, the circular id→name→id lookup, and the unmount-order dependence. ## Relation to existing work - **#4211** — the same late-unmount race on the NoF path; an instance of the problem this RFC addresses structurally. - **#3360** (SegmentPool refactor) — its `MountedRegionCatalog` plans "client, name, host, and status indexes"; the name-cardinality decision is a prerequisite design input for that catalog, and the catalog is the natural home for single-ownership index maintenance. - **#3560** (distinguish NEW/REMOUNT/STALE registration intent) — complementary; explicit per-segment lifecycle intent interacts with what name uniqueness must mean during remount. ## Discussion questions 1. Is "one client mounts multiple same-name segments" an intended, supported case (multi-NUMA, DRAM+SSD tiering), or an artifact of the client API? 2. If intended → direction A; if not → direction B. Which behavior do existing deployments rely on? 3. Should this be folded into #3360 as an explicit design requirement for `MountedRegionCatalog`, or tracked independently? ### Before submitting a new issue... - [x] Make sure you already searched for relevant issues and read the [documentation](https://kvcache-ai.github.io/Mooncake/)

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

@github-actions[bot] commented on GitHub (Oct 3, 2026):

Thanks for opening this issue, @LinyuWang!

Field	Value
Issue	#4459
GitHub user ID	`10942216`
Reporter	@LinyuWang

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 (Oct 3, 2026): Thanks for opening this issue, @LinyuWang! | Field | Value | |-------|-------| | **Issue** | #4459 | | **GitHub user ID** | `10942216` | | **Reporter** | @LinyuWang | 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 referenced this issue

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

[PR #1126] [CLOSED] add heterogeneous-tcp #2026