Skip to content

[Store] Scan a standby remount under the shared snapshot lock - #4291

Open
Aionw wants to merge 1 commit into
kvcache-ai:mainfrom
Aionw:fix/remount-scan-before-mount
Open

Aionw wants to merge 1 commit into
kvcache-ai:mainfrom
Aionw:fix/remount-scan-before-mount

Conversation

@Aionw

@Aionw Aionw commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Description

After an HA promotion, ReMountSegment walks all 1024 metadata shards to find the replicas that survived on a standby-restored segment, and until now it did that holding snapshot_mutex_ exclusively, from the first validation to the last replica. Every data-plane operation on the master waits for the whole walk, which is seconds on a store with a few million objects, and concurrent remounts run one after another. This is the snapshot-lock half of the join-storm stall described in #3937 and RFC #4181 (scenario S1).

What changes

Two stages. ReMountSegment now prepares under the shared lock and commits under the exclusive one:

  • PrepareRemount (shared): validates the request, scans the shards for surviving replicas and rebuilds each segment's allocator from the regions they occupy. None of this mutates anything, so it only blocks the snapshot fork.
  • CommitRemount (exclusive): mounts the segment, swaps in the rebuilt allocator, re-points the replicas by key and ends the orphan. Its critical section is bounded by the number of replicas on the segment, not by the size of the store.

Splitting the operation lets the scan's findings go stale, which the commit stage handles:

  • A replica can disappear between the stages, so the scan records its key rather than a Replica*; the commit stage looks the key up again and re-matches the replica against the region of the buffer imported for it. A buffer nobody claims is dropped while the exclusive lock still covers the allocator its region returns to.
  • An HA restore can rewrite the state the scan read. A generation counter, advanced by every restore, is compared across the gap; a remount that loses the race returns UNAVAILABLE_IN_CURRENT_STATUS and the client retries.
  • The allocator a mount creates treats the whole segment as free, so the mount and the swap to the rebuilt allocator happen inside one segment-manager critical section.

OrphanedSegments (include/ha/orphaned_segments.h). What an HA restore leaves behind for such segments — the segment list, the bytes still charged to them, the allocators keeping their buffers alive and which endpoints are unreadable — was four MasterService members keyed three different ways, stitched together by callers. It is now one record per segment, and the interface follows the segment's lifecycle: Reset on restore, Find while preparing a remount, Adopt when committing it, IsUnreadable for readers, ReleaseAll on shutdown. Reset is the only way to install orphans and always advances the generation, so the check the commit stage depends on cannot be skipped by a future restore path.

Behavior change: allocated-memory metrics. Adopting a segment now releases everything still charged to it. Before, only the bytes of the replicas the remount found were released, so the bytes of replicas that died while their segment was orphaned stayed on that segment's allocated-memory metric until the next restore or shutdown, and the leftover entry sent later remounts of the segment through the full scan again. The re-adopted allocator already counts the surviving replicas itself (OffsetBufferAllocator::Restore, CachelibBufferAllocator::adoptImportedBuffer).

Smaller changes:

  • The placeholder allocators a restore creates for NoF replicas are kept in their own member instead of the memory-segment keepalive map, since no memory remount adopts them.
  • The scan no longer copies the buffer descriptor of every memory replica in the store, only of those on the segments being re-adopted.
  • The restore log line reports unreadable_segments=<n> (a segment count) instead of invalid_endpoints=<n> (an alias count).

Relation to open work

Size

+586/−315 lines outside tests. Most of the master_service.cpp churn is the existing ReMountSegment body moving into the stage helpers (net +271 in that file); the design context is in RFC #4181 and in this description. It merges cleanly with main at e88aacf.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake Conductor (mooncake-conductor)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

make -j14 master_service_ha_test master_service_test orphaned_segments_test
./mooncake-store/tests/master_service_ha_test   # 101 tests passed
./mooncake-store/tests/master_service_test      # 60 tests passed
./mooncake-store/tests/orphaned_segments_test   # 9 tests passed
pre-commit run --files <files changed in this PR>

