A failed merge-reader refresh leaks the pooled segment reader, so the segment's files stay open after IndexWriter.close

When a merge starts, ReadersAndUpdates.getReaderForMerge takes a reference on the segment's pooled SegmentReader. If that reader's live docs are stale, it opens a fresh reader that shares the old one's core and has the latest deletes, and releases the old reader only after that succeeds. For a segment with doc-values updates (soft deletes are doc-values updates too) the fresh reader re-reads the update's field-infos file, so an I/O error can make it throw; then nobody releases the reference. The merge never received the reader, so it cannot release it, and closing the writer drops only the pool's own reference: Lucene never closes that reader's files. In the 2-document repro the segment's compound file and three doc-values files stay open after close; with a default IndexWriterConfig a plain commit() (merge-on-commit) leaks the corresponding 4 files of that segment while the writer keeps working. Results, deletions and later commits stay correct. Introduced in 4.6.0 by LUCENE-5189; every release executed from 4.6.0 to 10.6.0 and main leaks. The fix is one hunk: release the reference when the refresh throws.

// ReadersAndUpdates.getReaderForMerge, condensed
SegmentReader reader = getReader(context);              // :1095  +1 on the pooled reader ("Ref for caller", :195-196)
if (pendingDeletes.needsRefresh(reader) || ...) {       // :1096  pooled live docs are stale
  reader = createNewReaderWithLatestLiveDocs(reader);   // :1100  may throw: nothing releases the +1
}

// ReadersAndUpdates.createNewReaderWithLatestLiveDocs, condensed
SegmentReader newReader = new SegmentReader(info, reader, liveDocs, ...); // :1070-1077  shares the core; does I/O
try {
  pendingDeletes.onNewReader(newReader, info);          // :1079
  reader.decRef();                                      // :1080  the caller's ref is released only here
} catch (Throwable t) {
  newReader.decRef();                                   // :1082  cleans up the NEW reader only
  throw t;
}

1. Every merge of the writer's own segments gets its readers through getReaderForMerge, from the merge-reader factories in IndexWriter.java:4083-4088 (merge-on-commit and merge-on-refresh) and :5484-5491 (all other merges), via OneMerge.initMergeReaders (MergePolicy.java:465-481); addIndexes(CodecReader...) merges use the caller's readers instead (MergePolicy.java:256).
2. ReadersAndUpdates.java:1095 takes a reference on the pooled reader. When its live docs are stale (:1096-1097), :1100 calls createNewReaderWithLatestLiveDocs.
3. The core-sharing SegmentReader constructor (SegmentReader.java:128-162) calls initFieldInfos() and initDocValuesProducer() (:156-157). For a segment with field updates, these read the new generation's field-infos file (:196-203) and set up the doc-values producers of the update generations (:186-187; shared with the pooled reader, opened only if not open yet), so the constructor does I/O and can throw. On failure it closes only what it took (doClose(), :159) and rethrows.
4. So :1080 never runs and neither method releases the reference from :1095. initMergeReaders keeps only the readers the factory returned (MergePolicy.java:474-479), so the merge's cleanup (IndexWriter.closeMergeReaders, :5394-5425) never sees this one.
5. When the writer closes (IndexWriter.close(); in the minimal test already the rollback that the tragic failure runs inside forceMerge, IndexWriter.java:2677), ReaderPool.dropAll (ReaderPool.java:314-341) calls ReadersAndUpdates.dropReaders, which releases only the pool's own reference (ReadersAndUpdates.java:213-225). The count stays at 1, so the reader's core (the compound file) and its doc-values producers are never closed.
6. Without field updates the constructor reuses the core's field infos and the already-open doc-values producer (SegmentReader.java:197-198, :188-190), so plain segments cannot hit this. A second, smaller leak sits on the same path: the two factories take a ReadersAndUpdates reference with getPooledInstance(sci, true) (IndexWriter.java:4085, :5486) and do not release it when getReaderForMerge throws. With fix A alone no file stays open; the only observed effect of this second leak is that the segment's ReadersAndUpdates reference count stays at 1 after close (0 with fix B; read by reflection in a probe of the same test).

