fix(inmem): invalidate index store for touched collections on abort - #270
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an in-memory transaction isolation bug where a persistent CollectionIndexStore could be (re)built from a transaction’s cloned snapshot and then survive an abort/commit path without invalidation, leaving stale (clone-identity) entries that later cause false duplicate-key failures against an otherwise-empty live collection.
Changes:
- Track all collections whose index store is accessed during a transaction via
InMemTransactionContext#indexStoreAccessedCollections, including read-only index-store rebuild paths. - Invalidate
CollectionIndexStore(and TTL queue) for those accessed collections on bothcommitTransaction()andabortTransaction(), not just for write-touched collections. - Add two regression tests covering both the write-path and read-only-path reproductions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java | Adds a transaction-scoped set to track collections whose index store was accessed while the transaction was active. |
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java | Records index-store access during a transaction and invalidates index store + TTL queue for accessed collections on commit/abort. |
| morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java | Adds regression tests ensuring aborted transactions (write and read-only) do not leak stale index entries into later inserts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
|
Hi Heiko, reviewed this closely (Opus, full read of Two things I'd like addressed before merge:
Smaller, non-blocking:
Happy to take another look once the recording scope and CHANGELOG are in. |
commitTransaction() correctly invalidates the persistent CollectionIndexStore for every collection the transaction touched before merging the snapshot back into the live database. abortTransaction() never did the equivalent, even though a store can be lazily rebuilt WHILE a transaction is open: buildIndexStore() reads via getCollection(), which resolves against the transaction's private snapshot while one is active (see getDB()). That snapshot's documents are structural clones (deepCloneDatabase() deep-copies every document), not the same object references stored in the live database. Those clone instances get registered into the store's unique-index buckets. On abort, the snapshot itself is discarded, but the store is a single object shared across the live database and every transaction (keyed only by db+collection). Without invalidating it, it keeps referencing the orphaned clones forever: CollectionIndexStore's IndexEntry.remove() matches only by reference identity, so no later onRemove/clearCollection against the REAL live documents can ever find and evict the clone. Every subsequent insert under that same key is then rejected as a duplicate, even after the live collection has been cleared to zero documents. Root cause found while debugging 14 real Quarkus integration test failures in a downstream project that had nothing to do with their own code: a duplicate-key error surfaced against a collection an @beforeeach had already provably cleared to zero documents. Reproduced at the driver level with a minimal transaction sequence: insert+commit, then a second transaction that lazily rebuilds the invalidated store from its own snapshot while failing a duplicate-key check, then abort, then clear via delete() (the codepath Morphium.clearCollection(Class) actually uses in production, not the dedicated ClearCollectionCommand, which already invalidates the store itself and would mask this bug), then a fresh insert under the same key - which failed before this fix and succeeds after it. Extended per review feedback on the first version of this fix: that version only invalidated collections in getTouchedCollections() (write-touched), but getIndexStore() can just as easily be reached by a purely READ-ONLY indexed query (getDataFromIndex(), called unconditionally by every find()) while a transaction is open, without ever calling markCollectionTouched. Introduced a separate, strictly broader InMemTransactionContext#indexStoreAccessedCollections set, populated by getIndexStore() itself, and invalidated by BOTH commitTransaction() and abortTransaction() for every collection recorded there - not just the written ones. Added two regression tests to InMemTransactionIsolationTest: - abortedTransactionDoesNotLeakStaleIndexEntriesIntoLaterInserts (the original write-path reproduction) - abortedReadOnlyTransactionDoesNotLeakStaleIndexEntriesEither (the read-only-transaction gap, uses only find() before abort) Both verified red without their respective fix (exact E11000 duplicate-key error against an empty collection) and green with it. Full InMemTransactionIsolationTest suite (8 tests) and the complete inmemory-tagged test group (843 tests) stay green. Maintainer review follow-up (sboesebeck, PR #270): - Narrowed the recording scope in getIndexStore() from every access to only an actual build (gated on putIfAbsent returning null): a plain reuse of an already-built store can never introduce clones, since the store already existed before this call and holds only references that were valid at the time it was built. Only a build reads via getCollection() against the transaction's cloned snapshot. Write paths remain covered separately by markCollectionTouched. This avoids discarding the index store and TTL queue on every read-only access to a collection, which was a real regression for request-scoped transactions. - Added a CHANGELOG entry under [Unreleased] -> Fixed for this bugfix, matching the file's existing style. - Extracted invalidateIndexStoreForKey(String) as a private helper in InMemoryDriver to de-duplicate the split-lock-invalidate logic shared by commitTransaction()'s new loop and abortTransaction()'s loop. commitTransaction()'s existing finally block keeps its distinct merge semantics and is not changed to use it. - Generalized the abortTransaction() javadoc and the second regression test's javadoc to remove a customer-specific field name/value and a specific AI-reviewer mention, keeping the root-cause explanation itself unchanged. - Softened the abortTransaction() javadoc: the fix bounds the damage (a clone can no longer outlive its transaction) rather than implying clones can never outlive a transaction, and explicitly calls out the narrower pre-existing race that remains out of scope (a concurrent non-transactional delete-then-reinsert under the same unique key while the transaction is still open). - Added an extra indexed-lookup assertion to both regression tests, right before the final insert: a find() on the unique-index field must return zero results against the cleared collection. Before the fix this would have returned the orphaned clone as a phantom document - the worse symptom, since it surfaces through the exact codepath the index exists to serve, not just a full-scan query. Verified via mutation testing that both new assertions turn red without the fix.
Bardioc1977
force-pushed
the
fix/inmem-abort-transaction-index-store-leak
branch
from
August 7, 2026 12:37
101973e to
35d7c61
Compare
Collaborator
Author
|
Thanks for the close read - the recording-scope objection was right, and for the reason you gave: a plain reuse cannot introduce clones. The store already existed before the call, so it only ever holds references that were valid when it was built; only an actual build reads via 1. Recording scope - moved into the build branch, gated on 2. CHANGELOG - 3. Duplicated invalidation - extracted 4. Names - 5. Indexed assertion - added to both tests before the final insert: an indexed 6. Javadoc strength - reworded to "bounds the damage rather than eliminating every related race", and the narrower pre-existing race you describe (concurrent non-transactional delete-then-reinsert against a still-open transaction's clone) is now named explicitly as out of scope and not introduced here. Verification (re-run and reproduced independently of the change itself, not just taken from the edit):
On the follow-ups: happy to fold the still-open-transaction race and the same identity-based staleness pattern in |
…ansaction-index-store-leak # Conflicts: # CHANGELOG.md
Owner
|
Hi Heiko, thanks for the thorough follow-up — every point addressed cleanly, and the indexed-lookup assertion turning out to be the strictly stronger check (failing before the insert even runs) is a nice catch on top. The branch had drifted behind Merging now. |
Bardioc1977
pushed a commit
to Bardioc1977/morphium
that referenced
this pull request
Aug 7, 2026
…transaction A CollectionIndexStore built before a transaction starts stayed stale for the whole transaction. PR sboesebeck#270 already invalidates a store built DURING a transaction, on commit and abort. It never covered the far more common case: a store built BEFORE one even started, since most collections already have a store from earlier reads or writes by the time a transaction opens. Such a pre-existing store was built by reading through the live database, so it holds live document instances. startTransaction() then clones the database for that transaction's writes to mutate in place, without ever telling this pre-existing store. An index-backed read inside the transaction (an equality lookup on a secondary index) kept returning the pre-transaction live instance, diverging from a full scan of the same collection, which does read through the transaction's snapshot. Worse, an update whose candidate came from that stale index-backed lookup mutated the live object instead of the snapshot clone the commit actually merges back, silently losing the write after a commit that reported success. getIndexStore() now records which transaction context, if any, each persistent store was built from, and inside a transaction reuses only a store that same transaction built. Anything else - a store built with no transaction active (live references) or by a different transaction (someone else's clones) - is treated as a cache miss and rebuilt from this transaction's own snapshot on first access. Keyed by context identity rather than by build order on purpose. currentTransaction is thread-local, so transactions genuinely overlap across threads, each with its own clone (see InMemTransactionIsolationTest). Ordering cannot distinguish "built later, for me" from "built later, for someone else": a transaction that accepted a concurrently open transaction's store would run its index-backed lookups against the other transaction's clones, so its update would land in the wrong snapshot - lost on its own commit and corrupting the other transaction's on the way. Rebuilding lazily on first access, rather than discarding every collection's store at transaction start, keeps the cost proportional to what a transaction actually touches. That matters for the request-scoped, many-short-transactions profile this driver mostly serves, and avoids the "too broad a wipe" objection already raised against a similar idea in sboesebeck#270's review. A transaction that never touches a collection's store pays one reference comparison. Making the store itself transaction-aware (its own store per transaction, layered over the live one) was the other option considered and rejected: it would touch all 11 call sites of getIndexStore() for a correctness class that the identity check settles. Checked cappedOnInsert/cappedOnRemove for the same root cause, as asked: same class of bug, arguably worse. cappedDocSizesByCollection is an IdentityHashMap keyed by document reference, and neither commitTransaction nor abortTransaction touches it or its companion byte counter at all - not just for stores built before a transaction, but unconditionally. Reporting this rather than fixing it here; it is unrelated in scope to this fix and belongs with the sboesebeck#269 follow-up. Two regression tests. The first reproduces both symptoms of the pre-existing-store case from a single-field unique index: the read-side divergence between an index-backed lookup and a full scan while the transaction is still open, and the update loss after commit. The second covers the overlapping-transactions case, holding the second transaction open across the first one's index-backed read and update - it has to stay open, since committing or aborting it would invalidate the shared store and hide the confusion under test. Both mutation-proofed. Restoring unconditional reuse reddens the first test with the reported divergence; accepting any transaction's store rather than only this transaction's reddens the second with transaction A reading transaction B's uncommitted write ("expected: <created> but was: <from-b>"). Reverting restores the file byte-for-byte, verified by md5. Verified: full "inmemory" test group 845/845 passing, 0 failures, 0 errors, 7 skipped (pre-existing, unrelated). InMemTransactionIsolationTest's 8 concurrency tests stay green, i.e. the identity check does not disturb existing transaction isolation.
Bardioc1977
pushed a commit
to Bardioc1977/morphium
that referenced
this pull request
Aug 7, 2026
A CollectionIndexStore built before a transaction starts stayed stale for the whole transaction. PR sboesebeck#270 already invalidates a store built DURING a transaction, on commit and abort. It never covered the far more common case: a store built BEFORE one even started, since most collections already have a store from earlier reads or writes by the time a transaction opens. Such a pre-existing store was built by reading through the live database, so it holds live document instances. startTransaction() then clones the database for that transaction's writes to mutate in place, without ever telling this pre-existing store. An index-backed read inside the transaction (an equality lookup on a secondary index) kept returning the pre-transaction live instance, diverging from a full scan of the same collection, which does read through the transaction's snapshot. Worse, an update whose candidate came from that stale index-backed lookup mutated the live object instead of the snapshot clone the commit actually merges back, silently losing the write after a commit that reported success. The underlying problem is that one global indexStoreByCollection has to serve both live documents and per-transaction clones. getIndexStore() now records which transaction context, if any, each store was built from, and hands a store back only to the caller whose data it was built from: - built with no transaction active -> only for non-transactional callers - built by transaction T -> only for T itself Anything else is treated as a cache miss and rebuilt from the caller's own view. Both directions matter, and both are covered by tests: - a transaction must not reuse another transaction's store, or its index-backed update lands in the wrong snapshot - lost on its own commit, corrupting the other's on the way; - a non-transactional reader must not reuse a store built by a still-open transaction, or it observes uncommitted writes. Keyed by context identity, not by build order, on purpose. currentTransaction is thread-local, so transactions genuinely overlap across threads (see InMemTransactionIsolationTest) and ordering cannot tell "built later, for me" from "built later, for someone else". Rebuilding lazily on first access, rather than discarding every collection's store at transaction start, keeps the cost proportional to what a transaction actually touches. That matters for the request-scoped, many-short-transactions profile this driver mostly serves, and avoids the "too broad a wipe" objection already raised against a similar idea in sboesebeck#270's review. A transaction that never touches a collection's store pays one reference comparison. Making the store itself transaction-aware (its own store per transaction, layered over the live one) was the other option considered and rejected: it would touch all 11 call sites of getIndexStore() for a correctness class the identity check settles. The whole-database drop path clears the provenance entries alongside the stores themselves; the single-collection paths already went through invalidateIndexStore, which does both. Checked cappedOnInsert/cappedOnRemove for the same root cause, as asked: same class of bug, arguably worse. cappedDocSizesByCollection is an IdentityHashMap keyed by document reference, and neither commitTransaction nor abortTransaction touches it or its companion byte counter at all - not just for stores built before a transaction, but unconditionally. Reporting this rather than fixing it here; it is unrelated in scope to this fix and belongs with the sboesebeck#269 follow-up. Three regression tests, each mutation-proofed - restoring the old behaviour reddens exactly the corresponding test, and reverting restores the file byte-for-byte (md5-verified): - pre-existing store: read-side divergence between an index-backed lookup and a full scan inside the transaction, and the update loss after commit; - overlapping transactions: transaction A reading B's uncommitted write ("expected: <created> but was: <from-b>"); - non-transactional reader: seeing an open transaction's uncommitted write ("expected: <created> but was: <uncommitted>"). The concurrent tests deliberately hold the second transaction open across the first caller's read and update: committing or aborting it would invalidate the shared store and hide the confusion under test. Verified: full "inmemory" test group 846/846 passing, 0 failures, 0 errors, 7 skipped (pre-existing, unrelated). InMemTransactionIsolationTest's 8 concurrency tests stay green, i.e. the identity check does not disturb existing transaction isolation.
Bardioc1977
pushed a commit
to Bardioc1977/morphium
that referenced
this pull request
Aug 7, 2026
A CollectionIndexStore built before a transaction starts stayed stale for the whole transaction. PR sboesebeck#270 already invalidates a store built DURING a transaction, on commit and abort. It never covered the far more common case: a store built BEFORE one even started, since most collections already have a store from earlier reads or writes by the time a transaction opens. Such a pre-existing store was built by reading through the live database, so it holds live document instances. startTransaction() then clones the database for that transaction's writes to mutate in place, without ever telling this pre-existing store. An index-backed read inside the transaction (an equality lookup on a secondary index) kept returning the pre-transaction live instance, diverging from a full scan of the same collection, which does read through the transaction's snapshot. Worse, an update whose candidate came from that stale index-backed lookup mutated the live object instead of the snapshot clone the commit actually merges back, silently losing the write after a commit that reported success. The underlying problem is that one global indexStoreByCollection has to serve both live documents and per-transaction clones. Each cache entry now carries the data provenance it was built from - a specific InMemTransactionContext, or NO_TRANSACTION for the live database - and getIndexStore() hands a store back only to the caller whose data it matches: - built with no transaction active -> only for non-transactional callers - built by transaction T -> only for T itself Anything else is treated as a cache miss and rebuilt from the caller's own view. Both directions matter, and both are covered by tests: - a transaction must not reuse another transaction's store, or its index-backed update lands in the wrong snapshot - lost on its own commit, corrupting the other's on the way; - a non-transactional reader must not reuse a store built by a still-open transaction, or it observes uncommitted writes. Store and provenance are one immutable map value rather than two parallel maps, so they are published in a single atomic operation. Kept apart, a concurrent getIndexStore() on another thread could observe a store whose provenance was not written yet, or already overwritten by a third thread, and reuse it for the wrong caller - re-introducing through the bookkeeping the very confusion the check exists to prevent. The putIfAbsent race is handled explicitly for the same reason: when another thread wins, its entry is only returned if its provenance matches, otherwise this caller uses its own build. Building twice is wasteful but never incorrect; returning a foreign snapshot's store never is. Keyed by context identity, not by build order, on purpose. currentTransaction is thread-local, so transactions genuinely overlap across threads (see InMemTransactionIsolationTest) and ordering cannot tell "built later, for me" from "built later, for someone else". Rebuilding lazily on first access, rather than discarding every collection's store at transaction start, keeps the cost proportional to what a transaction actually touches. That matters for the request-scoped, many-short-transactions profile this driver mostly serves, and avoids the "too broad a wipe" objection already raised against a similar idea in sboesebeck#270's review. A transaction that never touches a collection's store pays one reference comparison. Making the store itself transaction-aware (its own store per transaction, layered over the live one) was the other option considered and rejected: it would touch all 11 call sites of getIndexStore() for a correctness class the provenance check settles. Checked cappedOnInsert/cappedOnRemove for the same root cause, as asked: same class of bug, arguably worse. cappedDocSizesByCollection is an IdentityHashMap keyed by document reference, and neither commitTransaction nor abortTransaction touches it or its companion byte counter at all - not just for stores built before a transaction, but unconditionally. Reporting this rather than fixing it here; it is unrelated in scope to this fix and belongs with the sboesebeck#269 follow-up. Three regression tests, each mutation-proofed - restoring the old behaviour reddens exactly the corresponding test, and reverting restores the file byte-for-byte (md5-verified): - pre-existing store: read-side divergence between an index-backed lookup and a full scan inside the transaction, and the update loss after commit; - overlapping transactions: transaction A reading B's uncommitted write ("expected: <created> but was: <from-b>"); - non-transactional reader: seeing an open transaction's uncommitted write ("expected: <created> but was: <uncommitted>"). The concurrent tests deliberately hold the second transaction open across the first caller's read and update: committing or aborting it would invalidate the shared store and hide the confusion under test. Verified: full "inmemory" test group 846/846 passing, 0 failures, 0 errors, 7 skipped (pre-existing, unrelated). InMemTransactionIsolationTest's 8 concurrency tests stay green, i.e. the provenance check does not disturb existing transaction isolation.
Bardioc1977
pushed a commit
to Bardioc1977/morphium
that referenced
this pull request
Aug 7, 2026
A CollectionIndexStore built before a transaction starts stayed stale for the whole transaction. PR sboesebeck#270 already invalidates a store built DURING a transaction, on commit and abort. It never covered the far more common case: a store built BEFORE one even started, since most collections already have a store from earlier reads or writes by the time a transaction opens. Such a pre-existing store was built by reading through the live database, so it holds live document instances. startTransaction() then clones the database for that transaction's writes to mutate in place, without ever telling this pre-existing store. An index-backed read inside the transaction (an equality lookup on a secondary index) kept returning the pre-transaction live instance, diverging from a full scan of the same collection, which does read through the transaction's snapshot. Worse, an update whose candidate came from that stale index-backed lookup mutated the live object instead of the snapshot clone the commit actually merges back, silently losing the write after a commit that reported success. The underlying problem is that one global indexStoreByCollection has to serve both live documents and per-transaction clones. Each cache entry now carries the data provenance it was built from - a specific InMemTransactionContext, or NO_TRANSACTION for the live database - and getIndexStore() hands a store back only to the caller whose data it matches: - built with no transaction active -> only for non-transactional callers - built by transaction T -> only for T itself Anything else is treated as a cache miss and rebuilt from the caller's own view. Both directions matter, and both are covered by tests: - a transaction must not reuse another transaction's store, or its index-backed update lands in the wrong snapshot - lost on its own commit, corrupting the other's on the way; - a non-transactional reader must not reuse a store built by a still-open transaction, or it observes uncommitted writes. Store and provenance are one immutable map value rather than two parallel maps, so they are published in a single atomic operation. Kept apart, a concurrent getIndexStore() on another thread could observe a store whose provenance was not written yet, or already overwritten by a third thread, and reuse it for the wrong caller - re-introducing through the bookkeeping the very confusion the check exists to prevent. The putIfAbsent race is handled explicitly for the same reason: when another thread wins, its entry is only returned if its provenance matches, otherwise this caller uses its own build. Building twice is wasteful but never incorrect; returning a foreign snapshot's store never is. Keyed by context identity, not by build order, on purpose. currentTransaction is thread-local, so transactions genuinely overlap across threads (see InMemTransactionIsolationTest) and ordering cannot tell "built later, for me" from "built later, for someone else". Rebuilding lazily on first access, rather than discarding every collection's store at transaction start, keeps the cost proportional to what a transaction actually touches. That matters for the request-scoped, many-short-transactions profile this driver mostly serves, and avoids the "too broad a wipe" objection already raised against a similar idea in sboesebeck#270's review. A transaction that never touches a collection's store pays one reference comparison. Making the store itself transaction-aware (its own store per transaction, layered over the live one) was the other option considered and rejected: it would touch all 11 call sites of getIndexStore() for a correctness class the provenance check settles. Checked cappedOnInsert/cappedOnRemove for the same root cause, as asked: same class of bug, arguably worse. cappedDocSizesByCollection is an IdentityHashMap keyed by document reference, and neither commitTransaction nor abortTransaction touches it or its companion byte counter at all - not just for stores built before a transaction, but unconditionally. Reporting this rather than fixing it here; it is unrelated in scope to this fix and belongs with the sboesebeck#269 follow-up. Three regression tests, each mutation-proofed - restoring the old behaviour reddens exactly the corresponding test, and reverting restores the file byte-for-byte (md5-verified): - pre-existing store: read-side divergence between an index-backed lookup and a full scan inside the transaction, and the update loss after commit; - overlapping transactions: transaction A reading B's uncommitted write ("expected: <created> but was: <from-b>"); - non-transactional reader: seeing an open transaction's uncommitted write ("expected: <created> but was: <uncommitted>"). The concurrent tests deliberately hold the second transaction open across the first caller's read and update: committing or aborting it would invalidate the shared store and hide the confusion under test. Verified: full "inmemory" test group 846/846 passing, 0 failures, 0 errors, 7 skipped (pre-existing, unrelated). InMemTransactionIsolationTest's 8 concurrency tests stay green, i.e. the provenance check does not disturb existing transaction isolation.
sboesebeck
pushed a commit
that referenced
this pull request
Aug 7, 2026
…#271) Follow-up to #270 in the same InMemoryDriver index-store area, covering the complementary case: a store built BEFORE a transaction starts, which is the common case since most collections already have one by the time a transaction opens. Independently reviewed (own worktree, develop untouched during review): atomicity of the store+provenance value verified across all 6 access sites, the putIfAbsent race path confirmed to behave as described (loser uses its own unpublished build, never a foreign one), both directions (tx vs other tx, tx vs NO_TRANSACTION) confirmed symmetric, no leak/double-bookkeeping with #270's invalidation path. The three new regression tests were mutation-tested (guard removed -> all three fail with the exact claimed messages; InMemTransactionIsolationTest stays green under the same mutation, i.e. they're specific, not incidentally passing). Full inmemory test group re-run independently: 846 tests, 0 failures, 0 errors, 7 skipped - matches the PR description exactly. Four non-blocking notes left as a PR comment (a probably-unnecessary defensive remove() call quantified with numbers, an orphaned-transaction snapshot-pinning edge case worth a javadoc note, a comment line-wrap nit, and a narrative correction on which assertion fires first in test 1) - none of them block merging.
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.
Root cause
commitTransaction()correctly invalidates the persistentCollectionIndexStorefor every collection the transaction touched before merging the snapshot back into the live database.abortTransaction()never did the equivalent, even though a store can be lazily rebuilt WHILE a transaction is open:buildIndexStore()reads viagetCollection(), which resolves against the transaction's private snapshot while one is active (seegetDB()). That snapshot's documents are structural clones (deepCloneDatabase()deep-copies every document), not the same object references stored in the live database.Those clone instances get registered into the store's unique-index buckets. On abort, the snapshot itself is discarded, but the store is a single object shared across the live database and every transaction (keyed only by db+collection). Without invalidating it, it keeps referencing the orphaned clones forever:
CollectionIndexStore'sIndexEntry.remove()matches only by reference identity, so no lateronRemove/clearCollectionagainst the REAL live documents can ever find and evict the clone. Every subsequent insert under that same key is then rejected as a duplicate, even after the live collection has been cleared to zero documents.Root cause found while debugging 14 real Quarkus integration test failures in a downstream project that had nothing to do with their own code: a duplicate-key error surfaced against a collection an
@BeforeEachhad already provably cleared to zero documents.Fix
Extended per review feedback (Codex found a gap in the first version): a purely READ-ONLY indexed query can just as easily cause
getIndexStore()to lazily rebuild a collection's store from a transaction's snapshot, without ever callingmarkCollectionTouched(which only records writes). So instead of invalidating onlygetTouchedCollections()on abort, introduced a separate, strictly broaderInMemTransactionContext#indexStoreAccessedCollectionsset, populated bygetIndexStore()itself on every access (build or reuse, read or write) while a transaction is active, and invalidated by BOTHcommitTransaction()andabortTransaction()for every collection recorded there.Verification
Added two regression tests to
InMemTransactionIsolationTest:abortedTransactionDoesNotLeakStaleIndexEntriesIntoLaterInserts(the write-path reproduction)abortedReadOnlyTransactionDoesNotLeakStaleIndexEntriesEither(the read-only-transaction gap, uses onlyfind()before abort)Both verified red without their respective fix (exact
E11000duplicate-key error against an empty collection) and green with it. FullInMemTransactionIsolationTestsuite (8 tests) and the completeinmemory-tagged test group (843 tests) stay green.Known pre-existing gap noted during review (not fixed here)
Codex flagged that
invalidateTtlQueue()+ttlEnqueue()'scomputeIfAbsentcan silently drop older TTL documents from expiry tracking if an enqueue races between an invalidation and the next sweep - this bug already exists independently of this PR (reachable viacreateIndex/dropIndexes/rename/drop/commit today); this PR's abort-side and read-only-commit invalidation calls just make the race window open more often. Filed as #269 so it doesn't get lost; deliberately not addressed here to keep this diff focused.Review history
Reviewed as Bardioc1977#20 ahead of this upstream PR - Copilot and Codex both reviewed the final diff (3 files) clean.