[pjdurden]

Issue #3826 — NoF replicas restored from a standby snapshot can alias reallocated offsets

Revised after review. The first revision invalidated the restored handle; this one quarantines the endpoint instead. Discussion in the thread below.

1. Root cause

MasterService::RestoreFromStandbySnapshot() rebuilds in-memory Replica objects from the standby snapshot. For a NoF (NVMe-oF SSD) descriptor it created a DummyBufferAllocator and kept it alive in standby_allocator_keepalive_, so the replica's weak_ptr never expired and it looked like a live, COMPLETE, readable replica everywhere.

But DummyBufferAllocator holds no allocation state, and the snapshot carries no NoF segment registry: MountNoFSegment emits no SEGMENT_MOUNT oplog entry, and StandbySegmentInfo has no base-offset field. So when the namespace is later mounted, ScopedNoFSegmentAccess::MountSegment() builds a fresh, empty allocator over the whole [base, base+size) (segment.cpp, via CreateBufferAllocator). It considers the offsets held by restored replicas free and can hand them to a different (tenant_id, user_key). Nothing validates that two keys map to the same physical range, so the old descriptor keeps reporting COMPLETE while the bytes underneath belong to another object: a silent wrong-data read.

Memory replicas already have both halves of the protection this needs. ReMountSegment() collects the restored descriptors for the remounted endpoint, calls BuildRegionLiveAllocations() + ImportOffsetBufferAllocator() / ImportCachelibBufferAllocator(), and swaps in an allocator that already owns those ranges via ReplaceAllocators. Until that import happens, restored replicas whose segment is absent are recorded in invalid_replica_endpoints_ so they cannot be read. The NoF path had neither.

2. The fix

Quarantine the NoF endpoint, mirroring the memory path.

• Keep the descriptor. The placeholder allocator stays alive, in a NoF-specific keepalive map. get_descriptor() therefore still returns the complete descriptor including transport_endpoint_ (which it reads off the allocator), and has_invalid_nof_handle() stays false, so BuildStaleHandleCleanupPlan() no longer classifies the replica as stale and ClearInvalidHandles() leaves it alone. The state a future import needs survives.
• Hide it from reads. The endpoint goes into invalid_nof_replica_endpoints_, which IsReplicaReadable() consults, so the replica is excluded from GetReplicaList, GetReplicaListForAdmin, BatchGetReplicaList and the eviction paths.
• Block fresh allocation. MountNoFSegment() and ReMountNoFSegment() refuse a namespace whose te_endpoint or name is quarantined, returning UNAVAILABLE_IN_CURRENT_STATUS. Nothing can alias the offset while the descriptor survives.

The NoF keepalive and quarantine sets are deliberately separate from standby_allocator_keepalive_ and invalid_replica_endpoints_. ReMountSegment() erases both of those by endpoint when a memory segment comes back, so a NoF te_endpoint that collides with a memory segment alias would otherwise be un-quarantined, and its placeholder dropped, by an unrelated memory remount. NoF replicas are still checked against invalid_replica_endpoints_ as before, so this is strictly additive on the read path.

Why not simply invalidate the handle, as the first revision did? It closes the read window, but ClearInvalidHandles() then permanently removes the replica and get_descriptor() loses the endpoint once the allocator expires, so the breadcrumbs a later remount needs are gone. Reaching "restored NoF data is disposable" through an expired weak pointer also decides a #3796 contract question by side effect rather than explicitly.

Scope: this is the fail-closed half

Making the data readable again is the second half and is not in this PR. It needs a NoF analog of the memory import path, so that a mount can adopt the restored (offset, size) ranges, swap in an allocator that owns them, replace the buffers and lift the quarantine. Neither replace_nof_buffer nor a NoF ReplaceAllocators exists in the tree today. Happy to fold it in here instead if you would rather see them together.

Note this corrects a claim in the first revision of this description. I argued that rebuilding the allocator was not implementable because the snapshot carries no NoF base or capacity. That is true at restore time, but the gate belongs at mount time, where the real NoFSegment with base, size and te_endpoint is in hand. The snapshot gap does not block it.

