perf(inmem): stop evicting the index store on a provenance mismatch

[Bardioc1977]

Follow-up to #271, taking up the four non-blocking points from your review. Thanks for the independent verification - especially for re-running the mutation proofs rather than taking them on trust, and for catching that InMemTransactionIsolationTest staying green under the mutation is the part that actually matters. That framing is better than mine.

Point 1: the eviction is gone

I reproduced your measurement before changing anything, and the effect is real. My numbers differ from yours in absolute terms but agree in direction and magnitude - on a 5000-document collection with two secondary indexes and 20 interleaved transactional / non-transactional lookups:

with    eviction: 12 buildIndexStore passes
without eviction:  2
40 plain non-transactional lookups, no transaction open: 0

One thing worth recording for anyone repeating this: it needs two secondary indexes. With a single one the index plan falls back to a full scan (defs.size() <= 1) and no store is ever built, so the eviction makes no measurable difference at all - my first attempt at the measurement showed 3 vs 3 for exactly that reason and I nearly concluded the effect was not there.

Your reasoning for why the line buys nothing holds up: any other caller applies the same provenance check and rejects the entry anyway, and the putIfAbsent branch already handles a foreign entry occupying the key. Removed, with the measurement recorded in the comment so it does not get "tidied" back in.

I also took the secondary point in that section. Not evicting keeps the "no entry present" windows rare rather than routine, which matters precisely because of the two lock-free callers you identified (runCommand's ExplainCommand path and recordAggregateSlowQueryIfNeeded) - that reasoning is now in the comment too, credited to the review in the commit message.

Verified the removal does not weaken the proof: with the eviction gone I re-ran the mutation with the provenance guard itself disabled (if (true) { return existing.store(); } plus return prev.store()), and all three tests still go red with the same messages. So their proof rests on the guard, not on the eviction.

Point 2: reachability documented

Added to the OwnedIndexStore javadoc. Your framing was more precise than what I would have written - the pre-provenance version pinned only that collection's clones, whereas a context owner pins the whole deepCloneDatabase snapshot, and it is the abandoned transaction (dead thread, or a pooled thread whose currentTransaction ThreadLocal is never cleared) that turns this from theoretical into a real footprint difference. Written up as such, including that it is bounded at one entry per collection.

Point 3: comment reflow

Fixed.

Point 4: test narrative corrected

You are right, and I should have caught this myself - it was visible in my own mutation output. The full-scan assertion is the one that fires first, and the reason is the sharper statement of the bug: the stale index-backed candidate makes the update land on the live document, so the transaction's own snapshot never sees the change at all. The divergence is the visible surface, the lost write the consequence. Rewrote the class javadoc that way; the assertions themselves are unchanged.

Verification

Full inmemory group: 846 tests, 0 failures, 0 errors, 7 skipped - unchanged from #271.

Not in this PR

cappedOnInsert/cappedOnRemove and the cappedDocSizesByCollection IdentityHashMap, as agreed - that belongs in the #269 bundle. Good to hear on CodeRabbit; happy to be a data point if it turns out noisy on other parts of the codebase.

[sboesebeck]

Hi Heiko,

reviewed this the same way as #271 — clean worktree of the branch, full read of getIndexStore and every indexStoreByCollection access point, mutation proof re-run, and my own measurements rather than taking the numbers on trust.

Points 2, 3 and 4 are good and I would take them as-is. Point 1 I have to walk back — and that is my fault, not yours: my measurement in #271 only covered the interleaved case, and I presented it as if it were the general one. It is not. Removing the eviction is a large regression in the far more common case, and the measurement I gave you did not surface it. Details below.

Verified clean

No correctness gap from dropping the remove(). This was the thing I most wanted to be sure of, since without the eviction a mismatching entry now survives in the map. indexStoreByCollection is read in exactly two places, both inside getIndexStore: the get(key) and the putIfAbsent return value. Both apply the provenance check before the store escapes. Everything else is clear() (resetData), remove() (invalidateIndexStore), and keySet().removeIf(startsWith(dbPrefix)) (drop) — none of them reads a value. And all ~11 getIndexStore call sites take the result into a local and use it within the call; none caches it in a field or across a transaction boundary. So a stale entry left behind is unobservable. No correctness bug is introduced by the removal.

Mutation proof holds after the removal. With if (true) { return existing.store(); } plus return prev.store(), all three tests go red with exactly the messages from #271, and InMemTransactionIsolationTest stays 8/8 green under the mutation. Your point 4 correction is confirmed by the mutation output itself: the first failure is full scan reads through the transaction's snapshot and must see the update ==> expected: <updated> but was: <created>. The rewritten class javadoc describes what actually happens.

Full inmemory group on the branch: 846 tests, 0 failures, 0 errors, 7 skipped. Matches.

The problem: the eviction removal starves the transaction side

Without the remove(), a transaction that meets a pre-existing NO_TRANSACTION entry can never publish its own: putIfAbsent fails against the surviving entry, and the code deliberately does not overwrite. So it falls into the "build, fail to publish, return an unpublished store" branch on every single getIndexStore call, for the whole life of the transaction. It never gets a cache — it gets a full buildIndexStore per operation.

Measured with the indexStoreRebuilds counter, 5000 documents, counting buildIndexStore passes:

scenario	develop (#271, with eviction)	this PR
A: 10 transactional + 10 non-transactional lookups, interleaved	20	10
B: 20 transactional lookups only, store pre-exists	1	20
C: 20 transactional inserts only, store pre-exists	1	20
D: 40 non-transactional lookups, no transaction open	0	0

Identical numbers for one and for two secondary indexes.

A is my case from #271 and your improvement there is real. But B and C are the ordinary case, and they go from one rebuild for the entire transaction to one rebuild per operation — O(documents × indexes) per read or write, unbounded in the length of the transaction. A needs concurrent non-transactional traffic on the same collection to occur at all; B and C need only that the collection was touched once before the transaction started, which in any long-running process is essentially always. C is worse than the count suggests: the write path mutates an unpublished store, so its incremental onInsert/onUpdate maintenance is discarded too, on top of the rebuild.

So the "cost proportional to what a transaction actually touches" argument, which is the right argument, actually cuts against the change as written.

Suggestion

Keep the removal of the eviction, but let the newcomer take the entry over atomically after building instead of leaving it to putIfAbsent — CAS on the exact entry we saw:

prev = indexStoreByCollection.replace(key, existing, built) ? null : indexStoreByCollection.get(key);

with the putIfAbsent retained for the existing == null path. I measured that variant: 20 / 1 / 1 / 0. It keeps the benefit you identified in the second half of point 1 — no "no entry present" window at all for the two lock-free callers, since it is a swap and never a removal, which is strictly better than what develop does today — and it removes the starvation. It does not fix the ping-pong in A.

Getting A and B/C to 1 needs the cache keyed by (collection, owner) rather than by collection alone. That is a bigger change and I would rather see it on its own than bolted onto this PR, but it is the version where the ping-pong stops being a trade-off.

One factual correction

The "needs TWO secondary indexes" note is not right, and it is currently baked into the source comment, the commit message and the CHANGELOG, so it would ship as-is. CollectionIndexStore.definitions() includes the built-in _id_ definition (registered in the constructor), so a single secondary index already gives defs.size() == 2 and the defs.size() <= 1 full-scan fallback does not trigger. The #271 regression tests themselves use exactly one secondary index and depend on index-backed lookups working. My table above is identical for one and two indexes. Whatever produced your 3-vs-3 first attempt, it was something else — worth finding, since an explanation that fits the data would be useful, but the defs.size() one does not.

I could also not reproduce 12 vs 2 for the interleaved case; I get 20 vs 10 for the analogous setup. Same direction, different magnitude — probably setup differences, and not important once the shape of the effect is agreed.

Where that leaves it

Points 2, 3 and 4 are ready. For point 1 I would ask for the replace() variant plus the correction of the two-index claim in all three places, and then I am happy to merge. Everything else in the PR checks out, and the reasoning in the new comment is worth keeping almost verbatim — it just needs the entry to change hands rather than be abandoned.

Apologies again for sending you after a measurement that only covered half the picture.

[Bardioc1977]

You were right on both counts, and the regression was mine. Corrected in b0d5a3c02.

The starvation is real

I reproduced your B and C before changing anything, with the indexStoreRebuilds counter, 5000 documents:

                                     develop   #272 (first version)   CAS
B: 20 tx lookups, store pre-exists      1              20              1
C: 20 tx inserts, store pre-exists      1              20              1
D: 40 non-tx lookups, no tx open        0               0              0

Identical for one and for two secondary indexes. Mechanism exactly as you describe: the surviving entry keeps winning putIfAbsent, so the transaction never publishes and pays a full buildIndexStore per operation for its whole lifetime. And your prioritisation is right - my case A needs concurrent non-transactional traffic on the same collection to occur at all, while B and C need only that the collection was touched once before the transaction opened, which in a long-running process is always. My own "cost proportional to what a transaction touches" argument cut against my own change.

Took the replace() variant. I like it better than either previous version for the reason you give: it is a swap rather than a remove-then-publish, so there is no "no entry present" window at all for the two lock-free callers - strictly better than what develop does today. Verified 20 / 1 / 1 / 0 matches your measurement.

I also re-ran the mutation proof after the change, since with the CAS a foreign entry can now be overwritten and I did not want to assume that left the guard's proof intact: disabling the provenance check still reddens all three tests with the same messages as #271, and the file restores byte-identically by md5. Provenance recording still sits behind the prev == null early return, so commit and abort keep invalidating exactly the context-owned entries.

The two-index claim was wrong

Confirmed, and thanks for not letting it ship. CollectionIndexStore registers _id_ in its constructor, so a single secondary index already gives defs.size() == 2 and the full-scan fallback never triggers - and my own measurements above are identical for one and two indexes, which settles it. The claim was in the source comment, the commit message and the CHANGELOG; it is gone from all three.

I have not been able to explain my original 3-vs-3 result. My scratch harness then used two CountDownLatches to interleave the two threads, and I suspect the interleaving simply did not do what I thought, so both runs measured the same thing. What I should have done at that point was distrust the measurement instead of inventing a mechanism that fit it - the defs.size() story sounded plausible enough that I stopped looking. Noted.

Your 20-vs-10 for the interleaved case is more trustworthy than my 12-vs-2 for the same reason; I would go with yours.

Points 2, 3, 4

Unchanged from the previous version, as you suggested. Your framing on point 4 is in the class javadoc now.

Follow-up

Leaving (collection, owner) keying out of this PR as you asked. Happy to open it separately - that is the version where A stops being a trade-off, and it would also make the reachability note on OwnedIndexStore less relevant, since an entry would no longer be a single slot two owners contend for.

Full inmemory group: 846 tests, 0 failures, 0 errors, 7 skipped.

No apology needed - the measurement you gave me was reproducible and pointed at something real. Presenting one case as the general one is exactly the kind of thing a second pass is for, and yours caught it.

[Bardioc1977]

Filed the (collection, owner) keying follow-up as #279 so it does not get lost - with the measurement table (A/B/C/D across all three variants), the growth and cleanup caveats, and the note that scenario A still has no test asserting rebuild counts.

Also cross-referenced it with #278: if MVCC / copy-on-write lands first, #279 may become moot rather than just cheaper, since transaction-owned stores might stop being a thing at all.

[sboesebeck]

Reviewed the CAS version the same way as before: fresh worktree of the branch, read of
getIndexStore in full context rather than the diff alone, my own rebuild measurements, and
the inmemory group. This is good to merge.

Verified

The CAS is the right shape and correctly placed. replace(key, existing, built) in the
mismatch branch, putIfAbsent retained for existing == null. The swap never leaves the key
empty, so the two lock-free callers can no longer observe a gap — that is strictly better than
what develop does today, as you say.

Rebuild counts reproduce. 5000 documents, one secondary index, indexStoreRebuilds
counter, develop (3592c2191) vs this branch:

                                      develop   this PR
B: 20 tx lookups, store pre-exists       1         1
C: 20 tx inserts, store pre-exists       1         1
D: 40 non-tx lookups, no tx open         0         0
A: interleaved                          17        15

B/C/D match yours exactly. My A differs from both our earlier figures again (17/15 where you
had 12/2 and I had 20/10) — my harness interleaves with Thread.sleep rather than latches, so
the absolute number is scheduling noise. What matters is that A is unchanged by this PR in
either measurement, which is what #279 says.

inmemory group: 842 tests, 0 failures, 0 errors, 3 skipped — including
InMemTransactionPreExistingIndexStoreStalenessTest 3/3 and InMemTransactionIsolationTest
8/8. Slightly different totals from your 846/7, presumably environment; no failures either way.

The two-index claim is gone from all three places and replaced with the correct statement.

Re-running the mutation proof after the CAS rather than assuming it still held was the right
call, and the same for naming the 3-vs-3 result a method failure instead of explaining it away.
Both are the reason this took two rounds instead of shipping a regression.

One correction, for #279 rather than for this PR

#279 point 3 says provenance recording "stays behind prev == null" and therefore behind
"actually published". Since the CAS that is only true in one direction. If replace fails
and the entry has meanwhile been removed (invalidateIndexStore, drop, resetData), then
get(key) returns null, so prev == null while our built was never published — and the code
records provenance for a store that is not in the map.

The safety-critical direction still holds: published ⟹ recorded, so no clone-seeded store can
survive a commit unrecorded. The broken direction is recorded-but-not-published, which at worst
invalidates someone else's entry once and costs one rebuild. Not worth changing here — the race
is narrow and the cost is a rebuild — but #279 plans to build on that invariant, so it should
carry the precise version rather than the simplified one.

Merging.

[Bardioc1977]

Thanks for merging — and your correction to #279 is right. I checked it against the code rather than taking it on trust, and the race is exactly where you say.

prev == null is reached either because replace succeeded, or because it failed and the following get(key) found nothing. Three removers can produce the second case: invalidateIndexStore (remove, ~6185), drop (keySet().removeIf(startsWith(dbPrefix)), ~9812) and resetData (clear, ~874). So published ⟹ recorded holds — the direction #270 actually needs — but recorded ⟹ published does not, and I had written the invariant as if it were symmetric.

Agreed it does not warrant a change here: worst case is one wasted rebuild from invalidating a key that is not ours, and no clone-seeded store can escape a transaction unrecorded, which is the property that matters. But you are right that #279 would have inherited the simplified version and built on it, so I have replaced point 3 there with the precise statement, both directions spelled out, the three removers named, and the consequence for the (collection, owner) design: "recorded" must not be read as proof that an entry exists under that key, and cleanup has to tolerate recorded-but-absent entries — which matters more there than here, since per-owner keys make cleanup mandatory rather than tidy.

On scenario A: agreed it is scheduling noise. Three harnesses, three different magnitudes (12/2, 20/10, 17/15), and the only thing all three agree on is the direction and that this PR does not change it. #279 needs a deterministic harness before it can claim an improvement — the issue notes that no test asserts rebuild counts at all today, and A is the one case where the count is the assertion.

Test-count difference is on my side: I run the group with -Dgroups="inmemory", which pulls in tagged tests outside the in-memory driver package too. Not worth chasing given both runs are clean.

[sboesebeck]

Thanks — and agreed on all three points.

The asymmetry is the part worth having written down precisely, and #279 is where it matters: with per-owner keys, cleanup becomes mandatory rather than tidy, so "recorded" being weaker than "an entry exists under that key" stops being a footnote and starts being a constraint on the design. Good that it landed there before anything was built on the simplified version.

On scenario A — agreed, and I think the deterministic harness is the actual first task in #279, not a prerequisite to it. No test asserts rebuild counts today, so there is nothing to regress against either way; A is the one case where the count is the assertion, and without a harness that pins it, any improvement claim is unfalsifiable. I have put #279 on the 6.4.0 milestone.

Unrelated find while writing the release notes

The 6.3.0 changelog entry for DualChannelMessaging claimed it "interoperates with nodes running the other implementations". That is wrong, and it is the kind of wrong that costs somebody a weekend: directed messages and answers go into the recipient's DM collection (DualChannelMessaging.java:3299), which a SingleCollectionMessaging node never watches — so broadcast and topic traffic keeps flowing while request/reply silently times out. MultiCollectionMessaging shares no collection with either of the other two at all. docs/howtos/messaging-implementations.md had it right all along under "Mixed-cluster requirement"; only the changelog entry was wrong, and it had already propagated into the README and a draft release post before it got caught. Fixed in all of them.

That turned into three tickets, since the situation is currently detected only by DualChannelMessaging warning about itself — the direction that actually fails silently says nothing:

• Messaging: detect a messaging-implementation mismatch between participants on a queue (currently fails silently) #280 (6.3.1) — detect the mismatch. The catch is that it cannot be built on MessagingRegistry: the registry learns participants from StatusInfoListener responses, i.e. over the messaging channel itself, so two nodes on incompatible layouts never see each other's status messages. It needs a layout-independent announcement channel.
• Messaging: make the implementation a property of the queue instead of each client (transparent, mismatch impossible by construction) #281 (7.0.0) — make the implementation a property of the queue rather than of each client, so a mismatch cannot occur in the first place. The client setting becomes a proposal for a fresh queue.
• Messaging: translation bridge between implementations, for migration without a drain window #282 (unscheduled) — a translation bridge for migrating without a drain window, filed with an honest argument for why it is probably not worth building.

Review welcome on #280 in particular, since it is scheduled for the next patch.

On the release

The regression chain from #267 is fully merged on develop — #270 (5866558c7), #271 (2984091a6), #272 (45f9c3147) — and the 6.3.0 milestone is now clear. A full suite run is going across all five phases (InMem, MongoDB RS/Single, PoppyDB RS/Single); InMem and both PoppyDB phases are already green with no flakes, the two MongoDB phases are still running. If it stays green, I would like to tag 6.3.0 later today.

You asked me to hold back on 7 August because of the regression you saw in two of your applications. Since your fixes are what closed it, you are in a better position than I am to say whether that hold can come off: are those two applications clean against current develop? If so I will go ahead, otherwise I will wait for whatever you are still seeing.

[Bardioc1977]

Ran the first of the two applications from 7 August against current develop (088451821), the same Quarkus/InMemDriver application whose failures surfaced this whole bug class, and the same one you already verified on #271.

Built origin/develop fresh, confirmed the jar actually changed (InMemoryDriver.class timestamp matches the build, not a stale cache), dropped it into the application's bundled repo, ran the full backend suite: 1794 tests, 0 failures, 0 errors. No transaction-specific test names in there — the exposure was through ordinary repository/adapter tests running against InMemDriver, same as before, not a dedicated transaction suite — but that is exactly the surface that broke originally, and it is clean now.

The second application from 7 August is not one I have access to, so I can't speak to it — that half of the hold is yours to lift. From my side, the hold has nothing left standing on it.

[sboesebeck]

Thanks for running it — and for checking the jar actually changed, which is the step everyone skips.

Worth noting for the record: you built 088451821, which is a documentation-only commit sitting on top of 42454e54f (changelog wording plus the missing README sections). So the code you tested and the code we tested are byte-identical — the only delta between the two runs is prose.

From our side the full matrix finished green this morning on 42454e54f:

Phase	Classes	Methods	Result	Duration
InMem Driver	287	2019 (+16 skipped)	PASSED	1:04
MongoDB ReplicaSet	212	1206 (+16)	PASSED	1:24
PoppyDB RS	208	1188 (+11)	PASSED	1:06
MongoDB Single	212	1206 (+22)	PASSED	1:19
PoppyDB Single	208	1188 (+16)	PASSED	1:04

1127 test classes / 6807 test methods, 1:35 h wall clock across the parallel waves. Zero failures, zero errors, zero flakes, zero broken — I checked every per-phase counter individually rather than trusting the summary banner. The two PoppyDB phases run against a real three-node replica set and a real single node, so the replication and failover surface that changed most in this release is exercised rather than emulated.

Your 1794 are the more interesting number of the two, though. Ours prove the library is self-consistent; yours prove it behaves inside an application that actually broke.

On the second application — that one is mine to answer, and it is in a better state than a test suite. Two of our own production applications have been running a 6.3.0-SNAPSHOT for a while now, and both are clean. Both were also hit by the eviction-without-reseed failure behind #233, which is part of why this release matters to us rather than being a nice-to-have.

So both halves of the hold are lifted. I am tagging 6.3.0 today. Thanks for the thoroughness through this whole cycle — three regressions in the index-store/transaction area found and closed before a release rather than after one is exactly the outcome that makes this kind of review worth the effort on both sides.