On main, LRUQueryCache's statistics callbacks run concurrently, but its javadoc still says "under a lock"

LRUQueryCache invites subclasses to keep "more fine-grained statistics, such as per-index or per-query-class statistics" by overriding its callbacks (onHit, onMiss, onClear and the deprecated per-query and per-DocIdSet hooks; it recommends the newer unified onCacheEntryInserted / onCacheEntryEvicted), and its class javadoc ends: "It is better to not perform heavy computations in these methods though since they are called synchronously and under a lock." Up to 10.6.0 that lock was one lock for the whole cache, so every insertion, eviction and clear callback ran alone, and Lucene's own base callbacks updated plain fields after asserting the lock was held.

On main, #15558 split the cache into partitions (16 by default) with one lock each, and a query goes to the partition picked by its hash. Insertion and eviction callbacks hold only their partition's lock, so callbacks of different partitions run at the same time, and clear() calls onClear() after releasing every partition lock, holding none of the cache's locks. That concurrency is the purpose of the change. The defect is documentation: the sentence was kept, it is false for onClear(), and neither the javadoc nor the MIGRATE.md section for #15558, which shows a subclass overriding these callbacks, says that implementations must now be thread-safe. A subclass that counts with plain fields silently loses updates; search results, the cache's own counters and its memory bound stay correct.

The class javadoc (LRUQueryCache.java:88-101 at 645d4fe14a20) lists the callbacks for "per-index or per-query-class statistics", onClear among them, and ends: "It is better to not perform heavy computations in these methods though since they are called synchronously and under a lock." The sentence dates from LUCENE-6157 (2015). In every release it described one cache-wide lock: in 10.5.2 the base onQueryCache, onQueryEviction, onDocIdSetCache, onDocIdSetEviction and onClear each start with assert writeLock.isHeldByCurrentThread(); and then do plain += / -= / = 0 (10.5.2 LRUQueryCache.java:229-280), and clear() calls onClear() inside writeLock.lock() (:415-426).

1. A query goes to partition queryHash & (numberOfPartitions - 1) (:426, :437; 16 partitions by default, :150). Each partition has its own ReentrantReadWriteLock (:956-959).
2. putIfAbsent takes only that partition's write lock and calls onQueryCache, onDocIdSetCache and onCacheEntryInserted under it (:1100-1131); evictions do the same in remove (:1155-1189). Two queries in different partitions take different locks, so their callbacks run in parallel.
3. clear() clears partition 0 to 15, each under its own lock, then calls onClear() holding none of the cache's locks (:457-463). So onClear() is not "under a lock" for any number of partitions.
4. onHit / onMiss run under the partition's shared read lock (:1064-1081); they could already overlap since 9.11.0 (#13306). Only the insertion, eviction and clear overlap is new on main.
5. The cache's own numbers are kept per partition, under each partition's lock (:1037-1058, :1139-1143), and the memory bound uses them, so Lucene stays correct. Only state a subclass updates from more than one partition is exposed, and the documented per-index statistics normally are such state: they add up many queries, and different queries go to different partitions by hash (all entries of one query stay in one partition, :949-951).

• Minimal test (one helper thread held inside onCacheEntryInserted, a second query in another partition): insertion callbacks running at the same time expected:<1> but was:<2>, with assertions on and off; 20 of 20 runs.
• The finder's stress (4 threads x 4,000 searches for 64 random terms over 8 segments, occasional clearQuery() + cleanUp(), no clear(), no latches or sleeps (each callback calls Thread.onSpinWait() once after its update), a plain int counter in onDocIdSetCache / onDocIdSetEviction): the counter ends off in 4 of 5 rounds (489, 491, 493, 499 against getCacheSize() 492, 492, 492, 500), up to 3 callbacks at once; on 10.6.0 0 of 5 rounds, 1 at a time.
• With one partition (the 6-argument constructor), insertions are serialized again, but onClear() still overlaps an insertion: events=[clearer onClear active=1, searcher-B onCacheEntryInserted active=2].

