fix(inmem): reuse an index store only for the caller it was built for - #271
Conversation
A CollectionIndexStore built before a transaction starts stayed stale for the whole transaction. PR #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 #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 #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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-loss bug in InMemoryDriver's persistent index-store cache. A CollectionIndexStore built before a transaction opened held live document instances, but startTransaction() clones the database so the transaction's writes mutate a private snapshot instead. That pre-existing store was never invalidated, so index-backed reads inside the transaction returned stale live documents (diverging from a full scan), and an index-backed update could mutate a live object the commit never merged back — silently losing the write. This is a follow-up to #270, which only covered stores built during a transaction.
The fix keys each cache entry to the data provenance it was built from, so a store is only reused by the caller whose view it matches (the specific InMemTransactionContext, or a NO_TRANSACTION sentinel for the live database), rebuilding lazily otherwise. Keying by context identity (not build order) is required because currentTransaction is thread-local and transactions genuinely overlap across threads.
Changes:
- Replace the
Map<String, CollectionIndexStore>cache withMap<String, OwnedIndexStore>, whereOwnedIndexStorebundles the store with its owner provenance in one atomically published value. - Add a provenance check in
getIndexStore()that only reuses a store for its matching owner, evicting and rebuilding on a mismatch, and handling theputIfAbsentrace explicitly. - Add three regression tests (pre-existing store, overlapping transactions, non-transactional reader) plus CHANGELOG and javadoc updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
morphium-core/.../inmem/InMemoryDriver.java |
Introduces OwnedIndexStore/NO_TRANSACTION, provenance-based reuse in getIndexStore(), and updated javadoc. |
morphium-core/.../inmem/InMemTransactionPreExistingIndexStoreStalenessTest.java |
New tests covering pre-existing-store staleness and cross-caller isolation, including two-thread concurrency cases. |
CHANGELOG.md |
Documents the bug and the provenance-based fix. |
I reviewed the provenance-check logic, the putIfAbsent/remove(key, existing) race handling, the single-map atomic publication, the whole-DB drop and invalidation paths (which are key-based and unaffected by the value-type change), and the new tests' happens-before synchronization. I did not find a concrete, objectively demonstrable defect. That said, this is a deeply concurrency-sensitive change to a core driver's transaction/index-store interaction with several interacting thread-local, lock, and cache-eviction paths, so it warrants human review rather than automated approval.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Verified this fix against a downstream application's full backend test suite (Quarkus, tests configured to run against First, proof that the build under test actually contained this PR's change — that project resolves morphium from a bundled repository rather than the local one, so I checked the jar instead of trusting resolution: Tested at Result: the suite is fully green — zero failures, zero errors. Every test that failed before now passes, including the integration tests that read through a unique secondary index inside a transaction, which was the exact pattern that broke. What I found worth reporting is that three fixes were needed in combination, and two of them were masking each other:
The ordering matters: this bug only became observable after #268 was fixed. Before that, the missing unique index meant no secondary index existed at all, so the stale-store path was never taken. Anyone still on an older version may be carrying this silently. It also confirms the point about the remaining race not being purely a concurrency issue — this reproduces single-threaded, with no second thread involved, which makes the softened wording in Happy to run anything else against this build if that would help. |
|
Hi Heiko, reviewed this independently (full read of VerifiedAtomicity. Folding store+owner into The Both directions. Interaction with #270 — no double bookkeeping, and no dead-context entries. The invariant that matters is "every entry whose owner is a context is recorded in that context's Tests. Mutation-proofing checks out; I did not take it on trust. Replacing the guard with
and Non-blocking1. The Without the line, the transaction keeps its incrementally-maintained store and only the mismatching side rebuilds; with it, both sides throw their store away on every single access. Since Secondary point in the same area: removing the entry also creates extra "no entry present" windows, and two 2. The cache now pins a whole database clone, not just a collection's documents. 3. Comment reflow damage at lines 6091-6092: 4. Small narrative correction on test 1. Without the fix, the assertion that actually fires first in On the process note: agreed, and thanks for flagging it rather than quietly banking the wins. I will look at enabling CodeRabbit for the repo — the non-transactional-reader direction is exactly the kind of thing that is easy to miss once you have convinced yourself the first direction is fixed, and it would have been a data-visibility bug in released code. Also noted on Happy to merge as-is; the four points above are all optional. If you want to drop the |
Follow-up to #271, correcting the four points from review - and correcting a regression the first version of this commit introduced. The mismatch branch in getIndexStore() originally evicted the entry before rebuilding. My first attempt simply removed that eviction, which fixed one case and badly broke a more common one: with the entry left in place, a transaction that meets a pre-existing NO_TRANSACTION entry loses putIfAbsent against it on every call, forever. It never publishes its own store, so it pays a full buildIndexStore - O(documents x indexes) - per operation for the whole life of the transaction. The entry now changes owner atomically once the rebuild finishes, via a compare-and-swap keyed on the exact entry this call observed. A same-key swap, never a remove-then-publish, so there is no moment with no entry for the key at all - which is strictly better than the eviction it replaces, because two callers reach getIndexStore() without holding the collection lock (the ExplainCommand path in runCommand, and recordAggregateSlowQueryIfNeeded) and each gap is a chance to publish a store built from a document list another thread is mutating. Measured on 5000 documents, counting buildIndexStore passes: 20 operations in a transaction against a pre-existing store with eviction: 20 with CAS: 1 40 lookups with no transaction open with eviction: 0 with CAS: 0 Retracting a claim from the first version of this commit: it said the measurement needs TWO secondary indexes because a single one falls into the defs.size() <= 1 full-scan path. That is wrong. CollectionIndexStore registers the built-in _id_ definition in its constructor, so one secondary index already gives defs.size() == 2. The numbers above are identical for one and for two secondary indexes. The claim had reached the source comment, the commit message and the CHANGELOG; it is gone from all three. Unchanged from the previous version of this commit: - Documented the reachability consequence of provenance on the OwnedIndexStore javadoc: a context owner keeps that transaction's whole deepCloneDatabase snapshot reachable, so an ABANDONED transaction (dead thread, or a pooled thread whose currentTransaction ThreadLocal is never cleared) pins the snapshot until a later caller replaces the entry. - Fixed a comment wrapped mid-sentence. - Corrected the test class javadoc: without the fix it is the FULL SCAN assertion that fires first, not the index-vs-full-scan divergence. The stale index-backed candidate makes the update land on the live document, so the transaction's own snapshot never sees it. Verified: provenance recording still runs only when this call actually published (behind the prev == null early return), so commit and abort keep invalidating exactly the context-owned entries. Disabling the provenance guard itself still reddens all three regression tests with the same messages as #271, i.e. the CAS does not weaken their proof; restoring the file is byte-identical by md5. Full "inmemory" group 846/846, 0 failures, 0 errors, 7 skipped.
… it (#272) Follow-up to #271, correcting the four points from review - and correcting a regression the first version of this commit introduced. The mismatch branch in getIndexStore() originally evicted the entry before rebuilding. My first attempt simply removed that eviction, which fixed one case and badly broke a more common one: with the entry left in place, a transaction that meets a pre-existing NO_TRANSACTION entry loses putIfAbsent against it on every call, forever. It never publishes its own store, so it pays a full buildIndexStore - O(documents x indexes) - per operation for the whole life of the transaction. The entry now changes owner atomically once the rebuild finishes, via a compare-and-swap keyed on the exact entry this call observed. A same-key swap, never a remove-then-publish, so there is no moment with no entry for the key at all - which is strictly better than the eviction it replaces, because two callers reach getIndexStore() without holding the collection lock (the ExplainCommand path in runCommand, and recordAggregateSlowQueryIfNeeded) and each gap is a chance to publish a store built from a document list another thread is mutating. Measured on 5000 documents, counting buildIndexStore passes: 20 operations in a transaction against a pre-existing store with eviction: 20 with CAS: 1 40 lookups with no transaction open with eviction: 0 with CAS: 0 Retracting a claim from the first version of this commit: it said the measurement needs TWO secondary indexes because a single one falls into the defs.size() <= 1 full-scan path. That is wrong. CollectionIndexStore registers the built-in _id_ definition in its constructor, so one secondary index already gives defs.size() == 2. The numbers above are identical for one and for two secondary indexes. The claim had reached the source comment, the commit message and the CHANGELOG; it is gone from all three. Unchanged from the previous version of this commit: - Documented the reachability consequence of provenance on the OwnedIndexStore javadoc: a context owner keeps that transaction's whole deepCloneDatabase snapshot reachable, so an ABANDONED transaction (dead thread, or a pooled thread whose currentTransaction ThreadLocal is never cleared) pins the snapshot until a later caller replaces the entry. - Fixed a comment wrapped mid-sentence. - Corrected the test class javadoc: without the fix it is the FULL SCAN assertion that fires first, not the index-vs-full-scan divergence. The stale index-backed candidate makes the update land on the live document, so the transaction's own snapshot never sees it. Verified: provenance recording still runs only when this call actually published (behind the prev == null early return), so commit and abort keep invalidating exactly the context-owned entries. Disabling the provenance guard itself still reddens all three regression tests with the same messages as #271, i.e. the CAS does not weaken their proof; restoring the file is byte-identical by md5. Full "inmemory" group 846/846, 0 failures, 0 errors, 7 skipped. Co-authored-by: Heiko Kopp <extern.heiko.kopp1@porsche.de>
Follow-up to #270, on the same
InMemoryDriverindex-store area but a different case. Not urgent to merge in lockstep with anything; it is self-contained.Why this class of bug is surfacing now
The trailing-underscore index-name bug (#268) was masking a whole family of index defects, including this one.
While Morphium generated
campaignNumber_1_instead ofcampaignNumber_1, index creation against an existing database failed with MongoDB error 85 ("index already exists with a different name"). That failure was logged as a WARN and execution continued, so the index - including anyunique: trueconstraint - was silently never created. Anything that depended on a secondary index therefore ran with no index at all, and every code path that only misbehaves when an index exists was effectively unreachable.That is exactly the shape of this bug: it only manifests through an index-backed lookup. With the naming bug in place those lookups fell back to full scans and everything looked correct. Fixing #268 turned the indexes back on, and the index-dependent defects started becoming observable one after another - #270, this one, and the
cappedOnInsert/cappedOnRemovefinding below. None of them are new; they were just invisible.The bug
A
CollectionIndexStorebuilt before a transaction starts stayed stale for the entire transaction. #270 covers a store built during a transaction (invalidated on commit and abort). It does not cover a store that already existed when the transaction opened - which is the common case, since any earlier read or write builds one.Such a store was built by reading through the live database, so it holds live document instances.
startTransaction()then clones the database for the transaction's writes to mutate in place, without ever telling that pre-existing store. Inside the transaction:Reproduced downstream first: a Quarkus application using
InMemDriverin tests had 7 integration tests failing because a status update stayed invisible. Instrumentation showed collections whose store was built inside the transaction were tracked inindexStoreAccessedCollections, while the affected collection - the one with@Index(unique:true), whose store predated the transaction - was consistently absent. Commit ran,touchedCollectionswas correct, no abort, no exception.The fix
The root cause is that one global
indexStoreByCollectionhas to serve both live documents and per-transaction clones. Each cache entry now carries the data provenance it was built from - a specificInMemTransactionContext, or aNO_TRANSACTIONsentinel for the live database - andgetIndexStore()hands a store back only to the caller whose data it matches:Anything else is a cache miss and gets 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 (its index-backed update would land in the wrong snapshot - lost on its own commit, corrupting the other's on the way), and a non-transactional reader must not reuse a store built by a still-open transaction (it would observe uncommitted writes).
Keyed by context identity, not build order.
currentTransactionis thread-local, so transactions genuinely overlap across threads (InMemTransactionIsolationTestexercises exactly that), and ordering cannot separate "built later, for me" from "built later, for someone else".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()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. TheputIfAbsentrace is handled explicitly for the same reason: when another thread wins, its entry is returned only if its provenance matches, otherwise this caller uses its own build. Building twice is wasteful but never incorrect (the method contract already permits a benign duplicate build under the shared read lock); returning a foreign snapshot's store never is.On cost, since this was the blocker in #270's review: 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. A transaction that never touches a collection's store pays one reference comparison. That matters for the request-scoped, many-short-transactions profile this driver mostly sees.
I also considered making the store itself transaction-aware - each transaction holding its own store layered over the live one - and rejected it: it would touch all 11 call sites of
getIndexStore()for a correctness class the provenance check settles. Happy to go the structural route instead if you prefer it; flagging the trade-off rather than quietly picking.Verification
Three regression tests, each mutation-proofed individually - removing the corresponding guard reddens exactly that test, and reverting restores the file byte-for-byte (md5-verified):
expected: <created> but was: <from-b>);expected: <created> but was: <uncommitted>).The two concurrent tests deliberately hold the second transaction open across the first caller's read and update. My first attempt at the overlapping test let it commit instead - which invalidates the shared store via the #270 path, so the mutation stayed green and the test proved nothing. Worth knowing if you touch these.
Full
inmemorygroup: 846 tests, 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 isolation.Reported, not fixed here
cappedOnInsert/cappedOnRemoveshare the same root cause and are arguably worse.cappedDocSizesByCollectionis anIdentityHashMapkeyed by document reference, and neithercommitTransactionnorabortTransactiontouches it or its companion byte counter at all - not merely for entries predating a transaction, but unconditionally. So the same identity-staleness applies in both directions there. Out of scope for this PR; it fits the #269 follow-up bundle.I also updated the
abortTransaction()javadoc: the "known remaining gap" paragraph now distinguishes the concurrent-thread race it originally described (still out of scope, unchanged) from this single-threaded pre-existing-store case, which is now handled ingetIndexStore().One process note that might interest you
Four separate defects in this PR were found by automated reviewers rather than by me, and each tool found a different one:
Long.comparethat no longer existed.Relevant to you because upstream currently only has Copilot configured. CodeRabbit is not installed here - I only got those passes because it runs on my fork - and it was the one that found the non-transactional-reader hole, which is a genuine data-visibility bug Copilot did not flag.
Since morphium is open source, CodeRabbit is free for this repo (their OSS plan covers public repositories). Given how much of this driver is concurrency-sensitive, enabling it upstream looks like a straightforward win: a second independent reviewer on every PR at no cost. Entirely your call, of course - just passing on that the two tools demonstrably catch different things rather than duplicating each other.