[STORE] Implement FIFO eviction for OffsetAllocatorStorageBackend

[LujhCoconut]

Description

Add FIFO eviction support to OffsetAllocatorStorageBackend, matching the watermark-based eviction pattern already used by BucketStorageBackend. Previously, when the offset allocator ran out of space, offload was permanently disabled (KEYS_ULTRA_LIMIT). With this change, the backend can now evict old keys (FIFO order) to make room for new ones, and notify the master via the existing eviction_handler callback.

Key points:

• Default behavior unchanged: eviction_policy=NONE preserves the original break-on-full path byte-for-byte.
• Leverages existing refcount mechanism: AllocationPtr (shared_ptr) keeps in-flight read extents pinned, so eviction is a simple shard-map erase — no two-phase delete or inflight-reads spin-wait needed (unlike Bucket).
• Dual-dimension watermarks: both byte and key-count high/low watermarks trigger eviction, with lazy-repair of stale FIFO index entries.
• Fallback eviction loop: when allocate() fails despite watermarks being satisfied (fragmentation / node exhaustion), a bounded retry loop evicts victims and rechecks largest_free_region_.
• No changes to FileStorage or MasterService: the eviction_handler → BatchEvictDiskReplica contract is already in place.

Module

• Mooncake Store (mooncake-store)

Type of Change

• New feature

How Has This Been Tested?

Test commands:

cd build && cmake --build . --target storage_backend_test
./mooncake-store/tests/storage_backend_test

Test results:

• Unit tests pass: 55/55 (18 existing OffsetAllocator + 6 new eviction-specific tests)
• Builds clean with zero warnings

New eviction tests:

Test	What it verifies
Eviction_FifoOrder	Oldest key evicted first under byte watermark
Eviction_NoEvictionWhenNONE	Default NONE policy preserves break-on-full
Eviction_MasterNotifiedBeforeReuse	eviction_handler called before allocate reuses space
Eviction_PostLoopFlush	Evicted keys flushed after loop when last key fails allocate
Eviction_KeyCountTrigger	Key-count watermark triggers eviction with low bytes
Eviction_ConcurrentReadSafety	No data corruption under concurrent read + evict (refcount test)

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 run --all-files and all hooks pass
• I have updated the documentation (if applicable) — design doc at docs/source/deployment/offset-evict.md
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

• AI tools were used (specify below)

AI tools (Claude) assisted with design exploration, adversarial review, and iterative bug-fixing of the eviction implementation. The human submitter reviewed and approved all changes.

[gemini-code-assist]

Code Review

This pull request implements watermark-driven and fallback eviction policies (such as FIFO) for the OffsetAllocatorStorageBackend to manage storage capacity and key limits. The review feedback highlights two issues: a mathematically flawed progress check in the fallback eviction loop that prematurely terminates when evicting small keys for a larger allocation, and an incomplete warning condition when parsing the MOONCAKE_OFFSET_MAX_EVICT_PER_OFFLOAD environment variable that fails to warn on negative values.

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.

[LujhCoconut]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces watermark-driven FIFO eviction capabilities to the OffsetAllocatorStorageBackend, including configuration validation, proactive and fallback eviction logic in BatchOffload, and comprehensive unit tests. A critical logical bug was identified in the fallback eviction loop where clearing the evicted_keys vector causes the loop to prematurely break on the first iteration. A fix has been suggested to track the number of evicted keys per iteration before clearing the vector.

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.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 72.31969% with 142 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mooncake-store/src/storage_backend.cpp	52.17%	132 Missing ⚠️
mooncake-store/tests/storage_backend_test.cpp	95.74%	10 Missing ⚠️

📢 Thoughts on this report? Let us know!

[he-yufeng]

Read through the eviction path end to end. This is a careful implementation, and a few things are done right that are easy to get wrong: the dual-dimension (bytes + keys) high/low watermarks, the batch_keys protection so in-flight keys are never evicted mid-write, the lazy-repair of stale fifo_index_ slots, and the consistent eviction_mutex_ -> shard.mutex lock order across both the insert and evict paths. I also checked that the total_value_size > UINT32_MAX guard from #2570 survived the BatchOffload rewrite (and you extended it with the record_size > UINT32_MAX check), and that eviction frees the extent through the RefCountedAllocationHandle refcount so a concurrent BatchLoad reader keeps its bytes pinned. Nice.

A few points, none blocking:

1. Single-writer invariant. EvictToMakeRoom and the insert both do a check-then-act on total_size_ / total_keys_, and the atomics only make each individual load/store atomic, not the sequence. You already DCHECK and note "single heartbeat_thread_ serialises offload; if concurrent offload is added, these must be re-evaluated." Since the whole watermark accounting rests on that, it would be worth making the assumption harder to break by accident, e.g. a short comment or DCHECK at the BatchOffload entry stating that offload is single-writer, so a future change that parallelises offload doesn't silently turn the watermark math into a TOCTOU.

