[Store] Restore standby index tolerantly, discard ambiguous overlaps with durable repair

[he-yufeng]

Fixes #3760

What

Standby promotion no longer discards the whole metadata index because one entry fails validation. RestoreFromStandbySnapshot runs in three phases: cheap per-entry validation (each failure skips that entry with its key and reason logged and counted on the mooncake_ha_standby_restore_rejected_objects_total{reason} counter), overlap resolution, then construction for the survivors only. Only segment-level structural corruption (empty name/endpoint, zero capacity) stays fail-fast, since that is a malformed context rather than one bad object.

Overlap is treated as ambiguity, not recency. The promotion input comes from StandbyMetadataStore::Snapshot(), which enumerates unordered maps, so no replay order reaches the promotion context and neither side of an overlap can prove it is newer. Every descriptor in a transitively overlapping group is discarded, replicas independent of the conflict are kept, and an object is dropped only when no reliable replica remains. A snapshot whose objects all fail to land returns INVALID_PARAMS instead of reporting an empty-cluster success, with one carve-out: objects rejected because the live index already holds them are not losses.

Review-driven fixes on top of that:

• Ranges join pending_ranges only after the object is accepted, so a rejected object never leaves a range whose owner index a later accepted object would reuse.
• Segment accounting accumulates per object and commits only once the whole object validates, so a mid-construction rejection leaves no phantom bytes behind for later capacity checks or metrics.

