IndexWriter.prepareCommit() can return the next commit's sequence number

IndexWriter.prepareCommit() stores the sequence number of the commit it prepared in the shared volatile field pendingSeqNo, then runs the post-flush maybeMerge(FULL_FLUSH) outside commitLock, then returns the field again instead of its own value (IndexWriter.java:3735-3743 on main). The prepared commit is already published at that point, so another thread may finish it with commit() (which returns the right number, a local copy), index more and call prepareCommit() again, which overwrites the field. The first call then returns the second commit's number: in the repro 6, while commit() returned 3 for the same commit and operation 4 is not in it. This breaks prepareCommit()'s @return ("All sequence numbers <= this value will be reflected in the commit, and all others will not"). The index contents are correct and nothing throws. It needs one writer's two-phase commit driven from two threads in this interleaving, so it is contrived. The merge step was moved between the store and the re-read by a deadlock fix (LUCENE-7570) first released in 6.4.0; every release since, through 10.6.0, and main are affected. The fix returns the local value.

• Sequence number: every IndexWriter method that changes the index returns a long that orders the change. commit() and prepareCommit() return one too, "describing which changes are in the commit point and which are not" (IndexWriter.java:117-121).
• Two-phase commit: prepareCommit() flushes, syncs and writes most of the next segments_N (the commit is now pending); commit() finishes it (:3720-3732). commit() with no pending commit does both phases itself.

// IndexWriter.java:3735-3743
public final long prepareCommit() throws IOException {
  ensureOpen();
  pendingSeqNo = prepareCommitInternal();   // shared volatile field (:280)
  // we must do this outside of the commitLock else we can deadlock:
  if (maybeMerge.getAndSet(false)) {
    maybeMerge(config.getMergePolicy(), MergeTrigger.FULL_FLUSH, UNBOUNDED_MAX_MERGE_SEGMENTS);
  }
  return pendingSeqNo;                      // re-reads the field
}

1. prepareCommitInternal() holds commitLock (:3775), publishes the pending commit (pendingCommit = toSync, :5984) and returns its number, then releases the lock.
2. prepareCommit() stores the number and runs the merge step with no lock held: the merge policy picks merges and the merge scheduler gets them (:2442-2452). SerialMergeScheduler runs them on this thread (SerialMergeScheduler.java:32-40); ConcurrentMergeScheduler may stall this thread while merges are backed up (ConcurrentMergeScheduler.java:566, :609).
3. Meanwhile another thread's commit() takes commitLock, sees the pending commit ("commit: already prepared"), takes seqNo = pendingSeqNo and finishes it (:4253-4260); it returns that local copy (:4268). Its next prepareCommit() is accepted, since no commit is pending now (:3787-3790), and stores the next commit's number in the field.
4. Thread A leaves the merge step and returns the field (:3742): the next commit's number.

In the repro: the first addDocument gets 1; the main thread's commit() finishes A's commit (1 document) and returns 3; the next addDocument gets 4; the second prepareCommit() returns 6; A's prepareCommit() returns 6, expected 3. The final commit() returns 6 and holds 2 documents. (Sequence numbers skip values around a full flush; the gaps are normal.)

Related, not claimed (read only; no test targets it, never observed): the field is stored after prepareCommitInternal releases commitLock, so a commit() that takes the lock in that short gap would read the previous value.

• Who is hit. Only applications that let a second thread finish a commit another thread prepared, and prepare the next one, while the first prepareCommit() is still in its merge step, and that then trust the first call's number as the cut of that commit. Lucene's own coordinator (TwoPhaseCommitTool) and its threaded tests prepare and commit from one thread or under an application lock.
• Default vs opt-in. No special codec or setting is needed: the window is the merge step that prepareCommit() runs after a flush with changes (IndexWriter.java:3739-3741), with any merge policy and scheduler. It is longest when merges are picked and SerialMergeScheduler runs them on the caller, or ConcurrentMergeScheduler stalls it. Every executed test that shows the bug held the window open with a latch in a custom merge scheduler (an @lucene.experimental extension point); a stock configuration was not run, and the window's length in practice was not measured.
• Scale. None: two documents.
• How bad. Only the returned long is wrong. The committed index is correct and nothing throws, with assertions on or off. If thread A then calls commit(), that call finishes the second commit (the one still pending, prepared by the other thread) and returns the same number, so everything up to it becomes durable (executed). So the harm is an over-stated cut for code that pairs the prepare-time number with the earlier commit. By reasoning only (not executed): an application that trims its log up to 6 once A's commit is durable, and crashes before the second commit finishes, has operation 4 neither in the index nor in its log.
• Likelihood: contrived. No public report of it was found since 6.4.0 (January 2017).

The merge policy returns an empty merge specification on the full-flush trigger so that IndexWriter calls the merge scheduler; the scheduler holds the first caller (thread A, inside prepareCommit()) until the main thread releases it. The test draws no random values itself (only the test framework's randomized newDirectory()); every run of it failed (main, 10.6.0, 10.5.2, 9.12.3; 10 of 10 iterations on main). It belongs in lucene/core/src/test/org/apache/lucene/index/.

The test file has no Apache license header, which Lucene's checkLicenses requires; with the header added, the test and either fix pass :lucene:core:check -x test.

Regression test: this same test.

TestPrepareCommitOwnSeqNo > testPrepareCommitReturnsItsOwnSeqNo FAILED
    java.lang.AssertionError: expected:<3> but was:<6>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:EE88E9F533D2A047]: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.TestPrepareCommitOwnSeqNo.testPrepareCommitReturnsItsOwnSeqNo(TestPrepareCommitOwnSeqNo.java:53)

• The contract. prepareCommit() @return (IndexWriter.java:3730-3732): "The sequence number of the last operation in the commit. All sequence numbers <= this value will be reflected in the commit, and all others will not." commit() has the same @return (:4207-4209). The class javadoc: "IndexWriter instances are completely thread safe, meaning multiple threads can call any of its methods, concurrently" (:167-168).
• The trigger breaks no stated rule. The javadoc orders the phases ("After calling this you must call either commit() to finish the commit, or rollback() to revert the commit", :3723-3725; TwoPhaseCommit.java:36-40) but does not say which thread may finish a prepared commit. commitInternal finishes whatever commit is pending, without tracking which call or thread prepared it (:4253-4257), and the second prepareCommit() passes its own "already called" check because the first commit is finished.
• History. The first sequence-number patch (LUCENE-7302, #8356, 6.2.0) stored and returned the field on adjacent lines (6.3.0: pendingSeqNo = prepareCommitInternal(...); return pendingSeqNo;), so a wrong value needed thread A to be preempted between two adjacent field accesses. LUCENE-7570 (#8621, 6.4.0) moved the merge between them to fix a deadlock; neither the issue nor the commit mentions the return value. commit() kept returning a local copy.
• Maintainer intent for the number. On java-user@ (2017-08-10, "Storing external transaction log-ids in lucene...") a user planned to let a background commit thread call prepareCommit() and use its number; Michael McCandless replied "That's exactly right! That is the purpose of the sequence numbers ... You can know exactly which ops made it into your commit" (https://lists.apache.org/thread/75rfkqdby1ln96x0g9s6mmn2yh0n46wp).
• Tests. No upstream test compares prepareCommit()'s return value with a concurrent commit().

The strongest case (rated moderate by the cross-model reviews and the devil's-advocate verifier): two-phase commit is meant to have one coordinator. TwoPhaseCommit (@lucene.experimental, TwoPhaseCommit.java:21-41) describes commit() as the phase "following" prepareCommit(), and the prepareCommit() javadoc says to call commit() "after calling this" (IndexWriter.java:3723-3725). Lucene's threaded tests serialize under an application lock, around prepareCommit() plus commit() (TestIndexWriterWithThreads.java:568, :599-608) or around commit() alone (TestIndexingSequenceNumbers.java:166, :187). On LUCENE-1335 (#2412, 2008-08-05) Michael McCandless wrote: "I think we should not make any API promises about what it means when these methods (commit, close, rollback, optimize, addIndexes) are called concurrently from different threads, except that the methods all work correctly, IndexWriter won't throw an errant exception, and your index won't become corrupt." Here the index is correct and nothing throws. On dev@ (2014-12-16, "Transactions multiplexing IndexWriter", https://lists.apache.org/thread/w83vmtn0bbncd4dk3byfnmzrv3r6z0b6) they also wrote that "Lucene's IndexWriter only allows one transaction at a time". A maintainer is likely to cite the 2008 statement.

Why it fails: none of this is a documented precondition. The 2008 statement predates sequence numbers (LUCENE-7302, 2016) by 8 years, and "the methods all work correctly" includes the documented @return. The 2014 remark is about isolating the documents of concurrent transactions, which the repro does not need (the contents are correct). The numbers returned for one commit, 3 from commit() and 6 from prepareCommit(), fit no single-threaded order, which is what sequence numbers promise (CHANGES.txt:8995-8997, LUCENE-7302: "the effective equivalent single-threaded execution order"). What survives is a likelihood argument, hence priority Low; the zero-cost fix makes prepareCommit() consistent with commit().

Both options were validated with gradle in a clean scratch worktree of 5adff78bb168 (OpenJDK 25.0.3, seed DEADBEEF, 2026-11-03). The suites: the repro, TestIndexingSequenceNumbers, TestTwoPhaseCommitTool, TestIndexWriterCommit, TestIndexWriterWithThreads, TestTransactions, TestIndexWriter. No workaround inside Lucene is needed for ordinary use: an application that serializes prepare plus commit is not affected, and one that uses the number commit() returns does not hit the failure shown here.

Makes prepareCommit() return what commit() already returns for the same commit.

   public final long prepareCommit() throws IOException {
     ensureOpen();
-    pendingSeqNo = prepareCommitInternal();
+    final long seqNo = prepareCommitInternal();
+    pendingSeqNo = seqNo;
     // we must do this outside of the commitLock else we can deadlock:
     if (maybeMerge.getAndSet(false)) {
       maybeMerge(config.getMergePolicy(), MergeTrigger.FULL_FLUSH, UNBOUNDED_MAX_MERGE_SEGMENTS);
     }
-    return pendingSeqNo;
+    return seqNo;
   }

Trade-offs

No other behaviour changes; the field keeps its one reader (IndexWriter.java:4257). It leaves the related gap open (no test targets it, never observed; the field is still stored after commitLock is released).

What the checks validated

• Stock 5adff78bb168: the repro fails through gradle with assertions on and off; with this fix it passes, on and off.
• With the fix, 7 suites (the repro, TestIndexingSequenceNumbers, TestTwoPhaseCommitTool, TestIndexWriterCommit, TestIndexWriterWithThreads, TestTransactions, TestIndexWriter): 160 tests, 0 failures, 8 skipped.
• :lucene:core:check -x test (license headers, format, lint, forbidden APIs) with the fix and the test with its license header: passes.
• Earlier, the hunt's shadow-class runs of the same change passed the minimal repro (on, off), the finder's, the independent reproducer's and the cross-model reviewers' tests.

In prepareCommitInternal, set pendingSeqNo = seqNo right after startCommit published the pending commit, still inside commitLock; prepareCommit() returns its local value. The field and the pending commit are then written in the same locked section, and the field is read only under that lock.

@@ prepareCommit()
-    pendingSeqNo = prepareCommitInternal();
+    final long seqNo = prepareCommitInternal();
     ...
-    return pendingSeqNo;
+    return seqNo;
@@ prepareCommitInternal(), after startCommit(toCommit)
         if (pendingCommit == null) {
           return -1;
         } else {
+          // publish the pending commit's seqNo while still holding commitLock
+          pendingSeqNo = seqNo;
           return seqNo;
         }

Trade-offs

Also closes the store-after-unlock gap, but no test targets that gap, so this extra benefit is untested. Touches prepareCommitInternal, which commit() also calls when nothing is prepared; the store is harmless there, since the field is read only while a commit is pending.

What the checks validated

• With this fix the repro passes through gradle, on and off; the same 7 suites: 160 tests, 0 failures, 8 skipped; :lucene:core:check -x test with the license-headed test: passes.
• Its extra benefit, closing the store-after-unlock gap, has no test: no test targets that gap.

Method: semi-formal (a proof-obligation audit of a class's contract, witnessed by an executable repro).

• The obligation: the value prepareCommit() returns satisfies its @return for the commit this call prepared, under the class's "completely thread safe" promise. Between the store (:3737) and the return (:3742) no lock is held and another thread can publish a new value, so the obligation is not met.
• A JUnit test that holds the preparing thread in its merge step with a latch witnessed it on main (assertions on and off; 10 of 10 iterations) and on 10.6.0 and 9.12.3. An independent reproducer, two executed reviews by another model family (OpenAI GPT-5.6) and a devil's-advocate verifier wrote their own tests from the claim; the independent reproducer's test, as cut down in a final repro check, is the repro here.