An interrupted multi-threaded CheckIndex throws NullPointerException instead of reporting the interrupt

CheckIndex checks segments in parallel whenever it has an executor: the default checkIndex() creates a pool when threadCount > 1, and threadCount defaults to availableProcessors() (CheckIndex.java:541, :593-596 at 645d4fe14a20); checkIndex(List, ExecutorService) uses the caller's pool when it is given a non-null one. The calling thread collects each segment's result with segmentInfoStatus = futures[i].get() (:907-909). If it is interrupted while it waits, the catch (InterruptedException) (:910-918) prints "ERROR: Interrupted exception occurred when getting segment check result for segment ..." and falls through: no throw, no assignment, no continue. The null status goes to processSegmentInfoStatusResult (:929), which reads segmentInfoStatus.error (:978) and throws NullPointerException with no cause. The interrupt is swallowed and the remaining checks are not cancelled: on a caller-owned pool they keep running; on the default route the internal pool's finally waits up to 5 s for them, then calls shutdownNow(). No Status is returned and the index files are unchanged (probes). It came with concurrent CheckIndex (LUCENE-9662, #128, 2021), so every release from 8.11.0 to 10.6.0 and main are affected; restoring the flag and throwing ThreadInterruptedException (two lines) fixes it.

1. Which path. checkIndex() calls checkIndex(null) (:577-579). With threadCount > 1 (default availableProcessors(), :541) it creates Executors.newFixedThreadPool(threadCount, ...) (:593-596) and calls checkIndex(onlySegments, executorService) (:600). With threadCount == 1 it passes null and the check runs on the caller, one segment after the other (:823-844). checkIndex(List, ExecutorService) is public, so a caller can also pass its own pool.
2. Submit. The concurrent branch submits one CompletableFuture.supplyAsync(...) per segment, larger segments first (:871-896, :987-990).
3. Collect. For each segment, in order: Status.SegmentInfoStatus segmentInfoStatus = null; (:907), segmentInfoStatus = futures[i].get(); (:909).
4. The bug. catch (InterruptedException e) (:910-918) writes to the infoStream, when one is set, the segment's buffered output, "ERROR: Interrupted exception occurred when getting segment check result for segment " + the name, and a stack trace. It does not throw, assign or continue, so execution reaches processSegmentInfoStatusResult(result, info, segmentInfoStatus) (:929) with a null status.
5. The crash. processSegmentInfoStatusResult adds the null to result.segmentInfos (:977) and reads segmentInfoStatus.error (:978): NullPointerException: Cannot read field "error" because "segmentInfoStatus" is null, with no cause.

• Interrupt flag lost. Future.get cleared it when it threw; the catch does not restore it. The independent reproduction's test reads interruptFlagAfterCall=false on both routes.
• Checks keep running on a caller-owned pool. The futures after i are never cancelled; the intended-behaviour check saw getActiveCount() = 2 right after the throw.
• Up to 5 s wait on the default route. The finally of checkIndex(List) calls shutdown() and awaitTermination(5, TimeUnit.SECONDS) (:603-605). With the flag already gone it really waits, then calls shutdownNow() (:612). The independent reproduction's test: 5.2 s, against 0.19 s with the fix; the intended-behaviour check: about 5,015 ms against 4-8 ms for an uninterrupted control. Its own catch (InterruptedException) (:606-610) also only logs.

CompletableFuture.get on a pending future throws InterruptedException whether the flag is already set when it starts to wait or arrives while it waits (JDK semantics). The minimal test sets it just before the call; the independent reproduction's test interrupts the caller only once it is seen parked in CompletableFuture.get. Both fail at the same two lines.

• Who is hit. Applications that run CheckIndex with more than one thread and stop it with Thread.interrupt(): a cancelled maintenance job, a shutdown hook, or shutdownNow() on a pool whose thread runs the check. Once the checks are submitted, the caller only collects results with futures[i].get(), so on a long multi-segment check it can spend much of its time waiting there, and an interrupt in that phase lands in this catch (reasoned from the code, not measured). Any index with at least one segment to check; assertions on and off.
• Default vs opt-in. The default checkIndex() takes the executor path whenever availableProcessors() is greater than 1 (normally a machine with more than one core, though a container or JVM setting can lower it); checkIndex(List, ExecutorService) does whenever it is given a non-null executor. The command-line tool uses the same default unless -threadCount is given. Interrupting is the opt-in part.
• Effect. An unchecked NullPointerException with no cause instead of an interrupt-type exception: code that catches ThreadInterruptedException, or checks the interrupt flag, to recognise a cancellation sees an unexplained crash instead. Plus the side effects: the flag is lost, checks on a caller's pool keep running, and the default route waits up to 5 s.
• What does not happen. No Status is returned, so there is no false "clean" verdict and no -exorcise can follow. A security probe found the original index files unchanged (every file on the default route; on a caller-owned pool, where it commits a new document on purpose, the original segment files), nothing left open once the leftover checks finished, and the index still checking clean. These are the probes run, not a guarantee for every directory or filesystem.
• Likelihood: rare. Interrupt-based cancellation of a running CheckIndex is unusual. CheckIndex is @lucene.experimental.
• Future.cancel(true) caveat. If the interrupt comes from cancelling a task that runs checkIndex, the submitter's later get() throws CancellationException, not the NPE (JDK semantics; not executed). The NPE shows when the thread is interrupted directly or the task logs its own failure.

