DirectoryTaxonomyWriter.close() skips its cleanup when its commit fails, leaving the taxonomy reader or write lock open

DirectoryTaxonomyWriter.close() is commit(); indexWriter.close(); doClose(); with no try/finally, and doClose() is the only code that marks the writer closed, and on the close path the only code that closes its internal near-real-time reader and cache. So when the commit inside close() throws, nothing is freed. After a segment-write I/O error such as a full disk, the internal IndexWriter has closed itself, every close() throws AlreadyClosedException from its own commit(), and the reader keeps the taxonomy's segment file open until rollback(); with the default MMapDirectory its memory mapping outlives even the writer. After a failed fsync inside close(), the IndexWriter and its write lock stay open too, so a new taxonomy writer in the same JVM gets LockObtainFailedException until close() is retried or rollback() is called. Nothing is lost beyond the failed commit, nothing is corrupted, and the original exception reaches the caller. Every release run from 5.0.0 to 10.5.2 fails, and 10.6.0 and main have the same code (the write-lock form from 4.10.0). Low priority: it needs a storage error during a taxonomy commit in a process that keeps running.

// DirectoryTaxonomyWriter.java:288-299
public synchronized void close() throws IOException {
  if (!isClosed) {
    commit();              // :290  throws -> nothing below runs
    indexWriter.close();   // :291
    doClose();             // :292
  }
}
private void doClose() throws IOException {
  isClosed = true;         // :297  the only place close() marks the writer closed
  closeResources();        // :298  closes the reader manager and the cache (:308-318)
}

1. The reader exists from the start. The constructor adds the root category (DirectoryTaxonomyWriter.java:185); addCategoryDocument calls getTaxoArrays() (:475), which calls initReaderManager() (:672), which opens new ReaderManager(indexWriter, false, false) (:252), a near-real-time reader on segment _0. The repro's stack trace shows exactly this chain.
2. A full disk during commit is tragic. DirectoryTaxonomyWriter.commit() checks only its own isClosed flag (ensureOpen, :434-438) and calls indexWriter.commit() (:527). An I/O error while flushing the new segment makes the IndexWriter record a tragic exception and roll itself back (IndexWriter.java:6098-6105; javadoc of getTragicException: "closed as a side-effect of a tragic exception, e.g. disk full while flushing a new segment", :6109-6110). This self-closing came with LUCENE-5987 in 5.0.0.
3. Every close() now fails before its cleanup. close() sees isClosed == false and calls commit(); ensureOpen passes, and indexWriter.commit() throws AlreadyClosedException("this IndexWriter is closed", tragedy) (IndexWriter.java:914-916). doClose() never runs and isClosed stays false, so the next close() does the same.
4. The leak. The reader manager's reader is not owned by the IndexWriter, so the writer's own rollback does not close it; only closeResources() would. Its segment file stays open: _0.cfs in the repro. In this form the IndexWriter has already released write.lock.
5. The fsync form. A Directory.sync failure inside the commit is not tragic: the IndexWriter stays open, so both the reader and write.lock stay held. NativeFSLockFactory keeps held lock paths in a static JVM-wide set (NativeFSLockFactory.java:68) that only closing the lock clears (:146), so garbage collection does not free it.
6. The exit that works. rollback() is ensureOpen(); indexWriter.rollback(); doClose(); (DirectoryTaxonomyWriter.java:861-865), and IndexWriter.rollback() is a no-op on a writer that already closed itself (IndexWriter.java:2576-2584), so doClose() runs and everything is freed.

• Who. Applications that use taxonomy facets (the opt-in facet module) with DirectoryTaxonomyWriter. The default constructor, cache, IndexWriterConfig, codec and directory are enough.
• When. An I/O error inside a taxonomy commit (a full disk, a failed file creation, a failed fsync) in a process that keeps running, followed by close(), try-with-resources or IOUtils.close, which is how Lucene's own facet demos close the writer, instead of rollback().
• Tragic form (since 5.0.0). close() throws on every call, and one internal reader per failed writer keeps the taxonomy's segment files open until rollback(). With the default MMapDirectory (what FSDirectory.open returns on a 64-bit JVM) the memory mapping outlives even the writer: it survived five System.gc() calls after the writer became unreachable, as a shared Arena is freed only by an explicit close. With NIOFSDirectory the file handle stays open only while the writer is reachable; garbage collection then released it. If a later writer replaces the taxonomy, the deleted files' disk space stays allocated: 2 deleted files, 36,864 bytes, for a 2,000-category taxonomy in the security probe.
• Fsync form (since 4.10.0). The taxonomy stays write-locked inside the JVM: every new taxonomy writer on it fails fast with LockObtainFailedException until the old writer is closed again or rolled back.
• Bounded. 2 leaked inputs per injected fault in the probe (25 faults, 50 inputs); 200 close() retries or 200 blocked opens added nothing.
• What it does not do. No lost acknowledged change and no corruption (CheckIndex clean); readers see exactly the last successful commit; ordinals stay stable; the commit's or close's exception still reaches the caller.
• How likely. Rare: it needs a storage error during a taxonomy commit in a long-running process, and most applications restart after such an error. No public report of it has been found since 5.0.0 (see Prior reports).

