IndexWriter.hasUncommittedChanges() can return false for an uncommitted delete while another thread flushes

IndexWriter.hasUncommittedChanges() evaluates changeCount.get() != lastCommitChangeCount || hasChangesInRam() without a lock: first the change counter, then the in-RAM buffers. A buffered delete is visible only to the second check, because deleteDocuments does not increment the counter (unless buffered deletes use too much heap; then they are applied at once). A flush (flush() and an NRT reader open were run; a refresh or a RAM-triggered apply takes the same path) applies the delete, increments the counter in checkpoint(), and only then removes the delete from the buffered-updates stream. A call that reads the counter before that checkpoint() and the buffers after the delete left them returns false, although the delete was acknowledged before the call and is still not committed. The next call returns true. The writer still holds the delete and the next commit() or close() writes it, so the impact is Low: only code that decides whether to commit from this method skips a commit, and the delete is lost only if rollback() or a crash comes first. The read order dates from LUCENE-5289 (2013) and is in every release from 4.5.1 through 10.6.0 (read in all 114 release tags; 13 of them were run and fail). Reading the counter again after an empty RAM check fixes the demonstrated case without adding a lock.

1. The method. IndexWriter.java:4225-4227, no lock: return changeCount.get() != lastCommitChangeCount || hasChangesInRam();. changeCount is an AtomicLong, lastCommitChangeCount a volatile long (:272-273). hasChangesInRam() is docWriter.anyChanges() || bufferedUpdatesStream.any() (:4230-4232): documents in RAM, the delete queue (read under its globalBufferLock), the flush tickets and the full-flush flag (DocumentsWriter.java:344-357, :376-378; DocumentsWriterDeleteQueue.java:181-196), then bytesUsed != 0 of the buffered-updates stream (BufferedUpdatesStream.java:108-110).
2. A delete only buffers (for a small delete like the repro's). deleteDocuments(Term...) hands the term to the delete queue (IndexWriter.java:1864-1872); the diagram probe prints changeCount 4 = lastCommitChangeCount 4 after the delete. When buffered deletes use too much heap, DocumentsWriter.applyDeleteOrUpdate freezes them at once and deleteDocuments applies them before it returns (DocumentsWriter.java:153-186, IndexWriter.java:6345-6350); that is not the case shown here. An added document is different: creating its in-memory segment increments changeCount at once (:2173-2179), which explains why add-only changes were not seen to fail (0 of about 17,000 hook-free rounds; the tested case, not a proof for every add path).
3. A flush moves the delete stage by stage. flush() runs doFlush (IndexWriter.java:4403-4413): flushAllThreads() freezes the delete into a flush ticket; publishFlushedSegments(true) pushes the frozen packet to the buffered-updates stream (IndexWriter.java:2928-2947), then lowers the ticket count (DocumentsWriterFlushQueue.java:99-105); processEvents(false) runs the packet's apply event, tryApply(packet). If another thread already holds the packet, tryApply returns and that thread applies it (IndexWriter.java:6491-6500); flush() then waits for all packets (:4417-4419, :4440-4452). Whoever applies it runs forceApply (:6508): updates.apply(segStates) (:6592), then finishApply (:6597) calls checkpoint() because a document was deleted (:6721-6724), which increments changeCount (:2923-2926); then bufferedUpdatesStream.finished(updates) (:6640) subtracts the packet's bytes (BufferedUpdatesStream.java:152-167). A delete packet that matches no document is finished with no checkpoint() (:6721; or it reaches no segment, :6562-6567, :6661-6664); that delete leaves nothing to commit. The NRT path getReader runs the same steps (IndexWriter.java:584-593).
4. Each hop overlaps the next. The ticket count goes up before the freeze (DocumentsWriterFlushQueue.java:39: "first inc the ticket count - freeze opens a window for #anyChanges to fail"), the packet reaches the stream before the ticket count goes down, and the counter goes up before the packet leaves the stream. A reader that looks at the stages in the order the change travels (RAM first, then the counter) always finds a delete that matches a document.
5. The method reads the other way round. It reads the last stage, the counter, first. If checkpoint() runs after that read and finished() before its stream read, it sees neither. The caller does not have to straddle a whole flush, only the short gap between checkpoint() and finished(), which is why a busy-polling caller can hit it.
6. The sibling is safe. nrtIsCurrent (IndexWriter.java:6140-6146) is synchronized on the writer, like checkpoint() and the finished() call, so it cannot fall into this gap.

The minimal test (one committed document, one delete, one flush()) returns false with assertions on and off, 10 of 10 random seeds. A TLA+ model of the two reads against one full flush gives a 16-state counterexample for the stock order and passes exhaustively for both fixes (105 and 117 distinct states); "exhaustively" holds for that model only (one matching delete, no buffered documents, one full flush; no commits, non-matching packets, merge retries or doc-values updates). Hook-free public-API runs with busy-polling threads, default config: 47 of 8,753 flush rounds in 10 s (adjudication, 63 wrong calls among about 2.94 billion); with 2 polling threads 238 of 19,211 flush rounds (302 wrong calls among about 2.56 billion) and 173 of 20,962 NRT-open rounds, 0 with the re-read fix; one call per flush round 0 of 61,904.

• Who. Code that calls hasUncommittedChanges() on one thread while another thread runs flush() or DirectoryReader.open(writer) (both run), or openIfChanged, SearcherManager.maybeRefresh or a RAM-triggered apply of deletes (the same path by reading, not run); default IndexWriterConfig. IndexWriter is documented as "completely thread safe, meaning multiple threads can call any of its methods, concurrently": that promises safe concurrent calls, not an atomic snapshot of the writer's state.
• What breaks. A transient false for an acknowledged, uncommitted delete. Code that commits only when the method returns true (a periodic "commit if dirty" task, a shutdown step, a replication trigger) skips that commit.
• What does not break. The writer still holds the delete: the next commit() or close() (commitOnClose defaults to true) writes it, a newly opened NRT reader already sees it, and searches are correct. The delete is lost only if the skipped commit was the last one before rollback() or a crash; otherwise a skipped commit only delays durability. No wrong search result, corruption or downstream incident was shown. In Lucene itself the only non-test caller is an assert in IndexUpgrader.
• How often. Not seen with one check per flush: 0 of 61,904 rounds. Observable under sustained busy polling while another thread flushes: 47 of 8,753 rounds in 10 s; with NRT opens, 173 of 20,962 rounds. Each such round had at least one wrong call among billions of tight-loop calls (302 among about 2.56 billion in the tie-break flush run), so these are not rates for a periodic poller.
• Scope. Deletes are shown; doc-values updates take the same path but were not executed, so they are not claimed. Add-only changes were not seen to fail (0 of about 17,000 rounds; the tested case, not a proof).

The test sits in org.apache.lucene.index because it uses the delete queue's globalBufferLock ("pkg-private for access by tests"; Lucene's own TestDocumentsWriterDeleteQueue.testPartiallyAppliedGlobalSlice holds it the same way) and IndexWriter.getDocsWriter() ("for testing only"). The lock only stands in for thread B being descheduled between its counter read and its RAM read; flush() runs exactly as without it, because the lock is reentrant. So the test forces the schedule through package-private hooks and is not a public-API-only repro; the public-API program needs sustained busy polling to hit the window.

