BPIndexReorderer leaks the term-ids temp input when opening the start-offsets temp file fails

BPIndexReorderer (opt-in, lucene/misc) computes its doc-id order from a temporary on-disk forward index of two files, term-ids and start-offsets. buildForwardIndex writes both, then opens both for reading and builds the ForwardIndex with three plain statements outside any try (BPIndexReorderer.java:768-770 on main 645d4fe14a20). If the second openInput throws, the term-ids input opened one statement earlier is lost with the exception; if the ForwardIndex constructor throws, both are lost. The caller's cleanup only deletes the temp files. So a failed reorder, or a failed merge through BPReorderingMergePolicy, leaves an IndexInput open on a deleted temp file, even after IndexWriter.close() (Linux runs): a file descriptor until garbage collection on NIOFSDirectory, and a mapping that survives Directory.close() and System.gc() on MMapDirectory. A failed IndexWriter.addIndexes(CodecReader...) through the same merge policy leaves the writer open, so each retry leaks one more input. Every release from 9.8.0 to 10.6.0 and main has it. It needs an I/O failure at that exact open, so it is rare, and the harm is a leak only: no wrong results and no index damage.

// BPIndexReorderer.buildForwardIndex, condensed
try (IndexOutput termIDs = tempDir.createTempOutput("term-ids", "", IOContext.DEFAULT);          // :715
     IndexOutput startOffsets = tempDir.createTempOutput("start-offsets", "", IOContext.DEFAULT)) { // :716-717
  ... sort the postings and write both files ...
}
IndexInput termIDsInput = tempDir.openInput(termIDsFileName, IOContext.DEFAULT);       // :768  opened
IndexInput startOffsets = tempDir.openInput(startOffsetsFileName, IOContext.DEFAULT);  // :769  throws: :768 is lost
return new ForwardIndex(startOffsets, termIDsInput, maxTerm);                          // :770  throws: both lost

// BPIndexReorderer.computePermutation, condensed
try {
  ...
  try (ForwardIndex forwardIndex = buildForwardIndex(trackingDir, postingsName, maxDoc, numTerms)) { // :871-872
    ...
  }
} catch (Throwable t) {
  IOUtils.deleteFilesSuppressingExceptions(t, trackingDir, trackingDir.getCreatedFiles());  // :900 deletes, never closes
  throw t;
}

1. buildForwardIndex writes the two temp files in a try-with-resources (:715-766), then opens both for reading (:768, :769) and passes them to the ForwardIndex constructor (:770), with no try around the three statements.
2. Directory.openInput may throw ("@throws IOException in case of I/O error", Directory.java:142). When :769 throws, termIDsInput exists only as a local variable; the exception leaves the method and nothing closes it.
3. The ForwardIndex constructor calls randomAccessSlice and rethrows an IOException as UncheckedIOException (:613-624). If it throws, both inputs are lost. Its close() would close both (:647-650), but the object never exists. This path needs a failure on a file written a moment earlier, so it is rare.
4. computePermutation takes ownership in a try-with-resources only after buildForwardIndex returns (:871-872). Its catch (Throwable) deletes every tracked temp file (:899-902), so the open input now points at a deleted file (on Linux; where the OS refuses to delete an open file, as Windows can, FSDirectory keeps it as a pending delete, FSDirectory.java:355-366, and the file stays on disk).
5. Merge path: BPReorderingMergePolicy calls reorderer.computeDocMap(reader, dir, executor) and catches only NotEnoughRAMException (BPReorderingMergePolicy.java:133-145); IndexWriter passes its own directory (IndexWriter.java:5558); the merge exception ends in tragicEvent(t, "merge") (IndexWriter.java:5109-5113), which closes the writer. The writer never tracked the leaked input, so IndexWriter.close() cannot release it. IndexWriter.addIndexes(CodecReader...) reorders through the same merge policy (BPReorderingMergePolicy.java:200-203, IndexWriter.java:3562), but there the exception goes back to the caller without a tragic event (tragicEvent only for an Error, IndexWriter.java:3390-3392), so the writer stays open.
6. Why the mapping outlives Directory.close() on MMapDirectory (by reading): temp names like term-ids__1.tmp do not match IndexFileNames.CODEC_FILE_PATTERN (IndexFileNames.java:199), so the default GROUP_BY_SEGMENT grouping gives no group and each input gets its own Arena.ofShared() (MMapDirectory.java:124-127, :192, :402-407), which only the input's close() frees. The input is unreachable, so the mapping stays until the JVM exits; the runs show it after Directory.close() and System.gc().

The same class does it right elsewhere: ForwardIndexSorter opens every temp input in try-with-resources (:1094, :1104-1105).

• Who. Applications that opt into BP reordering: BPReorderingMergePolicy with a BPIndexReorderer (merge time), or direct calls to BPIndexReorderer.reorder / computeDocMap. Not the default configuration.
• When. Mainly when the I/O fails at one exact point: opening the second forward-index temp file, which Lucene wrote a moment earlier (a failing randomAccessSlice in the ForwardIndex constructor is a second, rarer point). Realistic causes: file-descriptor exhaustion (NIOFSDirectory), address-space or mapping-count limits (MMapDirectory), a storage error, or a faulty custom Directory. Every run injected the IOException; no public report was found (the code dates from 9.8.0, 2023).
• What leaks. One IndexInput per failed reorder (two on the constructor path), on a deleted temp file: a descriptor until garbage collection on NIOFSDirectory; a mapping on MMapDirectory (seen after Directory.close() and System.gc(); until the JVM exits by reading). On Linux either one keeps the deleted file's disk blocks allocated (file-system semantics, not measured).
• Silent. Lucene's production code does not report the leaked input (only the test framework's MockDirectoryWrapper tracks open files), and on Linux the directory listing no longer shows the deleted file.
• Scale. No scale is needed to trigger it: the repro uses 1 document. A leaked term-ids file grows with the merge (it holds each document's qualifying term ids); how large it gets was not measured.
• Accumulation. A failed merge closes the IndexWriter (tragic event) and Lucene does not retry it, so on the merge path each failed merge leaks one input (two), and only merges already running when the writer closes can add to that (reasoning). A failed IndexWriter.addIndexes(CodecReader...) through BPReorderingMergePolicy does not close the writer, so an application that retries it leaks one input per failed call (executed: 3 failed calls on one writer, with a serial and with a concurrent merge scheduler, left 3 inputs open after IndexWriter.close(); 0 with fix A). An application that keeps reopening writers while a descriptor or mapping shortage lasts would also pile them up (reasoning, not measured).
• Not affected. Search results, index contents, deletes and commits: the exception comes before the merged segment is written, and the temp files are deleted.

One test class, one method, single thread, 1 empty document; the injected failure does not depend on the seed, and the test fails the same way with both seeds run. The only subclass is the test framework's MockDirectoryWrapper, which throws from openInput for start-offsets* names and counts open files. With no indexed field no term qualifies, but computePermutation still writes the postings file and calls buildForwardIndex, which writes both temp files and then opens term-ids and start-offsets, so the index content does not matter.

The assertion reports a count. A diagnostic copy that calls tempDir.close() instead names the open file and where it was opened: "cannot close: there are still 1 open files: {term-ids__1.tmp=1}", caused by "unclosed IndexInput: term-ids__1.tmp" opened at BPIndexReorderer.buildForwardIndex(BPIndexReorderer.java:768). (The FilterDirectory frame there is the TrackingDirectoryWrapper that computePermutation wraps around the temp directory.)

This same test is the suggested regression test: it fails on stock main and passes with either fix.

Earlier tests of the hunt also showed the merge path: forceMerge through BPReorderingMergePolicy(LogDocMergePolicy, BPIndexReorderer) with the same injected failure throws, and after IndexWriter.close() the index directory still has {term-ids__5.tmp=1} open.

# gradle, scratch worktree of 645d4fe14a20, seed DEADBEEF, -Ptests.asserts=true (some frames cut)
TestBPIndexReordererOpenFailureLeak > testFailedStartOffsetsOpenClosesTermIdsInput FAILED
    java.lang.AssertionError: expected:<0> but was:<1>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:B35574837A23A9E5]:0)
        at junit@4.13.2/org.junit.Assert.assertEquals(Assert.java:633)
        at org.apache.lucene.misc.index.TestBPIndexReordererOpenFailureLeak.testFailedStartOffsetsOpenClosesTermIdsInput(TestBPIndexReordererOpenFailureLeak.java:56)

1 test completed, 1 failed
# the same with -Ptests.asserts=false

# diagnostic copy of the test (tempDir.close() in place of the count assertion), 645d4fe14a20, seed DEADBEEF
java.lang.RuntimeException: MockDirectoryWrapper: cannot close: there are still 1 open files: {term-ids__1.tmp=1}
    at org.apache.lucene.tests.store.MockDirectoryWrapper.close(MockDirectoryWrapper.java:884)
    ...
