LUCENE-6835, LUCENE-6684: move the 'suppress NSFE on windows' hack down lower, out of IFD into FSDir; also fix IFD to remove segments files before others

lucene/core/src/java/org/apache/lucene/index/IndexFileDeleter.java

@@ -696,24 +696,23 @@ final class IndexFileDeleter implements Closeable {
     ensureOpen();
 
     if (infoStream.isEnabled("IFD")) {
-      infoStream.message("IFD", "delete \"" + names + "\"");
+      infoStream.message("IFD", "delete " + names + "");
+    }
+
+    // We make two passes, first deleting any segments_N files, second deleting the rest.  We do this so that if we throw exc or JVM
+    // crashes during deletions, even when not on Windows, we don't leave the index in an "apparently corrupt" state:
+    for(String name : names) {
+      if (name.startsWith(IndexFileNames.SEGMENTS) == false) {
+        continue;
+      }
+      directory.deleteFile(name);
     }
 
     for(String name : names) {
-      try {
+      if (name.startsWith(IndexFileNames.SEGMENTS) == true) {
-        directory.deleteFile(name);
+        continue;
-      } catch (NoSuchFileException | FileNotFoundException e) {
-        // IndexWriter should only ask us to delete files it knows it wrote, so if we hit this, something is wrong!
-
-        if (Constants.WINDOWS) {
-          // TODO: can we remove this OS-specific hacky logic?  If windows deleteFile is buggy, we should instead contain this workaround in
-          // a WindowsFSDirectory ...
-          // LUCENE-6684: we suppress this assert for Windows, since a file could be in a confusing "pending delete" state, and falsely
-          // return NSFE/FNFE
-        } else {
-          throw e;
-        }
       }
+      directory.deleteFile(name);
     }
   }
 

lucene/core/src/java/org/apache/lucene/store/FSDirectory.java

@@ -327,7 +327,7 @@ public abstract class FSDirectory extends BaseDirectory {
     if (pendingDeletes.contains(name)) {
       throw new NoSuchFileException("file \"" + name + "\" is already pending delete");
     }
-    privateDeleteFile(name);
+    privateDeleteFile(name, false);
     maybeDeletePendingFiles();
   }
 
@@ -347,7 +347,7 @@ public abstract class FSDirectory extends BaseDirectory {
 
       // Clone the set since we mutate it in privateDeleteFile:
       for(String name : new HashSet<>(pendingDeletes)) {
-        privateDeleteFile(name);
+        privateDeleteFile(name, true);
       }
     }
   }
@@ -363,14 +363,21 @@ public abstract class FSDirectory extends BaseDirectory {
     }
   }
 
-  private void privateDeleteFile(String name) throws IOException {
+  private void privateDeleteFile(String name, boolean isPendingDelete) throws IOException {
     try {
       Files.delete(directory.resolve(name));
       pendingDeletes.remove(name);
     } catch (NoSuchFileException | FileNotFoundException e) {
       // We were asked to delete a non-existent file:
       pendingDeletes.remove(name);
-      throw e;
+      if (isPendingDelete && Constants.WINDOWS) {
+        // TODO: can we remove this OS-specific hacky logic?  If windows deleteFile is buggy, we should instead contain this workaround in
+        // a WindowsFSDirectory ...
+        // LUCENE-6684: we suppress this check for Windows, since a file could be in a confusing "pending delete" state, failing the first
+        // delete attempt with access denied and then apparently falsely failing here when we try ot delete it again, with NSFE/FNFE
+      } else {
+        throw e;
+      }
     } catch (IOException ioe) {
       // On windows, a file delete can fail because there's still an open
       // file handle against it.  We record this in pendingDeletes and

lucene/core/src/test/org/apache/lucene/index/TestIndexFileDeleter.java

@@ -488,4 +488,80 @@ public class TestIndexFileDeleter extends LuceneTestCase {
     w.close();
     dir.close();
   }
+
+  // LUCENE-6835: make sure best-effort to not create an "apparently but not really" corrupt index is working:
+  public void testExcInDeleteFile() throws Throwable {
+    int iters = atLeast(10);
+    for(int iter=0;iter<iters;iter++) {
+      if (VERBOSE) {
+        System.out.println("TEST: iter=" + iter);
+      }
+      MockDirectoryWrapper dir = newMockDirectory();
+
+      final AtomicBoolean doFailExc = new AtomicBoolean();
+
+      dir.failOn(new MockDirectoryWrapper.Failure() {
+          @Override
+          public void eval(MockDirectoryWrapper dir) throws IOException {
+            if (doFailExc.get() && random().nextInt(4) == 1) {
+              Exception e = new Exception();
+              StackTraceElement stack[] = e.getStackTrace();
+              for (int i = 0; i < stack.length; i++) {
+                if (stack[i].getClassName().equals(MockDirectoryWrapper.class.getName()) && stack[i].getMethodName().equals("deleteFile")) {
+                  throw new MockDirectoryWrapper.FakeIOException();
+                }
+              }
+            }
+          }
+        });
+
+      IndexWriterConfig iwc = newIndexWriterConfig();
+      iwc.setMergeScheduler(new SerialMergeScheduler());
+      RandomIndexWriter w = new RandomIndexWriter(random(), dir, iwc);
+      w.addDocument(new Document());
+
+      // makes segments_1
+      if (VERBOSE) {
+        System.out.println("TEST: now commit");
+      }
+      w.commit();
+
+      w.addDocument(new Document());
+      doFailExc.set(true);
+      if (VERBOSE) {
+        System.out.println("TEST: now close");
+      }
+      try {
+        w.close();
+        if (VERBOSE) {
+          System.out.println("TEST: no exception (ok)");
+        }
+      } catch (RuntimeException re) {
+        assertTrue(re.getCause() instanceof MockDirectoryWrapper.FakeIOException);
+        // good
+        if (VERBOSE) {
+          System.out.println("TEST: got expected exception:");
+          re.printStackTrace(System.out);
+        }
+      } catch (MockDirectoryWrapper.FakeIOException fioe) {
+        // good
+        if (VERBOSE) {
+          System.out.println("TEST: got expected exception:");
+          fioe.printStackTrace(System.out);
+        }
+      }
+      doFailExc.set(false);
+      assertFalse(w.w.isOpen());
+
+      for(String name : dir.listAll()) {
+        if (name.startsWith(IndexFileNames.SEGMENTS)) {
+          if (VERBOSE) {
+            System.out.println("TEST: now read " + name);
+          }
+          SegmentInfos.readCommit(dir, name);
+        }
+      }
+      dir.close();
+    }
+  }
 }