SegmentOrder: two concurrent reorder() calls on one shared instance can throw NullPointerException

SegmentOrder.fromSort(sort) (new in 10.4.0) returns a helper whose reorder(reader) gives a view of the reader with its segments ordered so that the best hits for the sort come first. For an INT, LONG, FLOAT or DOUBLE primary sort, numericSorter() builds one NumericFieldReaderContextComparator (SegmentOrder.java:91-92 at 5adff78bb168) and every reorder() hands that same instance to the view's constructor (:96-98), which sorts the leaves with it (BaseCompositeReader.java:76). The comparator caches each segment's sort value in a plain HashMap (:115) through containsKey, put and get with no lock, and unboxes the result (:138-141). When two threads call reorder() on the same instance and meet segments that are not cached yet, a get() can return null, and reorder() throws NullPointerException. The one-test repro fails through gradle on main in 20 of 20 iterations with assertions on and 19 of 20 with them off; building the comparator inside reorder() (+3/-2 lines) removes it. No wrong results, index damage or hang were seen. Opt-in helper, no known caller outside Lucene's own test; priority Low.

1. fromSort with an INT, LONG, FLOAT or DOUBLE primary sort (or a SortedNumericSortField of one) returns numericSorter(...) (SegmentOrder.java:42-77). Other sort types return a no-op strategy (:79-85), which is not affected.
2. numericSorter creates one NumericFieldReaderContextComparator outside reorder() (:91-92); every reorder() passes it to new BaseCompositeReader<>(leaves, comparator) (:95-98), whose constructor runs Arrays.sort with it on the calling thread (BaseCompositeReader.java:74-76).
3. The comparator's getSortValue (:136-142) caches values in private final Map<Object, Long> cachedSortValues = new HashMap<>() (:115) keyed by the leaf's core cache key: if (containsKey(key) == false) put(key, loadSortValue(reader)); then return cachedSortValues.get(key); (:141), unboxed to long. No lock and no eviction.
4. Two threads that fill one HashMap can lose or hide entries, or corrupt its structure (JDK 25 HashMap.java): an empty bucket is written with a plain store (tab[i] = newNode(...), :638), so one of two concurrent puts into it can be lost; and resize() publishes the new, empty table (table = newTab, :712) before it moves the old buckets (:714-717), so a get() meanwhile can miss a key just put. Either way :141 unboxes null.
5. Every thread computes the same value for a given segment (loadSortValue, :144-172, reads that segment's own doc-values skipper or point min/max), so a lost or hidden entry shows up as the NullPointerException: that is the failure in all but one of the failed iterations recorded for this bug. The race can also corrupt the map's internal structure. In the fact check's re-run of the gradle command (assertions on), 1 of 20 failures was an AssertionError from HashMap's own tree-bin check (HashMap.java:2009, under the put at SegmentOrder.java:139). A probe that kept calling reorder() from one thread on an instance that had lost the race saw later calls fail too, in 5 of 670 failed rounds (three NullPointerExceptions in a row, then success; once a ClassCastException inside HashMap.TreeNode, HashMap.java:1995). Every successful call returned the correct order (897 race calls, 3,587 later calls) and none hung, so no wrong order, wrong hits or hang is claimed.

1,000 one-document segments with f = 0 ... 999 and one descending LONG sort: one thread, or the same instance reused serially, gets f = 999, 998, ..., 0. Two threads at once: 20 of 20 rounds fail with assertions on and 19 of 20 with them off (gradle, main). With default settings, 1,000 puts grow the map 7 times, from 16 to 2,048 buckets, so each reorder() over 1,000 new segments has many windows.

• Who is hit. Applications that cache one SegmentOrder (for example one per Sort, like a cached Sort or Query) and call reorder() from several search threads.
• When. The map is written only for segment cores the instance has not cached yet (SegmentOrder.java:138-139), so the risk is while concurrent calls insert new cores: mostly the first concurrent searches on a newly opened or refreshed reader, whenever a flush or merge adds segments. A call whose own segments are all cached can still fail if another thread is inserting at that moment, and after a lost race a few later calls on that instance can fail too, even from one thread (fact-check probe).
• Effect. The failing reorder() call throws (almost always NullPointerException), so that search fails. Loud; no index damage, and no wrong results were seen.
• Default vs opt-in. SegmentOrder is opt-in; nothing in Lucene calls it outside TestSegmentReordering. Once used, the default IndexWriterConfig, codec and directory suffice, with assertions on or off. A GitHub code search found no downstream caller (it indexes only part of GitHub).
• Scale. The repro's 1,000 one-document segments only widen the window. It is a two-thread race, so real hit rates depend on how many new segments a call meets and on timing.

TestSegmentOrderConcurrentReorder (one method, extends LuceneTestCase, package org.apache.lucene.search of the lucene/core tests, next to TestSegmentReordering). A CyclicBarrier releases the two tasks together; Future.get() rethrows the failure.

Concurrency is the bug, so the test has two threads. It is probabilistic, and no deterministic public-API repro was found: IndexReader.CacheKey is a final class with identity hashing, and no user code runs inside the HashMap calls. About half of cold single runs fail (5 of 9 with assertions on, 5 of 8 off, standalone runner); 20 iterations make a failure near-certain. The 1,000 segments only widen the window: the same shape with 40 segments failed 120 of 200 warm iterations and 2 of 10 cold single runs (assertions off), so a smaller index also works with 20 iterations.

Suggested regression test: this same test. It passes checkGoogleJavaFormat, ecjLintTest and forbiddenApisTest. It differs from the hunt's minimal repro only in a shorter comment and a named thread factory (the forbidden-API rules reject Executors.newFixedThreadPool(int)), and was re-run as shown here.

Under gradle's default test JVM flags every failure carried the message; with a standalone runner, later failures in one JVM lose it (the JIT's fast-throw) and show a bare NullPointerException.

cp $REPRO/TestSegmentOrderConcurrentReorder.java $LUCENE/lucene/core/src/test/org/apache/lucene/search/
cd $LUCENE && ./gradlew :lucene:core:test --tests TestSegmentOrderConcurrentReorder -Ptests.iters=20
# also with -Ptests.asserts=false

TestSegmentOrderConcurrentReorder > testConcurrentReorderOnSharedSegmentOrder > testConcurrentReorderOnSharedSegmentOrder[seed=[1187B001:13A24C3AEC58A3F]] FAILED
    java.util.concurrent.ExecutionException: java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because the return value of "java.util.Map.get(Object)" is null
        at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:124)
        at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:193)
        at org.apache.lucene.search.TestSegmentOrderConcurrentReorder.testConcurrentReorderOnSharedSegmentOrder(TestSegmentOrderConcurrentReorder.java:66)
        Caused by:
        java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because the return value of "java.util.Map.get(Object)" is null
            at org.apache.lucene.index.SegmentOrder$NumericFieldReaderContextComparator.getSortValue(SegmentOrder.java:141)
            at org.apache.lucene.index.SegmentOrder$NumericFieldReaderContextComparator.compare(SegmentOrder.java:132)
            at org.apache.lucene.index.SegmentOrder$NumericFieldReaderContextComparator.compare(SegmentOrder.java:113)
            at java.base/java.util.TimSort.countRunAndMakeAscending(TimSort.java:356)
            at java.base/java.util.TimSort.sort(TimSort.java:234)
            at java.base/java.util.Arrays.sort(Arrays.java:1230)
            at org.apache.lucene.index.BaseCompositeReader.<init>(BaseCompositeReader.java:76)
            at org.apache.lucene.index.SegmentOrder$2$1.<init>(SegmentOrder.java:98)
            at org.apache.lucene.index.SegmentOrder$2.reorder(SegmentOrder.java:97)
            at org.apache.lucene.search.TestSegmentOrderConcurrentReorder.lambda$testConcurrentReorderOnSharedSegmentOrder$0(TestSegmentOrderConcurrentReorder.java:59)
...
20 tests completed, 20 failed

# with -Ptests.asserts=false (seed 1187B002): 20 tests completed, 19 failed (the same exception and stack)

• The API looks stateless. fromSort: "Builds a SegmentSorter that will reorder segments based on a given numeric Sort" (SegmentOrder.java:36-41); reorder: "Produces a new view over an IndexReader by re-ordering the reader's segments" (:33-34). No word on threads, reuse or a cache anywhere in the file.
• Lucene documents the opposite case explicitly. StoredFields.java:29-30 ("This class is not thread-safe and should only be consumed in the thread where it was acquired"), HnswGraphBuilder.java:45 ("This class is NOT thread safe, it cannot be shared across threads"); a verifier counted 24 such notes in core. SegmentOrder is built from a Sort, which is immutable (lucene/MIGRATE.md:1638, "Sort is immutable (LUCENE-9325)"), and it carries no such note.
• The design suggests reuse. The cache key is reader.getCoreCacheHelper().getKey() (:137), stable across reader reopens, and the cache is owned by the instance. A per-call cache would still serve the many comparisons inside one sort; keeping it on the instance, rather than per call, pays off only across calls. #15436 (closed) built a comparator per search with a cache keyed by LeafReaderContext.ord; #15591's first commit moved the comparator up into numericSorter and switched to the core key. No review comment on either PR discusses threads, sharing or the cache's lifetime.
• The fix is expected to be cheap, but it is not benchmarked. On main, since #16388 (1593f87193b), a segment's skipper and point min/max are available without IO (:144-172); fix A gives up only the reuse of sort values across calls. For context, in #15436 (a comparator per search) its author measured a fixed per-search overhead of the whole re-sorting path (TermDTSort -37.2% QPS) and wrote "Given the fixed overhead of segment sorting, I think having this as something that happens automatically isn't viable".