Caused by: java.lang.RuntimeException: unclosed IndexInput: term-ids__1.tmp
    ...
    at org.apache.lucene.tests.store.MockDirectoryWrapper.openInput(MockDirectoryWrapper.java:843)
    at org.apache.lucene.misc.index.TestBPIndexReordererOpenFailureLeak$1.openInput(TestBPIndexReordererOpenFailureLeak.java:47)
    at org.apache.lucene.store.FilterDirectory.openInput(FilterDirectory.java:101)
    at org.apache.lucene.misc.index.BPIndexReorderer.buildForwardIndex(BPIndexReorderer.java:768)
    ...

• IndexWriter.close(): "Closes all open resources and releases the write lock." (IndexWriter.java:1342). On the merge path the input was opened by the writer's own merge on the writer's own Directory (IndexWriter.java:5558).
• The class's own failure handling. computePermutation wraps the whole computation in a catch (Throwable) that deletes its temp files (BPIndexReorderer.java:899-902); ForwardIndexSorter opens every temp input in try-with-resources (:1094, :1104-1105). Before #14913 (2025-07, main only) the failure branch called IOUtils.closeWhileHandlingException(postingsOutput, forwardIndex): the authors meant to close the forward index on failure, but forwardIndex was still null when buildForwardIndex threw, so this window was never covered.
• The contract Lucene holds every component to. CHANGES 3.3.0, LUCENE-3147/3152: "Fixed open file handles leaks in many places in the code. Now MockDirectoryWrapper (in test-framework) tracks all open files, including locks, and fails if the test fails to release all of them." That tracking is the repro's oracle.
• The trigger is in contract. Directory.openInput: "@throws IOException in case of I/O error" (Directory.java:142). The test framework ships MockDirectoryWrapper.setRandomIOExceptionRateOnOpen to test exactly this, and CHANGES 4.5.1, LUCENE-5263, treats "Transient IOExceptions, e.g. due to disk full or file descriptor exhaustion" inside IndexWriter as bugs.
• Nothing sanctions the leak. IndexReorderer.computeDocMap says only "temporary files may be stored here while reordering"; MergePolicy.OneMerge.reorder says the Directory "may be used to create temporary files"; TestBPIndexReorderer and TestBPReorderingMergePolicy inject no I/O failure; no CHANGES or MIGRATE entry mentions it.
• History. The three lines came with the class in #12489 and were never revisited; the PR's discussion is about quality and benchmarks.

The strongest case for "works as designed" (the intended-behaviour verifier, a cross-model reviewer and the adjudication): computeDocMap is an Expert API and the caller supplies the temp Directory; no javadoc promises failure atomicity; an openInput failure on a file just written is artificial; the constructor path is theoretical; and the impact is small.

Why it fails: the javadoc calls computeDocMap Expert because it exists for BPReorderingMergePolicy (BPIndexReorderer.java:773-777), and that recommended path reaches the same lines with IndexWriter's own Directory. The contract comes from the class's own cleanup, IndexWriter.close() and the test framework's open-file rule. openInput declares IOException, and descriptor and mapping exhaustion are real. The constructor path is indeed rare, which is why the second openInput is the headline; that narrows the claim, not its validity. Small impact is a severity question, and the bug is classed minor. A reviewer's objection that the finder's test subclasses the @lucene.internal FilterDirectory does not apply to the minimal repro, which overrides the test framework's MockDirectoryWrapper.openInput.

A and B change only the last three statements of buildForwardIndex; the success path is unchanged. C is a larger change. A and B were run through gradle in a scratch worktree of 645d4fe14a20; C was not tried.

In BPIndexReorderer.buildForwardIndex (BPIndexReorderer.java:768-770), +10/-3.

-    IndexInput termIDsInput = tempDir.openInput(termIDsFileName, IOContext.DEFAULT);
-    IndexInput startOffsets = tempDir.openInput(startOffsetsFileName, IOContext.DEFAULT);
-    return new ForwardIndex(startOffsets, termIDsInput, maxTerm);
+    IndexInput termIDsInput = null;
+    IndexInput startOffsets = null;
+    try {
+      termIDsInput = tempDir.openInput(termIDsFileName, IOContext.DEFAULT);
+      startOffsets = tempDir.openInput(startOffsetsFileName, IOContext.DEFAULT);
+      return new ForwardIndex(startOffsets, termIDsInput, maxTerm);
+    } catch (Throwable t) {
+      IOUtils.closeWhileSuppressingExceptions(t, termIDsInput, startOffsets);
+      throw t;
+    }

Trade-offs

• The catch-and-rethrow style that #14633 ("Catch and re-throw Throwable rather than using a success boolean") introduced and #14913 ("Remove a few more boolean success flags") applied to this class; computePermutation already ends in catch (Throwable t) ... throw t (:899-902).
• closeWhileSuppressingExceptions skips nulls and adds close failures to t as suppressed (IOUtils.java:104-137), so the caller still sees the original IOException (if t is not an Error and a close throws one, that Error is thrown with t suppressed; if t is an Error, t is thrown). throw t compiles under throws IOException by Java's precise rethrow.
• IOUtils.closeWhileSuppressingExceptions came to main with #14633 and to the 10.x releases in 10.4.0 (absent at the 9.12.3 and 10.3.2 tags; present on branch_10_6), so only a backport to 9.x or 10.0-10.3 would need B.

What the checks validated

• Stock 645d4fe14a20: the repro fails with assertions on and off.
• With A: the repro, TestBPIndexReorderer and TestBPReorderingMergePolicy pass with assertions on and off (3 suites, 12 tests, 0 failures, 2 skipped: the merge-policy suite's two @Nightly tests) and with -Ptests.nightly=true (12 tests, 0 skipped); the whole :lucene:misc:test passes with assertions on (24 suites, 317 tests, 0 failures, 9 skipped).
• checkGoogleJavaFormat, ecjLintMain, ecjLintTest, forbiddenApisMain and forbiddenApisTest of :lucene:misc pass with the fix and the repro test in place.

Same place, +13/-3: open both inputs and build the ForwardIndex in a try, set success = true before returning, and in finally call IOUtils.closeWhileHandlingException(termIDsInput, startOffsets) unless it succeeded.

-    IndexInput termIDsInput = tempDir.openInput(termIDsFileName, IOContext.DEFAULT);
-    IndexInput startOffsets = tempDir.openInput(startOffsetsFileName, IOContext.DEFAULT);
-    return new ForwardIndex(startOffsets, termIDsInput, maxTerm);
+    IndexInput termIDsInput = null, startOffsets = null;
+    boolean success = false;
+    try {
+      termIDsInput = tempDir.openInput(termIDsFileName, IOContext.DEFAULT);
+      startOffsets = tempDir.openInput(startOffsetsFileName, IOContext.DEFAULT);
+      ForwardIndex forwardIndex = new ForwardIndex(startOffsets, termIDsInput, maxTerm);
+      success = true;
+      return forwardIndex;
+    } finally {
+      if (success == false) {
+        IOUtils.closeWhileHandlingException(termIDsInput, startOffsets);
+      }
+    }

Trade-offs

• Maintainers removed boolean success flags from this class in #14913, so A is the likelier preferred shape on main.
• closeWhileHandlingException drops close failures instead of attaching them to the original exception. It exists on every affected branch (9.8.0 on), so B suits a backport to 9.x.

What the checks validated

• With B: the repro and the two BP suites pass with assertions on (3 suites, 12 tests, 0 failures, 2 skipped @Nightly).
• Earlier, on main ea57360912ed: the finder's, the independent reproducer's and the history tests pass with it; the two upstream BP suites pass (11 tests, 3 seeds, nightly on and off; the same job also ran TestSoftDeletesRetentionMergePolicy with an unrelated fix shadowed, 25 tests in all); both OS-level probes report 0 leaked descriptors and 0 mappings.

buildForwardIndex returns the two file names, and computePermutation opens both inputs and the ForwardIndex in one try-with-resources, which closes earlier resources when a later one fails.

Trade-offs

A larger diff, and ForwardIndex.close() already closes both inputs, so they would be closed twice unless ForwardIndex stops owning them. A keeps ownership as it is.

Method: exception-point enumeration (the study's semi-formal tier). An engine runs random sequences of index operations and makes each I/O and callback throw site fail in turn, checks the outcome against a model, and uses MockDirectoryWrapper.close() as the leak oracle. A hunting AI tool re-ran it unchanged on the real jars with one added dimension, BPReorderingMergePolicy with BPIndexReorderer around the engine's LogDocMergePolicy, over the same 700 scripts (seed 7).

• With BP reordering, the only new "cannot close" signature was {term-ids__N.tmp=1}: 532 of 327,533 injection runs, always with the failure injected at BPIndexReorderer.buildForwardIndex:769, the start-offsets openInput.
• A deterministic reverse reorderer, which never reaches buildForwardIndex, had the same BUG counts as the base run.
• The root cause was then read from the code, and the finding minimized to a JUnit test on the public API, later cut down to the one-method test above.