persistit: revalidate pending index insertions after the tree claim is dropped mid-store (bug 1017957 residual)#275
Merged
vharseko merged 1 commit intoJul 12, 2026
Conversation
…s dropped mid-store Fixes the residual bug-1017957 race from OpenIdentityPlatform#274. When storeInternal has committed a split, the key-pointer pair for the new right sibling is inserted at the next level by a later loop iteration. If that iteration hits a RetryException (tree height growth with a contended claim upgrade), the handler releases the tree claim before re-acquiring it; a concurrent covering removeKeyRange could unlink the just-split page onto a garbage chain inside that window, and the resumed iteration planted the stale pointer -- a dangling index entry to a garbage page. Searches then failed transiently with CorruptVolumeException "invalid page type 30: should be 2" while IntegrityCheck stayed clean, matching the CI failure on OpenIdentityPlatform#274. storeInternal now re-validates the pending pointer under the re-acquired claim and abandons the insertion when the page is gone. Also hardens fixIndexHole against stale CleanupIndexHole actions whose page has been garbage collected and reused as a live indexed page of the same tree (ABA): the repair now runs only when the parent level really has no entry for the page, instead of relying on the separator-equals-first-key invariant to turn the stale store into a same-key replace. Both behaviors are covered deterministically by Bug1017957ResidualTest, driven by a new STORE_PENDING_SPLIT ThreadSequencer schedule; both tests fail with the fixes reverted.
maximthomas
approved these changes
Jul 12, 2026
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.
Fixes #274.
Root cause (verified)
When
Exchange#storeInternalhas committed a split, the key-pointer pair for the new right sibling is inserted at the next level by a later iteration of the store loop. If that iteration throws aRetryException— in non-MVCC use this only happens on tree height growth with a contended claim upgrade — the handler releases the tree claim entirely before re-acquiring it. In that window a concurrent coveringremoveKeyRangecan take the writer claim, unlink the just-split page onto a garbage chain, and retype itPAGE_TYPE_GARBAGE. The resumed iteration then inserted the now-stale pointer blindly, planting a dangling index entry to a garbage page.This matches the CI evidence exactly:
PAGE_TYPE_DATAis 1 andPAGE_TYPE_INDEX_MINis 2, so the observedinvalid page type 30: should be 2means searches were descending through a root entry into a garbage-collected level-1 index page — the page split off the root whose pending root entry was planted after the page had already been unlinked. The volume stays consistent at rest once later removes delete the entry, which is whyIntegrityCheckreported 0 faults. (See the correction comment on #274: two residual-mechanism hypotheses from the original analysis did not survive verification; this one did.)Fix
storeInternaltracks that a committed split's key-pointer pair is still pending when a retry handler releases the tree claim, and re-validates it under the re-acquired claim at the top of the main loop: a search for the pending key at the child level must land on the pending page itself (via the B-link right-walk, since the page has no parent entry yet). If it lands elsewhere, the page — and its keys — are gone, and the deferred insertion is abandoned.fixIndexHoleis additionally hardened against staleCleanupIndexHoleactions whose page has been garbage collected and reused as a live indexed page of the same tree at the same level (ABA): both existing guards pass legitimately against the new incarnation, and the action used to perform a parent-level store that is a harmless same-key replace only while every parent entry key equals its page's first key. The repair now runs only when the parent level really has no entry for the page, dropping stale actions outright.Tests
New
Bug1017957ResidualTestcovers both deterministically (the originalBug1017957Test#induceCorruptionByStressreproduces the race only probabilistically under a 10-second stress):pendingSplitPointerRevalidatedAfterTreeClaimDropuses a newSTORE_PENDING_SPLITThreadSequencer schedule to park the writer inside the retry window with the root split committed, runs a covering remove that garbage-collects the pending page, and asserts no index entry points to it,IntegrityCheckis clean, and a full traversal does not throw. Without the fix it fails withNo index entry may point to the garbage page expected:<0> but was:<1>.staleIndexHoleActionOnReusedPageIsDroppedunlinks a page, drives its reuse through the garbage chain, then performs a staleCleanupIndexHoleagainst it and asserts the action stores nothing (tree store counter unchanged) and the page keeps exactly one parent entry. Without the fix it fails withexpected:<73> but was:<74>.Verified: both new tests fail with the fixes reverted and pass with them;
Bug1017957Teststress passes withTotal errors 0; fullpersistit/coresuite: 569 tests, 0 failures, 0 errors (5 pre-existing skips).