fix(inmem): invalidate a pre-existing index store when it predates the open transaction - #21
fix(inmem): invalidate a pre-existing index store when it predates the open transaction#21Bardioc1977 wants to merge 1 commit into
Conversation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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: 591c877b41
ℹ️ 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".
| if (builtAtEpoch != null && builtAtEpoch >= ctx.getStartEpoch()) { | ||
| return existing; |
There was a problem hiding this comment.
When transactions overlap on different threads, a higher build epoch does not make a store valid for an older transaction: transaction A can rebuild the store from snapshot A, transaction B can then replace it with a store built from snapshot B at a higher epoch, and A will accept B's store here. A subsequent index-backed update in A therefore mutates B's cloned document rather than A's snapshot, so A can report success and silently lose the update when it commits. Since transaction state is thread-local, overlapping transactions are supported; the cache must track snapshot/transaction identity rather than treating every newer epoch as compatible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bc648f7 - this was a real hole, thanks.
You are right that a higher build epoch does not imply the store belongs to the transaction reading it. currentTransaction is thread-local, transactions genuinely overlap (InMemTransactionIsolationTest exercises exactly that), and ordering cannot tell "built later, for me" apart from "built later, for someone else". Transaction A would have accepted B store, run its index-backed lookups against B clones, and landed its update in B snapshot - lost on A commit, corrupting B on the way.
Replaced the epoch counter with an identity check: indexStoreBuiltByTx records which InMemTransactionContext (or NO_TRANSACTION, since ConcurrentHashMap rejects null values) each store was built from, and inside a transaction only a store built by that same context is reused. Everything else - live-built or foreign - is a cache miss and gets rebuilt from the current snapshot. The txEpoch counter and the startEpoch field are gone entirely, which incidentally leaves InMemTransactionContext byte-identical to develop again.
Added a regression test for your exact scenario. Worth noting that my first attempt at it was worthless: I let the second transaction commit, which invalidates the shared store via the sboesebeck#270 path, so the mutation stayed green. It only bites when B is held open across A read and update, so the test now uses two CountDownLatches to keep B open and aborts it at the end.
Mutation-proof for it: accepting any transaction store instead of only the current one reddens that test with expected: <created> but was: <from-b> - A reading B uncommitted write. Reverting restores the file byte-for-byte (md5 verified). Full inmemory group is 845/845 green, and the 8 concurrency tests in InMemTransactionIsolationTest stay green, so the identity check does not disturb existing isolation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java`:
- Around line 349-354: Update the database-level drop flow in drop(String db,
WriteConcern) to remove matching entries from both indexStoreByCollection and
indexStoreBuiltAtEpoch. Prefer introducing or reusing a single helper that
clears the corresponding collection and epoch metadata together, while
preserving existing database-matching behavior.
- Around line 4865-4871: Replace the global-epoch ownership logic at
InMemoryDriver transaction snapshot initialization (lines 4865-4871) so it does
not treat index stores as transaction-owned based on epoch alone. In the
index-store creation/retrieval flow at InMemoryDriver lines 6020-6057, keep
stores built from a transaction snapshot exclusively in InMemTransactionContext
and do not publish them to indexStoreByCollection; add a two-thread regression
test covering an open transaction after a snapshot update and confirming a
non-transactional reader sees only the live document.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 589ce6e7-f440-4af5-8251-19810930195e
📒 Files selected for processing (4)
CHANGELOG.mdmorphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.javamorphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.javamorphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionPreExistingIndexStoreStalenessTest.java
| /** | ||
| * Per-key build epoch for {@link #indexStoreByCollection}, used to lazily detect a store | ||
| * that predates the currently open transaction - see {@link #startTransaction} and | ||
| * {@link #getIndexStore} for the mechanism and {@link #txEpoch} for the counter itself. | ||
| */ | ||
| private final Map<String, Long> indexStoreBuiltAtEpoch = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Remove epoch metadata on database drop.
drop(String db, WriteConcern) removes matching indexStoreByCollection entries at Line 9758 without removing matching indexStoreBuiltAtEpoch entries. Dropping databases with distinct collection names therefore retains epoch metadata indefinitely.
Make database-level removal clear both maps, preferably through one helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java`
around lines 349 - 354, Update the database-level drop flow in drop(String db,
WriteConcern) to remove matching entries from both indexStoreByCollection and
indexStoreBuiltAtEpoch. Prefer introducing or reusing a single helper that
clears the corresponding collection and epoch metadata together, while
preserving existing database-matching behavior.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in InMemoryDriver's transaction handling. A previous fix invalidated a CollectionIndexStore built during a transaction, but a store built before the transaction started (the common case) was never invalidated. Because such a store holds live document instances while a transaction mutates its own cloned snapshot, index-backed reads inside the transaction returned stale documents (diverging from a full scan), and an update whose candidate came from that stale lookup was silently lost on commit.
The fix introduces a monotonic txEpoch counter bumped once per startTransaction(), records the build epoch of each persistent store, and lazily rebuilds any store whose epoch predates the currently open transaction on first access inside that transaction.
Changes:
- Add
txEpochcounter andindexStoreBuiltAtEpochmap; record start epoch on each transaction context and compare it ingetIndexStore()to detect/rebuild stale pre-transaction stores. - Keep the epoch map in sync on
resetData(),invalidateIndexStore(), and store rebuilds. - Add a regression test reproducing both the in-transaction read divergence and the post-commit write loss, plus a CHANGELOG entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java | Core fix: epoch counter, per-store build-epoch tracking, staleness check and rebuild in getIndexStore, plus cleanup in invalidateIndexStore/resetData. |
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemTransactionContext.java | Adds startEpoch field with getter/setter to record the transaction's start epoch. |
| morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionPreExistingIndexStoreStalenessTest.java | New regression test covering the read divergence and post-commit write loss. |
| CHANGELOG.md | Documents the pre-existing-index-store staleness fix. |
Notes: the >= epoch boundary is correct for both the pre-transaction (stale) and during-transaction (valid) cases, and startEpoch correctly travels with the context object across setTransaction(). The main gap found is that the whole-database drop(String db, WriteConcern) path removes stores from indexStoreByCollection without the corresponding indexStoreBuiltAtEpoch cleanup, leaving the unbounded-growth leak this change otherwise addresses.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // commit never merges back - the write is lost. This is checked on every lookup | ||
| // (not just once per transaction) because it must also catch a store built AFTER | ||
| // this transaction started but BEFORE it (elsewhere) went stale for some other | ||
| // reason - see the Long.compare below. |
There was a problem hiding this comment.
Right, that comment was stale - it referenced a Long.compare that never existed in the code. It is gone entirely now: the epoch counter it described has been replaced by an identity check (see the discussion on the other thread), and I verified no reference to Long.compare, getStartEpoch or builtAtEpoch remains anywhere in the file.
| // 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 indexStoreBuiltAtEpoch for any workload that creates | ||
| // and drops many short-lived collections. | ||
| indexStoreBuiltAtEpoch.remove(key); |
There was a problem hiding this comment.
Good catch, fixed in 4f7f58c. drop(String db, WriteConcern) now clears the provenance entries with the same db + "." prefix filter it already uses for indexStoreByCollection, so the two maps stay in step on the whole-database path too. Harmless for correctness (the map is only consulted after a store lookup already hit) but an unbounded leak for create/drop-heavy workloads, which is exactly the kind of thing that would have gone unnoticed.
591c877 to
bc648f7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java:6145
- This new cleanup keeps
indexStoreBuiltByTxin sync withindexStoreByCollectionfor the per-collection invalidation path, but the whole-database drop path is not updated to match.drop(String db, WriteConcern wc)prunesindexStoreByCollectiondirectly viaindexStoreByCollection.keySet().removeIf(key -> key.startsWith(dbPrefix))(around line 9772) and never touchesindexStoreBuiltByTx, so those entries are left behind. There's no correctness impact (a latergetIndexStoreoverwrites the stale entry on rebuild), but it is exactly the "needless unbounded growth" this comment guards against — here for workloads that create and drop many short-lived databases. Consider removing the matchingindexStoreBuiltByTxkeys with the samedbPrefixfilter in the whole-DB drop path.
// 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.
indexStoreBuiltByTx.remove(key);
morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java:10800
- This references
getIndexStore's "epoch check", but the mechanism added here is not epoch/version-based. TheindexStoreBuiltByTxfield doc explicitly states it is "Keyed by context IDENTITY, not by any ordering/epoch", and the class elsewhere reserves "epoch" for actual version checks (see thegetIndexStoreJavadoc noting "there is no epoch/version check here"). Calling it an "epoch check" here (and "stale epoch entry" ininvalidateIndexStore) is misleading — a reader may look for a version counter that doesn't exist. Consider "context-identity check" for consistency with the design.
* it and outliving it - is addressed by {@link #getIndexStore}'s epoch check, not here:
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.