A failed reopen of a FilterDirectoryReader leaks the new inner reader and the segments it shares

Every reopen of a FilterDirectoryReader is wrapDirectoryReader(in.doOpenIfChanged(...)): Lucene first reopens the wrapped reader, which (on a reopen from a commit) shares each unchanged segment reader with your old reader by adding a reference to it, then wraps the result. wrapDirectoryReader is return in == null ? null : doWrapDirectoryReader(in); with no try/catch. When the wrap step throws, as SoftDeletesDirectoryReaderWrapper does on an I/O error while it reads a new segment's soft-deletes doc values, the freshly reopened inner reader is neither returned nor closed. Its references on the segment readers (from a commit, an extra one on every segment it shares with your reader), the new segments' readers and, for a near-real-time reader, its hold on the writer's files are never released: closing your reader leaves the shared segment at refCount 1 and its files open. Each failed refresh leaks again; results stay correct. The line is unchanged since the class was added in 4.3.0; a ten-line try/catch that closes the inner reader and rethrows fixes it.

// FilterDirectoryReader.java:91-94: the constructor runs the wrap step before 'in' is stored
public FilterDirectoryReader(DirectoryReader in, SubReaderWrapper wrapper) throws IOException {
  super(in.directory(), wrapper.wrap(in.getSequentialSubReaders()), null);   // :92, can throw
  this.in = in;
}
// FilterDirectoryReader.java:107-109: no cleanup of 'in' when doWrapDirectoryReader throws
private final DirectoryReader wrapDirectoryReader(DirectoryReader in) throws IOException {
  return in == null ? null : doWrapDirectoryReader(in);
}
// FilterDirectoryReader.java:111-114, and five more final overloads to :144
protected final DirectoryReader doOpenIfChanged() throws IOException {
  return wrapDirectoryReader(in.doOpenIfChanged());
}

1. DirectoryReader.openIfChanged(oldReader) calls oldReader.doOpenIfChanged() (DirectoryReader.java:245-249); for a FilterDirectoryReader that is wrapDirectoryReader(in.doOpenIfChanged()).
2. in.doOpenIfChanged() builds a new StandardDirectoryReader. From a commit (StandardDirectoryReader.java:477-511, then open at :174-184), the segments are walked from the last one (:233): a new segment gets a new reader (:296), and an unchanged one is shared: "No change; this reader will be shared between the old and the new one, so we must incRef it", then oldReader.incRef() (:317-323). The new reader is then constructed on them (:182-183). A near-real-time reopen goes through the writer instead (:456-466): its segment readers come from the writer's reader pool (IndexWriter.java:554), and it takes writer.incRefDeleter(segmentInfos) (StandardDirectoryReader.java:148), released only in its doClose (:548-568).
3. doWrapDirectoryReader(newInner) constructs the subclass, whose constructor wraps the new leaves (FilterDirectoryReader.java:92). SoftDeletesDirectoryReaderWrapper (SoftDeletesDirectoryReaderWrapper.java:67-82) reuses a leaf it already wrapped when the reader-cache key matches (:116-120); for any other leaf that has soft-deletes doc values it reads them (:129-152; no such field: early return at :131; a FixedBitSet(maxDoc) at :139) and rethrows an IOException as UncheckedIOException (:124).
4. If that throws, wrapDirectoryReader lets the exception through (FilterDirectoryReader.java:108). The new inner reader is never returned or closed, and keeps its references: +1 on every shared segment reader (from a commit) or the pooled readers' references (near-real-time), the new segment readers and their files, and (near-real-time) the deleter reference.
5. The application owns only its old reader (DirectoryReader.java:236-238). From a commit, closing it takes each shared segment reader from 2 to 1, so it is never closed (IndexReader.java:164-170).

Two one-document commits: _0 shared, _1 new. After the failed openIfChanged: _0 refCount 2 (expected 1), the dropped inner reader 1, _1 1. After reader.close(): _0 1 (expected 0), the inner reader 1, _1 1. With the fix: 1, then 0.

The inner reopen's tested failure path, an I/O error while the inner StandardDirectoryReader opens the new segment, is cleaned up by Lucene (the independent reproduction's control passes). This report concerns the wrap step, which runs after the inner reopen succeeded and has no cleanup.

