[kunkunblueberry]

Description

First implementation slice of the RFC discussion in #4700 (Rethinking the Master Global State Lock). See the issue for the full audit (69 guard sites, interleaving risks, alternatives A/B/C). This PR implements Section 7.1 (restore lifecycle) only.

Motivation: PR #4406 removed the legacy Master snapshot path but kept snapshot_mutex_ (renamed master_state_mutex_) and its ~69 guard sites, many of which now protect lifecycle interactions that merely accumulated around the original barrier. Before any of those guard sites can be removed, the restore window needs an explicit owner.

What this slice does:

• Adds MasterService::RestoreLifecycleState (kConstructed, kRecovering, kReady, kServing, kFailed) with the transitions wired where they exist today: HA promotion restore moves Constructed -> Recovering -> {Ready, Failed}; the supervisor's ActivateServingState takes Ready -> Serving; the non-HA path and EmbeddedMaster open Constructed -> Ready -> Serving explicitly.
• RestoreFromStandbyState becomes a lifecycle wrapper around the existing implementation (renamed RestoreFromStandbyStateCore). Entry from Serving is refused per the RFC's proposed no-cycles-out-of-Serving constraint; the existing restore retry semantics (replacement restore from Ready/Failed, exercised by the HA tests) are preserved.
• Adds AdmitsBusinessWorkers() and pauses the business background workers (eviction, client monitor, task cleanup, job dispatch, NoF heartbeat, dynamic replication, replica cleanup) while a promotion restore is in flight, independent of master_state_mutex_. kConstructed keeps workers admitted so direct construction in tests keeps the constructor-starts-workers behavior; tightening that (pause-until-activation) is deliberately left to the follow-up slices once callers carry explicit activation.

What Serving means

Serving is a commitment about restore, not an observation about reachability. After it, the instance refuses restore for the rest of its life; it says nothing about whether the RPC endpoint has accepted a request yet. The commit point lands on both sides of the publish boundary, and the placement is per-path:

entry point	commit point	why there
non-HA master	before RegisterRpcService	no restore path exists, so committing early costs nothing
EmbeddedMaster	before RegisterRpcService	same reasoning; aligned with the non-HA path
HA supervisor	after PublishServiceReady succeeds	the term is only committed once readiness is out

A failed HA publication never falls back to Recovering: release_service() destroys the per-term WrappedMasterService, and the retry builds a fresh instance that starts at Constructed. That is why the retry path needs no Serving -> Recovering edge.

The invariant the follow-up admission slices can rely on is the same on all three paths: once Serving is observed, restore cannot happen for that instance, so an admission decision taken in that state needs no restore-window reasoning. It must not be used as a "service is up" flag.

master_state_mutex_ stays exactly where it was. This slice changes no locking behavior — it is the foundation the follow-up slices (remount prepare/rebind/publish, endpoint readiness ownership, LOCAL_DISK registration admission) build on before the global lock is removed from ordinary paths.

RFC: #4700

Suggested Merge Priority

• P0 — Urgent
• P1 — High
• P2 — Normal
• P3 — Low

Module

• Transfer Engine (mooncake-transfer-engine)
• Mooncake Store (mooncake-store)
• Mooncake Conductor (mooncake-conductor)
• Reshard (mooncake-reshard)
• Mooncake EP (mooncake-ep)
• Mooncake PG (mooncake-pg)
• Integration (mooncake-integration)
• P2P Store (mooncake-p2p-store)
• Python Wheel (mooncake-wheel)
• Common (mooncake-common)
• Mooncake RL (mooncake-rl)
• CI/CD
• Docs
• Other

Type of Change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation update
• Performance improvement
• Other

How Has This Been Tested?

Release build (gcc 13, Ninja) plus a new dedicated unit-test target. Rebased onto main@b7a50b39:

Test commands:

./master_service_restore_lifecycle_test            # 9/9 passed
./master_service_test                              # 84/84 passed
./embedded_master_test                             # 9/9 passed (includes the new case)
./master_service_ha_test                           # 134/138 passed