• Who is hit. Applications on 11.0 (main and branch_11x today) that install their own LRUQueryCache subclass and keep statistics in onCacheEntryInserted, onCacheEntryEvicted, the deprecated per-query / per-DocIdSet hooks or onClear() with plain (non-atomic) fields. Their numbers drift silently: no exception, no assertion.
• Default vs opt-in. Opt-in: the query cache is off by default on main (IndexSearcher.java:84, DEFAULT_QUERY_CACHE = null; CHANGES.txt:343, #14187), and only a subclass that overrides the callbacks is affected. Lucene's own production code has no such subclass.
• Scale. Ordinary concurrent searching is enough for the cross-partition part: the stress uses no clear(), no latches and no sleeps (each callback calls Thread.onSpinWait() once after its update) and ends off in 4 of 5 rounds. That shows the race happens; it is not a rate for real workloads. The error per round is small (off by 1 to 3 of about 500 in the stress), and Lucene never corrects a subclass's field; later lost updates can add to it or offset it.
• Not affected. Search results, getCacheSize(), ramBytesUsed(), the base class's hit/miss counts (LongAdder, LRUQueryCache.java:124-125) and the memory bound.
• No release has the insertion, eviction or clear overlap, 10.6.0 included: #15558 was not backported. Only onHit / onMiss overlapped before, under the shared read lock since 9.11.0 (#13306), with the same missing thread-safety note; the documentation fix covers them too.

The test uses onCacheEntryInserted, which exists only on main, like the bug. It picks the second query with a loop because a TermQuery's hash goes through BytesRef.hashCode(), whose seed is set once per JVM (from tests.seed under the test framework, else from the clock, StringHelper.java:136-152), and it needs only one extra thread because concurrency is the bug; all waits are bounded, there are no sleeps or stress loops. The stress, the controls and the version-history runs are separate checks, described below.

The assertion maxInside == 1 encodes the 10.x behaviour (one cache-wide lock), not a written guarantee: the sentence never said "one cache-wide lock". A failure therefore shows that the behaviour changed while the sentence did not; no code fix that keeps the partitions makes it pass, and after the documentation fix it still fails, by design. It shows the cross-partition insertion overlap only; the onClear() clause rests on the code (LRUQueryCache.java:458-463) and on an executed one-partition control.

Controls, executed: with one partition (the 6-argument constructor) the same test passes on main after about 11 s, because the second search now waits for the partition lock until the first callback's bounded wait ends. With the held hook switched to onDocIdSetCache, which also exists on 10.x, it passes on 10.5.2 and 10.6.0 and fails on main.

The comment above the assertion (line 86) was reworded on 2026-11-01 after the runs above, with no code change and the same line numbers; the standalone runner re-ran the edited file with assertions on and off (seed DEADBEEF): same message, same seed frame, line 87.

cp $REPRO/TestLRUQueryCacheCallbacksOverlap.java $LUCENE/lucene/core/src/test/org/apache/lucene/search/
cd $LUCENE && ./gradlew :lucene:core:test --tests TestLRUQueryCacheCallbacksOverlap -Ptests.seed=DEADBEEF -Ptests.asserts=true
# assertions off fail the same way: -Ptests.asserts=false

TestLRUQueryCacheCallbacksOverlap > testInsertionCallbacksOfTwoPartitionsRunAtTheSameTime FAILED
    java.lang.AssertionError: insertion callbacks running at the same time expected:<1> but was:<2>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:B046B89A4F35ED28]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.apache.lucene.search.TestLRUQueryCacheCallbacksOverlap.testInsertionCallbacksOfTwoPartitionsRunAtTheSameTime(TestLRUQueryCacheCallbacksOverlap.java:87)

1 test completed, 1 failed

# the same with -Ptests.asserts=false, and with the documentation fix applied (by design)

