Assert That AbstractRefCounted#decRef Does not Throw on Closing (#70373)

We should not allow exceptions in the internal close I think.
If the exceptions thrown here are fatal then they should just be errors
instead as well. If they are not and need handling then the handling should happen
in the `closeInternal` call. Otherwise, all callers of `decRef` must be aware of
necessary exception handling which would make reasoning about the behavior
of exceptions in `#closeInternal` extremely complicated.

Author: original-brownbear

libs/core/src/main/java/org/elasticsearch/common/util/concurrent/AbstractRefCounted.java

@@ -51,7 +51,12 @@ public final boolean decRef() {
         int i = refCount.decrementAndGet();
         assert i >= 0;
         if (i == 0) {
-            closeInternal();
+            try {
+                closeInternal();
+            } catch (Exception e) {
+                assert false : e;
+                throw e;
+            }
             return true;
         }
         return false;
@@ -81,5 +86,9 @@ public String getName() {
         return name;
     }
 
+    /**
+     * Method that is invoked once the reference count reaches zero.
+     * Implementations of this method must handle all exceptions and may not throw any exceptions.
+     */
     protected abstract void closeInternal();
 }

server/src/main/java/org/elasticsearch/index/store/Store.java

@@ -76,7 +76,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
-import java.io.UncheckedIOException;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -420,7 +419,8 @@ private void closeInternal() {
                 onClose.accept(shardLock);
             }
         } catch (IOException e) {
-            throw new UncheckedIOException(e);
+            assert false : e;
+            logger.warn(() -> new ParameterizedMessage("exception on closing store for [{}]", shardId), e);
         }
     }
 

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

@@ -3095,11 +3095,14 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
         Files.walkFileTree(indexPath, corruptedVisitor);
         assertThat("store has to be marked as corrupted", corruptedMarkerCount.get(), equalTo(1));
 
+        // Close the directory under the shard first because it's probably a MockDirectoryWrapper which throws exceptions when corrupt
         try {
-            closeShards(corruptedShard);
-        } catch (RuntimeException e) {
-            // Ignored because corrupted shard can throw various exceptions on close
+            ((FilterDirectory) corruptedShard.store().directory()).getDelegate().close();
+        } catch (CorruptIndexException | RuntimeException e) {
+            // ignored
         }
+
+        closeShards(corruptedShard);
     }
 
     public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception {