[2sumtech]

Description

SelectBestReplica documents the priority local MEMORY > local NOF_SSD > remote MEMORY > remote NOF_SSD, and its comments note twice that the master may return replicas in any order so the scan must not stop early. The scan did stop early. The first COMPLETE local NOF_SSD replica was returned as soon as it was seen, so a local MEMORY replica sitting later in the list was never examined and the read went over NVMe-over-Fabrics instead of local host memory.

The result depends only on the order the master happened to return, which makes it easy to miss. With the memory replica listed first the selection is correct, with the NoF replica first it is not.

The fix remembers the first local NOF_SSD replica and keeps scanning. It is returned after the loop, so it still outranks every remote replica and the rest of the policy is unchanged.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Bug fix

How Has This Been Tested?

Three cases added to replica_selection_test.cpp. LocalMemoryWinsOverEarlierLocalNoF fails on main and passes with this change. The other two pin behavior the fix must not break, so they pass either way. A local MEMORY replica that is not COMPLETE still falls back to the local NoF one, and a local NoF replica still beats a remote MEMORY one.

Test commands:

ctest -R replica_selection_test --output-on-failure

Test results:

• Unit tests pass

Checklist

• I have performed a self-review of my own code
• I have added tests to prove my changes are effective

AI Assistance Disclosure

• AI tools were used (specify below)

Claude Code helped review the selection logic against the documented priority, wrote the failing test case, and drafted the fix. I have reviewed every changed line and can defend the change end to end.

[Icedcoco]

Thanks for isolating this issue and adding focused regression coverage. The bug is real, but there is now substantial overlap with #3662: its latest revision already updates SelectBestReplica to ensure that a local MEMORY replica wins over an earlier local NOF_SSD replica. Merging both PRs as-is would duplicate the same fix and likely create a conflict.

From my perspective, the cleanest approach would be to keep the functional fix in #3662, since it is part of the broader session-read correction there, and move the regression tests from #3742 into that PR. We could then merge #3662 with the additional coverage and close #3742 as a duplicate. This would preserve the useful tests while keeping a single implementation of the fix.

[2sumtech]

Sounds good to me, keeping one implementation in #3662 is the right call. I split the tests out into a single commit rebased onto the #3662 head so they can be cherry picked straight in, branch tests/replica-selection-coverage on my fork, commit 67bfff5. I dropped my LocalMemoryWinsOverEarlierLocalNoF case since LocalMemoryBeatsLocalNoFInBothOrders in #3662 already covers it, so what is left is the two guard cases, incomplete local MEMORY still falls back to local NoF, and local NoF still beats remote MEMORY. Happy to open it as a PR against the #3662 branch instead if that is easier. I will close this one once they land.

[XFDG]

The two guard tests from 67bfff5d have now been adapted into #3662 in commit 45522cec, alongside the existing both-orderings regression. #3662 has also been reduced to the replica-priority fix and tests only; all session overlap with merged #3903 was removed. Thanks @2sumtech for preparing the focused coverage. This completes the consolidation proposed by @Icedcoco, so #3742 can be closed as the duplicate implementation when convenient.

[Icedcoco]

Thanks for preparing the additional regression cases and coordinating their inclusion in #3662. The changes have now landed in main, including coverage for the incomplete local MEMORY fallback and local NOF vs. remote MEMORY priority.

There’s nothing further needed from your side for this fix. When convenient, could you close #3742 as superseded by #3662? Thanks again for helping make sure these edge cases are covered.