One failed fsync makes PersistentSnapshotDeletionPolicy's next snapshot() fail on its own leftover save file

PersistentSnapshotDeletionPolicy.persist() writes the snapshot list to snapshots_<nextWriteGen> inside a try whose catch deletes that file, then calls dir.sync(...) after that block and increments nextWriteGen only at the end. When the sync throws, the new file, already written and closed, stays in the directory and the generation is not advanced, while snapshot() and release(IndexCommit) restore their reference counts in memory and rethrow. The next persist() creates the same file name, which a Directory must refuse, so the next fault-free snapshot() or release(IndexCommit) throws FileAlreadyExistsException. Since 9.1.0 that call's cleanup deletes the leftover and the call after it normally works; on 6.2.0-9.0.0 every later save fails until the policy is re-created. While the process runs, no index data is lost. Lucene's commit path handles the same fault: SegmentInfos.write keeps the sync inside its delete-on-failure block and always advances its generation. Moving the sync into the existing try fixes it. Secondary, contract open: a policy re-opened before the next call adopts the leftover, so a snapshot() that threw is in effect and a release(IndexCommit) that threw has taken effect.

• Save file: snapshots_N, a small file holding the commit generations that are snapshotted and their reference counts. nextWriteGen is the N the next save will use.
• Write-once: Directory.createOutput "must throw FileAlreadyExistsException if the file already exists" (Directory.java:86); FSDirectory.createOutput returns new FSIndexOutput(name) (FSDirectory.java:215), which opens the file with WRITE, CREATE_NEW (:381).

1. persist() (PersistentSnapshotDeletionPolicy.java:161-184) creates and writes snapshots_<nextWriteGen> inside a try whose catch deletes the file (:163-173).
2. dir.sync(Collections.singletonList(fileName)) comes after that block (:175); then the previous save file is deleted (:177-181) and nextWriteGen++ runs (:183). If the sync throws, neither happens and the new file, already written and closed, stays.
3. The callers restore their reference counts: snapshot() calls super.release(ic) (:119), release(IndexCommit) calls incRef(commit) (:141), and both rethrow. release(long) has no undo (:156-159): repeating it for the same generation throws IllegalArgumentException before persist().
4. The next persist() uses the same name; createOutput (:163) throws FileAlreadyExistsException.
5. Since #589 (9.1.0) createOutput is inside the try, so that exception reaches the catch, which deletes the name, the leftover (:171). The call after it works if that delete succeeds. On 6.2.0-9.0.0 createOutput was called before the try, so every later save failed.
6. A real sync can fail anywhere in FSDirectory.sync (FSDirectory.java:244-251): in ensureOpen(), in IOUtils.fsync (reached through FSDirectory.fsync, :295-296), which opens the file and forces it (IOUtils.java:534-537), or in maybeDeletePendingFiles(); for example too many open files, or an I/O error from the storage. The executed runs inject the fault in a wrapper that throws before delegating.

What the checks observed on main (645d4fe14a20, and a9779ef3c193 with the same code): a FilterDirectory whose next sync() throws once, an IndexWriter that commits an empty new index once:

snapshot() #1  sync of snapshots_0 throws -> IOException; snapshot count 0, getLastSaveFile() null
               directory still holds snapshots_0
snapshot() #2  no fault -> FileAlreadyExistsException: File already exists: snapshots_0
snapshot() #3  no fault -> works, snapshot count 1

After call 1 the directory lists [segments_1, snapshots_0]; with FSDirectory it lists [segments_1, snapshots_0, write.lock]. release(IndexCommit) fails the same way on snapshots_1; after its failed call snapshots_0 and snapshots_1 are both on disk. Nothing outside the policy removes the leftover: IndexFileDeleter handles only codec files, segments_N and pending_segments_N. Assertions on or off make no difference.

• Who is hit. Applications using the opt-in, @lucene.experimental PersistentSnapshotDeletionPolicy (PersistentSnapshotDeletionPolicy.java:47); no other Lucene class uses it outside tests.
• What it takes. One IOException from Directory.sync of the new save file: an fsync error from the storage, a failure to open the file for fsync (too many open files), or a wrapping Directory whose sync throws. Then a further call that saves the list in the same process, typically a retry: snapshot(), release(IndexCommit), or release(long) of a generation that is still snapshotted. No scale or concurrency is needed; fsync errors are uncommon (likelihood rare).
• What happens. The next call fails with an exception that names a file the caller never created. On 9.1.0 and later the call after that normally works; on 6.2.0-9.0.0 every later save fails until the policy is re-created. A later commit or IndexWriter.deleteUnusedFiles() does not remove the leftover (IndexFileDeleter.java:142-148, :461-475).
• What does not happen. While the process runs, no index data is lost or corrupted, and the reference counts in memory, which decide what the writer keeps, are restored. One detail, read and not run: after a failed release(IndexCommit) of the last reference to an older commit, incRef maps that generation to the latest commit object (SnapshotDeletionPolicy.java:115-126), so getIndexCommit(gen) can name the wrong commit.
• Secondary, contract open. If the policy is re-opened before the next call (for example after a restart), it adopts the leftover as the newest save file: a snapshot() that threw is in effect (count 1 instead of 0) and a release(IndexCommit) that threw has taken effect (count 0 instead of 1), so the writer may delete a commit the application still believes protected. The re-opened policy loads the save file with the highest generation and deletes older ones (PersistentSnapshotDeletionPolicy.java:211-263). Reviewers argue, with some weight, that the outcome of a failed fsync is indeterminate, so this is an observation, not the claim.

