[Store] Extract per-object metadata and runtime state headers from MasterService by CAICAIIs · Pull Request #4127 · kvcache-ai/Mooncake

CAICAIIs · 2026-09-15T04:42:36Z

Description

Moves the per-object metadata envelope and the per-key runtime state records out of MasterService into their own headers:

New header	Contents
`mooncake-store/include/object_metadata.h`	`ResolvedSoftPinRequest`, `ObjectMetadata` (with its nested `SoftPinEvaluation`, `PendingSoftPinAction`, `DynamicReplicaRecord`)
`mooncake-store/include/object_runtime_state.h`	`DynamicReplicaPending`, `ReplicationTask`, `OffloadingTask`, `PromotionQueueResult`, `PromotionCandidateReason`, `PromotionCandidate`, `PromotionTask`

These are per-object types, but they are declared inside the service that owns the metadata shard tables. Anything that needs one of them pulls in the whole service header and the quota/HA machinery behind it, and the object's own state cannot be moved out of the shard tables while its declarations live there. RFC #3670 moves metadata ownership from shards to tenants and narrows the mutation boundary from one shard lock to one object lock; separating the types is the step that makes that move expressible.

object_runtime_state.h names the records after what they are rather than after their consumer.

No behavior change. The declarations are unchanged apart from the dedent and the clang-format re-wrap that follow from leaving their old nesting, and the MasterService:: qualification dropped where these types are named.

Changes on top of the move:

ResolvedSoftPinRequest::ttl_ms and PendingSoftPinAction::ttl_ms become std::chrono::milliseconds ttl, and ComputeSoftPinDeadline takes that duration rather than a raw uint64_t. The rep-range guard it carried is unreachable once the parameter is a duration, so it is gone; the config-facing uint64_t values are converted at the boundary in ResolveSoftPinRequest.
PendingSoftPinAction::eligible_replica_ids is a std::unordered_set<ReplicaID>. The only question asked of it is membership, once per replica on two paths, so the set states that directly and neither query scans.
The object_metadata.h module comment now says what actually guards the envelope: it has no ownership lock, and the internal spin lock it does carry covers only lease_ and soft_pin_timeout.

Module

Mooncake Store (mooncake-store)

Type of Change

Refactor

How Has This Been Tested?

Build and the store regression set on a 256-core Ubuntu box, Debug + Ninja, USE_ETCD=ON, GNU 11, run over the same subset on upstream/main and on this branch so the two can be compared.

Test commands:

cmake --build build -j$(nproc)

cd build && LD_LIBRARY_PATH=$PWD:$PWD/mooncake-store:$PWD/mooncake-store/src:$PWD/mooncake-common:$PWD/mooncake-common/etcd \
  MC_METADATA_SERVER=http://127.0.0.1:18080/metadata DEFAULT_KV_LEASE_TTL=500 \
  ctest -R 'master_service|master_scenario|high_availability|tenant|object_entry|object_index|group_index|dynamic_replication|promotion' \
  -j8 --output-on-failure

Test results:

