Skip to content

[Store] Keep an object's in-flight tasks on its own record - #4562

Merged
Aionw merged 2 commits into
kvcache-ai:mainfrom
Aionw:refactor/object-state
Oct 12, 2026
Merged

Aionw merged 2 commits into
kvcache-ai:mainfrom
Aionw:refactor/object-state

Conversation

@Aionw

@Aionw Aionw commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Step 1 of #4557: an object's in-flight tasks now live on the object's own record instead of in per-tenant side maps.

An object's processing marker and its replication, offload and promotion tasks were kept in four per-tenant maps keyed by the object's key, apart from its metadata (processing_keys, replication_tasks, offloading_tasks, promotion_tasks). Every path that ended an object or a task had to look the key up in each map and keep them consistent by hand, and a marker could outlive its object: the expiry sweep carried a branch for a processing key whose object was gone, and the accessor's double-erase crash of 2026-08-03 came from erasing the processing marker twice.

  • ObjectRecord. TenantState's object map now holds an ObjectRecord, the object's ObjectMetadata next to a route::ObjectState with the processing flag and the three tasks (HeapOptional, so an object with no task pays a pointer per kind). They live and die with the object under the same shard lock. EraseMetadata still gives back what they hold (source refcounts, pending quota, the promotion in-flight count) before the record goes.
  • Accessors. MetadataAccessorRW drops its iterators into the processing set and the replication map and exposes GetState(). The helpers that need a task (CancelQueuedOffloadTask, CleanupStaleHandles, CancelPromotionTaskForRemovedReplicas, ErasePromotionTaskIfPresent) take the record or its state instead of a key.
  • Expired-work sweep. DiscardExpiredProcessingReplicas walks a shard's objects once to collect those with work in flight, then runs its four passes over them in the same order, so a round costs N + 4I (main walks only its in-flight maps). It runs once per put_start_release_timeout_sec_ (10 minutes by default) or inside the eviction census, which already walks every object; a later step of [Track]: [Store] Land the tenant-first MasterService migration in staged PRs #4557 gives it an in-flight list so the walk drops to O(I).
  • Not in this PR. Promotion candidates and the dynamic-replication pending records and cooldowns keep their maps: each has a periodic scan that would otherwise walk every object, and they move with PromotionCandidateTracker and DynamicReplicationController (steps 2 and 3 of [Track]: [Store] Land the tenant-first MasterService migration in staged PRs #4557).
  • route::ObjectState is named after where it ends up: the striped object route keeps it in each key's slot (step 5 of [Track]: [Store] Land the tenant-first MasterService migration in staged PRs #4557). HeapOptional comes from the same series.

Behavior is unchanged: no RPC, snapshot or OpLog format changes.

Part of the code comes from #4370 by @CAICAIIs (the per-object state).

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Refactor

How Has This Been Tested?

Test commands:

cmake --build build -j16 -- -k
ctest -j12 -R 'master_service|promotion|offload|heap_optional|dynamic_replication|snapshot|master_scenario|tenant|evict|ssd'

Test results:

  • Unit tests pass

  • Integration tests pass (if applicable)

  • Manual testing done (describe below)

  • Build: the tree builds with GCC 16 (RelWithDebInfo, BUILD_UNIT_TESTS=ON). The only targets that fail, chaosctl (sleep not declared) and ordered_oplog_writer_test (libaio link), fail the same way on main.

  • GCC 11.4 (Ubuntu 22.04, the CI floor): every changed translation unit passes -fsyntax-only, except file_storage.cpp, whose change is one comment line and which needs liburing headers the check container does not have.

  • Tests: 58 of 58 pass, including master_service_test, master_service_test_for_snapshot, master_service_ha_test, promotion_on_hit_test, master_service_ssd_test, offload_on_evict_test, master_service_offload_scenario_test, master_service_processing_key_double_erase_test, dynamic_replication_test, master_service_tenant_quota_test, snapshot_child_process_test, the master_service_*_scenario_test suites and the new heap_optional_test.

  • Formatting: ./scripts/code_format.sh --check --changed-lines --base origin/main is clean with clang-format 20.1.8.

Checklist

AI Assistance Disclosure

  • AI tools were used (specify below)

AI assistance was used for the refactor and to draft this description. The submitter has reviewed every changed line and can defend the change end to end.

@Aionw
Aionw force-pushed the refactor/object-state branch from af647b3 to 12ef8a7 Compare October 10, 2026 08:59
@Aionw Aionw changed the title [Store] Keep each object's per-key task state on the object [Store] Keep an object's in-flight tasks on its own record Oct 10, 2026

@Icedcoco Icedcoco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI

Comment thread mooncake-store/src/master_service.cpp Outdated
LOG(ERROR) << "Key " << *key_it
<< " was removed while in processing";
key_it = tenant_state.processing_keys.erase(key_it);
for (auto it = tenant_state.metadata.begin();

@CAICAIIs CAICAIIs Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR these four passes walked the in-flight sets, so a round cost O(in-flight). They now walk every object of the tenant (11163, 11263, 11388, 11406), and BatchEvict calls this per shard under the shard write lock, so a round is 4N whether or not anything is in flight.

Could we collect the objects with anything in flight in one pass and run the same four passes over that collection? A round would then be N + 4I, with the pass order unchanged.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you've said already covered in following PR. This PR is intentionally doing this way to keep change small and easy to review.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconsidered: done in 899316c. One walk now collects the objects with work in flight, and the four passes run over that collection in the same order, so a round is N + 4I instead of 4N. Getting it down to O(I) like main needs an in-flight list on the record, which comes in a later step of #4557.

@CAICAIIs CAICAIIs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Aionw and others added 2 commits October 12, 2026 11:06
An object's processing marker and its replication, offload and promotion
tasks lived in four per-tenant maps keyed by the object's key, apart from
its metadata, so every path that ended an object or a task had to look the
key up in each map and keep them consistent by hand. Each object's metadata
now sits in an ObjectRecord next to a route::ObjectState holding these
four, so they live and die with the object under the same shard lock.
EraseMetadata still gives back what they hold (source refcounts, pending
quota) before the record goes.

The expired-work sweep now walks a shard's objects instead of the
processing set. It runs once per put_start_release_timeout_sec_ (10 minutes
by default) or inside the eviction census, which walks every object
already.

Promotion candidates and dynamic-replication state keep their maps for now:
each has a periodic scan that would otherwise walk every object, and they
move with PromotionCandidateTracker and DynamicReplicationController.

Part of kvcache-ai#4557.

Co-authored-by: CAICAIIs <3360776475@qq.com>
DiscardExpiredProcessingReplicas walked every object of a tenant once
per kind of in-flight work, four times in all, and BatchEvict runs it on
every shard under the shard write lock. Collect the objects with work in
flight in one walk and run the four passes over them, so a round costs
N + 4I instead of 4N, in the same pass order.
@Aionw
Aionw force-pushed the refactor/object-state branch from 12ef8a7 to 899316c Compare October 12, 2026 03:44
@Aionw
Aionw merged commit 96af10e into kvcache-ai:main Oct 12, 2026
29 checks passed
DHX98 added a commit to DHX98/Mooncake that referenced this pull request Oct 12, 2026
Adapt to upstream moving per-object in-flight tasks off TenantState onto
the object's own runtime record (kvcache-ai#4562): promotion_tasks is gone, so
RegisterPrefetchTask and the failure/success handlers work through
accessor.GetState().promotion_task (a HeapOptional<PromotionTask>) with
the from_prefetch flag re-added to PromotionTask. Also picks up the
tenant-quota control-plane split (kvcache-ai#4563).
he-yufeng added a commit to he-yufeng/Mooncake that referenced this pull request Oct 12, 2026
…move

kvcache-ai#4562 moved the in-flight promotion task from TenantState onto the
record's route::ObjectState and kvcache-ai#4563 put tenant_id/user_key on the
metadata, so the lease-gate cancel path no longer compiled against
main. Follow the new layout: read the task off record.state, take
source/tenant fields from the metadata, and reuse
ErasePromotionTaskIfPresent for the quota refund and counters.
Venkat2811 added a commit to Venkat2811/Mooncake that referenced this pull request Oct 19, 2026
The per-key ObjectState change (kvcache-ai#4562) moved the offload task from the
tenant's offloading_tasks map onto the object's record. The census now
counts records with an offloading_task, before the hard-pin skip, and
keeps the oldest start time for the queue-full log line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Venkat2811 added a commit to Venkat2811/Mooncake that referenced this pull request Oct 20, 2026
The per-key ObjectState change (kvcache-ai#4562) moved the offload task from the
tenant's offloading_tasks map onto the object's record. The census now
counts records with an offloading_task, before the hard-pin skip, and
keeps the oldest start time for the queue-full log line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants