Fix page over-release in CountRange - #26
Merged
Merged
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.
Problem
In
TreeWalker.CountRange/CountRangeAsync, the sibling-walk loop reassignspageto the newly acquired right sibling inside thetryblock, while thefinallyreleasespage. So each iteration releases the page it just acquired instead of the page it just finished counting:Any later traversal that touches that dead entry spins forever:
TryGetkeeps failing (refcount is 0) andLoadkeeps no-op'ing (the dead entry still occupies the map slot). Since the buffer went back toMemoryPoolwhile still reachable, data corruption is also possible.A single
CountRangecall succeeds — the dead entry is only observed by the next traversal — which is why existing tests never caught it. It surfaced as an infinite loop when runningCountRangein a benchmark loop.The start-position retry loop also leaked one reference per cache miss (
TryGetintopage, which the subsequentTrySearchretry overwrites).Fix
Snapshot the owned reference per iteration (
var currentPage = page) and release that, matching the existing pattern inGetRangeAscending. The retry loop now just callsLoad, also matchingGetRangeAscending.Test
Added a regression test that runs
CountRangerepeatedly over a multi-page range and then reads a key on the range's last page, guarded by a 10s timeout. Before the fix it deadlocks; after the fix the whole suite passes.🤖 Generated with Claude Code