Public API only, one method, single caller thread. The pool's thread is part of the API under test; a latch only keeps it busy, so the segment's future is still pending when get() is called. The test accepts both non-buggy outcomes (a returned Status, or ThreadInterruptedException); the NPE fails it with an explaining assertion (any other exception would fail it too). It does not check the interrupt flag, the leftover checks or the shutdown wait.

If a reviewer asks for an interrupt that arrives during the wait: the independent verifier's test interrupts the caller only after it sees it parked in CompletableFuture.get, on a busy newFixedThreadPool(1) and on the default checkIndex() with setThreadCount(2); both fail with the same NPE on 645d4fe14a20. It is a backup, not the regression test.

Suggested regression test: this same test, as its own class or as one method in TestCheckIndex. With the recommended fix it passes through the ThreadInterruptedException branch.

Seeds: DEADBEEF, and 5EED1234 with 5 iterations (5 of 5 fail, -ea and -da); the result does not depend on the seed.

# main 645d4fe14a20, gradle, seed DEADBEEF, -Ptests.asserts=true
TestCheckIndexInterruptedCaller > testInterruptedCallerWithBusyExecutor FAILED
    java.lang.AssertionError: expected a Status or ThreadInterruptedException, got: java.lang.NullPointerException: Cannot read field "error" because "segmentInfoStatus" is null
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:B7CC9B6393C858AA]:0)
        at org.apache.lucene.index.TestCheckIndexInterruptedCaller.testInterruptedCallerWithBusyExecutor(TestCheckIndexInterruptedCaller.java:47)

        Caused by:
        java.lang.NullPointerException: Cannot read field "error" because "segmentInfoStatus" is null
            at org.apache.lucene.index.CheckIndex.processSegmentInfoStatusResult(CheckIndex.java:978)
            at org.apache.lucene.index.CheckIndex.checkIndex(CheckIndex.java:929)
            at org.apache.lucene.index.TestCheckIndexInterruptedCaller.testInterruptedCallerWithBusyExecutor(TestCheckIndexInterruptedCaller.java:45)

1 test completed, 1 failed

# the same with -Ptests.asserts=false; 10.6.0 jars (-ea, -da): CheckIndex.java:942 <- :893
# mid-wait test, both routes: thrown=java.lang.NullPointerException ... interruptFlagAfterCall=false

