fix: copy the catalog's stored name set instead of mutating it in place - #1296
Conversation
StoreCatalog.writeCollectionEntry (and the repository variants) read the
catalog document, wrapped it in MapMetaData, and added the new name to the
Set instance that MapMetaData had taken straight out of the document. On
MVStore that instance is the one held in the catalog page, and MVStore
serializes dirty pages on a background thread that any write can start
through tryCommit. Creating collections in a burst interleaved with writes,
which is what a data migration does, put the serializer's HashSet.writeObject
and the catalog's HashSet.add on the same set at the same time:
java.util.ConcurrentModificationException
at java.util.HashSet.writeObject
...
at org.h2.mvstore.type.ObjectDataType.serialize
at org.h2.mvstore.FileStore.serializeAndStore
org.h2.mvstore.MVStoreException: Could not serialize {mapNames=[...]}
at org.h2.mvstore.MVStore.panic
after which the store is closed and every later operation throws. Observed
on the first start after a Xodus-to-Nitrite migration that created eleven
collections in forty milliseconds.
MapMetaData now copies the stored set, so a write always puts a new set and
the instance a serializer may be reading is never touched, the same rule
WriteOperations.update already follows by cloning a document before
merging into it.
The test writes two entries and asserts that the set stored after the first
write is not the instance stored after the second and did not gain the
second name.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthrough
ChangesMetadata Copy-on-Write
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Catalog metadata updates now use copy-on-write semantics, preventing writes from mutating a set that may be serialized concurrently. The regression coverage confirms collection names remain intact, with no current merge-blocking risk identified. 🚥 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 |
|
Merged. For the record, the Ubuntu failure here was never this change: which is the same 0/0 chunk-accounting family as the Thanks for this one — "copy, never adopt" is the rule the codebase already followed in |
* fix: port three index and catalog fixes from nitrite-java - A unique index no longer rejects a document over a key that document already holds. addNitriteIds treated any existing id under the key as a violation, so it counted the writer's own id against it: a unique index over an array field with a repeated element (['a', 'b', 'a']) collided with the entry it had just written, and so did an index rebuild or a replayed write. Another document under the key is still a violation. (nitrite/nitrite-java#1295) - An update that leaves an indexed value unchanged no longer rewrites the index. "Affected" only meant the update carried the field, and an upsert that writes the whole document back carries every indexed field with its old value, so every index was rebuilt on every update for nothing. A dirty index is still rebuilt. (nitrite/nitrite-java#1297) - MapMetaData copies the stored name set instead of adopting it. cast<String>() returns a view onto the set held in the catalog document, so mapNames.add() edited the stored set in place, before the write meant to record it. (nitrite/nitrite-java#1296) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: getById hands out a copy, not the stored instance The in-memory store returns the very Document it holds, so a caller's doc.put(...) on a getById result edited the store directly and bypassed every index. find() already copied, through ProcessedDocumentStream. getById now clones as the cursor does, and returns null for an unknown id instead of putting null through the processor chain. (nitrite/nitrite-java#1294) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This exception was seen in production (!)
StoreCatalog.writeCollectionEntry (and the repository variants) read the catalog document, wrapped it in MapMetaData, and added the new name to the Set instance that MapMetaData had taken straight out of the document. On MVStore that instance is the one held in the catalog page, and MVStore serializes dirty pages on a background thread that any write can start through tryCommit. Creating collections in a burst interleaved with writes, which is what a data migration does, put the serializer's HashSet.writeObject and the catalog's HashSet.add on the same set at the same time:
java.util.ConcurrentModificationException
at java.util.HashSet.writeObject
...
at org.h2.mvstore.type.ObjectDataType.serialize
at org.h2.mvstore.FileStore.serializeAndStore
org.h2.mvstore.MVStoreException: Could not serialize {mapNames=[...]}
at org.h2.mvstore.MVStore.panic
after which the store is closed and every later operation throws. Observed on the first start after a Xodus-to-Nitrite migration that created eleven collections in forty milliseconds.
MapMetaData now copies the stored set, so a write always puts a new set and the instance a serializer may be reading is never touched, the same rule WriteOperations.update already follows by cloning a document before merging into it.
The test writes two entries and asserts that the set stored after the first write is not the instance stored after the second and did not gain the second name.
Summary by CodeRabbit