After failNextSync = true no commit runs, so the only sync() before the first snapshot() returns is persist()'s sync of snapshots_0. The wrapper throws before delegating, so the sync has no effect (the conservative case). ByteBuffersDirectory is write-once like FSDirectory; the commit of an empty new index needs no document. The test sits in the index package only to match Lucene's layout and uses no package-private member.

As the regression test: the same test, for example as a method in TestPersistentSnapshotDeletionPolicy.

cp TestPersistentSnapshotSyncFailure.java $LUCENE/lucene/core/src/test/org/apache/lucene/index/
cd $LUCENE && ./gradlew :lucene:core:test --tests org.apache.lucene.index.TestPersistentSnapshotSyncFailure -Ptests.seed=DEADBEEF
# assertions off: add -Ptests.asserts=false (same result)

# gradle, main 645d4fe14a20, seed DEADBEEF (assertions on; the same with -Ptests.asserts=false)
TestPersistentSnapshotSyncFailure > testSnapshotAfterFailedSaveFileSync FAILED
    java.lang.AssertionError: expected the next, fault-free snapshot() to succeed, but it threw java.nio.file.FileAlreadyExistsException: File already exists: snapshots_0
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:2F8125E02E33202F]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.apache.lucene.index.TestPersistentSnapshotSyncFailure.testSnapshotAfterFailedSaveFileSync(TestPersistentSnapshotSyncFailure.java:56)

1 test completed, 1 failed

