[tensorcopy]

[Store] HA: release the leader lease after the SIGTERM drain (part 2 of #4710)

Description

This is the second half of #4710, on top of #4745. #4745 made an HA master end SIGTERM/SIGINT with a bounded, ordered drain and a terminal _exit, and left the lease to TTL expiry. This PR adds the one release it reserved: FinishShutdown now stops the term's renewal polling guard and calls ReleaseLeadership once, after the admin drain and immediately before the same exit. Reverting this PR restores #4745's behavior.

What changes:

• FinishShutdown (master_service_supervisor.cpp:194): when the supervisor holds the lease, it stops the LeadershipRenewalGuard, releases once, logs the result and returns the status. ShutdownLease carries the coordinator and the guard; they are set where the lease is acquired (:597) and where the guard is created (:623), so no shutdown_exit call site changes.
• The remote HTTP metadata cleanup worker stops before the release. Queued segment names are dropped, at most one in-flight DELETE completes or times out (the HTTP client's connect and request timeouts apply, and the 15 s shutdown bound covers the shutdown), and the worker skips the rest of its batch, so a local cleanup request cannot outlive the lease. The skip also applies when ~MasterService stops the worker outside HA, so a non-HA shutdown can now leave the second key of one segment uncleaned; before, the final batch ran to completion, while queued segments were already dropped. This is a deliberate trade-off.
• A release that an ordinary failure path already made is not repeated; its recorded result decides the status.
• Standby, candidate and starting phases hold no lease and release nothing, as before.
• A release that is not confirmed exits with status 2 and logs a WARNING. This matches the RFC's exit-status table.
• No change to the drain order, the 15 s watchdog, the second-signal exit or the signal handler. A hung release ends by the watchdog with status 1.
• Deployment guide: a graceful-shutdown section with the steps, the bound, the exit statuses, the failover latency before and after, and the Kubernetes grace-period note.

Exit statuses after this PR: 0 = the selected drain completed and the release succeeded or was not needed (released, already released, or none held); 2 = the drain completed but the release was not confirmed; 1 = watchdog expiry; 130/143 = second SIGINT/SIGTERM; 255 = request seen only after a fatal backend error made Start() return -1.

Status 2 means the release was not confirmed, not that the lease is certainly still held. If the lease remains, TTL expiry removes it. A timeout can also mean that the lease had already expired, or that the revoke reached etcd and its reply was lost. On the Kubernetes backend, 0 means the cancel request was accepted; it does not confirm the release.

Failover, on etcd. Before this PR a planned leader restart cost the old lease's remaining TTL plus the successor's warmup. Now, when the release is confirmed, the old key is removed during the shutdown, after the drain and before the process exits, so the successor does not wait for the TTL; warmup is unchanged. Measured in the new tests, from the signal, on etcd v3.6.1 with a v3.5.34 client and the default 5 s TTL (20 runs each; min / median / max):

Mode	Signal to key removed	Key removed to acquired	Signal to serving
SIGTERM	15 / 21 / 28 ms	4 / 6 / 25 ms	5024 / 5031 / 5051 ms
SIGKILL	4117 / 4216 / 4278 ms	4 / 5 / 12 ms	9126 / 9225 / 9289 ms

Both modes run with a remaining-TTL margin of at least 2.5 s read from the server just before the signal (2998 to 2999 ms in every run). The serving time includes the successor's 5 s warmup in both rows. The same loops on etcd v3.5.21 gave the same shape: SIGTERM key removal 18 to 29 ms and serving 5027 to 5037 ms; SIGKILL key removal 4132 to 4277 ms and serving 9142 to 9287 ms. The SIGTERM threshold in the test (the successor acquires within 2 s of the signal) is a performance target validated on 40 runs across the two servers, not an etcd guarantee. The SIGKILL lower bound comes from the lease's remaining TTL as read from the etcd server just before the kill. In a healthy local setup the saving is roughly the remaining TTL, typically 3-5 s per planned restart (4.2 s median here), minus the drain and release time; with a slow drain or release it can be smaller or negative. With the OpLog enabled the successor still runs its producer claim, catch-up and promotion before warmup.

Test notes. The admin-ordering test proves that the admin server has stopped before leadership release. It does not hold an admitted quota write across shutdown; that completion guarantee relies on the admin server's existing stop semantics. The RFC's admitted-quota-write test is not included here and can be added in a follow-up. The failed-release test cuts the child's etcd connection through a local TCP relay and, while the child's release fails, keeps the real lease alive from the test process with a keepalive only (never a revoke), so that the key can be checked at exit; it then stops that keepalive and waits for the key to expire by TTL.

What this PR does not change: independent lease loss, external revocation and keepalive failure keep today's limits; a client can still lose the response to a request that completed during the drain; the successor's warmup is not shortened; the k8s backend's release (CancelElection with ReleaseOnCancel) now runs on SIGTERM but is not covered by these tests, which use etcd. Its wrapper returns only the cancel result, and when no election monitor thread is running it returns without waiting for the election goroutine, so status 0 does not confirm a Kubernetes release. Planned restarts of a standalone (non-HA) master, such as the index-preserving handoff in #4666 and #4804, are a separate path and are unchanged here.

Size. About 1,400 lines changed (1,356 added, 42 deleted), mostly tests and docs; production is 110 changed lines (95 added, 15 deleted), the docs section is 49 lines, and the test file is 972 changed lines. The RFC estimated ~440 lines for part 2 when it still held the watchdog; the watchdog and its tests moved into #4745. The actual size is more than twice that estimate and above 500 lines. The excess is in the test file: the shared helpers (local TCP relay, test-side keepalive, key watch with event capture, failover test helpers) are about 500 lines, the five HA cases about 360, and the cleanup-stop tests about 250. The RFC (#4710) covers the series.

RFC: #4710. Builds on #4745.

Suggested Merge Priority

• P0 — Urgent: Critical security vulnerability, data loss/corruption, or widespread production outage requiring immediate action
• P1 — High: Significant correctness or stability issue in existing functionality, or a regression blocking users without a practical workaround
• P2 — Normal: Routine bug fix, validated performance/compatibility improvement, or new feature for a confirmed use case
• P3 — Low: Experimental feature, optional enhancement, refactoring, or cleanup with no urgent user impact

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?

1. high_availability_test, default build, against a real etcd: a serving leader, a leader in warmup and a predicate-driven shutdown exit 0 with master_view already gone at exit; SIGTERM racing a lease revoke exits 0 with one release result; a standby exits 0 and releases nothing; a leader-plus-standby pair measures failover from the signal with a watch started before it: after SIGTERM the successor acquires within 2 s (performance target), after SIGKILL the old key outlives the kill by at least the remaining TTL read from the server just before it (at least 2.5 s). Result: 29 passed, 9 skipped (the nine need fail points). The suite also includes a parser test for semicolon-separated endpoints, which needs no etcd.
2. Fail-point build (-DMOONCAKE_ENABLE_TEST_FAILPOINTS=ON): at a new fail point just before the release, the admin server is already stopped and the key is still present; with the child's etcd connection cut at that point, the release fails, the process exits 2, the key is present at exit and expires by TTL after the test's keepalive stops; a request after an ordinary release keeps that result and makes no second release; an ordinary release that fails and is followed by a request exits 2 with the single recorded result and no second release; a second SIGTERM during the drain still exits 143 and leaves the key to TTL. Result: 38 passed.
3. Negative control: the same test file, rebuilt against [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of #4710) #4745's unchanged production code (git diff --stat on mooncake-store/src and mooncake-store/include is empty; the only edit to the test file is kReleaseFailedStatus = 2 in place of the new header constant), fails the release assertions. The filter selects the serving-leader, warmup, predicate-driven and SIGTERM-failover cases: "Running 4 tests", "4 FAILED TESTS", none passed. The serving-leader case reports "the release precedes the exit: the key is gone at once" (Value of: ReadEtcdKey(key).has_value(), actual true, expected false). The failover case reports (m.acquired_ms) < (2000), actual: 4058 vs 2000, "after SIGTERM the standby must acquire without waiting for the TTL". The same filter on this PR's build: 4 passed. This control ran on the test file before the last two helper fixes; those fixes change neither production code nor the four assertions.
4. Repeated runs, fail-point build, etcd v3.6.1 (the server version CI uses): the admin-ordering, failed-release, failed-ordinary-release, SIGTERM-failover and SIGKILL-failover cases each passed 20 of 20. Selected durations (min / median / max): release barrier 70 / 70 / 81 ms after the signal and exit 10 / 20 / 20 ms after the barrier; failed release exits 5013 / 5015 / 5024 ms after the barrier (the 5 s revoke deadline) with the key gone 4141 / 4148 / 4351 ms after the test's keepalive stops. The SIGTERM-failover and SIGKILL-failover cases also passed once in the default build. The same five cases passed 20 of 20 on etcd v3.5.21. All loops and both full suites ran on the final code. Twenty local passes are evidence; they do not prove the bounds under arbitrary host stalls.
5. Docs: make html SPHINXOPTS=-W from docs/ succeeds; the new section renders with its two tables and appears in the page contents between "Promotion and Failover" and "Example: HA Deployment with etcd".

Test commands (reproduction commands against an already-running local etcd):

cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_UNIT_TESTS=ON -DBUILD_EXAMPLES=OFF -DBUILD_BENCHMARK=OFF \
  -DWITH_STORE=ON -DWITH_STORE_RUST=OFF -DUSE_ETCD=ON -DSTORE_USE_ETCD=ON
cmake --build build --target high_availability_test
./build/mooncake-store/tests/high_availability_test --etcd_endpoints=127.0.0.1:2379
cmake -S . -B build-fp -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_UNIT_TESTS=ON -DBUILD_EXAMPLES=OFF -DBUILD_BENCHMARK=OFF \
  -DWITH_STORE=ON -DWITH_STORE_RUST=OFF -DUSE_ETCD=ON -DSTORE_USE_ETCD=ON \
  -DMOONCAKE_ENABLE_TEST_FAILPOINTS=ON
cmake --build build-fp --target high_availability_test
./build-fp/mooncake-store/tests/high_availability_test --etcd_endpoints=127.0.0.1:2379
cmake --build build --target master_service_test
./build/mooncake-store/tests/master_service_test --gtest_filter='*HttpMetadataCleanup*'
(cd docs && make html SPHINXOPTS=-W)
git diff --name-only origin/main...HEAD | xargs pre-commit run --files

Test results:

• Unit tests pass: shutdown_request_test (15), master_service_config_test (19) and standby_controller_test (4 passed, 1 skipped) are unchanged and pass. master_service_test passes 73 in the fail-point build, including the two new cleanup-stop tests; the first default-build run had one unrelated timing failure in MasterServiceTest.CancelDrainJobRestoresSegmentStatus, which passed 10 of 10 single-test reruns and a full-suite rerun (73 passed).
• Integration tests pass (if applicable): high_availability_test against etcd v3.6.1 (client v3.5.34), default build 29 passed and 9 skipped, fail-point build 38 passed.
• Manual testing done (describe below): not done; no real-binary run is claimed.

Who ran each step and where: the implementation and every test execution above (unit, integration, the 20-run loops and the negative control) were performed by an AI coding assistant (see the AI Assistance Disclosure) in a local development environment. All runs were on one Linux x86_64 host (kernel 5.10, GCC 13) against single-member local etcd servers, v3.6.1 and v3.5.21, started with default flags on non-default ports. No result in this description comes from a test run by a human. All pre-commit hooks pass on the changed files, and code_format.sh --check --changed-lines passes.

Checklist

• I have performed a self-review of my own code
• C/C++ changes were formatted with git clang-format (clang-format 20.1.8); ./scripts/code_format.sh --check --changed-lines --base origin/main passed after committing. Transcript:

./scripts/code_format.sh --check --changed-lines --base origin/main (exit 0)

$ ./scripts/code_format.sh --check --changed-lines --base origin/main
[INFO] Mooncake Code Format Script
[INFO] Using clang-format version 20.1.8
[INFO] Formatting only C/C++ line ranges changed against origin/main

- [x] I have run pre-commit on the files changed in this PR and all hooks pass - [x] I have updated the documentation (if applicable) - [x] I have added tests to prove my changes are effective - [x] For changes >500 lines: I have filed an RFC issue (#4710; this PR is above 500 lines, 1,356 added)

AI Assistance Disclosure

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

AI tools assisted the design/review and implementation; the human submitter is responsible for reviewing every changed line and validating the reported results. Tools used: the implementation and all test executions were done by an AI coding assistant (Claude Sonnet 5.5); the design was written by an AI coding assistant (Claude Opus 5.5); the design and implementation reviews were done by AI assistants (Claude Opus 5.5 and OpenAI GPT-6 Astra).

[Icedcoco]

Thanks for putting this together. The motivation is clear, and the distinction between a confirmed release and an unconfirmed one is helpful. I also appreciate that the documentation calls out the Kubernetes limitation. I had a few questions while reviewing the shutdown path, in case I’m missing an existing safeguard:

1. Background work around lease release. stop_term_resources stops the RPC workers, leadership monitor, and OpLog writer, while the remote HTTP metadata cleanup worker appears to be stopped and joined only in MasterService’s destructor. Since the shutdown path ends with _exit, that destructor does not run. When remote metadata cleanup is enabled, could a queued or in-flight DELETE therefore complete after the lease is released? If the successor has already started serving and the same segment metadata has been recreated, I wonder whether the old cleanup could affect the new entry. This seems like a conditional timing risk rather than an inevitable outcome, but would it be worth stopping or fencing that worker before releasing the lease?

2. In-flight admin writes. SigtermReleasesAfterAdminDrain verifies that the admin server has stopped before release, but it does not hold an already admitted quota write across shutdown. The RFC called out that specific case. The server’s existing stop behavior may already provide the required guarantee; would a test that observes the write completing before release help make that ordering explicit?

3. Kubernetes release semantics. The deployment guide already notes that the Kubernetes wrapper reports cancellation being accepted, rather than confirming that the lease was actually released. Could we clarify whether that limitation is acceptable for this PR’s intended scope, or whether the “confirmed release” behavior should be described as applying specifically to etcd?

4. Etcd endpoint parsing in the tests. FirstEtcdHostPort() splits the endpoint list on commas, whereas the production parser also accepts semicolon-separated endpoints. Could a valid semicolon-separated configuration cause the new relay or TTL probe to parse the endpoint incorrectly? Reusing the production parsing behavior, or skipping configurations the helper cannot handle, might make these tests more portable.

I’m raising these as points to clarify the intended guarantees, not to suggest that each scenario necessarily causes a failure. The etcd release flow itself—stopping renewal polling before releasing the lease—looks reasonable to me.

[tensorcopy]

Thanks for the careful read and for the four questions. Answers below, with the changes now in the branch.

1. Background work around lease release. You are right that the remote HTTP metadata cleanup worker is not stopped today. It starts in setHttpMetadataRemoteUrl, and only ~MasterService stops and joins it, which the terminal _exit skips. With [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of #4710) #4745 the successor waits for the TTL, so the window was small but not closed: a server that accepted a DELETE can still apply it after its reply is lost. With this PR the release removes that delay, so the branch stops the worker before releasing the lease: the stop drops the queued segment names, waits for at most the one in-flight DELETE (the HTTP client's connect and request timeouts apply), and the worker skips the rest of its batch. A new test holds the first DELETE on a localhost server, queues further segments, starts the stop on another thread, checks that the stop has not returned while a DELETE is held, and then asserts that exactly three requests were sent, none for a skipped key or segment, and that a cleanup request after the stop is dropped. The co-located cleanup path is unchanged: it removes keys from the old process's own metadata server.

2. In-flight admin writes. The test proves the ordering (the admin server has stopped before the release) but does not hold an admitted quota write across shutdown, and the RFC's quota-write test is not in this PR. The completion guarantee rests on the admin server's existing stop draining its executor; the PR description discloses the gap and offers the follow-up, which needs a second fail point in HandleUpsertTenantQuota plus a multi-tenant child configuration. I can open that follow-up separately if you want it tracked.

3. Kubernetes release semantics. Agreed. The confirmed-release semantics and statuses 0 and 2 describe the etcd backend; the guide now says so next to the exit-status table. The Limits bullet keeps the Kubernetes wording: the wrapper reports only that the cancel request was accepted, and when no election monitor thread is running it does not wait for the election goroutine.

4. Etcd endpoint parsing in the tests. Fixed. FirstEtcdHostPort() now splits on both separators, trims whitespace and skips empty entries like the Go wrapper, and a small test covers the semicolon form. Configurations the helper cannot handle (TLS or other syntax) still fail with the existing message.

The changes are in 5a28cc5d, with the current head at f41900da.

[Icedcoco]

LGTM