2. fifo_index_ orphan cleanup. The lazy-repair comment mentions "orphan slot from overwrite or prior deletion", but the overwrite path already erases the old fifo_seq under the lock, and I couldn't find a per-key delete path for this backend that removes from shard.map without also cleaning fifo_index_. Is the "prior deletion" case actually reachable today? If a delete/remove path exists (or is planned), it should erase the fifo_index_ slot under the same lock order, otherwise orphans accumulate between eviction runs on a delete-heavy workload. If keys are only ever removed via evict/overwrite, the comment could just say so.

3. Silent skip observability. The fallback loop bails on now_largest <= prev_largest (fragmentation, or extents still pinned by readers) and skips the key with a one-shot warning. Under concurrent-read or fragmentation pressure this degrades offload quietly. A counter/metric for eviction-driven skips would let operators tell "watermark is doing its job" apart from "we're thrashing and dropping offloads".

Question on tests: does the concurrent-read-safety case assert the pinned-extent path, i.e. evict a key while a BatchLoad refcount is held and confirm the allocator does not re-issue that offset until the read completes? That is the load-bearing safety property here, so an explicit assertion would be good to have.

[LujhCoconut]

Read through the eviction path end to end. This is a careful implementation, and a few things are done right that are easy to get wrong: the dual-dimension (bytes + keys) high/low watermarks, the batch_keys protection so in-flight keys are never evicted mid-write, the lazy-repair of stale fifo_index_ slots, and the consistent eviction_mutex_ -> shard.mutex lock order across both the insert and evict paths. I also checked that the total_value_size > UINT32_MAX guard from #2570 survived the BatchOffload rewrite (and you extended it with the record_size > UINT32_MAX check), and that eviction frees the extent through the RefCountedAllocationHandle refcount so a concurrent BatchLoad reader keeps its bytes pinned. Nice.

A few points, none blocking:

1. Single-writer invariant. EvictToMakeRoom and the insert both do a check-then-act on total_size_ / total_keys_, and the atomics only make each individual load/store atomic, not the sequence. You already DCHECK and note "single heartbeat_thread_ serialises offload; if concurrent offload is added, these must be re-evaluated." Since the whole watermark accounting rests on that, it would be worth making the assumption harder to break by accident, e.g. a short comment or DCHECK at the BatchOffload entry stating that offload is single-writer, so a future change that parallelises offload doesn't silently turn the watermark math into a TOCTOU.
2. fifo_index_ orphan cleanup. The lazy-repair comment mentions "orphan slot from overwrite or prior deletion", but the overwrite path already erases the old fifo_seq under the lock, and I couldn't find a per-key delete path for this backend that removes from shard.map without also cleaning fifo_index_. Is the "prior deletion" case actually reachable today? If a delete/remove path exists (or is planned), it should erase the fifo_index_ slot under the same lock order, otherwise orphans accumulate between eviction runs on a delete-heavy workload. If keys are only ever removed via evict/overwrite, the comment could just say so.
3. Silent skip observability. The fallback loop bails on now_largest <= prev_largest (fragmentation, or extents still pinned by readers) and skips the key with a one-shot warning. Under concurrent-read or fragmentation pressure this degrades offload quietly. A counter/metric for eviction-driven skips would let operators tell "watermark is doing its job" apart from "we're thrashing and dropping offloads".

Question on tests: does the concurrent-read-safety case assert the pinned-extent path, i.e. evict a key while a BatchLoad refcount is held and confirm the allocator does not re-issue that offset until the read completes? That is the load-bearing safety property here, so an explicit assertion would be good to have.

Thanks for the insightful suggestions — all of the points you raised have been addressed in the latest commit.

[he-yufeng]

Thanks — went through 6b3e5199 and all four are addressed cleanly:

• The SINGLE-WRITER PRECONDITION block in BatchOffload now spells out that the watermark check → evict → update sequence isn't atomic across threads and points at the DCHECK_GE guards, so a future concurrent-offload change can't silently turn the accounting into a TOCTOU. That's exactly what I was after.
• eviction_skips_ with the GetEvictionSkips() accessor gives operators the signal to tell "watermark is doing its job" apart from "we're thrashing and dropping offloads".
• The fifo_index_ orphan case is now documented in EvictToMakeRoom, so the reachability question is answered inline.
• The strengthened ConcurrentReadSafety test asserts the load-bearing property directly: the refcount keeps an evicted key's extent pinned, the allocator doesn't re-issue an in-use offset, and there's zero corruption across the concurrent reads.

Nice work on the eviction path overall — the dual-dimension watermarks, in-flight batch_keys protection, and the consistent lock order make it a careful implementation. LGTM.