3. Files changed

• mooncake-store/include/master_service.h — invalid_nof_replica_endpoints_, standby_nof_allocator_keepalive_, IsNoFSegmentQuarantined().
• mooncake-store/src/master_service.cpp — quarantine on restore; IsReplicaReadable() honours it; MountNoFSegment() / ReMountNoFSegment() fail closed; restore summary logs the quarantine count.
• mooncake-store/tests/ha/master_service_ha_test.cpp — RestoreQuarantinesNoFReplicas (descriptor intact, endpoint preserved, not served, not reaped), QuarantinedNoFEndpointCannotBeMounted (USE_NOF only), and the updated RestoreFromStandbyPreservesNoFBufferDescriptor tail.

4. Risk and open questions

• Availability cost, and the main thing worth your call. While a NoF endpoint is quarantined, that whole namespace cannot be mounted at all, not just the restored ranges. After a promotion that had NoF replicas, NoF is unusable until the import path lands. That is what fail-closed means here, and it is bounded by the follow-up, but it is a real regression in availability traded for correctness. If you would rather have a narrower gate, or an operator escape hatch, say so and I will add it rather than guess.
• Nothing lifts the quarantine yet. Only a subsequent restore reassigns the set. Objects on a permanently gone endpoint stay in the metadata index, since nothing reaps a quarantined replica any more. Restored memory replicas behave the same way today, so I matched them rather than inventing a NoF-specific TTL, but if you want quarantine to expire that is a decision for you.
• [TENT] Add native UB endpoint bootstrap and READ/WRITE data path #3796 contract. RestoreFromStandbyPreservesNoFBufferDescriptor keeps its descriptor assertions, which quarantine satisfies including transport_endpoint_. Only its tail, which asserted GetReplicaList serves the replica, is updated, because serving it is the aliasing window. That is the narrow part of the contract I believe has to change; if the project reads [TENT] Add native UB endpoint bootstrap and READ/WRITE data path #3796 as promising readability, this needs a different answer.
• CI coverage. The restore side is compiled unconditionally and covered by master_service_ha_test. The mount gate is inside USE_NOF, and CI builds with USE_NOF=OFF, so QuarantinedNoFEndpointCannotBeMounted will not run there.
• Lock order. Both mount gates take snapshot_mutex_ (3) before segment_mutex_ (6), matching memory MountSegment(). Both are shared locks and the only callers are the RPC entry points, so there is no nesting.

5. How I verified it

• Traced the consumers this depends on: IsReplicaReadable() and the VisitReplicas filters, and BuildStaleHandleCleanupPlan() / ClearStaleHandles(), confirming the stale predicate keys only on has_invalid_mem_handle() || has_invalid_nof_handle() || has_stale_local_disk_client() and never consults the endpoint sets, which is what makes a quarantined replica survive the sweep.
• Confirmed get_descriptor() reads transport_endpoint_ off the allocator, which is why the keepalive is what preserves it.
• Confirmed MountSegment() (memory) establishes the snapshot_mutex_ before segment-access order the new gates follow.
• clang-format --style=file clean on all three files, with a pre-existing unrelated diff in the test file left untouched.
• Not compiled or run. This checkout has none of the C++ dependencies (no glog, gflags, boost, yalantinglibs, cachelib), so scripts/run_ci_test.sh cannot run here. master_service_ha_test needs to be built and run before this merges, and I would treat CI as the gate rather than my reading.

[Copilot]

🟡 Changes recommended

There is a small but concrete performance/implementation issue in the new NoF-restore path (unnecessary per-replica DummyBufferAllocator allocation) that should be adjusted before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a HA-restore correctness bug where NoF (NVMe-oF) replicas restored from a standby snapshot could remain “readable” despite lacking allocator state, potentially aliasing later reallocated offsets and causing silent wrong-data reads. The change forces restored NoF replicas to be treated as invalid immediately, so existing “invalid handle” filtering and cleanup logic prevents serving stale descriptors and reclaims them.

