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 #3715] [Store] Root fix for the RemoveAll/broadcast wipe-vs-lease divergence #943

New issue

Open

opened 2026-10-29 21:21:10 +00:00 by bsakaguchi · 6 comments

bsakaguchi commented

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

Owner

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

Summary

Non-forced RemoveAll (and the master mailbox broadcast) physically wipes client SSD files without consulting lease state, while the master keeps lease-protected metadata. The divergence this creates (#3709) is currently mitigated on the Put path by #3711, which proves a dangling LOCAL_DISK replica is gone before evicting and retrying. That heal is convergent but lazy: keys that are only ever read, or written through batch paths, stay inconsistent until a Put touches them. The root fix belongs at the wipe boundary.

Problem

Two independent paths physically delete client SSD files while the master can still hold valid leases:

RemoveAll (non-forced): storage_backend_->RemoveAll() on the client wipes every local offload file unconditionally; the master removes only metadata whose leases have expired.
The master mailbox broadcast tells clients to wipe without carrying per-key lease information.

Once a file is gone but the master's completed replica entry remains, the key is wedged: reads fail and every Put reports OBJECT_ALREADY_EXISTS.

Directions, with their costs

Master returns the exact keys each client may delete. After lease evaluation the master knows precisely which keys lost protection. Returning that set (or skipping the physical wipe entirely for non-forced RemoveAll) stops the divergence from forming. Cost: an RPC contract change, so old clients against a new master need a defined fallback.
Master enqueues per-client delete sets for offline clients. Covers clients not online at RemoveAll time. Cost: durable per-client queue state on the master, which also has to replicate in HA mode; that is real new statefulness, not a small patch.
Lease-aware mailbox broadcast. The broadcast path has no response channel at all, so option 1 alone never covers it; the broadcast would need to carry the lease-checked key set.

The wipe side also races with concurrent Puts in any design (a key re-written between the master's lease check and the client's file deletion), so the Put-time heal in #3711 stays useful as the convergent safety net even after a boundary fix lands.

Scope of this issue

Track the RemoveAll/broadcast boundary redesign: contract shape, old-client compatibility, whether offline enqueue is worth the durable state, and coverage of the broadcast path. #3711 should be read as mitigation, not resolution.

Prepared with Kimi K3 assistance; the mechanism summary above was re-verified against current main before filing.

Originally created by @he-yufeng on GitHub (Aug 27, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3715 ## Summary Non-forced `RemoveAll` (and the master mailbox broadcast) physically wipes client SSD files without consulting lease state, while the master keeps lease-protected metadata. The divergence this creates (#3709) is currently mitigated on the Put path by #3711, which proves a dangling LOCAL_DISK replica is gone before evicting and retrying. That heal is convergent but lazy: keys that are only ever read, or written through batch paths, stay inconsistent until a Put touches them. The root fix belongs at the wipe boundary. ## Problem Two independent paths physically delete client SSD files while the master can still hold valid leases: - `RemoveAll` (non-forced): `storage_backend_->RemoveAll()` on the client wipes every local offload file unconditionally; the master removes only metadata whose leases have expired. - The master mailbox broadcast tells clients to wipe without carrying per-key lease information. Once a file is gone but the master's completed replica entry remains, the key is wedged: reads fail and every Put reports `OBJECT_ALREADY_EXISTS`. ## Directions, with their costs 1. **Master returns the exact keys each client may delete.** After lease evaluation the master knows precisely which keys lost protection. Returning that set (or skipping the physical wipe entirely for non-forced RemoveAll) stops the divergence from forming. Cost: an RPC contract change, so old clients against a new master need a defined fallback. 2. **Master enqueues per-client delete sets for offline clients.** Covers clients not online at RemoveAll time. Cost: durable per-client queue state on the master, which also has to replicate in HA mode; that is real new statefulness, not a small patch. 3. **Lease-aware mailbox broadcast.** The broadcast path has no response channel at all, so option 1 alone never covers it; the broadcast would need to carry the lease-checked key set. The wipe side also races with concurrent Puts in any design (a key re-written between the master's lease check and the client's file deletion), so the Put-time heal in #3711 stays useful as the convergent safety net even after a boundary fix lands. ## Scope of this issue Track the RemoveAll/broadcast boundary redesign: contract shape, old-client compatibility, whether offline enqueue is worth the durable state, and coverage of the broadcast path. #3711 should be read as mitigation, not resolution. Prepared with Kimi K3 assistance; the mechanism summary above was re-verified against current main before filing.

bsakaguchi commented

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

Author

Owner

@github-actions[bot] commented on GitHub (Aug 27, 2026):

Thanks for opening this issue, @he-yufeng!

Field	Value
Issue	#3715
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.

@github-actions[bot] commented on GitHub (Aug 27, 2026): Thanks for opening this issue, @he-yufeng! | Field | Value | |-------|-------| | **Issue** | #3715 | | **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:21:12 +00:00

Author

Owner

@200lz commented on GitHub (Aug 27, 2026):

I went through the discussion here and the recent #3711 / #3715 work around RemoveAll.

The Phase-1 containment idea discussed above still looks like a useful independent slice to me: fail closed for tenant-scoped local cleanup before any master metadata mutation or global SSD wipe, with cross-tenant regression coverage.

I also noticed @he-yufeng had previously offered to work on this direction, so I don't want to duplicate ongoing work. If this containment slice is still open, I'd be happy to take it and keep it narrowly scoped to the fail-safe behavior + tests, without touching the broader generation/ACK protocol.

Alternatively, if the implementation is already underway, I could contribute the missing crash/failover regression matrix around poll-before-wipe, lost ACK, and stale generation behavior.

Would either of those be useful?

@200lz commented on GitHub (Aug 27, 2026): I went through the discussion here and the recent #3711 / #3715 work around RemoveAll. The Phase-1 containment idea discussed above still looks like a useful independent slice to me: fail closed for tenant-scoped local cleanup before any master metadata mutation or global SSD wipe, with cross-tenant regression coverage. I also noticed @he-yufeng had previously offered to work on this direction, so I don't want to duplicate ongoing work. If this containment slice is still open, I'd be happy to take it and keep it narrowly scoped to the fail-safe behavior + tests, without touching the broader generation/ACK protocol. Alternatively, if the implementation is already underway, I could contribute the missing crash/failover regression matrix around poll-before-wipe, lost ACK, and stale generation behavior. Would either of those be useful?

bsakaguchi commented

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

Author

Owner

@he-yufeng commented on GitHub (Aug 29, 2026):

Welcome, and thanks for asking before starting. Nothing is assigned yet: the direction here is open design space, and the maintainer read on it is still pending, so the safest way to spend effort is to write the containment slice as a small design note in this thread first (what "fail closed for tenant-scoped local cleanup" checks before mutating, and which surfaces it covers) rather than code that might land differently. If fcczzz or another maintainer signs off on the shape, a narrow PR with the fail-safe behavior plus the crash/failover regression matrix you listed would be a genuinely useful contribution. The poll-before-wipe and lost-ACK cases are exactly the parts the current discussion has not covered.

@he-yufeng commented on GitHub (Aug 29, 2026): Welcome, and thanks for asking before starting. Nothing is assigned yet: the direction here is open design space, and the maintainer read on it is still pending, so the safest way to spend effort is to write the containment slice as a small design note in this thread first (what "fail closed for tenant-scoped local cleanup" checks before mutating, and which surfaces it covers) rather than code that might land differently. If fcczzz or another maintainer signs off on the shape, a narrow PR with the fail-safe behavior plus the crash/failover regression matrix you listed would be a genuinely useful contribution. The poll-before-wipe and lost-ACK cases are exactly the parts the current discussion has not covered.

bsakaguchi commented

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

Author

Owner

@200lz commented on GitHub (Aug 29, 2026):

Thanks — I traced the current path on 53e41174ff.

One thing that became clearer from the trace is that the containment boundary needs to be earlier than the LocalSSD mailbox publication. In the tenant overload, MasterService::RemoveAll(const TenantId&, bool) mutates eligible metadata/accounting first and only calls RequestRemoveAll(...) after the shard walk. By that point metadata/replica state, quota/accounting, LocalSSD used-byte state, and offload/promotion state may already have changed.

So I think the Phase-1 invariant should be:

A tenant-scoped RemoveAll must never degrade into an unscoped physical cleanup. If the current backend/protocol cannot prove tenant-scoped deletion, the operation should fail before the tenant MasterService::RemoveAll shard mutation begins and before any RemoveAll mailbox state is published.

force=true should not bypass this containment. On current main it bypasses lease expiry only; it does not make an unscoped LOCAL_DISK wipe tenant-safe. At the same time, memory-only tenant RemoveAll is already supported, so I would avoid rejecting tenant operations unconditionally.

For the first containment change I see two possible predicates:

Conservative/capability-level: reject real tenant-scoped RemoveAll when unsupported LOCAL_DISK/offload physical cleanup is enabled.
Object-level: reject only when the affected operation can touch LOCAL_DISK replicas.

I currently prefer (1) for the first PR because it is race-free. A separate metadata preflight for (2) would otherwise need to be atomic with the mutation pass; an offload completion could introduce a LOCAL_DISK replica between the two.

There is one API question I would like maintainer guidance on before coding: MasterClient::RemoveAll already exposes tl::expected<long, ErrorCode>, while WrappedMasterService::RemoveAll currently returns a plain long. That means the server cannot safely propagate an explicit UNSUPPORTED_TENANT_LOCAL_CLEANUP today. Reusing the existing tl::expected<long, ErrorCode> RPC pattern looks natural, but it changes the RPC response contract.

Would you prefer changing the existing RemoveAll RPC to carry an application error, or a versioned/otherwise compatibility-safe error-capable surface?

For the first regression I would put tenants A and B on the same LOCAL_DISK holder and require rejected RemoveAll(A) to leave A/B metadata, quota/accounting, mailbox state, and both physical objects unchanged, with a force=true mirror.

I would keep generation/ACK, durable markers, failover recovery, mutation epochs, and tenant-aware physical deletion out of this first containment change.

@fcczzz, if you have a preference on the rejection predicate or RPC compatibility shape, I can keep the first PR aligned with that.

@200lz commented on GitHub (Aug 29, 2026): Thanks — I traced the current path on 53e41174ff45f17369ed84278e43773c9f51ddb8. One thing that became clearer from the trace is that the containment boundary needs to be earlier than the LocalSSD mailbox publication. In the tenant overload, MasterService::RemoveAll(const TenantId&, bool) mutates eligible metadata/accounting first and only calls RequestRemoveAll(...) after the shard walk. By that point metadata/replica state, quota/accounting, LocalSSD used-byte state, and offload/promotion state may already have changed. So I think the Phase-1 invariant should be: A tenant-scoped RemoveAll must never degrade into an unscoped physical cleanup. If the current backend/protocol cannot prove tenant-scoped deletion, the operation should fail before the tenant MasterService::RemoveAll shard mutation begins and before any RemoveAll mailbox state is published. force=true should not bypass this containment. On current main it bypasses lease expiry only; it does not make an unscoped LOCAL_DISK wipe tenant-safe. At the same time, memory-only tenant RemoveAll is already supported, so I would avoid rejecting tenant operations unconditionally. For the first containment change I see two possible predicates: Conservative/capability-level: reject real tenant-scoped RemoveAll when unsupported LOCAL_DISK/offload physical cleanup is enabled. Object-level: reject only when the affected operation can touch LOCAL_DISK replicas. I currently prefer (1) for the first PR because it is race-free. A separate metadata preflight for (2) would otherwise need to be atomic with the mutation pass; an offload completion could introduce a LOCAL_DISK replica between the two. There is one API question I would like maintainer guidance on before coding: MasterClient::RemoveAll already exposes tl::expected<long, ErrorCode>, while WrappedMasterService::RemoveAll currently returns a plain long. That means the server cannot safely propagate an explicit UNSUPPORTED_TENANT_LOCAL_CLEANUP today. Reusing the existing tl::expected<long, ErrorCode> RPC pattern looks natural, but it changes the RPC response contract. Would you prefer changing the existing RemoveAll RPC to carry an application error, or a versioned/otherwise compatibility-safe error-capable surface? For the first regression I would put tenants A and B on the same LOCAL_DISK holder and require rejected RemoveAll(A) to leave A/B metadata, quota/accounting, mailbox state, and both physical objects unchanged, with a force=true mirror. I would keep generation/ACK, durable markers, failover recovery, mutation epochs, and tenant-aware physical deletion out of this first containment change. @fcczzz, if you have a preference on the rejection predicate or RPC compatibility shape, I can keep the first PR aligned with that.

bsakaguchi commented

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

Author

Owner

@fcczzz commented on GitHub (Aug 31, 2026):

@200lz Thanks for tracing this. For the first containment change, I prefer the conservative capability-level check: when tenant-scoped physical cleanup cannot be guaranteed, fail before any metadata/accounting mutation and before publishing the LocalSSD mailbox request. force=true should not bypass this check.

One clarification: the physical storage keys are already tenant-scoped through TenantId::MakeScopedKey(), but the backend interface and mailbox protocol do not support tenant-scoped deletion. Please describe the limitation in terms of deletion capability rather than absence of a tenant dimension.

For RPC compatibility, I prefer a versioned error-capable surface instead of changing the existing scalar RemoveAll response in place. Please first post a short design covering new/old client behavior and the fail-closed fallback before starting the implementation.

The proposed cross-tenant regression, including the force=true case, looks appropriate.

@fcczzz commented on GitHub (Aug 31, 2026): @200lz Thanks for tracing this. For the first containment change, I prefer the conservative capability-level check: when tenant-scoped physical cleanup cannot be guaranteed, fail before any metadata/accounting mutation and before publishing the LocalSSD mailbox request. `force=true` should not bypass this check. One clarification: the physical storage keys are already tenant-scoped through `TenantId::MakeScopedKey()`, but the backend interface and mailbox protocol do not support tenant-scoped deletion. Please describe the limitation in terms of deletion capability rather than absence of a tenant dimension. For RPC compatibility, I prefer a versioned error-capable surface instead of changing the existing scalar `RemoveAll` response in place. Please first post a short design covering new/old client behavior and the fail-closed fallback before starting the implementation. The proposed cross-tenant regression, including the `force=true` case, looks appropriate.

bsakaguchi commented

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

Author

Owner

@200lz commented on GitHub (Sep 1, 2026):

Thanks. I traced the current path again on origin/main (4956149bcb). Based on your guidance, I propose the following Phase-1 design.

Containment invariant

A tenant-scoped RemoveAll must never fall through to an unscoped physical cleanup. If the active backend/mailbox path cannot guarantee tenant-scoped deletion, reject the request:

before the first tenant metadata/accounting mutation in MasterService::RemoveAll(const TenantId&, bool);
before publishing any LocalSSD RemoveAll mailbox request.

force=true does not bypass this capability check.

Physical keys are already tenant-scoped through TenantId::MakeScopedKey(). The missing capability is deletion: StorageBackend exposes only unscoped RemoveAll(), and the LocalSSD mailbox currently carries only an unscoped remove-all signal.

For Phase 1 I would use the conservative capability-level predicate rather than inspecting current replicas. If real multi-tenancy is active and the LocalSSD/offload cleanup path is enabled, tenant-scoped physical deletion cannot currently be guaranteed, so tenant RemoveAll fails closed. With LocalSSD/offload disabled, memory-only tenant RemoveAll remains supported.

RPC compatibility

I would keep the existing scalar RemoveAll RPC and add a separate versioned/error-capable handler. I did not find an established *V2 naming convention, so I would leave the final method name open for review.

new client → new server: call only the new handler. Capability rejection returns an explicit application error. Local cache/backend cleanup happens only after RPC success.
new client → old server: the new handler is unregistered, so the client fails closed and does not retry through legacy RemoveAll.
old client → new server: retain the legacy scalar success behavior for safe cases. For a capability-rejected tenant request, return an RPC-level failure rather than scalar 0, because old clients perform local unscoped storage_backend_->RemoveAll() after any successful scalar response.

One compatibility detail: current old clients bump their local hot-cache epoch before making the RPC. Therefore an old client that receives this RPC failure may still have advanced that local epoch, but it will not execute the unsafe physical RemoveAll. For the new client path, I would move local cache/epoch mutation after successful completion of the new RPC.

Regression coverage

For the first regression:

Put tenants A and B on the same LocalSSD holder, with a physical object for each.
Call tenant A RemoveAll(false) and require the capability error.
Verify A/B metadata and replicas, quota/accounting, LocalSSD usage, offload/promotion state, mailbox state, and both physical objects are unchanged.
Repeat from a fresh fixture with force=true.
Add a control with LocalSSD/offload disabled showing memory-only RemoveAll(A) still succeeds and leaves B unchanged.

I would keep generation/ACK redesign, durable delete markers, failover recovery, mutation epochs, tenant-aware physical deletion, offline delete queues, and broader mailbox changes out of this Phase-1 PR.

If this compatibility shape looks right, I can proceed with the narrow implementation and regression coverage.

@200lz commented on GitHub (Sep 1, 2026): Thanks. I traced the current path again on origin/main (4956149bcba2af5dcb99d51a0e7365dc07025f3e). Based on your guidance, I propose the following Phase-1 design. Containment invariant A tenant-scoped RemoveAll must never fall through to an unscoped physical cleanup. If the active backend/mailbox path cannot guarantee tenant-scoped deletion, reject the request: before the first tenant metadata/accounting mutation in MasterService::RemoveAll(const TenantId&, bool); before publishing any LocalSSD RemoveAll mailbox request. force=true does not bypass this capability check. Physical keys are already tenant-scoped through TenantId::MakeScopedKey(). The missing capability is deletion: StorageBackend exposes only unscoped RemoveAll(), and the LocalSSD mailbox currently carries only an unscoped remove-all signal. For Phase 1 I would use the conservative capability-level predicate rather than inspecting current replicas. If real multi-tenancy is active and the LocalSSD/offload cleanup path is enabled, tenant-scoped physical deletion cannot currently be guaranteed, so tenant RemoveAll fails closed. With LocalSSD/offload disabled, memory-only tenant RemoveAll remains supported. RPC compatibility I would keep the existing scalar RemoveAll RPC and add a separate versioned/error-capable handler. I did not find an established *V2 naming convention, so I would leave the final method name open for review. new client → new server: call only the new handler. Capability rejection returns an explicit application error. Local cache/backend cleanup happens only after RPC success. new client → old server: the new handler is unregistered, so the client fails closed and does not retry through legacy RemoveAll. old client → new server: retain the legacy scalar success behavior for safe cases. For a capability-rejected tenant request, return an RPC-level failure rather than scalar 0, because old clients perform local unscoped storage_backend_->RemoveAll() after any successful scalar response. One compatibility detail: current old clients bump their local hot-cache epoch before making the RPC. Therefore an old client that receives this RPC failure may still have advanced that local epoch, but it will not execute the unsafe physical RemoveAll. For the new client path, I would move local cache/epoch mutation after successful completion of the new RPC. Regression coverage For the first regression: Put tenants A and B on the same LocalSSD holder, with a physical object for each. Call tenant A RemoveAll(false) and require the capability error. Verify A/B metadata and replicas, quota/accounting, LocalSSD usage, offload/promotion state, mailbox state, and both physical objects are unchanged. Repeat from a fresh fixture with force=true. Add a control with LocalSSD/offload disabled showing memory-only RemoveAll(A) still succeeds and leaves B unchanged. I would keep generation/ACK redesign, durable delete markers, failover recovery, mutation epochs, tenant-aware physical deletion, offline delete queues, and broader mailbox changes out of this Phase-1 PR. If this compatibility shape looks right, I can proceed with the narrow implementation and regression coverage.