diff --git a/CHANGELOG.md b/CHANGELOG.md index e2e759947..74199d7e0 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. + #### PoppyDB: a re-syncing secondary broadcast its own initial-sync wipe as change-stream drop events, letting stale watchers destroy `admin.system.users` cluster-wide during a stepdown The initial sync's `clearLocalDatabases()` wipe and snapshot copy ran as regular commands and therefore emitted live change-stream events on the syncing node - including 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 2cc54ef24..61eecf579 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 @@ -6000,7 +6000,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 { @@ -10626,10 +10645,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); + } + } }