On main, LRUQueryCache.cleanUp() deadlocks with subclasses whose statistics callbacks are synchronized

On main, LRUQueryCache.cleanUp() is public synchronized: it holds the cache object's monitor while it takes each partition's read lock in keys() and the write lock in the partition remove(). Searches call the statistics callbacks while they hold a partition lock: onCacheEntryInserted under the write lock, onHit and onMiss under the read lock. A subclass that makes its counters thread-safe with synchronized methods or synchronized (this), the plain Java idiom, therefore takes the two locks in the opposite order to cleanUp(), and the two threads deadlock as soon as cleanUp() has stale entries to remove while a search is in such a callback. The hang is permanent and uninterruptible: every later cleanUp(), every search whose callback needs the monitor, and every other search that looks up or inserts in that partition queue behind it. The class says "This class is thread-safe.", invites callback overrides for statistics, and calls cleanUp() "safe to call concurrently"; on main cleanUp() must run, from a background scheduler or the caller. On 10.x the same subclass is safe, because LRUQueryCache there locks only a private lock. The modifier came with #15558 (2026-04-14), so only main and branch_11x are affected. Moving cleanUp() to a private lock object removes the cycle without changing what the cache stores or evicts; with it the repro passes, and so do 2,339 tests of the org.apache.lucene.search package.

1. cleanUp() takes the cache monitor. public synchronized void cleanUp() (:1392). It copies the stale-key sets and returns early if both are empty (:1393-1399). Otherwise, still holding the monitor, it iterates keys() (:1404), which visits every partition (:214-222) and takes each partition's read lock (:1025) just long enough to copy its keys (released at :1029), and then calls the partition remove() for each stale entry (:1410), which takes the write lock (:1156) and fires onCacheEntryEvicted under it (:1183). There is no read-to-write upgrade: the monitor is the only lock cleanUp() holds across these steps.
2. Stale entries are ordinary. clearQuery() marks a query (:451-455, :1250-1256); a closed segment marks its key through clearCoreCacheKey() (:443-448). On main they are not removed inline: cleanUp() must run, from the scheduler configured with CacheCleanUpParameters (:197-204, scheduleWithFixedDelay(this::cleanUp, ...)) or from the caller (:103-113).
3. Searches call the callbacks under partition locks. putIfAbsent() takes the write lock (:1100) and calls onCacheEntryInserted (:1126); get() takes the read lock (:1064) and calls onMiss or onHit (:1071-1077).
4. The cycle. The search holds a partition lock and waits for the cache monitor in the subclass callback; cleanUp() holds the monitor and waits for that partition lock. In the repro cleanUp() parks on the read lock in keys(), blocked by the search's write lock. With synchronized onHit, cleanUp() waits for the write lock in remove() while searches hold read locks; the JVM's deadlock detector does not report that variant, because a read lock has no owner.
5. The monitor is used nowhere else. No other method locks the cache object (:445 locks keysToClean, :960 is a Collections.synchronizedMap with its own mutex). The second lock exists only because of the modifier, whose job is to serialize concurrent cleanUp() calls. Javadoc does not render synchronized, so the API docs show public void cleanUp().

