fix: persist the index dirty marker - #1281
Conversation
markDirty() flips isDirty on the IndexMeta that get() returned and never puts it back, so nothing tells the store the entry changed and the new value is not guaranteed to be written. Both beginIndexing() and endIndexing() go through it, so the marker is unreliable in both directions: an index left half built by a crash can come back reading as clean, and a completed one can stay marked dirty and be rebuilt on the first write after every open. Put the IndexMeta back after flipping the flag.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesIndex dirty marker durability
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized fix that persists index dirty-state changes and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hi! We ran into this while tracking down why some of our users' databases rebuilt their
indexes on every start, and it looks like a real bug rather than something intentional.
markDirty()reads theIndexMetaout of the index meta map, flipsisDirtyon it, andstops there:
Nothing puts the instance back, so the store is never told the entry changed and the new
value is not guaranteed to be written. Both
beginIndexing()andendIndexing()gothrough this method, so neither direction of the marker is reliably durable.
That bites in two ways:
truenever made it to disk. Nothing rebuilds it and queries quietly use an incompleteindex.
truedid reach disk through anunrelated commit of the same page and the following
falsedid not. Then every openrebuilds the index again on the first write. This is the one we actually hit.
The fix is to put the meta back after flipping the flag. It is crash-recovery state, so
both directions have to survive a restart.
Test
IndexDirtyMarkerDurabilityTestin the MVStore adapter drivesbeginIndexing()/endIndexing()directly, closes the database and reopens it, then asserts what the markerreads as. Sitting in
org.dizitart.no2.collection.operationso it can reach thepackage-private methods. The "started, then restarted" case fails on current main and
passes with the fix; the "completed" case passes either way and is there to pin the
complementary expectation.
Happy to move the test somewhere else if you would rather not have a test in that package.
Summary by CodeRabbit
Bug Fixes
Tests