fix(inmem): invalidate index store for touched collections on abort - #20
fix(inmem): invalidate index store for touched collections on abort#20Bardioc1977 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: c429b8dc08
ℹ️ 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 (ctx == null) { | ||
| return; | ||
| } | ||
| for (String key : ctx.getTouchedCollections()) { |
There was a problem hiding this comment.
When a transaction only reads this collection, an indexed find can lazily build the shared CollectionIndexStore from cloned snapshot documents, but markCollectionTouched records writes only, so this loop skips the collection on abort. The store therefore retains clone references; subsequent live deletes cannot remove them by identity, and inserts can be rejected as duplicates even after the live collection is empty. Track collections whose stores are accessed or built during the transaction and invalidate those as well.
Useful? React with 👍 / 👎.
| log.info("{} discovered higher term {} from {}, updating from {}", | ||
| myAddress, requestTerm, request.getCandidateId(), myTerm); | ||
| becomeFollower(requestTerm, null); | ||
| becomeFollower(requestTerm, null, false); |
There was a problem hiding this comment.
If the current leader receives a higher-term vote request that it later denies because of priority or an outdated candidate log, this call demotes it with resetTimer=false; however, becomeLeader() previously canceled and nulled its election timer, and the denial branch performs no reset. The node is consequently left as a follower with no election timeout—in a two-node cluster where the lower-priority peer keeps being denied, this can prevent the cluster from electing any leader indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR primarily addresses correctness issues in the in-memory transaction/indexing lifecycle (preventing stale CollectionIndexStore state from surviving an aborted transaction), with additional fixes and optimizations in indexing, change-stream event stability, and PoppyDB election/messaging behavior.
Changes:
- Invalidate per-collection index/TTL structures on
InMemoryDriver.abortTransaction()and add a regression test for stale unique-index entries leaking past abort. - Optimize
insert()duplicate-_idpre-check to use the_id_index (O(1) lookup) and addCollectionIndexStore.containsId. - Fix/adjust several independent behaviors: IndexDescription auto-name trailing underscore bug, PoppyDB election timer reset semantics on denied vote requests, remove a dead messaging index, and deep-copy change-stream documents for event stability.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| poppydb/src/test/java/de/caluga/test/poppydb/election/ElectionManagerTest.java | Adds regression test for election-priority denial not starving own election timer. |
| poppydb/src/main/java/de/caluga/poppydb/messaging/MessagingOptimizer.java | Removes dead locked_by/locked index from standard messaging indexes. |
| poppydb/src/main/java/de/caluga/poppydb/election/ElectionManager.java | Adds conditional election-timer reset when becoming follower (prevents starvation via denied vote requests). |
| morphium-core/src/test/java/de/caluga/test/morphium/driver/inmem/InMemTransactionIsolationTest.java | Adds regression test for stale unique-index entries after aborted transactions. |
| morphium-core/src/test/java/de/caluga/test/mongo/suite/base/IndexDescriptionTest.java | Adds regression tests for IndexDescription auto-generated index names. |
| morphium-core/src/main/java/de/caluga/morphium/IndexDescription.java | Fixes auto-generated index-name formatting (no trailing underscore). |
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/InMemoryDriver.java | Invalidates index/TTL structures on abort; optimizes insert duplicate _id check; deep-copies change-stream docs. |
| morphium-core/src/main/java/de/caluga/morphium/driver/inmem/CollectionIndexStore.java | Adds containsId helper for O(1) _id presence checks. |
| CHANGELOG.md | Documents the behavior/performance changes included in this PR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 400ms of continuous, correctly-denied lower-priority requests have passed - well past | ||
| // our own 150-200ms timeout. If denials still reset our timer, we'd still be FOLLOWER | ||
| // here (last reset was only 50ms ago). With the fix, our own timeout fired on schedule | ||
| // partway through the loop and we became CANDIDATE independently of the peer's retries. | ||
| assertEquals(ElectionState.CANDIDATE, manager.getState(), | ||
| "Node should have started its own election despite continuous lower-priority vote requests"); |
| #### InMemoryDriver: insert's duplicate-`_id` pre-check is an O(1) index lookup instead of an O(N) collection scan | ||
| Every `insert()` call built a `HashSet` of all existing `_id`s by iterating the entire collection — under the exclusive write lock. For single-document inserts into large collections (the messaging workload) that scan was the dominant per-insert cost, and it was redundant: the per-collection `CollectionIndexStore` always carries a unique `_id_` index that reflects exactly the committed documents. The pre-check now asks that index directly (new `CollectionIndexStore.containsId`, a single hash lookup). Semantics are unchanged: ordered inserts still throw on a committed duplicate, unordered ones still collect a code-11000 writeError, and duplicates *within* one batch still surface at the per-document index insert, as before. As a side effect the check now uses the index's `MorphiumId`/`ObjectId` normalization, so a duplicate no longer slips past the pre-check just because caller and store hold the same id in different wrapper types. | ||
|
|
||
| #### PoppyDB: dead `locked_by`/`locked` messaging index removed | ||
| `MessagingOptimizer` created a `msg_locked_by_1_locked_1` index on every registered messaging collection, but those fields no longer exist on `Msg` — locking moved to the separate `MsgLock` collection long ago. Nothing ever queried the index; it only added per-insert maintenance cost on the hottest collection. Removed. |
c429b8d to
101973e
Compare
|
@codex review Addressed the P1 finding from the previous review: the abort-side fix now invalidates Added a second regression test, |
💡 Codex Review Check duplicate IDs against the transaction snapshot
When an in-memory transaction is active, ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Note: the earlier review comments on
@copilot review |
The updated implementation looks correct and complete. The
The |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 101973e9bb
ℹ️ 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".
| lock.writeLock().lock(); | ||
| try { | ||
| invalidateIndexStore(dbName, collName); | ||
| invalidateTtlQueue(dbName, collName); |
There was a problem hiding this comment.
When an indexed transaction aborts on a TTL-indexed collection, this removes its queue, but a live insert or update before the next TTL sweep calls ttlEnqueue, whose computeIfAbsent recreates a non-null queue containing only that document. sweepTtlQueue bootstraps existing documents only when the queue is null, so all older TTL documents are then permanently omitted and never expire. The same risk applies to the newly added read-only commit invalidation; either rebuild synchronously or retain an explicit “bootstrap required” state that ttlEnqueue cannot erase.
Useful? React with 👍 / 👎.
commitTransaction() correctly invalidates the persistent CollectionIndexStore for every collection the transaction touched before merging the snapshot back into the live database. abortTransaction() never did the equivalent, even though a store can be lazily rebuilt WHILE a transaction is open: buildIndexStore() reads via getCollection(), which resolves against the transaction's private snapshot while one is active (see getDB()). That snapshot's documents are structural clones (deepCloneDatabase() deep-copies every document), not the same object references stored in the live database. Those clone instances get registered into the store's unique-index buckets. On abort, the snapshot itself is discarded, but the store is a single object shared across the live database and every transaction (keyed only by db+collection). Without invalidating it, it keeps referencing the orphaned clones forever: CollectionIndexStore's IndexEntry.remove() matches only by reference identity, so no later onRemove/clearCollection against the REAL live documents can ever find and evict the clone. Every subsequent insert under that same key is then rejected as a duplicate, even after the live collection has been cleared to zero documents. Root cause found while debugging 14 real Quarkus integration test failures in a downstream project that had nothing to do with their own code: a duplicate-key error surfaced against a collection an @beforeeach had already provably cleared to zero documents. Reproduced at the driver level with a minimal transaction sequence: insert+commit, then a second transaction that lazily rebuilds the invalidated store from its own snapshot while failing a duplicate-key check, then abort, then clear via delete() (the codepath Morphium.clearCollection(Class) actually uses in production, not the dedicated ClearCollectionCommand, which already invalidates the store itself and would mask this bug), then a fresh insert under the same key - which failed before this fix and succeeds after it. Extended per review feedback on the first version of this fix: that version only invalidated collections in getTouchedCollections() (write-touched), but getIndexStore() can just as easily be reached by a purely READ-ONLY indexed query (getDataFromIndex(), called unconditionally by every find()) while a transaction is open, without ever calling markCollectionTouched. Introduced a separate, strictly broader InMemTransactionContext#indexStoreAccessedCollections set, populated by getIndexStore() itself, and invalidated by BOTH commitTransaction() and abortTransaction() for every collection recorded there - not just the written ones. Added two regression tests to InMemTransactionIsolationTest: - abortedTransactionDoesNotLeakStaleIndexEntriesIntoLaterInserts (the original write-path reproduction) - abortedReadOnlyTransactionDoesNotLeakStaleIndexEntriesEither (the read-only-transaction gap, uses only find() before abort) Both verified red without their respective fix (exact E11000 duplicate-key error against an empty collection) and green with it. Full InMemTransactionIsolationTest suite (8 tests) and the complete inmemory-tagged test group (843 tests) stay green. Maintainer review follow-up (sboesebeck, PR sboesebeck#270): - Narrowed the recording scope in getIndexStore() from every access to only an actual build (gated on putIfAbsent returning null): a plain reuse of an already-built store can never introduce clones, since the store already existed before this call and holds only references that were valid at the time it was built. Only a build reads via getCollection() against the transaction's cloned snapshot. Write paths remain covered separately by markCollectionTouched. This avoids discarding the index store and TTL queue on every read-only access to a collection, which was a real regression for request-scoped transactions. - Added a CHANGELOG entry under [Unreleased] -> Fixed for this bugfix, matching the file's existing style. - Extracted invalidateIndexStoreForKey(String) as a private helper in InMemoryDriver to de-duplicate the split-lock-invalidate logic shared by commitTransaction()'s new loop and abortTransaction()'s loop. commitTransaction()'s existing finally block keeps its distinct merge semantics and is not changed to use it. - Generalized the abortTransaction() javadoc and the second regression test's javadoc to remove a customer-specific field name/value and a specific AI-reviewer mention, keeping the root-cause explanation itself unchanged. - Softened the abortTransaction() javadoc: the fix bounds the damage (a clone can no longer outlive its transaction) rather than implying clones can never outlive a transaction, and explicitly calls out the narrower pre-existing race that remains out of scope (a concurrent non-transactional delete-then-reinsert under the same unique key while the transaction is still open). - Added an extra indexed-lookup assertion to both regression tests, right before the final insert: a find() on the unique-index field must return zero results against the cleared collection. Before the fix this would have returned the orphaned clone as a phantom document - the worse symptom, since it surfaces through the exact codepath the index exists to serve, not just a full-scan query. Verified via mutation testing that both new assertions turn red without the fix.
101973e to
35d7c61
Compare
Review PR ahead of the upstream fix against sboesebeck/morphium#develop. See commit message for full analysis (root cause, regression status, verification).
Known pre-existing gap noted during review (not fixed here, scope kept to the transaction-abort index-store leak): Codex flagged that
invalidateTtlQueue()+ttlEnqueue()'scomputeIfAbsentcan silently drop older TTL documents from expiry tracking if an enqueue races between an invalidation and the next sweep - this bug already exists independently of this PR (it's reachable viacreateIndex/dropIndexes/rename/drop/commit today), this PR's abort-side and read-only-commit invalidation calls just make the race window open more often. Filed as sboesebeck#269 so it doesn't get lost; deliberately not addressed in this PR to keep its diff focused.