Monitor entry and lock() ignore interrupts and timeouts, so the pair never returns. Every later cleanUp() and every search whose callback needs the monitor block behind them, and so do all other searches that look up or insert in that partition and every clearQuery() call (it takes each partition's write lock, :1250-1256), because the stuck search holds that partition's write lock. With synchronized onHit/onMiss, that is every search that consults the cache. Searches that skip the cache (segment not cacheable, not eligible, or without a core cache helper: :816-831) are not blocked. Lucene logs nothing about it; results and the index are not damaged.

• Who. Applications on main / 11.0 with an LRUQueryCache subclass whose callbacks synchronize on the cache object (synchronized methods or synchronized (this)), that run cleanUp() while searching, through the background scheduler or their own periodic calls.
• Opt-in. The query cache is off by default on main (IndexSearcher.java:84, GITHUB#14187), and the stock callbacks use LongAdder. Subclasses with atomics or a private lock are not affected (executed controls).
• Why the idiom is likely. On main, callbacks of different partitions run at the same time (LFV-1000) and onHit/onMiss run under a shared read lock, so subclasses now need their own synchronization, and synchronized is the default Java choice. The Elasticsearch and OpenSearch subclasses say today "It's ok to not protect these callbacks by a lock since it is done in LRUQueryCache"; neither synchronizes on the cache, so neither is known to hit this.
• Scale: none needed. The finder's stress (3 search threads, background cleanup every 1, 100 or 1000 ms) hung after about 1 s in 5 of 5 runs, an independent stress in 7 of 7. A workload with nothing waiting inside a callback (16 partitions, cleanup every 1 s, NRT refresh closing merged segments) hung within 2 s in 3 of 3 JVMs, and 3 of 3 with clearQuery() on 1 in 20 searches. Controls, 30 s each: with the fix 12,978,280 searches, with a private-lock subclass on stock main 10,485,017, no hang.
• Effect. A permanent hang of the cleanup thread and of searches that consult the cache; no wrong results, no index damage.
• How likely. Rare: none of the four public downstream subclasses surveyed (Elasticsearch, OpenSearch, nrtsearch, mongot) uses the idiom today, and the code is unreleased.
• Workaround. Keep subclass statistics in atomics or LongAdder, or under a private lock object, never under this.

clearQuery(f:y) leaves one stale query, so cleanUp() takes partition locks instead of returning early. The spin in the callback only orders the two threads the way a background cleanup thread can meet them; nothing waits inside Lucene code, and runs with no wait inside any callback hang the same way. The test is plain JUnit 4, not LuceneTestCase, because deadlocked threads cannot be stopped and the thread-leak checker would add suite errors on stock; the threads are daemons. The test itself uses no randomness; the seed only picks gradle's randomized JVM arguments (seeds DEADBEEF and 5EED1234 were run).

Lint: :lucene:core:ecjLintTest rejects the unused lambda parameter context (line 43); write _ -> true when adding the test to Lucene's tree. With that one-token change the test still fails on stock and passes with the fix, and the test lint passes.

Regression test: this same test (with _ -> true); with the fix it passes in 0.58 s to 7.3 s through gradle, depending on host load, while on stock it waits the full 10 s bound and fails (a passing run returns at once, so a larger bound costs nothing). As a LuceneTestCase run as its own suite it passes with the fix with no thread-leak report, and on stock adds "2 threads leaked from SUITE scope", so it could also become a method of TestLRUQueryCache (not run there).

cp $REPRO/TestLRUQueryCacheCleanUpDeadlock.java $LUCENE/lucene/core/src/test/org/apache/lucene/search/
cd $LUCENE && ./gradlew :lucene:core:test --tests TestLRUQueryCacheCleanUpDeadlock -Ptests.seed=DEADBEEF
# the same result with -Ptests.asserts=false

# gradle, stock 645d4fe14a20, -Ptests.seed=DEADBEEF, assertions on (the same with -Ptests.asserts=false)
TestLRUQueryCacheCleanUpDeadlock > testCleanUpDuringInsertionCallback FAILED
    java.lang.AssertionError: expected the search to finish; after 10 s search=BLOCKED, cleanUp()=WAITING
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertFalse(Assert.java:65)
        at org.apache.lucene.search.TestLRUQueryCacheCleanUpDeadlock.testCleanUpDuringInsertionCallback(TestLRUQueryCacheCleanUpDeadlock.java:96)

# thread dump (jcmd Thread.print -l) 5 s into an unchanged standalone run, -ea
# (Thread-0 is the search, Thread-1 runs cleanUp(); frames below the test left out, "java.base@25.0.3/" module prefixes removed)
Found one Java-level deadlock:
=============================
"Thread-0":
  waiting to lock monitor 0x0000ffff0c00ae60 (object 0x00000000f78f2ae0, a org.apache.lucene.search.TestLRUQueryCacheCleanUpDeadlock$StatsCache),
  which is held by "Thread-1"

"Thread-1":
  waiting for ownable synchronizer 0x00000000f7705898, (a java.util.concurrent.locks.ReentrantReadWriteLock$NonfairSync),
  which is held by "Thread-0"

"Thread-0" BLOCKED (on object monitor)
  at org.apache.lucene.search.TestLRUQueryCacheCleanUpDeadlock$StatsCache.onCacheEntryInserted(TestLRUQueryCacheCleanUpDeadlock.java:55)
  - waiting to lock <0x00000000f78f2ae0> (a org.apache.lucene.search.TestLRUQueryCacheCleanUpDeadlock$StatsCache)
  at org.apache.lucene.search.LRUQueryCache$LRUQueryCachePartition.putIfAbsent(LRUQueryCache.java:1126)
  at org.apache.lucene.search.LRUQueryCache.putIfAbsent(LRUQueryCache.java:438)
  at org.apache.lucene.search.LRUQueryCache.tryPopulateCache(LRUQueryCache.java:610)
  ...
  Locked ownable synchronizers:
  - <0x00000000f7705898> (a java.util.concurrent.locks.ReentrantReadWriteLock$NonfairSync)

"Thread-1" WAITING (parking)
  - parking to wait for  <0x00000000f7705898> (a java.util.concurrent.locks.ReentrantReadWriteLock$NonfairSync)
  at java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.lock(ReentrantReadWriteLock.java:744)
  at org.apache.lucene.search.LRUQueryCache$LRUQueryCachePartition.keys(LRUQueryCache.java:1025)
  at org.apache.lucene.search.LRUQueryCache.keys(LRUQueryCache.java:217)
  at org.apache.lucene.search.LRUQueryCache.cleanUp(LRUQueryCache.java:1404)
  - locked <0x00000000f78f2ae0> (a org.apache.lucene.search.TestLRUQueryCacheCleanUpDeadlock$StatsCache)

# with the fix: 1 test, 0 failures (assertions on and off)

• The contract. "This class is thread-safe." (LRUQueryCache.java:64). The class javadoc invites overriding the callbacks "to have more fine-grained statistics" (:88-101). cleanUp(): "This method is safe to call concurrently." (:1384), and on main it must run (:103-113).
• Lucene's own migration example for #15558 is a class MyQueryCache extends LRUQueryCache that overrides onCacheEntryInserted and onCacheEntryEvicted, with no locking caveat (lucene/MIGRATE.md:514-542).
• It worked before. Since LUCENE-7237 (2016, Adrien Grand) the cache has locked only a private lock, a ReentrantReadWriteLock since #13306 (10.5.2 and 10.6.0 LRUQueryCache.java:100-101). In 2015 (LUCENE-6303, releases 5.1 to 6.0) the methods that read or change the cache's state (get, putIfAbsent, eviction, the clear* methods) were synchronized on the cache, its only lock and re-entrant, so no cycle was possible. No release has cleanUp().
• Nobody decided it. The #15558 PR body, its 22 issue comments and 5 review comments never discuss cleanUp()'s monitor, a lock order or locking in callbacks; #16212 changed cleanUp()'s locking and kept the modifier without comment.
• Standard Java guidance. For classes designed for inheritance, Effective Java (3rd ed., Item 82) recommends a private lock object, so that a subclass cannot "unintentionally interfere with the operation of the base class, or vice versa".

The strongest case: the callbacks are Expert / @lucene.experimental, and the class javadoc warns "It is better to not perform heavy computations in these methods though since they are called synchronously and under a lock" (LRUQueryCache.java:99-101), so a subclass that locks inside a callback does so at its own risk, and a maintainer may prefer a javadoc note to a code change. (The independent cross-model reviewer, OpenAI GPT-5.6, raised this case.) In #10081 (LUCENE-9038, 2019) Ben Manes read that contract as: the callbacks "are executed under the global lock. This means implementations may rely on this exclusive read/write access to data structures".

Why it fails: the sentence is advice against heavy work and names no lock or lock order; a counter update is not heavy. It dates from LUCENE-6157 (2015), when "the lock" was the cache's own re-entrant monitor, so a synchronized callback simply re-entered it. Ben Manes' reading is why subclasses never needed a lock before, and why they need one now that main runs callbacks concurrently. @lucene.experimental allows API changes, not a permanent hang. The cycle needs Lucene to reuse the subclass's public monitor as an internal lock, which the rendered javadoc does not even show. Verdict: not intended. A javadoc-only answer stays possible.

One fix was validated in a scratch worktree of 645d4fe14a20; the others are directions, not attempted. The cycle needs cleanUp() holding the public cache monitor and a callback that takes that monitor under a partition lock; Lucene controls the first.

One file, +10/-1 in LRUQueryCache.java: a private final Object cleanUpLock, and cleanUp() runs its unchanged body inside synchronized (cleanUpLock). Concurrent cleanUp() calls stay serialized, the modifier's apparent purpose, but Lucene no longer takes the cache object's monitor anywhere, so a subclass's monitor is a leaf lock again, as in every release since 6.1 (2016).

  private final Object cleanUpLock = new Object();

  public void cleanUp() {
    // A private lock, not this object's monitor: subclasses may synchronize on the cache in their
    // hit, miss, insertion and eviction callbacks, run under the partition locks cleanUp() takes.
    synchronized (cleanUpLock) {
      doCleanUp();
    }
  }

  private void doCleanUp() {
    // the former body of cleanUp(), unchanged
  }

Trade-offs

• No change to what the cache stores, evicts or reports, and cleanups stay serialized; the cleanup body is unchanged.
• One observable change: code that synchronized on the cache to keep cleanUp() out would no longer exclude it. That was never documented, and the rendered javadoc does not show the modifier; the issue can ask the maintainers whether anyone relies on it.
• Callbacks of different partitions still run at the same time (LFV-1000, a javadoc matter); subclasses still need their own thread-safety. With this fix, synchronizing on the cache object in a callback no longer deadlocks against cleanUp(); locks a subclass shares with other code remain its own concern.
• The comment in the diff was reworded after the fact check to name the hit, miss, insertion and eviction callbacks (onClear() runs with no partition lock, :458-463); the code is unchanged, compiles to byte-identical class files, and google-java-format 1.37.0 leaves the patched file unchanged.
• Land it with LFV-991's clear() fix. In the finder's TLA+ model, that fix alone (all partition locks held while onClear() runs) deadlocks with the synchronized cleanUp() (run M4); both fixes together are clean (run M3, 2,180,461 distinct states; its vacuity probe fails as required).
• Worth adding to the class javadoc (documentation only): which locks are held during each callback, and that the cache never synchronizes on itself.

What the checks validated

Scratch worktree of 645d4fe14a20, gradle --no-daemon, OpenJDK 25.0.3, seed DEADBEEF, 2026-10-31 (worktree removed afterwards):

• stock: the minimal test fails with assertions on and off;
• with the fix: it passes with assertions on and off (7.3 s once, against a 10 s bound; the lint-clean copy in 0.77 s and, with seed 5EED1234, 0.58 s; the fact check's gradle re-runs 0.78 s, 0.80 s and 3.7 s);
• closest suites TestLRUQueryCache, TestUsageTrackingFilterCachingPolicy, TestBooleanScorer, TestDocValuesOrdinalSetIterator: 88 tests, 0 failures, 3 skipped;
• the whole org.apache.lucene.search package (subpackages included): 214 classes, 2,339 tests, 0 failures, 49 skipped;
• checkGoogleJavaFormat, ecjLintMain, forbiddenApisMain pass (with the lint-clean test also ecjLintTest, forbiddenApisTest).

Earlier, the same diff: no hang in a 60 s stress (96,563,907 searches) and upstream TestLRUQueryCache 37 of 37. Not run: nightly tests, other modules, benchmarks.

Drop the modifier and let concurrent cleanUp() calls run in parallel, each on its own snapshot of the stale sets.

Trade-offs

Smaller in code, but whether two concurrent cleanups stay correct (RAM accounting, eviction callbacks fired once) needs its own analysis and tests; the private lock keeps today's serialization.

State that cleanUp() holds the cache object's monitor and that callbacks must not synchronize on the cache.

Trade-offs

No code change, but the most common Java idiom stays a permanent hang that no release had, and the modifier stays invisible in the rendered API docs.

Collect callback events under the lock and fire them after releasing it.

Trade-offs

Removes every lock-order hazard with callbacks, but changes the documented "under a lock" behaviour and the exclusive access subclasses may rely on (#10081); a much larger change.

Method: semi-formal (a hypothesis formed while modelling, then witnessed by execution).

• The model (LruPartClearMon.tla: the partitioned cache plus the cache monitor) deadlocks on main with a 4-state trace (run M1). It has no deadlock with cleanUp() on a private lock (M2, 491,278 distinct states) or without a monitor-synchronized subclass (M5, 1,237,688). The sibling clear() fix alone deadlocks (M4); both fixes together are clean (M3, 2,180,461), with a vacuity probe (M3v) that fails as required.
• The finder's three-case JUnit, an independent reproducer's six-case JUnit and two no-latch stresses failed on main; the minimal test here is the insertion case cut down to one method with one partition.