From 0216965131a5f89c62598827ea9ba0ac7a157074 Mon Sep 17 00:00:00 2001 From: pjdurden Date: Thu, 16 Jul 2026 10:20:42 -0500 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20kvcache-ai/Mooncake#2827=20=E2=80=94?= =?UTF-8?q?=20[Bug]:=20[SSD]=20DSV4=20enable=20SSD=20offload=EF=BC=8Cstres?= =?UTF-8?q?s=20testing,=20repeat=20offloading=20the=20same=20batch=20key,?= =?UTF-8?q?=20lead=20to=20OBJECT=5FALREADY=5FEXISTS/persist=20failed/INVAL?= =?UTF-8?q?ID=5FKEY?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- mooncake-store/include/file_storage.h | 20 +++++++++++++++++ mooncake-store/src/file_storage.cpp | 11 +++++++++- mooncake-store/tests/file_storage_test.cpp | 25 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/mooncake-store/include/file_storage.h b/mooncake-store/include/file_storage.h index cce6602ae0..bf7f55e5b2 100644 --- a/mooncake-store/include/file_storage.h +++ b/mooncake-store/include/file_storage.h @@ -77,6 +77,26 @@ class FileStorage { tl::expected OffloadObjects( const std::vector& offloading_objects); + /** + * @brief Classifies a BatchOffload error as affecting only the current + * bucket rather than the whole offload cycle. + * + * Such an error means the bucket's keys simply cannot be persisted this + * round; OffloadObjects reports them back to the master as failed (so their + * offloading tasks and source-replica refcounts are released) and continues + * with the remaining buckets, instead of aborting the entire cycle. + * + * - INVALID_READ: source data for these keys could not be read/staged. + * - OBJECT_ALREADY_EXISTS: the key(s) were already offloaded, or are + * being offloaded concurrently. The bucket backend rejects the whole + * bucket atomically by design (see BucketStorageBackend::BatchOffload + * and its PrepareEviction duplicate guard). Treating this as fatal + * aborted the cycle, leaked offloading tasks, and left master/SSD + * metadata inconsistent, which surfaced as spurious INVALID_KEY on the + * read path (issue #2827). + */ + static bool IsPerBucketSoftOffloadError(ErrorCode error); + /** * @brief Performs a heartbeat operation for the FileStorage component. * 1. Sends object status (e.g., access frequency, size) to the master via diff --git a/mooncake-store/src/file_storage.cpp b/mooncake-store/src/file_storage.cpp index 7500059f53..1617974e95 100644 --- a/mooncake-store/src/file_storage.cpp +++ b/mooncake-store/src/file_storage.cpp @@ -440,6 +440,11 @@ tl::expected FileStorage::BatchGet( return batch_result; } +bool FileStorage::IsPerBucketSoftOffloadError(ErrorCode error) { + return error == ErrorCode::INVALID_READ || + error == ErrorCode::OBJECT_ALREADY_EXISTS; +} + tl::expected FileStorage::OffloadObjects( const std::vector& offloading_objects) { if (offloading_objects.empty()) { @@ -628,7 +633,11 @@ tl::expected FileStorage::OffloadObjects( enable_offloading_ = false; return tl::make_unexpected(offload_res.error()); } - if (offload_res.error() == ErrorCode::INVALID_READ) { + if (IsPerBucketSoftOffloadError(offload_res.error())) { + // Only this bucket failed to persist; report its keys back to + // the master as failed (releasing their offloading tasks and + // source-replica refcounts) and keep processing the remaining + // buckets instead of aborting the whole offload cycle. for (const auto& [key, _] : host_batch_object) { failed_tasks.push_back(task_by_storage_key.at(key)); } diff --git a/mooncake-store/tests/file_storage_test.cpp b/mooncake-store/tests/file_storage_test.cpp index 16413486df..86a05a5238 100644 --- a/mooncake-store/tests/file_storage_test.cpp +++ b/mooncake-store/tests/file_storage_test.cpp @@ -108,6 +108,12 @@ class FileStorageTest : public ::testing::Test { return bucket_backend->UngroupedOffloadingObjectsSize(); } + // Static funnel to the private FileStorage::IsPerBucketSoftOffloadError. + // FileStorageTest is friended; TEST_F-generated subclasses are not. + static bool CallIsPerBucketSoftOffloadError(ErrorCode error) { + return FileStorage::IsPerBucketSoftOffloadError(error); + } + void AssertHeartbeatEvictsAllKeys( FileStorage& fileStorage, const std::vector& expected_keys, const std::unordered_map>& @@ -521,6 +527,25 @@ TEST_F(FileStorageTest, NotifyEvictedDiskReplicasUsesTenantScopedKeys) { } } +// Regression test for issue #2827: under concurrent/repeat offload of the same +// keys, the bucket backend rejects a whole bucket atomically with +// OBJECT_ALREADY_EXISTS (see BucketStorageBackend duplicate-key tests). That +// error must be treated as a per-bucket soft failure so OffloadObjects reports +// the keys back to the master and continues, rather than aborting the whole +// offload cycle and leaving master/SSD metadata inconsistent (which surfaced as +// spurious INVALID_KEY on the read path). It stays alongside INVALID_READ, +// while genuinely fatal errors (e.g. KEYS_ULTRA_LIMIT, INTERNAL_ERROR) do not. +TEST_F(FileStorageTest, DuplicateOffloadErrorIsPerBucketSoftError) { + EXPECT_TRUE( + CallIsPerBucketSoftOffloadError(ErrorCode::OBJECT_ALREADY_EXISTS)); + EXPECT_TRUE(CallIsPerBucketSoftOffloadError(ErrorCode::INVALID_READ)); + + EXPECT_FALSE(CallIsPerBucketSoftOffloadError(ErrorCode::KEYS_ULTRA_LIMIT)); + EXPECT_FALSE(CallIsPerBucketSoftOffloadError(ErrorCode::INTERNAL_ERROR)); + EXPECT_FALSE(CallIsPerBucketSoftOffloadError(ErrorCode::INVALID_KEY)); + EXPECT_FALSE(CallIsPerBucketSoftOffloadError(ErrorCode::OK)); +} + TEST_F(FileStorageTest, InvalidIntValueUsesDefault) { SetEnv("MOONCAKE_OFFLOAD_BUCKET_KEYS_LIMIT", "abc"); SetEnv("MOONCAKE_OFFLOAD_TOTAL_SIZE_LIMIT_BYTES", "sdfsdf"); From a38cbe60c1a23500c99ac9e3eaae40475fcf0754 Mon Sep 17 00:00:00 2001 From: pjdurden Date: Fri, 17 Jul 2026 15:26:25 -0500 Subject: [PATCH 2/3] fix(file_storage): flush NACKs on whole-cycle offload abort Address review on #2967: the KEYS_ULTRA_LIMIT and hard-error paths in OffloadObjects returned immediately, leaving every drained key (the failing bucket plus all unvisited buckets) without a failure NACK, so their offloading tasks and source-replica refcounts leaked until the put_start_release_timeout_sec_ TTL reaper fired. Record the aborting error and break instead of returning, then fall through to the existing not-in-any-bucket sweep + NACK flush. The failing bucket's keys are pushed before the break; unvisited buckets are not yet in all_bucket_keys so the sweep NACKs them. The original error is still propagated to the caller after the flush. Also skip BatchOffload when host_batch_object is empty (every object in the bucket failed D2H staging): those keys are already in failed_tasks, and an empty map would otherwise return INVALID_KEY and trip the abort. --- mooncake-store/src/file_storage.cpp | 44 +++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/mooncake-store/src/file_storage.cpp b/mooncake-store/src/file_storage.cpp index 1617974e95..4740d5099c 100644 --- a/mooncake-store/src/file_storage.cpp +++ b/mooncake-store/src/file_storage.cpp @@ -512,6 +512,10 @@ tl::expected FileStorage::OffloadObjects( // orphaned offloading_tasks and release source replica refcounts. std::vector failed_tasks; std::unordered_set all_bucket_keys; + // Set when a whole-cycle error aborts the bucket loop early. We still fall + // through to the NACK flush below before returning it, so no drained key is + // left waiting on the TTL reaper. + std::optional abort_error; for (const auto& keys : buckets_keys) { for (const auto& k : keys) all_bucket_keys.insert(k); @@ -589,6 +593,14 @@ tl::expected FileStorage::OffloadObjects( } } + // If every object in this bucket failed D2H staging, host_batch_object + // is empty (those keys are already in failed_tasks). Skip BatchOffload, + // which rejects an empty map as INVALID_KEY and would otherwise trip the + // whole-cycle abort below for a bucket that has nothing left to persist. + if (host_batch_object.empty()) { + continue; + } + auto offload_start = std::chrono::steady_clock::now(); auto bucket_complete_handler = [this, offload_start, complete_handler]( @@ -628,22 +640,29 @@ tl::expected FileStorage::OffloadObjects( if (!offload_res) { LOG(ERROR) << "Failed to store objects with error: " << offload_res.error(); + // This bucket did not persist, so report its keys back to the + // master as failed regardless of whether we continue or abort. + // Doing it here (rather than only on the soft path) keeps their + // offloading tasks and source-replica refcounts from leaking until + // the put_start_release_timeout_sec_ TTL reaper fires. + for (const auto& [key, _] : host_batch_object) { + failed_tasks.push_back(task_by_storage_key.at(key)); + } if (offload_res.error() == ErrorCode::KEYS_ULTRA_LIMIT) { + // Disk is over the key-count limit: stop offloading entirely. MutexLocker locker(&offloading_mutex_); enable_offloading_ = false; - return tl::make_unexpected(offload_res.error()); } - if (IsPerBucketSoftOffloadError(offload_res.error())) { - // Only this bucket failed to persist; report its keys back to - // the master as failed (releasing their offloading tasks and - // source-replica refcounts) and keep processing the remaining - // buckets instead of aborting the whole offload cycle. - for (const auto& [key, _] : host_batch_object) { - failed_tasks.push_back(task_by_storage_key.at(key)); - } - } else { - return tl::make_unexpected(offload_res.error()); + if (!IsPerBucketSoftOffloadError(offload_res.error())) { + // Whole-cycle error (KEYS_ULTRA_LIMIT or any hard failure): + // stop processing further buckets, but fall through to the NACK + // flush below so every drained key is released. Unvisited + // buckets are not yet in all_bucket_keys, so the sweep NACKs + // them too; this bucket's keys were just pushed above. + abort_error = offload_res.error(); + break; } + // Soft per-bucket error: keep processing the remaining buckets. } } @@ -670,6 +689,9 @@ tl::expected FileStorage::OffloadObjects( } } + if (abort_error) { + return tl::make_unexpected(*abort_error); + } return {}; } From f48e1197686321f46897268526a72e2149ee88ec Mon Sep 17 00:00:00 2001 From: pjdurden Date: Mon, 20 Jul 2026 22:16:21 -0500 Subject: [PATCH 3/3] style: clang-format the empty-bucket guard comment Reflow the host_batch_object.empty() comment to satisfy ./scripts/code_format.sh (clang-format 20, ColumnLimit 80). No functional change. --- mooncake-store/src/file_storage.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mooncake-store/src/file_storage.cpp b/mooncake-store/src/file_storage.cpp index 4740d5099c..ab515b6948 100644 --- a/mooncake-store/src/file_storage.cpp +++ b/mooncake-store/src/file_storage.cpp @@ -595,8 +595,9 @@ tl::expected FileStorage::OffloadObjects( // If every object in this bucket failed D2H staging, host_batch_object // is empty (those keys are already in failed_tasks). Skip BatchOffload, - // which rejects an empty map as INVALID_KEY and would otherwise trip the - // whole-cycle abort below for a bucket that has nothing left to persist. + // which rejects an empty map as INVALID_KEY and would otherwise trip + // the whole-cycle abort below for a bucket that has nothing left to + // persist. if (host_batch_object.empty()) { continue; }