[Aionw]

Description

Step 2 of #4557: promotion retry candidates move onto their object's ObjectState, and the retry rules move into a PromotionCandidateTracker.

A candidate (a key whose LOCAL_DISK -> MEMORY promotion a transient gate turned away) sat in a per-tenant map beside the object map, and its rules (global limit, backoff, TTL, retry budget) were spread over eight MasterService helpers. The retry loop walked 64 of the 1024 shards per round and every candidate in them, due or not, and a candidate outlived its object until the loop happened to reach its shard, holding a slot toward the global limit meanwhile.

• Candidate on the object. route::ObjectState gains promotion_candidate (HeapOptional, so an object without one pays a pointer). TenantState loses its promotion_candidates map. A candidate is not work in flight: HasWorkInFlight() ignores it.
• PromotionCandidateTracker. It owns the global count against the limit, the backoff/TTL/retry-budget rules, and an index of the keys carrying a candidate, ordered by when each is due. It owns no object: its *Locked methods act on a state whose shard lock the caller holds, and every change to a candidate goes through them. Lock order is shard lock -> index lock; the index lock is a leaf.
• Retry round. RunPromotionCandidateRetry reads up to 128 due keys from the index, the longest overdue first, and resolves each again under its shard lock. The shard-slice cursor is gone, and a round no longer touches candidates that are not due.
• Lifetime. EraseMetadata drops the candidate with its object. An index entry whose object went away by another path is dropped when the retry round or a backoff finds the key gone.
• The limits and timings are unchanged (50000 candidates, 64 retries, 5 min TTL, 10 ms to 5 s backoff), and so are the metrics.

Behavior is otherwise unchanged: no RPC, snapshot or OpLog format changes.

Part of the code comes from #4370 by @CAICAIIs (the per-object state the candidate moves onto).

This is a transitional shape. The *Locked interface follows the end state described in #4557, where per-key state lives on ObjectState and changes only under the key's lock, so the later steps of the series build on it unchanged. Its API is still cut along the old MasterService call sites rather than designed as one cohesive class: the caller assembles part of the retry protocol itself (look the key up, ForgetLocked it when the object is gone, choose between backoff and erase with IsTransient). It will be refactored after the series lands, toward a self-contained retry component that owns the retry loop and reports outcomes; ForgetLocked and the EraseMetadata hook go away once step 9 adds generation-checked ObjectRefs.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Refactor

How Has This Been Tested?

Test commands:

make -j14 promotion_candidate_tracker_test promotion_on_hit_test master_service_test \
  master_service_test_for_snapshot master_service_ha_test master_service_ssd_test \
  master_service_offload_scenario_test offload_on_evict_test dynamic_replication_test
# then each binary, in parallel

Test results:

• Unit tests pass

• Integration tests pass (if applicable)

• Manual testing done (describe below)

• New promotion_candidate_tracker_test (11 cases): recording, refresh keeping failure history, erase/consume, backoff refiling, due order and limit, TTL and retry-budget expiry, forgetting a stale entry, tenant separation, reset, and which gates are transient.

• promotion_on_hit_test covers the wired-in retry path end to end.

• 391 of 391 pass across 9 suites: promotion_candidate_tracker_test (11), promotion_on_hit_test (55), master_service_test (69), master_service_test_for_snapshot (69), master_service_ha_test (127), master_service_ssd_test (7), master_service_offload_scenario_test (25), offload_on_evict_test (1), dynamic_replication_test (27). 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.

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 and all hooks pass
• I have updated the documentation (if applicable)
• I have added tests to prove my changes are effective (promotion_candidate_tracker_test; promotion_on_hit_test covers the wired path)
• 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 working on this refactor. Keeping the promotion retry candidate with the object, together with a due-time index, looks like a useful direction. It should make candidate cleanup easier to reason about and avoid repeatedly scanning unrelated metadata shards.

Really nice refactor—the motivation is clear and this looks like a great improvement overall.

I just had a couple of quick questions and wanted to see what you think about a few edge cases:

