From 35d7c61d50d7d69da9cba9111149d71c9c6c123d Mon Sep 17 00:00:00 2001 From: Heiko Kopp Date: Fri, 7 Aug 2026 13:09:06 +0200 Subject: [PATCH] fix(inmem): invalidate index store for touched collections on abort 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. --- CHANGELOG.md | 14 ++ .../driver/inmem/InMemTransactionContext.java | 20 +++ .../morphium/driver/inmem/InMemoryDriver.java | 104 +++++++++++- .../inmem/InMemTransactionIsolationTest.java | 158 ++++++++++++++++++ 4 files changed, 295 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9340700d..f7f715ae2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +#### InMemoryDriver: aborted/committed transactions could leave stale `CollectionIndexStore` entries, causing false duplicate-key errors on a provably empty collection +A persistent `CollectionIndexStore` lazily built while a transaction is open is built from +the transaction's private snapshot, i.e. from structurally-cloned document instances rather +than the live ones. Those clones were registered into the store's unique-index buckets same +as any real document. `commitTransaction()` already invalidated the store for every +collection the transaction touched, but `abortTransaction()` did not - so on abort the store +kept referencing the orphaned clones forever, since removal matches only by reference +identity and can never match a clone against the real document it was copied from. Every +later insert under that same unique-index key was then rejected as a duplicate, even after +the live collection had been cleared to zero documents. Both `abortTransaction()` and +`commitTransaction()` now invalidate the index store (and TTL queue) for every collection +whose store was actually built while the transaction was open, not merely the ones it wrote +to, since a read-only indexed query can trigger that same lazy rebuild without ever writing. + #### Driver: failover read path could throw a raw NPE past every retry; stale `getLastConnectFailure()` after recovery The read-preference fallback chain read the volatile `primaryNode` field multiple times; the heartbeat nulls that field on stepdown or connection error - exactly while the fallback code diff --git a/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java b/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java index 1cccdb3f1..fa9b708a2 100644 --- a/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java +++ b/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java @@ -25,6 +25,22 @@ public class InMemTransactionContext implements MorphiumTransactionContext { */ private final Set touchedCollections = ConcurrentHashMap.newKeySet(); + /** + * Keys ({@code db + "/" + collection}) of every collection whose persistent + * {@link CollectionIndexStore} was actually BUILT (not merely reused) while this + * transaction was active - a strict superset of {@link #touchedCollections}. A read-only + * indexed query (see {@code InMemoryDriver#getDataFromIndex}) can lazily build that store + * from {@code getCollection()}, which resolves against this transaction's private snapshot + * while one is active - i.e. against structurally-cloned document instances, not the live + * ones - without ever writing to the collection and therefore without ever calling + * {@code markCollectionTouched}. A plain reuse of an already-built store can never + * introduce clones (see {@code InMemoryDriver#getIndexStore}), so only builds are recorded + * here. On BOTH commit and abort, every collection recorded here (not just the written + * ones) must have its store invalidated, or a store lazily built from this transaction's + * clones could keep referencing them after the transaction ends. + */ + private final Set indexStoreAccessedCollections = ConcurrentHashMap.newKeySet(); + public Map getDatabase() { return database; } @@ -37,6 +53,10 @@ public Set getTouchedCollections() { return touchedCollections; } + public Set getIndexStoreAccessedCollections() { + return indexStoreAccessedCollections; + } + @Override public Long getTxnNumber() { return null; diff --git a/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java b/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java index f847294e3..e1102043a 100644 --- a/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java +++ b/morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java @@ -5971,7 +5971,26 @@ private static Object applyElemMatchProjection(String field, Object arrayVal, Ma } CollectionIndexStore built = buildIndexStore(db, collection); CollectionIndexStore prev = indexStoreByCollection.putIfAbsent(key, built); - return prev != null ? prev : built; + if (prev != null) { + return prev; + } + // Record that this collection's persistent index store was actually BUILT (not merely + // reused) while a transaction is open - see + // InMemTransactionContext#getIndexStoreAccessedCollections. Only a build reads via + // getCollection(), which resolves against this transaction's private (cloned) snapshot + // while one is active - i.e. against structurally-cloned document instances, not the + // live ones - so only a build can seed the store with clones that must not outlive the + // transaction. A plain reuse of an already-built store can never introduce clones: the + // store already existed before this call (built either outside any transaction or by an + // earlier one that has since been invalidated on commit/abort), so it holds only + // references that were valid at the time it was built. Write paths are covered + // separately and unconditionally by markCollectionTouched before their first store + // mutation, so they need no recording here even though they also call this method. + InMemTransactionContext ctx = currentTransaction.get(); + if (ctx != null) { + ctx.getIndexStoreAccessedCollections().add(db + "/" + collection); + } + return built; } private CollectionIndexStore buildIndexStore(String db, String collection) throws MorphiumDriverException { @@ -10591,10 +10610,93 @@ public void commitTransaction() { lock.writeLock().unlock(); } } + + // A read-only indexed query can lazily build a collection's persistent index store from + // THIS transaction's cloned snapshot (see getIndexStore) without ever writing to that + // collection, so it never appears in touchedCollections. That store must still be + // invalidated here - it may reference clone instances that must not outlive the + // transaction - even though there is no document list to merge back for it. + for (String key : ctx.getIndexStoreAccessedCollections()) { + if (ctx.getTouchedCollections().contains(key)) { + continue; // already invalidated above + } + invalidateIndexStoreForKey(key); + } } + /** + * Splits a {@code "db/collection"} key (as recorded in + * {@link InMemTransactionContext#getIndexStoreAccessedCollections}), takes that collection's + * write lock, and invalidates its persistent {@link CollectionIndexStore} and TTL expiry + * queue. Shared by {@link #commitTransaction}'s and {@link #abortTransaction}'s handling of + * index-store-accessed-but-not-written collections. Deliberately NOT used by + * {@code commitTransaction}'s {@code touchedCollections} loop above, which runs inside a + * lock already held for the document-list merge and needs that additional merge logic + * alongside the invalidation - folding it into this helper would change its semantics. + */ + private void invalidateIndexStoreForKey(String key) { + int sep = key.indexOf('/'); + String dbName = key.substring(0, sep); + String collName = key.substring(sep + 1); + java.util.concurrent.locks.ReadWriteLock lock = getCollectionLock(dbName, collName); + lock.writeLock().lock(); + try { + invalidateIndexStore(dbName, collName); + invalidateTtlQueue(dbName, collName); + } finally { + lock.writeLock().unlock(); + } + } + + /** + * Aborts the currently active in-memory transaction, discarding its private document + * snapshot. Every collection whose persistent {@link CollectionIndexStore} was actually + * built (not merely reused) while this transaction was open - not merely the ones it wrote + * to - must have that store invalidated here, mirroring {@link #commitTransaction}'s + * equivalent invalidation. + * + *