Test results:

• Unit tests pass
• Integration tests pass (if applicable)
• Manual testing done

New master_service_restore_lifecycle_test covers: fresh service starts kConstructed with workers admitted; successful restore reaches kReady; restore after ActivateServing is refused (UNAVAILABLE_IN_CURRENT_STATUS, no Serving -> Recovering edge); retry after a failed restore still reaches kReady (HA-tested retry semantics preserved); the non-HA Constructed -> Ready -> Serving path; the worker-admission predicate across all five states; and a scheduler-independent overlapping-restore admission test.

New EmbeddedMasterTest.StartCommitsRestoreLifecycleBeforeServing asserts the embedded path reaches kServing before it serves RPCs, and re-commits after a Stop()/Start() cycle rather than inheriting the old state.

Full disclosure on master_service_ha_test, which reports 4 failures in this environment:

• Four are stable and reproduce identically on pristine main@b46254de with this PR's files reverted, so they are environmental rather than caused by this change: WeightMetadataAcceptedTerminalFailureCompletesWithoutPublishTimeout, BatchEvictQueueSaturationDoesNotRequestAnotherPass, BatchEvictStopsWithoutRetryWhenOpLogWriterIsFenced, BatchEvictCommitFailureRestoresRemovedReplicas. All four are CaptureStderr assertions that observe an empty capture. I verified the reproduction on unmodified main rather than root-causing the glog/stderr interaction in this build.
  None of the four exercise the lifecycle code this PR adds, and CTest on CI is green.

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
• For changes >500 LOC: I have filed an RFC issue (RFC already exists: [RFC]: Rethinking the Master Global State Lock: Lifecycle Isolation and Finer-Grained Synchronization #4700)

AI Assistance Disclosure

• No AI tools were used
• AI tools were used (specify below)

The change was drafted with AI assistance (state-machine design, worker gating points, and the new test suite); the human submitter reviewed every changed line and can defend the change end to end. Build and test results above were produced on a private build host (gcc 13, conda-forge deps, rdma-core v46).

[kunkunblueberry]

CI status update: main currently does not build its test tree — #4314 changed MasterService::BatchProbeKey to take a GrantLeasePolicy but the MasterScenario DSL still called the two-argument form, so master_service_test_dsl fails to compile and the CTest job dies before running any test. That is unrelated to this PR, and it is why the first CTest run failed here.

To keep this branch verifiable, it now also carries the one-line fix from #4742 (GrantLeasePolicy{}, i.e. the same lease-free probe the DSL step used before). Once #4742 lands I will drop that commit and rebase. The Check code format failure was mine and is fixed (clang-format-20 verified locally on both touched files).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[kunkunblueberry]

Rebased onto current main (19e0ab28) and dropped the temporary [Bugfix] Pass a lease policy... commit: #4729 landed the same DSL fix, so this branch is now exactly one commit again and contains nothing but the lifecycle slice. Re-ran the suites on the rebased tree: lifecycle 8/8, master_service_test 69/69, master_service_ha_test 127/130 (the 3 BatchEvict* failures are the pre-existing environment-only ones documented in the description).

[kunkunblueberry]

Concurrency self-review of the slice, as a follow-up commit (ab40f56d). One of the three questions turned up a real interleaving; it is fixed.

1. Overlapping restores. The wrapper was not a single-flight admission: it loaded the state, then stored Recovering as a separate step. Two overlapping callers could therefore both pass admission, and the first one to finish would publish Ready while the second was still inside the core rewriting metadata — and since Ready admits business workers, that window also resumed eviction/cleanup against a half-rebuilt state. The core still holds master_state_mutex_ exclusively, so the metadata writes themselves serialized, but the lifecycle state did not describe reality.

Admission is now the transition itself: compare_exchange_strong(expected, kRecovering). A restore that overlaps an in-flight one is refused with UNAVAILABLE_IN_CURRENT_STATUS rather than queued, so "single admission point" is now true rather than aspirational. Sequential replacement restores are unchanged — a retry starts after the previous attempt has published Ready or Failed, which is exactly the path the HA allocator-rebind and discard-repair retries exercise. New test RestoreLifecycleOverlappingRestoreIsRefused starts a 20k-object restore, waits for Recovering, and asserts the overlapping call is refused while the in-flight one still reaches Ready with workers admitted.

2. What worker admission does and does not guarantee. It is evaluated once per worker iteration, so it gates the start of business work, not work already in progress: a worker that passed the check before the window opened finishes its current iteration. In this slice that is safe only because the restore core holds master_state_mutex_ exclusively, which is the actual exclusion. I documented that in AdmitsBusinessWorkers() and stated the consequence explicitly: a slice that removes the lock from these paths must add a quiesce step (drain or epoch) instead of relying on this predicate. I am deliberately not claiming the gate alone stops all in-flight tasks.

3. Failure / retry / stop boundaries. Covered and verified:

• failed restore → Failed, and Failed keeps workers paused (RestoreLifecycleFailedRestoreReachesFailed, RestoreLifecycleWorkersPausedOnlyInRestoreWindows);
• retry from Failed → Recovering → Ready (RestoreLifecycleRetryAfterFailedRestoreReachesReady);
• Serving → restore refused, and ActivateServing only from Ready;
• workers leave their loops on their existing running_ flags, which stop sets before joining, so there is no condition-variable wait that could miss a wakeup; the only latency is one sleep interval of the paused eviction loop. Work already inside an iteration finishes, as described above.

Re-ran everything on the new commit: lifecycle 9/9, master_service_test 69/69, master_service_ha_test 127/130 (the same three BatchEvict* environment-only failures as before, unchanged). Scope is unchanged — no remount, LOCAL_DISK, or lock removal in this PR.

[shivankur-code]

@kunkunblueberry @he-yufeng — a scoped test-reliability finding on ab40f56d695f226f6b40519d00d14032a9d1ee20, not a production lifecycle defect or a request to broaden this slice.

RestoreLifecycleOverlappingRestoreIsRefused has two scheduler-dependent failure paths before first.join():

1. The first restore can finish before the parent observes Recovering. A 20k-object workload makes this less likely, but does not synchronize it. The fatal ASSERT_EQ returns while first is still joinable, so its destructor calls std::terminate instead of reporting an ordinary failed test.
2. Even if the parent observes Recovering, it can be descheduled before calling the second restore. The first restore publishes Ready; the second restore is then a legal sequential replacement and succeeds. ASSERT_FALSE(second.has_value()) again returns before join(), terminating the test process. A passing poll does not pin the admission window.

Independent local validation: compiled a standalone C++20 scheduling harness with Zig 0.15.2's C++ driver, using the same admission-CAS/state ordering and a stub restore core. Promises forced each allowed schedule; a terminate handler made the result observable:

missed-window:   exit 86; expected Recovering, observed Ready; std::terminate
observation-gap: exit 86; sequential second restore succeeded; std::terminate
held-core gate:  exit 0; 1,000/1,000 schedules rejected the overlapping restore

This is not a native Mooncake/gtest run, a measured CI flake rate, or HA/RDMA validation. It isolates the scheduler/std::thread cleanup behavior; the source anchors were checked against the SHA above. The minimal C++ lifetime rule behind the abort is reproducible without Mooncake:

#include <cstdlib>
#include <exception>
#include <future>
#include <thread>
void fatal_assertion_path() {
    std::promise<void> finished;
    auto done = finished.get_future();
    std::thread first([&] { finished.set_value(); });
    done.wait();
    return;  // Fatal ASSERT_* returns too; first is still joinable.
    first.join();
}
int main() {
    std::set_terminate([] { std::_Exit(86); });
    fatal_assertion_path();
}

Possible small fix without a new production hook: the existing MasterServiceTestPeer::MasterStateMutex can hold the restore core after the wrapper publishes Recovering:

• Acquire that mutex exclusively before launching the first restore. The wrapper's admission CAS precedes the core's exclusive lock acquisition.
• Wait for Recovering, then attempt the second restore while the first cannot finish. That call should fail at admission, before taking the core locks. One object is sufficient; 20k is unnecessary.
• Use nonfatal checks/recorded outcomes before cleanup, and explicitly unlock before joining. Only then make fatal assertions. If using RAII thread joining, ensure destruction order cannot join while the mutex is still held.

This would make the regression test the intended overlap rather than scheduler luck, while preserving the PR's explicit distinction between worker admission and actual quiescence. Happy to keep this as an author-side adjustment rather than open a competing lifecycle PR. AI-assisted review; the scheduling harness was executed locally, with the native-test limitation above.

[kunkunblueberry]

Both paths are real, and the terminate behaviour is the worse half of it: ASSERT_* returns from the test body, so the joinable std::thread destructor calls std::terminate and the run reports an abort instead of a failed expectation. Fixed in 37e42ab2 — author-side, as you suggested, no new production hook.

Deterministic window instead of a lucky one. The test now takes MasterServiceTestPeer::MasterStateMutex(service) exclusively before launching the first restore. Admission is the compare_exchange into Recovering and it runs before the core acquires that lock, so the wrapper publishes Recovering while the core cannot make progress. The overlapping call then hits the admission CAS and returns before it ever touches the core lock, which is exactly the property under test. One object is enough; the 20k payload is gone.

Cleanup before fatal checks. The overlapping call runs on its own thread with a bounded wait. core_hold.unlock() and both join()s happen before the first ASSERT_*, so a failed expectation now reports normally. The bounded wait also covers the other direction: if admission ever regressed into a blocking wait on the core lock, the test fails with the overlapping restore blocked on the core lock instead of being refused at admission instead of hanging the suite for the CI timeout.

Evidence. Lifecycle suite 9/9. The specific case 50/50 with --gtest_repeat=50, zero failures, on gcc 13 Release. That is a repetition check, not a scheduler-forcing harness — I did not reproduce your promise-forced schedule enumeration, so I am not claiming coverage of every interleaving; the claim is narrower: the test no longer depends on the restore finishing slowly or on the parent thread being scheduled between the poll and the second call.

Also worth saying explicitly, since it constrains the next slice: the test now depends on admission preceding the core lock. If a later change moves admission inside the core critical section, this test will fail rather than silently pass, which is the right failure mode — the single-flight property would genuinely be gone at that point.

[kunkunblueberry]

CI note + review request.

The last CTest failure was infrastructure, not this change. The job died in mooncake-p2p-store while verifying Go module checksums:

golang.org/x/xerrors@...: verifying go.mod: reading https://sum.golang.org/tile/8/0/x002/013:
stream error: stream ID 321; INTERNAL_ERROR; received from peer

That is sum.golang.org returning an error mid-download, in a target this PR does not touch. I could not re-run the job (no write access), so I rebased onto current main (0e5b4b30) and pushed; a fresh run is in progress.

@he-yufeng @Icedcoco — this is ready for review. It is the focused slice @Icedcoco asked for: restore lifecycle + admission, no lock removal. Three commits, ~600 lines including tests:

1. [Store] Add restore lifecycle state machine and worker admission — Constructed → Recovering → Ready → Serving, plus Failed; the only refused edge is out of Serving; business background workers are gated off during Recovering/Failed. Zero locking changes.
2. [Store] Make restore admission single-flight — admission is the transition (compare_exchange into Recovering), so an overlapping restore is refused instead of being able to publish Ready while another one is still rebuilding.
3. [Store] Make the overlapping-restore test scheduler-independent — the regression test holds the core lock through the test peer so the overlap window is deterministic, and joins before any fatal assertion.

Three things I would most like an opinion on:

• Refuse vs queue for an overlapping restore. Refusing with UNAVAILABLE_IN_CURRENT_STATUS is what makes single-flight checkable, but it changes the HA supervisor path from "block until the in-flight restore finishes" to "get an error and retry". I believe the supervisor retries on restore failure, but a maintainer confirming that would settle it.
• kConstructed still admits workers so embedded/test construction keeps its current behaviour; tightening that to "no business workers before Ready" is a separate, riskier change.
• The test depends on admission preceding the core lock. If a later change moves admission inside the critical section, the test fails rather than silently passing — which is the behaviour I want, but it does couple the test to that ordering.

Non-goals, to keep the review bounded: no remount, no LOCAL_DISK, no lock-site changes, no oplog changes.

Local results on the rebased tree (gcc 13 Release): lifecycle 9/9, master_service_test 71/71, master_service_ha_test 127/130 — the same three BatchEvict* failures that fail identically on the unmodified tree in this environment (documented in the description, unrelated to the lifecycle state).

[Icedcoco]

Thanks for the update. I appreciate the work on this lifecycle foundation, and I agree that introducing it in stages makes sense as the later slices add admission control.

I had one clarifying question about what Serving is intended to represent. In the regular non-HA startup path, ActivateServing() is called before RPC registration, while EmbeddedMaster exposes the service with the restore lifecycle still in Constructed. Is Serving meant to mark the point where the service is committed to activation and restore should no longer be allowed, rather than the point when it is already externally reachable? If so, is bringing the embedded path into alignment something you intend to address in a follow-up, or is its current state intentional for this slice?

I’m mainly hoping to understand the lifecycle contract that the later admission-control work will build on. Thanks again for the thoughtful change.

[kunkunblueberry]

Thanks — that reading is right, and it exposed a real inconsistency that I fixed rather than deferred.

Serving is a commitment about restore, not an observation about reachability. After it, this instance refuses restore for the rest of its life. It says nothing about whether the RPC endpoint has accepted a request yet; anything that needs "is it reachable" must consult the transport.

One correction to the framing, though. The non-HA path does commit before RPC registration, but the HA supervisor does the opposite: ActivateServingState takes Ready -> Serving after PublishServiceReady succeeds. So the commit point lands on both sides of the publish boundary, and the placement is per-path:

entry point	commit point	why there
non-HA master	before RegisterRpcService	no restore path exists, so committing early costs nothing
EmbeddedMaster	before RegisterRpcService	same reasoning; now aligned with the non-HA path
HA supervisor	after PublishServiceReady succeeds	the term is only committed once readiness is out

For HA this is not a style choice: a failed publication never falls back to Recovering. release_service() destroys the per-term WrappedMasterService and the retry builds a fresh instance starting at Constructed. That is why the retry path needs no Serving -> Recovering edge, and also why moving ActivateServing earlier there would buy nothing — the instance is discarded either way.

What the next slices can rely on is the same on all three paths: once Serving is observed, restore cannot happen for that instance, so an admission decision taken in that state needs no restore-window reasoning. What it must not be used for is "the service is up".

Changes in 6b5a6c50:

1. EmbeddedMaster::Start() now runs MarkReadyForServing() + ActivateServing() before RegisterRpcService, mirroring the non-HA path — two lines plus a comment.
2. The ActivateServing doc comment was wrong on its own terms: it said "called once the RPC service is registered and exposed", which the non-HA path already contradicted. It now states the contract above, including the per-path commit points.
3. EmbeddedMasterTest.StartCommitsRestoreLifecycleBeforeServing asserts kServing after Start(), and again after a Stop()/Start() cycle, since a restart builds a fresh service and must re-commit rather than inherit.
4. Rebased onto main@b46254de (the branch had drifted 39 commits).

Worker behaviour is unchanged: kConstructed and kServing both admit business workers, so the embedded master's background threads behave exactly as before. The only difference is that the state now says what it always meant.

Local results (gcc 13 Release): lifecycle 9/9, master_service_test 81/81, embedded_master_test 9/9, master_service_ha_test 133/138. Of the 5 HA failures, 4 reproduce identically on pristine main with this PR's files reverted (all CaptureStderr assertions observing an empty capture), and the fifth passes 3/3 when run alone. Details are in the updated description.

[Icedcoco]

LGTM