• What the case rests on. No javadoc line promises that a call after a failed one works, and Directory.sync promises nothing when it throws. The case rests on consistency with the class's own cleanup and with Lucene's commit path, not on a documented contract.
• The class's own failure handling. snapshot() and release(IndexCommit) restore their reference counts on any persist() failure (PersistentSnapshotDeletionPolicy.java:119, :141; release(long) has no undo), and persist() deletes the new file when creating or writing it fails (:170-173). Only dir.sync (:175) is outside that cleanup.
• Tests. TestPersistentSnapshotDeletionPolicy.testExceptionDuringSave (:113-148) fails the first call inside persist() and asserts assertEquals(0, psdp.getSnapshotCount()); that first call is createOutput, so the sync branch never runs. testExistingSnapshots asserts "Make sure only 1 save file exists" (:57-64), fault-free. No test asserts the current behaviour after a sync failure.
• Lucene's commit path. SegmentInfos.write (SegmentInfos.java:605-630): "Always advance the generation on write" (:611), and directory.sync (:620) sits inside the try whose catch deletes the new file (:622-628). IndexWriter.startCommit rolls the pending commit back when sync fails (IndexWriter.java:5991-5998) and records the generations "on error or success so we don't double-write a segments_N file" (:6025-6029). TestIndexWriterExceptions.testExceptionDuringSync (LUCENE-1044, :942-972) makes 11 commits fail in sync, then asserts that the writer still commits all 23 documents. A contrast test with the same one-shot fault on pending_segments_N leaves no file and the retried commit works.
• History. LUCENE-4976 (#6040) added the single save file with the in-memory undo and the delete-on-failure (efeaac746ae, 2013-05-03). Two days later 8c6e38ee605 ("add missing sync / delete old save files", by mikemccand) appended the sync after the cleanup block; the issue thread never mentions a sync failure. #589 (2022) and #14913 (2025) refactored the exception handling and kept it there.
• Javadoc. Snapshots "are committed as soon as snapshot() or release(IndexCommit) is called" (PersistentSnapshotDeletionPolicy.java:35-37); "Once this method returns, the snapshot information is persisted in the directory" (:107-108, :129-130, :150-151). Nothing says a failed call may break the next one.

The strongest case, from two refute reviews by an independent reviewer from another model family (OpenAI GPT-5.6), both of which reproduced the failure and agreed on the root cause and the fix: the methods declare IOException and promise persistence only on normal return; Directory.sync has no transactional postcondition when it throws; after an fsync error the file may exist and its durable status is indeterminate, so deleting it "chooses a new rollback policy" and "can discard a record that may already be durable"; release(long) has no rollback at all; the class is @lucene.experimental. Hence the fix is optional hardening.

Why it fails for the headline: none of this explains why a later call, with no fault, should fail because Lucene reuses a file name it left behind itself. Lucene treats a sync failure on its own commit file as recoverable and advances the generation precisely to avoid double-writing a name. The class already treats a failed persist() in snapshot() and release(IndexCommit) as not having happened, so deleting the file matches the in-memory state, and fix B deletes nothing. @lucene.experimental marks API stability, not accepted wrong behaviour; open PR #16482 calls its release(long) rollback "consistent error recovery logic". The argument does carry weight for the restart effect, which stays out of the headline. A maintainer may still call the fix hardening (on #14334 dweiss called a silent write-back failure "a fringe case").

Option A was run through gradle in a scratch worktree of 645d4fe14a20 and is recommended. B is the alternative the adjudication suggested; it was run against the repro and the snapshot suites. C was not run. Open PR #16482 touches the class but not persist(). A related finding (LFV-194, a missing syncMetaData() after this sync) changes the same lines: a combined fix should put both calls inside the cleanup.

Wrap the existing try-with-resources and the dir.sync call in one try, keeping the existing catch that deletes fileName (+11/-8, mostly re-indentation). A failed sync then removes the new file, as a failed write already does, so for snapshot() and release(IndexCommit) disk matches the in-memory undo and the next call reuses the name safely. It also removes the restart effect: a re-loaded policy finds the previous save file. Both hold when the delete succeeds and the process does not crash between the failed sync and the delete.

String fileName = SNAPSHOTS_PREFIX + nextWriteGen;
try {
  try (IndexOutput out = dir.createOutput(fileName, IOContext.DEFAULT)) {
    // header and ref counts, unchanged
  }
  dir.sync(Collections.singletonList(fileName)); // moved into the try
} catch (Throwable t) {
  IOUtils.deleteFilesSuppressingExceptions(t, dir, fileName);
  throw t;
}

Trade-offs

• Error path only: no format, API or performance change.
• If the delete itself fails (it is suppressed), the leftover stays and the next call fails once, then cleans up, as today on 9.1.0 and later.
• The reviewers' worry about deleting a file that may already be durable weighs little here: the caller was told the call failed and memory was rolled back, so the file records a state nobody holds.
• It does not make every failed save a no-op: release(long) still changes memory before persist() and has no undo.
• If LFV-194's dir.syncMetaData() is added after the sync, it belongs inside the same try, or a failing syncMetaData() leaves the same leftover.

What the checks validated

Gradle in a scratch worktree of 645d4fe14a20: the minimal repro fails stock and passes with this change, with assertions on and off; TestPersistentSnapshotDeletionPolicy and TestSnapshotDeletionPolicy pass 20 of 20 tests with two seeds; ./gradlew :lucene:core:check -x test (format, ecjLint, forbiddenApis, including the added test) passes. The hunt's 5-test class, including the two restart tests, passes 5 of 5 with the same change. The fact check's probe with it re-loads counts 0 and 1, so the restart effect is gone. The full core suite was not run.

Take long gen = nextWriteGen; nextWriteGen++; at the top of persist() and use gen for the file name and for the delete of snapshots_(gen-1), like SegmentInfos.write ("Always advance the generation on write"). The retry writes snapshots_1 instead of colliding with snapshots_0; nothing is deleted on failure.

Trade-offs

• The failed call's file stays, so the restart effect remains: a re-loaded policy adopts it.
• After a failure getLastSaveFile() names the failed file, which may not exist.
• When an earlier successful save file exists (for example a failed release(IndexCommit) after a good snapshot()), the next successful save deletes only the failed name, so the last good file lingers until the policy is re-loaded: two save files on disk, which testExistingSnapshots forbids in the fault-free case.

What the checks validated

Gradle in the same worktree: the minimal repro passes (assertions on) and the two snapshot suites pass 20 of 20 tests. The fact check then ran all three trade-offs with its own probe: after a failed snapshot(), getLastSaveFile() is snapshots_0 and a re-loaded policy still counts 1; after a failed release(IndexCommit) and a successful retry, snapshots_0 and snapshots_2 are both on disk.

Advance the generation on every call and delete the new file on any failure: robust even when the delete fails, at the cost of B's getLastSaveFile() and lingering-file questions. Not run.

Method: non-formal (code reading, then executed tests). The hunt unit mainly enumerated crash states after a failed commit under a single-fault schedule; alongside, it read PersistentSnapshotDeletionPolicy.persist() under the same schedule (one Directory.sync that throws and has no effect) and saw that the sync is outside the cleanup. A 5-test JUnit class on the stock main jar (a FilterDirectory whose sync throws once) failed 4 of 5 with assertions on and off; the no-fault control passed. An earlier hunt unit had listed the mechanism as an unexecuted lead.

Before the finding was registered, an independent reviewer from another model family (OpenAI GPT-5.6) reproduced it with its own tests and argued hardening; that moved the restart effects to a secondary observation. On 2026-10-29 the class was cut to the one test shown here.