kvcache-ai / kvcache-ai/Mooncake
[Store][RFC] Make local SSD RemoveAll acknowledged, tenant-safe, and race-free
- Dominant language
- C++
- Stars
- 6.6k
- Forks
- 1.2k
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 312
Description
## Context
This tracks pre-existing `RemoveAll` correctness problems discovered while narrowing #2987. It is not introduced by #2987, which should remain limited to file-per-key `ScanMeta` recovery and reconciliation.
The call chains below were confirmed by static inspection at `53c14e09`. Crash, failover, and filesystem-fault timelines still need deterministic integration tests.
## Current behavior
- Global and tenant-specific `MasterService::RemoveAll` both reduce local cleanup to the same `LocalDiskSegment::pending_remove_all` boolean.
- `PollRemoveAll` returns that boolean and clears it immediately, before the client acknowledges cleanup.
- The poll response contains no generation, tenant ID, or scope.
- On `true`, `FileStorage` invokes backend-wide `RemoveAll()`.
- The local backend has no tenant dimension, so a tenant-scoped master operation becomes a global SSD wipe.
- `StorageBackendInterface::RemoveAll()` returns `void`; filesystem/backend failures may only be logged.
- `StorageBackendAdaptor::RemoveAll()` resets counters after the call even if physical cleanup was incomplete.
- With eviction disabled, the file backend scans only top-level regular files under `root_dir_`, so nested file-per-key data can survive.
- There is no exclusive fence against queued writes, eviction, deletion, `ScanMeta`, or offload completion.
- `StoreObject` accepts caller-provided paths without one enforced managed-root and tenant-ownership rule.
## Failure timelines
### Tenant-scoped removal deletes another tenant's SSD data
1. Tenants A and B have local files on the same client backend.
2. The master receives `RemoveAll(tenant=A)`.
3. It removes A's master metadata and sets the segment's unscoped boolean.
4. The client polls `true`, with no tenant identity.
5. The client executes backend-wide `RemoveAll()`.
6. Files belonging to both A and B are deleted.
7. B's master metadata is retained and can still reference replicas that no longer exist.
This can cause read failures and, when an affected local replica is the only valid copy, data loss.
### A clear request is lost
1. The master clears `pending_remove_all` while handling `PollRemoveAll`.
2. The response is lost, or the client crashes before cleanup completes.
3. Later polls return `false`.
4. Remaining files are never retried because there is no durable unacknowledged request.
### A write crosses the clear boundary
1. A write or offload task is admitted under the old state.
2. Cleanup snapshots its deletion candidates or finishes its filesystem walk.
3. The old write commits afterward.
4. The file survives the clear.
The heartbeat currently fetches offload tasks before polling `RemoveAll`, making stale-task epoch validation necessary in addition to a local lock.
### Cleanup reports success while files remain
With eviction disabled, nested file-per-key data is skipped by the top-level directory walk. Backend errors are not returned, but adaptor counters are reset and the client logs that cleanup completed.
## Required invariants
- Every request has immutable scope identity: `Global` or `Tenant(tenant_id)`.
- Generation, poll response, ACK, local marker, offload task, and completion all carry the same scope identity.
- A tenant request never deletes, resets accounting for, or fences as completed data belonging to another tenant.
- If the backend cannot prove tenant ownership for every affected path, tenant-local cleanup is explicitly unsupported.
- The master retains each request until every targeted stable storage identity ACKs the same scope and generation.
- Polling is non-destructive and idempotent.
- Required generations survive master failover; applied generations survive client restart.
- Cleanup failure never advances the applied marker, sends an ACK, resets counters, or reports readiness.
- `ScanMeta` and offload are blocked for a pending scope until cleanup is committed and acknowledged.
- Old-generation writes and completions cannot commit after the clear boundary.
- All stored and deleted paths are confined to one managed root and have verifiable scope ownership.
- Absolute paths, `..`, and symlink escapes are rejected; unrelated root siblings survive.
- Global and tenant generations have a defined order: a global clear fences all scopes and dominates earlier tenant requests.
## Minimum safe transition
Before implementing the full protocol:
1. Stop translating tenant-specific `RemoveAll` into the unscoped poll boolean.
2. Because the current backend cannot identify tenant-owned files, return an explicit `UNSUPPORTED_TENANT_LOCAL_CLEANUP` error before mutating master metadata when physical tenant cleanup is required.
3. Never silently substitute a global SSD clear.
4. If logical-only tenant deletion is desired, expose it as an explicit operation that leaves physical files for later scoped GC; do not report physical cleanup as completed.
This containment change should land independently and urgently.
## Proposed generation and ACK protocol
Persist a master request record:
```text
{ scope, generation, target_storage_ids, acked_storage_ids, state }
```
Use a stable, locally persisted storage identity rather than only an ephemeral client UUID. The client persists applied generations outside the managed-data subtree:
```text
applied_global_generation
applied_tenant_generation[tenant_id]
```
Mount/poll returns pending request records without modifying them. ACK is monotonic and idempotent:
```text
AckRemoveAll(storage_id, scope, generation)
```
For a supported request, the client:
1. Enters `Removing(scope, generation)`.
2. Acquires the appropriate mutation fence.
3. Drains or cancels old-generation mutations.
4. Deletes only paths proven to belong to the requested scope.
5. Verifies physical files, queues, maps, pending operations, and counters.
6. Durably persists the applied marker.
7. Sends the ACK.
8. Returns to ready only after the ACK succeeds or a status query confirms it was recorded.
A crash before marker persistence reapplies the generation. A crash after marker persistence resends the ACK without repeating deletion. A lost ACK response is handled by retry/query.
Offload tasks and completions must carry an epoch and be rejected if stale at commit time.
## Backend contract
Introduce backend state such as `Ready`, `Removing(scope, generation)`, and `Failed`.
Normal mutators hold an operation lease through their commit point. Global cleanup takes an exclusive backend lease; tenant cleanup may use a scope-aware lease only after tenant ownership is represented safely.
`RemoveAll` must return a checked result rather than `void`. Counters and queues are committed as empty only after verified physical cleanup. Partial failure leaves the request pending.
The backend should construct canonical managed paths itself. Tenant cleanup requires either a tenant-partitioned layout or an authoritative per-file tenant index shared by store, scan, eviction, and deletion. Until then it remains unsupported.
## Implementation phases
1. **Containment:** reject unsupported tenant-local wipes and add scope/generation observability.
2. **Backend safety:** managed-path enforcement, checked cleanup results, mutation fence, and correct file-per-key cleanup in both eviction modes.
3. **Protocol:** durable scoped generations, stable storage identity, non-destructive poll, ACK, readiness gating, and stale-task rejection.
4. **Recovery and backends:** client/master crash tests, failover recovery, and checked contracts for bucket and offset backends.
## Core test matrix
| Scenario | Required assertion |
|---|---|
| Tenant A clear with A and B sharing an SSD backend | B files and B accounting remain intact |
| Current backend receives tenant-local cleanup | Explicit unsupported error; no master mutation or global wipe |
| Poll response is lost | Request remains pending |
| Crash before, during, or after physical deletion | Same generation retries safely |
| Crash after marker persistence but before ACK | ACK is resent without another destructive clear |
| Master failover with pending or recorded ACK | Scoped generation state is preserved |
| Eviction enabled and disabled | All managed file-per-key data in the requested scope is removed |
| Write admitted before the fence | It finishes before deletion or is canceled |
| Write/offload attempted after the fence | It cannot commit until the scope is ready |
| Task fetched before a generation change | Stale completion is rejected |
| `ScanMeta` while cleanup is pending | No re-registration for the pending scope |
| Partial deletion or marker-fsync failure | No ACK and no false zero counters |
| Absolute, traversal, symlink, or out-of-root path | Store/delete is rejected |
| Unrelated managed-root sibling | It survives |
| Tenant request followed by global request | Ordering is monotonic; global clear dominates safely |
| Bucket/offset cleanup failure | Checked error or explicit unsupported result |
Use deterministic barriers and injected errors for races and partial failures, followed by process-kill and master-failover integration tests.
## Acceptance criteria
- Tenant-scoped removal cannot delete another tenant's local files or leave its retained metadata pointing at replicas deleted by this operation.
- A request cannot be lost through RPC loss, retry, client crash, or master failover.
- No mutation can commit across a scoped generation boundary without epoch validation.
- Successful ACK means the exact requested scope is physically and internally empty.
- Failed cleanup remains visible and retryable.
- Unsupported tenant cleanup fails explicitly and never degrades into a global wipe.
Contributor guide
Research direction
Trace MasterService::RemoveAll and PollRemoveAll through LocalDiskSegment, FileStorage, StorageBackendInterface::RemoveAll, StorageBackendAdaptor::RemoveAll, StoreObject, ScanMeta, and offload handling. Start with the confirmed call chains and use deterministic barriers, injected failures, process-kill, and master-failover tests. Done means scoped cleanup is explicit or safely acknowledged, failures remain retryable, and stale mutations cannot commit across a generation boundary.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100