Skip to content

fix: no-op on already-cached records in covert_insert_records_to_cache - #38

Open
Sheikh Nasrullah (snash4) wants to merge 1 commit into
microsoft:mainfrom
snash4:covert-panic-fix
Open

fix: no-op on already-cached records in covert_insert_records_to_cache#38
Sheikh Nasrullah (snash4) wants to merge 1 commit into
microsoft:mainfrom
snash4:covert-panic-fix

Conversation

@snash4

Copy link
Copy Markdown

Problem

After loading a CPR snapshot via new_from_cpr_snapshot, inserts panic with:

internal error: entered unreachable code: Base page should not have op type: Cache
covert_insert_records_to_cache (leaf_node.rs:285)
upgrade_to_full_page
LeafEntryXLocked::insert

The CPR sweep captures PageLocation::Full pages (Cache op-types) verbatim into the BASE section of the snapshot file. After recovery those pages load as
PageLocation::Base with Cache records. The next mini-page overflow triggers
upgrade_to_full_pagecovert_insert_records_to_cache, which hits unreachable!() on the existing Cache record.

Fix

Change the Cache | Phantom => unreachable!(...) arm to a no-op.
A record already in the target state needs no conversion — symmetric with
convert_cache_records_to_insert, which already tolerates the equivalent case as a no-op rather than panicking.

Why it is safe

  • Symmetric: the inverse function already treats its already-converted case as a harmless no-op. This makes the forward direction consistent.
  • Semantically correct: the function's job is "mark every record as cache." A record already Cache is already marked; Phantom is the cache-tier form of a delete and likewise already in the intended state.
  • No data change: only op-type tagging during a tier move; keys/values are untouched.

Repro

The bug can be reproduced
https://github.com/snash4/bf-tree/tree/covert-panic-fix

}
OpType::Cache | OpType::Phantom => {
unreachable!("Base page should not have op type: {:?}", op_type);
// Already in cache state — nothing to convert.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already in cache state — e.g. a page restored from a CPR snapshot that was
promoted at snapshot time, then re-promoted by a write after recovery.
Nothing to convert. Mirrors convert_cache_records_to_insert, which already handles the symmetric case as a no-op.

Sign up for freeto 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.

1 participant

@snash4