An NRT replica's merge pre-copy callback locks itself, not the replica, so a primary switch can slip into its install

In lucene-replicator's NRT segment replication, ReplicaNode.launchPreCopyMerge gives each merge pre-copy job an anonymous CopyJob.OnceDone callback. The callback checks whether the primary changed, renames the copied files into place and lists them in pendingMergeFiles, all inside synchronized (this); in an anonymous class this is the callback, which nothing else locks. newNRTPoint and maybeNewPrimary, which switch to a new primary and discard the pending files, lock the ReplicaNode. So a fail-over that reaches a replica during the install runs in the middle of it: the replica follows the new primary with the old primary's merged files pending and on disk, a state no serial order of the two locked blocks can produce, against the discard that newNRTPoint's comment describes. With assertions on, the new primary's later pre-copy of a same-named merged segment then fails; in a timing probe the race also made newNRTPoint throw ConcurrentModificationException. No wrong search results or corruption were shown. Present since 6.0.0 (LUCENE-5438); synchronized (ReplicaNode.this) fixes the repro and passes the module's nightly suites.

// ReplicaNode.launchPreCopyMerge(), abridged
protected synchronized CopyJob launchPreCopyMerge(...) {          // :730  node lock, only while launching
  maybeNewPrimary(newPrimaryGen);                                 // :736
  final long primaryGenStart = lastPrimaryGen;
  job = newCopyJob(..., new CopyJob.OnceDone() {                  // :754  anonymous class
    public void run(CopyJob job) throws IOException {             // later, on the copy thread
      mergeCopyJobs.remove(job);
      synchronized (this) {                                       // :761  'this' = the OnceDone
        if (lastPrimaryGen != primaryGenStart) {                  // :763  1. primary changed?
          job.cancel("primary changed during merge copy", null);
        } else {
          job.finish();                                           // :796  2. renames into place
          for (String fileName : fileNames) {
            pendingMergeFiles.add(fileName);                      // :802  3. register as pending
  ...
  launch(job);                                                    // :820  "Runs this job async'd"

// ReplicaNode.newNRTPoint() -> maybeNewPrimary(), both synchronized on the ReplicaNode
maybeNewPrimary(newPrimaryGen);                                   // :545  "defensively discard" (:536-544)
  if (newPrimaryGen != lastPrimaryGen) {
    for (String fileName : pendingMergeFiles) {                   // :717  deletes every pending file
      deleter.deleteIfNoRef(fileName);
    }
    lastPrimaryGen = newPrimaryGen;                               // :723
    pendingMergeFiles.clear();                                    // :724

The normal NRT-point callback does it right: finishNRTCopy runs its check, job.finish() and bookkeeping inside synchronized (this) in a ReplicaNode method (:412-430), so under the node lock. A lambda would also have bound this to the ReplicaNode. Nothing under lucene/ ever locks a OnceDone, and the shipped bytecode of the callback locks itself (aload_0; dup; astore_2; monitorenter).

1. Pre-copy request. An empty replica starts at generation 1 and gets launchPreCopyMerge(generation 1) for one file, _0.si. The callback records primaryGenStart = 1.
2. Check. The test thread runs the callback as the copy thread would, without the node lock. It takes the callback's own lock and passes the check: generation is still 1.
3. Switch. Inside the install (job.finish()), the test's job starts a thread that calls the public newNRTPoint(2, current version) and waits for it (up to 5 s). That thread gets the node lock at once: maybeNewPrimary(2) deletes and clears the still-empty pending set and sets generation 2.
4. Install. The callback creates _0.si and adds it to pendingMergeFiles. The replica now follows generation 2 with [_0.si] from generation 1 pending.
5. With the fix the switch thread blocks on the node lock, the 5 s wait times out, the install finishes, and then the switch deletes _0.si: pendingMergeFiles is empty.

• Real copy, two replicas (the study's test: a real forceMerge, Lucene's CopyOneFile, temp-file renames; replica A promoted to P2): replica B follows generation 2 with pendingMergeFiles=[_2.cfe, _2.cfs, _2.si] and the same files on disk, unreferenced. They stay until the next primary change, close(), a restart (:692-696, :716-724, :173), or an NRT point from the new primary that reuses or retires the same file names (:450, :464-472).
• Assertions on: P2's later pre-copy of a same-named merged segment _2 is rejected by the assert at :742, file "_2.si" is already being warmed!. With assertions off that pre-copy goes through.
• Unlocked iteration. pendingMergeFiles is a Collections.synchronizedSet (:83), whose iterator must be used under the set's own lock; maybeNewPrimary (:717) and close() (:693) iterate it holding only the node lock, and the callback's add is the one mutation without the node lock. In the tie-breaker's timing probe (no hooks; a copy thread runs the callback while another thread calls newNRTPoint after a random 0-200 microsecond spin; 2000 rounds), with one earlier pre-copy pending at each round start, newNRTPoint threw ConcurrentModificationException in 785 rounds (assertions off). In 781 of them the aborted loop had already deleted that file, which stayed listed, and the generation stayed old: a re-delivered switch then threw NoSuchFileException, and so did close(), which kept the directory's write lock. With the fix: no stale state and no exception in 8000 rounds. The spin makes the two events nearly simultaneous, so these counts are conditional, not production frequencies.

• Who is hit. Applications that use lucene-replicator's NRT segment replication with merge pre-copy (PrimaryNode / ReplicaNode / CopyJob subclasses, marked @lucene.experimental). Every release run from 6.0.0 to 10.6.0 fails, and the callback is unchanged in every release tag read; assertions on and off both fail where both were run. The repros use small indexes.
• What it takes. A switch to a new primary (newNRTPoint, or a pre-copy request from the new primary, both through maybeNewPrimary) reaching a replica while its callback for an old-primary merge pre-copy is between the check and the end of its add loop. Lucene's reference transport allows it: one handler thread per connection (TestSimpleServer.java:344-356, test code) calls newNRTPoint and launchPreCopyMerge (SimpleReplicaNode.java:218, :339), while the job runner thread calls the callback without the node lock (Jobs.java:107). Each step is an ordinary fail-over event, but the window, the renames and bookkeeping after the last byte, is short (not measured). Likelihood: rare; nothing has been reported since 2016.
• What goes wrong (executed). (1) The old primary's merged files stay pending and on disk under the new primary, against the discard that newNRTPoint's comment describes, until the next primary change, close, restart, or an NRT point from the new primary that reuses or retires those file names; for a large merge that is a whole merged segment's files on disk, unreferenced. (2) With assertions on, the new primary's later pre-copy of a same-named merged segment fails. (3) In the timing probe, newNRTPoint threw ConcurrentModificationException, and when another pre-copy was pending the replica then failed a re-delivered switch and close() with NoSuchFileException and kept its write lock (seen only in that probe variant, not in the minimal or real-copy tests).
• What does not go wrong. No reader or commit references the stale files, so no searcher sees them (by reading; no executed test checked searchability after the race); a different file with the same name is never mistaken for the local one, because a local file counts as identical only if its header (with the segment's random id) and footer match (ReplicaNode.java:849-873); otherwise it is re-copied. No wrong documents, corrupt commit or hang were shown.
• Impact class: minor, priority low (opt-in experimental module, rare trigger).

The test thread runs the job's OnceDone as the replica's copy thread would, without the node lock. The OnceDone passes its "primary changed?" check and calls the job's finish() (the install, here creating the file), which starts a thread calling the public newNRTPoint(2, current version) and waits for it with join(5000). With the bug that thread gets the node lock at once; with the fix it waits for the install, so the join times out and the passing run takes about 5 s longer. The wait sits in CopyJob.finish(), a method the integrator implements: it stretches the naturally short install window, and with the correct lock it cannot let the switch in. The NRT point carries the replica's current version, so newNRTPoint needs no copy job: it calls maybeNewPrimary before any version check.

It lives in the module's package because launchPreCopyMerge, start and the CopyJob hooks are protected and pendingMergeFiles is package-private, like the module's own tests. The seed does not change the outcome: 20 of 20 iterations under another seed fail. It rests on a 5-second window: only a switch thread stalled for more than 5 s could let the stock code pass by accident; with the fix it always waits out the 5 s and passes. The job is synthetic (no bytes copied, empty metadata); the study's longer test with a real merge and Lucene's CopyOneFile fails the same way (its nodes run in one JVM with test subclasses; the race was not run through the module's socket-based reference transport). Use this one test as the regression test too.

# gradle, main 645d4fe14a20, seed DEADBEEF, assertions on (assertions off: identical message)
TestMergePreCopyPrimaryChange > testPrimaryChangeWhileMergePreCopyInstalls FAILED
    java.lang.AssertionError: pendingMergeFiles at primaryGen 2 expected:<[]> but was:<[_0.si]>
        at org.apache.lucene.replicator.nrt.TestMergePreCopyPrimaryChange.testPrimaryChangeWhileMergePreCopyInstalls(TestMergePreCopyPrimaryChange.java:41)

# with the one-line fix: passes, assertions on and off (5.03 s: the switch waits for the install)
# 10.5.2 and 10.6.0 release jars: the same message

# the study's real-copy test (real forceMerge, CopyOneFile, temp-file renames; replica A promoted to P2)
AssertionError: newNRTPoint ran while the merge OnceDone was inside its synchronized block: true; replica B now
  follows primaryGen 2 but pendingMergeFiles=[_2.cfe, _2.cfs, _2.si] and P1's pre-copied merged files still on
  disk: [_2.cfe, _2.cfs, _2.si]

# independent reproducer's follow-up test, assertions on: the new primary's pre-copy of the same-named segment
AssertionError: replica rejected primaryGen 2's pre-copy of [_2.cfe, _2.cfs, _2.si]:
  java.lang.AssertionError: file "_2.si" is already being warmed!

# tie-breaker's timing probe (evidence only), assertions off, one earlier pre-copy pending at each round start
PROBE onePending=true rounds=2000 outcomes={CANCELLED=22, DISCARDED=1159, EXCEPTION ConcurrentModificationException=785,
  STALE=34} afterException={... pendingButDeletedFromDisk=some, redelivered switch threw NoSuchFileException, close threw
  NoSuchFileException (state closing), write lock still held (LockObtainFailedException)=781}
# with the fix: no STALE and no EXCEPTION in 8000 rounds

• The code states the contract (in an implementation comment, not in javadoc). ReplicaNode.java:536-544: "Cutover (possibly) to new primary first, so we discard any pre-copied merged segments up front ... we defensively discard here and re-copy"; maybeNewPrimary deletes and clears pendingMergeFiles (:716-724).
• The callback guards against this very race. It cancels when "primary changed during merge copy" (:763-767), and its own comment assumes it coordinates with newNRTPoint ("Even though in newNRTPoint we have similar logic, which cancels any merge copy jobs ...", :787-793). The asserts at :742 and :799-800 say a file is never pending twice.
• No serial order explains the result. Callback first: the switch deletes the files. Switch first: the callback cancels. Observed: generation 2 with generation-1 files pending.
• The twin path holds the node lock (finishNRTCopy, :412). A contributor read these blocks as the node lock too: in review of #13142 (2024-02-28) its author wrote that "all operations on the 'pendingMergeFiles' in that class are inside synchronized (this) blocks. But this is rather fragile."
• No locking duty on callers is documented. OnceDone: "Callback invoked by CopyJob once all files have (finally) finished copying" (CopyJob.java:92); launch: "Runs this job async'd" (:516). The module's job runner calls the callback without the node lock (Jobs.java:107, test code).
• Design intent. The module author's 2017 design post: replicas that switch to a new primary "must delete any files copied or partially copied from the old primary but not referenced by the new primary".
• History. The lock dates from the commit that added the package (1ae7291429ba, 2016) and no earlier discussion of which monitor it locks was found (the #13142 review read it as the node lock); the only later change is a 2021 reformat.

Strongest case for "intended" (an independent reviewer from another model family, OpenAI GPT-5.6, refute lens, reading only): the module is experimental and documents no instantaneous pendingMergeFiles invariant; OnceDone must be invoked only after a genuine copy, but the test calls it directly after a synthetic job with empty metadata that creates the file itself; and pendingMergeFiles is package-private state with best-effort cleanup, not a result a search client sees.

Why it fails:

1. The reviewer agreed with the root cause, could not run anything, called its verdict "not a substantive disproof of the race" and rated its own argument moderate.
2. Calling the OnceDone from the copy thread is what the module's job runner does. The study's real-copy test (a real merge, Lucene's CopyOneFile with length and checksum checks, temp-file renames) fails the same way, and so does a variant with the old primary closed before the promotion.
3. The stale files are on disk, not only in a field, and the probe shows the public newNRTPoint throwing.
4. Two of its three follow-ups were run: a production-like copy path (item 2) and the module's suites with the fix, which pass in nightly mode. After the race the replica still opens P2's NRT point (the independent reproducer's test asserts it); that no searcher sees the stale files is by reading.
5. @lucene.experimental is about API stability, not about breaking the contract the module's own comments state.

One fix was run through gradle (A), the one-line change the study proposed. B is the same lock in a clearer shape; C alone does not fix the bug.

The check, the renames and the add become atomic with newNRTPoint / maybeNewPrimary: a switch that arrives during the install waits and then discards the files, or the install sees the new generation and cancels. It also removes the only mutation of pendingMergeFiles that does not hold the node lock, so the unlocked iteration in maybeNewPrimary and close() can no longer race with an add.

--- a/lucene/replicator/src/java/org/apache/lucene/replicator/nrt/ReplicaNode.java
+++ b/lucene/replicator/src/java/org/apache/lucene/replicator/nrt/ReplicaNode.java
@@ -758,7 +758,7 @@ public abstract class ReplicaNode extends Node {
                 // Signals that this replica has finished
                 mergeCopyJobs.remove(job);
                 message("done warming merge " + fileNames + " failed?=" + job.getFailed());
-                synchronized (this) {
+                synchronized (ReplicaNode.this) {
                   if (job.getFailed() == false) {
                     if (lastPrimaryGen != primaryGenStart) {
                       message(

Trade-offs

• The renames now run under the node lock, as they already do for NRT points (finishNRTCopy, ReplicaNode.java:430), so a slow finish() delays newNRTPoint and commit for that long. The study's devil's advocate notes it adds no new lock-order pair: the same launcher thread already takes the node lock in finishNRTCopy (read).
• An integrator whose launch() blocked until the job finished on another thread would deadlock here, but such an integrator already deadlocks on every NRT point: newNRTPoint calls launch holding the node lock (:654) and its callback needs that lock (:412); launch is documented as async (:516) (read, 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 (5.03 s each, because the switch waits for the install).
• With A, the whole :lucene:replicator:test task passes (16 tests: the new one passes; the 14 nightly tests and TestSimpleServer.test, which runs only nested, are skipped).
• With A and nightly tests enabled: TestNRTReplication 13 tests, 0 failures (48 s); TestStressNRTReplication 1 test, 0 failures (JUnit time 324 s, one seed only; its log has 9 replica-side primary switches ("now change lastPrimaryGen" lines), 127 merge pre-copy callbacks ("done warming merge" lines) and no "already being warmed", ConcurrentModificationException or NoSuchFileException).
• Earlier, the 2026-10-31 tie-breaker with the fix shadowed into the stock jars: the real-copy test, the P1-closed variant and the independent reproducer's two tests pass; the timing probe shows no stale state and no exception in 8000 rounds; nightly TestNRTReplication 13 of 13 and TestStressNRTReplication 1 of 1 (a randomized run with crashes and closes, planned run time 478 s). The one-line change also fixes the port on 6.0.0.

Move the callback's body into a synchronized ReplicaNode method (for example finishMergeCopy(job, fileNames, primaryGenStart), like finishNRTCopy), or pass a lambda, whose this is the ReplicaNode.

Trade-offs

Same semantics as A and harder to get wrong again; a larger diff.

Open PR #13142 changes pendingMergeFiles and mergeCopyJobs to ConcurrentHashMap.newKeySet().

Trade-offs

Removes the ConcurrentModificationException, but not the check-then-act race: the old primary's files would still be registered after the switch. With A, every mutation already holds the node lock.

• Method (semi-formal). The replicator hunt wrote a TLA+ model of the NRT replication protocol (NrtReplication.tla). The hypothesis came from code reading while the merge pre-copy callback was being transcribed: because the lock is the callback, the model splits it into two actions (check, then install). TLC found the invariant "every pending file comes from the current primary generation" violated in a 7-state trace (689 distinct states with assertions off, 673 on; also under the module's test-controller discipline, 337 states). With the lock fixed TLC found no violation in the bounded configurations it checked (10,233 distinct states for the merge model, 82,805 for the full model).
• Executed. A JUnit with in-process nodes and a real forceMerge replayed the interleaving on the stock jars; an independent reproducer wrote its own test and added the assertions-on follow-up; a devil's advocate and an adjudication confirmed it; the one-test repro was then reduced from these tests; the 2026-10-31 re-verification and tie-breaker re-ran everything on 645d4fe14a20 and added the timing probe and the nightly suites with the fix.