Changes:

• MasterService::RestoreFromStandbySnapshot() no longer keeps a placeholder allocator alive for restored NoF replicas, making them immediately report has_invalid_nof_handle().
• Adds an HA regression test ensuring restored NoF-only objects are not served and are reclaimed by ClearInvalidHandles().
• Introduces small test helpers to construct standby NoF replica/object metadata and to assert invalid-NoF-handle state.

File summaries

File	Description
mooncake-store/src/master_service.cpp	Restored NoF replicas are invalidated by not retaining a keepalive allocator, preventing stale replicas from being served post-restore.
mooncake-store/tests/ha/master_service_ha_test.cpp	Adds a focused regression test and helpers to validate NoF restore invalidation and subsequent cleanup.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pjdurden]

ci caught a real conflict here, so flagging it rather than papering over it.

the new test RestoreDoesNotKeepNoFReplicasUsable passes, but this change breaks RestoreFromStandbyPreservesNoFBufferDescriptor, which #3796 added to main earlier today. master_service_ha_test comes out 93 passed, 1 failed, and that is the one.

the two assertions that fail:

• master_service_ha_test.cpp:2065, restored.transport_endpoint_ is "" where it expects standby_restore_nof_endpoint. AllocatedBuffer::get_descriptor() reads the endpoint off allocator_.lock(), so once the DummyBufferAllocator leaves scope the endpoint is gone. size_, buffer_address_ and protocol_ survive because they come off the buffer itself.
• master_service_ha_test.cpp:2069, GetReplicaList returns replica_not_ready, so has_value() is false.

so my comment in master_service.cpp overclaims: the replica does not keep its whole descriptor, it loses the endpoint. that wording needs fixing whichever way this goes.

the bigger problem is that the two tests encode opposite answers to question 1 in #3826, whether NoF replicas are expected to remain readable after HA promotion. #3796 says yes, this pr says no. that is a design call and not one i should make by overwriting a test that landed the same day, so i would rather have your read first.

as i see it the issue leaves three options:

1. restored NoF replicas stay readable and this pr is simply wrong, in which case the offset reuse in [Bug]: [NoF] Remount after HA recovery may reallocate offsets referenced by restored replicas #3826 has to be prevented somewhere else.
2. remount rebuilds the real allocator from the restored descriptors, so replicas stay readable and those offsets are never handed out again. that satisfies both tests, but it is a much larger change to the remount path and i would want a steer before attempting it.
3. restored replicas stay unavailable, which is what this pr does, and RestoreFromStandbyPreservesNoFBufferDescriptor gets updated to match.

happy to take any of the three, just say which. i lean 2 as the right end state if NoF HA recovery is meant to be supported at all. the review note about the per-replica DummyBufferAllocator only matters under option 3, so i will hold that until the direction is settled.

separately, build-wheel / build (3.10) is unrelated to this change. pip install torch==2.13.0 died mid-download with ProtocolError: Connection broken: IncompleteRead(433863424 bytes read, 409831283 more expected) and SetupPyTorchEnv.cmake failed the build off the back of it. that one just needs a re-run.

[catyans]

Thanks for calling this out so clearly. Catching the conflict with a test that had just landed — and pausing instead of quietly rewriting it — is exactly the kind of care we want around HA recovery.

Of the three options, I also lean toward option 2 as the end state, with the fail-closed step first as @he-yufeng suggested in #3826. There is just one part of the current patch that makes me hesitate: the temporary allocator does close the immediate read window, but ClearInvalidHandles() can then permanently remove the replica, and get_descriptor() loses the endpoint as soon as that allocator expires. We may be throwing away the breadcrumbs that a later remount needs to rebuild the real allocator.

Would it be possible to model this as an explicit restored-NoF quarantine instead — keep the complete descriptor, hide it from reads, and prevent fresh allocation on that endpoint until remount has imported all restored (offset, size) ranges? That still closes the corruption window now, while leaving us a clean path to make the data usable again.