• Who. Applications that update doc-values fields (updateNumericDocValue, updateBinaryDocValue, updateDocValues) or use soft deletes (once applying them has given the segment a field-update generation, SegmentCommitInfo.hasFieldUpdates), and get an I/O error at the one moment a merge opens the fresh reader of such a segment whose pooled reader has stale live docs. That step only opens and reads files Lucene already wrote, so the errors are those of opening a file: too many open files, a transient storage error, a permission error, or a custom Directory that throws.
• Default vs opt-in. Doc-values updates and soft deletes are opt-in APIs; the rest is default. Reader pooling is on by default (IndexWriterConfig.DEFAULT_READER_POOLING = true), and merge-on-commit and merge-on-refresh run with a default config (DEFAULT_MAX_FULL_FLUSH_MERGE_WAIT_MILLIS = 500). On those two paths the failure is not tragic and the writer keeps working (merge-on-commit executed on main 645d4fe14a20 with assertions on and off; merge-on-refresh executed on ea57360912ed and 10.5.2). Natural and forced merges reach it too; there the failure is tragic and the writer closes itself, as designed, with the same leak.
• Scale. None needed: 2 documents. Each failure leaks one reference on the segment's pooled reader. Repeated failures on the same pooled reader leak one more reference each (by code reading), but open no new file or mapping (4 failed attempts in the probe's refresh arms).
• What stays open (stock main ea57360912ed, real file system; byte counts are the probe's estimates from the sizes of the held files): on MMapDirectory, the 64-bit default, no descriptor is held, but the 4 leaked files, deleted by a later merge, were still mapped after IndexWriter.close(), Directory.close() and five System.gc() calls: 1,069,056 bytes in the refresh arm, 2,138,112 in the force-merge arm. While a deleted file stays mapped, the OS does not free its disk space. The force-merge arm holds twice as much because, by code reading, the per-segment shared arena also keeps later mappings of the same segment's files alive (bounded by its 64 permits). On NIOFSDirectory, 4 descriptors on the deleted files stayed open after IndexWriter.close() and Directory.close() (1,056,459 bytes in the refresh arm, 1,056,460 in the force-merge arm) and were released only after the five System.gc() calls.
• Not affected. Search results, deletions and later commits: no deleted document became visible in any run.

One test class, one method, single thread. MockDirectoryWrapper checks the failure on every file operation while getReaderForMerge is on the stack; on this path it fires once, on the open of the fresh reader's field-infos file _0_1.fnm. The merge that fails is the full-flush merge started by forceMerge's own flush (IndexWriter.java:2248, :4379), which also applies the buffered delete. The default codec is pinned so the file names are stable, and SerialMergeScheduler brings the merge exception to the test thread. The fault predicate names only the method, no package-private type; the class sits in org.apache.lucene.index by test convention. It fails on 20 of 20 random seeds and passes on 20 of 20 with fix A alone (and with A and B together). The only random input the test passes is MockDirectoryWrapper(random(), ...), which varies yields and throttling (its random I/O failure rates are 0 by default); no run changed the outcome.

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

# gradle, scratch worktree of 645d4fe14a20, seed DEADBEEF, -Ptests.asserts=true (some lines cut)
TestMergeReaderRefreshFailureLeak > testFailedMergeReaderRefreshLeavesFilesOpen FAILED
    java.lang.AssertionError: expected no open files after IndexWriter.close(), but MockDirectoryWrapper: cannot close: there are still 4 open files: {_0.cfs=1, _0_1_Lucene90_0.dvp=1, _0_1_Lucene90_0.dvs=1, _0_1_Lucene90_0.dvd=1}
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:B818C0E66CBDA5B3]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.apache.lucene.index.TestMergeReaderRefreshFailureLeak.testFailedMergeReaderRefreshLeavesFilesOpen(TestMergeReaderRefreshFailureLeak.java:62)

1 test completed, 1 failed

# the same failure with -Ptests.asserts=false; with fix A or B: 1 test completed, 0 failed

