Repository navigation
fix(storage): compute tombstone log resume slot from global offset - #21
Open
detail-app[bot] wants to merge 2 commits into
Open
detail-app[bot] wants to merge 2 commits into
detail-app[bot] wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Bug
TombstoneLog::opencomputed the writer's resume slot from a per-page-local byte offset. Inside the recovery loop,addrwas assignedslot * SERIALIZED_LEN— always< PAGE— solatest_tombstone_page = addr / PAGEwas always0and the cross-pageelsebranch that converts the offset to a global slot was unreachable dead code.After a reopen where the most-recent tombstone lives on a non-zero page (a multi-page tombstone log with more than
SLOTS_PER_PAGEtombstones appended since the last open), the writer resumed on page 0 instead of right after the last global slot. The next append then overwrote an older, still-needed tombstone on page 0. If that overwritten tombstone was the delete marker for a key whose data block was still on disk (deletes are logical; blocks persist until reclaimed), the next reopen re-indexed the stale block andloadreturned a deleted value — a data-integrity violation (phantom entry).This regressed in the partition-abstraction refactor, which dropped the page-base addend from
addr. The bug only affects deployments withenable_tombstone_log: true(off by default).Fix
foyer-storage/src/engine/block/tombstone.rs: track a global byte offset across all partitions and pages. Aglobal_addraccumulator advances bySERIALIZED_LENfor every slot (empty or not);addr(offset of the running most-recent tombstone) is set toglobal_addrinstead of the page-localslot * SERIALIZED_LEN.seq/addrare hoisted out of the page loop so they track the global most-recent tombstone. The formerly-deadelsebranch now yields the correct global slot, andslot = that + 1resumes immediately after the most-recent tombstone on both first-lap and wrapped-lap positions.Testing
test_tombstone_log_resume_after_wrap(tombstone.rs): writes 1290 tombstones into a 4-page (1024-slot) log so the ring wraps once, reopens, and asserts the resume slot is the global267(mapped to physical page 1) rather than the page-local11(page 0).test_store_tombstone_log_resume_no_phantom_entry(engine.rs): a 5-page tombstone-log end-to-end test that inserts+flushes an entry, deletes it, pushes the most-recent tombstone onto page 1, reopens, appends one more tombstone, reopens again, and assertsloadstaysNone(no phantom deleted value). Uses a newstore_for_test_with_multipage_tombstone_loghelper.11vs267; phantomSome((1, ...))vsNone) and pass under the fix, confirming they genuinely depend on the fix.engine::blocktests pass, includingtest_store_delete_recovery,test_store_destroy_recovery, andtest_tombstone_log(single-page and page-0-most-recent configs where the bug coincidentally didn't manifest).cargo check -p foyer-storage,cargo fmt -- --checkon both files, andcargo clippy -p foyer-storageintroduce no new warnings (a pre-existingload_throttle_switchdead-code warning instore.rsis unrelated to this change).foyer-storagetest suite with--features test_utilspasses (26/26, including the three integration fuzzy tests).--features serdeclippy matrix fails, but this is a pre-existing failure (foyer_common::properties::AgemissingSerialize/Deserializeimpls) unrelated to this fix and reproducible on the clean tree.Automatic Fixes PRs can be configured here.