kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #4508] [Bug]: `CleanupStaleHandles` removes keys without checking lease, causing `lookup` hit → `load` -704 race #1148

New issue

Open

opened 2026-10-29 21:23:20 +00:00 by bsakaguchi · 7 comments

bsakaguchi commented

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

Owner

Originally created by @xuyucheng220 on GitHub (Oct 8, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4508

Bug Report

Mooncake Issue Report: `CleanupStaleHandles` removes keys without checking lease, causing `lookup` hit → `load` -704 race

Summary

In Mooncake store (both the deployed v0.3.11.post1 and the current main branch), CleanupStaleHandles / ClearInvalidHandles erase object metadata based solely on has_invalid_mem_handle() && is_completed(), without checking whether the object's lease is still active. Because ExistKey (lookup) and GetReplicaList (load) each only GrantLease and do not inc_refcnt, a key can be reported as a hit by ExistKey and then erased by ClearInvalidHandles before GetReplicaList runs, producing OBJECT_NOT_FOUND (-704) on a key that was just reported as present.

This is a time-of-check-to-time-of-use race between the background ClientMonitorFunc cleanup and the two-step (lookup → load) access pattern used by vLLM-Ascend's KV pool. It is not fixed on main.

Environment

Mooncake store: v0.3.11.post1 (production) and main branch (verified via GitHub raw source)
Workload: vLLM-Ascend KV transfer (MooncakeConnectorV1 + AscendStoreConnector), GLM-5.2-w8a8c8, 8–12 node clusters, TP16/DP2 prefill, TP4/DP8 decode
Three independent clusters, each with its own mooncake master HA (etcd-backed leadership)

Observed symptom in production

Across three production clusters over ~7 days (2026-10-01 to 2026-10-08), batch_get_into_multi_buffers returns -704 (OBJECT_NOT_FOUND) for keys that batch_is_exist (ExistKey) had just reported as present:

Cluster	Master	-704 count	-703 count	-200 count	Eviction (delta)
xpod06	.76	4412	0	16	4 cycles, ~47000 keys / ~615 GB each
xpod07	.171	4892	2	0	1 cycle, 70519 keys / 922 GB
xpod19	.75	319	0	0	4 cycles, ~47000 keys / ~615 GB each

-704 accounts for >99.6% of all non-zero return codes from get.
Eviction: Success/Attempts is delta per Admin Metrics cycle (~10 s), not cumulative (verified: consecutive non-zero rows are 1/1 with 0/0 in between, not monotonically increasing).
After a manual leader switchover (metadata cleared, new leader starts fresh), -704 drops to near-zero and then gradually accumulates again as the cluster runs. This correlates with buffer invalidation (has_invalid_mem_handle) accumulating over time, not with eviction pressure.

Root cause (source-level)

1. `ExistKey` and `GetReplicaList` only `GrantLease`, never `inc_refcnt`

master_service.cpp:

// ExistKey (lookup)
auto MasterService::ExistKey(const std::string& key) ... {
    MetadataAccessorRO accessor(this, key);
    if (!accessor.Exists()) return false;
    const auto& metadata = accessor.Get();
    if (metadata.HasReplica(&Replica::fn_is_completed)) {
        metadata.GrantLease(default_kv_lease_ttl_, default_kv_soft_pin_ttl_);
        return true;
    }
    return false;
}

// GetReplicaList (load)
auto MasterService::GetReplicaList(const std::string& key) ... {
    MetadataAccessorRO accessor(this, key);
    if (!accessor.Exists()) return tl::make_unexpected(ErrorCode::OBJECT_NOT_FOUND); // -704
    const auto& metadata = accessor.Get();
    std::vector<Replica::Descriptor> replica_list;
    metadata.VisitReplicas(&Replica::fn_is_completed, [&replica_list](const Replica& replica) {
        replica_list.emplace_back(replica.get_descriptor());
    });
    if (replica_list.empty())
        return tl::make_unexpected(ErrorCode::REPLICA_IS_NOT_READY); // -703
    metadata.GrantLease(default_kv_lease_ttl_, default_kv_soft_pin_ttl_);  // "so it will not be removed when the client is reading it"
    ...
}

GrantLease only extends lease_timeout; it does not touch refcnt:

void GrantLease(const uint64_t ttl, const uint64_t soft_ttl) const {
    SpinLocker locker(&lock);
    lease_timeout = std::max(lease_timeout, now + std::chrono::milliseconds(ttl));
    if (soft_pin_timeout) { soft_pin_timeout = std::max(*soft_pin_timeout, now + ...); }
}

2. `BatchEvict` (normal eviction) DOES respect lease — this is NOT the culprit

// BatchEvict first pass skips objects whose lease is not expired
if (it->second.IsHardPinned() ||
    !it->second.IsLeaseExpired(now) ||      // <- lease guard
    it->second.IsSoftPinned(now) ||
    !can_evict_replicas(it->second)) {       // can_evict requires refcnt==0
    ++it; continue;
}

Because ExistKey/GetReplicaList both GrantLease, a key that was just looked up is protected from BatchEvict for the lease TTL. Eviction alone cannot explain lookup-hit → load--704.

3. `CleanupStaleHandles` / `ClearInvalidHandles` do NOT check lease — this IS the culprit

bool MasterService::CleanupStaleHandles(
    ObjectMetadata& metadata,
    const std::unordered_set<UUID, boost::hash<UUID>>& alive_clients) {
    metadata.EraseReplicas([&alive_clients](const Replica& replica) {
        return (replica.has_invalid_mem_handle() ||
                replica.has_invalid_nof_handle() ||
                replica.has_stale_local_disk_client(alive_clients)) &&
               replica.is_completed();
    });
    return !metadata.IsValid();
}

void MasterService::ClearInvalidHandles(...) {
    for (size_t i = 0; i < kNumShards; i++) {
        MetadataShardAccessorRW shard(this, i);
        auto it = shard->metadata.begin();
        while (it != shard->metadata.end()) {
            if (CleanupStaleHandles(it->second, alive_clients)) {
                shard->processing_keys.erase(it->first);
                ...
                it = shard->metadata.erase(it);   // <- key removed, lease never consulted
            } else { ++it; }
        }
    }
}

CleanupStaleHandles removes replicas whose memory handle is invalid (i.e. the segment was unmounted / buffer allocator expired) regardless of whether the object's lease is still active. When all replicas are removed and IsValid() becomes false, the key is erased from metadata.

4. `ClientMonitorFunc` triggers this cleanup on a background thread

void MasterService::ClientMonitorFunc() {
    while (client_monitor_running_) {
        // detect client_expired (no ping within client_live_ttl_sec_)
        ...
        // unmount expired clients' segments
        ...
        // Always clean up invalid handles when there are expired clients
        ClearInvalidHandles();   // <- runs without lease check
    }
}

Race window

T1  ExistKey(key):
      HasReplica(fn_is_completed) == true
      GrantLease(ttl)            // lease now valid until T1+ttl
      return true                // lookup HIT

    [background] ClientMonitorFunc: a client's segment was just unmounted,
    replica.has_invalid_mem_handle() became true for `key`
    ClearInvalidHandles -> CleanupStaleHandles:
      EraseReplicas(has_invalid_mem_handle && is_completed)  // lease NOT checked
      IsValid() == false -> metadata.erase(key)

T2  GetReplicaList(key):
      accessor.Exists() == false   // key erased
      return OBJECT_NOT_FOUND (-704)

The lease that ExistKey just granted provides no protection against CleanupStaleHandles, because the cleanup path never calls IsLeaseExpired().

Why this matters for vLLM-Ascend

vLLM-Ascend's KV pool uses a two-step access:

lookup() (calls batch_is_exist) to find the longest cached prefix
load() (calls batch_get_into_multi_buffers) to fetch the cached KV

When lookup reports a hit but load returns -704, the connector treats it as a load failure and recomputes the prefix. In the current vLLM-Ascend SAVE-CLAMP patch (a site-local poison-KV guard), a -704 load failure triggers record_failed_blocks → _record_load_failure → SAVE-CLAMP truncates the recomputed KV before writeback. Because -704 means "missing" (the data is genuinely gone), the recomputed KV is correct and safe to write back — but the truncation discards it, so the next request hits -704 again. Over time this prevents the cache from being rebuilt, inflating TTFT for long multi-turn conversations.

Even without the site-local patch, the lookup-hit → load--704 race itself is a correctness/usability issue: the client receives contradictory answers from two consecutive RPCs for the same key within milliseconds.

Verification that `main` is also affected

Fetched mooncake-store/src/master_service.cpp from the main branch. The CleanupStaleHandles wrapper and ClearInvalidHandles on main:

void MasterService::ClearInvalidHandles(
    const std::unordered_set<UUID, boost::hash<UUID>>& retaining_clients) {
    (void)ClearStaleHandles([&retaining_clients](const Replica& replica) {
        return (replica.has_invalid_mem_handle() ||
                replica.has_invalid_nof_handle() ||
                replica.has_stale_local_disk_client(retaining_clients)) &&
               replica.is_completed();
    });
}

ClearStaleHandles on main adds multi-tenant and quota accounting, but the predicate still does not include IsLeaseExpired. The lease guard is present in BatchReplicaClear (L4103) and PutStart (L6055) on main, but not in the ClearInvalidHandles/CleanupStaleHandles path.

Suggested fix

Add a lease check at the top of CleanupStaleHandles so that an active lease delays cleanup of stale-but-still-leased objects:

bool MasterService::CleanupStaleHandles(
    ObjectMetadata& metadata,
    const std::unordered_set<UUID, boost::hash<UUID>>& alive_clients) {
    // Do not remove replicas for an object whose lease is still active.
    // ExistKey/GetReplicaList grant a lease so the object survives the
    // lookup -> load window; honoring that lease here prevents the
    // lookup-hit -> load -704 race.
    if (!metadata.IsLeaseExpired()) {
        return false;
    }
    metadata.EraseReplicas([&alive_clients](const Replica& replica) {
        return (replica.has_invalid_mem_handle() ||
                replica.has_invalid_nof_handle() ||
                replica.has_stale_local_disk_client(alive_clients)) &&
               replica.is_completed();
    });
    return !metadata.IsValid();
}

Trade-offs

Benefit: eliminates the lookup-hit → load--704 race; the two RPCs become consistent for the duration of the lease.
Cost: stale handles (buffer already gone) are retained until lease expiry (default TTL is milliseconds-to-seconds). The memory for the metadata entry is tiny; the actual buffer is already freed (has_invalid_mem_handle means the allocator is invalid). So the cost is a brief delay in metadata cleanup, not a memory leak.
Alternative: if delaying cleanup of truly-dead buffers is undesirable, a narrower fix would be to have GetReplicaList re-validate that the returned replica descriptors still have valid handles before returning, and retry/skip invalid ones instead of returning -704 for the whole key. But this is more invasive.

Questions for the maintainers

Is there a design reason CleanupStaleHandles intentionally ignores the lease (e.g. "buffer gone = data gone, no point keeping metadata")? If so, would you consider the narrower alternative (re-validate in GetReplicaList)?
Is the two-step ExistKey → GetReplicaList pattern (with GrantLease on both) an intended usage contract? If yes, the lease should be honored by all cleanup paths.
Would a PR adding the IsLeaseExpired guard to CleanupStaleHandles be acceptable, or do you prefer the GetReplicaList-side re-validation?

Reproduction

The race is inherently timing-dependent, but the conditions that make it frequent in production are:

Long-running clusters where worker segments are occasionally unmounted/remounted (load fluctuation, rolling restarts)
High lookup rate (vLLM-Ascend issues batch_is_exist for every prefill chunk)
The ClientMonitorFunc running every few seconds with client_live_ttl_sec_ on the order of seconds

After a fresh leader switchover (metadata cleared) the race is quiet and then accumulates as buffer invalidations accumulate — this is the signature we observe.

Happy to provide the full production log excerpts, the -704 time series, and the ClientMonitorFunc/unmount_expired_mem_segment correlation if useful.

Before submitting...

Ensure you searched for relevant issues and read the [documentation]

Originally created by @xuyucheng220 on GitHub (Oct 8, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4508 ### Bug Report # Mooncake Issue Report: `CleanupStaleHandles` removes keys without checking lease, causing `lookup` hit → `load` -704 race ## Summary In Mooncake store (both the deployed `v0.3.11.post1` and the current `main` branch), `CleanupStaleHandles` / `ClearInvalidHandles` erase object metadata based solely on `has_invalid_mem_handle() && is_completed()`, **without checking whether the object's lease is still active**. Because `ExistKey` (lookup) and `GetReplicaList` (load) each only `GrantLease` and do **not** `inc_refcnt`, a key can be reported as a hit by `ExistKey` and then erased by `ClearInvalidHandles` before `GetReplicaList` runs, producing `OBJECT_NOT_FOUND (-704)` on a key that was just reported as present. This is a time-of-check-to-time-of-use race between the background `ClientMonitorFunc` cleanup and the two-step (lookup → load) access pattern used by vLLM-Ascend's KV pool. It is **not** fixed on `main`. ## Environment - Mooncake store: `v0.3.11.post1` (production) and `main` branch (verified via GitHub raw source) - Workload: vLLM-Ascend KV transfer (MooncakeConnectorV1 + AscendStoreConnector), GLM-5.2-w8a8c8, 8–12 node clusters, TP16/DP2 prefill, TP4/DP8 decode - Three independent clusters, each with its own mooncake master HA (etcd-backed leadership) ## Observed symptom in production Across three production clusters over ~7 days (2026-10-01 to 2026-10-08), `batch_get_into_multi_buffers` returns `-704` (OBJECT_NOT_FOUND) for keys that `batch_is_exist` (ExistKey) had just reported as present: | Cluster | Master | -704 count | -703 count | -200 count | Eviction (delta) | |---|---|---|---|---|---| | xpod06 | .76 | 4412 | 0 | 16 | 4 cycles, ~47000 keys / ~615 GB each | | xpod07 | .171 | 4892 | 2 | 0 | 1 cycle, 70519 keys / 922 GB | | xpod19 | .75 | 319 | 0 | 0 | 4 cycles, ~47000 keys / ~615 GB each | - `-704` accounts for **>99.6%** of all non-zero return codes from `get`. - `Eviction: Success/Attempts` is **delta per Admin Metrics cycle (~10 s)**, not cumulative (verified: consecutive non-zero rows are `1/1` with `0/0` in between, not monotonically increasing). - After a manual leader switchover (metadata cleared, new leader starts fresh), `-704` drops to near-zero and then **gradually accumulates again** as the cluster runs. This correlates with buffer invalidation (`has_invalid_mem_handle`) accumulating over time, not with eviction pressure. ## Root cause (source-level) ### 1. `ExistKey` and `GetReplicaList` only `GrantLease`, never `inc_refcnt` `master_service.cpp`: ```cpp // ExistKey (lookup) auto MasterService::ExistKey(const std::string& key) ... { MetadataAccessorRO accessor(this, key); if (!accessor.Exists()) return false; const auto& metadata = accessor.Get(); if (metadata.HasReplica(&Replica::fn_is_completed)) { metadata.GrantLease(default_kv_lease_ttl_, default_kv_soft_pin_ttl_); return true; } return false; } // GetReplicaList (load) auto MasterService::GetReplicaList(const std::string& key) ... { MetadataAccessorRO accessor(this, key); if (!accessor.Exists()) return tl::make_unexpected(ErrorCode::OBJECT_NOT_FOUND); // -704 const auto& metadata = accessor.Get(); std::vector<Replica::Descriptor> replica_list; metadata.VisitReplicas(&Replica::fn_is_completed, [&replica_list](const Replica& replica) { replica_list.emplace_back(replica.get_descriptor()); }); if (replica_list.empty()) return tl::make_unexpected(ErrorCode::REPLICA_IS_NOT_READY); // -703 metadata.GrantLease(default_kv_lease_ttl_, default_kv_soft_pin_ttl_); // "so it will not be removed when the client is reading it" ... } ``` `GrantLease` only extends `lease_timeout`; it does **not** touch `refcnt`: ```cpp void GrantLease(const uint64_t ttl, const uint64_t soft_ttl) const { SpinLocker locker(&lock); lease_timeout = std::max(lease_timeout, now + std::chrono::milliseconds(ttl)); if (soft_pin_timeout) { soft_pin_timeout = std::max(*soft_pin_timeout, now + ...); } } ``` ### 2. `BatchEvict` (normal eviction) DOES respect lease — this is NOT the culprit ```cpp // BatchEvict first pass skips objects whose lease is not expired if (it->second.IsHardPinned() || !it->second.IsLeaseExpired(now) || // <- lease guard it->second.IsSoftPinned(now) || !can_evict_replicas(it->second)) { // can_evict requires refcnt==0 ++it; continue; } ``` Because `ExistKey`/`GetReplicaList` both `GrantLease`, a key that was just looked up is **protected from `BatchEvict`** for the lease TTL. Eviction alone cannot explain lookup-hit → load-`-704`. ### 3. `CleanupStaleHandles` / `ClearInvalidHandles` do NOT check lease — this IS the culprit ```cpp bool MasterService::CleanupStaleHandles( ObjectMetadata& metadata, const std::unordered_set<UUID, boost::hash<UUID>>& alive_clients) { metadata.EraseReplicas([&alive_clients](const Replica& replica) { return (replica.has_invalid_mem_handle() || replica.has_invalid_nof_handle() || replica.has_stale_local_disk_client(alive_clients)) && replica.is_completed(); }); return !metadata.IsValid(); } void MasterService::ClearInvalidHandles(...) { for (size_t i = 0; i < kNumShards; i++) { MetadataShardAccessorRW shard(this, i); auto it = shard->metadata.begin(); while (it != shard->metadata.end()) { if (CleanupStaleHandles(it->second, alive_clients)) { shard->processing_keys.erase(it->first); ... it = shard->metadata.erase(it); // <- key removed, lease never consulted } else { ++it; } } } } ``` `CleanupStaleHandles` removes replicas whose memory handle is invalid (i.e. the segment was unmounted / buffer allocator expired) **regardless of whether the object's lease is still active**. When all replicas are removed and `IsValid()` becomes false, the key is erased from metadata. ### 4. `ClientMonitorFunc` triggers this cleanup on a background thread ```cpp void MasterService::ClientMonitorFunc() { while (client_monitor_running_) { // detect client_expired (no ping within client_live_ttl_sec_) ... // unmount expired clients' segments ... // Always clean up invalid handles when there are expired clients ClearInvalidHandles(); // <- runs without lease check } } ``` ### Race window ``` T1 ExistKey(key): HasReplica(fn_is_completed) == true GrantLease(ttl) // lease now valid until T1+ttl return true // lookup HIT [background] ClientMonitorFunc: a client's segment was just unmounted, replica.has_invalid_mem_handle() became true for `key` ClearInvalidHandles -> CleanupStaleHandles: EraseReplicas(has_invalid_mem_handle && is_completed) // lease NOT checked IsValid() == false -> metadata.erase(key) T2 GetReplicaList(key): accessor.Exists() == false // key erased return OBJECT_NOT_FOUND (-704) ``` The lease that `ExistKey` just granted provides **no protection** against `CleanupStaleHandles`, because the cleanup path never calls `IsLeaseExpired()`. ## Why this matters for vLLM-Ascend vLLM-Ascend's KV pool uses a two-step access: 1. `lookup()` (calls `batch_is_exist`) to find the longest cached prefix 2. `load()` (calls `batch_get_into_multi_buffers`) to fetch the cached KV When `lookup` reports a hit but `load` returns `-704`, the connector treats it as a load failure and recomputes the prefix. In the current vLLM-Ascend `SAVE-CLAMP` patch (a site-local poison-KV guard), a `-704` load failure triggers `record_failed_blocks` → `_record_load_failure` → `SAVE-CLAMP` truncates the recomputed KV before writeback. Because `-704` means "missing" (the data is genuinely gone), the recomputed KV is **correct** and safe to write back — but the truncation discards it, so the next request hits `-704` again. Over time this prevents the cache from being rebuilt, inflating TTFT for long multi-turn conversations. Even without the site-local patch, the lookup-hit → load-`-704` race itself is a correctness/usability issue: the client receives contradictory answers from two consecutive RPCs for the same key within milliseconds. ## Verification that `main` is also affected Fetched `mooncake-store/src/master_service.cpp` from the `main` branch. The `CleanupStaleHandles` wrapper and `ClearInvalidHandles` on `main`: ```cpp void MasterService::ClearInvalidHandles( const std::unordered_set<UUID, boost::hash<UUID>>& retaining_clients) { (void)ClearStaleHandles([&retaining_clients](const Replica& replica) { return (replica.has_invalid_mem_handle() || replica.has_invalid_nof_handle() || replica.has_stale_local_disk_client(retaining_clients)) && replica.is_completed(); }); } ``` `ClearStaleHandles` on `main` adds multi-tenant and quota accounting, but the predicate still does **not** include `IsLeaseExpired`. The lease guard is present in `BatchReplicaClear` (L4103) and `PutStart` (L6055) on `main`, but **not** in the `ClearInvalidHandles`/`CleanupStaleHandles` path. ## Suggested fix Add a lease check at the top of `CleanupStaleHandles` so that an active lease delays cleanup of stale-but-still-leased objects: ```cpp bool MasterService::CleanupStaleHandles( ObjectMetadata& metadata, const std::unordered_set<UUID, boost::hash<UUID>>& alive_clients) { // Do not remove replicas for an object whose lease is still active. // ExistKey/GetReplicaList grant a lease so the object survives the // lookup -> load window; honoring that lease here prevents the // lookup-hit -> load -704 race. if (!metadata.IsLeaseExpired()) { return false; } metadata.EraseReplicas([&alive_clients](const Replica& replica) { return (replica.has_invalid_mem_handle() || replica.has_invalid_nof_handle() || replica.has_stale_local_disk_client(alive_clients)) && replica.is_completed(); }); return !metadata.IsValid(); } ``` ### Trade-offs - **Benefit**: eliminates the lookup-hit → load-`-704` race; the two RPCs become consistent for the duration of the lease. - **Cost**: stale handles (buffer already gone) are retained until lease expiry (default TTL is milliseconds-to-seconds). The memory for the *metadata entry* is tiny; the actual buffer is already freed (`has_invalid_mem_handle` means the allocator is invalid). So the cost is a brief delay in metadata cleanup, not a memory leak. - **Alternative**: if delaying cleanup of truly-dead buffers is undesirable, a narrower fix would be to have `GetReplicaList` re-validate that the returned replica descriptors still have valid handles before returning, and retry/skip invalid ones instead of returning `-704` for the whole key. But this is more invasive. ## Questions for the maintainers 1. Is there a design reason `CleanupStaleHandles` intentionally ignores the lease (e.g. "buffer gone = data gone, no point keeping metadata")? If so, would you consider the narrower alternative (re-validate in `GetReplicaList`)? 2. Is the two-step `ExistKey` → `GetReplicaList` pattern (with `GrantLease` on both) an intended usage contract? If yes, the lease should be honored by all cleanup paths. 3. Would a PR adding the `IsLeaseExpired` guard to `CleanupStaleHandles` be acceptable, or do you prefer the `GetReplicaList`-side re-validation? ## Reproduction The race is inherently timing-dependent, but the conditions that make it frequent in production are: - Long-running clusters where worker segments are occasionally unmounted/remounted (load fluctuation, rolling restarts) - High lookup rate (vLLM-Ascend issues `batch_is_exist` for every prefill chunk) - The `ClientMonitorFunc` running every few seconds with `client_live_ttl_sec_` on the order of seconds After a fresh leader switchover (metadata cleared) the race is quiet and then accumulates as buffer invalidations accumulate — this is the signature we observe. --- Happy to provide the full production log excerpts, the `-704` time series, and the `ClientMonitorFunc`/`unmount_expired_mem_segment` correlation if useful. ### Before submitting... - [x] Ensure you searched for relevant issues and read the [documentation]

bsakaguchi added the

bug

label

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

bsakaguchi commented

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

Author

Owner

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

Thanks for opening this issue, @xuyucheng220!

Field	Value
Issue	#4508
GitHub user ID	`132881373`
Reporter	@xuyucheng220

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 8, 2026): Thanks for opening this issue, @xuyucheng220! | Field | Value | |-------|-------| | **Issue** | #4508 | | **GitHub user ID** | `132881373` | | **Reporter** | @xuyucheng220 | 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:21 +00:00

Author

Owner

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

Thanks for opening this issue, @xuyucheng220!

Field Value
Issue #4508
GitHub user ID 132881373
Reporter @xuyucheng220
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.

Summary
In Mooncake store (both the deployed v0.3.11.post1 and the current main branch), CleanupStaleHandles / ClearInvalidHandles erase object metadata based solely on has_invalid_mem_handle() && is_completed(), without checking whether the object's lease is still active. Because ExistKey (lookup) and GetReplicaList (load) each only GrantLease and do not inc_refcnt, a key can be reported as a hit by ExistKey and then erased by ClearInvalidHandles before GetReplicaList runs, producing OBJECT_NOT_FOUND (-704) on a key that was just reported as present.

This is a time-of-check-to-time-of-use race between the background ClientMonitorFunc cleanup and the two-step (lookup → load) access pattern used by vLLM-Ascend's KV pool. It is not fixed on main.

Environment
Mooncake store: v0.3.11.post1 (production) and main branch (verified via GitHub raw source)
Workload: vLLM-Ascend KV transfer (MooncakeConnectorV1 + AscendStoreConnector), GLM-5.2-w8a8c8, 8–12 node clusters, TP16/DP2 prefill, TP4/DP8 decode
Three independent clusters, each with its own mooncake master HA (etcd-backed leadership)

@xuyucheng220 commented on GitHub (Oct 8, 2026): > Thanks for opening this issue, [@xuyucheng220](https://github.com/xuyucheng220)! > > Field Value > **Issue** [#4508](https://github.com/kvcache-ai/Mooncake/issues/4508) > **GitHub user ID** `132881373` > **Reporter** [@xuyucheng220](https://github.com/xuyucheng220) > 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. Summary In Mooncake store (both the deployed v0.3.11.post1 and the current main branch), CleanupStaleHandles / ClearInvalidHandles erase object metadata based solely on has_invalid_mem_handle() && is_completed(), without checking whether the object's lease is still active. Because ExistKey (lookup) and GetReplicaList (load) each only GrantLease and do not inc_refcnt, a key can be reported as a hit by ExistKey and then erased by ClearInvalidHandles before GetReplicaList runs, producing OBJECT_NOT_FOUND (-704) on a key that was just reported as present. This is a time-of-check-to-time-of-use race between the background ClientMonitorFunc cleanup and the two-step (lookup → load) access pattern used by vLLM-Ascend's KV pool. It is not fixed on main. Environment Mooncake store: v0.3.11.post1 (production) and main branch (verified via GitHub raw source) Workload: vLLM-Ascend KV transfer (MooncakeConnectorV1 + AscendStoreConnector), GLM-5.2-w8a8c8, 8–12 node clusters, TP16/DP2 prefill, TP4/DP8 decode Three independent clusters, each with its own mooncake master HA (etcd-backed leadership)

bsakaguchi commented

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

Author

Owner

@he-yufeng commented on GitHub (Oct 8, 2026):

Verified against current main (dcddb56c). The mechanism reads as you describe, and there is a design-intent mismatch on top of it.

The lease is meant to cover exactly this window. GetReplicaList grants it with the comment "so it will not be removed when the client is reading it" (master_service.cpp:4440-4444), and ExistKey refreshes the same lease (3303). Essentially every other removal path honors it: Remove (7397), the eviction paths (7476, 7598, 7734), quota reclaim (8169, 8350, 11512, 12485). The stale-handle sweep is the one removal path that never looks at the lease: the background ClearInvalidHandles (193, 2761) drops replicas where (invalid mem/nof handle or stale disk client) && completed (2771-2775), and when that empties the replica set the metadata is erased outright (2847-2851, EraseMetadata kFull) with no IsLeaseExpired consult anywhere on the path.

Two reasons this matters beyond the error-code flip:

Invalidity is not necessarily terminal. has_invalid_mem_handle() is !buffer->isAllocatorValid() (replica.h:462-468), which flips when the hosting segment unmounts, and replace_memory_buffer (:470-483) exists precisely because a remount or allocator-import rebuild can make the same buffer valid again. A sweep that erases metadata inside a remount/rebuild window drops an object whose bytes are about to be reachable again, and the reader holding the fresh lease never gets the chance.
In the genuinely-dead case the sweep changes what your connector observes: without it, GetReplicaList on a lease-held object with no readable replicas returns REPLICA_IS_NOT_READY (4418-4425), which accurately says "the object exists but nothing is readable right now". The sweep converts that into OBJECT_NOT_FOUND immediately after ExistKey reported a hit, which is the sequence your production traces show.

Fix shape I would propose: keep the replica-level cleanup as is (dropping genuinely stale replicas is fine), but make the metadata erase lease-aware. If the object's read lease is still active, skip the EraseMetadata and leave the (possibly empty) metadata in place until the lease expires; from then the normal sweep takes it. That closes the lookup-then-load race for the TTL window, keeps remount/rebuild recovery intact, and stays consistent with how every other removal path treats the lease. The HA oplog flavor needs the same gate at the durable-finalize erase, matching the existing in-memory semantics.

Do you want to send the PR? Your report did the diagnosis, so first right of refusal is yours. Otherwise I can pick it up.

@he-yufeng commented on GitHub (Oct 8, 2026): Verified against current main (dcddb56c). The mechanism reads as you describe, and there is a design-intent mismatch on top of it. The lease is meant to cover exactly this window. GetReplicaList grants it with the comment "so it will not be removed when the client is reading it" (master_service.cpp:4440-4444), and ExistKey refreshes the same lease (3303). Essentially every other removal path honors it: Remove (7397), the eviction paths (7476, 7598, 7734), quota reclaim (8169, 8350, 11512, 12485). The stale-handle sweep is the one removal path that never looks at the lease: the background ClearInvalidHandles (193, 2761) drops replicas where (invalid mem/nof handle or stale disk client) && completed (2771-2775), and when that empties the replica set the metadata is erased outright (2847-2851, EraseMetadata kFull) with no IsLeaseExpired consult anywhere on the path. Two reasons this matters beyond the error-code flip: 1. Invalidity is not necessarily terminal. has_invalid_mem_handle() is !buffer->isAllocatorValid() (replica.h:462-468), which flips when the hosting segment unmounts, and replace_memory_buffer (:470-483) exists precisely because a remount or allocator-import rebuild can make the same buffer valid again. A sweep that erases metadata inside a remount/rebuild window drops an object whose bytes are about to be reachable again, and the reader holding the fresh lease never gets the chance. 2. In the genuinely-dead case the sweep changes what your connector observes: without it, GetReplicaList on a lease-held object with no readable replicas returns REPLICA_IS_NOT_READY (4418-4425), which accurately says "the object exists but nothing is readable right now". The sweep converts that into OBJECT_NOT_FOUND immediately after ExistKey reported a hit, which is the sequence your production traces show. Fix shape I would propose: keep the replica-level cleanup as is (dropping genuinely stale replicas is fine), but make the metadata erase lease-aware. If the object's read lease is still active, skip the EraseMetadata and leave the (possibly empty) metadata in place until the lease expires; from then the normal sweep takes it. That closes the lookup-then-load race for the TTL window, keeps remount/rebuild recovery intact, and stays consistent with how every other removal path treats the lease. The HA oplog flavor needs the same gate at the durable-finalize erase, matching the existing in-memory semantics. Do you want to send the PR? Your report did the diagnosis, so first right of refusal is yours. Otherwise I can pick it up.

bsakaguchi commented

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

Author

Owner

@he-yufeng commented on GitHub (Oct 8, 2026):

PR up: #4521.

The sweep now skips a record whose read lease is still live whenever the round would finish it (every replica stale, or the metadata already invalid), and the HA oplog REMOVE is held at the same decision point, so standby replay cannot erase early either. Regression test reproduces the lookup -> load race both ways: red without the gate, green with it. Full master_service_test 68/68 locally.

One honest scope note: inside the window, loads still report OBJECT_NOT_FOUND because the handles genuinely are invalid at that point. What changes is that the record survives, so the remount/rebind path revalidates the same metadata and the object comes back, instead of being orphaned at the first sweep. The failure becomes transient and bounded by the lease TTL.

@he-yufeng commented on GitHub (Oct 8, 2026): PR up: #4521. The sweep now skips a record whose read lease is still live whenever the round would finish it (every replica stale, or the metadata already invalid), and the HA oplog REMOVE is held at the same decision point, so standby replay cannot erase early either. Regression test reproduces the lookup -> load race both ways: red without the gate, green with it. Full master_service_test 68/68 locally. One honest scope note: inside the window, loads still report OBJECT_NOT_FOUND because the handles genuinely are invalid at that point. What changes is that the record survives, so the remount/rebind path revalidates the same metadata and the object comes back, instead of being orphaned at the first sweep. The failure becomes transient and bounded by the lease TTL.

bsakaguchi commented

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

Author

Owner

@xuyucheng220 commented on GitHub (Oct 9, 2026):

6. PR up: #4521.
The sweep now skips a record whose read lease is still live whenever the round would finish it (every replica stale, or the metadata already invalid), and the HA oplog REMOVE is held at the same decision point, so standby replay cannot erase early either. Regression test reproduces the lookup -> load race both ways: red without the gate, green with it. Full master_service_test 68/68 locally.
One honest scope note: inside the window, loads still report OBJECT_NOT_FOUND because the handles genuinely are invalid at that point. What changes is that the record survives, so the remount/rebind path revalidates the same metadata and the object comes back, instead of being orphaned at the first sweep. The failure becomes transient and bounded by the lease TTL.

ok, we will use your PR to test, thanks a lot.

@xuyucheng220 commented on GitHub (Oct 9, 2026): > 6\. PR up: [#4521](https://github.com/kvcache-ai/Mooncake/pull/4521). > The sweep now skips a record whose read lease is still live whenever the round would finish it (every replica stale, or the metadata already invalid), and the HA oplog REMOVE is held at the same decision point, so standby replay cannot erase early either. Regression test reproduces the lookup -> load race both ways: red without the gate, green with it. Full master_service_test 68/68 locally. > One honest scope note: inside the window, loads still report OBJECT_NOT_FOUND because the handles genuinely are invalid at that point. What changes is that the record survives, so the remount/rebind path revalidates the same metadata and the object comes back, instead of being orphaned at the first sweep. The failure becomes transient and bounded by the lease TTL. ok, we will use your PR to test, thanks a lot.

bsakaguchi commented

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

Author

Owner

@Aionw commented on GitHub (Oct 14, 2026):

Thanks for the detailed write-up. Before going further with the fix, I think the diagnosis needs one more step, because the -704 itself is intended behavior here.

Once a segment is unmounted, its memory can no longer be trusted, so readers must not see it. After unmount, the master has no guarantee about what that memory still holds. The owning client may have exited, released the buffer, or reused it, and a remote read could return stale or overwritten bytes. That is why UnmountSegment → PrepareUnmountSegment invalidates the allocator registration right away. From that moment has_invalid_mem_handle() is true, IsValid() is false, and GetReplicaList returns OBJECT_NOT_FOUND, all before ClearStaleHandles runs. Hiding the object at unmount time is the intended behavior. The sweep only cleans up the metadata afterwards. #4521's own regression test (SweepDefersToActiveReadLease) confirms this: reads return OBJECT_NOT_FOUND after unmount while the record is still retained.

So I don't think #4521 addresses this report. It changes when the record is erased, not what reads return. The remount-revival argument does not apply here either. On current main (ba6d44d96) the only caller of replace_memory_buffer is the standby-restore path in MountSegment (master_service.cpp:1106), which only runs for segments a newly promoted leader restored and kept alive (standby_accounted_memory_bytes_). After a plain client-expiry → unmount → remount, the master cannot vouch for what the old buffers contain, so the old replicas are not exposed to readers again.

The real question is why "ExistKey hit, then the replica becomes unreadable" happens thousands of times a week. There are two candidates, and the report doesn't yet tell them apart:

Segment unmounts from client expiry. If so, are those clients actually dead or restarted, or alive but missing pings within client_live_ttl_sec (a false expiry, e.g. master overload)? The second case would be the actual bug.
Ordinary eviction after the lease lapses. The lease only protects the lookup → load window for its TTL, and the default default_kv_lease_ttl in v0.3.11.post1 is 5 s. If the connector's lookup → load gap sometimes exceeds that (queueing, large prefill batches), BatchEvict can legitimately evict the key in between. The eviction volume in your table is large, and "near zero after a fresh leader, then gradually accumulating" also fits a store that starts empty and fills up until eviction kicks in.

Could you share:

Master logs around a few -704 bursts: client-expiry and segment-unmount events, plus eviction rounds, with timestamps, so we can see which one lines up with the -704 time series.
For any expired client: its own log around that time. Did it crash or restart, or did it keep running and remount?
The distribution of the lookup → load gap in the connector, if you can measure it.
The master flags in use: default_kv_lease_ttl, client_live_ttl_sec, and the eviction high watermark/ratio.

Separately, whatever the cause turns out to be, a -704 after a hit is a cache miss. The connector should recompute and write back. The site-local SAVE-CLAMP truncation turns a one-off miss into a persistent one, so that part is worth fixing on the vLLM-Ascend side regardless.

@Aionw commented on GitHub (Oct 14, 2026): Thanks for the detailed write-up. Before going further with the fix, I think the diagnosis needs one more step, because the `-704` itself is intended behavior here. **Once a segment is unmounted, its memory can no longer be trusted, so readers must not see it.** After unmount, the master has no guarantee about what that memory still holds. The owning client may have exited, released the buffer, or reused it, and a remote read could return stale or overwritten bytes. That is why `UnmountSegment` → `PrepareUnmountSegment` invalidates the allocator registration right away. From that moment `has_invalid_mem_handle()` is true, `IsValid()` is false, and `GetReplicaList` returns `OBJECT_NOT_FOUND`, all before `ClearStaleHandles` runs. Hiding the object at unmount time is the intended behavior. The sweep only cleans up the metadata afterwards. #4521's own regression test (`SweepDefersToActiveReadLease`) confirms this: reads return `OBJECT_NOT_FOUND` after unmount while the record is still retained. **So I don't think #4521 addresses this report.** It changes when the record is erased, not what reads return. The remount-revival argument does not apply here either. On current main (`ba6d44d96`) the only caller of `replace_memory_buffer` is the standby-restore path in `MountSegment` (`master_service.cpp:1106`), which only runs for segments a newly promoted leader restored and kept alive (`standby_accounted_memory_bytes_`). After a plain client-expiry → unmount → remount, the master cannot vouch for what the old buffers contain, so the old replicas are not exposed to readers again. **The real question is why "ExistKey hit, then the replica becomes unreadable" happens thousands of times a week.** There are two candidates, and the report doesn't yet tell them apart: 1. **Segment unmounts from client expiry.** If so, are those clients actually dead or restarted, or alive but missing pings within `client_live_ttl_sec` (a false expiry, e.g. master overload)? The second case would be the actual bug. 2. **Ordinary eviction after the lease lapses.** The lease only protects the lookup → load window for its TTL, and the default `default_kv_lease_ttl` in v0.3.11.post1 is 5 s. If the connector's lookup → load gap sometimes exceeds that (queueing, large prefill batches), `BatchEvict` can legitimately evict the key in between. The eviction volume in your table is large, and "near zero after a fresh leader, then gradually accumulating" also fits a store that starts empty and fills up until eviction kicks in. Could you share: - Master logs around a few `-704` bursts: client-expiry and segment-unmount events, plus eviction rounds, with timestamps, so we can see which one lines up with the `-704` time series. - For any expired client: its own log around that time. Did it crash or restart, or did it keep running and remount? - The distribution of the lookup → load gap in the connector, if you can measure it. - The master flags in use: `default_kv_lease_ttl`, `client_live_ttl_sec`, and the eviction high watermark/ratio. Separately, whatever the cause turns out to be, a `-704` after a hit is a cache miss. The connector should recompute and write back. The site-local `SAVE-CLAMP` truncation turns a one-off miss into a persistent one, so that part is worth fixing on the vLLM-Ascend side regardless.

bsakaguchi commented

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

Author

Owner

@xuyucheng220 commented on GitHub (Oct 14, 2026):

Thanks for the detailed write-up. Before going further with the fix, I think the diagnosis needs one more step, because the -704 itself is intended behavior here.

Once a segment is unmounted, its memory can no longer be trusted, so readers must not see it. After unmount, the master has no guarantee about what that memory still holds. The owning client may have exited, released the buffer, or reused it, and a remote read could return stale or overwritten bytes. That is why UnmountSegment → PrepareUnmountSegment invalidates the allocator registration right away. From that moment has_invalid_mem_handle() is true, IsValid() is false, and GetReplicaList returns OBJECT_NOT_FOUND, all before ClearStaleHandles runs. Hiding the object at unmount time is the intended behavior. The sweep only cleans up the metadata afterwards. #4521's own regression test (SweepDefersToActiveReadLease) confirms this: reads return OBJECT_NOT_FOUND after unmount while the record is still retained.

So I don't think #4521 addresses this report. It changes when the record is erased, not what reads return. The remount-revival argument does not apply here either. On current main (ba6d44d96) the only caller of replace_memory_buffer is the standby-restore path in MountSegment (master_service.cpp:1106), which only runs for segments a newly promoted leader restored and kept alive (standby_accounted_memory_bytes_). After a plain client-expiry → unmount → remount, the master cannot vouch for what the old buffers contain, so the old replicas are not exposed to readers again.

The real question is why "ExistKey hit, then the replica becomes unreadable" happens thousands of times a week. There are two candidates, and the report doesn't yet tell them apart:

Segment unmounts from client expiry. If so, are those clients actually dead or restarted, or alive but missing pings within client_live_ttl_sec (a false expiry, e.g. master overload)? The second case would be the actual bug.

Ordinary eviction after the lease lapses. The lease only protects the lookup → load window for its TTL, and the default default_kv_lease_ttl in v0.3.11.post1 is 5 s. If the connector's lookup → load gap sometimes exceeds that (queueing, large prefill batches), BatchEvict can legitimately evict the key in between. The eviction volume in your table is large, and "near zero after a fresh leader, then gradually accumulating" also fits a store that starts empty and fills up until eviction kicks in.

Could you share:

Master logs around a few -704 bursts: client-expiry and segment-unmount events, plus eviction rounds, with timestamps, so we can see which one lines up with the -704 time series.

For any expired client: its own log around that time. Did it crash or restart, or did it keep running and remount?

The distribution of the lookup → load gap in the connector, if you can measure it.

The master flags in use: default_kv_lease_ttl, client_live_ttl_sec, and the eviction high watermark/ratio.

Separately, whatever the cause turns out to be, a -704 after a hit is a cache miss. The connector should recompute and write back. The site-local SAVE-CLAMP truncation turns a one-off miss into a persistent one, so that part is worth fixing on the vLLM-Ascend side regardless.

thanks for your reply. i have patched pr-4521 in my ENV days ago, and also have found and fixed the problem of vllm-ascend yesterday( it also caused so many -704 error code, the issue link: https://github.com/vllm-project/vllm-ascend/issues/18509)

@xuyucheng220 commented on GitHub (Oct 14, 2026): > Thanks for the detailed write-up. Before going further with the fix, I think the diagnosis needs one more step, because the `-704` itself is intended behavior here. > > **Once a segment is unmounted, its memory can no longer be trusted, so readers must not see it.** After unmount, the master has no guarantee about what that memory still holds. The owning client may have exited, released the buffer, or reused it, and a remote read could return stale or overwritten bytes. That is why `UnmountSegment` → `PrepareUnmountSegment` invalidates the allocator registration right away. From that moment `has_invalid_mem_handle()` is true, `IsValid()` is false, and `GetReplicaList` returns `OBJECT_NOT_FOUND`, all before `ClearStaleHandles` runs. Hiding the object at unmount time is the intended behavior. The sweep only cleans up the metadata afterwards. [#4521](https://github.com/kvcache-ai/Mooncake/pull/4521)'s own regression test (`SweepDefersToActiveReadLease`) confirms this: reads return `OBJECT_NOT_FOUND` after unmount while the record is still retained. > > **So I don't think [#4521](https://github.com/kvcache-ai/Mooncake/pull/4521) addresses this report.** It changes when the record is erased, not what reads return. The remount-revival argument does not apply here either. On current main (`ba6d44d96`) the only caller of `replace_memory_buffer` is the standby-restore path in `MountSegment` (`master_service.cpp:1106`), which only runs for segments a newly promoted leader restored and kept alive (`standby_accounted_memory_bytes_`). After a plain client-expiry → unmount → remount, the master cannot vouch for what the old buffers contain, so the old replicas are not exposed to readers again. > > **The real question is why "ExistKey hit, then the replica becomes unreadable" happens thousands of times a week.** There are two candidates, and the report doesn't yet tell them apart: > > 1. **Segment unmounts from client expiry.** If so, are those clients actually dead or restarted, or alive but missing pings within `client_live_ttl_sec` (a false expiry, e.g. master overload)? The second case would be the actual bug. > 2. **Ordinary eviction after the lease lapses.** The lease only protects the lookup → load window for its TTL, and the default `default_kv_lease_ttl` in v0.3.11.post1 is 5 s. If the connector's lookup → load gap sometimes exceeds that (queueing, large prefill batches), `BatchEvict` can legitimately evict the key in between. The eviction volume in your table is large, and "near zero after a fresh leader, then gradually accumulating" also fits a store that starts empty and fills up until eviction kicks in. > > Could you share: > > * Master logs around a few `-704` bursts: client-expiry and segment-unmount events, plus eviction rounds, with timestamps, so we can see which one lines up with the `-704` time series. > * For any expired client: its own log around that time. Did it crash or restart, or did it keep running and remount? > * The distribution of the lookup → load gap in the connector, if you can measure it. > * The master flags in use: `default_kv_lease_ttl`, `client_live_ttl_sec`, and the eviction high watermark/ratio. > > Separately, whatever the cause turns out to be, a `-704` after a hit is a cache miss. The connector should recompute and write back. The site-local `SAVE-CLAMP` truncation turns a one-off miss into a persistent one, so that part is worth fixing on the vLLM-Ascend side regardless. thanks for your reply. i have patched pr-4521 in my ENV days ago, and also have found and fixed the problem of vllm-ascend yesterday( it also caused so many -704 error code, the issue link: https://github.com/vllm-project/vllm-ascend/issues/18509)

bsakaguchi referenced this issue

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

[PR #1148] [MERGED] [Store] pub_tensor for multiple replica #2039