[XFDG]

Description

SelectBestReplica documents the priority local MEMORY > local NOF_SSD > remote MEMORY > remote NOF_SSD, but it previously returned the first complete local MEMORY or NOF replica encountered. As a result, [local NOF, local MEMORY] selected NOF while the reverse ordering selected MEMORY.

This PR makes the documented priority independent of the order returned by the master.

Change

The selector scans all complete MEMORY and NOF candidates before deciding, records local and remote candidates separately, and then applies the existing priority. Other replica tiers and remote-replica scoring behavior remain unchanged.

Regression coverage

• LocalMemoryBeatsLocalNoFInBothOrders: local MEMORY wins in both permutations.
• IncompleteLocalMemoryFallsBackToLocalNoF: an unreadable local MEMORY candidate does not hide a usable local NOF replica.
• LocalNoFPrecedesRemoteMemory: locality still outranks a remote MEMORY candidate.

The final two guard cases were adapted from 2sumtech/Mooncake@67bfff5d, as coordinated in #3742.

Scope coordination

• [Bugfix][Store] Support DFS replicas in ranged get sessions #3903 has merged MEMORY/DFS ranged-session support. This PR no longer changes batch_get_session_start, session replica selection, or ranged-session execution.
• Maintainer guidance in [Bugfix][Store] Keep scanning for a local MEMORY replica past a local NoF one #3742 selected [Bugfix][Store] Make local MEMORY preference independent of replica order #3662 as the canonical implementation of the ordering fix and requested moving [Bugfix][Store] Keep scanning for a local MEMORY replica past a local NoF one #3742's guard coverage here.
• [Bug]: batch_get_session_start fails with 702 INVALID_REPLICA when SSD offloading is enabled #3658 remains open for file-backed/SSD session behavior; this PR does not claim to fix it.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Bug fix

How Has This Been Tested?

The focused regression tests are included in replica_selection_test.cpp. No additional local test run was performed after the final scope reduction; the refreshed upstream CI run validates the current head.

AI Assistance Disclosure

• AI tools were used; all final changes were reviewed against the documented replica-priority contract and maintainer coordination.

[XFDG]

@fcczzz @bitborne Fixed both issues:

1. Priority ordering bug (bitborne): SelectBestReplica now scans all replicas before returning. Previously the first local replica (MEMORY or NOF) caused an early return, so [local NOF, local MEMORY] would incorrectly select NOF. The fix collects local_memory and local_nof in one pass, then returns local_memory first — local MEMORY always beats local NOF regardless of list order.

2. NOF replica_limit=0 bug (fcczzz): batch_get_into_multi_buffer_ranges now reads the NOF buffer size for overflow detection instead of hardcoding 0. Both MEMORY and NOF replicas support range reads via RDMA, so the limit should come from their respective buffer descriptors.

[catyans]

Thanks @fcczzz and @bitborne for digging into the two selection issues, and @XFDG for fixing the order-dependent MEMORY/NoF choice. That part looks good now.

I think the session-read concern is still only halfway resolved, though. batch_get_session_start() can now cache a NoF replica, and the next step correctly derives its size, but Client::BatchTransferReadRanges() still rejects every non-MEMORY replica at client_service.cpp:4334. So a NoF-only object gets past the new size check and then fails the actual transfer; LOCAL_DISK/DFS/DISK selections still fail even earlier with a zero replica_limit.

Could we either add the NoF range-transfer path end to end, or keep session selection restricted to the replica types that BatchTransferReadRanges() actually supports? A NoF-only start -> ranged read -> end regression would make the intended fix especially clear. The two guard cases offered in #3742 would also be worth carrying over.

[XFDG]

Thanks @catyans — you're right, and I confirmed it end to end. batch_get_session_start was selecting via SelectBestReplica, which can return a NOF_SSD replica, but the session range-read path is memory-only: Client::BatchTransferReadRanges (client_service.cpp:4334) rejects every non-MEMORY entry. So a NOF-only object passed the new size check and then failed the actual transfer, and a LOCAL_DISK/DFS/DISK selection failed even earlier with a zero replica_limit. The comment claiming "both support range reads via RDMA" was simply wrong.

I took the second option you offered — keep session selection restricted to the replica types BatchTransferReadRanges() actually supports — rather than building an end-to-end NOF range-transfer path, which is a much larger transport-layer change and out of scope for this bugfix.

Fixed in 2c009ca:

• Session path is MEMORY-only again. Moved SelectCompleteMemoryReplica out of the real_client.cpp anonymous namespace into replica_selection.h (next to SelectBestReplica) so it's unit-testable, and batch_get_session_start now uses it. It returns only a complete MEMORY replica (local preferred), or nullptr — in which case the session is declined with INVALID_REPLICA at start, instead of caching a NOF/disk replica that fails later. That also closes the LOCAL_DISK/DFS/DISK case you noted.
• Single-object get path is unchanged — it keeps SelectBestReplica, which correctly routes NOF/DISK/DFS through Client::Get.
• The [Bug]: batch_get_session_start fails with 702 INVALID_REPLICA when SSD offloading is enabled #3658 fix is preserved. SelectCompleteMemoryReplica scans all replicas and prefers a local MEMORY replica, so local MEMORY still beats a later remote MEMORY regardless of master return order. It's orthogonal to the memory-vs-NOF ordering fix in SelectBestReplica.
• Removed the dead NOF branch in batch_get_into_multi_buffer_ranges and corrected its comment.

