Skip to content

fix(storage): preserve FIFO eviction order across restart for recovered blocks - #23

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-preserve-fifo-eviction-order-across-re-da6068
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-preserve-fifo-eviction-order-across-re-da6068

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 11, 2026

Copy link
Copy Markdown

Detail bug report: View on Detail

Bug

After a restart, the disk-cache FifoPicker evicted recovered blocks in a corrupted order — a newer-written recovered block could be reclaimed before an older-written one.

Root cause: BlockManager::init (the recovery path) rebuilt the evictable set as a HashSet<BlockId> and iterated it in arbitrary order when notifying eviction pickers. FifoPicker derives FIFO order entirely from the call order of on_block_evictable (push_back), so the per-process-random HashSet iteration permuted its queue. Immediately after recovery every block has invalid == 0, so InvalidRatioPicker returns None and FifoPicker is the sole deciding picker for the first post-restart eviction round — making the corruption directly observable. (Bug introduced by the recovery-path refactor in foyer-rs#1074, which batched picker notifications over a HashSet.)

Fix

Derive the recovered blocks' recency from the persisted per-entry write sequence and feed them to the pickers oldest-written-first, instead of from a HashSet.

  • foyer-storage/src/engine/block/recover.rs: while scanning recovered entries, aggregate each block's recency = max(addr.sequence) over its entries, then sort the evictable blocks ascending by that sequence via a new order_evictable_blocks_by_sequence helper before handing them to BlockManager::init.
  • foyer-storage/src/engine/block/manager.rs: BlockManager::init now takes an ordered evictable_blocks: &[BlockId] and iterates it in order, with a doc comment stating the contract (oldest-written-first). It no longer rebuilds/iterates a HashSet.

Recency is keyed on addr.sequence, not on BlockId, so the order stays correct even after block ids are recycled by reclamation (a reclaimed id returns to the back of the clean list and can be reused, so a low id may hold newer data than a high id). The live runtime path (on_writing_finish → push_back in real write order) is untouched and remains correct.

Testing

  • New unit tests (recover::tests): the sort is keyed on sequence rather than block id (a low-id block holding the newest write is ordered last — covers the recycled-id case), handles empty input, and tie-breaks stably by ascending block id.
  • New end-to-end test (engine::tests::test_store_fifo_eviction_order_after_recovery): writes one entry per block across 7 of 8 blocks, restarts, then drives 4 sequential evictions and asserts the recovered blocks drain in strict oldest→newest write order (keys 0→1→2→3). This passes deterministically on the fix; I verified it catches the regression by temporarily restoring the original HashSet-based init (test failed) and then restoring the fix (test passed).
  • No regressions: full foyer-storage suite (30 tests) and the downstream foyer hybrid-cache suite (15 tests, including test_load_after_recovery and the hybrid fuzzy test) all pass. Routine cargo check, clippy -- -D warnings, and stable + nightly cargo fmt --check are clean.

Automatic Fixes PRs can be configured here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants