One I/O error in PrimaryNode.flushAndRefresh leaves replicas on the old NRT point, and the retry reports no changes

In lucene-replicator's NRT segment replication, PrimaryNode.flushAndRefresh() publishes each new NRT point through setCurrentInfos. That method first switches curInfos to the new SegmentInfos, increfs its files and decrefs the previous point's files, and only then serializes the infos (in memory) and opens every new file to read its header, footer and checksum, which can throw. copyState, the point handed to replicas, is assigned last. One IOException in between (any type except EOF, corrupt-index and not-found, which the metadata read turns into null) leaves curInfos at the new point and copyState at the old one, whose own reference was already released. The retry compares the writer's version with curInfos, sees no change and returns false, so replicas are not offered the new point until the index changes and flushAndRefresh() runs again. In that window getCopyState() throws AssertionError with assertions on, or offers the old point, whose files may already be deleted if the new point no longer uses them, so a copy fails with NoSuchFileException. Nothing wrong is served and the primary's index is intact. The order dates from the feature's first commit (LUCENE-5438) and ships in every release from 6.0.0 to 10.6.0; doing the fallible work first and then switching everything together fixes the repro and passes the module's nightly suites.

// PrimaryNode.setCurrentInfos(), abridged
if (curInfos != null && infos.getVersion() == curInfos.getVersion()) {
  return false;                                       // :280-288  "no change"
}
SegmentInfos oldInfos = curInfos;                     // :290
writer.incRefDeleter(infos);                          // :291
curInfos = infos;                                     // :292  1. switched to the new point
if (oldInfos != null) {
  writer.decRefDeleter(oldInfos);                     // :294  2. old point's files released
}
infos.write(tmpIndexOutput);                          // :303  declared IOException (in memory)
for (SegmentCommitInfo info : infos) {
  for (String fileName : info.files()) {
    FileMetaData metaData = readLocalFileMetaData(fileName);   // :310  3. can throw
    ...
copyState = new CopyState(..., infos.getVersion(), ..., curInfos);   // :327-335  4. only now
return true;

1. Publish the empty point. The constructor's setCurrentInfos publishes version 1 (no files).
2. Add 1 document, refresh. flushAndRefresh() refreshes the searcher to version 3 (PrimaryNode.java:165) and calls setCurrentInfos. It sets curInfos to version 3 and releases version 1 (:290-295).
3. One read fails. readLocalFileMetaData opens _0.cfe to read its header (Node.java:213). It returns null only for EOFException | CorruptIndexException (:219) and FileNotFoundException | NoSuchFileException (:232); this plain IOException propagates out of flushAndRefresh(). There is no catch or finally between the switch and copyState, so copyState stays at version 1.
4. The retry. flushAndRefresh() finds nothing new to refresh, and the version check against curInfos (:280) sees 3 == 3 and returns false. getCopyStateVersion() is still 1 (:183-185).
5. Asking for the point. getCopyState() asserts curInfos == copyState.infos() (:236): with assertions on it throws; with them off it increfs and returns version 1 (no files here; in general, files only the old point used may already be deleted).
6. Healing. One more addDocument changes the writer's version; the next flushAndRefresh() passes the check and runs a full publish (versions 5 and 5). Nothing else publishes: the private setCurrentInfos is called only by the constructor (:126) and flushAndRefresh() (:166).

When the new point drops the old point's segment the window is worse: in the reproducer's probe (publish 1 document, delete it, add another, fail the read of _1.cfs, retry) the retry returns false with searching 7 and copy state 3; with assertions off getCopyState() offers _0.cfe, _0.si and _0.cfs, all already deleted, and releaseCopyState throws NoSuchFileException: _0.cfe. When the old segment stays in the new point its files keep a reference: a check probe (publish 1 document, add a second, fail the read of _1.cfs, retry, assertions off) gets false with searching 5 and copy state 3, but the offered _0.cfe, _0.si and _0.cfs all still exist and releaseCopyState succeeds.

• Who is hit. Applications that use lucene-replicator's NRT segment replication (a PrimaryNode subclass; the package is @lucene.experimental). Every release since 6.0.0, default IndexWriterConfig, codec and merge policy. Reproduced on small indexes; the order does not depend on the index size once the new point has files to read.
• What it takes. One transient IOException other than EOF / corrupt / not-found while flushAndRefresh() opens or reads the new point's files, for example a FileSystemException "Too many open files" or an EIO. The hunt's single-fault sweep (a NoMergePolicy script) injected one IOException at one representative event of each (operation, file) class inside setCurrentInfos, 72 classes covering 632 events: 57 of the 72 leave the stale state, with assertions on and off; the other 15 hit the constructor's first publish, which throws; with fix A none leaves the stale state. Likelihood: rare; nothing has been reported since 2016. All of it is injected faults on one platform: it shows the behaviour, not how often it happens in the field.
• What goes wrong. The caller's retry gets a false "no changes". A primary that pushes points only when flushAndRefresh() returns true, as Lucene's own reference test primary does (SimplePrimaryNode.java:344), never tells replicas about the point. A busy index heals at the next change and refresh; for a quiet index (the end of a bulk load, an idle shard) the last changes stay off the replicas indefinitely.
• In the window. A replica that asks for the current point (a new or restarted replica) is offered the old point. With assertions off, files only the old point used may already be deleted (when the new point dropped its segment), so the copy fails with NoSuchFileException and releaseCopyState throws; that failing release leaks the copy count, which can make close() wait forever under the default remoteCloseTimeoutMs = -1 (a separate bug with its own fix site, publicly reported as #11674). With assertions on every getCopyState() throws AssertionError until the next change and refresh. When the old segments are still in the new point, the stale point is offered with intact files.
• What does not go wrong. No wrong bytes are served: Lucene file names are write-once, so a deleted name makes the copy fail loudly. The primary's own index, searcher and commits are intact.
• Impact class: silent wrong results (the wrong result is the false "no changes" and the stale getCopyStateVersion(), not wrong bytes); priority low (opt-in experimental module, rare trigger, heals at the next change and refresh).

Public API only: an anonymous PrimaryNode with its one abstract method, and an anonymous FilterDirectory. The directory finds the metadata read by method name (StackWalker), so the fault hits only that open, not the searcher refresh that runs just before it (a fault there would throw before anything is switched). The expectThrows line proves the fault fired inside flushAndRefresh(). Use this one test as the regression test too.

The seed only drives the test framework's randomization: -Dtests.iters=20 gives 20 of 20 failures with the same message, with assertions on and off. The test compiles from 9.1.0 on; older releases were run with a main() port of the same steps.

# gradle, main 645d4fe14a20, seed DEADBEEF, assertions on (assertions off: identical)
TestPrimaryRetryAfterFailedPublish > testRetryPublishesNewPoint FAILED
    java.lang.AssertionError: expected:<3> but was:<1>
        at org.apache.lucene.replicator.nrt.TestPrimaryRetryAfterFailedPublish.testRetryPublishesNewPoint(TestPrimaryRetryAfterFailedPublish.java:61)

# repro harness, the same message: main -ea/-da, 10.5.2 -ea/-da, 10.6.0 -ea/-da (1 run, 1 failed each);
# -Dtests.iters=20 on main: 20 of 20 fail, -ea and -da
# with fix A: the test passes (gradle, assertions on and off)

# probe with the same steps, main, assertions off (assertions on: getCopyState threw java.lang.AssertionError)
initial: searching=1 copyState=1
first flushAndRefresh threw: java.io.IOException: transient I/O error opening _0.cfe
retry flushAndRefresh returned false; searching=3 copyState=1
getCopyState offers version 1 files []
after next change: flushAndRefresh returned true; searching=5 copyState=5

# probe with a published document in the old point, main, assertions off
after refresh1: searching=3 copyState=3 files=[_0.cfe, _0.cfs, _0.si]
refresh2 threw java.io.IOException: simulated transient I/O error opening _1.cfs
retry=false
after retry: searching=7 copyState=3 files=[_1.cfe, _1.cfs, _1.si]
  offered _0.cfe exists=false
  offered _0.si exists=false
  offered _0.cfs exists=false
releaseCopyState threw java.nio.file.NoSuchFileException: _0.cfe

• The module defines copyState as the published point. PrimaryNode.java:59-61: "IncRef'd state of the last published NRT point; when a replica comes asking, we give it this as the current NRT point". CopyState.java:26: "Holds incRef'd file level details for one point-in-time segment infos on the primary node". After the failure the copy state holds no reference of its own: the primary's reference for that point was released at :294, so files only it used are deleted.
• The code asserts the invariant. getCopyState(): assert curInfos == copyState.infos(); (:236).
• The API promises a change signal. flushAndRefresh() (:155-158): "Flush all index operations to disk and opens a new near-real-time reader. new NRT point, to make the changes visible to searching. Returns true if there were changes."
• No discard rule. No javadoc, package doc, CHANGES.txt or MIGRATE.md entry says a PrimaryNode must be thrown away after flushAndRefresh() throws; the node stays open and the IndexWriter has no tragic event.
• The core refresh is failure-atomic. ReferenceManager.doMaybeRefresh swaps only after refreshIfNeeded succeeded and releases the new reference on failure (ReferenceManager.java:167-178). The replica orders part of its cut-over the same way: ReplicaNode.finishNRTCopy reads the new infos (readCommit, ReplicaNode.java:433-434) before its cut-over call (:440) and increfs the new files before it decrefs the old ones ("Must first incRef new NRT files, then decRef old ones", :442-454). The replica's cut-over itself is not failure-atomic: SegmentInfosSearcherManager.setCurrentInfos sets currentInfos (:99) before the fallible reader open (:100, :116).
• History. The order dates from the class's first commit (2016) and was never discussed; no upstream test injects I/O errors during point publication. Maintainers have accepted robustness fixes in this module (#11822, LUCENE-9967).

Strongest case for "intended" (the devil's-advocate verifier): NRT replication follows a crash-and-promote failure model. The module's tests crash nodes and flip bits but never inject I/O errors, and the reference test server fails on any unexpected exception, so a primary whose flushAndRefresh() threw is never used again. readLocalFileMetaData's javadoc says it "returns null ... if there are any problems" (Node.java:193-194), and PrimaryNode.java:312 asserts the file exists, so the author assumed this step cannot fail. The module is @lucene.experimental.

Why it fails:

1. Nothing documents a discard rule, and instead of failing loudly the node answers the retry with a silent false.
2. The module's own invariants (the copyState comment, the getCopyState assert, the CopyState javadoc) break.
3. The method declares IOException, and the metadata read does propagate other IOExceptions despite its javadoc; a real filesystem raises them.
4. The reference server failing on exceptions is a test-failure mechanism, not a recovery contract; a downstream server rethrows such an exception rather than discarding the primary (read, not run).
5. Experimental lowers the reach, not the correctness bar. The cross-model reviews, the devil's advocate and the adjudication rejected the "intended" reading.

One fix was run (A), the study's patch. It keeps the incref-new-before-decref-old order of the replica's cut-over. A reviewer asked that its handling of an exception from the final decref of the old point be looked at (see trade-offs).

Incref the new infos so their files stay alive, serialize them and read all metadata into a local map; on failure decref the new infos and rethrow, leaving curInfos, lastFileMetaData, copyState and the old point's references exactly as they were. After everything was read, set curInfos, lastFileMetaData and copyState together, then decref the previous point. The retry then sees the old version in curInfos and publishes the new point.

--- lucene/replicator/src/java/org/apache/lucene/replicator/nrt/PrimaryNode.java
+++ lucene/replicator/src/java/org/apache/lucene/replicator/nrt/PrimaryNode.java
@@ -287,34 +287,44 @@
       return false;
     }
 
-    SegmentInfos oldInfos = curInfos;
+    // Protect the new point's files while we read their metadata, but publish nothing (curInfos,
+    // the deleter references of the previous point, copyState) until all of it was read:
     writer.incRefDeleter(infos);
-    curInfos = infos;
-    if (oldInfos != null) {
-      writer.decRefDeleter(oldInfos);
-    }
-
-    message("top: switch to infos=" + infos.toString() + " version=" + infos.getVersion());
-
-    // Serialize the SegmentInfos.
-    ByteBuffersDataOutput buffer = new ByteBuffersDataOutput();
-    try (ByteBuffersIndexOutput tmpIndexOutput =
-        new ByteBuffersIndexOutput(buffer, "temporary", "temporary")) {
-      infos.write(tmpIndexOutput);
-    }
-    byte[] infosBytes = buffer.toArrayCopy();
-
+    byte[] infosBytes;
     Map<String, FileMetaData> filesMetaData = new HashMap<>();
-    for (SegmentCommitInfo info : infos) {
-      for (String fileName : info.files()) {
-        FileMetaData metaData = readLocalFileMetaData(fileName);
-        // NOTE: we hold a refCount on this infos, so this file better exist:
-        assert metaData != null : "file \"" + fileName + "\" is missing metadata";
-        assert filesMetaData.containsKey(fileName) == false;
-        filesMetaData.put(fileName, metaData);
+    boolean success = false;
+    try {
+      // Serialize the SegmentInfos.
+      ByteBuffersDataOutput buffer = new ByteBuffersDataOutput();
+      try (ByteBuffersIndexOutput tmpIndexOutput =
+          new ByteBuffersIndexOutput(buffer, "temporary", "temporary")) {
+        infos.write(tmpIndexOutput);
+      }
+      infosBytes = buffer.toArrayCopy();
+
+      for (SegmentCommitInfo info : infos) {
+        for (String fileName : info.files()) {
+          FileMetaData metaData = readLocalFileMetaData(fileName);
+          // NOTE: we hold a refCount on this infos, so this file better exist:
+          assert metaData != null : "file \"" + fileName + "\" is missing metadata";
+          assert filesMetaData.containsKey(fileName) == false;
+          filesMetaData.put(fileName, metaData);
+        }
+      }
+      success = true;
+    } finally {
+      if (success == false) {
+        try {
+          writer.decRefDeleter(infos);
+        } catch (Throwable t) {
+          // keep the original exception
+        }
       }
     }
 
+    message("top: switch to infos=" + infos.toString() + " version=" + infos.getVersion());
+    SegmentInfos oldInfos = curInfos;
+    curInfos = infos;
     lastFileMetaData = Collections.unmodifiableMap(filesMetaData);
 
     message(
@@ -333,6 +343,9 @@
             completedMergeFiles,
             primaryGen,
             curInfos);
+    if (oldInfos != null) {
+      writer.decRefDeleter(oldInfos);
+    }
     return true;
   }
 

Trade-offs

• The previous point's files are held until the new metadata was read, which is the point of the fix (the cost was not measured).
• An exception from the final decRefDeleter(oldInfos) (a failed file delete, or AlreadyClosedException after a tragic event) now propagates after the new point is published: state stays consistent, but the caller sees an exception instead of true. Today that call runs before the metadata read and leaves the stale state, so A is not worse; a maintainer may prefer to catch and log it there (not run).
• The failure path's catch (Throwable t) drops a secondary exception; addSuppressed would be more idiomatic (cosmetic, not run).

What the checks validated

• Scratch worktree of main 645d4fe14a20 (removed afterwards), seed DEADBEEF: stock, the test fails through gradle with assertions on and off; with A it passes with both.
• With A, the whole :lucene:replicator:test task passes (16 tests: the repro passes; 15 are skipped, the 14 @Nightly tests and TestSimpleServer, which runs only nested).
• With A and nightly tests enabled: TestNRTReplication 13 tests, 0 failures (53 s); TestStressNRTReplication 1 test, 0 failures (one run of 324 s, one seed only).
• Earlier, with A shadowed into the stock jars: the reproducer's, the finder's and the cross-model reviewers' tests pass, and in the single-fault sweep of setCurrentInfos no class leaves the stale state (0 violations of its version, file-existence, release and close checks).
• Not fixed by A: the 15 constructor-fault classes still leave unreferenced files after close, and a separate sweep of faults inside releaseCopyState itself still fails its release check (3 of 3; that is the sibling bug).

Change the version check at PrimaryNode.java:280 to copyState.version(), so the retry re-runs the publish and the new point does reach replicas.

Trade-offs

Between the failure and the retry copyState still points at a point whose reference was released (files only it used deleted, yet offered; or AssertionError). Smaller diff, weaker fix. Not run; not recommended.

Close or mark the PrimaryNode failed when setCurrentInfos throws, like IndexWriter's tragic event, so callers cannot continue with a silent false.

Trade-offs

Matches the crash-and-promote model, but turns one transient IOException (for example a momentary file-handle shortage) into a primary failover. Not run; not recommended.

• Method (non-formal first signal). A failure-path review read PrimaryNode.setCurrentInfos with a failure-atomicity question and ran a probe on the stock main jars: one injected read error on a new .liv file, then searching version 6 against copy state version 3 and a getCopyState listing an already deleted file.
• Mapping (semi-formal). The finder reduced it to a JUnit and ran a bounded-exhaustive single-fault enumeration on the real jar: one IOException at every Directory/IndexInput event inside setCurrentInfos, one representative per (kind, file) class, five checks (copy state version equals searching version, offered files exist, release does not throw, close returns, no leaked files). With a NoMergePolicy script all 72 classes (covering 632 events) fail a check with assertions on and off: 57 leave the stale state, and the 15 that hit the constructor's first publish only leave unreferenced files; with the fix 0 fail the first four checks (the 15 leftover-file cases remain).
• Masking. With TieredMergePolicy in tiny test indexes a delete-reclaiming merge often gives the retry a new version and hides the defect; the repro adds one document and deletes nothing.
• Reduction. The independent reproducer's one-test JUnit, trimmed to the minimum, is the repro.