• Who is hit: refreshes of a FilterDirectoryReader via DirectoryReader.openIfChanged, SearcherManager or ReaderManager. In stock Lucene that is SoftDeletesDirectoryReaderWrapper (opt-in soft deletes); ExitableDirectoryReader does no I/O in its wrap step, so only an OutOfMemoryError reaches it there. Any application wrapper whose wrap step can throw is hit too.
• When: only when the wrap step throws after the inner reopen succeeded: an I/O error while the soft-deletes wrapper reads the doc values of a new or changed segment through an input that is already open (a storage read error), an OutOfMemoryError there, or a throwing application wrapper. With the default codec a missing doc-values file fails earlier, while the inner reopen builds the segment reader (SegmentReader.java:111, Lucene90DocValuesProducer.java:132), and that path cleans up; a missing file can reach the wrap step with storage that fetches data lazily. elastic/elasticsearch#160888 (Lucene 10.5.1) shows a production stack trace through exactly this point, a remote-storage read error thrown from SoftDeletesSubReaderWrapper.wrap inside wrapDirectoryReader during openIfChanged; it discusses only the missing file, not the leak.
• How much (stock SearcherManager, real NIOFSDirectory and MMapDirectory, main ea57360912ed (the sources involved are identical at 645d4fe14a20), 10.5.2 and 9.12.3; nothing leaks with the fix):
  • commit-point refresh, 100 failed refreshes: 104 index inputs never closed after the manager and the writer were closed; with MMapDirectory 110 mapping regions survived close and garbage collection; with NIOFSDirectory the JDK reclaimed most descriptors after garbage collection (4 left after five System.gc() calls);
  • near-real-time refresh: the leaked reader keeps the writer's file references, so 11 files of merged-away segments stayed after forceMerge(1) + commit() and writer.close(), until the next IndexWriter deleted them; with indexing during 47 failed refreshes, 368 files (160,777 bytes in the tiny test) and 180 inputs, and 180 files stayed mapped after the next IndexWriter deleted them; by reasoning (not observed to the end), their disk space stays allocated until the mappings are released or the process exits.
• Growth: linear in failed refresh attempts; a refresh loop that retries against a persistent storage fault leaks on every attempt.
• What does not happen: the manager keeps serving the previous reader (checked over 100 failures), no soft-deleted document reappears, nothing is written to the index, each failed refresh returns promptly.

The test sits in org.apache.lucene.index by convention and uses no package-private access. The wrapper passes leaves through on the first open and throws when it wraps the reopened reader; getReaderCacheHelper() returns null, which IndexReader allows ("not suited for caching") and which plays no part in the bug. Only LuceneTestCase's randomized codec varies, and reader reference counts do not go through the codec; observed: seeds DEADBEEF and 5EED1234 fail, and so do 10 of 10 random-seed iterations.

The stock trigger reproduces the same refCount 2: the finder's longer test wraps a Directory subclass that fails doc-values reads of the new segment under SoftDeletesDirectoryReaderWrapper (about 150 more lines for the fault injector, so it is not the minimal repro).

Regression test: this same test, as its own class or as a method of TestFilterDirectoryReader.

# gradle, stock 645d4fe14a20, seed DEADBEEF, assertions on (identical with -Ptests.asserts=false)
TestFilterDirectoryReaderReopenWrapFailure > testFailedWrapOnReopenReleasesSharedSegment FAILED
    java.lang.AssertionError: expected:<1> but was:<2>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:F1AACFCEB33224B0]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:633)
        at org.apache.lucene.index.TestFilterDirectoryReaderReopenWrapFailure.testFailedWrapOnReopenReleasesSharedSegment(TestFilterDirectoryReaderReopenWrapFailure.java:41)
1 test completed, 1 failed

# release jars, seed DEADBEEF, assertions on and off: the same assertion
10.6.0, 9.12.3: java.lang.AssertionError: expected:<1> but was:<2>

# 10 random-seed iterations (seed 0FAB0BB1E27A3D35) on 645d4fe14a20: 10 of 10 fail

# with the fix (wrapDirectoryReader closes the inner reader and rethrows)
classes=8 tests=93 failures=0 errors=0 skipped=1 (the minimal test and the 7 closest reader suites; assertions on and off)

