From 86abe19a852d879f20ee7649745ae25a3054f663 Mon Sep 17 00:00:00 2001 From: zhaoye Date: Tue, 25 Aug 2026 15:52:31 +0800 Subject: [PATCH 1/6] [Bugfix][Store] Use SelectBestReplica in batch_get_session_start to support SSD offloading (#3658) When SSD offloading is enabled, some data resides on SSD instead of in-memory. batch_get_session_start was using SelectCompleteMemoryReplica which only selects memory replicas, causing INVALID_REPLICA (702) errors for keys whose replicas exist only on SSD. Fix: replace SelectCompleteMemoryReplica with SelectBestReplica, which handles all replica types (memory, SSD, disk, DFS) and picks the best available one, matching the behavior of batch_get_into and other get methods. Co-Authored-By: Claude --- mooncake-store/src/real_client.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mooncake-store/src/real_client.cpp b/mooncake-store/src/real_client.cpp index 5a93169618..07d477eeab 100644 --- a/mooncake-store/src/real_client.cpp +++ b/mooncake-store/src/real_client.cpp @@ -5479,9 +5479,9 @@ std::vector RealClient::batch_get_session_start( } const auto *replica = - SelectCompleteMemoryReplica(query_result.replicas, local_endpoints); + SelectBestReplica(query_result.replicas, local_endpoints); if (!replica) { - LOG(ERROR) << "No complete memory replica for key: " << keys[i]; + LOG(ERROR) << "No complete replica for key: " << keys[i]; results[i] = static_cast(toInt(ErrorCode::INVALID_REPLICA)); get_sessions_.erase(keys[i]); continue; From be87e53a0593d5fc11d5bf7a97e388a0484bf13f Mon Sep 17 00:00:00 2001 From: zhaoye Date: Thu, 27 Aug 2026 12:01:34 +0800 Subject: [PATCH 2/6] fix: address fcczzz+bitborne reviews on batch_get_session_start Two bugs in PR #3662: 1. SelectBestReplica priority: scan all replicas before returning so local MEMORY always beats local NOF regardless of list order. Previously the first local replica (memory or NOF) caused an early return, so [local NOF, local MEMORY] would incorrectly select NOF. 2. batch_get_into_multi_buffer_ranges replica_limit: NOF replicas also support range reads via RDMA. Use the NOF buffer size for overflow detection instead of 0, which caused INVALID_PARAMS for any NOF replica selected by batch_get_session_start. Co-Authored-By: Claude --- mooncake-store/include/replica_selection.h | 23 +++++++++++++++------- mooncake-store/src/real_client.cpp | 8 ++++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/mooncake-store/include/replica_selection.h b/mooncake-store/include/replica_selection.h index eb7fef190b..f0f2427067 100644 --- a/mooncake-store/include/replica_selection.h +++ b/mooncake-store/include/replica_selection.h @@ -117,12 +117,16 @@ inline const Replica::Descriptor *PickBestRemoteMemory( // Select the best replica from a list: prefer local MEMORY, local NOF_SSD, // remote MEMORY, remote NOF_SSD, LOCAL_DISK, DFS, then DISK. Master may return -// replicas in any order, so we always scan. When scoring is enabled and there -// are multiple remote MEMORY replicas, the best-scoring one is chosen instead -// of the first encountered. +// replicas in any order, so we always scan all replicas before deciding — this +// ensures local MEMORY is always preferred over local NOF regardless of the +// order replicas appear in the list. When scoring is enabled and there are +// multiple remote MEMORY replicas, the best-scoring one is chosen instead of +// the first encountered. inline const Replica::Descriptor *SelectBestReplica( const std::vector &replicas, const std::unordered_set &local_endpoints) { + const Replica::Descriptor *local_memory = nullptr; + const Replica::Descriptor *local_nof = nullptr; const Replica::Descriptor *first_memory = nullptr; const Replica::Descriptor *first_nof = nullptr; for (const auto &r : replicas) { @@ -131,18 +135,23 @@ inline const Replica::Descriptor *SelectBestReplica( if (local_endpoints.count( r.get_memory_descriptor() .buffer_descriptor.transport_endpoint_)) { - return &r; // local MEMORY — best case + if (!local_memory) local_memory = &r; + } else { + if (!first_memory) first_memory = &r; } - if (!first_memory) first_memory = &r; } else if (r.is_nof_replica()) { if (local_endpoints.count( r.get_nof_descriptor() .buffer_descriptor.transport_endpoint_)) { - return &r; // local NOF_SSD — also good + if (!local_nof) local_nof = &r; + } else { + if (!first_nof) first_nof = &r; } - if (!first_nof) first_nof = &r; } } + // Local MEMORY always beats local NOF regardless of list order. + if (local_memory) return local_memory; + if (local_nof) return local_nof; // No local replica. Among remote MEMORY replicas, optionally pick the // best-scoring one instead of the first encountered (issue #2516). if (first_memory && RemoteReplicaScoringEnabled()) { diff --git a/mooncake-store/src/real_client.cpp b/mooncake-store/src/real_client.cpp index 07d477eeab..4a6efca301 100644 --- a/mooncake-store/src/real_client.cpp +++ b/mooncake-store/src/real_client.cpp @@ -5539,12 +5539,16 @@ std::vector RealClient::batch_get_into_multi_buffer_ranges( results[i] = static_cast(toInt(ErrorCode::LEASE_EXPIRED)); continue; } - // start cached a single complete memory replica via - // FilterQueryResult. + // start cached a single complete replica via FilterQueryResult. + // SelectBestReplica may return a MEMORY or NOF replica; both + // support range reads via RDMA. Extract the object size from + // whichever descriptor is present so overflow detection works. const auto &replica = it->second.replicas.front(); const size_t replica_limit = replica.is_memory_replica() ? replica.get_memory_descriptor().buffer_descriptor.size_ + : replica.is_nof_replica() + ? replica.get_nof_descriptor().buffer_descriptor.size_ : 0; bool overflow = false; std::vector entry_slices; From 954d0f63e34c830184b9f473813381f13a09c6b1 Mon Sep 17 00:00:00 2001 From: Haoran FENG <91861184+XFDG@users.noreply.github.com> Date: Mon, 31 Aug 2026 05:21:58 +0000 Subject: [PATCH 3/6] [Bugfix][Store] Add regression test for local MEMORY vs NOF ordering (issue #3658) Covers both permutations (local NOF before local MEMORY, and vice versa) to lock in the order-independent priority fix in SelectBestReplica. --- .../tests/replica_selection_test.cpp | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/mooncake-store/tests/replica_selection_test.cpp b/mooncake-store/tests/replica_selection_test.cpp index af5600bad7..5e88e3eb39 100644 --- a/mooncake-store/tests/replica_selection_test.cpp +++ b/mooncake-store/tests/replica_selection_test.cpp @@ -97,6 +97,40 @@ TEST_F(ReplicaSelectionTest, LocalMemoryAlwaysWins) { "nodeB"); // locality beats protocol } +// Regression for issue #3658: a complete local MEMORY replica must win over a +// complete local NOF replica regardless of the order they appear in the list. +// ObjectMetadata preserves insertion order, and promotion appends a new MEMORY +// replica to existing metadata, so a MEMORY replica may legally appear after a +// NOF replica. The session range-read path is memory-only, so selecting NOF +// here would regress a session that still has a usable MEMORY replica. +TEST_F(ReplicaSelectionTest, LocalMemoryBeatsLocalNoFInBothOrders) { + std::unordered_set local = {"memB", "nofB"}; + + // local NOF listed before local MEMORY + std::vector nof_first = { + MakeNoF("nofB"), + MakeMemory("memB", "tcp"), + }; + const auto* sel = SelectBestReplica(nof_first, local); + ASSERT_NE(sel, nullptr); + EXPECT_TRUE(sel->is_memory_replica()); + EXPECT_EQ( + sel->get_memory_descriptor().buffer_descriptor.transport_endpoint_, + "memB"); + + // local MEMORY listed before local NOF (the previously-correct order) + std::vector mem_first = { + MakeMemory("memB", "tcp"), + MakeNoF("nofB"), + }; + sel = SelectBestReplica(mem_first, local); + ASSERT_NE(sel, nullptr); + EXPECT_TRUE(sel->is_memory_replica()); + EXPECT_EQ( + sel->get_memory_descriptor().buffer_descriptor.transport_endpoint_, + "memB"); +} + TEST_F(ReplicaSelectionTest, ScoringOffKeepsFirstRemoteMemory) { // No scorer injected and env not set -> must return the FIRST remote // MEMORY. From 2c009ca3a811b43a633594432a74ac7b5933d6e7 Mon Sep 17 00:00:00 2001 From: XFDG Date: Fri, 4 Sep 2026 15:34:18 +0800 Subject: [PATCH 4/6] [Bugfix][Store] Restrict batch_get_session_start to MEMORY replicas (#3658) The session range-read path is memory-only: Client::BatchTransferReadRanges rejects every non-MEMORY replica (client_service.cpp), so a session started against a NOF_SSD replica passes the size/overflow check and then fails the actual transfer with INVALID_PARAMS, and a LOCAL_DISK/DFS/DISK selection fails even earlier with a zero replica_limit. The previous revision switched batch_get_session_start to SelectBestReplica, which can return NOF_SSD (and lower tiers). Restore a MEMORY-only selector for the session path: move SelectCompleteMemoryReplica into replica_selection.h (next to SelectBestReplica) so it is unit-testable, and use it in batch_get_session_start. The single-object get path keeps SelectBestReplica, which correctly routes NOF/DISK/DFS through Client::Get. The #3658 fix (local MEMORY preferred over local NOF regardless of master return order) is preserved: SelectCompleteMemoryReplica scans all replicas and prefers a local MEMORY replica. Adds SessionPathRejectsNoFOnlyObject: 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. Co-Authored-By: Claude Opus 4.8 --- mooncake-store/include/replica_selection.h | 29 ++++++++++++++ mooncake-store/src/real_client.cpp | 39 ++++++------------- .../tests/replica_selection_test.cpp | 34 ++++++++++++++++ 3 files changed, 75 insertions(+), 27 deletions(-) diff --git a/mooncake-store/include/replica_selection.h b/mooncake-store/include/replica_selection.h index f0f2427067..842246bf15 100644 --- a/mooncake-store/include/replica_selection.h +++ b/mooncake-store/include/replica_selection.h @@ -177,4 +177,33 @@ inline const Replica::Descriptor *SelectBestReplica( return best; } +// Select a complete MEMORY replica for the session range-read path. The +// session ranged-get path (Client::BatchTransferReadRanges) only ever reads +// MEMORY replicas: it rejects every non-MEMORY entry, so selecting a NOF_SSD +// replica here would let a session start and then fail the actual transfer. +// Among complete MEMORY replicas, a local one is preferred; otherwise the +// first complete MEMORY replica is returned (nullptr if there is none). +// +// Unlike SelectBestReplica this deliberately does NOT consider NOF_SSD: the +// session path cannot read it. Local MEMORY is still preferred over a remote +// MEMORY replica regardless of the order the master returned them in. +inline const Replica::Descriptor *SelectCompleteMemoryReplica( + const std::vector &replicas, + const std::unordered_set &local_endpoints) { + const Replica::Descriptor *first_memory = nullptr; + for (const auto &r : replicas) { + if (r.status != ReplicaStatus::COMPLETE || !r.is_memory_replica()) { + continue; + } + if (local_endpoints.count(r.get_memory_descriptor() + .buffer_descriptor.transport_endpoint_)) { + return &r; + } + if (!first_memory) { + first_memory = &r; + } + } + return first_memory; +} + } // namespace mooncake diff --git a/mooncake-store/src/real_client.cpp b/mooncake-store/src/real_client.cpp index 4a6efca301..2c775d7b41 100644 --- a/mooncake-store/src/real_client.cpp +++ b/mooncake-store/src/real_client.cpp @@ -395,6 +395,7 @@ inline tl::expected gather_maybe_device_to_host( // SelectBestReplica and the replica-scoring helpers live in // replica_selection.h (included above) so they can be unit-tested directly. using mooncake::SelectBestReplica; +using mooncake::SelectCompleteMemoryReplica; // Build a QueryResult containing only the chosen replica so that // Client::Get / Client::BatchGet (which internally call @@ -413,25 +414,6 @@ inline bool is_object_range_overflow(size_t offset, size_t size, size_t limit) { return size > limit || offset > limit - size; } -inline const Replica::Descriptor *SelectCompleteMemoryReplica( - const std::vector &replicas, - const std::unordered_set &local_endpoints) { - const Replica::Descriptor *first_memory = nullptr; - for (const auto &r : replicas) { - if (r.status != ReplicaStatus::COMPLETE || !r.is_memory_replica()) { - continue; - } - if (local_endpoints.count(r.get_memory_descriptor() - .buffer_descriptor.transport_endpoint_)) { - return &r; - } - if (!first_memory) { - first_memory = &r; - } - } - return first_memory; -} - inline bool HasMemoryReplica(const std::vector &replicas) { for (const auto &r : replicas) { if (r.is_memory_replica()) { @@ -5478,10 +5460,15 @@ std::vector RealClient::batch_get_session_start( continue; } + // The session range-read path (batch_get_into_multi_buffer_ranges -> + // Client::BatchTransferReadRanges) only reads MEMORY replicas, so the + // session must be started against a complete MEMORY replica. Selecting + // a NOF_SSD replica here would let the session start and then fail the + // actual transfer (BatchTransferReadRanges rejects non-MEMORY entries). const auto *replica = - SelectBestReplica(query_result.replicas, local_endpoints); + SelectCompleteMemoryReplica(query_result.replicas, local_endpoints); if (!replica) { - LOG(ERROR) << "No complete replica for key: " << keys[i]; + LOG(ERROR) << "No complete memory replica for key: " << keys[i]; results[i] = static_cast(toInt(ErrorCode::INVALID_REPLICA)); get_sessions_.erase(keys[i]); continue; @@ -5539,16 +5526,14 @@ std::vector RealClient::batch_get_into_multi_buffer_ranges( results[i] = static_cast(toInt(ErrorCode::LEASE_EXPIRED)); continue; } - // start cached a single complete replica via FilterQueryResult. - // SelectBestReplica may return a MEMORY or NOF replica; both - // support range reads via RDMA. Extract the object size from - // whichever descriptor is present so overflow detection works. + // start cached a single complete MEMORY replica via + // FilterQueryResult. The session range-read path only reads + // MEMORY replicas, so the cached replica is always MEMORY here; + // extract its object size for overflow detection. const auto &replica = it->second.replicas.front(); const size_t replica_limit = replica.is_memory_replica() ? replica.get_memory_descriptor().buffer_descriptor.size_ - : replica.is_nof_replica() - ? replica.get_nof_descriptor().buffer_descriptor.size_ : 0; bool overflow = false; std::vector entry_slices; diff --git a/mooncake-store/tests/replica_selection_test.cpp b/mooncake-store/tests/replica_selection_test.cpp index 5e88e3eb39..527d0fcabd 100644 --- a/mooncake-store/tests/replica_selection_test.cpp +++ b/mooncake-store/tests/replica_selection_test.cpp @@ -131,6 +131,40 @@ TEST_F(ReplicaSelectionTest, LocalMemoryBeatsLocalNoFInBothOrders) { "memB"); } +// Regression for issue #3658 (session range-read path): the session ranged-get +// path only reads MEMORY replicas, so a session must never be started against a +// NOF_SSD replica — it would pass the size check and then fail the actual +// transfer. SelectCompleteMemoryReplica must therefore skip a NOF-only object +// and return nullptr (the caller then rejects the session), even though +// SelectBestReplica would have selected the NOF replica for the single-object +// get path. +TEST_F(ReplicaSelectionTest, SessionPathRejectsNoFOnlyObject) { + std::unordered_set local = {"nofB"}; + + // A NOF-only object: SelectBestReplica returns the NOF replica (it is the + // last-resort tier for the single-object get path), but the session path + // cannot read NOF, so the session selector must decline it. + std::vector nof_only = {MakeNoF("nofB")}; + const auto* best = SelectBestReplica(nof_only, local); + ASSERT_NE(best, nullptr); + EXPECT_TRUE(best->is_nof_replica()); // single-object path would use it + EXPECT_EQ(SelectCompleteMemoryReplica(nof_only, local), + nullptr); // session path: nothing to read + + // A local MEMORY replica must still be selected for the session even when a + // local NOF replica is also present (local MEMORY outranks local NOF). + std::vector mem_and_nof = { + MakeNoF("nofB"), + MakeMemory("memB", "tcp"), + }; + const auto* sel = SelectCompleteMemoryReplica(mem_and_nof, local); + ASSERT_NE(sel, nullptr); + EXPECT_TRUE(sel->is_memory_replica()); + EXPECT_EQ( + sel->get_memory_descriptor().buffer_descriptor.transport_endpoint_, + "memB"); +} + TEST_F(ReplicaSelectionTest, ScoringOffKeepsFirstRemoteMemory) { // No scorer injected and env not set -> must return the FIRST remote // MEMORY. From 21dbb5829410abe39e05e690ab6f6241d8df4ca7 Mon Sep 17 00:00:00 2001 From: zhaoye Date: Mon, 7 Sep 2026 19:13:05 +0800 Subject: [PATCH 5/6] ci: re-run CI after #3870 fixes test_concurrent_stress flake From 45522cecb9f919bde9d3b8d0c1752d8ddd243a1a Mon Sep 17 00:00:00 2001 From: zhaoye Date: Sun, 20 Sep 2026 16:10:15 +0800 Subject: [PATCH 6/6] fix: narrow replica selection ordering scope --- mooncake-store/include/replica_selection.h | 29 ----------- mooncake-store/src/real_client.cpp | 19 ++++++++ .../tests/replica_selection_test.cpp | 48 ++++++++----------- 3 files changed, 40 insertions(+), 56 deletions(-) diff --git a/mooncake-store/include/replica_selection.h b/mooncake-store/include/replica_selection.h index 89c89c58ee..475020d042 100644 --- a/mooncake-store/include/replica_selection.h +++ b/mooncake-store/include/replica_selection.h @@ -174,33 +174,4 @@ inline const Replica::Descriptor *SelectBestReplica( return best; } -// Select a complete MEMORY replica for the session MEMORY range-read path. -// The session planner may fall back to DFS, but it does not support NOF_SSD; -// selecting a NOF_SSD replica here would let a session start and then fail -// request classification. -// Among complete MEMORY replicas, a local one is preferred; otherwise the -// first complete MEMORY replica is returned (nullptr if there is none). -// -// Unlike SelectBestReplica this deliberately does NOT consider NOF_SSD. -// Local MEMORY is still preferred over a remote MEMORY replica regardless of -// the order the master returned them in. -inline const Replica::Descriptor *SelectCompleteMemoryReplica( - const std::vector &replicas, - const std::unordered_set &local_endpoints) { - const Replica::Descriptor *first_memory = nullptr; - for (const auto &r : replicas) { - if (r.status != ReplicaStatus::COMPLETE || !r.is_memory_replica()) { - continue; - } - if (local_endpoints.count(r.get_memory_descriptor() - .buffer_descriptor.transport_endpoint_)) { - return &r; - } - if (!first_memory) { - first_memory = &r; - } - } - return first_memory; -} - } // namespace mooncake diff --git a/mooncake-store/src/real_client.cpp b/mooncake-store/src/real_client.cpp index bec5d39dfb..4baad1df33 100644 --- a/mooncake-store/src/real_client.cpp +++ b/mooncake-store/src/real_client.cpp @@ -462,6 +462,25 @@ inline bool is_object_range_overflow(size_t offset, size_t size, size_t limit) { return size > limit || offset > limit - size; } +inline const Replica::Descriptor *SelectCompleteMemoryReplica( + const std::vector &replicas, + const std::unordered_set &local_endpoints) { + const Replica::Descriptor *first_memory = nullptr; + for (const auto &r : replicas) { + if (r.status != ReplicaStatus::COMPLETE || !r.is_memory_replica()) { + continue; + } + if (local_endpoints.count(r.get_memory_descriptor() + .buffer_descriptor.transport_endpoint_)) { + return &r; + } + if (!first_memory) { + first_memory = &r; + } + } + return first_memory; +} + inline const Replica::Descriptor *SelectSessionReplica( const std::vector &replicas, const std::unordered_set &local_endpoints) { diff --git a/mooncake-store/tests/replica_selection_test.cpp b/mooncake-store/tests/replica_selection_test.cpp index 3e7232b009..4dc097f0a8 100644 --- a/mooncake-store/tests/replica_selection_test.cpp +++ b/mooncake-store/tests/replica_selection_test.cpp @@ -151,35 +151,29 @@ TEST_F(ReplicaSelectionTest, LocalMemoryBeatsLocalNoFInBothOrders) { "memB"); } -// Regression for issue #3658 (session range-read path): the session planner -// supports MEMORY and DFS replicas, but not NOF_SSD. The MEMORY selector must -// therefore skip a NOF-only object and return nullptr, even though -// SelectBestReplica would select that replica for the single-object get path. -TEST_F(ReplicaSelectionTest, SessionPathRejectsNoFOnlyObject) { - std::unordered_set local = {"nofB"}; - - // A NOF-only object: SelectBestReplica returns the NOF replica (it is the - // last-resort tier for the single-object get path), but the session planner - // cannot read NOF, so its MEMORY selector must decline it. - std::vector nof_only = {MakeNoF("nofB")}; - const auto* best = SelectBestReplica(nof_only, local); - ASSERT_NE(best, nullptr); - EXPECT_TRUE(best->is_nof_replica()); // single-object path would use it - EXPECT_EQ(SelectCompleteMemoryReplica(nof_only, local), - nullptr); // session path: nothing to read - - // A local MEMORY replica must still be selected for the session even when a - // local NOF replica is also present (local MEMORY outranks local NOF). - std::vector mem_and_nof = { - MakeNoF("nofB"), - MakeMemory("memB", "tcp"), +TEST_F(ReplicaSelectionTest, IncompleteLocalMemoryFallsBackToLocalNoF) { + // The local MEMORY replica is not readable yet, so the local NOF_SSD one + // remains the best choice. + std::unordered_set local = {"nodeA"}; + std::vector reps = { + MakeNoF("nodeA"), + MakeMemory("nodeA", "rdma", ReplicaStatus::PROCESSING), }; - const auto* sel = SelectCompleteMemoryReplica(mem_and_nof, local); + const auto* sel = SelectBestReplica(reps, local); ASSERT_NE(sel, nullptr); - EXPECT_TRUE(sel->is_memory_replica()); - EXPECT_EQ( - sel->get_memory_descriptor().buffer_descriptor.transport_endpoint_, - "memB"); + EXPECT_TRUE(sel->is_nof_replica()); +} + +TEST_F(ReplicaSelectionTest, LocalNoFPrecedesRemoteMemory) { + // Locality still outranks tier for remote MEMORY replicas. + std::unordered_set local = {"nodeA"}; + std::vector reps = { + MakeNoF("nodeA"), + MakeMemory("nodeB", "rdma"), + }; + const auto* sel = SelectBestReplica(reps, local); + ASSERT_NE(sel, nullptr); + EXPECT_TRUE(sel->is_nof_replica()); } TEST_F(ReplicaSelectionTest, ScoringOffKeepsFirstRemoteMemory) {