ByteBuffersDirectory.rename that fails with IllegalStateException still leaves the file under its new name

ByteBuffersDirectory.rename(source, dest) reads the entry under source, publishes it under dest with putIfAbsent, and then removes it from source with the identity-checked remove(source, file); if that remove finds nothing, it throws IllegalStateException("File was unexpectedly replaced: " + source) without taking dest back out (ByteBuffersDirectory.java:181-189 on main). When two renames of the same source run together, both publish their own dest, only one checked remove succeeds, and the loser throws with its dest still bound: the same file stays listed under both new names after the call has failed, until something deletes one of them, although one of the renames reported failure. Run one after the other, the second call throws NoSuchFileException and one name remains, and NIOFSDirectory keeps one name under the same two-thread program. The code shape came from RAMDirectory's rename (LUCENE-7345, 2016) and has been in ByteBuffersDirectory since the class was added (LUCENE-8468, 2018): every release from 7.5.0 to 10.6.0 and main are affected. No Lucene component is known to trigger it. The fix is files.remove(dest, file); before the throw; it belongs in the same patch as LFV-836, the other one-line defect in this method.

• ByteBuffersDirectory: Lucene's heap-resident Directory. Every file is an entry in one ConcurrentHashMap<String, FileEntry> files (ByteBuffersDirectory.java:93); listAll() lists its keys (:126). FileEntry has no equals (:226), so map removals by value are by identity.
• rename(source, dest) (:178-192) moves an entry to a name that must not exist yet.

1. FileEntry file = files.get(source) (:181).
2. files.putIfAbsent(dest, file) (:185): the file is now visible under both names, which the rename javadoc allows "temporarily" (Directory.java:125-126).
3. if (!files.remove(source, file)) throw new IllegalStateException("File was unexpectedly replaced: " + source) (:188-189). On this path nothing removes dest.

thread 2: rename("a", "y")              thread 1: rename("a", "x")
  get("a")             -> F
  putIfAbsent("y", F)  -> ok              get("a")             -> F
                                          putIfAbsent("x", F)  -> ok
                                          remove("a", F)       -> true   (returns normally)
  remove("a", F)       -> false
  throw IllegalStateException             <- "y" still maps to F
listAll() = [x, y]

The race needs only that both calls read a before the winner removes it. The message is also misleading here: a was moved by the other call, not replaced. The trailing files.remove(source) at :191 is a different defect (LFV-836) with a different fix; its fix alone leaves this one in place (the independent reproducer's test still fails with it).

• Who is exposed. Applications that, with no lock of their own, change one source name on a ByteBuffersDirectory from another thread while a rename of it runs. The executed case is two rename calls with the same source, for example workers that claim a job file by renaming it and expect one winner; a rename racing with deleteFile plus createOutput of its source leaves dest behind the same way by reading, but was not executed. No setting, index size or data shape is needed, and it fails with assertions on and off.
• Opt-in class. ByteBuffersDirectory is not the default and is @lucene.experimental (:48); its javadoc says MMapDirectory "is nearly always a better choice" (:43-46), and MIGRATE.md calls it "discouraged in favor of the default MMapDirectory".
• What the caller sees. The losing call throws an unchecked IllegalStateException (rename declares only IOException) with a message that misdescribes the case. Everyone else sees the file under a name whose rename failed, in listAll() and, for a closed file as here, with its complete, correct content through openInput (by reading; a file still open for writing makes openInput throw AccessDeniedException, :241-245), until someone deletes it; a later createOutput("y") gets FileAlreadyExistsException (:158-159, by reading). File contents are never wrong.
• Lucene's own paths are not known to be affected. IndexWriter's only rename is pending_segments_N to segments_N (SegmentInfos.java:1005), under the write lock, on a name nothing else touches. Most test directories are wrapped in MockDirectoryWrapper, whose rename is synchronized (MockDirectoryWrapper.java:221), and BaseDirectoryTestCase.testRename is single-threaded, so the tests never saw it.
• Likelihood: contrived. No matching report was found in the recorded searches, although the class has shipped since 7.5.0 (2018).

Input: one empty, closed file a. The test expects rename("a", "y") to throw IllegalStateException (it does) and then asserts listAll() is [x]. It sits in org.apache.lucene.store next to TestByteBuffersDirectory, draws no random values, and fails with assertions on and off (20 of 20 with 20 iterations), and on the 10.6.0 and 9.12.3 release jars.

The @SuppressForbidden annotation is needed: without it :lucene:core:forbiddenApisTest rejects AccessibleObject#setAccessible(boolean); with it :lucene:core:check -x test passes. No test in lucene/core uses setAccessible today, so a maintainer may prefer to take the fix without a reflective test.

Real threads reach the same state with public API only: two-thread tests that release both renames together fail in 20 of 20 runs on main, each within its first 31 rounds. They are evidence, not the repro.

Setting a private final field by reflection works on JDK 25; newer JDKs are moving to warn about or restrict it (JEP 500). With fix B (a NoSuchFileException for the loser), change the expectThrows type.

Regression test: this same test.

TestByteBuffersDirectoryFailedRenameLeavesDest > testFailedRenameLeavesNoDest FAILED
    java.lang.AssertionError: expected:<[x]> but was:<[x, y]>
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:4DE3EE4B61D3D00A]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.apache.lucene.store.TestByteBuffersDirectoryFailedRenameLeavesDest.testFailedRenameLeavesNoDest(TestByteBuffersDirectoryFailedRenameLeavesDest.java:53)