@he-yufeng, is that roughly the two-step split you had in mind? And if the project would rather declare restored NoF data disposable after promotion, that's a valid decision too — I would just prefer that we make it explicit and update the #3796 contract, rather than getting there indirectly through an expired weak pointer. Happy to follow the maintainers' call here.

[he-yufeng]

The approach checks out against what I traced in #3826. Keeping the DummyBufferAllocator scoped to the loop iteration lets the replica's weak handle expire, so the existing invalid-handle machinery does the rest: IsReplicaReadable excludes it, GetReplicaList answers REPLICA_IS_NOT_READY, and ClearInvalidHandles reaps it on the regular sweep. No new state, no new path. That is the step-1 shape from the issue.

The red CI is one test, not a build break. In the CTest lane (job 100702250265) the suite runs green except:

[  FAILED  ] MasterServiceHATest.RestoreFromStandbyPreservesNoFBufferDescriptor

That test has two halves. The descriptor half still passes under your change, because the restored Replica keeps its NoFDescriptor with size/address/protocol/endpoint intact. The failing half is the tail:

auto public_replicas =
    service.GetReplicaList("standby_restore_nof_key", kDefaultTenant);
ASSERT_TRUE(public_replicas.has_value());

That assertion pins exactly the behavior #3826 reports: a restored NoF replica being served as readable while the namespace may have handed its offset to someone else. So the test should flip, not the fix. Keep the descriptor assertions (preservation is still the contract), rename the tail to assert the fail-closed read, i.e. ASSERT_FALSE(public_replicas.has_value()) with REPLICA_IS_NOT_READY, mirroring your new RestoreDoesNotKeepNoFReplicasUsable. The wheel lanes look red from the same suite; once this one test is updated the gate should go green.

One thing worth a second opinion from the HA folks: after ClearInvalidHandles reaps the replica, the object is left with zero replicas, so a client that Put before the failover and reads after it gets REPLICA_IS_NOT_READY and has to re-Put. That is the intended fail-closed tradeoff, but it deserves a line in the PR description so the behavior change is explicit for release notes.

[pjdurden]

yes, that is implementable, and you are right that the expired weak pointer is the wrong mechanism. the quarantine you are describing already exists in this tree for memory replicas, i just did not reuse it.

the memory path does exactly this:

• RestoreFromStandbySnapshot keeps the placeholder allocator alive in standby_allocator_keepalive_ and records the endpoint in invalid_replica_endpoints_ (master_service.cpp:3434) when the segment is not present locally.
• IsReplicaReadable then hides any replica on that endpoint (master_service.cpp:3782), while get_descriptor() still returns the complete descriptor, because the allocator is alive.
• ReMountSegment imports the restored ranges (BuildRegionLiveAllocations + ImportOffsetBufferAllocator / ImportCachelibBufferAllocator + ReplaceAllocators), calls replace_memory_buffer on each restored replica, then erases the endpoint from invalid_replica_endpoints_ and drops the keepalive (master_service.cpp:1293).

applying that shape to NoF is a small change on the restore side: keep the allocator alive rather than scoping it to the loop, and add buffer.transport_endpoint_ to restored_invalid_endpoints.

that also removes the reaping you flagged. BuildStaleHandleCleanupPlan keys off has_invalid_mem_handle() || has_invalid_nof_handle() || has_stale_local_disk_client() (master_service.cpp:2150) and never consults invalid_replica_endpoints_. so with the allocator alive, has_invalid_nof_handle() is false (replica.h:445), ClearInvalidHandles leaves the replica alone, and the full descriptor including the endpoint survives for a later remount. strictly better than what i pushed.

one caveat on the keepalive. restored_allocators is keyed by transport endpoint and shared between memory and NoF, and ReMountSegment erases standby_allocator_keepalive_[te_endpoint] on a memory remount (master_service.cpp:1295). if a NoF endpoint collides with a memory one, an unrelated memory remount would drop the NoF keepalive and put us straight back into the expired pointer. so the NoF placeholders need their own map rather than sharing that one. this is the same aliasing hazard i mentioned in the PR description, it just has to be fixed rather than sidestepped.

