[KubrickLiu]

Description

ReMountSegment unconditionally scans all kNumShards metadata shards under
exclusive client_mutex_/snapshot_mutex_ locks to reconcile
DummyBufferAllocator-backed placeholder replicas left over from standby
snapshot restoration. This scan is only meaningful when the segment being
remounted still has unreconciled replicas tracked in
standby_accounted_memory_bytes_ (i.e. it was restored via
RestoreFromStandby after a leader promotion and hasn't been reclaimed by a
real client remount yet).

For segments that never went through standby restoration — most notably a
newly mounted segment during scale-up, whose first ReMountSegment call is
triggered by Ping() returning NEED_REMOUNT right after MountSegment() — the
scan still runs anyway and walks every tenant/key/replica across the full
metadata store while holding locks that PutStart/BatchPutStart also need,
causing them to stall for minutes under load.

This PR guards the scan with any_standby_kept_alive, short-circuiting the
loop whenever none of the segments being remounted has an entry in
standby_accounted_memory_bytes_. Behavior for the necessary case (post
leader-promotion reconciliation) is unchanged.

Fixes #3517

Module

• Transfer Engine (mooncake-transfer-engine)
• Mooncake Store (mooncake-store)
• 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

[he-yufeng]

Verified the gate against the two places standby_accounted_memory_bytes_ is written, and the skip is sound:

• The map is only ever populated by the wholesale reassignment in the standby snapshot restore (same pass that creates the DummyBufferAllocator placeholder replicas and fills standby_allocator_keepalive_). So a placeholder for segment X can only exist when X is in the map.
• Entries are erased only at the end of the success path, after replace_memory_buffer has converted that segment's placeholders into real buffers. Every fail_remount returns before the erase, so an erased name means no placeholders remain for it.

With those two directions closed, "no incoming segment in the map" really does imply "no placeholder can match a restore", and skipping the shard scan cannot leave a placeholder unreconciled. A partial two-batch remount also behaves: the batch carrying the kept-alive segment still scans.

Commenting rather than approving only because this is the HA restore path and I have no local build here; the invariant above is what I checked, and it holds.

[KubrickLiu]

Thanks yufeng @he-yufeng, appreciate you verifying the invariant ~

@Icedcoco @lxy-alexander could you also take a look when you have a chance ~~

[Icedcoco]

LGTM pending CI

[lxy-alexander]

Sorry for the late reply @KubrickLiu . The client still calls ReMountSegment() when Ping() returns NEED_REMOUNT. However, this RPC is also used for newly added P nodes, so calling it does not necessarily mean that standby recovery is required. I think the implementation should distinguish a fresh segment registration from an actual remount after Master failover. Maybe we need re-define the client status later. @Icedcoco @he-yufeng

[KubrickLiu]

Agreed @lxy-alexander, that's the real root cause — conflating fresh registration with post-failover remount under one status. I'd suggest keeping this PR scoped to the #3517 fix for now, and tracking the ClientStatus redesign as a follow-up.

[Aionw]

Sorry for the late reply @KubrickLiu . The client still calls ReMountSegment() when Ping() returns NEED_REMOUNT. However, this RPC is also used for newly added P nodes, so calling it does not necessarily mean that standby recovery is required. I think the implementation should distinguish a fresh segment registration from an actual remount after Master failover. Maybe we need re-define the client status later. @Icedcoco @he-yufeng

Would you mind opening a new issue to track this, @KubrickLiu @lxy-alexander?

From what I've seen so far, it would be much cheaper and architecturally cleaner to send segment status updates (e.g., stale, remount, new) instead.