1. Bookkeeping invariants & potential edge cases
   Looking at RecordLocked(), it updates ObjectState::promotion_candidate, due_, and count_ across distinct steps. It seems sound for standard flows, but I was wondering how it behaves if an object is deleted and then recreated with the identical (tenant, key).

   Would it be worth adding a targeted unit test just to confirm that slot counts don't leak and Empty() stays accurate across key recreation? Also, curious if you think a helper returning was_inserted could streamline the index/count updates, or if keeping them decoupled has specific advantages here.

2. Enforcing the locking contract
   The comments clearly state that callers of *Locked() must hold the shard lock. Do you think it's worth adding a debug assertion or scoping this through a metadata accessor to help guard future call sites, or do you think comment-level documentation is enough for our current usage?

Would love to hear your thoughts on these points.

[he-yufeng]

Read the whole change end to end and built it locally (mc-dev container, aarch64, Release): both targets compile clean, the 11 new tracker tests pass, and the 7 RetryCandidate_* cases in promotion_on_hit_test pass, including the two adapted to the no-arg retry driver.

The accounting holds on every path I walked. RecordLocked refresh keeps the slot and refreshes the budget without touching execution_failures; the new-candidate path reserves via CAS and inherits the stale slot when File finds a same-key entry a dead object left behind, so the count stays exact; EraseLocked/ConsumeLocked/ForgetLocked/Reset each release exactly what they hold. order_ and due_ stay consistent throughout (Unfile drops the order entry before the map node, File replaces in that order). The Due() snapshot copies keys out under the lock, so the retry loop never holds a pointer into the index while re-resolving under the shard lock.

Lock ordering is clean: shard lock first, tracker mutex second, and the tracker never calls back into shard code, so it is a leaf. HasWorkInFlight() excluding the candidate is the right call; a bare candidate is metadata, not in-flight work, and the drift that creates (object gone, entry behind) is exactly what ForgetLocked and the stale-slot inheritance repair.

Two behavior notes worth stating out loud, both fine by me:

• Candidates now die with the object via EraseMetadata. Under the tenant map they could linger until TTL and keep charging the 50000 cap for a key that no longer existed, so this closes a real leak window.
• A TTL-expired candidate is only reaped once it becomes due. With the backoff capped at 5s against a 300s TTL that gap is theoretical.

Interaction with open work: my #4521 touches ObjectState teardown paths nearby and calls ErasePromotionTaskIfPresent on replica removal, which is the task field, not this candidate, so there is no semantic conflict. Whichever of the two lands second will need a small mechanical rebase around the ObjectState layout. No preference on order from my side, just flagging it so it does not surprise whoever merges second.

Nothing blocking from me.

[Aionw]

Thanks both for the reviews.

To frame the scope: this PR is a preparatory step for the object-lock work (part of the #4370 split, tracked in #4557). It changes only where the retry candidate lives. The candidate moves from a tenant-level map onto ObjectState, so it shares the object's lifetime and will fall under the object's lock once that lands. Who guards it is deliberately left as is. For now that is still the shard lock, held by the caller and documented on the *Locked() methods.

A follow-up refactors this as a whole so the lock is self-contained: the object owns its lock, and callers no longer carry a "hold the shard lock" contract.

@Icedcoco, that is also how I'd answer your two points:

1. Recreating the same key. Correctness already holds in this PR. ForgetDropsAStaleEntry covers an object that disappears without telling the tracker, followed by a new object recording under the same (tenant, key): there is one entry and the slot count stays exact. EraseAndConsumeUnfile covers the normal erase-then-recreate path. Reshaping the bookkeeping API (e.g. a was_inserted helper) would have to be redone once the object owns its lock, so I'd fold it into that refactor.
2. Enforcing the lock contract. Agreed that it belongs in code rather than comments, and the refactor is where that happens. Here the tracker only sees ObjectState&. Asserting on the shard lock would mean widening its interface for a contract the follow-up removes.

@he-yufeng, thanks for the end-to-end walk and the local run. Noted on #4521; I'm fine rebasing whichever lands second.

[CAICAIIs]

lgtm