[GH-ISSUE #4487] [Store] remove + re-put of the same key never re-offloads with the bucket backend #1140

New issue

Open

opened 2026-10-29 21:23:15 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented 2026-10-29 21:23:15 +00:00

Owner

Copy link

Originally created by @he-yufeng on GitHub (Oct 6, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4487

While extending the regression coverage on #3889 I hit an offload quirk that looks worth its own report.

Repro, in-proc master harness with bucket_storage_backend and MOONCAKE_OFFLOAD_BUCKET_KEYS_LIMIT=1:

1. put(key) then the object offloads, LOCAL_DISK replica R1 registered.
2. remove(key), master record erased.
3. put(key) again, fresh memory replica, PutEnd queues the offload task.
4. The next heartbeat takes the task but the key comes back as a NACK (data_size=-1) and no new disk replica ever registers. The object stays memory-only indefinitely.

What happens on the client: FileStorage::OffloadObjects finds the local slices fine, but BucketStorageBackend still holds the key in object_bucket_map_ from the first offload (remove never reaches the backend), so BatchOffload skips it as an already-persisted duplicate without invoking the completion callback, and FileStorage NACKs it per the "a successful batch may skip duplicates" contract. The master erases the offload marker on the NACK, so the key can never be offloaded again until the backend evicts the stale entry.

Two consequences:

• Durability: a re-put object silently loses its disk tier. If the memory replica is later evicted the object is gone, even with free SSD capacity.
• Staleness: the backend map still points at the old bucket file with the old bytes. Nothing serves it while the master holds no disk record, but a rescan-based path (master restart + ReRegisterOffloadedObjects) looks like it could re-register the old bytes under the new object. I have not verified that second path end to end.

Possible directions: propagate remove() to the storage backend so the persisted entry drops with the master record, or let BatchOffload overwrite a persisted entry under a generation check instead of skipping it.

Observed on current main (ubuntu 22.04 container, gcc) while testing #3889. I have the full glog trace (heartbeat takes the task, backend processes zero keys, NACK goes back, master cleans the marker) if useful.

Originally created by @he-yufeng on GitHub (Oct 6, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4487 While extending the regression coverage on #3889 I hit an offload quirk that looks worth its own report. Repro, in-proc master harness with `bucket_storage_backend` and `MOONCAKE_OFFLOAD_BUCKET_KEYS_LIMIT=1`: 1. `put(key)` then the object offloads, LOCAL_DISK replica R1 registered. 2. `remove(key)`, master record erased. 3. `put(key)` again, fresh memory replica, PutEnd queues the offload task. 4. The next heartbeat takes the task but the key comes back as a NACK (`data_size=-1`) and no new disk replica ever registers. The object stays memory-only indefinitely. What happens on the client: `FileStorage::OffloadObjects` finds the local slices fine, but `BucketStorageBackend` still holds the key in `object_bucket_map_` from the first offload (remove never reaches the backend), so `BatchOffload` skips it as an already-persisted duplicate without invoking the completion callback, and `FileStorage` NACKs it per the "a successful batch may skip duplicates" contract. The master erases the offload marker on the NACK, so the key can never be offloaded again until the backend evicts the stale entry. Two consequences: - Durability: a re-put object silently loses its disk tier. If the memory replica is later evicted the object is gone, even with free SSD capacity. - Staleness: the backend map still points at the old bucket file with the old bytes. Nothing serves it while the master holds no disk record, but a rescan-based path (master restart + `ReRegisterOffloadedObjects`) looks like it could re-register the old bytes under the new object. I have not verified that second path end to end. Possible directions: propagate `remove()` to the storage backend so the persisted entry drops with the master record, or let `BatchOffload` overwrite a persisted entry under a generation check instead of skipping it. Observed on current main (ubuntu 22.04 container, gcc) while testing #3889. I have the full glog trace (heartbeat takes the task, backend processes zero keys, NACK goes back, master cleans the marker) if useful.

bsakaguchi commented 2026-10-29 21:23:16 +00:00

Author

Owner

Copy link

@github-actions[bot] commented on GitHub (Oct 6, 2026):

Thanks for opening this issue, @he-yufeng!

Field	Value
Issue	#4487
GitHub user ID	40085740
Reporter	@he-yufeng

A maintainer will triage this when possible. To help us respond faster, please include:

• Mooncake version or commit SHA
• Environment (OS, CUDA/driver, RDMA stack if relevant)
• Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

<!-- gh-comment-id:6024669632 --> @github-actions[bot] commented on GitHub (Oct 6, 2026): Thanks for opening this issue, @he-yufeng! | Field | Value | |-------|-------| | **Issue** | #4487 | | **GitHub user ID** | `40085740` | | **Reporter** | @he-yufeng | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented 2026-10-29 21:23:16 +00:00

Author

Owner

Copy link

@feichai0017 commented on GitHub (Oct 8, 2026):

Confirmed on upstream main b7595de3fab45a580e4ee8819c2a2940d43183cd in an independent Mooncake checkout, with production sources unchanged and no OrbitKV code. I also reproduced the stale-data rescan path mentioned in the report, with the scope described below.

Reproduction results

The test-only patch below uses the existing FileStorageTest fixture with the production embedded Master, Client, FileStorage, and BucketStorageBackend. It performs actual puts/gets and disk I/O. Offload heartbeats, OffloadObjects, and recovery rescan are invoked synchronously to make the sequence deterministic; the FileStorage background scheduler is not started.

Environment: Debian 12 container, GCC 12.2, USE_CUDA=OFF, USE_TENT=OFF, classic TCP configured, bucket key limit 1, 4,109-byte values. This is a single-process Store reproducer, not a TENT or cross-host transport test.

Case	Results over separate process runs	Observation after the second put
Remove + re-put the same key with different bytes	3/3 reproduce; exit 1	New value reads correctly from memory; no LOCAL_DISK replica; backend still returns the complete old value
Same sequence, then call production ReRegisterOffloadedObjects()	3/3 reproduce; exit 1	LOCAL_DISK count changes from 0 to 1, while the backend still returns the complete old value
Fresh-key control for the second put	3/3 pass; exit 0	One LOCAL_DISK replica and the correct new disk bytes
Existing duplicate-only / mixed fresh-and-duplicate offload controls	Both pass; exit 0	Existing skip/notification behavior remains intact

Each round has a different deterministic byte pattern. The test compares all bytes, poisons the Client::Get destination, checks both boundary guards, and uses ordinary Remove only after the read lease expires. The first offload and disk-byte check pass in all runs. The next heartbeat after the second offload returns zero tasks.

Representative second-round observations:

same_key=1 rescan=0 disk_replicas_before_rescan=0 disk_replicas_after_rescan=0 disk_matches_current=0 disk_matches_previous=1 next_heartbeat_tasks=0
same_key=1 rescan=1 disk_replicas_before_rescan=0 disk_replicas_after_rescan=1 disk_matches_current=0 disk_matches_previous=1 next_heartbeat_tasks=0
same_key=0 rescan=0 disk_replicas_before_rescan=1 disk_replicas_after_rescan=1 disk_matches_current=1 disk_matches_previous=0 next_heartbeat_tasks=0

The rescan result proves that the production rescan/registration path can attach a disk replica backed by old bytes to the new object. I have not tested an actual Master restart/HA transition, nor a subsequent user Get forced onto that disk replica. The new-value Client::Get check in this reproducer happens before rescan; the stale-byte observation is a direct backend BatchLoad afterward.

Root cause and proposed fix

One refinement to the trace in the issue: in this sequential reproduction, the stale entry is filtered before BatchOffload, by the IsExist(key) check in GroupOffloadingKeysByBucket. OffloadObjects NACKs keys skipped during grouping, and the Master clears the offload task. Fixing only the duplicate check inside BatchOffload would miss this path.

For rescan, ReRegisterOffloadedObjects reports persisted metadata through NotifyOffloadSuccess. With no pending offload task, the Master falls back to replica registration for the current key. OffloadTaskItem carries tenant/key/size, and StorageObjectMetadata carries location/size, but neither carries the object incarnation needed to distinguish the old and new values. Equal key and size are insufficient; both values in the reproducer deliberately have the same size.

My proposed fix is to keep the change within the offload lifecycle and distinguish same-incarnation retry from new-incarnation re-put:

1. Carry an authoritative object incarnation through the offload task and persisted per-object metadata. Validate it against the current Master object before adding a disk replica. Completion/NACK handling must also match the offload task being completed, so a late old notification cannot clear a newer task. Recovery rescan must use the persisted incarnation rather than adopt an entry by key/size alone.
2. Preserve duplicate idempotence for the same incarnation; conditionally replace older incarnations. Apply the distinction at grouping and at the backend's locked duplicate/commit checks. Write the new data before publishing its mapping, and retire the old per-key entry safely. Do not delete an entire bucket containing unrelated live keys, or reclaim files while readers still own them.
3. Make replacement/invalidation survive recovery. Merely removing an in-memory map entry leaves the old bucket discoverable by ScanMeta. The persisted metadata needs a replacement/invalidation rule, with conservative handling of legacy entries whose incarnation cannot be established. If recovery is intended to preserve explicit removals across complete Master metadata loss, deletion/tombstone semantics must be defined as well; an incarnation field alone does not establish which objects remain live.

This is a proposed design, not a tested fix. Before changing the wire/disk metadata, I would align the existing-bucket compatibility and recovery policy with maintainers. An alternative based on propagating remove() could work, but it would need equivalent conditional invalidation and ordering against in-flight offloads; a key-only delete or blind overwrite does not address late notifications and rescan.

The fix should preserve #3135's valid retry behavior. #3889 addresses missing backing files; here the file exists and contains old bytes, so a missing-file check alone would not detect this case. Regression coverage should include the three cases below plus delayed old completion/NACK, same-incarnation retry, a shared bucket with another live key, and persisted-state reopen/rescan.

Reproduction commands and test-only patch

In a standalone checkout of b7595de3fab45a580e4ee8819c2a2940d43183cd, save the diff below as reproduction.patch outside the checkout and apply it. With the normal Mooncake C++ build dependencies available (including Boost, msgpack, xxhash and zstd development headers/libraries):

git apply /path/to/reproduction.patch
git submodule update --init extern/pybind11
cmake -S . -B /tmp/mooncake-4487-build \
  -DCMAKE_BUILD_TYPE=Release \
  -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ \
  '-DCMAKE_CXX_FLAGS_RELEASE=-O1 -DNDEBUG' \
  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
  -DUSE_CUDA=OFF -DUSE_TENT=OFF \
  -DWITH_STORE=ON -DWITH_STORE_RUST=OFF \
  -DBUILD_UNIT_TESTS=ON -DBUILD_EXAMPLES=OFF -DBUILD_BENCHMARK=OFF \
  -DUSE_ETCD=OFF -DUSE_REDIS=OFF \
  -DSTORE_USE_ETCD=OFF -DSTORE_USE_REDIS=OFF -DSTORE_USE_JEMALLOC=OFF
cmake --build /tmp/mooncake-4487-build --target file_storage_test -j8

For my run, missing development packages were extracted into an external private dependency prefix and passed via CMAKE_PREFIX_PATH / -isystem; no production code changes or CUDA stubs were used. The executable, runtime libraries, patch and configuration were frozen and SHA256-checked before and after testing. Patch SHA256: ba13e352058e582841a1e59bb6328d4d942afcd7225798429657e9f465c713dc.

The following launches each new case three times with separate working directories and a timeout, then the two existing controls. It prints expected nonzero statuses instead of stopping at the first reproduced failure. Log/XML output stays outside the checkout.

python3 - /tmp/mooncake-4487-build/mooncake-store/tests/file_storage_test <<'PYRUN'
import os
from pathlib import Path
import subprocess
import sys
import tempfile

binary = str(Path(sys.argv[1]).resolve())
root = Path(tempfile.mkdtemp(prefix="mooncake-4487-results-"))
env = {k: v for k, v in os.environ.items()
       if not k.startswith(("MC_", "MOONCAKE_", "TENT_"))}
env.update(MC_FORCE_TCP="1", MC_STORE_MEMCPY="0", MC_USE_TENT="0", GLOG_v="1")
cases = {
    "fresh-key": "FileStorageTest.Issue4487FreshKeyControl",
    "same-key": "FileStorageTest.Issue4487ReputSameKeyMustOffloadNewBytes",
    "rescan": "FileStorageTest.Issue4487RescanMustNotReattachOldBytes",
}
jobs = [(f"{label}-{i}", case) for label, case in cases.items() for i in range(1, 4)]
jobs.append(("existing-controls",
    "FileStorageTest.SkippedOffloadAllDuplicatesReleaseSourceTasks:"
    "FileStorageTest.SkippedOffloadFreshBatchKeepsSuccessNotifications"))
for label, case in jobs:
    run = root / label
    run.mkdir()
    with (run / "run.log").open("wb") as log:
        result = subprocess.run(
            ["timeout", "--kill-after=5s", "120s", binary,
             "--gtest_filter=" + case, "--gtest_output=xml:" + str(run / "gtest.xml")],
            cwd=run, env=env, stdout=log, stderr=subprocess.STDOUT)
    print(label, "exit=" + str(result.returncode), "log=" + str(run / "run.log"))
print("Results:", root)
PYRUN

The patch adds tests only, using the fixture's existing friendship for deterministic calls to production offload/rescan methods. Both bug cases assert the desired correct behavior and therefore fail on the tested main revision.

diff --git a/mooncake-store/tests/file_storage_test.cpp b/mooncake-store/tests/file_storage_test.cpp
index 3f74941b..d4ad3c45 100644
--- a/mooncake-store/tests/file_storage_test.cpp
+++ b/mooncake-store/tests/file_storage_test.cpp
@@ -196,6 +196,133 @@ class FileStorageTest : public ::testing::Test {
         EXPECT_TRUE(client->unregisterLocalMemory(allocator.getBase()));
     }
 
+    void RunIssue4487Scenario(bool same_key, bool rescan) {
+        testing::InProcMaster master;
+        ASSERT_TRUE(master.Start(InProcMasterConfigBuilder()
+                                     .set_enable_offload(true)
+                                     .set_default_kv_lease_ttl(1000)
+                                     .set_root_fs_dir("")
+                                     .build()));
+        constexpr size_t kSegmentSize = 16 * 1024 * 1024;
+        constexpr size_t kValueSize = 4096 + 13;
+        constexpr size_t kGuardSize = 16;
+        std::unique_ptr<void, decltype(&std::free)> segment(
+            allocate_buffer_allocator_memory(kSegmentSize), &std::free);
+        ASSERT_NE(segment, nullptr);
+        SimpleAllocator allocator(kSegmentSize);
+        const auto endpoint = "127.0.0.1:" + std::to_string(getFreeTcpPort());
+        auto created = Client::Create(endpoint, master.metadata_url(), "tcp",
+                                      std::nullopt, master.master_address());
+        ASSERT_TRUE(created.has_value());
+        auto client = created.value();
+        ASSERT_TRUE(client->MountSegment(segment.get(), kSegmentSize, "tcp"));
+        ASSERT_TRUE(client->RegisterLocalMemory(
+            allocator.getBase(), kSegmentSize, "cpu:0", false, false));
+        ASSERT_TRUE(client->MountLocalDiskSegment(true));
+
+        FileStorageConfig config = FileStorageConfig::FromEnvironment();
+        config.storage_backend_type = StorageBackendType::kBucket;
+        config.storage_filepath = data_path + "/issue4487";
+        config.local_buffer_size = 1024 * 1024;
+        fs::create_directories(config.storage_filepath);
+        FileStorage storage(config, client, endpoint);
+        BucketBackendConfig bucket_config;
+        bucket_config.bucket_keys_limit = 1;
+        storage.storage_backend_ =
+            std::make_shared<BucketStorageBackend>(config, bucket_config);
+        auto backend = storage.storage_backend_;
+        ASSERT_TRUE(backend->Init());
+
+        auto* source = static_cast<char*>(allocator.allocate(kValueSize));
+        auto* output =
+            static_cast<char*>(allocator.allocate(kValueSize + 2 * kGuardSize));
+        ASSERT_NE(source, nullptr);
+        ASSERT_NE(output, nullptr);
+        std::vector<Slice> input{{source, kValueSize}};
+        std::vector<Slice> destination{{output + kGuardSize, kValueSize}};
+        ReplicateConfig replicate;
+        replicate.replica_num = 1;
+        const std::string first_key = "issue4487-key";
+        std::string previous_value;
+        for (size_t round = 0; round < 2; ++round) {
+            const std::string key =
+                same_key || round == 0 ? first_key : "issue4487-fresh-key";
+            const auto scoped_key = TenantId::Default().MakeScopedKey(key);
+            std::string expected(kValueSize, '\0');
+            for (size_t i = 0; i < expected.size(); ++i) {
+                expected[i] = static_cast<char>(
+                    (i * 37 + (i >> 7) + (round + 1) * 113) & 255);
+            }
+            std::memcpy(source, expected.data(), expected.size());
+            ASSERT_TRUE(client->Put(key, input, replicate));
+            std::vector<OffloadTaskItem> tasks;
+            ASSERT_TRUE(client->OffloadObjectHeartbeat(true, tasks));
+            ASSERT_EQ(tasks.size(), 1u);
+            ASSERT_EQ(tasks.front().key, key);
+            ASSERT_EQ(tasks.front().size, static_cast<int64_t>(kValueSize));
+            ASSERT_TRUE(storage.OffloadObjects(tasks));
+
+            auto query = client->Query(key);
+            ASSERT_TRUE(query.has_value());
+            auto disk_count = [](const auto& replicas) {
+                size_t count = 0;
+                for (const auto& replica : replicas) {
+                    count += replica.is_local_disk_replica();
+                }
+                return count;
+            };
+            const auto before_rescan = disk_count(query->replicas);
+            std::memset(output, 0xA5, kValueSize + 2 * kGuardSize);
+            ASSERT_TRUE(client->Get(key, destination));
+            EXPECT_EQ(std::string(output + kGuardSize, kValueSize), expected);
+            EXPECT_EQ(std::string(output, kGuardSize),
+                      std::string(kGuardSize, static_cast<char>(0xA5)));
+            EXPECT_EQ(std::string(output + kGuardSize + kValueSize, kGuardSize),
+                      std::string(kGuardSize, static_cast<char>(0xA5)));
+
+            if (round == 1 && rescan) {
+                ASSERT_TRUE(storage.ReRegisterOffloadedObjects());
+            }
+            auto final_query = client->Query(key);
+            ASSERT_TRUE(final_query.has_value());
+            std::string disk_bytes(kValueSize, '\0');
+            std::unordered_map<std::string, Slice> read{
+                {scoped_key, {disk_bytes.data(), disk_bytes.size()}}};
+            ASSERT_TRUE(backend->BatchLoad(read));
+            std::vector<OffloadTaskItem> later_tasks;
+            ASSERT_TRUE(client->OffloadObjectHeartbeat(true, later_tasks));
+            LOG(INFO) << "REPRO4487 same_key=" << same_key
+                      << " rescan=" << rescan << " round=" << round
+                      << " disk_replicas_before_rescan=" << before_rescan
+                      << " disk_replicas_after_rescan="
+                      << disk_count(final_query->replicas)
+                      << " disk_matches_current=" << (disk_bytes == expected)
+                      << " disk_matches_previous="
+                      << (round == 1 && disk_bytes == previous_value)
+                      << " next_heartbeat_tasks=" << later_tasks.size();
+            EXPECT_EQ(disk_count(final_query->replicas), 1u)
+                << "Completed re-put must obtain a new disk replica";
+            EXPECT_EQ(disk_bytes, expected)
+                << "Disk bytes must belong to the current object incarnation";
+            if (round == 0) {
+                ASSERT_EQ(before_rescan, 1u);
+                ASSERT_EQ(disk_bytes, expected);
+                // Let the last read lease expire; exercise ordinary Remove.
+                std::this_thread::sleep_until(final_query->lease_timeout +
+                                              std::chrono::milliseconds(100));
+                ASSERT_TRUE(client->Remove(key));
+                auto removed = client->Query(key);
+                ASSERT_FALSE(removed.has_value());
+                ASSERT_EQ(removed.error(), ErrorCode::OBJECT_NOT_FOUND);
+                previous_value = expected;
+            }
+        }
+        EXPECT_TRUE(client->UnmountSegment(segment.get(), kSegmentSize));
+        EXPECT_TRUE(client->unregisterLocalMemory(allocator.getBase()));
+        allocator.deallocate(source, kValueSize);
+        allocator.deallocate(output, kValueSize + 2 * kGuardSize);
+    }
+
     void SetUp() override {
         google::InitGoogleLogging("FileStorageTest");
         FLAGS_logtostderr = true;
@@ -666,6 +793,18 @@ class FileStorageTest : public ::testing::Test {
     }
 };
 
+TEST_F(FileStorageTest, Issue4487ReputSameKeyMustOffloadNewBytes) {
+    RunIssue4487Scenario(true, false);
+}
+
+TEST_F(FileStorageTest, Issue4487FreshKeyControl) {
+    RunIssue4487Scenario(false, false);
+}
+
+TEST_F(FileStorageTest, Issue4487RescanMustNotReattachOldBytes) {
+    RunIssue4487Scenario(true, true);
+}
+
 TEST_F(FileStorageTest, SkippedOffloadAllDuplicatesReleaseSourceTasks) {
     RunSkippedOffloadScenario(2, false);
 }

<!-- gh-comment-id:6056524813 --> @feichai0017 commented on GitHub (Oct 8, 2026): Confirmed on upstream main **`b7595de3fab45a580e4ee8819c2a2940d43183cd`** in an independent Mooncake checkout, with production sources unchanged and no OrbitKV code. I also reproduced the stale-data **rescan path** mentioned in the report, with the scope described below. ### Reproduction results The test-only patch below uses the existing `FileStorageTest` fixture with the production embedded Master, `Client`, `FileStorage`, and `BucketStorageBackend`. It performs actual puts/gets and disk I/O. Offload heartbeats, `OffloadObjects`, and recovery rescan are invoked synchronously to make the sequence deterministic; the FileStorage background scheduler is not started. Environment: Debian 12 container, GCC 12.2, `USE_CUDA=OFF`, `USE_TENT=OFF`, classic TCP configured, bucket key limit 1, 4,109-byte values. This is a single-process **Store** reproducer, not a TENT or cross-host transport test. | Case | Results over separate process runs | Observation after the second put | | --- | --- | --- | | Remove + re-put the same key with different bytes | 3/3 reproduce; exit 1 | New value reads correctly from memory; no LOCAL_DISK replica; backend still returns the complete old value | | Same sequence, then call production `ReRegisterOffloadedObjects()` | 3/3 reproduce; exit 1 | LOCAL_DISK count changes from 0 to 1, while the backend still returns the complete old value | | Fresh-key control for the second put | 3/3 pass; exit 0 | One LOCAL_DISK replica and the correct new disk bytes | | Existing duplicate-only / mixed fresh-and-duplicate offload controls | Both pass; exit 0 | Existing skip/notification behavior remains intact | Each round has a different deterministic byte pattern. The test compares all bytes, poisons the `Client::Get` destination, checks both boundary guards, and uses ordinary `Remove` only after the read lease expires. The first offload and disk-byte check pass in all runs. The next heartbeat after the second offload returns zero tasks. Representative second-round observations: ```text same_key=1 rescan=0 disk_replicas_before_rescan=0 disk_replicas_after_rescan=0 disk_matches_current=0 disk_matches_previous=1 next_heartbeat_tasks=0 same_key=1 rescan=1 disk_replicas_before_rescan=0 disk_replicas_after_rescan=1 disk_matches_current=0 disk_matches_previous=1 next_heartbeat_tasks=0 same_key=0 rescan=0 disk_replicas_before_rescan=1 disk_replicas_after_rescan=1 disk_matches_current=1 disk_matches_previous=0 next_heartbeat_tasks=0 ``` The rescan result proves that the production rescan/registration path can attach a disk replica backed by old bytes to the new object. I have **not** tested an actual Master restart/HA transition, nor a subsequent user `Get` forced onto that disk replica. The new-value `Client::Get` check in this reproducer happens before rescan; the stale-byte observation is a direct backend `BatchLoad` afterward. ### Root cause and proposed fix One refinement to the trace in the issue: in this sequential reproduction, the stale entry is filtered **before `BatchOffload`**, by the `IsExist(key)` check in [`GroupOffloadingKeysByBucket`](https://github.com/kvcache-ai/Mooncake/blob/b7595de3fab45a580e4ee8819c2a2940d43183cd/mooncake-store/src/storage_backend.cpp#L2356). [`OffloadObjects`](https://github.com/kvcache-ai/Mooncake/blob/b7595de3fab45a580e4ee8819c2a2940d43183cd/mooncake-store/src/file_storage.cpp#L545) NACKs keys skipped during grouping, and the Master clears the offload task. Fixing only the duplicate check inside `BatchOffload` would miss this path. For rescan, `ReRegisterOffloadedObjects` reports persisted metadata through `NotifyOffloadSuccess`. With no pending offload task, the Master falls back to replica registration for the current key. `OffloadTaskItem` carries tenant/key/size, and `StorageObjectMetadata` carries location/size, but neither carries the object incarnation needed to distinguish the old and new values. Equal key and size are insufficient; both values in the reproducer deliberately have the same size. My proposed fix is to keep the change within the offload lifecycle and distinguish **same-incarnation retry** from **new-incarnation re-put**: 1. **Carry an authoritative object incarnation through the offload task and persisted per-object metadata.** Validate it against the current Master object before adding a disk replica. Completion/NACK handling must also match the offload task being completed, so a late old notification cannot clear a newer task. Recovery rescan must use the persisted incarnation rather than adopt an entry by key/size alone. 2. **Preserve duplicate idempotence for the same incarnation; conditionally replace older incarnations.** Apply the distinction at grouping and at the backend's locked duplicate/commit checks. Write the new data before publishing its mapping, and retire the old per-key entry safely. Do not delete an entire bucket containing unrelated live keys, or reclaim files while readers still own them. 3. **Make replacement/invalidation survive recovery.** Merely removing an in-memory map entry leaves the old bucket discoverable by `ScanMeta`. The persisted metadata needs a replacement/invalidation rule, with conservative handling of legacy entries whose incarnation cannot be established. If recovery is intended to preserve explicit removals across complete Master metadata loss, deletion/tombstone semantics must be defined as well; an incarnation field alone does not establish which objects remain live. This is a proposed design, **not a tested fix**. Before changing the wire/disk metadata, I would align the existing-bucket compatibility and recovery policy with maintainers. An alternative based on propagating `remove()` could work, but it would need equivalent conditional invalidation and ordering against in-flight offloads; a key-only delete or blind overwrite does not address late notifications and rescan. The fix should preserve #3135's valid retry behavior. #3889 addresses missing backing files; here the file exists and contains old bytes, so a missing-file check alone would not detect this case. Regression coverage should include the three cases below plus delayed old completion/NACK, same-incarnation retry, a shared bucket with another live key, and persisted-state reopen/rescan. <details> <summary>Reproduction commands and test-only patch</summary> In a standalone checkout of `b7595de3fab45a580e4ee8819c2a2940d43183cd`, save the diff below as `reproduction.patch` outside the checkout and apply it. With the normal Mooncake C++ build dependencies available (including Boost, msgpack, xxhash and zstd development headers/libraries): ```bash git apply /path/to/reproduction.patch git submodule update --init extern/pybind11 cmake -S . -B /tmp/mooncake-4487-build \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ \ '-DCMAKE_CXX_FLAGS_RELEASE=-O1 -DNDEBUG' \ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ -DUSE_CUDA=OFF -DUSE_TENT=OFF \ -DWITH_STORE=ON -DWITH_STORE_RUST=OFF \ -DBUILD_UNIT_TESTS=ON -DBUILD_EXAMPLES=OFF -DBUILD_BENCHMARK=OFF \ -DUSE_ETCD=OFF -DUSE_REDIS=OFF \ -DSTORE_USE_ETCD=OFF -DSTORE_USE_REDIS=OFF -DSTORE_USE_JEMALLOC=OFF cmake --build /tmp/mooncake-4487-build --target file_storage_test -j8 ``` For my run, missing development packages were extracted into an external private dependency prefix and passed via `CMAKE_PREFIX_PATH` / `-isystem`; no production code changes or CUDA stubs were used. The executable, runtime libraries, patch and configuration were frozen and SHA256-checked before and after testing. Patch SHA256: `ba13e352058e582841a1e59bb6328d4d942afcd7225798429657e9f465c713dc`. The following launches each new case three times with separate working directories and a timeout, then the two existing controls. It prints expected nonzero statuses instead of stopping at the first reproduced failure. Log/XML output stays outside the checkout. ```bash python3 - /tmp/mooncake-4487-build/mooncake-store/tests/file_storage_test <<'PYRUN' import os from pathlib import Path import subprocess import sys import tempfile binary = str(Path(sys.argv[1]).resolve()) root = Path(tempfile.mkdtemp(prefix="mooncake-4487-results-")) env = {k: v for k, v in os.environ.items() if not k.startswith(("MC_", "MOONCAKE_", "TENT_"))} env.update(MC_FORCE_TCP="1", MC_STORE_MEMCPY="0", MC_USE_TENT="0", GLOG_v="1") cases = { "fresh-key": "FileStorageTest.Issue4487FreshKeyControl", "same-key": "FileStorageTest.Issue4487ReputSameKeyMustOffloadNewBytes", "rescan": "FileStorageTest.Issue4487RescanMustNotReattachOldBytes", } jobs = [(f"{label}-{i}", case) for label, case in cases.items() for i in range(1, 4)] jobs.append(("existing-controls", "FileStorageTest.SkippedOffloadAllDuplicatesReleaseSourceTasks:" "FileStorageTest.SkippedOffloadFreshBatchKeepsSuccessNotifications")) for label, case in jobs: run = root / label run.mkdir() with (run / "run.log").open("wb") as log: result = subprocess.run( ["timeout", "--kill-after=5s", "120s", binary, "--gtest_filter=" + case, "--gtest_output=xml:" + str(run / "gtest.xml")], cwd=run, env=env, stdout=log, stderr=subprocess.STDOUT) print(label, "exit=" + str(result.returncode), "log=" + str(run / "run.log")) print("Results:", root) PYRUN ``` The patch adds tests only, using the fixture's existing friendship for deterministic calls to production offload/rescan methods. Both bug cases assert the desired correct behavior and therefore fail on the tested main revision. ```diff diff --git a/mooncake-store/tests/file_storage_test.cpp b/mooncake-store/tests/file_storage_test.cpp index 3f74941b..d4ad3c45 100644 --- a/mooncake-store/tests/file_storage_test.cpp +++ b/mooncake-store/tests/file_storage_test.cpp @@ -196,6 +196,133 @@ class FileStorageTest : public ::testing::Test { EXPECT_TRUE(client->unregisterLocalMemory(allocator.getBase())); } + void RunIssue4487Scenario(bool same_key, bool rescan) { + testing::InProcMaster master; + ASSERT_TRUE(master.Start(InProcMasterConfigBuilder() + .set_enable_offload(true) + .set_default_kv_lease_ttl(1000) + .set_root_fs_dir("") + .build())); + constexpr size_t kSegmentSize = 16 * 1024 * 1024; + constexpr size_t kValueSize = 4096 + 13; + constexpr size_t kGuardSize = 16; + std::unique_ptr<void, decltype(&std::free)> segment( + allocate_buffer_allocator_memory(kSegmentSize), &std::free); + ASSERT_NE(segment, nullptr); + SimpleAllocator allocator(kSegmentSize); + const auto endpoint = "127.0.0.1:" + std::to_string(getFreeTcpPort()); + auto created = Client::Create(endpoint, master.metadata_url(), "tcp", + std::nullopt, master.master_address()); + ASSERT_TRUE(created.has_value()); + auto client = created.value(); + ASSERT_TRUE(client->MountSegment(segment.get(), kSegmentSize, "tcp")); + ASSERT_TRUE(client->RegisterLocalMemory( + allocator.getBase(), kSegmentSize, "cpu:0", false, false)); + ASSERT_TRUE(client->MountLocalDiskSegment(true)); + + FileStorageConfig config = FileStorageConfig::FromEnvironment(); + config.storage_backend_type = StorageBackendType::kBucket; + config.storage_filepath = data_path + "/issue4487"; + config.local_buffer_size = 1024 * 1024; + fs::create_directories(config.storage_filepath); + FileStorage storage(config, client, endpoint); + BucketBackendConfig bucket_config; + bucket_config.bucket_keys_limit = 1; + storage.storage_backend_ = + std::make_shared<BucketStorageBackend>(config, bucket_config); + auto backend = storage.storage_backend_; + ASSERT_TRUE(backend->Init()); + + auto* source = static_cast<char*>(allocator.allocate(kValueSize)); + auto* output = + static_cast<char*>(allocator.allocate(kValueSize + 2 * kGuardSize)); + ASSERT_NE(source, nullptr); + ASSERT_NE(output, nullptr); + std::vector<Slice> input{{source, kValueSize}}; + std::vector<Slice> destination{{output + kGuardSize, kValueSize}}; + ReplicateConfig replicate; + replicate.replica_num = 1; + const std::string first_key = "issue4487-key"; + std::string previous_value; + for (size_t round = 0; round < 2; ++round) { + const std::string key = + same_key || round == 0 ? first_key : "issue4487-fresh-key"; + const auto scoped_key = TenantId::Default().MakeScopedKey(key); + std::string expected(kValueSize, '\0'); + for (size_t i = 0; i < expected.size(); ++i) { + expected[i] = static_cast<char>( + (i * 37 + (i >> 7) + (round + 1) * 113) & 255); + } + std::memcpy(source, expected.data(), expected.size()); + ASSERT_TRUE(client->Put(key, input, replicate)); + std::vector<OffloadTaskItem> tasks; + ASSERT_TRUE(client->OffloadObjectHeartbeat(true, tasks)); + ASSERT_EQ(tasks.size(), 1u); + ASSERT_EQ(tasks.front().key, key); + ASSERT_EQ(tasks.front().size, static_cast<int64_t>(kValueSize)); + ASSERT_TRUE(storage.OffloadObjects(tasks)); + + auto query = client->Query(key); + ASSERT_TRUE(query.has_value()); + auto disk_count = [](const auto& replicas) { + size_t count = 0; + for (const auto& replica : replicas) { + count += replica.is_local_disk_replica(); + } + return count; + }; + const auto before_rescan = disk_count(query->replicas); + std::memset(output, 0xA5, kValueSize + 2 * kGuardSize); + ASSERT_TRUE(client->Get(key, destination)); + EXPECT_EQ(std::string(output + kGuardSize, kValueSize), expected); + EXPECT_EQ(std::string(output, kGuardSize), + std::string(kGuardSize, static_cast<char>(0xA5))); + EXPECT_EQ(std::string(output + kGuardSize + kValueSize, kGuardSize), + std::string(kGuardSize, static_cast<char>(0xA5))); + + if (round == 1 && rescan) { + ASSERT_TRUE(storage.ReRegisterOffloadedObjects()); + } + auto final_query = client->Query(key); + ASSERT_TRUE(final_query.has_value()); + std::string disk_bytes(kValueSize, '\0'); + std::unordered_map<std::string, Slice> read{ + {scoped_key, {disk_bytes.data(), disk_bytes.size()}}}; + ASSERT_TRUE(backend->BatchLoad(read)); + std::vector<OffloadTaskItem> later_tasks; + ASSERT_TRUE(client->OffloadObjectHeartbeat(true, later_tasks)); + LOG(INFO) << "REPRO4487 same_key=" << same_key + << " rescan=" << rescan << " round=" << round + << " disk_replicas_before_rescan=" << before_rescan + << " disk_replicas_after_rescan=" + << disk_count(final_query->replicas) + << " disk_matches_current=" << (disk_bytes == expected) + << " disk_matches_previous=" + << (round == 1 && disk_bytes == previous_value) + << " next_heartbeat_tasks=" << later_tasks.size(); + EXPECT_EQ(disk_count(final_query->replicas), 1u) + << "Completed re-put must obtain a new disk replica"; + EXPECT_EQ(disk_bytes, expected) + << "Disk bytes must belong to the current object incarnation"; + if (round == 0) { + ASSERT_EQ(before_rescan, 1u); + ASSERT_EQ(disk_bytes, expected); + // Let the last read lease expire; exercise ordinary Remove. + std::this_thread::sleep_until(final_query->lease_timeout + + std::chrono::milliseconds(100)); + ASSERT_TRUE(client->Remove(key)); + auto removed = client->Query(key); + ASSERT_FALSE(removed.has_value()); + ASSERT_EQ(removed.error(), ErrorCode::OBJECT_NOT_FOUND); + previous_value = expected; + } + } + EXPECT_TRUE(client->UnmountSegment(segment.get(), kSegmentSize)); + EXPECT_TRUE(client->unregisterLocalMemory(allocator.getBase())); + allocator.deallocate(source, kValueSize); + allocator.deallocate(output, kValueSize + 2 * kGuardSize); + } + void SetUp() override { google::InitGoogleLogging("FileStorageTest"); FLAGS_logtostderr = true; @@ -666,6 +793,18 @@ class FileStorageTest : public ::testing::Test { } }; +TEST_F(FileStorageTest, Issue4487ReputSameKeyMustOffloadNewBytes) { + RunIssue4487Scenario(true, false); +} + +TEST_F(FileStorageTest, Issue4487FreshKeyControl) { + RunIssue4487Scenario(false, false); +} + +TEST_F(FileStorageTest, Issue4487RescanMustNotReattachOldBytes) { + RunIssue4487Scenario(true, true); +} + TEST_F(FileStorageTest, SkippedOffloadAllDuplicatesReleaseSourceTasks) { RunSkippedOffloadScenario(2, false); } ``` </details>

bsakaguchi referenced this issue 2026-10-29 21:32:17 +00:00

[PR #1140] [MERGED] [mooncake-store]: prevent orphaned bucket data files from leaking dis… #2035

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

te-coro

staryxchen-tent-capability-research

feature/musa-ep-jit

zxpdemonio/fix-tent-bootstrap-admission

add-store-rs-tone-tests

scatter-rdma-completion-pr2

P2P-Mooncake-Store

release/v0.3.14

feat/tent-rdma-adaptive-adapter

fix/ep-local-counter-gpu-scope

codex/kvcs-oplog-eviction

give-me-a-star

cruz/mooncake-kvcs-lowlevel-pr1-20261008

ylt-handshake-limits

ci/rocm-e2e-tent

sima/docs-pypi-downloads

fix-ci/hicache-mooncake-metadata-loopback

sima/cuda-driver-loading

sima/ci-performance-gate

fix/tent-rdma-unknown-topology-rail-pairing

ci/tent-e2e-dual-job

mc-patch-1

codex/p2p-merge-prerequisites

sima/store-hca-numa-placement

codex/p2p-merge-cli

codex/p2p-merge-ipv6

copilot/fix-ctests-job-failure

czh/pg-device-collectives-pr6

dependabot/cargo/mooncake-store-rs/pyo3-0.29.0

czh/pg-device-collectives-pr5

codex/segment-pool-master-cutover

sima/nightly-ci-recovery

ci/tent-params

stmatengss-patch-1

dependabot/cargo/mooncake-store-rs/rand-0.8.8

dependabot/cargo/mooncake-store-rs/opentelemetry_sdk-0.32.1

dependabot/cargo/mooncake-store-rs/xxhash-rust-0.8.16

codex/migrate-store-rest-service

fix/tone-wheel-arch-selection

czh/pg-device-collectives-pr4

czh/pg-device-collectives-pr3

ketong/fix_moe_tc

copilot/fix-test-wheel-ubuntu-job

fix/ci-wheel-torch-pg-pin

codex/migrate-structured-object-storage

codex/migrate-allocator-python

codex/migrate-vllm-python

codex/migrate-shared-segment

ci/fix_sgl_eval_install

codex/segment-pool-nof-placement-cleanup

feat/pip-native-release

copilot/bugfix-ha-standby-promotion

copilot/mooncake-store-transport-settings

copilot/fix-tensor-support-issues

copilot/optimize-rl-direction

copilot/mooncake-storeagentic-workflow

copilot/fix-client-reconnect-timeout

copilot/mooncake-error-debugging

copilot/mooncake-ipv6-support

feat/nccl-host-python-api

dependabot/go_modules/mooncake-p2p-store/src/p2pstore/google.golang.org/grpc-1.82.1

copilot/migrate-and-move-operations

copilot/migrate-and-move-operations-again

refactor/centralize-env-vars

idhanani/mooncake-retain-groups

copilot/codeownerreviewer

copilot/give-feedback-on-issue-2123

copilot/create-issue-rfc

copilot/break-cuda-monopoly-logic

copilot/compare-cudamemcpy-cu12-cu13

copilot/report-ci-error-issue

copilot/research-commit-history-analysis

copilot/explain-mooncake-store-service

copilot/analyze-project-functionality

copilot/sub-pr-1680

copilot/review-pr-1680

copilot/sub-pr-1597

copilot/fix-github-actions-job-failure

copilot/move-test-sglang-integration-job

v0.3.14

v0.3.14-pre-4

v0.3.14-pre-3

v0.3.14-pre-2

v0.3.14-pre

v0.3.14-rc1

v0.3.13.post1

v0.3.13-rc2

v0.3.13-rc1

v0.3.13

v0.3.12.post1

v0.3.12

v0.3.11-rc1

v0.3.11.post1

v0.3.11

v0.3.10.post2

v0.3.10.post1

v0.3.10

v0.3.9

v0.3.8.post1

v0.3.8

v0.3.7.post2

v0.3.7.post1

v0.3.7

v0.3.6.post1

v0.3.6

v0.3.5

v0.3.4.post2

v0.3.4.post1

v0.3.4

v0.3.3.post2

v0.3.3.post1

v0.3.3

v0.3.2.post1

v0.3.2

v0.3.1

v0.3.0

v0.3.0-beta

v0.3.0-alpha

v0.2.0

Labels

Clear labels   

ascend

  

auto-closed

  

bug

  

bug

  

CI/Build

  

Common

  

documentation

  

enhancement

  

feature request

  

good first issue

  

help wanted

  

high priority

  

MetaX

  

nightly-failure

  

pull-request

Mirrored from GitHub Pull Request

  

PyTorch Backend

  

question

  

RFC

  

Roadmap

  

stale

  

Store

  

Tests

  

Transfer Engine

No labels

ascend

auto-closed

bug

bug

CI/Build

Common

documentation

enhancement

feature request

good first issue

help wanted

high priority

MetaX

nightly-failure

pull-request

PyTorch Backend

question

RFC

Roadmap

stale

Store

Tests

Transfer Engine

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

kvcache-ai/Mooncake#1140

No description provided.