DirectoryReader.openIfChanged leaks segment reader references when the leaf sorter throws while the new reader is built

DirectoryReader.open(Directory, Comparator<LeafReader> leafSorter) (LUCENE-9507, since 8.9.0) orders the segments with an application comparator, and the reader keeps that sorter for its refreshes. A non-NRT openIfChanged (and also an NRT reader's refresh to an explicit IndexCommit) goes through the reopen factory StandardDirectoryReader.open(Directory, SegmentInfos, List, Comparator, ExecutorService). It first takes a reference on every segment reader it shares with the old reader and opens readers for new segments, then constructs the new reader with no try/catch. That constructor runs Arrays.sort with the sorter. If the sorter throws, openIfChanged throws too, no new reader is returned, and nothing releases those references: with two one-document segments the shared segment reader's refCount is 2 instead of 1 after the failed call, 1 instead of 0 after the old reader is closed, and the new segment's reader is never closed. The initial open and the NRT open catch any Throwable and release their readers; the reopen factory is the only open path with no cleanup at all. Results are unaffected and the sorter failure is loud; the leak is silent and repeats on every retry. A try/catch of 7 code lines, mirroring the initial open, fixes it. Every release from 8.9.0 to 10.6.0 and main is affected.

• Leaf sorter: the Comparator<LeafReader> given to DirectoryReader.open(Directory, Comparator) or a commit overload (LUCENE-9507). It is null by default. The reader keeps it as subReadersSorter and uses it for every refresh.
• Reopen factory: StandardDirectoryReader.open(Directory, SegmentInfos, List, Comparator, ExecutorService) (StandardDirectoryReader.java:174-184 at 645d4fe14a20), @lucene.internal, behind every openIfChanged of a reader that was not opened from an IndexWriter (also the ExecutorService and IndexCommit overloads), and behind openIfChanged(reader, IndexCommit) of an NRT reader.

1. openIfChanged(reader) reaches doOpenNoWriter (StandardDirectoryReader.java:477-494), doOpenFromCommit (:496-505) and doOpenIfChanged(SegmentInfos, ExecutorService) (:507-511), which calls the reopen factory with the reader's own sorter.
2. The factory calls createSegmentReaders (:181). For an unchanged segment, "this reader will be shared between the old and the new one, so we must incRef it": oldReader.incRef() (:317-323), refCount 1 to 2. A new segment gets new SegmentReader(...) (:292-296), refCount 1. createSegmentReaders releases these itself only when one of its own steps fails (:218-228, :241-244).
3. Then return new StandardDirectoryReader(directory, newReaders, null, infos, leafSorter, false, false); (:182-183) runs with no try/catch. Through DirectoryReader's constructor (DirectoryReader.java:539) it reaches BaseCompositeReader, which runs Arrays.sort(subReaders, subReadersSorter) (BaseCompositeReader.java:75-77): the application's comparator.
4. The comparator throws, openIfChanged throws, and the array newReaders is gone: the caller has no handle on the extra reference or on the new reader.
5. The initial open wraps the same constructor call: "it must be inside try clause so we close readers in that case", catch (Throwable t) { IOUtils.closeWhileSuppressingExceptions(t, readers); throw t; } (:96-105). The NRT open also catches Throwable and decRefs its readers (:128-165).

The new segment's reader _1 stays at refCount 1 and is never closed. On Lucene's MockDirectoryWrapper, closing the directory then fails with "cannot close: there are still 2 open files: {_1.cfs=1, _0.cfs=1}". Without a leaf sorter no application code runs in that constructor; its only other throw is the "Too many documents" check (BaseCompositeReader.java:89-98), which needs out-of-limit input and is not claimed.

• Who is hit. Readers opened with DirectoryReader.open(Directory, Comparator) (DirectoryReader.java:88-91), its ExecutorService overload (:106-110) or the commit overloads (:189-194, :218-226), refreshed with openIfChanged, SearcherManager(DirectoryReader, SearcherFactory) (SearcherManager.java:158; refreshIfNeeded calls openIfChanged, :194-205) or ReaderManager(DirectoryReader) (ReaderManager.java:82); a fact-check probe ran all three. Also NRT readers from an IndexWriter with IndexWriterConfig#setLeafSorter when they are refreshed to an explicit commit: openIfChanged(reader, IndexCommit) (DirectoryReader.java:282-301) goes to doOpenFromWriter, which for a non-null commit calls doOpenFromCommit and so the same reopen factory with the writer's leaf sorter (StandardDirectoryReader.java:155, :406-431, :456-460); a SearcherManager(IndexWriter, ..., RefreshCommitSupplier) (SearcherManager.java:114-131) takes this route whenever its supplier returns a commit (:199-200). In the fact-check probe the segment files stay open after the failed refresh, the NRT reader and the writer are closed ("cannot close: there are still 2 open files: {_1.cfs=1, _0.cfs=1}"); a sorter that does not throw leaves nothing open, and the fix covers this route too. There the old segment readers are NRT readers, so the factory opens new readers that share their cores (:298-315): the symptom is the open files, not the old reader's refCount.
• Default vs opt-in. The leaf sorter is an expert option, null by default. Plain NRT refreshes (openIfChanged(reader) or openIfChanged(reader, writer) on an NRT reader, or a SearcherManager whose refresh commit supplier returns null, the default) go through the NRT open, which releases its readers, so this leg does not hit them.
• What triggers it. A sorter that throws during a refresh: a comparator bug on a segment that lacks the sort field (for example a timestamp sorter that dereferences getPointValues("ts"), which is null for such a segment, LeafReader.java:510-514), or an I/O error inside a correct sorter: Comparator.compare cannot throw checked exceptions, while per-segment readers such as getNumericDocValues declare IOException (LeafReader.java:175), so such a sorter that propagates the error must wrap it in an unchecked exception.
• How much. Per failed refresh: one extra reference on every segment reader shared with the old reader (every unchanged segment) and one open reader per new segment. Lucene never releases them: after the old reader is closed the shared reader stays at refCount 1 and the segment files stay open. It grows linearly with failed attempts and happens at any index size; one failed call leaks once (the commit lookup retries only after an IOException, SegmentInfos.java:884). Whether garbage collection later reclaims the file handles was not measured.
• What does not happen. No wrong results, no index change, no hang. The refresh fails loudly with the sorter's exception: SearcherManager.maybeRefresh() rethrows it to its caller and keeps serving the previous searcher (executed), and the old reader keeps working.
• Related legs, not part of this report (found by the first finder on 645d4fe14a20): the NRT open with IndexWriterConfig#setLeafSorter keeps the IndexFileDeleter references it took (StandardDirectoryReader.java:148) when the constructor throws, so files of merged-away segments stay until the writer closes; and the initial open's cleanup iterates the array that Arrays.sort was rearranging, so a late sorter failure in a TimSort merge (32+ segments) leaks some readers ("still 4 open files").

The smallest input: two segments are the fewest that make the sorter run, and a sorter that always throws needs no field or data. The single assertion is the reference count the failed call should have given back; the same test is the regression test. Assertions on and off give the same failure. All runs were on Linux aarch64 with OpenJDK 25.0.3; the bug is plain control flow, so the platform does not matter.

# gradle, stock 645d4fe14a20, seed DEADBEEF, assertions on
TestReopenSorterFailureRefCount > testFailedLeafSortDuringReopenReleasesSharedSegment FAILED
    java.lang.AssertionError: expected:<1> but was:<2>
        at org.apache.lucene.index.TestReopenSorterFailureRefCount.testFailedLeafSortDuringReopenReleasesSharedSegment(TestReopenSorterFailureRefCount.java:43)

# assertions off (-Ptests.asserts=false): the same failure
# 10.6.0 release jars, assertions off: java.lang.AssertionError: expected:<1> but was:<2>

# with the fix, assertions on and off
classes=1 tests=1 failures=0 errors=0 skipped=0

# closest suites (TestDirectoryReader, TestDirectoryReaderReopen, TestIndexWriterReader, TestIndexReaderClose, TestReaderClosed, TestSearcherManager), seed DEADBEEF
stock:    classes=6 tests=104 failures=0 errors=0 skipped=1
with fix: classes=6 tests=104 failures=0 errors=0 skipped=1

# a second test on MockDirectoryWrapper (sorter dereferencing missing points), after reader.close():
# shared segment reader refCount=1 (expected 0), directory close: MockDirectoryWrapper: cannot close: there are still 2 open files: {_1.cfs=1, _0.cfs=1}

• Ownership contract. openIfChanged (DirectoryReader.java:236-243): "The provided reader is not closed (you are responsible for doing so); if a new reader is returned you also must eventually close it."
• Reference counting. IndexReader.incRef (IndexReader.java:164-174): "Be sure to always call a corresponding {@link #decRef}, in a finally clause; otherwise the reader may never be closed."
• The same class. The initial open (StandardDirectoryReader.java:96-105), the NRT open (:128-165) and createSegmentReaders (:218-228, :241-244) all release their readers on failure (the two related legs under "Who is affected" are remaining gaps of the first two). An independent verifier's control: the initial open with the same throwing sorter over two segments leaves no file open.
• Tests. TestDirectoryReaderReopen.testOverDecRefDuringReopen (:673-735) makes openIfChanged fail with a FakeIOException, checks that the old reader still works and closes its MockDirectoryWrapper, which fails on files left open. No test makes a leaf sorter throw.
• History. LUCENE-5262 (#6326, 2013, "StandardDirectoryReader should decRef readers on exception, not close them") made the NRT open exception-safe; LUCENE-6299 (#7360, 2015; 4e53fae38e9 on its branch, 33fad60de91 on trunk) guarded the initial open only; LUCENE-9507 (48715fe898bf, 2021, #32) put the user callback into the constructor and passed it to the reopen factory's unguarded call. None of the 69 comments and reviews on LUCENE-9507, #32, apache/lucene-solr#2256 and #2473 mentions exceptions.

The strongest case for "intended" (the devil's-advocate verifier and the intended-lens reviewer): the exception comes from the application's own comparator, so "fix your comparator"; nothing documents exception safety for callbacks; the factory is @lucene.internal (StandardDirectoryReader.java:168-173).

Why it fails: the path is public and untagged (DirectoryReader.open(Directory, Comparator), then openIfChanged); Comparator.compare may throw, and no javadoc of the leaf-sorter overloads or of IndexWriterConfig.setLeafSorter (IndexWriterConfig.java:517-527) asks for a sorter that never throws; a correct sorter that reads doc values and propagates an I/O error must wrap it in an unchecked exception; and Lucene already cleans up after this very failure on the initial open. The argument explains the Low priority, not the leak. Reviewers rated it weak to moderate.

The hunt's diff, unchanged, run through gradle in a scratch worktree of 645d4fe14a20 (OpenJDK 25.0.3, seed DEADBEEF) with the minimal repro and the closest suites.

In StandardDirectoryReader.open(Directory, SegmentInfos, List, Comparator, ExecutorService) (StandardDirectoryReader.java:181-183), wrap the constructor call in try { ... } catch (Throwable t), pass newReaders.clone() to the constructor, and on failure call decRefWhileSuppressingException(t, newReaders) (:364-374, already used by createSegmentReaders) and rethrow. A shared reader goes back from 2 to 1 (the old reader keeps its own reference); a new reader goes from 1 to 0 and closes.

     SegmentReader[] newReaders = createSegmentReaders(infos, oldReaders, executor);
-    return new StandardDirectoryReader(
-        directory, newReaders, null, infos, leafSorter, false, false);
+    try {
+      // The constructor sorts the array it is given in place (leaf sorter), so pass a copy and
+      // release from the original if it throws:
+      return new StandardDirectoryReader(
+          directory, newReaders.clone(), null, infos, leafSorter, false, false);
+    } catch (Throwable t) {
+      decRefWhileSuppressingException(t, newReaders);
+      throw t;
+    }

Trade-offs

• Why the copy. The cleanup must see every reader exactly once, and Arrays.sort rearranges the array it is given. A probe on JDK 25: when the comparator throws mid-sort, arrays of up to 31 elements stay a permutation (insertion sort), but from 32 elements on (TimSort merges) they can hold duplicates and miss elements, up to 7 of each at 32 elements and 15 at 64. Without the copy, the cleanup could release a shared reader twice and miss others.
• Cost. One array copy per refresh; no API or behaviour change when the sorter succeeds.
• Also covers. Each original entry is decRef'd exactly once, so the readers are also released when the same constructor throws "Too many documents", and the NRT-reader-to-commit route passes the fact-check probe with the fix.
• Scope. Fixes the reopen leg only; the NRT deleter leg and the initial open's late-TimSort leg need the same treatment separately.

What the checks validated

Through gradle in a scratch worktree of 645d4fe14a20 (OpenJDK 25.0.3, seed DEADBEEF): the minimal test fails on stock and passes with the fix, assertions on and off. The closest suites (TestDirectoryReader, TestDirectoryReaderReopen, TestIndexWriterReader, TestIndexReaderClose, TestReaderClosed, TestSearcherManager: 6 classes, 104 tests) pass on stock and with the fix (1 skipped both times); none of them makes a sorter throw. google-java-format 1.37.0 leaves the fix's lines and the test unchanged. Earlier, the hunt's shadow check, an independent verifier's controls and the refute reviewer's run (57 of 57 tests of TestDirectoryReader and TestIndexWriterReader) passed with the same diff.

Also pass a copy of the readers array in the initial open (its cleanup iterates the array being sorted) and, in the NRT open, release the deleter references taken by writer.incRefDeleter(segmentInfos) (StandardDirectoryReader.java:148) when the constructor throws. Not run.

Trade-offs

Fixes the two related legs as well, but it is three changes needing three tests, two of them (32+ segments; deleter state) harder to keep minimal.

Catch and decRef newReaders directly. Not recommended.

Trade-offs

Correct below 32 segments, but a late sorter failure on a larger reader leaves the array with duplicates and gaps, so the cleanup could release a shared reader twice, closing it under the old reader, and miss others.

Method: formal, TLA+ model checking with TLC (a hunt unit, 2026-10-26), on a model of IndexReader reference counting and the StandardDirectoryReader open and reopen factories built from the code with a file:line map.

• With the ctorThrow fault (the StandardDirectoryReader constructor throws, for example from the leaf sorter), the NoLeak invariant fails in the sequential and executor configurations (27 and 137 distinct states); the same configurations with the modelled fix (decRef the new readers when the constructor throws) pass.
• The fault edge was added while modelling the code, so a strict reading could call the discovery semi-formal.
• Found independently the same day by another hunt unit's static exception-edge resource scan: the SegmentReader[] from createSegmentReaders is live on the exception edge of new StandardDirectoryReader(...) with no handler.
• A later hunt unit re-ran the model on 645d4fe14a20 (violation without the fix, pass with it) and reduced the counterexample to the one-test repro.