[Store] OffsetAllocator: survive restarts with configurable persistence#2932
[Store] OffsetAllocator: survive restarts with configurable persistence#2932LujhCoconut wants to merge 11 commits into
Conversation
Add persistence support for OffsetAllocatorStorageBackend: - Add OffsetPersistMode (disabled/relaxed/strict) and config fields. - Persist allocator state, FIFO index, and eviction tombstones to kv_cache.meta via atomic tmp + fdatasync + rename + dir fsync. - Implement recovery path in Init(): load metadata, rebuild shard maps from allocator used nodes, restore/repair FIFO index. - Add datasync() to StorageFile/PosixFile/UringFile. - Add SetDeleteOnWriteFail flag to keep arena file on write failures. - Add recovery helpers visit_used_nodes / createHandleAtNode to OffsetAllocator.
There was a problem hiding this comment.
Code Review
This pull request introduces a persistence and recovery mechanism for the OffsetAllocatorStorageBackend. Key additions include data synchronization interfaces, metadata serialization, and recovery routines to rebuild shard maps and restore the FIFO index from persisted checkpoints. The review feedback identifies a critical bug where stale tombstones are not filtered out when checkpointing is skipped, potentially leading to data loss on recovery. Additionally, the reviewer suggests defensive checks to prevent recovering corrupted empty keys and recommends using FdGuard for directory file descriptors to ensure resource safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…tence - Respect delete_on_write_fail_ in PosixFile/UringFile destructors so arena files are preserved on write failures when persistence is on. - Set metadata_dirty_ after eviction loop in BatchOffload so that batches with only evictions (or all writes failing) still get checkpointed. - Keep SetDeleteOnWriteFail(false) conditional on persist mode to preserve legacy kDisabled behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces persistence and recovery capabilities to the OffsetAllocatorStorageBackend. It adds support for periodic or strict metadata checkpointing, including serialization of the allocator state and FIFO index, as well as recovery logic to rebuild shard maps from the data file. Additionally, it updates PosixFile and UringFile with datasync operations and options to prevent unlinking files on write failures. The review feedback highlights two important issues: a potential space leak of allocator nodes during recovery when corrupt records are skipped, and a failure in SaveMetadata when using relative paths due to an empty parent path being passed to open.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
he-yufeng
left a comment
There was a problem hiding this comment.
I focused this pass on the crash-consistency of the persistence path — the part most likely to have a subtle durability bug — and the ordering is correct at every level, which is great to see:
- Data-before-metadata:
data_file_->datasync()runs beforeSaveMetadata(), so the arena bytes an offset refers to are durable before the metadata that claims that offset is used. That's the ordering that matters for recovery correctness. - Atomic metadata swap:
SaveMetadatawrites to.tmp,fdatasyncs the tmp fd before the rename, renames, thenfsyncs the parent directory. The directory fsync is the step that's usually missing — without it a crash right afterrenamecan lose the swap even though the tmp bytes were durable — so it's good to see it here. - Fault coverage: the
test_metadata_write_failure_step_injection at the pre-fdatasync (step==2) and post-rename (step==3) points, plusSetDeleteOnWriteFail(false)guarding the data file across the metadata write, means the failure windows are actually exercised rather than assumed.
Defaulting to kDisabled keeps this a no-op for existing deployments, and gating kRelaxed on persist_interval_seconds >= 5 is a sane floor.
I haven't done a full pass on the recovery reconstruction (visit_used_nodes / createHandleAtNode / RebuildShardMapsFromAllocator) or the per-mode BatchOffload semantics, so I'm leaving this as a comment rather than an approval — but the durability discipline is solid.
- Implement destructor-based graceful-shutdown checkpoint. - Switch all_evicted_this_batch_ to unordered_set for dedup and O(1) stale-tombstone filtering. - Add metadata_consecutive_failures_ counter and test getters. - Harden SaveMetadata with fsync, full try/catch, and parent-dir fallback for relative paths. - Cross-validate node_index/offset in createHandleAtNode. - Remove duplicate friend declaration in __Allocator.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a persistence and recovery mechanism for the OffsetAllocatorStorageBackend. It introduces a datasync interface for storage files, persistence configuration modes, and metadata serialization to enable state recovery upon restart. The review feedback suggests improving the robustness of metadata loading by handling EINTR and short reads in pread, and using fstat instead of lseek to query the data file size safely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ize check - Loop pread() in LoadMetadata to handle EINTR and partial reads. - Use fstat() instead of lseek() to verify data file size during recovery.
- Use fstat() instead of lseek() to get metadata file size in LoadMetadata. - Only accumulate eviction tombstones when persist mode is enabled, avoiding unnecessary work in kDisabled mode.
Resolved conflicts from upstream changes: - EvictionHandler typedef (eviction_handler now returns tl::expected) - EvictAboveDiskWatermark virtual method on StorageBackendInterface - Disk watermark config fields on FileStorageConfig - Dead eviction_handler lambda in file_storage.cpp (upstream uses NotifyEvictedDiskReplicas inline lambda instead) - Orphan merge-residue lambdas in storage_backend_test.cpp
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…end-persist Resolve conflicts in mooncake-store/src/storage_backend.cpp against upstream kvcache-ai#2964 (roll back failed OffsetAllocator evictions): - Adopt upstream's PendingEviction + NotifyAndCommitPreparedEviction flow in BatchOffload: eviction victims stay pinned until the master accepts their removal, and failed notifications roll the metadata back via RestorePreparedEviction. - Move eviction tombstone recording from EvictToMakeRoom (victim selection time) to commit time via the new RecordEvictionTombstones, so rolled-back evictions never leave stale tombstones that Phase C would wrongly delete on recovery. - Take upstream's storage_backend_test.cpp verbatim: the branch's changes there were lambda-formatting churn only, and upstream adds rollback/post-loop-flush eviction coverage. Verified: storage_backend_test 72/72 passed.
Fix the previous merge commit's missing RecordEvictionTombstones declaration in storage_backend.h (broke full builds), plus a round of crash-consistency fixes for the persistence feature: - Record format v2: RecordHeader grows 8B -> 20B (seq + CRC-32C). Every write is stamped with an insert_seq and a CRC over header-prefix + key + value. Recovery now drops records with seq >= checkpoint insert_seq (post-checkpoint / reused extents) and rejects CRC mismatches (torn writes) instead of serving stale data. Duplicate keys from pinned-overwrite extents resolve by higher seq. Persisted metadata version bumped to 2. - kStrict: move the persistence barrier after the post-loop eviction flush so trailing eviction tombstones are checkpointed in-batch. - Recovery distinguishes transient errors (fd exhaustion, OOM, I/O errors -> fail Init, keep data) from corrupt/incompatible metadata (fresh start), avoiding cache loss on transient failures. - Allocator deserialization sanity-checks capacity/multiplier fields (corrupt meta no longer risks OOM-kill or shift UB); freeAllocation saturates metrics instead of wrapping; leaked nodes are freed with their exact requested size when known. - Reject keys > 1MB at write time (matches the recovery-side cap). - datasync failures now count into metadata_consecutive_failures_. - Add 6 persistence/recovery unit tests (roundtrip, destructor checkpoint, eviction tombstone, CRC corruption, post-checkpoint write rejection, oversized key) -- the feature had none. Verified: storage_backend_test 78/78, offset_allocator_test 42/42, full build clean.
…ed value) RecordHeader grows from 20 to 24 bytes: a new flags field makes the per-record CRC-32C optional (kFlagHasCrc), and the value region now starts at a 4 KiB boundary within the record via zero padding derived purely from key_len. seq stays mandatory as the checkpoint guard. - Write/read/recovery/ScanMeta all derive the layout from the shared RecordHeader helpers (ValuePadding/ValueOffsetInRecord/RecordSize). - Recovery skips the value scan for unchecksummed records and drops records carrying unknown flag bits (future formats). - New OffsetAllocatorBackendConfig::enable_record_crc (default true) with MOONCAKE_OFFSET_RECORD_CRC env override. - Persist format version bumped to 3; older metadata is rejected. - ScanMeta reports key_size from the key itself (total_size now includes alignment padding). This prepares the layout for DMA (GDS) writers whose values never touch the CPU and therefore cannot be CRC'd on write. Tests: storage_backend_test 82/82 (4 new), offset_allocator_test 42/42, file_storage_test 23/23.
Description
Add configurable crash-safe persistence to
OffsetAllocatorStorageBackend, allowing cached KV data to survive restarts.Background
Currently
OffsetAllocatorStorageBackend::Init()openskv_cache.datawithO_TRUNC, destroying all cached data on every process restart. While this is acceptable for LLM inference (KV cache is ephemeral — loss only causes cache miss, not correctness errors), some deployments benefit from warm restarts.This PR adds an optional persistence layer with three configurable modes, defaulting to existing
O_TRUNCbehavior (zero overhead, zero change for existing users). Also includes graceful-shutdown checkpoint (destructor forcesSaveMetadataon clean exit) and eviction-tombstone deduplication (unordered_set+ consecutive-failure counter with rate-limited logging).Persistence Modes
kDisabledO_TRUNCon Init, no metadata writtenkRelaxeddatasync()data + atomic-write metadata on each checkpoint. Destructor forces final checkpoint on clean exit.kStrictBatchOffloadis durable before returningUsage / Configuration
All persistence settings are controlled via environment variables. No configuration changes are required for existing deployments — the default
kDisabledmode is identical to current behavior.Enabling persistence (set before starting the storage node):
Full list of persistence environment variables:
MOONCAKE_OFFSET_PERSIST_MODEdisabled,relaxed,strictdisabledMOONCAKE_OFFSET_PERSIST_INTERVAL_SECONDS60kRelaxedmodeUse cases:
disabled(or unset)relaxedstrictrelaxedwithpersist_interval_seconds=10Verifying persistence is working:
Rollback safety: if you need to revert to an older binary without persistence support, the old binary ignores
kv_cache.metaandkv_cache.meta.tmpfiles (treats them as unknown on-disk artifacts). Data will be lost on restart (same as pre-PR behavior), but no crash or corruption occurs. To clean up leftover files after rollback, deletekv_cache.metaandkv_cache.meta.tmpfrom the storage directory.Monitoring (via public getters on
OffsetAllocatorStorageBackend):GetMetadataSaveFailures()SaveMetadatafailures (disk full, permission errors, etc.)GetMetadataLoadFallbacks()last_save_metadata_cost_us_)SaveMetadatacallArchitecture
On-disk layout: two files per arena
kv_cache.data[u32 key_len][u32 value_len][key bytes][value bytes]kv_cache.metastruct_pbserializedOffsetAllocatorPersistedMetadataWrite ordering (crash consistency):
data_file_->datasync()— ensure all written records are durablestruct_pb::to_pb(meta)→ write to.meta.tmp→fsync(tmp)→rename(tmp → meta)→fsync(parent_dir)— atomic metadata updatecomplete_handler()— notify master of new keysRecovery flow (
Init()):kv_cache.metaexistspersist_mode != kDisabled):LoadMetadata()→deserialize_from<OffsetAllocator>(blob)→visit_used_nodes()to walk all used allocator nodes → read each key from data file → rebuild shard maps +total_size_/total_keys_→ restore FIFO index → Phase C delete evicted-but-resurrected keysFallbackGuardincrementsmetadata_load_fallbacks_counter, fall through to fresh-start path (O_TRUNC)Key RAII guards:
DirtyGuard: wrapsSaveMetadata()body. On any failure (includingstd::bad_alloc), restoresmetadata_dirty_ = trueand incrementsmetadata_save_failures_. On success,disarm()keeps dirty=false.FallbackGuard: wrapsTryRecoverFromMetadata(). On anyreturn false, incrementsmetadata_load_fallbacks_. On success,disarm().FdGuard: anonymous namespace RAII fd closer, shared byInit(),TryRecoverFromMetadata(),LoadMetadata(), andSaveMetadata()'s directory fsync.Graceful-shutdown checkpoint:
~OffsetAllocatorStorageBackend()checkspersist_mode,initialized_,data_file_, andmetadata_dirty_, then forcesdatasync()+SaveMetadata()+clear()on clean exit (SIGTERM, rolling restart). Wrapped intry/catch(...)— destuctor never throws. Move-ctor/default-copy-delete explicitly declared to avoid implicit suppression.Eviction-tombstone dedup:
all_evicted_this_batch_isstd::unordered_set<std::string>— natural dedup, O(1) lookup for stale-tombstone filtering.metadata_consecutive_failures_counter logs every 10th consecutive checkpoint failure atWARNINGlevel (using bareLOG(WARNING), notLOG_FIRST_N— the latter counts by call-site, not content).Tombstone filtering: runs on every persist-enabled batch (not just checkpoint-triggering ones), removing keys that were successfully re-written in the current batch from
all_evicted_this_batch_. Prevents stale-tombstone data-loss bug where a re-written key would be deleted on the next checkpoint's recovery.Corrupt-node recovery:
RebuildShardMapsFromAllocatorusesfree_leaked_node()lambda (constructs temporaryOffsetAllocationHandlewithrequested_size=0then lets RAII free the node) at every corrupt-record skip branch. Duplicate-key nodes are freed via the normalallocation_ptrRAII path (keep-first, free-duplicate).createHandleAtNodecross-validates thatreal_offsetmatches the node's storeddataOffset << multiplier_bits— mismatch returnsnulloptfor defense-in-depth.Compatibility
kDisabled: zero behavior change. All 56 existing tests pass.kRelaxed/kStrict: data files backward-compatible (record format unchanged). Metadata hasversionfield — unrecognized versions trigger fresh-start fallback.FileStorage/MasterService: no changes needed.all_evicted_this_batch_type change: internal only — no on-disk format impact (serialized viaassign(begin,end)tovectorfield instruct_pb).Known Limitations (V1)
kRelaxedaccepted trade-off; destructor checkpoint reduces window for clean exits;kStricteliminates itmetadata_dirty_skips idle checkpoints; future: incremental WALPost-Merge Optimizations
unordered_setdedup + consecutive-failure counter (this PR)kv_cache.indexfile for fast recovery (no data-file I/O)Module
mooncake-store)Type of Change
How Has This Been Tested?
Test commands:
Test results:
kDisabledbehavior verified unchangedChecklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passdocs/source/deployment/offset-evict.md/home/ljh/develop/todo/plan_offset_presist.md(v22)/home/ljh/develop/todo/opt_persist.mdAI Assistance Disclosure
AI tools (Claude Code) assisted with: design iteration (~22 rounds adversarial review), code generation (all 7 files), iterative bug fixing, and optimization design. The human submitter reviewed, understood, and approved all changes.