MergeObserver.await() returns true after a failed or aborted merge, although its javadoc says false on error

IndexWriter.forceMergeDeletes(false) (since 10.4.0) hands the forced-deletes merges to the merge scheduler without waiting (with the default ConcurrentMergeScheduler they run on background threads) and returns a MergePolicy.MergeObserver (@lucene.experimental). Its await() is documented to return true if all merges completed successfully and false on error. It returns true as soon as the merges stop, whatever their outcome: each merge's CompletableFuture<Boolean> is completed normally with the success flag as its value, and await() waits for the futures without reading the values; its false branches catch exceptions that a failed merge never produces. So after a merge that failed with an I/O error (the writer then closes itself) or was aborted by rollback() or deleteAll() (no tragic exception; after deleteAll() the writer stays open), the caller is told the merges succeeded, and the observer itself has no other way to report the outcome. Nothing in the index goes wrong, the blocking forceMergeDeletes(true) throws on failure, and no Lucene code calls the observer: a minor, deterministic contract bug in a young, experimental API, in 10.4.0 to 10.6.0 and main. A three-line check of hasCompletedSuccessfully(), the one addIndexes(CodecReader...) already makes, fixes it and passes the whole core test suite; a javadoc correction plus a public success query is the alternative.

// MergePolicy.java:285-290  OneMerge.close, called once per merge, success or not
final void close(boolean success, boolean segmentDropped, IOConsumer<MergeReader> readerConsumer)
    throws IOException {
  if (mergeCompleted.complete(success) == false) {   // :289 completes NORMALLY, value true or false
  ...

// MergePolicy.java:533-544  MergeSpecification.await() (package-private)
boolean await() {
  try {
    CompletableFuture<Void> future = getMergeCompletedFutures();   // :528-531 allOf(every mergeCompleted)
    future.get();
    return true;                                                    // :538 every merge finished, any outcome
  } catch (InterruptedException e) {
    throw new ThreadInterruptedException(e);
  } catch (ExecutionException | CancellationException _) {
    return false;                                                   // :541-542 never reached by a failed merge
  }
}

// MergePolicy.java:908-917  MergeObserver.await() (public)
 * @return {@code true} if all merges completed successfully or no merges were needed, {@code
 *     false} on error
public boolean await() {
  return spec == null || spec.await();
}

1. forceMergeDeletes(false) (IndexWriter.java:2345-2405) flushes one segment (10 documents, 1 deleted), asks the merge policy for forced-deletes merges (1), registers it, hands it to the merge scheduler and returns new MergePolicy.MergeObserver(spec); with doWait == false it skips the loop that checks for failures (:2369-2397).
2. The merge fails on the merge thread: the Directory throws when the merge creates its first file. The exception is thrown inside mergeMiddle's try (IndexWriter.java:5482-5483), and mergeMiddle's own finally calls closeMergeReaders(merge, true, false) (:5808-5813), which calls merge.close(false, ...) (:5394-5402): mergeCompleted.complete(false), before the exception reaches IndexWriter.merge (:5065-5114). There handleMergeException rethrows it; the second closeMergeReaders in merge's finally (:5092) is a no-op (hasFinished(), :5397), and only after that finally does tragicEvent(t, "merge") record the failure (:5112). A probe traced the completing call: closeMergeReaders:5400 <- mergeMiddle:5812 <- merge:5081.
3. await() returns true. allOf(...).get() returns normally once the future is complete; the Boolean is never read. Nothing in org.apache.lucene.index completes mergeCompleted exceptionally or cancels it (it is private final; grep finds no completeExceptionally or cancel), so the false branches are reachable only by a timeout in await(timeout).
4. An abort ends the same way. rollback() and deleteAll() call abortMerges() (IndexWriter.java:2621, :2771, :2821-2861): a pending merge goes through abortOneMerge (:5133-5136, closeMergeReaders(merge, true, false)), a running one stops with a MergeAbortedException that handleMergeException swallows (:5043-5053), then merge's finally completes the future (:5092; probe: closeMergeReaders:5400 <- merge:5092). No tragic event.
5. The outcome is recorded but not public. OneMerge.hasCompletedSuccessfully() (MergePolicy.java:452-459) returns the flag, and addIndexes(CodecReader...) checks it right after its own spec.await() (IndexWriter.java:3313-3315). The observer's numCompletedMerges() counts failed merges as completed (isDone(), :901) and awaitAsync() completes normally, so nothing on the observer tells success from failure. Outside it the routes are indirect: IndexWriter.getTragicException() reports a tragic failure but not an abort, and a custom merge policy can wrap its merges and override the public hook OneMerge.mergeFinished(success, segmentDropped).

• Who is hit. Applications that call forceMergeDeletes(false) (since 10.4.0; MergeObserver is @lucene.experimental) and use await() or await(timeout, unit) as a success signal, when a forced-deletes merge fails (an I/O error such as a full disk) or is aborted by their own rollback() or deleteAll() while it runs. The default component types (IndexWriterConfig, ConcurrentMergeScheduler, TieredMergePolicy) suffice once a segment's deleted share is above the policy's threshold (more than 10% by default); the repro sets the threshold to 0 so that 1 delete in 10 documents qualifies. No scale and no timing are needed for the I/O path; assertions on or off.
• What goes wrong. A wrong true from a method whose only job is to report the outcome: the caller believes the deleted documents were merged away although the merge never finished. The observer itself offers no other way to learn the outcome; the routes outside it are indirect.
• How much it hides. An I/O failure is tragic: the writer closes itself, and later calls throw AlreadyClosedException, so the failure surfaces soon. But the future completes before the writer records the tragedy: in a probe that deleted without committing, then called await() and commit(), commit() still succeeded in 3 of 30 iterations (and threw in 27). That depends on timing, so it shows only that the writer can still accept a commit right after await() returns. An abort records no tragic exception, and after deleteAll() the writer stays open, so await()'s true is all the observer reports.
• What does not go wrong. The failed merge is never committed; committed deletes stay hidden after a failed or aborted merge (probe: 0 of 30 deleted ids visible after reopening). Uncommitted deletes are lost only when the writer closes itself after the tragic failure and rolls back to the last commit, and the application then sees the exception from commit(). No Lucene code calls MergeObserver.await() (only the test framework's RandomIndexWriter passes the observer through; luke calls the blocking form).
• Downstream. A read-only public code search (not exhaustive) found Elasticsearch, OpenSearch and Solr calling the blocking forceMergeDeletes.
• Impact class: minor (a wrong return value of an experimental API with no in-tree caller); priority low.

Why each piece is there (from minimization): handleMergeException is overridden empty, otherwise the merge thread's exception escapes and the test framework fails the test for an uncaught exception, not for the claim; cms.sync() is only for the precondition, because the future completes before the writer records the tragic exception; with 2 documents and 1 delete a natural merge took the segment first, numMerges() was 0 and await() rightly returned true, so the test uses 10 documents with 1 delete and setForceMergeDeletesPctAllowed(0). Only the untimed await() is asserted, because its @return is unambiguous.

Nothing uses random(); the seed sets only the test framework's randomization (codec, locale, timezone and similar), and seeds DEADBEEF and 5EED1234 (10 iterations) fail the same way, assertions on and off. The same failure, by the repro harness: main 645d4fe14a20 and the 10.6.0 jars, assertions on and off. Use this one test as the regression test too. The file has no ASF license header, and ./gradlew tidy rewraps one 101-character comment.

The abort path (rollback() or deleteAll() while the merge runs, no fault injection) has the same root cause; it was run by the independent reproducer (await(timeout) asserted, rollback()) and the devil's advocate (both awaits, deleteAll()) with their own tests, and the fact check ran the untimed await() after rollback() too (true) and needs a merge held running until it is aborted, so it is not part of the minimal repro.

# gradle, main 645d4fe14a20, seed DEADBEEF, assertions on
TestMergeObserverAwaitOnFailure > testAwaitAfterFailedMerge FAILED
    java.lang.AssertionError: expected await() == false after the failed merge, got true
        at org.apache.lucene.index.TestMergeObserverAwaitOnFailure.testAwaitAfterFailedMerge(TestMergeObserverAwaitOnFailure.java:50)
1 test completed, 1 failed

# repro harness, the same message: main -ea, main -da, 10.6.0 -ea, 10.6.0 -da (1 run, 1 failed each)
# both preconditions (lines 43 and 46) pass first; line 50 is the final assertFalse
# with fix A: the test passes (gradle; harness -ea and -da)

• Contract. MergePolicy.java:912-913, await(): "@return true if all merges completed successfully or no merges were needed, false on error". :925-926, await(timeout, unit): "@return true if all merges completed within timeout or no merges were needed, false on timeout or error".
• The documented false is unreachable. The only false branches catch ExecutionException and CancellationException (untimed) or ExecutionException and TimeoutException (timed); nothing completes the futures exceptionally or cancels them, and an interrupt throws instead of returning false. A contract with a dead branch is a mismatch, whichever side is changed.
• Aborts are meant to be visible. The deleteAll() javadoc: "this method will forcefully abort all merges in progress. If other threads are running forceMerge, addIndexes(CodecReader[]) or forceMergeDeletes methods, they may receive MergePolicy.MergeAbortedExceptions" (IndexWriter.java:2743-2745).
• History: an oversight when the API went public. #15378 (2025-11-02) wrapped the package-private MergeSpecification.await methods unchanged and wrote the two @return lines; git blame gives every line of both await methods and their javadoc to that change. Its review (23 inline comments, 16 reviews) covers naming, numCompletedMerges and toString, never failure semantics; its tests cover a successful merge, no merges and a timeout, never a failed or aborted merge.
• Nothing found says otherwise. Our searches (see Prior reports, with their limits) found no test, review comment, issue, CHANGES/MIGRATE text or list thread saying that await() returns true after a failure.

Strongest case for "intended" (the cross-model reviews rated it strong; a later cross-model fact check rated it moderate): await() means completion, by design. The package-private await(timeout) (LUCENE-8962, 2020) and await() (LUCENE-10216, 2022) were always completion barriers, with success as the separate query hasCompletedSuccessfully(), which addIndexes checks separately (IndexWriter.java:3313-3315). Everything around the @return lines speaks of completion: the class javadoc ("Provides methods to query merge status and wait for completion"), forceMergeDeletes' @return ("to monitor merge progress and wait for completion"), awaitAsync() and the CHANGES entry. The timed @return contradicts itself for a failed merge. An I/O failure is tragic, so the writer reports it anyway. And the API is @lucene.experimental.

Why it fails: it explains the code, not the public contract. The untimed @return says "completed successfully" and "false on error" without ambiguity, and that false can never happen; the observer itself gives callers no other way to learn the outcome (hasCompletedSuccessfully() is package-private, numCompletedMerges() counts failed merges, awaitAsync() completes normally), and the routes outside it are indirect (getTragicException() misses aborts; a custom merge policy must wrap its merges); and the abort path is not tragic, so nothing else reports it. The argument does decide the shape of the fix: a javadoc correction is a legitimate answer, and both are listed under Suggested fix. Verdict: a real contract mismatch (adjudication: confirmed, high).

One code fix was run; A is recommended, B is the javadoc route maintainers may prefer. Workaround for users today: call the blocking forceMergeDeletes(true), which throws on a failed merge, or check IndexWriter.getTragicException() after ConcurrentMergeScheduler.sync() (it does not cover aborts).

After the wait, require hasCompletedSuccessfully() of every merge, as addIndexes(CodecReader...) does (IndexWriter.java:3314-3315). The check runs only once every merge has finished, so orElse(false) is never taken. The package-private MergeSpecification.await and its addIndexes caller are untouched. A merge whose segment was dropped because all its documents were deleted still counts as a success (close(true, true, ...) from commitMerge). The diagrams show this same fix A, run in the diagram probe. The full diff also changes one word in the timed @return.

public boolean await() {
  return spec == null || (spec.await() && allMergesSucceeded());
}

public boolean await(long timeout, TimeUnit unit) {
  return spec == null || (spec.await(timeout, unit) && allMergesSucceeded());
}

private boolean allMergesSucceeded() {
  return spec.merges.stream().allMatch(m -> m.hasCompletedSuccessfully().orElse(false));
}

Trade-offs

A behaviour change in an experimental API: a caller that used await() as a pure barrier now gets false after a failure or abort, which is what the javadoc says. In the timed variant false now means timeout or failure; checking numCompletedMerges() == numMerges() afterwards hints at which, but not atomically (the last merge may finish in between). Aborts the caller started itself (rollback(), deleteAll()) now also give false; whether those count as an "error" is an API-design call for the maintainers. awaitAsync() still completes normally after a failure; its javadoc promises only completion, so leave it or add a sentence saying so.

What the checks validated

• Scratch worktree of main 645d4fe14a20 (removed afterwards), seed DEADBEEF: stock, the test fails through gradle; with A it passes.
• The repro and the 9 closest suites with A, -Ptests.asserts=true and false: TestMergeObserverAwaitOnFailure, TestIndexWriterMerging (the upstream MergeObserver tests), TestMergePolicy, TestAddIndexes, TestConcurrentMergeScheduler, TestIndexWriterForceMerge, TestTieredMergePolicy, TestIndexWriterExceptions, TestIndexWriterOnDiskFull, TestIndexWriterDelete: 172 tests, 11 skipped, 0 failures each time.
• All of :lucene:core:test with A (assertions on): 8,898 tests in 749 classes, 322 skipped, 0 failures.
• With A shadowed in the repro harness: the repro passes (assertions on and off), and so do the devil's advocate's 2 tests (I/O failure, deleteAll() abort) and the independent reproducer's 3 (I/O failure, rollback() abort, successful-merge control).
• checkGoogleJavaFormat reports no violation in MergePolicy.java.

Say that both methods report completion ("true once all merges have finished, successfully or not, or no merges were needed; false only on timeout"), and add a public query such as boolean allMergesSucceeded() (A's helper, made public).

Trade-offs

No behaviour change. A javadoc change without the query leaves the non-blocking API with no way to report a failed or aborted merge. Not run.

Return false there when a merge completed with success == false.

Trade-offs

The same effect for the observer only if both package-private await variants change; their other callers ignore the return value (addIndexes(CodecReader...), which checks success itself, and the merge-on-commit and merge-on-getReader waits, IndexWriter.java:739, :3894), but it changes the completion-only internal layer from 2020/2022 for no gain over A. Not run.

• Method (semi-formal). The hunt unit modelled the ConcurrentMergeScheduler / IndexWriter merge lifecycle in TLA+, with a merge-completion property (every merge ends closed). Writing that property meant reading how merges complete, and the finder saw that OneMerge.close completes the future normally with a success flag that the public await() never reads.
• Witness. The model does not represent await's return value, so the model checker produced no trace. The finder wrote a public-API JUnit with a FilterDirectory that fails merge output; it failed on main with assertions on and off, and later was minimized to the one test above.
• The finder labelled it formal; the registry records semi-formal, because the hypothesis came from code reading while modelling and a JUnit witnessed it.