New tests:

  • orphaned_segments_test: the class's invariants — every Reset advances the generation (identical contents included), Find rejects a request that contradicts its orphan or names two, unmounted orphans are unreadable by endpoint and by name, Adopt ends the orphan, releases its allocator and returns its whole charge, Reset returns the charges it discards.
  • MasterServiceHATest.RemountReleasesTheChargeOfReplicasThatDiedOrphaned: restores two replicas, erases one before the remount, and checks the segment's allocated-memory metric ends at the survivor's size. It would read 2048 instead of 1024 under the old accounting.

Not covered: no test drives the two stages across the lock gap (a replica erased between them, or a restore landing between them). Those paths are exercised only by review.

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • 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

Pre-commit note: cmake-format rewraps dynamic_replication_lease_table_test in mooncake-store/tests/CMakeLists.txt, a line that is already on main and not touched here; that rewrap is left out. Every other hook passes. No separate RFC was filed; see Size above.

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Claude Code wrote the implementation and tests under the submitter's direction; the submitter decided the design at each step (splitting the stages, keeping test seams out of production code, the shape of OrphanedSegments, and taking the metrics behavior change). OpenAI Codex ran independent review passes over the diff.

Re-adopting a standby-restored segment held snapshot_mutex_ exclusively
from the first validation to the last replica, so a remount stopped every
operation on the master for as long as it took to walk all 1024 metadata
shards. With a store of a few million objects that walk is seconds, and
concurrent remounts ran one after another because each one held the lock
for its whole duration.

Nothing in the walk mutates anything: it reads each shard to find the
replicas that live on the segment, and rebuilds the segment's allocator
from the regions they occupy. ReMountSegment now prepares under the
shared lock, where it blocks only the snapshot fork, and commits under
the exclusive one. The commit stage publishes the segment and re-points
the replicas the scan found by looking their keys up directly, so its
critical section is bounded by the replica count on the segment rather
than by the size of the store.

Splitting the operation means the scan's findings can go stale:

- A replica can disappear before the commit stage reaches it. The scan
  therefore records the key rather than a Replica*, and the commit stage
  looks the key up again and re-matches the replica against the region
  its imported buffer covers. A buffer nobody claims is dropped, which
  returns its region to the allocator that has just taken ownership of
  it.
- An HA restore can rewrite the standby state the scan read. A
  generation counter, advanced by every restore, is compared across the
  gap; a remount that loses the race refuses and the client retries.

The allocator the mount creates covers the whole segment as free space,
so it must not be visible to an allocation before it is replaced by the
rebuilt one. The mount and the swap happen in one segment-manager
critical section inside the exclusive stage.

What an HA restore leaves behind for such segments -- the segment list,
the bytes still charged to them, the allocators keeping their buffers
alive and which endpoints are unreadable -- was four MasterService
members keyed three different ways. It becomes OrphanedSegments, one
record per segment, with an interface that follows the segment's
lifecycle: Reset on restore, Find while preparing a remount, Adopt when
committing it, IsUnreadable for readers and ReleaseAll on shutdown. Reset
is the only way to install orphans and always advances the generation,
so the check the commit stage relies on cannot be bypassed. The
placeholder allocators a restore creates for NoF replicas are kept apart
from it, since no remount adopts them.

Adopting a segment now releases everything still charged to it. Before,
only the bytes of the replicas the remount found were released: the
bytes of replicas that died while the segment was orphaned stayed on the
segment's allocated-memory metric until the next restore or shutdown,
and the leftover entry sent later remounts of the segment through the
full scan again. The re-adopted allocator already counts the surviving
replicas itself.

The scan no longer copies the buffer descriptor of every memory replica
in the store, only of those on the segments being re-adopted.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant