On main, LRUQueryCache.clear() can wipe the statistics of an entry that stays cached

LRUQueryCache invites subclasses to keep per-index or per-query statistics through callbacks: count in onCacheEntryInserted / onCacheEntryEvicted, reset in onClear(), the "callback when the cache is completely cleared"; its javadoc says these callbacks "are called synchronously and under a lock". On main, #15558 split the cache into partitions with one lock each, and clear() now empties the partitions one at a time and calls onClear() afterwards with no lock held. A search that caches an entry into an already emptied partition, or after the loop and before the subclass's onClear() has reset its statistics, is counted by the subclass and then wiped by that reset, while the entry stays cached.

The subclass then reports 0 entries and 0 bytes for a cache that holds 1 entry of 336 bytes, and -1 / -336 once that entry is evicted, until the next clear(). Search results, the memory bound, and getCacheSize() / ramBytesUsed() once the threads are done stay correct. Every release, 10.6.0 included, runs clear() and onClear() under the lock every insertion takes, so only main and branch_11x (the coming 11.0) are affected.

// LRUQueryCache.java:457-463
/** Clear the content of this cache. */
public void clear() {
  for (int i = 0; i < this.numberOfPartitions; i++) {
    this.lruQueryCachePartition[i].clear();   // :1191-1203, that partition's write lock only
  }
  onClear();                                  // the subclass hook: no lock held
}

1. A query goes to partition query.hashCode() & (numberOfPartitions - 1) (:437); each partition has its own read-write lock (:956-959). An insertion takes only its partition's write lock and fires onCacheEntryInserted under it (putIfAbsent, :1100-1131). On main a search waits for a busy lock (get(), :1064); nothing skips the cache.
2. clear() releases each partition's lock as soon as that partition is empty, so two windows open: partitions already passed accept new entries while later ones are being cleared, and after the loop every partition accepts new entries until the subclass's onClear() has reset its statistics (before onClear() is called, and while it runs, since it holds no lock).
3. An insertion in either window is counted by the subclass, then wiped by its reset in onClear(), while the entry stays cached. The callback history "insert(q), then clear" with q still cached matches no serial order of the two operations.
4. getCacheSize() and ramBytesUsed() sum the partitions' own counters (:730-736, :563-569). The partition's own onClear() still asserts the write lock that insertions take before it resets them (:1139-1143), so the cache's counts are right once the threads are done (the getters sum the partitions without a lock); only the subclass hook lost the lock.
5. A subclass cannot repair it: clear() reports no per-entry evictions (neither does 10.x), and onClear() says nothing about partitions or entries. Serializing its own callbacks with clear() does not work either: a private lock held around super.clear() and taken in every ledger callback deadlocks, because the callbacks run under a partition write lock that super.clear() then needs (executed: a forced schedule with one partition and a latch-free run with 16 partitions both deadlock, assertions on and off).
6. On every release, 5.1.0 to 10.6.0, clear() runs cache.clear() and onClear() under the one lock every insertion takes (10.5.2 and 10.6.0 LRUQueryCache.java:415-426), and from 6.1.0 on a search that finds it busy skips the cache (lock.tryLock() in 6.1.0-9.10.0, readLock.tryLock() from 9.11.0, 10.5.2 :821); in 5.1.0-6.0.1 the cache is synchronized and a search waits.

• Minimal test (one partition, main 645d4fe14a20): getCacheSize() 1, subclass count 0, with assertions on and off.
• The finder's longer test (main a9779ef3c193, identical LRUQueryCache.java): "after clear() returned, subclass stats entries=0 bytes=0 but getCacheSize()=1 ramBytesUsed()=336; after evicting the survivor, subclass stats entries=-1 bytes=-336", both for an insertion just before onClear() and for one into an already cleared partition (16 partitions).
• A TLA+ model of the partitioned cache (TLC) finds the violation with 2 partitions (17-state trace) and with 1; the 10.x single lock (853,774 distinct states) and the cache without clear() (1,237,688) are clean.