A store built (lazily, on first {@link #getIndexStore} access) WHILE the transaction was + * open is built from {@link #getCollection}, which resolves against the transaction's + * snapshot while one is active (see {@link #getDB}) - i.e. against structurally-cloned + * document instances ({@link #deepCloneDatabase} deep-copies every document). Those clone + * instances get registered into the store's unique-index buckets via + * {@link CollectionIndexStore#addIndex}/{@code onInsert}. This happens for a WRITE (insert, + * update, delete - all of which call {@link #markCollectionTouched}) but just as easily for a + * purely READ-ONLY indexed query ({@code getDataFromIndex}), which never touches + * {@code markCollectionTouched} at all - see + * {@link InMemTransactionContext#getIndexStoreAccessedCollections} for why that set, not + * {@link InMemTransactionContext#getTouchedCollections}, is the correct one to invalidate + * against here. + * + *

On abort, the snapshot itself is simply dropped - but the *store* is a single object + * shared across the live database and every transaction (keyed only by "db.collection", see + * {@link #indexStoreByCollection}), so without an explicit invalidation here it keeps + * referencing those now-orphaned clone instances. The real live documents that were never + * part of this aborted transaction (or that a subsequent commit/clear removed) then can never + * be found by {@link CollectionIndexStore.IndexEntry#remove}, which matches by reference + * identity - the clone is a different object from the live document, so removal silently + * no-ops and the bucket keeps "existing" forever. Every later duplicate-key check against + * that key then fails, even after the real live collection has been cleared to zero + * documents - see the bug this fixes: a unique-index key rejected a totally fresh insert, + * because onInsert() found a bucket seeded from a clone that outlived its aborted + * transaction. + * + *

This bounds the damage rather than eliminating every related race: it guarantees a + * clone can no longer outlive the transaction that created it. A narrower, pre-existing race + * remains out of scope - while a transaction is still OPEN (before commit or abort), a + * concurrent non-transactional thread that deletes and then re-inserts a live document under + * the same unique key can still collide with the transaction's clone and see a false + * duplicate. That race is not introduced by this fix and is not addressed here. + */ public void abortTransaction() { + InMemTransactionContext ctx = currentTransaction.get(); currentTransaction.set(null); + if (ctx == null) { + return; + } + for (String key : ctx.getIndexStoreAccessedCollections()) { + invalidateIndexStoreForKey(key); + } } public void setTransactionContext(MorphiumTransactionContext ctx) { diff --git a/morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java b/morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java index 3d2004097..bfcba1e51 100644 --- a/morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java +++ b/morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java @@ -11,6 +11,7 @@ import java.util.List; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -207,4 +208,161 @@ void transactionRemainsAbortableAfterRejectedDropDatabase() throws Exception { drv.shutdown(true); } } + + /** + * Regression test for the bug fixed alongside {@code abortTransaction}: a persistent + * {@link de.caluga.morphium.driver.inmem.CollectionIndexStore} lazily built WHILE a + * transaction is open is built from the transaction's private snapshot - i.e. from + * structurally-cloned document instances, not the live documents. If that transaction then + * aborts without invalidating the store, the store keeps registering those orphaned clones + * under their unique-index key forever (removal only matches by reference identity, so the + * clone can never be found and evicted by any later {@code onRemove}/{@code clearCollection} + * against the real live documents). Every subsequent insert of a brand-new, never-before-seen + * document under that same key is then rejected as a duplicate, even though the live + * collection is provably empty. + * + *

This is exactly the failure this test drives directly at the driver level, without + * needing to touch a real MongoDB or start a real multi-document transaction: create a + * unique index, insert a document, then force a duplicate-key insert to fail INSIDE a + * transaction (which lazily builds the persistent index store from the transaction's + * snapshot for the first time), abort, clear the collection down to zero documents, and + * finally insert a fresh document under the very same key. Before the fix, the last insert + * fails with a duplicate-key error against an empty collection; after the fix, it succeeds. + */ + @Test + void abortedTransactionDoesNotLeakStaleIndexEntriesIntoLaterInserts() throws Exception { + InMemoryDriver drv = new InMemoryDriver(); + drv.connect(); + try { + drv.createIndex("testdb", "uniqcoll", Doc.of("k", 1), Doc.of("name", "k_1", "unique", true)); + + // Insert the first, real document INSIDE a transaction that COMMITS. The commit is + // essential: commitTransaction() invalidates the persistent index store for every + // collection the transaction touched (existing, correct behaviour) - so after this, + // the store for "uniqcoll" no longer exists and the NEXT access must rebuild it. + drv.startTransaction(false); + drv.store("testdb", "uniqcoll", List.of(Doc.of("_id", 1, "k", "SB01")), null); + drv.commitTransaction(); + + // Open a SECOND transaction and attempt to insert a duplicate under the same key. + // Handling the unique-index check forces getIndexStore() to lazily rebuild the + // (invalidated) persistent store for the first time since the commit above - and it + // builds that rebuild from getCollection(), which resolves against THIS transaction's + // private snapshot while it is open (see InMemoryDriver#getDB). The snapshot's copy of + // the already-committed SB01 document is a structural CLONE + // ({@link InMemoryDriver#deepCloneDatabase}), not the same object reference stored in + // the live database. That clone gets registered into the rebuilt store's unique-index + // bucket for key "SB01". + drv.startTransaction(false); + assertThrows(MorphiumDriverException.class, + () -> drv.store("testdb", "uniqcoll", List.of(Doc.of("_id", 2, "k", "SB01")), null)); + + // Abort - the transaction's own writes are discarded, but the persistent index store + // that was just rebuilt (seeded with the CLONE of the committed SB01) is a single + // object shared across the live database and every transaction. Before the fix, + // nothing invalidates it here, so it survives the abort holding a reference to an + // object that is not the one in the live collection. + drv.abortTransaction(); + + // Clear the collection down to zero documents via delete() with an empty query - + // this is exactly the codepath Morphium.clearCollection(Class) uses in production + // (Morphium#clearCollection -> remove(createQueryFor(cls)) -> + // MorphiumWriterImpl#remove -> DeleteMongoCommand -> InMemoryDriver#delete), NOT the + // dedicated ClearCollectionCommand (which already correctly invalidates the index + // store itself and would mask this bug). The real, live SB01 document is deleted + // here via reference-identity removal from the index store. It matches and is + // removed correctly, because it was inserted through the FIRST (committed) + // transaction as itself, never as a clone. + drv.delete("testdb", "uniqcoll", Doc.of(), null, true, null, null); + assertEquals(0, drv.find("testdb", "uniqcoll", Doc.of(), null, null, 0, 0).size(), + "collection must be empty after clear"); + + // Before this fix, a lookup ON THE INDEXED FIELD (not just a full-scan query) would + // return the orphaned clone as a phantom document, since the stale index bucket + // still "finds" it even though the live collection is empty - arguably the worse + // symptom, since it surfaces through the exact codepath the index exists to serve. + assertEquals(0, drv.find("testdb", "uniqcoll", Doc.of("k", "SB01"), null, null, 0, 0).size(), + "indexed lookup on the unique-index field must not return the orphaned clone " + + "as a phantom document"); + + // A completely fresh insert under the SAME key, against a provably empty collection, + // must succeed. Before the fix this throws a duplicate-key error against the orphaned + // clone that was seeded into the store during the second (aborted) transaction's + // rebuild and never evicted, because reference-identity removal can never match a + // clone against the real object it was copied from. + assertDoesNotThrow(() -> + drv.store("testdb", "uniqcoll", List.of(Doc.of("_id", 3, "k", "SB01")), null), + "fresh insert under a key that was only ever seen (as a clone) inside an ABORTED " + + "transaction, against a now-empty collection, must not be rejected as a duplicate"); + assertEquals(1, drv.find("testdb", "uniqcoll", Doc.of(), null, null, 0, 0).size()); + } finally { + drv.shutdown(true); + } + } + + /** + * Regression test for the gap in the initial version of the {@code abortTransaction} fix + * above: it only invalidated collections in + * {@link de.caluga.morphium.driver.inmem.InMemTransactionContext#getTouchedCollections} + * (collections the transaction WROTE to). A purely READ-ONLY transaction can just as easily + * cause the persistent {@link de.caluga.morphium.driver.inmem.CollectionIndexStore} to be + * lazily rebuilt from the transaction's cloned snapshot (any {@code find()} call reaches + * {@code getIndexStore()} via {@code getDataFromIndex()}, regardless of whether an index plan + * is ultimately used), without ever calling {@code markCollectionTouched} - so the write-only + * {@code touchedCollections} set never records it, and the original fix silently skipped + * invalidating it on abort. + * + *

This test drives exactly that: commit a document so the store starts fresh-buildable, + * then open a SECOND transaction that only ever calls {@code find()} (never a write) before + * aborting for an unrelated reason, then verify a later insert under the same key - against a + * now-empty collection - is not rejected as a duplicate. + */ + @Test + void abortedReadOnlyTransactionDoesNotLeakStaleIndexEntriesEither() throws Exception { + InMemoryDriver drv = new InMemoryDriver(); + drv.connect(); + try { + drv.createIndex("testdb", "uniqcoll", Doc.of("k", 1), Doc.of("name", "k_1", "unique", true)); + + drv.startTransaction(false); + drv.store("testdb", "uniqcoll", List.of(Doc.of("_id", 1, "k", "SB01")), null); + drv.commitTransaction(); + + // Second transaction: READ ONLY. This find() call forces getIndexStore() to lazily + // rebuild the (invalidated-by-commit) persistent store for the first time since the + // commit above, from getCollection() resolving against THIS transaction's private + // snapshot - i.e. from a structurally-cloned copy of the committed SB01 document. + // markCollectionTouched is never called anywhere on this path. + drv.startTransaction(false); + assertEquals(1, drv.find("testdb", "uniqcoll", Doc.of(), null, null, 0, 0).size()); + + // Abort for an unrelated reason - no write ever happened in this transaction, so + // "uniqcoll" is absent from getTouchedCollections(), but its index store was still + // rebuilt from a clone while this transaction's snapshot was live. + drv.abortTransaction(); + + // Clear via the same production codepath as before, then insert fresh under the + // same key against a provably empty collection. + drv.delete("testdb", "uniqcoll", Doc.of(), null, true, null, null); + assertEquals(0, drv.find("testdb", "uniqcoll", Doc.of(), null, null, 0, 0).size(), + "collection must be empty after clear"); + + // Before this fix, a lookup ON THE INDEXED FIELD (not just a full-scan query) would + // return the orphaned clone as a phantom document, since the stale index bucket + // still "finds" it even though the live collection is empty - arguably the worse + // symptom, since it surfaces through the exact codepath the index exists to serve. + assertEquals(0, drv.find("testdb", "uniqcoll", Doc.of("k", "SB01"), null, null, 0, 0).size(), + "indexed lookup on the unique-index field must not return the orphaned clone " + + "as a phantom document"); + + assertDoesNotThrow(() -> + drv.store("testdb", "uniqcoll", List.of(Doc.of("_id", 3, "k", "SB01")), null), + "fresh insert under a key that was only ever seen (as a clone, via a read-only " + + "find()) inside an ABORTED transaction, against a now-empty collection, must " + + "not be rejected as a duplicate"); + assertEquals(1, drv.find("testdb", "uniqcoll", Doc.of(), null, null, 0, 0).size()); + } finally { + drv.shutdown(true); + } + } }