One test class, one method, single thread. The only subclass is the test framework's MockDirectoryWrapper.Failure, which makes every createOutput throw IOException("simulated disk full"). The outcome does not depend on the seed or the random test codec: 30 of 30 derived seeds and 4 other seeds fail the same way.

The test asserts only the two exceptions, which the buggy and the fixed code both throw; the failure is MockDirectoryWrapper's own check that every file is closed. It covers the tragic form only; the fsync form comes from the verifiers' tests (How it was found, Independent checks).

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

# gradle, scratch worktree of 645d4fe14a20, seed DEADBEEF, -Ptests.asserts=true (module prefixes and some frames cut; -Ptests.asserts=false prints the same)
TestDirectoryTaxonomyWriterCloseAfterFailure > testCloseAfterFailedCommitReleasesFiles FAILED
    java.lang.RuntimeException: MockDirectoryWrapper: cannot close: there are still 1 open files: {_0.cfs=1}
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:77E8C06B083C34E8]:0)
        at org.apache.lucene.tests.store.MockDirectoryWrapper.close(MockDirectoryWrapper.java:884)
        at ...TestDirectoryTaxonomyWriterCloseAfterFailure.testCloseAfterFailedCommitReleasesFiles(TestDirectoryTaxonomyWriterCloseAfterFailure.java:43)
        Caused by:
        java.lang.RuntimeException: unclosed IndexInput: _0.cfs
            at org.apache.lucene.tests.store.MockDirectoryWrapper.openInput(MockDirectoryWrapper.java:843)
            ...
            at org.apache.lucene.index.SegmentCoreReaders.<init>(SegmentCoreReaders.java:83)
            ...
            at org.apache.lucene.index.ReaderManager.<init>(ReaderManager.java:62)
            at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyWriter.initReaderManager(DirectoryTaxonomyWriter.java:252)
            at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyWriter.getTaxoArrays(DirectoryTaxonomyWriter.java:672)
            at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyWriter.addCategoryDocument(DirectoryTaxonomyWriter.java:475)
            ...
            at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyWriter.<init>(DirectoryTaxonomyWriter.java:185)
            ...
1 test completed, 1 failed

• The contract text. close(): "Frees used resources as well as closes the underlying IndexWriter, which commits whatever changes made to it to the underlying Directory" (DirectoryTaxonomyWriter.java:284-285). openIndexWriter: "the instance this method returns will be closed upon calling to close()" (:206). Neither excludes the failure path, and nothing documents that the caller must call rollback() after close() throws. The javadoc does not explicitly promise cleanup on failure either: "expected" rests on this text plus the IndexWriter.close() precedent.
• The delegate's contract. IndexWriter.close(): "If this method throws any other exception, the IndexWriter will be closed, but changes may have been lost" (IndexWriter.java:1351-1352). LUCENE-4246 (#5316) introduced this because a close() that keeps resources was considered a bug.
• History. LUCENE-4246's trunk commit 8559eafd99e (2014-04-08) edited DirectoryTaxonomyWriter.close() (it moved the IndexWriter close out of doClose()) but left commit() outside any try/finally. LUCENE-5987 (c064aaa4850, 2014-12-05) then made IndexWriter close itself on aborting exceptions. The #5316, #7049 and #6933 threads never mention the taxonomy writer.
• Tragic form. Once the internal IndexWriter is closed, no number of close() calls can succeed, so even the old "retry close()" contract does not cover it.
• Callers. Five facet demos close with IOUtils.close(indexWriter, taxoWriter) (for example SimpleFacetsExample.java:93); no non-test caller rolls back after a failed close.
• Tests. TestDirectoryTaxonomyWriter has testRollback, testRecreateRollback and testEnsureOpen (:165, :182, :193), none with fault injection; no test under lucene/facet/src/test uses failOn or MockDirectoryWrapper.Failure, so no test asserts or tolerates the current behaviour.