Regression test: this same test. It passes with either fix and can go into TestIndexWriter next to testHasUncommittedChanges. The bare AssertionError comes from the assertTrue without a message; a message does not change the result.

cp $REPRO/TestHasUncommittedChangesConcurrentFlush.java $LUCENE/lucene/core/src/test/org/apache/lucene/index/
cd $LUCENE && ./gradlew :lucene:core:test --tests TestHasUncommittedChangesConcurrentFlush -Ptests.seed=DEADBEEF
# the same result with -Ptests.asserts=false

# gradle, stock 645d4fe14a20, -Ptests.seed=DEADBEEF (assertions on; the same with -Ptests.asserts=false)
TestHasUncommittedChangesConcurrentFlush > testFlushBetweenChangeCountAndRamChecks FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:13DB0C8BB0ABC52C]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.apache.lucene.index.TestHasUncommittedChangesConcurrentFlush.testFlushBetweenChangeCountAndRamChecks(TestHasUncommittedChangesConcurrentFlush.java:55)

# with either fix: 1 test, 0 failures (assertions on and off)

• The contract. IndexWriter.java:4218-4224: "Returns true if there may be changes that have not been committed. There are cases where this may return true when there are no actual "real" changes to the index, for example if you've deleted by Term or Query but that Term or Query does not match any documents. Also, if a merge kicked off ... this method may return true right after you had just called commit." Every listed exception is a false positive.
• The test. TestIndexWriter.testHasUncommittedChanges asserts true right after a buffered deleteDocuments (TestIndexWriter.java:2418-2419), sequentially.
• The history. LUCENE-5289 (#6353) fixed the sequential form of this false as a bug and was backported to 4.5.1 because "Solr's replication depends on this feature" (Shalin Shekhar Mangar on #6353). Michael McCandless explained there: "It was failing to take into account any buffered changes that hadn't yet been 'resolved' (which increments the changeCount)." The fix (f9f0cdd56fd) added the RAM checks after the counter test. The javadoc's two "may"s both describe false positives: the merge caveat ("this method may return true right after you had just called commit") was added on the day of the fix (e1d5edea421, 2013-10-17, released in 4.5.1), and the hedge "there may be changes" with the no-match delete example came in a3c48b5ac41 ("improve javadocs", 2014-01-17, first in 4.7.0).
• Precedent. LUCENE-3546 (#4620) and LUCENE-3551 (#4625, "Yet another race in IW#nrtIsCurrent") fixed the same kind of in-transit "no change" window in nrtIsCurrent; the comment at DocumentsWriterFlushQueue.java:39 comes from that work (Simon Willnauer's "window for #anyChanges" reasoning).

The strongest case: the method is a deliberately cheap predicate that takes no writer lock. In LUCENE-5079 (#6143), which added it, Uwe Schindler argued for synchronized; Yonik Seeley chose a volatile read because it "might be nice to have on a GUI display", and Michael McCandless agreed ("volatile is best"). The javadoc says "may", and "thread safe" does not promise a linearizable answer. In a 2014 java-user thread Michael McCandless gave "Just history :)" and "maybe also paranoia/defensive" as the reasons commit() does not consult the method, and asked what the application was doing "that it so strongly relies on knowing whether commit() would do anything".

Why it fails: the cheap-read choice was made for a single comparison, changeCount != lastCommitChangeCount, which cannot give this answer; the two-source read came later, and neither fix adds a lock. Both "may"s describe false positives only. No linearizability is needed: in the case shown (a delete that matches a document, acknowledged before the call, no concurrent commit) the delete existed and stayed uncommitted for the whole call, so false matches no state the writer was in during the call. The same 2014 reply calls a wrong answer "a bug in hasUncommittedChanges", and LUCENE-5289 was fixed because downstream code gates commits on it. Verdict: not intended; it supports the Low impact, not "works as designed". A cross-model fact check rated the intended case moderate: the javadoc lists no false-negative exception but does not promise an atomic answer either, so the issue is best framed as a question (is the stronger behaviour wanted?).

Two one-statement fixes were validated in a scratch worktree of 645d4fe14a20. Both close the demonstrated case: for a delete that matches a document, checkpoint() increments the counter before the packet leaves the buffered-updates stream. A delete that matches no document is finished without a checkpoint(); a false is then not wrong, because that delete leaves nothing to commit. Neither was proven for concurrent commit(), rollback(), close or tragedy paths, merge retries or doc-values updates. A third option is not needed.

One file, +5/-1 in IndexWriter.hasUncommittedChanges(). Preferred by the adjudication and the intended-lens reviewer.

  public final boolean hasUncommittedChanges() {
    // a flush moves a change from RAM to changeCount (checkpoint) before it leaves RAM, so check
    // changeCount again after an empty RAM check
    return changeCount.get() != lastCommitChangeCount
        || hasChangesInRam()
        || changeCount.get() != lastCommitChangeCount;
  }

Trade-offs

One extra volatile read, only when the method is about to return false. It keeps the counter-first fast path, so no delete-queue lock is taken when the counter already differs, and it adds no lock (no writer lock; the RAM check takes the delete queue's lock as it does today), in line with the cheap read LUCENE-5079 chose.

What the checks validated

• Stock: the minimal test fails with assertions on and off.
• With the fix: the minimal test passes with assertions on and off.
• Closest suites with the fix: TestIndexWriter (incl. testHasUncommittedChanges), TestIndexWriterCommit, TestIndexWriterDelete, TestIndexWriterReader, TestIndexWriterNRTIsCurrent, TestIndexWriterWithThreads, TestIndexWriterThreadsToSegments, TestDocumentsWriterDeleteQueue: 8 classes, 209 tests, 0 failures, 11 skipped.
• :lucene:core:checkGoogleJavaFormat, ecjLintMain and forbiddenApisMain pass.
• Earlier evidence: TLC passes exhaustively for the model (117 distinct states), and the hook-free race shows 0 false answers in 25,579 flush and 34,622 NRT-open rounds.
• Not run: nightly tests, benchmarks.
• Not proven: concurrent commit(), rollback(), close or tragedy paths, merge retries, doc-values updates.

return hasChangesInRam() || changeCount.get() != lastCommitChangeCount;: it reads the stages in the order the change travels.

Trade-offs

Always evaluates hasChangesInRam() first, which takes the delete queue's lock whenever no document is buffered, even when the counter alone would answer. The cost was not measured.

What the checks validated

• With the swap: the minimal test passes with assertions on and off (same gradle run; closest suites not re-run for this option).
• Earlier evidence: TLC passes exhaustively for the model (105 distinct states); the independent reproduction's looped race 0 of 22,150 rounds; testHasUncommittedChanges* still pass (refute review).
• Same limits as the re-read fix.

Like nrtIsCurrent, this would close the window.

Trade-offs

LUCENE-5079 chose a cheap read without the writer lock on purpose ("there is IO that happens when 'this' is locked", Michael McCandless on #6143), and neither fix above adds one.

Method: code reading, then executed checks (the study's "non-formal" tier: the first signal was a code-reading lens, the TLA+ model came after).

• A lens on IndexWriter's check-then-act and read-order sites proposed that hasUncommittedChanges() reads its two sources in an order that a flush can slip between.
• A choreographed probe (the checker parked between its two reads while another thread's flush ran) and a hook-free public-API race witnessed the false answer on main, the 10.6.0 RC1 and 10.5.2 (flush: 32 of 67,775 rounds; NRT open: 42 of 75,992; 0 with the fix).
• A TLA+ model of the read sequence against one full flush (TLC): a 16-state counterexample for the stock order; exhaustive passes for the swapped order (105 distinct states) and the re-read (117).
• The finder reduced it to one deterministic JUnit; independent checks then reproduced it (below).