• Contracts. IndexWriter.close(): "Closes all open resources and releases the write lock." (IndexWriter.java:1342). IndexReader.incRef(): "Be sure to always call a corresponding decRef, in a finally clause; otherwise the reader may never be closed." (IndexReader.java:166-168).
• The surrounding code is exception-safe. createNewReaderWithLatestLiveDocs releases the new reader when onNewReader fails (ReadersAndUpdates.java:1081-1083); initMergeReaders keeps partly acquired readers "to close them in the case of an exception" (MergePolicy.java:476-479); the constructor closes what it opened (SegmentReader.java:158-160). Only the caller's reference on the old reader is missed.
• Readers may outlive the writer only when the application holds them. ReaderPool.dropAll: "it is allowed that these decRefs do not actually close the SRs; this happens when a near real-time reader is kept open after the IndexWriter instance is closed" (ReaderPool.java:327-330). The repro closes its NRT reader first.
• Upstream precedent: tests close a MockDirectoryWrapper after injected failures. TestIndexWriterOutOfFileDescriptors injects IOExceptions on file opens, lets merges fail (tragically, when they hit the merge scheduler), rolls the writer back and closes a MockDirectoryWrapper, which fails on any open file; it uses no doc-values updates and does not target this path, so it is precedent, not a test of it. TestIndexWriterOnError uses doc-values updates, but its failures throw an Error with a 1-in-3000 draw at each failure check while IndexWriter is on the stack. CrankyCodec never fails the reads this step does (its field-infos, doc-values and compound-file readers delegate without injecting).
• History. The same class of leak was fixed as a bug before: LUCENE-5656 "Fix rare fd leak in SegmentReader when multiple docvalues fields have been updated with IndexWriter.updateXXXDocValue and one hits exception" and LUCENE-5262 "Fixed file handle leaks when multiple attempts to open an NRT reader hit exceptions". In the history and prior-art material reviewed, no commit, comment, test, CHANGES or MIGRATE entry accepts this one.

Strongest case: the minimal test is a tragic merge failure, and the IndexWriter javadoc says that after an Error or a disaster it closes itself "in case any internal state (buffered documents, deletions, reference counts) were corrupted" (IndexWriter.java:162-165); on #12228 a maintainer said that on a tragedy it is unsafe to delete files.

Why it fails: (1) the leak needs no tragedy: on the default merge-on-commit path getTragicException() is null, the writer commits again and closes normally, and the corresponding 4 files of that segment stay open (executed on 645d4fe14a20); (2) the javadoc note is about an Error or a checkpoint disaster and does not say files may stay open, and TestIndexWriterOutOfFileDescriptors expects every file closed after tragic merge failures from IOExceptions; (3) #12228 is about deleting files, while the fix only releases a reference.

Both options were run through gradle in a scratch worktree of 645d4fe14a20. A is the root-cause fix and alone removes the leak in every repro; B adds the release of the ReadersAndUpdates reference that the merge-reader factories take, whose leak leaves no file open (its only observed effect is an internal reference count of 1 after close). Fixing it inside createNewReaderWithLatestLiveDocs instead would be wrong: its other caller, swapNewReaderWithLatestLiveDocs (ReadersAndUpdates.java:1088-1090), passes the pool's own reader and keeps it in the reader field when the call throws, so a decRef there would drop the pool's reference while the field still points at the reader.

In ReadersAndUpdates.getReaderForMerge (ReadersAndUpdates.java:1100), +8/-1.

       // beware of zombies:
       assert pendingDeletes.getLiveDocs() != null;
-      reader = createNewReaderWithLatestLiveDocs(reader);
+      final SegmentReader pooled = reader;
+      try {
+        reader = createNewReaderWithLatestLiveDocs(pooled);
+      } catch (Throwable t) {
+        // createNewReaderWithLatestLiveDocs releases our reference only on success
+        pooled.decRef();
+        throw t;
+      }
     }

Trade-offs

