[GH-ISSUE #4529] [Bug]: Expired Get/BatchGet reads can populate the local hot cache #1154

New issue

Open

opened 2026-10-29 21:23:23 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-29 21:23:23 +00:00

Owner

Originally created by @jacklin78911-collab on GitHub (Oct 9, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4529

Problem

With local hot caching enabled, Client::Get and both BatchGet paths promote completed READ data before validating the query's read lease. They can return LEASE_EXPIRED while leaving that rejected payload in the hot cache. A later read with a live lease and a same-size replica can return success using those cached bytes.

Audited main: dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2.

Mechanism

Single Get: checksum validation, then admission/ProcessSlicesAsync, then IsLeaseExpired.
Preferred same-node BatchGet: promotion happens inside the segment completion loop, before the final batch lease check.
Regular BatchGet: the same ordering.
SubmitPutTask synchronously copies the payload, then queues publication with a cache-generation token. That token protects against local invalidation during a fill; it does not validate the query lease.
RedirectToHotCache looks up the key and size. Thus the rejected payload can affect a subsequent successful read, not just the already-failed request.

Reproduction and observed results

I added the test below to the existing client_local_hot_cache_test.cpp fixture. It uses the real Client/cache paths, registered memory, an in-process master, TCP for the preferred batch path, and SHM hot caching (8 KiB, 4 KiB blocks, admission threshold 1). Single Get and regular BatchGet use the local memcpy path.

For each of single Get, regular BatchGet, and preferred same-node BatchGet:

Read 64 B bytes using an already-expired QueryResult.
Confirm the read returns LEASE_EXPIRED.
Observe admission count 1, and wait for the key to be published in the cache.
Change the registered source buffer in place to 64 A bytes and provide a fresh, live QueryResult.
The fresh Get returns success but 64 B bytes from the rejected read.

All three modes exhibited both failures in the test: nonzero admission count and wrong payload on the subsequent read. There were no transfer failures in the final registered-buffer reproduction.

This deliberately controls the metadata and source buffer; it does not simulate a master-driven eviction or establish the frequency of this failure in production. It proves that an expired read can publish a cache entry and that entry is subsequently usable. Region reuse after lease expiry is the concerning production case.

Build note: current main needed the existing unrelated build prerequisite from #4513 (Environ API removal). I applied those changes only in the validation copy. Neither the Client read paths nor hot-cache implementation was modified.

cmake --build build --target client_local_hot_cache_test -j3
build/mooncake-store/tests/client_local_hot_cache_test \
  --gtest_filter=LocalHotCacheTest.ExpiredReadsMustNotPopulateHotCache
# build succeeds; the new test fails in all three modes as described above

Reproduction test (append inside the existing mooncake::testing namespace)

TEST_F(LocalHotCacheTest, ExpiredReadsMustNotPopulateHotCache) {
    setenv("MC_STORE_LOCAL_HOT_CACHE_SIZE", "8192", 1);
    setenv("MC_STORE_LOCAL_HOT_BLOCK_SIZE", "4096", 1);
    setenv("MC_STORE_LOCAL_HOT_CACHE_USE_SHM", "1", 1);
    setenv("MC_STORE_LOCAL_HOT_ADMISSION_THRESHOLD", "1", 1);
    setenv("MC_STORE_MEMCPY", "1", 1);
    auto created = CreateTestClient("127.0.0.1");
    ASSERT_TRUE(created.has_value());
    auto client = *created;
    ASSERT_TRUE(client->IsHotCacheEnabled());
    ASSERT_TRUE(client->GetHotCache()->IsShm());

    for (int mode = 0; mode != 3; ++mode) {
        SCOPED_TRACE(mode);
        const std::string key = "expired-read-" + std::to_string(mode);
        std::string source(64, 'B');
        std::string destination(64, '?');
        ASSERT_TRUE(client->RegisterLocalMemory(source.data(), source.size(),
                                                "cpu:0").has_value());
        ASSERT_TRUE(client->RegisterLocalMemory(destination.data(),
                                                destination.size(), "cpu:0",
                                                false, false).has_value());
        MemoryDescriptor memory;
        memory.buffer_descriptor.transport_endpoint_ =
            client->GetTransportEndpoint();
        memory.buffer_descriptor.buffer_address_ =
            reinterpret_cast<uintptr_t>(source.data());
        memory.buffer_descriptor.size_ = source.size();
        Replica::Descriptor replica;
        replica.id = 1;
        replica.status = ReplicaStatus::COMPLETE;
        replica.descriptor_variant = memory;
        QueryResult expired({replica}, std::chrono::steady_clock::now() -
                                           std::chrono::seconds(1));
        std::vector<Slice> slices{{destination.data(), destination.size()}};
        tl::expected<void, ErrorCode> result;
        if (mode == 0) {
            result = client->Get(key, expired, slices);
        } else {
            std::unordered_map<std::string, std::vector<Slice>> batch_slices{
                {key, slices}};
            result = client->BatchGet({key}, {expired}, batch_slices,
                                      mode == 2).at(0);
        }
        ASSERT_FALSE(result.has_value());
        ASSERT_EQ(result.error(), ErrorCode::LEASE_EXPIRED);
        const auto count = client->GetAdmissionCount(key);
        EXPECT_EQ(count, 0) << "Rejected data reached cache admission";

        // Evidence for the baseline: wait for the already-submitted fill,
        // then give the same key a fresh descriptor with the correct bytes.
        if (count != 0) {
            const auto deadline = std::chrono::steady_clock::now() +
                                  std::chrono::seconds(2);
            while (!client->GetHotCache()->HasHotKey(key) &&
                   std::chrono::steady_clock::now() < deadline) {
                std::this_thread::yield();
            }
            ASSERT_TRUE(client->GetHotCache()->HasHotKey(key));
            std::memset(source.data(), 'A', source.size());
            QueryResult fresh({replica}, std::chrono::steady_clock::now() +
                                            std::chrono::seconds(60));
            auto retry = client->Get(key, fresh, slices);
            ASSERT_TRUE(retry.has_value());
            EXPECT_EQ(destination, source)
                << "Fresh read returned bytes from the rejected read";
        }
        ASSERT_TRUE(client->unregisterLocalMemory(destination.data(), false)
                        .has_value());
        ASSERT_TRUE(client->unregisterLocalMemory(source.data()).has_value());
    }
    unsetenv("MC_STORE_LOCAL_HOT_CACHE_SIZE");
    unsetenv("MC_STORE_LOCAL_HOT_BLOCK_SIZE");
    unsetenv("MC_STORE_LOCAL_HOT_CACHE_USE_SHM");
    unsetenv("MC_STORE_LOCAL_HOT_ADMISSION_THRESHOLD");
    unsetenv("MC_STORE_MEMCPY");
}

Expected behavior / minimal fix direction

Data from a read rejected for an expired lease must not be admitted or published into the local hot cache. Move promotion behind lease validation, preserving transfer/checksum errors and the existing final aggregate lease semantics for batch reads. This requires ordering the existing operations, not adding a new cache policy or transport mechanism.

#2471 protects detached fills against local invalidation; #4334 addresses block-key access races. Neither addresses admission before lease validation. The broader object-identity/region-reuse discussion in #4489 is related, but this issue isolates the Client cache side effect of a read that it already rejects.

AI assistance: Codex assisted source inspection and the reproduction. No production read/cache fix is included in this report.

Originally created by @jacklin78911-collab on GitHub (Oct 9, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4529 ### Problem With local hot caching enabled, `Client::Get` and both `BatchGet` paths promote completed READ data before validating the query's read lease. They can return `LEASE_EXPIRED` while leaving that rejected payload in the hot cache. A later read with a live lease and a same-size replica can return success using those cached bytes. Audited main: `dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2`. ### Mechanism - [Single Get](https://github.com/kvcache-ai/Mooncake/blob/dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2/mooncake-store/src/client_service.cpp#L1423-L1446): checksum validation, then admission/`ProcessSlicesAsync`, then `IsLeaseExpired`. - [Preferred same-node BatchGet](https://github.com/kvcache-ai/Mooncake/blob/dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2/mooncake-store/src/client_service.cpp#L1637-L1662): promotion happens inside the segment completion loop, before the final batch lease check. - [Regular BatchGet](https://github.com/kvcache-ai/Mooncake/blob/dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2/mooncake-store/src/client_service.cpp#L1882-L1914): the same ordering. - [SubmitPutTask](https://github.com/kvcache-ai/Mooncake/blob/dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2/mooncake-store/src/local_hot_cache.cpp#L518-L561) synchronously copies the payload, then queues publication with a cache-generation token. That token protects against local invalidation during a fill; it does not validate the query lease. - [RedirectToHotCache](https://github.com/kvcache-ai/Mooncake/blob/dcddb56c0cddc96f41eeb8dd82e5bd9f497183a2/mooncake-store/src/client_service.cpp#L1932-L1960) looks up the key and size. Thus the rejected payload can affect a subsequent successful read, not just the already-failed request. ### Reproduction and observed results I added the test below to the existing `client_local_hot_cache_test.cpp` fixture. It uses the real Client/cache paths, registered memory, an in-process master, TCP for the preferred batch path, and SHM hot caching (8 KiB, 4 KiB blocks, admission threshold 1). Single Get and regular BatchGet use the local memcpy path. For each of single Get, regular BatchGet, and preferred same-node BatchGet: 1. Read 64 `B` bytes using an already-expired `QueryResult`. 2. Confirm the read returns `LEASE_EXPIRED`. 3. Observe admission count **1**, and wait for the key to be published in the cache. 4. Change the registered source buffer in place to 64 `A` bytes and provide a fresh, live `QueryResult`. 5. The fresh Get returns **success but 64 `B` bytes** from the rejected read. All three modes exhibited both failures in the test: nonzero admission count and wrong payload on the subsequent read. There were no transfer failures in the final registered-buffer reproduction. This deliberately controls the metadata and source buffer; it does not simulate a master-driven eviction or establish the frequency of this failure in production. It proves that an expired read can publish a cache entry and that entry is subsequently usable. Region reuse after lease expiry is the concerning production case. **Build note:** current main needed the existing unrelated build prerequisite from #4513 (Environ API removal). I applied those changes only in the validation copy. Neither the Client read paths nor hot-cache implementation was modified. ```bash cmake --build build --target client_local_hot_cache_test -j3 build/mooncake-store/tests/client_local_hot_cache_test \ --gtest_filter=LocalHotCacheTest.ExpiredReadsMustNotPopulateHotCache # build succeeds; the new test fails in all three modes as described above ``` <details> <summary>Reproduction test (append inside the existing mooncake::testing namespace)</summary> ```cpp TEST_F(LocalHotCacheTest, ExpiredReadsMustNotPopulateHotCache) { setenv("MC_STORE_LOCAL_HOT_CACHE_SIZE", "8192", 1); setenv("MC_STORE_LOCAL_HOT_BLOCK_SIZE", "4096", 1); setenv("MC_STORE_LOCAL_HOT_CACHE_USE_SHM", "1", 1); setenv("MC_STORE_LOCAL_HOT_ADMISSION_THRESHOLD", "1", 1); setenv("MC_STORE_MEMCPY", "1", 1); auto created = CreateTestClient("127.0.0.1"); ASSERT_TRUE(created.has_value()); auto client = *created; ASSERT_TRUE(client->IsHotCacheEnabled()); ASSERT_TRUE(client->GetHotCache()->IsShm()); for (int mode = 0; mode != 3; ++mode) { SCOPED_TRACE(mode); const std::string key = "expired-read-" + std::to_string(mode); std::string source(64, 'B'); std::string destination(64, '?'); ASSERT_TRUE(client->RegisterLocalMemory(source.data(), source.size(), "cpu:0").has_value()); ASSERT_TRUE(client->RegisterLocalMemory(destination.data(), destination.size(), "cpu:0", false, false).has_value()); MemoryDescriptor memory; memory.buffer_descriptor.transport_endpoint_ = client->GetTransportEndpoint(); memory.buffer_descriptor.buffer_address_ = reinterpret_cast<uintptr_t>(source.data()); memory.buffer_descriptor.size_ = source.size(); Replica::Descriptor replica; replica.id = 1; replica.status = ReplicaStatus::COMPLETE; replica.descriptor_variant = memory; QueryResult expired({replica}, std::chrono::steady_clock::now() - std::chrono::seconds(1)); std::vector<Slice> slices{{destination.data(), destination.size()}}; tl::expected<void, ErrorCode> result; if (mode == 0) { result = client->Get(key, expired, slices); } else { std::unordered_map<std::string, std::vector<Slice>> batch_slices{ {key, slices}}; result = client->BatchGet({key}, {expired}, batch_slices, mode == 2).at(0); } ASSERT_FALSE(result.has_value()); ASSERT_EQ(result.error(), ErrorCode::LEASE_EXPIRED); const auto count = client->GetAdmissionCount(key); EXPECT_EQ(count, 0) << "Rejected data reached cache admission"; // Evidence for the baseline: wait for the already-submitted fill, // then give the same key a fresh descriptor with the correct bytes. if (count != 0) { const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); while (!client->GetHotCache()->HasHotKey(key) && std::chrono::steady_clock::now() < deadline) { std::this_thread::yield(); } ASSERT_TRUE(client->GetHotCache()->HasHotKey(key)); std::memset(source.data(), 'A', source.size()); QueryResult fresh({replica}, std::chrono::steady_clock::now() + std::chrono::seconds(60)); auto retry = client->Get(key, fresh, slices); ASSERT_TRUE(retry.has_value()); EXPECT_EQ(destination, source) << "Fresh read returned bytes from the rejected read"; } ASSERT_TRUE(client->unregisterLocalMemory(destination.data(), false) .has_value()); ASSERT_TRUE(client->unregisterLocalMemory(source.data()).has_value()); } unsetenv("MC_STORE_LOCAL_HOT_CACHE_SIZE"); unsetenv("MC_STORE_LOCAL_HOT_BLOCK_SIZE"); unsetenv("MC_STORE_LOCAL_HOT_CACHE_USE_SHM"); unsetenv("MC_STORE_LOCAL_HOT_ADMISSION_THRESHOLD"); unsetenv("MC_STORE_MEMCPY"); } ``` </details> ### Expected behavior / minimal fix direction Data from a read rejected for an expired lease must not be admitted or published into the local hot cache. Move promotion behind lease validation, preserving transfer/checksum errors and the existing final aggregate lease semantics for batch reads. This requires ordering the existing operations, not adding a new cache policy or transport mechanism. #2471 protects detached fills against local invalidation; #4334 addresses block-key access races. Neither addresses admission before lease validation. The broader object-identity/region-reuse discussion in #4489 is related, but this issue isolates the Client cache side effect of a read that it already rejects. AI assistance: Codex assisted source inspection and the reproduction. No production read/cache fix is included in this report.

bsakaguchi commented

2026-10-29 21:23:25 +00:00

Author

Owner

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

Thanks for opening this issue, @jacklin78911-collab!

Field	Value
Issue	#4529
GitHub user ID	`251732545`
Reporter	@jacklin78911-collab

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.

@github-actions[bot] commented on GitHub (Oct 9, 2026): Thanks for opening this issue, @jacklin78911-collab! | Field | Value | |-------|-------| | **Issue** | #4529 | | **GitHub user ID** | `251732545` | | **Reporter** | @jacklin78911-collab | 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 referenced this issue

2026-10-29 21:32:19 +00:00

[PR #1154] [MERGED] Improve AMD HIP support with hipify-perl #2042