The strongest "works as designed" case (the devil's-advocate verifier; the cross-model intended lens rated it moderate): close() follows the old, pre-LUCENE-4246 IndexWriter contract, in which a close() that throws did not happen, the writer stays usable, and the caller retries close() or calls rollback(), which keeps uncommitted categories for a retry. AutoCloseable.close() says "@throws Exception if this resource cannot be closed"; TwoPhaseCommitTool rolls back on failure; the class is @lucene.experimental (DirectoryTaxonomyWriter.java:76); and in the fsync form a retried close() does recover.

Why it fails: LUCENE-4246 rejected exactly that contract for IndexWriter; it was never documented for DirectoryTaxonomyWriter; it cannot cover the tragic form, where close() never succeeds; the JDK's AutoCloseable apiNote itself advises releasing resources before throwing because close "is unlikely to be invoked more than once"; and Lucene's own demos rely on a single close(). Keeping uncommitted categories is the trade-off IndexWriter.close() already gave up ("changes may have been lost"). Verdict: a defect of Low priority; the tragic form is the stronger case.

Option A was run through gradle in a scratch worktree of 645d4fe14a20; B and C were not run. All three change only close()'s failure path: a successful close() still commits and closes as before.

In DirectoryTaxonomyWriter.close() (DirectoryTaxonomyWriter.java:288-294), -3/+8 plus an IOUtils import.

   public synchronized void close() throws IOException {
     if (!isClosed) {
-      commit();
-      indexWriter.close();
-      doClose();
+      try {
+        commit();
+      } catch (Throwable t) {
+        // still free everything, as IndexWriter.close() does when it fails
+        IOUtils.closeWhileSuppressingExceptions(t, indexWriter::rollback, this::doClose);
+        throw t;
+      }
+      IOUtils.close(indexWriter, this::doClose);
     }
   }

Trade-offs

• The caller still gets the original exception, with any secondary failure suppressed in it (only if a cleanup step throws an Error is that Error thrown instead, with the original suppressed in it, IOUtils.java:114-136). Afterwards the writer is closed, its reader and cache are closed, and the IndexWriter is rolled back (a no-op if it already closed itself), so write.lock is free.
• After a failed final commit, uncommitted categories are discarded instead of kept for a retried close(): the same choice IndexWriter.close() made ("changes may have been lost"). A caller that wants to retry can call commit() itself before close(). Main-index documents that already use those ordinals must then not be committed either: the taxonomy "must always be committed to disk before the main index" (TaxonomyWriter.java:38-40).
• On success, IOUtils.close still runs doClose() if indexWriter.close() throws.

What the checks validated

• Stock 645d4fe14a20 (gradle, scratch worktree, seed DEADBEEF): the repro fails with assertions on and off.
• With A: the repro and TestDirectoryTaxonomyWriter pass with assertions on and off (2 suites, 16 tests, 0 failures, 1 skipped: testHugeLabel is @Nightly); the whole :lucene:facet:test passes with assertions on (40 suites, 307 tests, 0 failures, 7 skipped); checkGoogleJavaFormat, ecjLintMain, ecjLintTest, forbiddenApisMain and forbiddenApisTest of :lucene:facet pass.
• Earlier, standalone: the finder's, the independent reproducer's and the minimal tests pass with it, assertions on and off. A root-cause check, not a reviewed upstream patch.

try { commit(); } finally { IOUtils.close(indexWriter, this::doClose); }: smaller, and it frees the reader and, through IndexWriter.close(), which closes even when it throws, the lock.

Trade-offs

• On a failed commit, IndexWriter.close() (commit-on-close by default) flushes and commits again (IndexWriter.java:1312-1339), so after a transient error changes could land after the caller already saw an exception, and it does a second round of I/O on a failing disk.

What the checks validated

Not run.

State in the close() javadoc that after close() throws, the caller must call rollback().

Trade-offs

• No code change, but try-with-resources and IOUtils.close users, the facet demos included, stay exposed, and in the tragic form close() stays unusable.

What the checks validated

Not run.

Method: semi-formal (a bounded-exhaustive single-fault enumeration on the real jar). The sweep runs each public operation once per MockDirectoryWrapper failure event, injects a one-shot IOException at exactly that event, and then checks three properties: every write lock can be obtained again, no file is left open, and the injected exception reaches the caller.

• Its two DirectoryTaxonomyWriter add, commit and close scenarios (empty and existing taxonomy, 8,307 runs) gave file-leak and lock-leak outcomes; the buckets count runs, not distinct bugs, and also hold a separate constructor bug.
• The finder root-caused them by reading close() and reduced them to a public-API JUnit test with two methods (the tragic form and the fsync form); the minimal repro keeps the tragic form only.
• The same unit's bytecode exception-edge scan does not model close() of a resource held in a field, and did not flag this.