[Store][RFC] Make local SSD RemoveAll acknowledged, tenant-safe, and race-free

[feichai0017]

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:

{ 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:

applied_global_generation
applied_tenant_generation[tenant_id]

Mount/poll returns pending request records without modifying them. ACK is monotonic and idempotent:

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.

[github-actions]

Thanks for opening this issue, @feichai0017!

Field	Value
Issue	#3231
GitHub user ID	132118805
Reporter	@feichai0017

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.

[he-yufeng]

Read through the whole chain; the three problems are all real on current main:

• MasterService::PollRemoveAll (master_service.cpp:6063-6067) reads pending_remove_all and clears it in the same lock, before the client has wiped anything. A client crash between the poll and the wipe loses the request forever, while the master has already deleted metadata, so master and SSD disagree permanently.
• The poll response is a bare bool with no generation and no tenant scope.
• FileStorage reacts to true by calling its own backend-wide RemoveAll() (file_storage.cpp:885-887), so the tenant-scoped master overload, which signals only the owning tenant's clients, still wipes every other tenant's data on those clients.

A shape that fixes all three without breaking the overlap benefit:

1. Replace the boolean with a monotonic per-segment remove_all_generation plus a tenant scope field. PollRemoveAll returns {generation, tenant_scope} and no longer clears anything.
2. Add AckRemoveAll(client_id, generation): the client calls it after its wipe completes, and only then does the master clear the flag, and only if the generation still matches (a stale ack after a newer RemoveAll is ignored). A crash before the wipe just makes the next poll return the same generation again, so the request is at-least-once instead of zero-or-once.
3. For the tenant-isolation part there are two honest options: (a) teach the local backend to remove by tenant scope (bigger but correct), or (b) stop signaling tenant-scoped wipes to clients entirely, so a tenant RemoveAll only touches master metadata and never risks a global SSD wipe (small, fail-safe, keeps the isolation breach from hurting anyone today). (b) can land first and (a) can follow when the backend grows tenant filtering.
4. Tests: crash-between-poll-and-wipe retry (flag survives), tenant-scoped RemoveAll not cross-wiping, stale ack ignored, generation bump on repeated RemoveAll.

Happy to build this if you and the maintainers want it — including doing (b) first so the isolation breach closes immediately and (a) as the follow-up. If you already have a different shape in mind I would rather review yours than duplicate it.

[he-yufeng]

@feichai0017 it's been a day, so one check before I start building: the design skeleton above (a: tenant-filtered RemoveAll with generation, b: fail-safe stop at the tenant level instead of broadcast RemoveAll). Does that match what you had in mind when you filed this? If the direction reads right I'll start the implementation; if you were picturing something different, better to hear it before the code exists.

[feichai0017]

Thanks for validating the call chain and for offering to implement this. The overall direction matches what I had in mind, especially landing the tenant-isolation containment first.

I would prefer option (b) as the first independent change, with one important constraint: the existing tenant-scoped RemoveAll should not silently become metadata-only if its contract requires physical cleanup. Because the current backend cannot prove tenant ownership, it should return an explicit unsupported error before mutating master metadata or signaling a backend-wide wipe. If a logical-only tenant deletion is useful, it should be exposed explicitly and leave physical cleanup to a later scoped GC path.

For the generation/ACK protocol, I think we need to preserve a few details from the RFC:

• The request, poll response, and ACK should all carry {scope, generation}.
• The ACK should use a stable storage identity rather than only an ephemeral client_id.
• A single per-segment {generation, scope} slot is not sufficient for unrelated tenant requests, since a Tenant B request must not overwrite an unacknowledged Tenant A request. A global request may dominate earlier tenant requests, but unrelated tenant scopes do not dominate each other.
• The master request state and ACK state need to survive failover.
• The client needs a durable applied-generation marker so that a crash after deletion but before ACK resends the ACK without performing another destructive clear.
• The backend must return a checked cleanup result before an ACK can mean success.

I am happy for the first PR to stay narrowly scoped to containment: stop tenant-scoped operations from becoming global SSD wipes, fail explicitly before metadata mutation, and add the cross-tenant regression tests. The durable generation/ACK protocol, mutation fencing, stale offload epoch validation, and tenant-filtered physical layout can then follow as separate changes.

That split would close the isolation breach immediately without accidentally treating the smaller first step as the complete race-free protocol.