New regression SessionPathRejectsNoFOnlyObject pins exactly the halves you asked for: a NOF-only object is selected by SelectBestReplica (single-object path) but declined by SelectCompleteMemoryReplica (session path), and a local MEMORY replica still wins when both MEMORY and NOF are present.

One note on the #3742 guard cases: those tighten SelectBestReplica's local-NOF-vs-remote scan, which now only affects the single-object path (the session path no longer touches NOF). Happy to carry them over here once #3742 lands, or leave them to #3742 — whichever you prefer.

[XFDG]

CI note (not a code change): the one red leg — test-wheel-ubuntu (ubuntu-22.04, 3.12) → "Run SSD offload and promotion end-to-end tests" → test_concurrent_stress (test_ssd_offload_in_evict.py:431, AssertionError: 108 != 0 : 108 concurrent read(s) failed) — is a pre-existing flake, not a regression from 2c009ca.

Evidence:

• The ubuntu-24.04, 3.10 leg of the same commit passes; only ubuntu-22.04, 3.12 fails. That OS/matrix-leg split, independent of branch content, is the flake signature.
• The identical test/line/assertion fails on several unrelated branches that don't touch replica selection or session-start at all (e.g. feat/hp-tcp-read-slicing, codex/tent-unknown-rpc-error, codex/tensor-from-multibuffer-pr), with only the failure count varying (108 / 140 / 640 / 748 / 1860) — nondeterministic concurrency timing under eviction pressure.
• test_concurrent_stress exercises the single-object store.get() path (SelectBestReplica), which this PR leaves unchanged; it never calls batch_get_session_*. The session-path change in 2c009ca is out of its code path.

All logic-relevant checks are green: CTest unit tests, Check code format, and every build-wheel (cuda / cu13 / rocm / efa). A re-run of the flaky leg should clear the gate.

[catyans]

Thanks for tracing this all the way through. LGTM.

Restricting session start to complete MEMORY replicas now matches what BatchTransferReadRanges() can actually execute, so a NoF-only object is rejected up front instead of failing halfway through the session. The new selector still preserves the order-independent local-MEMORY preference that @fcczzz and @bitborne called out, and the focused test captures both sides of that contract nicely.

The remaining Ubuntu 22.04 failure is in the known SSD concurrent-stress path and doesn't exercise this session-selection change, so I don't see it as a blocker for this fix.

[fcczzz]

The current MEMORY-only session selection matches BatchTransferReadRanges, and the order-independent local MEMORY preference looks correct.

However, the session selector and start/read/end behavior are unchanged from the merge base, so SSD-only objects still fail session start with INVALID_REPLICA. Please update the title and description, remove “Fixes #3658”, and coordinate the overlapping priority fix with #3742.

The latest CI run also failed.

[catyans]

Good catch, @fcczzz. I agree the current title/body overstate what this head does.

My approval was for the safety correction—keeping the session path MEMORY-only so it cannot cache a replica that BatchTransferReadRanges() will reject—and for the separate order-independent local MEMORY preference in SelectBestReplica. But the session selector itself remains MEMORY-only, so an SSD-only object still returns INVALID_REPLICA; this does not deliver the end-to-end SSD session support requested by #3658.

@XFDG, could you retitle/rewrite the PR around the actual replica-priority fix, remove Fixes #3658, and coordinate the overlapping SelectBestReplica change with #3742? The code can still be useful, but the issue should stay open until the ranged session read supports SSD/NoF end to end (or the API contract is explicitly changed). Thanks for flagging the distinction.

[XFDG]

Thanks @fcczzz and @catyans for the precise review.

Done in this update:

1. Title and description rewritten to reflect the actual scope: the PR now describes "restrict batch_get_session_start to MEMORY replicas + order-independent local preference", and explicitly states that SSD-only objects still return INVALID_REPLICA (full SSD session support is out of scope, tracked in [Bug]: batch_get_session_start fails with 702 INVALID_REPLICA when SSD offloading is enabled #3658 / [Bugfix][Store] Keep scanning for a local MEMORY replica past a local NoF one #3742).

2. Fixes #3658 removed from the PR body.

The code itself is unchanged — @catyans's approval covers the safety correction (MEMORY-only session start) and the order-independent SelectBestReplica fix, both of which remain correct as-is.

[XFDG]

@Icedcoco @2sumtech @catyans I narrowed #3662 in commit 45522cec following the coordination on #3742. All ranged-session/helper overlap with merged #3903 has been removed; the PR now contains only the order-independent SelectBestReplica fix and focused regression coverage. I also adapted both guard cases from 2sumtech/Mooncake@67bfff5d: incomplete local MEMORY falls back to local NOF, and local NOF still outranks remote MEMORY. Thank you for preparing those cases. The title and description now reflect the reduced scope and no longer claim session or #3658 behavior. Please re-review the refreshed two-file change when convenient.

[fcczzz]

LGTM