From c46bcf75cc6592cc492fabd42e1ec41dbe96304d Mon Sep 17 00:00:00 2001
From: Nhat Nguyen <nhat.nguyen@elastic.co>
Date: Mon, 31 May 2021 11:03:48 -0400
Subject: [PATCH] LUCENE-9980: Do not expose deleted commits (#158)

If we fail to delete files that belong to a commit point, then we will
expose that deleted commit in the next calls of IndexDeletionPolicy#onCommit.
I think we should never expose those deleted commit points as
some of their files might have been deleted already.
---
 .../apache/lucene/index/IndexFileDeleter.java | 17 +++++--
 .../lucene/index/TestIndexFileDeleter.java    | 47 +++++++++++++++++++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/lucene/core/src/java/org/apache/lucene/index/IndexFileDeleter.java b/lucene/core/src/java/org/apache/lucene/index/IndexFileDeleter.java
index a2aaf4b97ad..85cd7eb2fe3 100644
--- a/lucene/core/src/java/org/apache/lucene/index/IndexFileDeleter.java
+++ b/lucene/core/src/java/org/apache/lucene/index/IndexFileDeleter.java
@@ -440,10 +440,6 @@ final class IndexFileDeleter implements Closeable {
       }
       commitsToDelete.clear();
 
-      if (firstThrowable != null) {
-        throw IOUtils.rethrowAlways(firstThrowable);
-      }
-
       // Now compact commits to remove deleted ones (preserving the sort):
       size = commits.size();
       int readFrom = 0;
@@ -463,6 +459,10 @@ final class IndexFileDeleter implements Closeable {
         commits.remove(size - 1);
         size--;
       }
+
+      if (firstThrowable != null) {
+        throw IOUtils.rethrowAlways(firstThrowable);
+      }
     }
   }
 
@@ -518,6 +518,13 @@ final class IndexFileDeleter implements Closeable {
     }
   }
 
+  boolean assertCommitsAreNotDeleted(List<CommitPoint> commits) {
+    for (CommitPoint commit : commits) {
+      assert commit.isDeleted() == false : "Commit [" + commit + "] was deleted already";
+    }
+    return true;
+  }
+
   /**
    * Revisits the {@link IndexDeletionPolicy} by calling its {@link
    * IndexDeletionPolicy#onCommit(List)} again with the known commits. This is useful in cases where
@@ -532,6 +539,7 @@ final class IndexFileDeleter implements Closeable {
     }
 
     if (commits.size() > 0) {
+      assert assertCommitsAreNotDeleted(commits);
       policy.onCommit(commits);
       deleteCommits();
     }
@@ -578,6 +586,7 @@ final class IndexFileDeleter implements Closeable {
       commits.add(new CommitPoint(commitsToDelete, directoryOrig, segmentInfos));
 
       // Tell policy so it can remove commits:
+      assert assertCommitsAreNotDeleted(commits);
       policy.onCommit(commits);
 
       // Decref files for commits that were deleted by the policy:
diff --git a/lucene/core/src/test/org/apache/lucene/index/TestIndexFileDeleter.java b/lucene/core/src/test/org/apache/lucene/index/TestIndexFileDeleter.java
index a9ee9df1534..4fcc3e67963 100644
--- a/lucene/core/src/test/org/apache/lucene/index/TestIndexFileDeleter.java
+++ b/lucene/core/src/test/org/apache/lucene/index/TestIndexFileDeleter.java
@@ -575,4 +575,51 @@ public class TestIndexFileDeleter extends LuceneTestCase {
       dir.close();
     }
   }
+
+  public void testThrowExceptionWhileDeleteCommits() throws Exception {
+    try (MockDirectoryWrapper dir = newMockDirectory()) {
+      final AtomicBoolean failOnDeleteCommits = new AtomicBoolean();
+      dir.failOn(
+          new MockDirectoryWrapper.Failure() {
+            @Override
+            public void eval(MockDirectoryWrapper dir) throws IOException {
+              if (failOnDeleteCommits.get()) {
+                if (callStackContains(IndexFileDeleter.class, "deleteCommits")
+                    && callStackContains(MockDirectoryWrapper.class, "deleteFile")
+                    && random().nextInt(4) == 1) {
+                  throw new MockDirectoryWrapper.FakeIOException();
+                }
+              }
+            }
+          });
+      final SnapshotDeletionPolicy snapshotDeletionPolicy =
+          new SnapshotDeletionPolicy(new KeepOnlyLastCommitDeletionPolicy());
+      final IndexWriterConfig config =
+          newIndexWriterConfig().setIndexDeletionPolicy(snapshotDeletionPolicy);
+
+      try (RandomIndexWriter writer = new RandomIndexWriter(random(), dir, config)) {
+        writer.addDocument(new Document());
+        writer.commit();
+
+        final IndexCommit snapshotCommit = snapshotDeletionPolicy.snapshot();
+        int commits = TestUtil.nextInt(random(), 1, 3);
+        for (int i = 0; i < commits; i++) {
+          writer.addDocument(new Document());
+          writer.commit();
+        }
+        snapshotDeletionPolicy.release(snapshotCommit);
+        failOnDeleteCommits.set(true);
+        try {
+          writer.w.deleteUnusedFiles();
+        } catch (IOException e) {
+          assertTrue(e instanceof MockDirectoryWrapper.FakeIOException);
+        }
+        failOnDeleteCommits.set(false);
+        for (int c = 0; c < commits; c++) {
+          writer.addDocument(new Document());
+          writer.commit();
+        }
+      }
+    }
+  }
 }