fix: Range/RangeBackwards no longer skip items reordered mid-iteration#213
Open
amitmishra11 wants to merge 1 commit into
Open
fix: Range/RangeBackwards no longer skip items reordered mid-iteration#213amitmishra11 wants to merge 1 commit into
amitmishra11 wants to merge 1 commit into
Conversation
Range and RangeBackwards walked the live LRU list while releasing the read lock between steps, so a concurrent Set/Touch could call MoveToFront and move an unvisited item ahead of the iterator, skipping it. Snapshot the keys in iteration order up front, then re-look-up each key (skipping ones since evicted) instead of walking the live list. Fixes jellydator#205 This PR was written primarily by Claude Code; I reviewed the change and ran the test suite before submitting.
Coverage Report for CI Build 28315180578Coverage increased (+0.001%) to 99.739%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
This PR was written primarily by Claude Code; I reviewed the change and ran the test suite before submitting.
Problem
As described in #205,
Range/RangeBackwardswalk the live LRU list while releasing the read lock between steps. A concurrentSet/Touchcall in that window can callMoveToFront, moving an item the iterator hasn't visited yet ahead of the current position, so it gets skipped entirely.Approach
@swithek noted in the issue thread that fully cloning the cache to avoid this would double its memory footprint, and @d1lmr0d suggested snapshotting keys instead. This PR takes that approach: instead of walking the live list, it takes a snapshot of keys (in current iteration order) up front under the read lock, then looks each key up again as it goes, skipping any that have since been evicted/deleted. This is the same number of lock acquisitions as before, but the iteration order is now fixed by the key snapshot rather than by list links that can move underneath it, so a concurrent reorder can no longer cause skips. The extra cost is one
[]Kslice (just keys, not full items), not a clone of the cache.I'm not certain this is the direction you'd want to take it, given the issue is still open with no agreed approach. Happy to adjust if you'd prefer something else (e.g. documenting the existing semantics instead of changing them, or a different consistency guarantee).
Changes
cache.go: extractedsnapshotKeysUnsafe(collects keys in list order under the read lock) andrangeKeys(re-looks-up each key, skips ones no longer present, callsfnoutside the lock);RangeandRangeBackwardsnow use these instead of walking the live list directly.cache_test.go: addedTest_Cache_Range_VisitsItemsReorderedDuringIteration, reproducing the exact repro from Range() returns inconsistent results due to race condition with Set/Touch #205 (Seton an unvisited key from within theRangecallback). Verified it fails against the current code and passes with this fix.Testing
go test ./...passes (a few timer-based tests —Test_Cache_Get,Test_Cache_OnInsertion,Test_Cache_OnUpdate— are pre-existing flakes unrelated to this change; confirmed by running the full suite against unmodifiedv3as well). Could not run with-racein my sandbox (only a 32-bit MinGW gcc is available, which can't build cgo's race detector), but the bug itself is deterministic single-threaded reentrancy (callingSetfrom within theRangecallback), not a true data race, so the new test exercises it directly without needing-race.Fixes #205