fix(inmem): rebuild an index store that was not built by the current transaction - #23
fix(inmem): rebuild an index store that was not built by the current transaction#23Bardioc1977 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in the in-memory driver’s transactional index handling: a CollectionIndexStore created before (or by another) transaction could be incorrectly reused inside a transaction, leading to stale reads and “successful” updates that are silently lost on commit.
Changes:
- Track which transaction context (or no-transaction sentinel) an index store was built from, and rebuild the store when accessed from a different context.
- Add regression coverage for (1) pre-transaction store staleness and (2) overlapping transactions across threads not sharing index stores.
- Document the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java | Adds transaction-context attribution for index stores and rebuild logic inside getIndexStore(). |
| morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionPreExistingIndexStoreStalenessTest.java | Adds regression tests for stale pre-transaction stores and overlapping transactions. |
| CHANGELOG.md | Documents the transactional index-store staleness bug and the identity-based fix. |
Suppressed comments (1)
morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java:10803
- This JavaDoc says the pre-transaction-store case is addressed by
getIndexStore’s “epoch check”, but the new logic is an identity check againstindexStoreBuiltByTx(transaction-context identity), not an epoch/version. Updating the wording avoids confusion with the earlier “epoch/version” cache design mentioned above.
* <p>A separate, single-threaded variant of the general "identity-based staleness" problem
* class - a store built BEFORE the transaction even started, rather than one built during
* it and outliving it - is addressed by {@link #getIndexStore}'s epoch check, not here:
* such a store holds live document instances that this method's touchedCollections/
* indexStoreAccessedCollections invalidation never sees, because it was never recorded as
* accessed by this (or any) transaction in the first place.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Object builtByTx = indexStoreBuiltByTx.get(key); | ||
| if (builtByTx == ctx) { | ||
| return existing; | ||
| } | ||
| // Stale (predates this transaction) or foreign (belongs to a different, still-open | ||
| // transaction on another thread): fall through to a rebuild, exactly like a cache | ||
| // miss. Remove the entry first (rather than leaving it for putIfAbsent below to lose | ||
| // the race against) so a concurrent reader on another thread cannot observe it in | ||
| // between. | ||
| indexStoreByCollection.remove(key, existing); | ||
| } | ||
| CollectionIndexStore built = buildIndexStore(db, collection); | ||
| CollectionIndexStore prev = indexStoreByCollection.putIfAbsent(key, built); | ||
| if (prev != null) { | ||
| return prev; | ||
| } | ||
| // Record which transaction (if any) this build belongs to, keyed by context identity | ||
| // rather than by build order - see the read side above for why identity, not ordering, | ||
| // is required once transactions can overlap across threads. NO_TRANSACTION marks the | ||
| // store as universally valid: it was built from the live database, which every | ||
| // transaction's clone started from at some point. | ||
| indexStoreBuiltByTx.put(key, ctx == null ? NO_TRANSACTION : ctx); |
There was a problem hiding this comment.
Correct, and fixed in 782ff5b - Codex independently flagged the same race from the atomicity angle. putIfAbsent returning a non-null prev no longer short-circuits: the winner entry is returned only when its provenance matches this caller, otherwise this caller uses its own build. Store and owner are also folded into one immutable map value now, so they cannot be observed out of step.
| // Must be removed together with the store entry above, not left behind: a stale epoch | ||
| // entry with no matching store is harmless on its own (getIndexStore only consults it | ||
| // when indexStoreByCollection.get(key) already returned non-null), but leaving it around | ||
| // is needless unbounded growth in indexStoreBuiltByTx for any workload that creates | ||
| // and drops many short-lived collections. |
There was a problem hiding this comment.
Fixed in 782ff5b. The provenance is now part of the cache entry itself rather than a separate map, so both of those cleanup blocks (and their misleading "stale epoch entry" wording) are gone entirely - removing the store removes its provenance by construction. Also verified no stale epoch/Long.compare wording remains anywhere in the file.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc648f703b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // is required once transactions can overlap across threads. NO_TRANSACTION marks the | ||
| // store as universally valid: it was built from the live database, which every | ||
| // transaction's clone started from at some point. | ||
| indexStoreBuiltByTx.put(key, ctx == null ? NO_TRANSACTION : ctx); |
There was a problem hiding this comment.
When two transactions perform indexed reads of the same collection concurrently, the shared read lock allows their builds to overlap, but the store and its owner are published through separate maps. For example, transaction A can insert its store, transaction B can observe the missing/stale owner, replace that store with B's store and publish B as owner, and then A can execute this delayed put, permanently labeling B's store as A's. A's next index-backed update then accepts B's document instances as its own, mutates B's snapshot, and silently loses the update when A commits. Store replacement and owner identity need to be one atomic cache entry (or publication must retry and validate the exact store-owner pair).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 782ff5b - Copilot flagged the same race from the putIfAbsent side, so two reviewers landed on this independently.
Store and owner are now a single immutable map value (OwnedIndexStore(store, owner)) instead of two parallel maps, so they are published in one atomic operation and no thread can observe one without the other. The putIfAbsent race is handled explicitly too: when another thread wins, its entry is returned only if its provenance matches this caller, otherwise this caller uses its own build. Building twice is wasteful but never incorrect (the method contract already allows a benign duplicate build under the shared read lock); handing back a foreign snapshot store never is.
Full inmemory group 846/846 green after the change, InMemTransactionIsolationTest included.
4f7f58c to
782ff5b
Compare
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.
782ff5b to
0b009f6
Compare
Follow-up to sboesebeck#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 (sboesebeck#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 sboesebeck#268 turned the indexes back on, and the index-dependent defects started becoming observable one after another - sboesebeck#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. sboesebeck#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 sboesebeck#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 sboesebeck#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 sboesebeck#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.