• The handler's intent. The catch logs the interrupt, with a comment about ordering that output (CheckIndex.java:911-917), then hands on the local it knows is null. Nothing marks the fall-through as deliberate.
• The documented convention (stated for IndexWriter; CheckIndex documents no interrupt outcome). IndexWriter.java:172-175: "If you call Thread.interrupt() on a thread that's within IndexWriter, IndexWriter will try to catch this ..., and will then throw the unchecked exception ThreadInterruptedException and clear the interrupt status on the thread." ThreadInterruptedException.java:20: "Thrown by lucene on detecting that Thread.interrupt() had been called." Of the 22 catch (InterruptedException sites in lucene/core/src/java, 18 throw it; the two in CheckIndex (:606, :910) are the only ones that log and fall through.
• One thread vs several. With threadCount 1 and an interrupted caller the check completes clean and keeps the flag (executed by the intended-behaviour check with RAFDirectory and by a cross-model review with ByteBuffersDirectory; other directories not run). So the same call behaves differently with one setting of one knob.
• History. #637 ("null check on infoStream", 9.1.0) fixed an earlier NPE in this same block (an unguarded print with no infoStream set; its diff also guarded the print right after the try/catch, which by code reading threw on every concurrent check without an infoStream) and left the null status. The introducing PR #128 (98 review comments) and LUCENE-9662 (#10701, 15 comments) never discuss interrupts.
• Tests and docs. No upstream test asserts a CheckIndex interrupt outcome; CHANGES.txt and MIGRATE.md say nothing about it.
• Acceptable outcomes. The sequential path ignores the interrupt; it does not throw ThreadInterruptedException. Either of two outcomes avoids the NPE: report as the handler intends, or throw ThreadInterruptedException; no single choice (a particular exception type, keeping the interrupt flag) is the only correct one.

The strongest case for "not a bug" (the devil's-advocate verifier and the cross-model reviews): CheckIndex is experimental and no javadoc states an interrupt policy; the Directory javadocs warn that interrupting a thread blocked on NIO I/O closes the channel (FSDirectory.java:77-82), so Lucene does not support interrupts; and TestIndexWriter clears the flag before it calls TestUtil.checkIndex (TestIndexWriter.java:1036-1037).

Why it fails: the interrupted thread is parked in Future.get and does no I/O. The javadoc's remedy for applications that use Thread.interrupt() is RAFDirectory, and the verifier's probe with RAFDirectory fails the same way; the minimal test uses ByteBuffersDirectory. "Experimental" allows API changes, not a dereference of a known-null local. The TestIndexWriter line dates from 2010, 11 years before CheckIndex became concurrent, and serves the writer rollback. What the argument does show is that there is no written contract: the defect is the NPE, not a broken promise.

Option A was validated with gradle in a scratch worktree of 645d4fe14a20, and option B the same way. Do not record an error status for the interrupted segment: processSegmentInfoStatusResult keeps only error-free segments in newSegments (CheckIndex.java:978-984) and exorciseIndex commits newSegments (:4606-4614), so -exorcise and repair tools would then drop an unchecked, possibly healthy segment. Do not silently skip it either: the Status could then say "clean" for an index that was not fully checked.

At the end of the catch (InterruptedException e) around futures[i].get() (CheckIndex.java:910-918), add two lines and the import. One file, +3 lines.

          if (infoStream != null) e.printStackTrace(infoStream);
          Thread.currentThread().interrupt();
          throw new ThreadInterruptedException(e);
// plus: import org.apache.lucene.util.ThreadInterruptedException;

Trade-offs

• The caller gets Lucene's usual unchecked interrupt exception, with the InterruptedException as cause, and keeps its flag on a caller-owned pool. It is a RuntimeException like today's NPE, so no catch (IOException) changes meaning.
• Not covered: on a caller-owned pool the remaining checks still run; on the default route the finally block's own catch (:606) clears the restored flag again (flag true on a caller's pool, false on the default route). That route no longer waits 5 s: shutdownNow() runs at once (0.19 s against 5.2 s) and interrupts any check still running, as it already did after the wait.
• Restoring the flag is a choice for the maintainers: the IndexWriter javadoc says IndexWriter clears it. Throwing ThreadInterruptedException without restoring it would match that wording; restoring it lets callers that check the flag see the cancellation.

What the checks validated

• Gradle, scratch worktree of 645d4fe14a20 with the repro test added, seed DEADBEEF: the repro passes with -Ptests.asserts=true and false (the test also accepts a Status; the ThreadInterruptedException itself is shown by the mid-wait test and the diagram probe's fix runs).
• --tests 'org.apache.lucene.index.TestCheckIndex*' --tests org.apache.lucene.index.TestIndexWriter: 129 tests (9 + the repro + 119), 0 failures, 1 skipped; the same run without the fix has exactly 1 failure, the repro. :lucene:core:checkGoogleJavaFormat passes.
• The mid-wait test passes 2 of 2 with the fix shadowed (ThreadInterruptedException on both routes).
• Earlier: the same two lines passed the independent verifier's test, the adjudication's re-run, the history A/B at the introducing commit and the registry re-run.

Before the throw, cancel futures[j] for j >= i (queued checks then never start; a running check still finishes, because CompletableFuture.cancel does not interrupt; from CompletableFuture semantics, not measured), and add Thread.currentThread().interrupt(); to the catch at :606-610. One file, +9 lines.

          for (int j = i; j < numSegments; j++) {
            if (futures[j] != null) {
              futures[j].cancel(false);
            }
          }
          Thread.currentThread().interrupt();
          throw new ThreadInterruptedException(e);
// and in the catch at :606-610: Thread.currentThread().interrupt();

Trade-offs

Fixes the side effects too, at the cost of more code for things that are not the reported bug; a check that is already running still goes on until it finishes (read-only, the work it would have done anyway), and on a caller-owned pool nothing stops it earlier.

What the checks validated

Gradle, same worktree and seed: the repro passes (-Ptests.asserts=true; B was not run with assertions off); the same suites pass (129 tests, 0 failures, 1 skipped); checkGoogleJavaFormat passes. The mid-wait test passes 2 of 2 and keeps the interrupt flag on both routes.

Wait for every result without giving up on the interrupt, then restore the flag before returning. Consistent with threadCount 1, but a long check could then not be cancelled at all. Not run.

Non-formal. A hunt unit swept Lucene APIs that accept an Executor with a zoo of executor behaviours (rejecting, caller-runs, interrupting a worker, and so on) and compared their results. Its interrupt cells for CheckIndex interrupted a pool thread, which CheckIndex handles, and stayed clean. Reading the code next to those cells showed that the caller's own InterruptedException handler falls through with a null status. The finder then wrote a deterministic JUnit (a gated executor, the caller interrupted before the call), which failed on the first run with assertions on and off.