• Contract: DirectoryReader.java:236-238 (quoted above). Closing the provided reader no longer frees its segments, because "the IndexReader will not really be closed until decRef has been called for all outstanding references" (IndexReader.java:164-170).
• Lucene cleans up on comparable paths: StandardDirectoryReader.java:96-105, "This may throw CorruptIndexException if there are too many docs, so it must be inside try clause so we close readers in that case", with IOUtils.closeWhileSuppressingExceptions(t, readers); SearcherManager.java:246, "NOTE: this decRefs incoming reader on throwing an exception" (:262-264).
• Tests require it: TestDirectoryReaderReopen.testOverDecRefDuringReopen (:673-735, and the executor variant at :1530) injects a reopen failure and then closes a MockDirectoryWrapper, which fails on open files; TestIndexWriterReader.testNRTOpenExceptions (:1108-1110): "LUCENE-5262: test that several failed attempts to obtain an NRT reader don't leak file handles".
• The trigger is legal: doWrapDirectoryReader declares throws IOException (FilterDirectoryReader.java:105, since LUCENE-6299, 2015), and the stock soft-deletes wrapper raises UncheckedIOException itself (SoftDeletesDirectoryReaderWrapper.java:124).
• History: in the LUCENE-4902 review (2013) the doOpenIfChanged methods were made final so the base class owns reopen; nobody discussed failures, and the searches found no test, CHANGES or MIGRATE entry that accepts the leak. The same bug class was fixed or handled in SearcherTaxonomyManager (#9291, LUCENE-8244) and the SearcherManager constructor (#4871, LUCENE-3798).

The strongest case for "intended" (the devil's-advocate checker and the works-as-designed reviewer): doWrapDirectoryReader receives in, so a subclass whose wrap step can fail should close it itself; the constructor does not close in on a throw either; the javadoc promises nothing about the exceptional path; and a throwing application wrapper could be called misuse.

Why it fails: the doWrapDirectoryReader javadoc (FilterDirectoryReader.java:96-100) says only "Implementations should just return an instantiation of themselves, wrapping the passed in DirectoryReader", and the class javadoc (:23-31) says nothing about cleanup either; the reopen methods are final, so the base class owns reopen (a subclass could still catch inside its own doWrapDirectoryReader, close in and rethrow, but nothing asks it to); and Lucene's own SoftDeletesDirectoryReaderWrapper.doWrapDirectoryReader (SoftDeletesDirectoryReaderWrapper.java:67-82) does no cleanup, so the stock class leaks wherever the duty is placed. In the constructor case the caller still holds in; after a failed reopen the caller never had the new inner reader. The misuse argument does not touch the stock soft-deletes I/O-error path.

The hunt's fix (the same added and removed lines, re-made as a git diff against 645d4fe14a20), applied in a scratch worktree of 645d4fe14a20 and run through gradle (OpenJDK 25.0.3, seed DEADBEEF) with the minimal test and the closest reader suites.

In FilterDirectoryReader.wrapDirectoryReader (:107-109), wrap doWrapDirectoryReader(in) in a try/catch; on any Throwable, call IOUtils.closeWhileSuppressingExceptions(t, in) and rethrow (+11/-1 with the import). With the built-in reopen implementations, in is the new reader that in.doOpenIfChanged(...) just returned, which the caller would have owned and nobody holds after the throw, so closing it is exactly the missing release. This relies on the normal ownership convention for returned readers, which holds for the built-in implementations but is not an API guarantee for an arbitrary custom inner DirectoryReader.

private final DirectoryReader wrapDirectoryReader(DirectoryReader in) throws IOException {
  if (in == null) {
    return null;
  }
  try {
    return doWrapDirectoryReader(in);
  } catch (Throwable t) {
    // the new inner reader is not returned to anybody: release it
    IOUtils.closeWhileSuppressingExceptions(t, in);
    throw t;
  }
}

Trade-offs

• Covers all six reopen overloads, SoftDeletesDirectoryReaderWrapper and every application subclass at once, and nested wrappers (each level closes the reader it just received; reasoned, not run). It mirrors StandardDirectoryReader.open (:96-105).
• If a subclass already called in.close(), the fix's close is a no-op (IndexReader.close() is idempotent, if (!closed), IndexReader.java:351-356); if it called in.decRef() instead, the fix decrements again (IndexReader.java:215-222). No in-tree subclass's doWrapDirectoryReader does either.
• No cost on the normal path (one try block). Catching Throwable also releases on OutOfMemoryError, as StandardDirectoryReader.open does.

What the checks validated

Through gradle in a scratch worktree of 645d4fe14a20 (OpenJDK 25.0.3, --offline, seed DEADBEEF): the minimal test fails on stock and passes with the fix, assertions on and off. With the fix, the minimal test plus TestFilterDirectoryReader, TestDirectoryReaderReopen, TestSoftDeletesDirectoryReaderWrapper, TestExitableDirectoryReader, TestDirectoryReader, TestReaderClosed and TestSearcherManager (8 classes, 93 tests, 1 skipped) pass with 0 failures, assertions on and off. Core's checkGoogleJavaFormat, ecjLintMain and forbiddenApisMain pass with the fix. Earlier, the hunt's shadow fix check passed every repro (the finder's, the independent reproduction's, the cross-model review's and the intended-behaviour check's tests, the history instrument on 4.3.0 and main, and the security probe with 0 leaked inputs) and kept the independent reproduction's controls green.

Add the same try/catch to SoftDeletesDirectoryReaderWrapper.doWrapDirectoryReader and document a cleanup duty in the doWrapDirectoryReader javadoc. Not run.

Trade-offs

Fixes the stock trigger only; every application wrapper must repeat it, although the base class owns reopen (final overloads).

Close in when wrapper.wrap(...) throws in the constructor (:91-94). Not run.

Trade-offs

It would also close a reader that a direct caller of new SomeWrapper(reader) still owns, which changes the constructor's contract.

Method: formal, TLA+ model checking (TLC) in a hunt unit (ReaderLifecycle.tla, a model of IndexReader reference counting, segment-core sharing and StandardDirectoryReader / FilterDirectoryReader reopen, built from the code with a file:line map).

• With the wrapThrow fault (eight injectable faults plus a fault-free run were checked), the NoLeak invariant fails in the sequential configuration (32 distinct states) and the executor configuration (142); the same configurations with the modelled fix pass.
• The counterexample was replayed as a public-API JUnit test on the stock jars, then minimized to the one-test repro.
• The fault edge itself was added while modelling the code, which a stricter reading could call semi-formal.