Out of scope here, by design: the pre-promotion dry run and promotability predicate (#3774), ReplicaID preservation (I00 dependency in the #3808 roadmap, covered by #3811), and the partial-restore serving gate. Those belong to the N07/N08 promotion contract work.

Tests

master_service_test.cpp and master_service_ha_test.cpp pin the new semantics: per-descriptor overlap discard with independent-replica survival, transitive overlap groups, the stale-range-owner regression, accounting rollback on construction rejection, all-rejected restores returning INVALID_PARAMS, and mixed good+bad tolerance. Existing duplicate, remount, and tenant-scoping pins are unchanged.

Verification

Local build of this repo is not possible in this environment (no rdma-core, and upstream main fails to compile here with the same pre-existing header errors, so this is environmental, not the change). The overlap sweep and the accounting commit flow were exercised locally through a standalone harness running the same logic against the pinned scenarios (identical, contained, transitive, and adjacent ranges, dead-replica drop, capacity rollback). clang-format 20.1.8 and codespell are clean. CI runs the full Linux build and both suites.

AI assistance

Prepared with AI assistance (Kimi K3). The ambiguity-discard semantics come from the review discussion on this PR; the submitter reviewed every changed line and can defend the change end-to-end.

[he-yufeng]

All lanes are green now, including CTest at 25m. Getting here took four CI rounds and each one caught something real, so a short account for reviewers:

1. The first round was a transcription error on my side (UUID vs the ReplicaID the descriptor actually wants), plus a clang-format drift my local binary was too old to see.
2. The second round taught me the endpoint semantics I had wrong in the new tests: replicas restored onto an unmounted endpoint stay unreadable by design (invalid_replica_endpoints_ gates IsReplicaReadable), so ExistKey must see them as absent until the segment remounts. The tests now mount the endpoint they assert on.
3. The third round was the important one. Migrating the six HA tests to the tolerant contract exposed two genuine bugs in my implementation, not in the tests: I had routed grouped objects by group_id, but routing is decoupled from groups everywhere else, so the cross-group duplicate check looked in the wrong shard and would have restored the same key twice. And because restored buffers hold their DummyBufferAllocator by weak_ptr with the only strong ref in standby_allocator_keepalive_, a second successful restore swapped that map and silently expired every buffer the first restore had inserted (IsValid() false, objects vanish from ExistKey/GetReplicaList). Fixed by reusing the live keepalive allocator when the new snapshot attests the same endpoint; only endpoints the new snapshot drops lose their allocator, which is the honest signal.

Neither of those would have been caught without actually running the suite, and I could not run it locally: this machine has no rdma-core and the bundled fmt headers do not compile under Apple clang, which reproduces on a clean upstream checkout too, so it is the environment and not this diff. All test validation above is from CI, and I have tried to keep each fix small enough that the reasoning is checkable from the diff alone.

One pre-existing failure that is not this diff: MasterServiceSSDSnapshotTest.EvictObject failed on an earlier round and fails identically on the unrelated feat/tenant-eviction-watermark branch. That is the lease-expiry cleanup from #3761 wiping restored objects; the fix is in flight at #3771, which I reviewed.

The six migrated HA tests are the contract change reviewers should look at most carefully: bad entries are now skipped per object with a mooncake_ha_standby_restore_rejected_objects_total{reason} counter and the key plus reason logged, overlaps resolve to the later replay, and segment-level structural corruption stays fail-fast. The old all-or-nothing behavior only survives where it was already the right answer.

[he-yufeng]

Coordination note: #3811 (Icedcoco's PR-I00 ReplicaID preservation) touches the same RestoreFromStandbySnapshot and the same replica construction sites, so whichever lands second will need a real merge, not a rebase-and-go. The designs compose cleanly in one direction: this PR's per-entry tolerant restore can carry ID preservation per object too (an invalid or duplicate ReplicaID becomes a rejected entry with a reason label, next to the other per-entry rejections, rather than a wholesale INVALID_PARAMS). The one semantic decision for reviewers is whether duplicate or invalid ReplicaIDs should reject just that object (this PR's model) or the whole restore (#3811's current model). Happy to rebase and integrate the ID-preserving constructors here once the sequencing is clear, or to leave that merge to whoever lands second.

[he-yufeng]

One addendum to the SSD EvictObject note above: #3771 was the production-side fix I found first (lease-expiry cleanup wiping restored objects), and #3813 (just opened) offers a test-side explanation for the same failure: the eviction worker racing the original-vs-restored comparison in the shared snapshot fixture. Both account for the observed signature; which one the maintainers treat as the real root is theirs to call, and it does not change anything in this diff either way.

[Icedcoco]

I recommend deferring this PR rather than merging the current implementation.

The motivation is valid: one malformed standby descriptor should not discard the entire restored index. However, the current PR changes a central promotion path while the OpLog/Snapshot workflow is still experimental and not a committed production contract. There is no immediate production requirement to land this semantic change, so I would prefer to align it with the N07/N08 promotion and recovery work in roadmap #3808.

The main design issue is the latest replay wins policy. The promotion context does not carry an object-level or replica-level replay sequence/generation. Moreover, StandbyMetadataStore::Snapshot() enumerates an unordered_map, and the restore code sorts ranges by address rather than by replay order. Therefore the code cannot establish which descriptor is actually newer. Keeping one side of an overlap can preserve stale or otherwise unreliable state.

For the initial recovery contract, an overlap must be treated as ambiguity: discard every descriptor participating in the overlapping range (including transitive overlap groups), and keep only replicas that are independent of the conflict. If an object has no reliable replica left, discard that object. Do not discard unrelated objects merely because another object conflicts.

There are also correctness issues that should be resolved independently of the policy:

• pending_ranges records accepted.size() before the object is appended to accepted. If a later descriptor rejects the object, the range owner becomes invalid and can either index past the vector or refer to a different object.
• Memory accounting is updated while an object is still being constructed. If a later descriptor rejects the object, the previously accumulated bytes remain in restored_accounted_memory_bytes, which can make valid later objects fail capacity checks and can corrupt the memory metrics.
• An overlap currently nulls the whole AcceptedEntry, so an object with one conflicting replica also loses its healthy replicas.
• The restore function returns success even when all objects are rejected. The supervisor treats that as a successful restore and proceeds to serving. This weakens the fail-closed behavior established by [Store] Gate HA serving on successful standby restore #3497 and conflicts with the [RFC]: Master rolling upgrade has no index-preserving path today, and no readiness signal predicts a safe promotion #3774 guidance and the [RoadMap]: Mooncake Store HA and OpLog Roadmap #3808 invariant that malformed state must not become an empty-cluster success.

This PR also does not implement the pre-promotion dry run or promotability predicate discussed in #3774, and the restore path still does not preserve ReplicaID, which is listed as the I00/N07 dependency in #3808. Those are not new regressions in this diff, but they mean this PR should not be considered a complete promotion-safety fix.

Given the current development status and the absence of an urgent production hotfix requirement, I recommend keeping #3760 open as a tracked restore validation issue and carrying this work into the N07/N08 design. A follow-up should define:

1. deterministic conflict handling that never guesses which overlapping entry is newer;
2. explicit partial-restore semantics and a serving gate;
3. stable object/replica identity and ordering metadata;
4. rollback-safe accounting and per-replica filtering; and
5. focused multi-replica tests plus a real-etcd promotion/recovery test.

The CI results are useful and currently green, but they do not establish these recovery semantics. I would not merge #3806 in its current form.

[he-yufeng]

Thanks for the close read. I checked each point against the code before answering, and you are right on the substance. Reworked in ae0d633.

On the policy, agreed, and stronger than you put it: the objects vector comes from StandbyMetadataStore::Snapshot(), which enumerates unordered maps, so the promotion context has no replay order at all. "Latest replay wins" was not implementable with the data on hand; sorting by address only made it look ordered. Overlap is now treated as ambiguity: every descriptor in a transitively overlapping group is discarded, replicas independent of the conflict are kept, and an object is dropped only when no reliable replica remains.

The three correctness bugs were all real:

• pending_ranges recorded accepted.size() during the replica loop, so an object rejected mid-loop left ranges whose owner index the next accepted object would reuse. Ranges are now collected per object and merged only after acceptance.
• Accounting accumulated into the segment totals during construction, so a capacity rejection left phantom bytes behind. Bytes now accumulate per object and commit only once the whole object validates.
• Whole-entry nulling on overlap is gone with the per-descriptor discard above.

On the return contract: a snapshot whose objects all fail to land now returns INVALID_PARAMS instead of an empty-cluster success. The one deliberate carve-out is the all-duplicates case, where the live index already holds the state. The partial-restore serving gate stays with N07/N08 per #3808, and this PR does not try to own it. No dry run, no promotability predicate, no ReplicaID preservation here either; those remain with #3774/#3808 and #3811, and this PR is just the restore-tolerance stop-gap with honest conflict semantics.

Tests now pin the new behavior in both suites: per-descriptor discard with independent-replica survival, transitive groups, the stale-range-owner regression, accounting rollback, all-rejected failure, and mixed good+bad tolerance. One note on evidence: my environment cannot build the repo (no rdma-core, and upstream main fails here with the same pre-existing header errors), so the execution evidence is CI plus a standalone harness that runs the sweep and accounting logic through the pinned scenarios. I will watch the Linux run and report back if anything moves.

[he-yufeng]

CI on ae0d633 is fully green: 21 lanes pass including CTest (39m26s), which runs both restored suites with the new ambiguity-discard pins. Nothing moved.

[Icedcoco]

Thanks for the rework. I agree with the ambiguity-discard policy for overlapping memory descriptors.

I also want to clarify one point from my earlier review: I do not think a partial restore must always remain non-serving. For a cache, serving a smaller but internally correct index can be a valid availability tradeoff. Losing some cache entries is acceptable here. Returning a descriptor that refers to the wrong bytes is not.

The remaining question is therefore not simply whether partial restore may serve. It is whether the discard decision becomes durable state, or remains a local decision made only by this promotion attempt.

At the moment, RestoreFromStandbySnapshot filters conflicting descriptors only in the newly constructed Master. I do not see a corresponding durable REMOVE or canonical replacement record in the OpLog. That creates a convergence problem:

1. a standby contains two conflicting historical descriptors;
2. promotion filters them locally and starts serving a smaller index;
3. the original standby metadata or another standby can still retain the old descriptors, because no later OpLog entry supersedes them;
4. after a later leadership change, a new promotion can replay/load the old descriptors again;
5. meanwhile the current primary may have reused the discarded address for a different key.

At that point an old key can be restored with a descriptor pointing at memory that now belongs to a different key. This is the failure mode I want to rule out. Data loss is acceptable; resurrection of discarded state and incorrect reads are not.

I think conflict handling needs to be defined as a durable repair operation, not only as a restore-time filter. A preliminary contract could be:

• For each object, derive one canonical post-repair state from validated, COMPLETE descriptors only.
• If no reliable descriptor survives, append one key-level REMOVE.
• If independent descriptors survive, append one canonical PUT_END whose payload contains only those survivors. The current REMOVE is key-level, so it cannot represent removal of only one replica.
• Every repair record must be ordered after the promoted standby's final catch-up cursor and before any newly admitted client mutation.
• Queue admission is not enough. The repair batch must become durable in etcd and advance the durable prefix before RPC serving is exposed.
• If repair persistence fails, times out, or leadership/fencing is lost while waiting, the candidate must not serve.
• After repair is durable, future standbys replay the same REMOVE or canonical PUT_END, so the discarded descriptor cannot return on another promotion.

The intended ordering would be:

1. final catch-up and freeze/export the promotion context;
2. acquire and verify the fenced writer for the elected view;
3. derive the canonical metadata and repair plan while mutation admission and RPC serving are still closed;
4. append all repair records through the ordered writer;
5. wait until the repair records are durable, not merely queued;
6. install the canonical local metadata and register the RPC service;
7. allow remount and later allocations only from the canonical descriptor set.

This also means that discarded memory descriptors should not be imported into the remounted allocator. Their removal is a logical metadata repair, not a direct remote-memory free RPC; after the repair is durable, allocator recovery must treat only canonical live descriptors as occupied.

I would avoid serializing a synthetic “conflict placeholder” descriptor. A placeholder can accidentally re-enter later snapshots or allocation recovery. The conflict evidence can remain in logs/metrics and in a temporary repair plan; the durable result should be only REMOVE or canonical PUT_END.

Also, PROCESSING and INITIALIZED replicas cannot simply be treated as absent. They are not readable survivors, but an in-progress write may still finish after leadership changes. They need a separate pending-reservation state: not served, not eligible for conflicting allocation, and retained until a defined completion path or timeout produces a durable PUT_END, FAILED, or REMOVE. This requires explicit callback and identity semantics, especially alongside ReplicaID preservation, rather than being folded into the normal overlap discard path.

The current PR already improves the local validation behavior substantially: it removes the unsupported “latest replay wins” assumption, discards transitive overlap groups, preserves independent replicas, and fixes the stale range-owner and accounting issues. Those changes are directionally correct.

However, the durable repair protocol crosses the promotion supervisor, writer fencing, OpLog ordering, callback identity, remount allocation recovery, and failure handling. This is the reason I previously recommended deferring the feature: the local restore algorithm is only one part of the correctness contract.

Since the OpLog/Snapshot path is still experimental and there is no urgent production requirement, I would prefer that we agree on this repair and pending-write contract first, then implement it with focused real-etcd tests:

• conflict -> durable repair -> later promotion cannot resurrect the conflict;
• discarded address reused by a new object -> later promotion cannot expose the old key;
• repair queue accepted but not durable -> candidate does not serve;
• leadership loss during repair -> candidate does not serve;
• long-running PROCESSING write -> no premature address reuse, then defined durable completion or timeout cleanup.

I am happy to continue the design discussion, but I do not think the current restore-only change should merge before this convergence behavior is specified.

[catyans]

issue: restoring (name=N, endpoint=E2) after (N, E1) reuses E1's allocator through the by_name fallback. The new replica then reports E1, while the unreadable set contains E2, so it may appear readable before remount. Please reuse only an exact endpoint match and add an endpoint-change regression test.

[he-yufeng]

@catyans Confirmed against the code, your scenario is real. Replica::get_descriptor() for memory replicas goes through AllocatedBuffer::get_descriptor(), which takes the endpoint from the allocator, not from the snapshot descriptor. So a restore of (name=N, endpoint=E2) that hits the by_name alias ends up with a buffer whose allocator reports E1: IsReplicaReadable checks E1 against an unreadable set that tracks E2 and N, and the replica is servable through GetReplicaList with the stale endpoint before E2 ever remounts. The E2 remount's allocator swap also cannot find buffers holding E1's allocator, so the mismatch persists past remount.

Fixed in 3444ea0: reuse is now exact-endpoint only, anything else gets a fresh DummyBufferAllocator for the attested endpoint. Added RestoreWithChangedEndpointGetsFreshAllocator covering your case: same name rebinds to a new endpoint across restores, and the test pins that the second replica reports the new endpoint, stays unreadable until that endpoint remounts, and turns readable after.

One caveat on verification: the store targets need Linux-only headers (linux/memfd.h, SOCK_CLOEXEC in uds_transport), so there is no local build on this Mac. I ran clang-format 20.1.8 on the touched files and I am relying on the CTest lane for the new test.

[Aionw]

The restore function right now is too hudge and mix up multiple functions, I'd suggest to extract this into more small and single-purpose pieces.

[catyans]

Thanks, the exact-endpoint reuse and regression address my allocator concern.

I am not approving yet because @Icedcoco's durable-repair concern remains unanswered: restore-local filtering may let a later promotion replay discarded descriptors. Please resolve that convergence contract before merge.

[he-yufeng]

@Icedcoco The convergence gap is real in the current patch, and your framing is the right one: the discard decision is local to this promotion attempt, nothing supersedes the old descriptors in the log, and a later promotion can replay them while the primary may already have reused the address. Restore-local filtering buys availability and correctness of the serving index, but it cannot stand in for the durable repair.

Mapping your contract onto what the tree already has, so we specify against real primitives rather than in the abstract:

• The ordered writer (OrderedOpLogWriter, started per term by InitializeBatchOpLogWriter against the HA KV backend) already gives us fenced, ordered appends with a durability signal: Reserve() + Commit(..., DurableFinalizeCallback), and the write path advances the durable prefix atomically (WriteBatchAndAdvancePrefix). Writer terminal states (kFenced, kRetryTimeout, kNonRetryableWriteError) are exactly the fencing/loss signals the candidate must honor.
• OpType::REMOVE is key-level, and PUT_END payloads go through SerializeMetadataPayload, which carries the replica list. A canonical PUT_END whose replicas contain only the validated survivors represents replica-level repair without needing a placeholder descriptor.
• The supervisor's serving order today already makes restore the gate: RestoreFromStandby runs before RegisterRpcService, which runs before the RenewLeadership serve preflight. The repair batch slots between restore and registration with no reordering of what exists.

The contract as I would write it:

1. During promotion, after the final catch-up, the candidate derives the canonical post-repair state per object from validated COMPLETE descriptors only. Zero survivors appends one key-level REMOVE; survivors append one canonical PUT_END carrying exactly those replicas. No synthetic conflict placeholders anywhere; evidence stays in logs, metrics, and the in-memory repair plan.
2. The repair batch commits through the fenced writer with durable finalize. Serving is blocked until the durable prefix covers the repair batch, not merely until the batch was accepted.
3. If the write fails, times out, or the writer goes terminal (fenced or leadership lost) while waiting, the candidate takes the same path a restore failure takes today: back to standby, no registration, no serving.
4. PROCESSING/INITIALIZED replicas do not count as absent for repair purposes. They are not served, not eligible for allocation overlap, and they wait for a defined completion (PUT_END/FAILED/REMOVE via callback identity) or timeout. I agree this needs ReplicaID-stable identity to name the callback target, so it lands with the N07/I00 work rather than being faked here.
5. Remount/allocator recovery imports occupancy only from canonical live descriptors. Discarded memory descriptors are never imported; their cleanup is logical metadata repair, not a remote free.

On split: this PR's local side (ambiguity discard, independent-replica preservation, the stale range-owner fix, commit-time accounting) is the restore half of that contract and stays. I will implement the durable repair half next, in this PR if you want the whole thing gated together, or as the immediate stacked follow-up so this one stops growing. Either way the merge waits until the repair path exists with the real-etcd tests you listed: no resurrection on later promotion, no old-key exposure after address reuse, accepted-not-durable means no serve, leadership loss during repair means no serve, and pending writes never allow premature reuse.

One place I want your read before I build it: whether the repair batch should also carry a marker the standby store understands as "these descriptors were superseded by repair", so a standby that never saw this promotion can still drop them on snapshot, or whether canonical PUT_END/REMOVE ordering alone is enough given the durable-prefix fence. I lean toward the latter (the prefix fence already orders it), but you know the standby snapshot path better than I do.

[catyans]

Thanks @he-yufeng. PUT_END replaces per-key metadata and the reader advances only after the durable batch, so a separate marker seems unnecessary if an older standby must replay that batch or load a newer snapshot. The later-promotion/address-reuse test should pin this. @Icedcoco, WDYT? I would still wait for the durable repair and tests before merge.

[he-yufeng]

@Icedcoco @catyans The durable repair half is in 2426e80, following the contract as agreed (no marker record, catyans' read on ordering):

• The discard phase now collects repair intents: a conflict that leaves no reliable replica becomes a key-level REMOVE; one that leaves survivors becomes a canonical PUT_END serialized from the installed metadata, so the payload carries exactly the surviving descriptors. No placeholder descriptors anywhere.
• After the index installs and before the restore reports success, the batch goes through the fenced writer with durable finalize, and the restore blocks until every record is durable (10s bound). Any append failure, timeout, or missing install (a survivor that failed construction later, e.g. capacity) fails the restore outright: the candidate drops back to standby without registering or serving. A survivor that failed construction is repudiated with REMOVE, since absence is its canonical state.
• With no OpLog configured the local filter stands alone, since nothing replays into the gap.

What I could not do here: the store targets need Linux-only headers, so there is no local build on this Mac; the CTest lane is the verifier, same as the earlier rounds. The real-etcd promotion/recovery tests from your list (no resurrection on later promotion, no old-key exposure after address reuse, accepted-not-durable means no serve, leadership loss during repair means no serve) I have not added yet: they need the etcd harness, and I would rather build them against your read of this shape first. PROCESSING/INITIALIZED still fold into the normal path; per our discussion that pending-reservation state belongs with the ReplicaID work (N07/I00), not faked here.

[he-yufeng]

Rebased onto the ReplicaID-preserving main (#3811). Two things worth noting for reviewers:

• The restored replica construction now carries desc.id on every branch, so the durable-repair PUT_END payload serializes with stable replica ids as well.
• The new duplicate-id validation interacted with this PR's conflict fixtures, which reused the default id=1 across replicas within one object. Fixed the fixtures to distinct ids in 4ee658b, and adapted FailedRestoreDoesNotAdvanceReplicaIdCounter: under the ambiguity-discard semantics the only whole-restore failure is the nothing-lands case, so the test now drives both entries onto unknown endpoints and asserts the id counter does not move. The tolerant-skip behavior on a single bad entry stays pinned by RestoreSkipsBadObjectAndKeepsExistingState.

CTest on 4ee658b is the verifier as before.

[he-yufeng]

@Aionw addressed on the new head. The tolerant machinery is no longer one block: it now sits in three single-purpose methods off RestoreFromStandbyState — ValidateLegacyStandbyEntries (cheap per-entry validation plus the ambiguity-discard overlap resolution), ConstructLegacyStandbyReplicas (survivor-only replica construction with per-object accounting), and InstallLegacyStandbyObjects (the all-rejected stop-gap, the existence double-check, and the shard installs). Shared state travels in a small LegacyRestoreContext, and the bounded handoff path is untouched main code.

Both suites re-run against this head in a Linux container: master_service_test 71/71, master_service_ha_test 101/101.

[he-yufeng]

@Aionw A week on from addressing your review, re-verified: the branch is still MERGEABLE against current main (a8106d4), and the standby-restore area was not touched by the recent #3723/#3984 landings. The tolerant restore machinery now sits in three pieces as discussed (standby index restore, ambiguous-overlap discard, durable repair), with the HA and master-service tests covering the new paths. Anything else you want changed before this moves forward?

[Icedcoco]

Thanks for the thorough rework. The changes address the earlier review points well, especially per-object filtering, transitive overlap handling, range cleanup, accounting rollback, and the fail-closed result for fully rejected snapshots.

I still have a few concerns that may be worth discussing before merging:

1. Durability of conflict resolution
   The overlap decision currently applies only to the MasterService instance created by this promotion. I do not see a corresponding durable REMOVE or canonical replacement record in the OpLog/etcd. Could a later promotion from the original or another standby replay the discarded descriptors again, especially after the address has been reused for another key? It may be useful to define how this repair becomes durable, or explicitly track it as a prerequisite for merging.

2. End-to-end promotion coverage
   The new tests exercise RestoreFromStandbySnapshot directly, which is valuable, but they do not appear to cover snapshot creation, OpLog catch-up, persistence, a second promotion, and address reuse together. Would an integration test for that sequence be feasible, or should this remain a documented follow-up owned by the N07/N08 work?

3. ReplicaID dependency
   Since replica construction is modified here and ReplicaID preservation is deferred to [Store] Preserve ReplicaID during standby restore #3811, could we clarify whether [Store] Preserve ReplicaID during standby restore #3811 must land first, or whether this PR is intentionally safe to merge independently with that limitation?

4. Terminal replica states
   One additional edge case may deserve an explicit test: an object whose replicas are all FAILED or REMOVED without any overlap. Should that object be omitted entirely, or retained as metadata with no readable replica? Making this behavior explicit would help prevent ambiguity.

I do not intend these as objections to the improvements already made. The local restore behavior is substantially clearer now; I mainly want to confirm that the durability and repeated-promotion semantics are either covered here or clearly assigned to the follow-up work.

[he-yufeng]

@Icedcoco Thanks for the four points — two of them were real gaps and are now closed in 4b79c8a. Taking them in order.

1. Durability of conflict resolution. The overlap-derived discards are already durable; that was the "durable repair" half of the change and it is easy to miss in the diff. For every conflict-affected key, the restore writes a fenced OpLog record before completing: REMOVE for a fully dropped object, PUT_END carrying only the surviving descriptors for a partial one (master_service.cpp, the block right after RebuildTenantQuotaUsageFromMetadata). If the batch does not go durable within 10s the whole restore fails, so a promoted candidate never serves an index whose discards could resurrect on a later promotion, address reuse included. With no OpLog configured there is nothing to replay into, so the local filter stands alone — that case is memory-only by construction.

The remaining hole you sensed is real but one category over: non-conflict rejections (unknown endpoint, invalid descriptor, capacity overflow) are log-and-metric only, so a replayed snapshot re-rejects them every round. That is safe but noisy, and I deliberately did not tombstone them here — capacity_overflow reflects this promotion's capacity view, not the entry's validity, and a durable REMOVE could suppress an entry a roomier later promotion should accept. Tracked as a scoped follow-up in #4120 with the per-reason allowlist shape.

2. End-to-end coverage. Fair. This round adds an OpLog-enabled test that proves the repair channel end to end at the writer boundary: a partial conflict now produces a durable REMOVE for the dropped key and a canonical PUT_END for the survivor, read back from the batch storage; and a rejecting writer makes the whole restore fail closed. The full chain you describe (snapshot creation, OpLog catch-up, a second promotion, address reuse) belongs to the N07/N08 integration scope, and I would rather keep it there than bolt half of that harness onto this PR.

3. ReplicaID dependency. No ordering left: #3811 is already on main and is in this branch's ancestry, and the restore-only Replica constructor it added is what every construction path here uses. This PR is safe to merge independently on current main.

4. Terminal replica states. Now explicit. An object whose replicas all arrive REMOVED or FAILED (no overlap involved) is dropped as no_reliable_replica and flows through the same durable REMOVE as overlap casualties, instead of lingering as metadata with zero readable replicas. Covered by a new test with one REMOVED object, one FAILED object, and a clean survivor.

Verification: master_service_ha_test 104/104 green including the three new tests, master_service_test 71/71 green, and the terminal-only test was red before the master_service.cpp change and green after it.

[he-yufeng]

Rebased onto current main (6cc1a10) and fixed one real interaction the rebase surfaced: the client-liveness work that landed in #2991 makes a restored local-disk replica readable only while its owner holds a liveness record, and the bounded promotion path wires that through record_for_known_owner. My factored-out legacy constructor was still building the replica without it, so a restored local-disk object landed in the index but unreadable (ExistKey false immediately after a green restore). CI caught it as StandbySnapshotRestorePreservesTenantScopedKeys; on the old base this combination could not happen, which is why the earlier local runs were green.

ae25fa2 threads the staged owner records through the legacy context and shares one RecordRestoreKnownOwner helper between the bounded and legacy paths. The CI format failure is folded in as well (the repo's clang-format-20 gate realigns the whole file once it is touched).

Re-verified locally on the rebased head: master_service_ha_test 109/109 and master_service_test 64/64 pass, including the tenant-scoped restore test that failed in CI.

[he-yufeng]

CI update: the CTest failure on the merged snapshot was MasterServiceEvictScenarioTest.OpLogDurabilityGatesTenantQuotaReclamation (TENANT_QUOTA_EXCEEDED still set after the 4s wait margin). Rerun passes, and the signature matches the known 4s-margin flake family tracked in #4136: two other branches hit the sibling MasterServiceSSDSnapshotTest.EvictObject at 4011/4012 ms today (runs 34980652857 and 34979380422), this one at 4001 ms. All checks are green now; nothing in this PR's diff needed changing.

[he-yufeng]

CI note on the CTest unit tests (Python 3.12) red: the only failure is FilereadWorkerPoolTest.AcceptsTypedTrailingWhitespaceAndCaches, failing at fileread_worker_pool_test.cpp:76 on WaitForProcessThreadCount(baseline) — the first teardown wait, i.e. the worker threads had not fully exited inside the wait window on that runner. Attribution:

• The diff touches only master_service.{h,cpp} and the two master-service test files; the fileread worker pool is not on any path this PR changes.
• The same binary passes on main's latest Build & Test (Linux) (run 35241301803), and the test was just restructured by [Store] Structure FilereadWorkerPoolConfig environment settings #4166, so the wait is freshly tuned and inherently timing-sensitive on a loaded runner.
• This reads as the same CI-flake family as [CI] MasterServiceSSDSnapshotTest.EvictObject is flaky around a 4s margin #4136 (timing-margin tests under load), different test.

Rebase in the previous push brought the tree current; the two test binaries this PR actually affects (master_service_test, master_service_ha_test) pass locally end to end (60/60 and 109/109 in the Linux container build). Happy to rebase once more if the lane wants a clean rerun signal.