Build clean on both trees
31/31 pass on this branch, 31/31 on upstream/main — same subset, same build, same result. ObjectMetadata still carries no ownership lock; its internal SpinLock and the GUARDED_BY annotations are unchanged.
clang-format --dry-run reports no new violations on the files this PR touches.

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 — the existing suites exercise every moved type
For changes >500 LOC: this is derived from the tenant-first design RFC ([RFC]: Tenant-First Object-Level Metadata Locking #3670)

AI Assistance Disclosure

AI tools were used (see issue [RFC]: Tenant-First Object-Level Metadata Locking #3670; the submitter has reviewed every changed line)

Copilot

🟢 Approval recommended

The refactor is fully reviewed; the remaining documentation nit is non-blocking.

Pull request overview

Extracts per-object metadata and runtime-state types from MasterService into standalone headers without behavior changes.

Changes:

Adds object_metadata.h and object_runtime_state.h.
Updates master_service.h, implementation, and tests to use extracted types.
Preserves existing declarations and locking behavior.

File summaries

File	Summary
`mooncake-store/tests/promotion_on_hit_test.cpp`	Updates promotion result type references.
`mooncake-store/tests/master_service_test_fixture.h`	Updates metadata type references.
`mooncake-store/tests/ha/master_service_ha_test.cpp`	Updates promotion task type references.
`mooncake-store/src/master_service.cpp`	Updates extracted type references.
`mooncake-store/include/object_runtime_state.h`	Defines standalone runtime-state types.
`mooncake-store/include/object_metadata.h`	Defines standalone metadata types; module documentation has a minor locking clarification nit.
`mooncake-store/include/master_service.h`	Includes the new headers and removes moved declarations.

Review details

Suppressed comments (1)

mooncake-store/include/object_metadata.h:9

This module comment says ObjectMetadata carries no lock, but the type still declares mutable SpinLock lock below (line 105), which protects the lease and soft-pin fields. Please distinguish the absence of an object-level ownership lock from this internal lock so the extraction does not document a locking contract that contradicts the implementation.

// state and the tenant quota ledger. It carries no lock of its own: whoever
// owns the envelope decides how it is guarded.

Files reviewed: 7/7 changed files
Comments generated: 0
Review effort level: Lite

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

Aionw · 2026-09-15T06:19:00Z

+
+struct ResolvedSoftPinRequest {
+    SoftPinAction action{SoftPinAction::PRESERVE};
+    uint64_t ttl_ms{0};


Use std::chrono::millisecond

Aionw · 2026-09-15T06:19:07Z

+
+    struct PendingSoftPinAction {
+        SoftPinAction action{SoftPinAction::PRESERVE};
+        uint64_t ttl_ms{0};


Same as above

…aders Move MasterService::ObjectMetadata, ResolvedSoftPinRequest and the per-key runtime task records (DynamicReplicaPending, ReplicationTask, OffloadingTask, PromotionQueueResult, PromotionCandidateReason, PromotionCandidate, PromotionTask) to namespace scope in their own headers. This is a relocation: the moved text is unchanged apart from the four-space nesting indent. No type is used with a MasterService:: qualification anywhere in the tree, and the seven members the move drops later (EraseReplica/EraseReplicaByID/GetMemReplicaCount/GetNoFReplicaCount/ HasMemReplica/HasNoFReplica/NeedsReadLeaseRefresh) have no callers, so the extraction is self-contained. The runtime-state records are named for their responsibility rather than for their consumer, per review feedback on the module boundary.

Aionw · 2026-09-15T06:44:59Z

+    struct PendingSoftPinAction {
+        SoftPinAction action{SoftPinAction::PRESERVE};
+        std::chrono::milliseconds ttl{0};
+        std::vector<ReplicaID> eligible_replica_ids;


Using std::unordered_set to manage those replica ids

The pending soft-pin action only ever asks whether a replica id belongs to the write being committed, never in what order or with duplicates, so an unordered_set says that directly. Both queries become O(1) instead of a linear scan over a vector: PendingSoftPinOwnsReplica runs per replica of a PutEnd walk, and ClearPendingSoftPinIfNoViableReplica runs per replica of the object. The two construction sites (PutStart and the UpsertStart replacement path) build the set instead of a vector.

Aionw

LGTM

Picks up 1a0c0a4 so the branch builds against current main.

1d7a189 made DeleteRange a pure virtual on HaKvBackend and updated the other test backends, but missed this one, so batch_oplog_snapshot_gc_test does not compile: FakeBackend is abstract and the test cannot instantiate it. Return INVALID_PARAMS, which is what the interface asks an unsupported backend to do and what the sibling fakes return. The GC path under test never deletes through this backend.

Resolves the collision with kvcache-ai#4135, which added MasterServiceTestPeer as the single test access boundary and aliased the per-object types off MasterService. Three of those aliases (ObjectMetadata, PromotionQueueResult, PromotionTask) have pointed at namespace-scope types since the extraction, so the peer now names them directly; the two test files that reach these types through the peer keep upstream's spelling and the fixture keeps the chrono TTL signature.

ykwd · 2026-09-16T06:17:04Z

Could you resolve the conflict? Thanks

The alias target already says which namespace the type lives in.

rishabhsinha17

I reviewed #3822 and followed the split here. Read the full diff at 348108e against main. The extraction is faithful. object_runtime_state.h carries the per-key task records over unchanged, and object_metadata.h carries the envelope with only the two refinements Aionw asked for, the chrono ttl type and the unordered_set of eligible replica ids. Both preserve behavior. Eligible replica ids are only ever membership tested and are unique within a write, and the rewritten ComputeSoftPinDeadline keeps the saturation contract, since ResolveSoftPinRequest now does the only uint64 to milliseconds conversion and does it behind the max_kv_soft_pin_ttl_ guard. One boundary did move. A ttl between int64 max ms and uint64 max used to saturate to a permanent pin and would now wrap negative, but hitting that takes an operator raising max_kv_soft_pin_ttl past roughly 292 million years, so I would leave it.

Nothing from the #3822 review is lost in this slice. This split leaves TenantScopedRemoveAllSuppressesClearOnRace and the rest of the suppression coverage in master_service_test.cpp alone, and the fixture change is only the deadline helper signature. My two open asks there, the suppression race coverage and a TEST_F for the rebind helper, belong to the tenant-scoped split and I will re-raise them on that PR.

Restore the MasterService class body that the merge resolution truncated, place PromotionTask::from_prefetch in the upstream object_runtime_state.h layout (kvcache-ai#4127), close the DummyClient batchProbeKey brace, and restore the buffer_pool.py lines the smoke suite imports. Also drop a duplicated MasterSnapshotManager friend line and an empty namespace left by the merge.

Adapt to upstream changes since the PR opened: - MasterService header split (kvcache-ai#4127): PromotionTask::from_prefetch moves to object_runtime_state.h; friend list follows the MasterServiceTestPeer form (kvcache-ai#4135) plus PrefetchTaskMasterTest. - probeKey/BatchProbeKey RPCs (kvcache-ai#3801) coexist with ExistOptions. - Python unified-package layout (kvcache-ai#3586): keep buffer_pool.py at its pre-existing path; migration is a separate follow-up.

Adapt to upstream changes since the PR opened: - MasterService header split (kvcache-ai#4127): PromotionTask::from_prefetch moves to object_runtime_state.h; friend list follows the MasterServiceTestPeer form (kvcache-ai#4135), plus PrefetchTaskMasterTest. - probeKey/BatchProbeKey RPCs (kvcache-ai#3801) coexist with ExistOptions. - Python unified-package layout (kvcache-ai#3586): keep buffer_pool.py at its pre-existing path; migration is a separate follow-up. - Design doc moved to docs/source/design/store/ssd-prefetch.md per the kvcache-ai#4196 module-grouped layout; drop stale toctree entries for moved docs.

github-actions Bot added run-ci Store labels Sep 15, 2026

CAICAIIs marked this pull request as ready for review September 15, 2026 05:56

Copilot AI lite review requested due to automatic review settings September 15, 2026 05:56

CAICAIIs requested review from 00fish0, Aionw, Icedcoco, Libotry, XucSh, YiXR, stmatengss and ykwd as code owners September 15, 2026 05:56

Copilot started reviewing on behalf of CAICAIIs September 15, 2026 05:56

View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Aionw reviewed Sep 15, 2026

View reviewed changes

CAICAIIs force-pushed the tenant-first-module-foundation branch from 4814bcf to b918c9b Compare September 15, 2026 06:33

Aionw reviewed Sep 15, 2026

View reviewed changes

Aionw approved these changes Sep 15, 2026

View reviewed changes

CAICAIIs added 2 commits September 15, 2026 15:54

Merge upstream/main into the extraction branch

2f972ae

Picks up 1a0c0a4 so the branch builds against current main.

Aionw mentioned this pull request Sep 15, 2026

[Store] Centralize MasterService test access in a test-only peer #4135

Merged

6 of 32 tasks

style(store): drop a comment that restates the alias it sits on

348108e

The alias target already says which namespace the type lives in.

rishabhsinha17 approved these changes Sep 16, 2026

View reviewed changes

ykwd approved these changes Sep 16, 2026

View reviewed changes

ykwd merged commit 28644ce into kvcache-ai:main Sep 16, 2026
23 checks passed

rishabhsinha17 mentioned this pull request Sep 22, 2026

[RFC][Store] Scenario DSL and deterministic concurrency runner for MasterService tests #3098

Open

DHX98 mentioned this pull request Sep 28, 2026

[Store] Prefetch SSD-only objects to DRAM on exist #4272

Merged

10 tasks done

CAICAIIs mentioned this pull request Sep 28, 2026

[Store] Serve MasterService metadata from the tenant registry #4370

Closed

11 of 13 tasks

Navigation Menu

Repository navigation

[Store] Extract per-object metadata and runtime state headers from MasterService - #4127

[Store] Extract per-object metadata and runtime state headers from MasterService#4127
ykwd merged 6 commits into
kvcache-ai:mainfrom
CAICAIIs:tenant-first-module-foundation

CAICAIIs commented Sep 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Aionw Sep 15, 2026

Uh oh!

Aionw Sep 15, 2026

Uh oh!

Aionw Sep 15, 2026

Uh oh!

Aionw left a comment

Uh oh!

ykwd commented Sep 16, 2026

Uh oh!

rishabhsinha17 left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

CAICAIIs commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Module

Type of Change

How Has This Been Tested?

Checklist

AI Assistance Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (1)

Uh oh!

Aionw Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Aionw Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Aionw Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Aionw left a comment

Choose a reason for hiding this comment

Uh oh!

ykwd commented Sep 16, 2026

Uh oh!

rishabhsinha17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CAICAIIs commented Sep 15, 2026 •

edited

Loading