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 #3052] [Bug]: client 重启后， ssd 上的数据全部失效 #781

New issue

Closed

opened 2026-10-29 21:19:28 +00:00 by bsakaguchi · 7 comments

bsakaguchi commented

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

Owner

Originally created by @sun-baoshan-sunshine on GitHub (Jul 22, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3052

Bug Report

mooncake version： v0.3.11.post1

1. Background
When a client with SSD offload enabled crashes unexpectedly and is subsequently restarted in a short timeframe, the SSD data previously held by the client becomes stale/invalid, rendering it inaccessible.
2. Analysis
Under normal circumstances, upon restart, the client is expected to scan the data residing on the SSD and report the metadata to the Master via ScanMeta. However, if the new client's registration/reporting occurs within the TTL (Time-To-Live) window of the previous client instance — meaning the old client has not yet been evicted by the Master — the new client's registration attempt is effectively a no-op and is rejected. The root cause of this issue resides in the logic of auto MasterService::AddReplica(const UUID& client_id, const std::string& key, Replica& replica).

auto MasterService::AddReplica(const UUID& client_id, const std::string& key,
                               Replica& replica)
    -> tl::expected<void, ErrorCode> {
    std::shared_lock<std::shared_mutex> shared_lock(snapshot_mutex_);
    MetadataAccessorRW accessor(this, key);
    if (!accessor.Exists()) {
        accessor.Create(
            client_id,
            replica.get_descriptor().get_local_disk_descriptor().object_size,
            std::vector<Replica>{}, false);
    }
    auto& metadata = accessor.Get();
    if (replica.type() != ReplicaType::LOCAL_DISK) {
        LOG(ERROR) << "Invalid replica type: " << replica.type()
                   << ". Expected ReplicaType::LOCAL_DISK.";
        return tl::make_unexpected(ErrorCode::INVALID_PARAMS);
    }

    if (!metadata.HasReplica(&Replica::fn_is_local_disk_replica)) {
        std::vector<Replica> replicas;
        replicas.emplace_back(std::move(replica));
        metadata.AddReplicas(std::move(replicas));
        return {};
    }

    metadata.VisitReplicas(
        [client_id](const Replica& rep) {
            return rep.type() == ReplicaType::LOCAL_DISK &&
                   rep.get_descriptor().get_local_disk_descriptor().client_id ==
                       client_id;
        },
        [&replica](Replica& rep) {
            rep.get_descriptor()
                .get_local_disk_descriptor()
                .transport_endpoint = replica.get_descriptor()
                                          .get_local_disk_descriptor()
                                          .transport_endpoint;
            rep.get_descriptor().get_local_disk_descriptor().object_size =
                replica.get_descriptor()
                    .get_local_disk_descriptor()
                    .object_size;
        });
    return {};
}

When the new client reports its keys, since the old client has not yet expired, those keys already exist in the Master's metadata. During the check for whether a disk replica exists for the keys reported by the new client, there is no validation to verify whether the client_id matches the one associated with the existing metadata entry. As a result, the Master incorrectly concludes that the data reported by the new client is already present and takes no further action.

Subsequently, when the old client reaches its TTL and is evicted by the Master, its corresponding metadata is also purged. Consequently, the KV cache data residing on the SSD becomes orphaned and can no longer be accessed or served.

Before submitting...

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

Originally created by @sun-baoshan-sunshine on GitHub (Jul 22, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3052 ### Bug Report mooncake version： v0.3.11.post1 **1. Background** When a client with SSD offload enabled crashes unexpectedly and is subsequently restarted in a short timeframe, the SSD data previously held by the client becomes stale/invalid, rendering it inaccessible. **2. Analysis** Under normal circumstances, upon restart, the client is expected to scan the data residing on the SSD and report the metadata to the Master via ScanMeta. However, if the new client's registration/reporting occurs within the TTL (Time-To-Live) window of the previous client instance — meaning the old client has not yet been evicted by the Master — the new client's registration attempt is effectively a no-op and is rejected. The root cause of this issue resides in the logic of auto MasterService::AddReplica(const UUID& client_id, const std::string& key, Replica& replica). ```c++ auto MasterService::AddReplica(const UUID& client_id, const std::string& key, Replica& replica) -> tl::expected<void, ErrorCode> { std::shared_lock<std::shared_mutex> shared_lock(snapshot_mutex_); MetadataAccessorRW accessor(this, key); if (!accessor.Exists()) { accessor.Create( client_id, replica.get_descriptor().get_local_disk_descriptor().object_size, std::vector<Replica>{}, false); } auto& metadata = accessor.Get(); if (replica.type() != ReplicaType::LOCAL_DISK) { LOG(ERROR) << "Invalid replica type: " << replica.type() << ". Expected ReplicaType::LOCAL_DISK."; return tl::make_unexpected(ErrorCode::INVALID_PARAMS); } if (!metadata.HasReplica(&Replica::fn_is_local_disk_replica)) { std::vector<Replica> replicas; replicas.emplace_back(std::move(replica)); metadata.AddReplicas(std::move(replicas)); return {}; } metadata.VisitReplicas( [client_id](const Replica& rep) { return rep.type() == ReplicaType::LOCAL_DISK && rep.get_descriptor().get_local_disk_descriptor().client_id == client_id; }, [&replica](Replica& rep) { rep.get_descriptor() .get_local_disk_descriptor() .transport_endpoint = replica.get_descriptor() .get_local_disk_descriptor() .transport_endpoint; rep.get_descriptor().get_local_disk_descriptor().object_size = replica.get_descriptor() .get_local_disk_descriptor() .object_size; }); return {}; } ``` When the new client reports its keys, since the old client has not yet expired, those keys already exist in the Master's metadata. During the check for whether a disk replica exists for the keys reported by the new client, there is no validation to verify whether the client_id matches the one associated with the existing metadata entry. As a result, the Master incorrectly concludes that the data reported by the new client is already present and takes no further action. Subsequently, when the old client reaches its TTL and is evicted by the Master, its corresponding metadata is also purged. Consequently, the KV cache data residing on the SSD becomes orphaned and can no longer be accessed or served. ### Before submitting... - [x] Ensure you searched for relevant issues and read the [documentation]

bsakaguchi

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

closed this issue
added the
stale

auto-closed

bug
labels

bsakaguchi commented

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

Author

Owner

@github-actions[bot] commented on GitHub (Jul 22, 2026):

Thanks for opening this issue, @sun-baoshan-sunshine!

Field	Value
Issue	#3052
GitHub user ID	`78401405`
Reporter	@sun-baoshan-sunshine

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 (Jul 22, 2026): Thanks for opening this issue, @sun-baoshan-sunshine! | Field | Value | |-------|-------| | **Issue** | #3052 | | **GitHub user ID** | `78401405` | | **Reporter** | @sun-baoshan-sunshine | 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:19:29 +00:00

Author

Owner

@he-yufeng commented on GitHub (Jul 22, 2026):

Confirmed this chain on current main (12df58c7):

On restart the client runs ReRegisterOffloadedObjects, which scans the SSD and reports every object through NotifyOffloadSuccess (file_storage.cpp:1216 stamps transport_endpoint with the current local_rpc_addr_).
In MasterService::AddReplica (master_service.cpp:3344), HasReplica(fn_is_local_disk_replica) is true because the dead incarnation's replica is still registered. The VisitReplicas update path only matches a replica whose client_id equals the reporting one, so the restarted client's report matches nothing and is silently dropped. AddReplica returns false and the caller treats the batch as fine.
Once the old UUID expires, ClientMonitorFunc -> ClearInvalidHandles -> CleanupStaleHandles removes completed local_disk replicas whose owner is not alive, and the object is erased when no valid replica remains. The files stay on the SSD but the master no longer knows they exist.

One wrinkle for the fix: master_service_tenant_quota_test.cpp:480 (NotifyOffloadSuccessDoesNotCountAddReplicaUpdateAsNewDiskUsage) pins the current drop as intended behavior for the two-live-clients case: client B re-reporting the same key gets deduped and is not charged usage. So "just append the second replica" contradicts a tested design decision.

A direction that keeps that test green: treat the report as an ownership takeover only when the existing disk replica's transport_endpoint equals the reporter's endpoint. A restarted client re-scans and reports the same local_rpc_addr_ its dead incarnation registered with, so the master can retarget the replica to the new client_id and charge the usage there. A genuinely different client reports a different endpoint and still lands in the dedup path. The caveat is that this only works when local_rpc_addr_ is stable across restarts (fixed RPC port, not ephemeral). Is that true in your deployment?

Happy to send the PR if this direction sounds reasonable to @ykwd and @staryxchen.

@he-yufeng commented on GitHub (Jul 22, 2026): Confirmed this chain on current main (12df58c7): 1. On restart the client runs `ReRegisterOffloadedObjects`, which scans the SSD and reports every object through `NotifyOffloadSuccess` (`file_storage.cpp:1216` stamps `transport_endpoint` with the current `local_rpc_addr_`). 2. In `MasterService::AddReplica` (`master_service.cpp:3344`), `HasReplica(fn_is_local_disk_replica)` is true because the dead incarnation's replica is still registered. The `VisitReplicas` update path only matches a replica whose `client_id` equals the reporting one, so the restarted client's report matches nothing and is silently dropped. `AddReplica` returns false and the caller treats the batch as fine. 3. Once the old UUID expires, `ClientMonitorFunc` -> `ClearInvalidHandles` -> `CleanupStaleHandles` removes completed local_disk replicas whose owner is not alive, and the object is erased when no valid replica remains. The files stay on the SSD but the master no longer knows they exist. One wrinkle for the fix: `master_service_tenant_quota_test.cpp:480` (`NotifyOffloadSuccessDoesNotCountAddReplicaUpdateAsNewDiskUsage`) pins the current drop as intended behavior for the two-live-clients case: client B re-reporting the same key gets deduped and is not charged usage. So "just append the second replica" contradicts a tested design decision. A direction that keeps that test green: treat the report as an ownership takeover only when the existing disk replica's `transport_endpoint` equals the reporter's endpoint. A restarted client re-scans and reports the same `local_rpc_addr_` its dead incarnation registered with, so the master can retarget the replica to the new `client_id` and charge the usage there. A genuinely different client reports a different endpoint and still lands in the dedup path. The caveat is that this only works when `local_rpc_addr_` is stable across restarts (fixed RPC port, not ephemeral). Is that true in your deployment? Happy to send the PR if this direction sounds reasonable to @ykwd and @staryxchen.

bsakaguchi commented

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

Author

Owner

@LujhCoconut commented on GitHub (Jul 23, 2026):

@he-yufeng Sounds right. This makes sense to me. The ransport_endpoint match is the proper discriminator here. Happy to review when the PR is up.

@LujhCoconut commented on GitHub (Jul 23, 2026): @he-yufeng Sounds right. This makes sense to me. The `ransport_endpoint` match is the proper discriminator here. Happy to review when the PR is up.

bsakaguchi commented

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

Author

Owner

@sun-baoshan-sunshine commented on GitHub (Jul 24, 2026):

@he-yufeng @LujhCoconut Has it been confirmed that a pr will be submitted to resolve this issue?

@sun-baoshan-sunshine commented on GitHub (Jul 24, 2026): @he-yufeng @LujhCoconut Has it been confirmed that a pr will be submitted to resolve this issue?

bsakaguchi commented

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

Author

Owner

@LujhCoconut commented on GitHub (Jul 24, 2026):

@sun-baoshan-sunshine There will definitely be a PR to fix this issue. If you or @he-yufeng are willing to submit a PR, I'd be happy to review it. Alternatively, I can also submit a PR to address this.

@LujhCoconut commented on GitHub (Jul 24, 2026): @sun-baoshan-sunshine There will definitely be a PR to fix this issue. If you or @he-yufeng are willing to submit a PR, I'd be happy to review it. Alternatively, I can also submit a PR to address this.

bsakaguchi commented

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

Author

Owner

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

This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

@github-actions[bot] commented on GitHub (Oct 22, 2026): This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

bsakaguchi commented

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

Author

Owner

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

Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

@github-actions[bot] commented on GitHub (Oct 29, 2026): Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

bsakaguchi referenced this issue

2026-10-29 21:30:35 +00:00

[PR #781] [MERGED] [DOC] Add all badges #1815