Strongest case for "intended": no document promises thread safety, Java's usual rule is that an undocumented class is not thread-safe, the PR title says "per-query segment re-ordering", CHANGES says "re-order segments at query time" (lucene/CHANGES.txt:1426), and the only caller (TestSegmentReordering.java:382) builds one per use. The cross-model reviewer stopped at "uncertain" for this reason.

Why it does not settle it: "per query" says when the application chooses to reorder, not how long the object lives; the object exposes no state, so a user has no hint that sharing is unsafe; and keeping the cache on the instance, rather than per call, only pays off across calls. A caveat against the bug reading: the core key may simply have been forced by the comparator receiving LeafReaders (no ord available), so it is weak evidence of intended reuse; the case rests on the API shape. Hence medium confidence and Low priority.

Option A was applied to 5adff78bb168 in a fresh scratch worktree and run through gradle. B, C and D were not run. None of A, B or D fixes LFV-296 (same method, a null core cache helper); C would fix both.

Move the new NumericFieldReaderContextComparator(...) of numericSorter (SegmentOrder.java:91-92) into the returned reorder(), so each call sorts with its own comparator and cache.

  private static SegmentOrder numericSorter(
      String field, Long missingValue, boolean reverse, ToLongFunction<byte[]> pointDecoder) {
    return new SegmentOrder() {
      @Override
      public IndexReader reorder(IndexReader reader) throws IOException {
        // a fresh comparator (and sort-value cache) per call: the strategy holds no mutable state
        NumericFieldReaderContextComparator comparator =
            new NumericFieldReaderContextComparator(field, missingValue, reverse, pointDecoder);
        return new BaseCompositeReader<>(

Trade-offs

Each reorder() loads the sort value once per distinct segment core it sees (a skipper or point min/max read) instead of reusing values from earlier calls; expected to be cheap on main, but no benchmark was run. Each call's map is then used only by the calling thread (Arrays.sort is sequential), so the race cannot occur. It also ends the cache's growth across calls (one small entry per segment core ever seen; not claimed as a bug). It does not touch the getCoreCacheHelper().getKey() call (:137), so LFV-296 stays.

What the checks validated

• Gradle, fresh scratch worktree of 5adff78bb168 + the test, 2026-11-03: stock, the test fails 20 of 20 (assertions on) and 19 of 20 (off).
• + fix: the test passes 20 of 20 (on) and 50 of 50 (off); TestSegmentReordering (3 tests) passes 60 of 60 and 150 of 150 runs; checkGoogleJavaFormat, ecjLintMain, ecjLintTest, forbiddenApisMain and forbiddenApisTest on lucene/core pass.
• Earlier, the same change shadowed into a standalone runner: 0 failures in 50 (finder), 50 (independent reproducer), 30 (cross-model reviewer), 20 (a separate re-run), and 20 + 20 (minimal repro, assertions on and off) iterations.
• Not run: the full core suite, nightly tests, a benchmark.

ConcurrentHashMap plus computeIfAbsent(key, k -> loadSortValue(reader)).

Trade-offs

Keeps reuse across calls; loadSortValue does not touch the map, so no recursive update. Keeps the never-evicted growth. Not run.

What the checks validated

Not run.

Inside reorder(), cache by LeafReader identity (an IdentityHashMap) or compute every leaf's value into an array before sorting.

Trade-offs

Removes the race and never calls getCoreCacheHelper(), so it would also fix LFV-296 in one change; a larger change than A. Not run.

What the checks validated

Not run.

State on SegmentOrder that an instance is not thread-safe and should be created per query.

Trade-offs

No code change, but it keeps a trap in an API that looks stateless.

What the checks validated

Not run.

Method: semi-formal (a proof-obligation audit of a class's contract, witnessed by an executable repro), on main 5adff78bb168, 2026-11-03.

• The obligation: a strategy from fromSort() exposes no state, so each reorder(reader) should be a function of reader alone. numericSorter (SegmentOrder.java:89-111) breaks it: one comparator with a mutable, unsynchronized cache is captured by every call.
• The finder wrote a public-API JUnit test (two readers of 400 one-document segments, one shared SegmentOrder, two threads) that fails in most iterations on main, 10.6.0 and 10.4.0, and the per-call-comparator change that makes it pass.
• An independent reproducer, a cross-model reviewer, a devil's-advocate verifier and an adjudication re-ran it or wrote their own tests ("Prior reports, history and checks"). The adjudication chose the shared-reader shape as the one repro.