From 03ec8c2a8cf18a3ff829a538a41789b790249f4b Mon Sep 17 00:00:00 2001 From: "detail-app[bot]" <180357370+detail-app[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:55:09 +0000 Subject: [PATCH 1/2] fix(storage): compute tombstone log resume slot from global offset --- foyer-storage/src/engine/block/engine.rs | 107 ++++++++++++++++++++ foyer-storage/src/engine/block/tombstone.rs | 91 +++++++++++++++-- 2 files changed, 190 insertions(+), 8 deletions(-) diff --git a/foyer-storage/src/engine/block/engine.rs b/foyer-storage/src/engine/block/engine.rs index 1b9d19d8..0cae0832 100644 --- a/foyer-storage/src/engine/block/engine.rs +++ b/foyer-storage/src/engine/block/engine.rs @@ -976,6 +976,52 @@ mod tests { .unwrap() } + /// Like `store_for_test_with_tombstone_log`, but with a 4 MiB + 64 KiB device so the + /// tombstone log spans 5 pages (1280 slots). The extra pages let the most-recent + /// tombstone leave page 0 after enough deletes, which is required to exercise the + /// cross-page resume path in `TombstoneLog::open`. + async fn store_for_test_with_multipage_tombstone_log( + dir: impl AsRef, + ) -> Arc, TestProperties>> { + let device = FsDeviceBuilder::new(dir) + .with_capacity(ByteSize::mib(4).as_u64() as usize + ByteSize::kib(64).as_u64() as usize) + .build() + .unwrap(); + let spawner = Spawner::current(); + let io_engine = io_engine_for_test(spawner.clone()).await; + let metrics = Arc::new(Metrics::noop()); + let builder = BlockEngineConfig { + device, + block_size: 16 * 1024, + compression: Compression::None, + indexer_shards: 4, + recover_concurrency: 2, + flushers: 1, + reclaimers: 1, + clean_block_threshold: 1, + eviction_pickers: vec![Box::::default()], + admission_filter: StorageFilter::new(), + reinsertion_filter: StorageFilter::new().with_condition(RejectAll), + enable_tombstone_log: true, + buffer_pool_size: 16 * 1024 * 1024, + blob_index_size: 4 * 1024, + submit_queue_size_threshold: 16 * 1024 * 1024 * 2, + flush_switch: Switch::default(), + load_holder: Holder::default(), + marker: PhantomData, + }; + let builder = Box::new(builder); + builder + .build(EngineBuildContext { + io_engine, + metrics, + spawner, + recover_mode: RecoverMode::Strict, + }) + .await + .unwrap() + } + fn enqueue( store: &BlockEngine, TestProperties>, entry: CacheEntry, ModHasher, TestProperties>, @@ -1194,6 +1240,67 @@ mod tests { ); } + /// Regression test for the tombstone-log resume bug. After a reopen with the + /// most-recent tombstone on a non-zero page, the writer must resume at the *global* + /// slot and not overwrite an older, still-needed tombstone on page 0. Otherwise a + /// deleted key whose data block is still on disk reappears as a phantom `load`. + #[test_log::test(tokio::test)] + async fn test_store_tombstone_log_resume_no_phantom_entry() { + let dir = tempfile::tempdir().unwrap(); + let memory = cache_for_test(); + let store = store_for_test_with_multipage_tombstone_log(dir.path()).await; + + // 1. Insert + flush entry E (key 1). Its data block is now persisted on disk. + let e = memory.insert(1, vec![1; 3 * KB]); + enqueue(&store, e); + store.wait().await; + assert_eq!( + store.load(memory.hash(&1)).await.unwrap().kv().unwrap(), + (1, vec![1; 3 * KB]) + ); + + // 2. Delete E. Tombstone T_E is appended at tombstone-log slot 1 (page 0, local 1). + store.delete(memory.hash(&1)); + store.wait().await; + assert_eq!(store.load(memory.hash(&1)).await.unwrap().kv(), None); + + // 3. Append (SLOTS_PER_PAGE - 1) more tombstones for keys without on-disk data + // blocks so the most-recent tombstone moves onto page 1 (logical slot + // SLOTS_PER_PAGE), leaving T_E on page 0. The log has 5 pages, so no wrap. + let extras = TombstoneLog::SLOTS_PER_PAGE - 1; + for i in 100u64..100 + extras as u64 { + store.delete(memory.hash(&i)); + } + store.wait().await; + + // 4. Close + reopen. The recovered tombstones still suppress E here under both + // the buggy and fixed code (T_E is still on disk at this point). + store.close().await.unwrap(); + drop(store); + let store = store_for_test_with_multipage_tombstone_log(dir.path()).await; + assert_eq!(store.load(memory.hash(&1)).await.unwrap().kv(), None); + + // 5. Append one more tombstone for an unrelated hash. Under the bug the writer + // resumes at page 0 local slot 1 (T_E's slot) and silently overwrites T_E. + // Under the fix it resumes on page 1 and T_E survives. + store.delete(memory.hash(&9999u64)); + store.wait().await; + + // 6. Close + reopen again. Under the bug T_E is gone, so E's still-on-disk data + // block is re-indexed by recovery and `load` returns the stale deleted value + // (a phantom entry). Under the fix T_E survives and `load` stays `None`. + store.close().await.unwrap(); + drop(store); + let store = store_for_test_with_multipage_tombstone_log(dir.path()).await; + assert_eq!( + store.load(memory.hash(&1)).await.unwrap().kv(), + None, + "deleted key must not resurface as a phantom entry after tombstone-log reopen" + ); + + store.close().await.unwrap(); + } + // FIXME(MrCroxx): Move the admission test to store level. // #[test_log::test(tokio::test)] // async fn test_store_admission() { diff --git a/foyer-storage/src/engine/block/tombstone.rs b/foyer-storage/src/engine/block/tombstone.rs index 7be936da..5715912a 100644 --- a/foyer-storage/src/engine/block/tombstone.rs +++ b/foyer-storage/src/engine/block/tombstone.rs @@ -69,6 +69,15 @@ impl TombstoneLog { ) -> Result { let mut recovered = vec![]; + // `addr` records the *global* byte offset of the most-recent tombstone seen so + // far. It must accumulate across all partitions and pages (not reset per page), + // otherwise `latest_tombstone_page` below always collapses to `0` and the + // writer resumes on page 0 instead of right after the last global slot, + // silently overwriting older tombstones that may still be needed. + let mut seq = 0; + let mut addr = 0; + let mut global_addr = 0usize; + for partition in partitions.iter() { for offset in (0..partition.size()).step_by(PAGE) { tracing::trace!(offset, "[tombstone log]: recover at"); @@ -76,19 +85,18 @@ impl TombstoneLog { let (buffer, res) = io_engine.read(Box::new(buf), partition.as_ref(), offset as u64).await; res?; - let mut seq = 0; - let mut addr = 0; - - for (slot, buf) in buffer.chunks_exact(Tombstone::SERIALIZED_LEN).enumerate() { + for buf in buffer.chunks_exact(Tombstone::SERIALIZED_LEN) { let tombstone = Tombstone::read(buf); - if tombstone.sequence > seq { - seq = tombstone.sequence; - addr = slot * Tombstone::SERIALIZED_LEN; - } if tombstone.sequence == 0 { + global_addr += Tombstone::SERIALIZED_LEN; continue; } + if tombstone.sequence > seq { + seq = tombstone.sequence; + addr = global_addr; + } recovered.push((tombstone, addr)); + global_addr += Tombstone::SERIALIZED_LEN; } } } @@ -312,4 +320,71 @@ mod tests { assert_eq!(inner.buffer.page, page); } } + + /// Regression test for the cross-page resume bug: after a ring wrap, the most-recent + /// tombstone lives on a non-zero page, so the writer must resume at its *global* slot + /// rather than its page-local slot. Under the bug, the resume slot always collapses to + /// a page-local value and the next append overwrites an older tombstone on page 0. + #[test_log::test(tokio::test)] + async fn test_tombstone_log_resume_after_wrap() { + let dir = tempdir().unwrap(); + + // 4 MB cache device => 16 KB tombstone log => 1K tombstones => 16K partition => 4 pages. + let device = FsDeviceBuilder::new(dir.path()) + .with_capacity(4 * 1024 * 1024 + 16 * 1024) + .build() + .unwrap(); + let p0 = device.create_partition(8 * 1024).unwrap(); + let p1 = device.create_partition(8 * 1024).unwrap(); + let io_engine = PsyncIoEngineConfig::new() + .boxed() + .build(IoEngineBuildContext { + spawner: Spawner::current(), + }) + .await + .unwrap(); + + // 4 pages * 256 slots/page = 1024 slots in total. + let total_slots = TombstoneLog::SLOTS_PER_PAGE * 4; + + let log = TombstoneLog::open(vec![p0.clone(), p1.clone()], io_engine.clone(), &mut vec![]) + .await + .unwrap(); + + // Write enough tombstones to wrap the ring exactly once plus 266 more, so the + // most-recent tombstone lands on page 1 at local slot 10 (physical slot 266). + let written = total_slots + 266; // 1290 + log.append( + (0..written) + .map(|i| Tombstone { + hash: i as u64 + 1, + sequence: i as u64 + 1, + }) + .collect_vec() + .iter(), + ) + .await + .unwrap(); + + drop(log); + + let log = TombstoneLog::open(vec![p0.clone(), p1.clone()], io_engine.clone(), &mut vec![]) + .await + .unwrap(); + + { + let inner = log.inner.lock().await; + // Resume must be the global slot right after the most-recent tombstone: + // (last physical slot 266) + 1 = 267. Under the bug this collapses to the + // page-local slot 10 + 1 = 11, i.e. page 0 instead of page 1. + let expected_slot = (written + 1) % total_slots; // 267 + assert_eq!( + inner.slot, expected_slot, + "tombstone log must resume at the global slot after a ring wrap" + ); + let (page, offset) = log.slot_addr(inner.slot); + assert_eq!(page, 1, "resume must land on page 1, not page 0, after wrap"); + assert_eq!(offset, 11 * Tombstone::SERIALIZED_LEN); + } + } } From fa7c84ff860dc5856b295956502ba664dac80b19 Mon Sep 17 00:00:00 2001 From: "detail-app[bot]" Date: Fri, 11 Sep 2026 11:03:52 +0000 Subject: [PATCH 2/2] style: reflow test comments to satisfy rustfmt nightly wrap_comments --- foyer-storage/src/engine/block/engine.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/foyer-storage/src/engine/block/engine.rs b/foyer-storage/src/engine/block/engine.rs index 0cae0832..dec76829 100644 --- a/foyer-storage/src/engine/block/engine.rs +++ b/foyer-storage/src/engine/block/engine.rs @@ -1264,31 +1264,30 @@ mod tests { store.wait().await; assert_eq!(store.load(memory.hash(&1)).await.unwrap().kv(), None); - // 3. Append (SLOTS_PER_PAGE - 1) more tombstones for keys without on-disk data - // blocks so the most-recent tombstone moves onto page 1 (logical slot - // SLOTS_PER_PAGE), leaving T_E on page 0. The log has 5 pages, so no wrap. + // 3. Append (SLOTS_PER_PAGE - 1) more tombstones for keys without on-disk data blocks so the most-recent + // tombstone moves onto page 1 (logical slot SLOTS_PER_PAGE), leaving T_E on page 0. The log has 5 pages, so + // no wrap. let extras = TombstoneLog::SLOTS_PER_PAGE - 1; for i in 100u64..100 + extras as u64 { store.delete(memory.hash(&i)); } store.wait().await; - // 4. Close + reopen. The recovered tombstones still suppress E here under both - // the buggy and fixed code (T_E is still on disk at this point). + // 4. Close + reopen. The recovered tombstones still suppress E here under both the buggy and fixed code (T_E is + // still on disk at this point). store.close().await.unwrap(); drop(store); let store = store_for_test_with_multipage_tombstone_log(dir.path()).await; assert_eq!(store.load(memory.hash(&1)).await.unwrap().kv(), None); - // 5. Append one more tombstone for an unrelated hash. Under the bug the writer - // resumes at page 0 local slot 1 (T_E's slot) and silently overwrites T_E. - // Under the fix it resumes on page 1 and T_E survives. + // 5. Append one more tombstone for an unrelated hash. Under the bug the writer resumes at page 0 local slot 1 + // (T_E's slot) and silently overwrites T_E. Under the fix it resumes on page 1 and T_E survives. store.delete(memory.hash(&9999u64)); store.wait().await; - // 6. Close + reopen again. Under the bug T_E is gone, so E's still-on-disk data - // block is re-indexed by recovery and `load` returns the stale deleted value - // (a phantom entry). Under the fix T_E survives and `load` stays `None`. + // 6. Close + reopen again. Under the bug T_E is gone, so E's still-on-disk data block is re-indexed by recovery + // and `load` returns the stale deleted value (a phantom entry). Under the fix T_E survives and `load` stays + // `None`. store.close().await.unwrap(); drop(store); let store = store_for_test_with_multipage_tombstone_log(dir.path()).await;