[Aionw]

Description

Step 10 of #4557: MasterService walks a tenant and its objects through shard-backed cursors, and Remove, RemoveByRegex, BatchRemove and both RemoveAll overloads are built on that walk and on the per-key accessor from step 9 (#4717).

The removes each walked the 1024 shards by hand, found the tenant's state, iterated its map, and carried their own copy of the lease, replica and replication-task checks and of the HA mark-removed-and-persist step.

• Iteration primitives. VisitTenants calls back with each tenant that holds an object, holding no lock, so the callback may reach any key. ReadCursor(tenant) and WriteCursor(tenant) walk one tenant's objects under each shard's read or write lock and hand out the same Object view (key, generation, ref(), metadata, state) as the route's cursor, so the swap in step 15 keeps the loop bodies as they are. A write cursor may change an object's metadata and state but not publish or tear one down; a body that acts later keeps ref() and comes back through MetadataAccessorRW(service, tenant_id, ref).
• Removes on the walk. RemoveByRegex and both RemoveAll overloads go through one RemoveTenantObjects: a ReadCursor pass names the objects that look removable, and each is checked again and removed under its own write lock, so an object republished meanwhile is left alone. BatchRemove holds each key through MetadataAccessorRW instead of grouping the keys by shard, so like the single-key Remove it runs the invalid-memory-replica cleanup first. All four share CheckRemovable and MarkRemovedAndPersist.
• ObjectRef accessor. It no longer runs the invalid-memory-replica cleanup: that cleanup's KV sync bumps the tenant epoch, which would make RemoveAll suppress its own cleared. Its only other callers are the durable finalizers, which run with the OpLog on, where the cleanup is a no-op.
• Empty tenant rows. RemoveAll now visits only tenants that hold an object, so it no longer sweeps the empty rows a failed standby restore left behind; the restore rollback drops them itself.
• Smaller behavior notes. The RemoveAll test hook fires once per tenant instead of once per shard. RemoveByRegex logs the keys it skips at VLOG(1) instead of LOG(WARNING).

Until the swap, RemoveAll(force) walks every shard once per tenant (VisitTenants then one ReadCursor each), where it used to visit each shard once; the namespace table removes that multiplier. No RPC, standby snapshot or OpLog format changes.

Part of the code comes from #4370 by @CAICAIIs.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Refactor

How Has This Been Tested?

Test commands:

make -C build -j14 -k
cd build && ctest -j8 --timeout 240 --output-on-failure
ctest -R '^pybind_client_test$' --timeout 600   # timed out at 240 s under -j8

Test results:

• Unit tests pass

• Integration tests pass (if applicable)

• Manual testing done (describe below)

• New master_service_tenant_iteration_test (7 cases): VisitTenants names only tenants holding objects, a ReadCursor visits each object of its tenant once, leaving a cursor early releases its shard, a WriteCursor changes state in place, RemoveByRegex stays in its tenant, RemoveAll walks every tenant, and RemoveAll leaves leased objects unless forced.

• Full ctest on top of origin/main (37642c5): 250 of 254 pass. pybind_client_test timed out under -j8 and passes alone (272 s). The other three are known local environment failures, unrelated to this change: tcp_transport_test and transfer_metadata_test need a network/RDMA environment this host lacks, and ordered_oplog_writer_test does not link locally (libaio). Built with GCC 16 (Release, BUILD_UNIT_TESTS=ON).

• Formatting: ./scripts/code_format.sh --check --changed-lines --base origin/main is clean with clang-format 20.1.8. pre-commit run --files <changed files>: codespell passes; cmake-format only rewrites lines in mooncake-store/tests/CMakeLists.txt that this PR does not touch (already on main), so those edits are left out.

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit on the files changed in this PR (see the formatting note above)
• I have updated the documentation (if applicable)
• I have added tests to prove my changes are effective (master_service_tenant_iteration_test)
• For changes >500 LOC: I have filed an RFC issue ([RFC]: Tenant-First Object-Level Metadata Locking #3670, tracked in [Track]: [Store] Land the tenant-first MasterService migration in staged PRs #4557)

AI Assistance Disclosure

• AI tools were used (specify below)

AI assistance was used for the refactor and to draft this description. The submitter has reviewed every changed line and can defend the change end to end.

[Icedcoco]

Thanks for putting this together. I understand this PR is groundwork for the #4557 migration, and the tenant-scoped cursors seem like a useful step in that direction.

I had a question about RemoveTenantObjects: it collects all eligible ObjectRefs for a tenant before deleting them. For tenants with a large number of objects, that could require a sizable temporary allocation. Do you think buffering the full candidate set is acceptable for the expected workloads, or would bounded batches be worth considering?

One other point for discussion: MarkRemovedAndPersist marks replicas as REMOVED before appending the oplog entry, but doesn’t restore their status if the append fails. I noticed the previous inline removal code had the same behavior, so this may be outside the scope of this PR. Since the logic is now centralized, though, should we handle the rollback here or track it separately?

Curious to hear your thoughts on both.

[Aionw]

@Icedcoco Thanks for the careful read!

Buffering the candidates. It's a consequence of the cursor contract: the cursor holds the shard lock across the loop body, so the body can neither tear an object down nor block on the oplog reservation. It keeps the ObjectRef and comes back under the object's own write lock, which matches how the NamespaceTable cursor will behave after the #4557 swap. An ObjectRef is just the key plus an 8-byte generation, a small fraction of the metadata already held for those objects, and RemoveAll() frees it between tenants. In exchange, the scan now takes only shard read locks, where main held each shard's write lock for its whole scan-and-erase, which is friendlier to online requests. Bounded batches would need a cursor that can resume from a position. The single-pass cursor deliberately doesn't offer one, so I'd revisit this once the NamespaceTable cursor lands, if it turns out to matter for these admin-only paths.

REMOVED without rollback. Good catch. Commit can still fail after a successful reservation (writer terminal, stopping, or entry validation), which left the replicas REMOVED with no durable callback to drop them. Since the logic is centralized now, I fixed it here in ea30bd1 rather than adding a rollback: MarkRemovedAndPersist collects the replica ids, queues the REMOVE, and marks the replicas removed only after the append succeeds. That's safe because the durable finalizer takes the same shard write lock the caller holds, so it can't run before the marks land. The other mark-then-commit sites outside the remove paths are left for a follow-up.

[Icedcoco]

LGTM pending CI

[CAICAIIs]

lgtm