• The contract. "Renames source file to dest file where dest must not already exist"; "It is permitted for this operation to not be truly atomic, for example both source and dest can be visible temporarily in listAll()" (Directory.java:121-133). On the natural reading, an extra name that outlives the call, held by a call that threw, is not "temporarily"; the javadoc does not spell out rollback or failure atomicity, so this is a reading of the contract, not an explicit guarantee.
• No serial order gives [x, y]. Sequentially, the second rename("a", ...) finds no a and throws NoSuchFileException (:181-183); the cross-model review executed this control. FSDirectory.rename moves the file with one Files.move(..., ATOMIC_MOVE) (FSDirectory.java:265, after its pending-delete checks), and the devil's-advocate verifier's two-thread program on NIOFSDirectory kept exactly one name in 20,000 of 20,000 rounds (behaviour on this JDK and platform, not a documented Directory requirement).
• Lucene removes dest when an operation fails. copyThroughCreateOutput: "On failure, deletes the file named dest" (Directory.java:184-185). SegmentInfos.finishCommit deletes dest itself when the step after a successful rename fails (:1006-1013); when rename itself throws, its catch deletes only pending_segments_N (:1014-1017) and does not clean up dest.
• The check exists to detect interference on source. The identity-checked remove at :188 notices that source no longer holds the file, and the author chose to fail; a half-published dest goes against that choice.
• History. The shape (publish dest, checked remove, throw) came from LUCENE-7345 (#8399, 2016, "RAMDirectory now enforces write-once as well"), whose discussion never mentions the failure branch; LUCENE-8468 (#9514, 2018) copied it. Nothing in the history says a failed rename was meant to keep dest.

The strongest case (the devil's-advocate verifier and the review by another model family, OpenAI GPT-5.6, which rated it "moderate"): Directory promises no failure atomicity or linearizability for two conflicting renames of one name; its class javadoc tells applications that need external synchronization to use their own lock (Directory.java:43-45); the class is experimental; and the loser does throw, an unchecked fail-fast check that arguably promises nothing about the state it leaves.

Why it fails: the javadoc's relaxation is both names "temporarily", not an extra name that outlives a failed call; no documented precondition is broken (public API, fresh dest names); Lucene's own failure convention removes dest; and the rollback is one identity-checked line on the failure path. What survives is a severity argument: the loser is loud, the trigger is an application-level race, nothing in Lucene triggers it. Hence priority Low.

Both built options were validated with gradle in a clean scratch worktree of 5adff78bb168 (OpenJDK 25.0.3, seed DEADBEEF, 2026-11-03). Workaround until fixed: do not rename one source name from two threads without your own lock (not on the Directory instance, Directory.java:43-45), delete the dest of a rename that threw, or use FSDirectory / MMapDirectory.

Undo this call's own publication when its checked remove fails. FileEntry uses identity, so a different file that someone put under dest meanwhile is left alone; the check cannot tell this call's binding apart from a later re-binding of the same entry to dest (see the trade-offs). Single-threaded behaviour is unchanged: the branch is reached only under interference.

     if (!files.remove(source, file)) {
+      files.remove(dest, file); // withdraw the publication of a rename that did not happen
       throw new IllegalStateException("File was unexpectedly replaced: " + source);
     }

Trade-offs

One line on a failure path only, scoped to the race shown here (two renames of one source). The identity check cannot tell this call's binding from a later re-binding of the same entry, so in one longer interleaving, traced by reading and not executed, it removes a binding it did not make: rename("s", "d") publishes d; another thread renames d to z; a third deletes s and creates a new s; the second renames z back to d; the first call's checked remove of s then fails and the fix removes the second thread's successful d, leaving the file with no name, where stock code keeps it under d. It needs four interleaved operations, including a delete-and-recreate of the source (raised by the cross-model fact check of this doc).

What the checks validated

• Stock 5adff78bb168: the repro fails through gradle with assertions on and off; with the fix it passes, on and off.
• Every suite in org.apache.lucene.store.* of lucene/core plus the repro (including TestByteBuffersDirectory, 171 tests with 6 skipped by assumptions, and TestMultiByteBuffersDirectory): 29 suites, 778 tests, 0 failures, 17 skipped.
• :lucene:core:check -x test (format, ecj lint, forbidden APIs) with the annotated test: passes.
• Earlier, with the same line shadowed in: the finder's and the independent reproducer's two-thread tests pass all 100,000 rounds, and the cross-model review's passes all 20,000 rounds, with assertions on and off.

The withdraw-dest line plus the deletion of the trailing files.remove(source); at :191 (LFV-836's fix). LFV-836's fix alone does not fix this bug.

     if (!files.remove(source, file)) {
+      files.remove(dest, file); // withdraw the publication of a rename that did not happen
       throw new IllegalStateException("File was unexpectedly replaced: " + source);
     }
-    files.remove(source);
   }

Trade-offs

The same as the one-line fix (same scope, same traced interleaving).

What the checks validated

• Every org.apache.lucene.store.* suite with both one-test repros (this one and LFV-836's): 30 suites, 779 tests, 0 failures, 17 skipped; both repros pass.

After the withdraw-dest line, throw NoSuchFileException(source) when files.get(source) is null (the other call moved it), as the sequential case and FSDirectory do, and keep IllegalStateException for a genuinely replaced source.

Trade-offs

Changes the exception type callers see in this race (to a declared IOException), and the repro's expectThrows type must change with it. Not built or tested.

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

• The finder, an AI tool, read rename against Directory.rename (Directory.java:121-133): dest may be visible early, but only "temporarily"; the audit's obligation was that a call that fails leaves no lasting effect. The branch at ByteBuffersDirectory.java:188-189 throws after putIfAbsent(dest) has published the file, so that obligation is not met when the checked remove fails.
• A two-thread public-API JUnit then showed it on the real jar: one call throws, both names stay (main, assertions on and off; 20 of 20 runs; 10.6.0, 9.12.3). An independent reproducer, an executed review by another model family (OpenAI GPT-5.6) and a devil's-advocate verifier wrote their own tests from the claim alone. The single-thread, forced-order repro was cut on 2026-11-02.