• The smallest change, at the one caller that owns the extra reference; the success path is unchanged.
• The decRef cannot close the reader: the pool still holds its own reference, and the method holds the ReadersAndUpdates lock.
• The same hunk applies, with line offsets, to the 10.6.0 and 9.12.3 sources (checked with git apply --check, and again by the fact check with patch --dry-run: offsets +62 and -309; not built there).
• Scope: A covers a failure anywhere in createNewReaderWithLatestLiveDocs. A failure after the refresh, in readerConsumer.accept (ReadersAndUpdates.java:1105), is not covered; the two current consumers call IndexFileDeleter.incRef(Collection), which declares no IOException, so that is not a demonstrated path.

What the checks validated

• Stock 645d4fe14a20: the minimal test fails through gradle with assertions on and off (4 open files).
• With A: the minimal test passes with assertions on and off, and on 20 of 20 random seeds (fact check).
• With A, assertions on, seed DEADBEEF: the test plus TestIndexWriterOutOfFileDescriptors, TestIndexWriterOnError, TestIndexWriterExceptions, TestIndexWriterExceptions2, TestIndexWriterReader, TestIndexWriterMergePolicy, TestNumericDocValuesUpdates, TestBinaryDocValuesUpdates, TestMixedDocValuesUpdates, TestSoftDeletesRetentionMergePolicy, TestPendingSoftDeletes and TestReaderPool pass (13 suites, 195 tests, 0 failures, 10 skipped); the whole :lucene:core:test passes (749 suites, 8,898 tests, 0 failures, 322 skipped).
• checkGoogleJavaFormat, ecjLintMain, ecjLintTest, forbiddenApisMain and forbiddenApisTest of :lucene:core pass with A and the test in place.
• Earlier, on ea57360912ed: A alone made the independent reproducer's, the devil's advocate's and the adjudication's tests pass, with the injection still firing at the same site.

In IndexWriter.java:4084-4088 and :5485-5491, +24/-4: when getReaderForMerge throws, call release(rld) for the getPooledInstance(sci, true) reference, which closeMergeReaders releases (IndexWriter.java:5413) only for readers the merge received.

               sci -> {
                 final ReadersAndUpdates rld = getPooledInstance(sci, true);
-                return rld.getReaderForMerge(
-                    context, mr -> deleter.incRef(mr.reader.getSegmentInfo().files()));
+                boolean gotReader = false;
+                try {
+                  MergePolicy.MergeReader mr =
+                      rld.getReaderForMerge(
+                          context, r -> deleter.incRef(r.reader.getSegmentInfo().files()));
+                  gotReader = true;
+                  return mr;
+                } finally {
+                  if (gotReader == false) {
+                    release(rld); // the merge never got this reader, so it will not release it
+                  }
+                }
               });
(the same change in the mergeMiddle factory, inside its synchronized block)

Trade-offs

• Also closes the second, smaller leak, which leaves no file open (its only observed effect is the segment's ReadersAndUpdates count staying at 1 after close).
• Open PR #16571 ("Let a merge write more than one segment") moves the mergeMiddle factory that this part also changes.

What the checks validated

• With B: the minimal test passes with assertions on and off, and the same 13 suites pass (195 tests, 0 failures, 10 skipped); the whole core module was not run with B.
• Earlier, on ea57360912ed: the finder's version of B with the three other fixes of its hunt passed 15 upstream test classes (380 tests per seed, seeds DEADBEEF and 1234ABCD, 0 failures).

Method: bounded-exhaustive exception-point enumeration on the real jar (the study's semi-formal tier). Each index-level operation was run against an executable reference of its documented outcome while an IOException was injected at every distinct I/O throw point, one-shot and sticky, followed by a check that the MockDirectoryWrapper closes with no open file.

• 68 runs failed that last check ("cannot close: there are still N open files") after a correctly classified outcome; the first was in a soft-deletes configuration with pooled readers, failing in SegmentReader.initFieldInfos under createNewReaderWithLatestLiveDocs and getReaderForMerge.
• The root cause was then located by reading the code and confirmed with a shadow fix.
• A second, independent fault-injection run (random model-based testing, a merge started from updateDocuments with SerialMergeScheduler) found the same root cause through a second trigger path.