[Store] Assert HA restore state from domain state, not metrics - #5
Open
Hubert-Zhu wants to merge 1 commit into
Open
Hubert-Zhu wants to merge 1 commit into
Hubert-Zhu wants to merge 1 commit into
Conversation
The standby restore and remount tests read master_allocated_mem_size to verify how many bytes each step accounted for. That makes an observability gauge the oracle for business state, which RFC kvcache-ai#3158 explicitly rules out, and it pins the tests to the legacy incremental metric writes rather than to the authoritative state those writes describe. Assert the two authoritative owners instead. Bytes restored from a standby snapshot live in MasterService::standby_accounted_memory_bytes_ until ReMountSegment hands them to a real allocator; before that they hang off DummyBufferAllocator, which is never attached to the segment usage tracker and so is invisible to the allocator-side view. Bytes owned by a mounted segment live in that allocator, already reachable through the existing SegmentAllocatedSizeForTesting helper. Splitting the assertions along that boundary also lets each remount test state that the handoff drained the standby ledger, which the single aggregate gauge could not express. Guard the restore-failure comparison so it cannot hold on two zeroes if the accounting is ever empty at that point, which matters because the upcoming DRAM gauge work touches this bookkeeping. No production behavior changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Hubert-Zhu
force-pushed
the
hzhu/store-tests-domain-state-oracle
branch
from
September 4, 2026 01:58
2f56143 to
ff122df
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The standby restore and remount tests in
master_service_ha_test.cppreadmaster_allocated_mem_sizeto verify how many bytes each step accounted for.That makes an observability gauge the oracle for business state, which
RFC #3158 explicitly
rules out, and it pins the tests to the legacy incremental metric writes
rather than to the authoritative state those writes describe.
This replaces all 11 such assertions with assertions on the authoritative
owners. The gauge is the sum of two distinct things, and the tests were
conflating them:
MasterService::standby_accounted_memory_bytes_untilReMountSegmenthands them to a real allocator. Before the remount these replicas hang off
DummyBufferAllocator, which is never attached to the segment usagetracker, so they are invisible to the allocator-side view.
reachable via the existing
SegmentAllocatedSizeForTestinghelper.Splitting the assertions along that boundary also lets each remount test
state that the handoff drained the standby ledger
(
StandbyAccountedBytesForTesting(service) == 0), which the single aggregategauge could not express.
This is a prerequisite for removing the legacy DRAM gauge writers: those
tests would otherwise be the only thing keeping the incremental writes alive.
No production code and no production behavior changes.
Module
mooncake-store)Type of Change
How Has This Been Tested?
Test-only change, scoped to
master_service_ha_test.cpp. The affected testsare
RestoreFailureKeepsExistingState,RemountMakesRestoredMemoryReplicaReadyandRemountRestoresCachelibMemoryReplica.Test commands:
Test results:
Not yet verified locally: the development machine has no cmake,
clang-format or pre-commit available, so this branch has not been compiled,
run or formatted locally. Every added line was checked by hand against the
80-column limit, but clang-format's exact wrapping is unverified.
Checklist
./scripts/code_format.shAI Assistance Disclosure
Cursor was used to locate the gauge-based assertions, trace the standby
accounting lifecycle through
RestoreFromStandbySnapshotandReMountSegment, and draft the replacement assertions.Made with Cursor