• The contract text (LRUQueryCache.java:91-101): "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." onClear() is in the list and runs with no cache lock held (:458-463).
• Lucene's reference implementation relied on it. 10.5.2 LRUQueryCache.java:229-280: each base insertion, eviction and clear callback asserts the single write lock and updates plain fields.
• #15558 edited this paragraph and kept the sentence. Its squash commit d50fb36d9af3 added the "per cache entry, not per unique query" and "Prefer the newer unified callbacks" sentences (:94-99) and the partition sentence (:66-68). Its review never discusses callback concurrency; msokolov summed it up as "segment the cache into blocks that can be updated concurrently, so no functional change expected?".
• The migration guide is silent. MIGRATE.md:514-542 (#16756) shows MyQueryCache extends LRUQueryCache overriding onCacheEntryInserted / onCacheEntryEvicted, with no thread-safety note.
• Lucene states such duties where they apply: QueryCachingPolicy.java:24, "Implementations of this class must be thread-safe." (precedent pointed out by the independent cross-model reviewer).
• Reliance is real. In 2019 Ben Manes read the contract as running these callbacks "under the global lock", so that "implementations may rely on this exclusive read/write access" (#10081 / LUCENE-9038).

The strongest case for "nothing to fix" (built by the devil's-advocate verifier, and from the refute review's reading): per-partition concurrency is the whole point of #15558, and main documents that "the cache is split into multiple partitions to reduce lock contention" (:66, :155-157); "under a lock" is performance advice that never said "the same lock" and is still true for every callback but onClear(); onHit / onMiss have overlapped under a shared read lock since 9.11.0 (#13306), where, at benwtrent's suggestion, the base class's own hit/miss counters became LongAdder; Lucene's own callback tests count with AtomicLong / AtomicInteger (TestLRUQueryCache.java:991-997, :2727-2736); and every hook is @lucene.experimental.

Why it fails: it defeats a code-bug claim, which is why none is made, and one cache-wide callback lock would be the wrong fix. It does not defeat the documentation defect. onClear() is called with none of the cache's locks held, so the sentence is false for one of "these methods" whatever "a lock" means (shown even with one partition). The documented use, per-index statistics, must keep state across partitions, and nothing tells the author that this state is now updated concurrently, while the 10.x base class showed the plain-field pattern. @lucene.experimental permits the change but does not make stale javadoc correct, and tests are not documentation. The verifier also noted, fairly, that the onHit / onMiss gap already existed on 10.x; the same doc fix covers it. Verdict: a documentation defect, not a code bug. This framing is the cross-model reviewer's and the verifier's.

One option was built and checked (A); B depends on a sibling fix; C and D are listed because they come up and should be avoided.

The finder's wording, narrowed by the reviewers: the class javadoc's last sentence of the callback paragraph becomes a description of main's locking plus a thread-safety duty; onClear() gets one line; MIGRATE.md's #15558 section gets one paragraph.

 * perform heavy computations in these methods though since they are called synchronously, and all
 * of them but {@link #onClear} are called while holding the lock of a cache partition.
 *
 * <p>These callbacks may be called concurrently from several threads: callbacks for entries of
 * different partitions can run at the same time, {@link #onHit} and {@link #onMiss} only hold a
 * shared read lock, and {@link #onClear} is called without holding any lock. Implementations that
 * keep statistics in these callbacks must therefore be thread-safe, for instance by using {@link
 * LongAdder}.

// onClear(): "called by clear() after all partitions have been cleared, without holding any lock"
// MIGRATE.md, #15558 section: the same note for subclasses

Trade-offs

• Documents the intended behaviour instead of changing it: no runtime cost, no behaviour change, the partitioning keeps its purpose.
• It does not help a subclass that already relies on the old lock; such code has to switch to atomics (LongAdder, AtomicLong). The note should not suggest synchronized (this) on the cache: cleanUp() is synchronized on the cache instance (LRUQueryCache.java:1392), the sibling deadlock LFV-1001.
• If LFV-991's fix lands (clear() calls onClear() under every partition lock), the onClear() clause must be dropped again.
• Wording caveats: "without holding any lock" means none of the cache's locks (the caller of clear() may hold its own), and "after all partitions have been cleared" does not mean the cache is still empty when onClear() runs: other threads can refill it first (LFV-991).

What the checks validated

• Stock 645d4fe14a20 (gradle, scratch worktree): the minimal test fails with assertions on (seed DEADBEEF) and off.
• With A: checkGoogleJavaFormat, ecjLintMain and renderJavadoc for core pass; TestLRUQueryCache passes (40 tests, 3 skipped, 0 failures) with assertions on (seed DEADBEEF) and off (seed C0FFEE).
• With A the minimal test still fails, with assertions on and off, as expected: A changes no code, and the test encodes the 10.x behaviour.
• The diff applies cleanly to a pristine 645d4fe14a20.

Make clear() take every partition's write lock and call onClear() under them (LFV-991's validated fix), then document only the cross-partition concurrency.

Trade-offs

Makes "under a lock" true again for onClear() and fixes LFV-991's lost statistics, but must land together with LFV-1001's fix (cleanUp() on a private lock) to avoid a lock-order deadlock. The cross-partition overlap stays and still needs A's note: with that fix the finder's cross-partition test still fails.

What the checks validated

Not run (in this doc; LFV-991's doc reports its own validation, and the finder's run with that fix shows that only the onClear test passes).

Wrap every callback in a single extra lock, so the old sentence becomes true.

Trade-offs

Brings back the cache-wide contention #15558 removed on every insertion and eviction; the reviewers and the devil's-advocate verifier called it the wrong fix. Not run.

A one-line javadoc telling implementations to use synchronized methods.

Trade-offs

Deadlocks: cleanUp() holds the cache's monitor while it waits for partition locks, and a search holds a partition lock while its callback waits for the monitor (LFV-1001). Recommend atomics instead.

Method: non-formal first signal, formal confirmation. A probe for the partitioned query cache ran a latch-free plain-counter stress on the stock jars: on main up to 3 insertion and eviction callbacks ran at once and the counter lost updates in 11 of 15 rounds of a three-fold re-run; on the 10.6 release commit 1 at a time and 0 of 5.

The finder (a hunt unit) turned it into deterministic public-API JUnits (one callback held open by a latch with a bounded wait: does a second callback start?) for two insertions and for onClear() plus an insertion, and wrote a TLA+ model of the partitioned cache. TLC violated the invariant CallbacksSerialized on main in 4 steps (two inserts in two partitions), found no violation for the 10.x single-lock reference (853,774 distinct states, exhaustive), and still violated it with the sibling clear() fix.

The independent cross-model reviewer (OpenAI GPT-5.6, intended lens, read-only) and the devil's-advocate verifier then narrowed the claim: the concurrency is intended, the documentation is not. The minimal repro reduces the evidence to one test of the cross-partition insertion overlap.