• Who. Main and branch_11x only. Caching is disabled by default on main (IndexSearcher.java:82-84, #14187), so it takes an application that installs its own LRUQueryCache subclass keeping statistics through the callbacks and resetting them in onClear() (the pattern the class javadoc suggests and upstream's testFineGrainedStats uses), and that calls clear() while searches run.
• What goes wrong. The subclass undercounts the entries and bytes that survive the clear, goes negative once they are evicted, and stays wrong until the next clear(). Each entry cached in the window that is still cached after the reset is one entry the subclass no longer counts. No exception, no assertion.
• Not affected. Search results; the cache's own getCacheSize() and ramBytesUsed() once the threads are done; the memory bound (eviction uses the partitions' own counters, requiresEviction(), LRUQueryCache.java:1214-1221); every release.
• How likely: plausible. Without any latch, harnesses with 3 search threads (2 in the cross-model review's) and one thread calling clear() 5 times per round ended with a mismatch in 4 of 300 (assertions off) and 7 of 300 (on) rounds (finder), 132-140 of 300, 2 of 500 and 18-29 of 1,000 (history step); 0 without clear(), and 0 on 10.5.2 and 10.6. The rates depend on the harness and the machine load.

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => java.lang.AssertionError: callback entry count expected:<1> but was:<0>
       org.junit.Assert.fail(Assert.java:89)
       org.junit.Assert.failNotEquals(Assert.java:835)
       org.junit.Assert.assertEquals(Assert.java:647)
       org.apache.lucene.search.TestLRUQueryCacheClearOnClear.testInsertBeforeOnClearIsNotLost(TestLRUQueryCacheClearOnClear.java:83)

• The callbacks exist for this. "In case you would like to have more fine-grained statistics, such as per-index or per-query-class statistics, it is possible to override various callbacks: ... and {@link #onClear}. ... they are called synchronously and under a lock." (LRUQueryCache.java:88-101). The sentence dates from LUCENE-6157 ("Add the ability to compute finer-grained stats on the LRU filter cache", Adrien Grand, 2015); on main it is false for onClear().
• Lucene's own code still relies on the lock. The partition's onClear() asserts the write lock insertions take before it resets its counters (:1139-1143).
• Upstream's test keeps exactly this ledger. testFineGrainedStats counts in onDocIdSetCache / onDocIdSetEviction, resets in onClear() and asserts the count equals getCacheSize() (TestLRUQueryCache.java:954-1130, sequential); testConcurrency calls clear() while searches run (:114, :155). The class is "thread-safe" (:64).
• Nothing announces a change. CHANGES says "GITHUB#15558: Refactor QueryCache for performance." (lucene/CHANGES.txt:212); MIGRATE covers clearCoreCacheKey(CacheKey) and the per-entry firing of the old hooks (lucene/MIGRATE.md:514-542). On #15558 Michael Sokolov asked "so no functional change expected?" (their question); no comment there mentions clear() or onClear.
• The risk was named in general before. On #10081 (LUCENE-9038, 2019) Ben Manes read the contract as letting implementations "rely on this exclusive read/write access" under the cache's single lock and warned that a concurrent redesign would need to review external users; Adrien Grand replied that a better cache would come with "a way to transition our users to it" and that he "wouldn't worry about the migration path yet" (his words). That warning predates the partitioned code and names neither clear() nor onClear().

The strongest case for "intended" (built by the refute reviewer, the intended-lens reviewer and the devil's-advocate verifier): clear() says only "Clear the content of this cache." and promises no atomicity; the hooks are Expert and @lucene.experimental; the "under a lock" sentence is performance advice, and onHit / onMiss already run under a shared read lock since #13306; a global barrier would undo the partitioning; on #15558 sgup432 said the deprecated per-unique-query hooks "won't report accurate data"; and the deterministic tests block inside a callback.

Why it fails: latch-free stress runs show the drift with no blocking callback; sgup432's remark concerns the deprecated per-unique-query hooks, while the minimal test uses the recommended onCacheEntryInserted, and the drift is 0 without clear(); Lucene's own testConcurrency calls clear() while searches run; a subclass cannot fix it on its side (a private lock around super.clear() and its callbacks deadlocks, see the mechanism, point 5); clear() is rare, so taking every partition lock there costs little; and the drift is permanent until the next clear(). Verdict: a regression against 10.x and the documented statistics use, not a broken atomicity promise. Documenting a best-effort onClear() remains a legitimate maintainer choice.

One fix was built and validated in a scratch worktree of 645d4fe14a20 with gradle, alone and together with the sibling fix for LFV-1001; two alternatives are described, not built.

The finder's fix, with its comment rewrapped for google-java-format. clear() becomes:

int locked = 0;
try {
  for (; locked < numberOfPartitions; locked++) {
    lruQueryCachePartition[locked].writeLock.lock(); // always in ascending order
  }
  for (int i = 0; i < numberOfPartitions; i++) {
    lruQueryCachePartition[i].clear(); // re-enters the write lock it holds
  }
  onClear(); // now under every partition lock, like the single lock on 10.x
} finally {
  for (int i = locked - 1; i >= 0; i--) {
    lruQueryCachePartition[i].writeLock.unlock();
  }
}

Trade-offs

• Restores the 10.x ordering for clear() and makes "under a lock" true for onClear() again. It does not make callbacks of different partitions mutually exclusive (LFV-1000, the stale javadoc).
• While clear() runs, lookups and inserts on every partition wait for it (on main a search waits for a busy partition lock; on 10.x it skipped the cache instead). clear() is rare and only clears maps, so the pause is short unless a subclass's onClear() is slow.
• No other path holds two partition locks (assertConsistent() takes them one at a time), so ascending order suffices among them; TLC model E: no deadlock and the statistics invariant holds (4,852,161 distinct states).
• Coupling with LFV-1001. cleanUp() is synchronized on the cache and takes partition locks while it holds that monitor (LRUQueryCache.java:1392). With this fix alone, a subclass whose onClear() synchronizes on the cache can deadlock against cleanUp(): found by the finder's TLC model (M4) and raised independently by the cross-model reviewer. With cleanUp() on a private lock (LFV-1001's fix) the combination is clean (TLC M3, 2,180,461 distinct states). Land both together.

What the checks validated

• Stock 645d4fe14a20: the minimal test fails with assertions on and off.
• With A: the minimal test and TestLRUQueryCache pass (41 tests: the minimal test plus TestLRUQueryCache's 40, of which 3 are skipped as @AwaitsFix; 0 failures), seed DEADBEEF with assertions on and seed C0FFEE with assertions off.
• checkGoogleJavaFormat, ecjLintMain, forbiddenApisMain, ecjLintTest and forbiddenApisTest for core pass with A and the lint-clean test (re-run from scratch by the fact check).
• A plus LFV-1001's private-lock fix: the minimal test and TestLRUQueryCache pass (seed 5EED1234, assertions on; that run had the earlier comment wording, a comment-only difference; the fact check ran the final combined diff with the same seed: the five checks and both test classes pass).
• From the hunt: the finder's and the independent reproduction's JUnits pass with the fix shadowed in, and the independent reproduction's latch-free stress drops to 0 of 300 rounds.

Say in the class and onClear() javadocs that on a partitioned cache onClear() runs after the partitions are cleared, with no lock, and that entries cached concurrently with clear() may survive it.

Trade-offs

No behaviour change, but subclass statistics keep drifting and a subclass has no way to resync (onClear() carries no partition or entry information). The minimal test keeps failing, so it is no regression test for this option. The stale "under a lock" sentence needs fixing anyway (LFV-1000).

Each partition fires the eviction callback for its entries, under its own lock, as it clears them, so a per-entry ledger balances without relying on onClear().

Trade-offs

Changes what clear() has reported since 10.x (no per-entry evictions) and costs one callback per entry; a subclass that still resets in onClear() keeps losing the entries cached after the loop unless onClear() also moves under the locks.

Method: non-formal first signal, then TLA+ model checking (TLC).

• A round-14 planning probe for the partitioned query cache ran a deterministic interleaving test on the stock main jar: a latch in the insertion callback let a search cache into an already cleared partition, and the subclass reported 0 entries / 0 bytes while getCacheSize() was 1 and ramBytesUsed() 336, then -1 after eviction.
• A hunt unit turned it into public-API JUnits (16 partitions and the window before onClear()), a 10.x-callback variant, and a latch-free stress harness.
• It then model-checked the partitioned cache in TLA+ (LruPartClear.tla: N partitions with one write lock each, insertions with their callback, evictions, clear()'s loop then onClear(), cleanUp()'s steps, a subclass ledger). TLC's counterexample widened the finding to a single partition (onClear() runs outside the partition lock), the 10.x single-lock reference and the all-partition-locks fix were clean, and a second model found the fix's lock-order coupling with the synchronized cleanUp() (M4).