this also recovers most of #3796. RestoreFromStandbyPreservesNoFBufferDescriptor asserts the descriptor round trips, including transport_endpoint_ (lines 1991-1994), and quarantine satisfies all four, which is what the test name is actually about. what it cannot satisfy is the tail, lines 1996-2000, where GetReplicaList has to serve the replica. serving it is the corruption. so that tail is the specific part of the #3796 contract i think needs amending, not the whole test. much narrower than option 3.

on blocking fresh allocation, i want to correct my own PR description. i argued option 2 was not implementable because the snapshot carries no NoF base or capacity. that is true at restore time, but it is the wrong place to look. ScopedNoFSegmentAccess::MountSegment builds the fresh empty allocator via CreateBufferAllocator over the whole [base, base+size) (segment.cpp:1200), and at that point it has the real NoFSegment with base, size and te_endpoint (types.h:460). the gate belongs in the mount path, where the parameters are already in hand, so the snapshot gap does not block it.

which splits the way @he-yufeng suggested in #3826:

1. fail closed. quarantine on restore, and reject a NoF mount whose te_endpoint is quarantined. closes the corruption window, keeps every breadcrumb, and is almost entirely existing machinery.
2. make it usable. mount imports the restored (offset, size) ranges for that endpoint, swaps in an allocator that owns them, replaces the buffers, and clears the quarantine, mirroring ReMountSegment. this is the real work: there is no NoF analog of ReplaceAllocators or replace_memory_buffer in the tree today, so it needs a replace_nof_buffer and a NoF import path.

i would like to keep this PR at step 1 and do step 2 as a follow up, since this one is a corruption fix. say the word if you would rather see them together.

two things i would like your read on before i push:

• an endpoint that never returns leaves those objects in the metadata index indefinitely, since nothing reaps a quarantined replica any more. memory behaves the same way today, so i would rather match it than invent a NoF specific TTL, but if you want quarantine to expire that is not my call to make.
• the mount side gate sits inside USE_NOF and CI builds with USE_NOF=OFF, so it will not be covered there. the restore side is compiled unconditionally and stays covered by master_service_ha_test.

and if the project would rather declare restored NoF data disposable after promotion, i am happy to keep what is pushed now and update the #3796 contract instead. i just agree it should be an explicit decision rather than a side effect of an expired weak pointer.

[pjdurden]

pushed the fail-closed half as 1ef2989, rebased onto current main (c329f194). title and description updated, since the change is no longer about invalidating the handle.

what is in it:

• restore keeps the placeholder allocator alive and puts the endpoint in invalid_nof_replica_endpoints_, so the descriptor stays complete including transport_endpoint_, has_invalid_nof_handle() stays false, and ClearInvalidHandles no longer reaps the replica.
• IsReplicaReadable consults the new set, so the replica is hidden from every read path. NoF replicas are still checked against invalid_replica_endpoints_ too, so this is additive there.
• MountNoFSegment and ReMountNoFSegment refuse a quarantined namespace with UNAVAILABLE_IN_CURRENT_STATUS, both taking snapshot_mutex_ before segment access to match memory MountSegment.
• the NoF keepalive and quarantine sets are separate from the memory ones, for the aliasing reason above: ReMountSegment erases those by endpoint, so a colliding NoF te_endpoint would otherwise be released by an unrelated memory remount.

RestoreFromStandbyPreservesNoFBufferDescriptor keeps its four descriptor assertions unchanged, quarantine satisfies all of them. only the GetReplicaList tail is updated.

two things i would rather you decide than have me guess, both in the description but repeating the first here because it is the real cost of fail-closed: while an endpoint is quarantined the whole namespace cannot be mounted, not just the restored ranges, so NoF is unavailable after a promotion that had NoF replicas until the import path lands. if you want a narrower gate or an operator override, i will add it. and nothing lifts the quarantine yet, so objects on a permanently gone endpoint stay in the index, the same way restored memory replicas do today.

usual caveat, i still cannot build this checkout, so the new tests have not been run. worth treating CI as the gate. and the build-wheel / build (3.10) failure earlier in this PR was a pip install torch==2.13.0 download that died mid-stream, unrelated to the change.

@he-yufeng does this match the two-step split you had in mind in #3826?

[catyans]

Thanks @pjdurden—this is a much cleaner fail-closed shape. LGTM.

The restored NoF descriptor now stays intact for a future allocator import, while the separate keepalive/quarantine state keeps it out of every read path and prevents a fresh mount from reusing its offsets. Separating the NoF state from the memory-remount maps also avoids the endpoint-aliasing lifetime problem we discussed. The updated HA test captures the important contract well: preserved, not served, and not reaped.

This also lines up with @he-yufeng's two-step split from #3826. Making quarantined replicas usable again can stay as the follow-up; keeping the namespace unavailable until that import path exists is the right safety tradeoff for this corruption fix. Appreciate how carefully you worked through the conflicting contracts here.

[catyans]

LGTM on the rebased head. I checked the range-diff rather than relying on the stale approval: the separate NoF keepalive/quarantine contract is preserved, and the conflict resolution cleanly adapts it to main's new restore-state and readable-descriptor paths. CI is green.

Thanks again @pjdurden for turning the original allocator-lifetime conflict into an explicit, testable quarantine state. The fail-closed half still looks like the right boundary for this corruption fix; allocator import can remain the follow-up.

[fcczzz]

One question about the descriptor-preservation guarantee: does NoFBatchEvict need to check the quarantine set too? Its predicate appears to accept these restored replicas (NoF, COMPLETE, refcnt == 0), so allocation pressure could remove their descriptors after the lease expires. I may be missing a guard here—could you confirm what prevents that path?

[pjdurden]

good catch, it did. is_evictable_nof_replica only checked type, completeness and refcount, and restored objects that aren't grouped come back with an already-expired lease, so the first NoF eviction pass would erase the descriptor the quarantine is meant to keep. the memory path never had this because it goes through IsEvictableMemoryReplica(), which checks IsReplicaReadable().

pushed d2e85c6, which adds the same readability check to the NoF predicate, plus NoFBatchEvictSkipsQuarantinedReplicas, which restores a NoF replica, runs NoF eviction, and asserts the descriptor is still there. the description's "excluded from the eviction paths" was only true for memory before this.

[fcczzz]

Thanks for addressing the eviction gap. The readability check and the new regression test cover the descriptor-preservation issue I raised.

The CTest build is still failing because these tests use interfaces that have moved to MasterServiceTestPeer on main. Please change the eviction call to MasterServiceTestPeer(service).RunNoFBatchEvictForTesting(...), and use MasterServiceTestPeer::MetadataAccessorRO and MasterServiceTestPeer::ObjectIdentity in HasInvalidNoFHandleForTesting.

Both errors appear in the current CI log. Once those accesses are updated and the HA tests pass, my previous concern is resolved.

[cage-goat]

@pjdurden Thanks for the fix! Are you planning to follow up with the allocator recovery step discussed in #3826, so restored NoF replicas can become usable again? cc @he-yufeng

[pjdurden]

@fcczzz thanks, rebased onto current main and switched both to MasterServiceTestPeer (RunNoFBatchEvictForTesting and the MetadataAccessorRO / ObjectIdentity aliases) in f80c7e2. While rebasing I also noticed main's new rollback_restored_state snapshots the memory keepalive and invalid endpoint set but not the NoF ones, so a failed restore would have left the quarantine behind. Added those two to the rollback in the same commit.

I can't build this tree locally, so CI is the check here.

[pjdurden]

merged current main in (794bcb9). main now gates reads through IsAwaitingRemount(), so the NoF quarantine check lives there instead of in TryGetReadableReplicaDescriptor; behaviour is the same.