From eb8638706def9f1eb9ebee5c5acf7ce8283676ef Mon Sep 17 00:00:00 2001
From: Tanguy Leroux <tlrx.dev@gmail.com>
Date: Wed, 29 Nov 2017 21:51:23 +0100
Subject: [PATCH] Do not swallow exception in
 ChecksumBlobStoreFormat.writeAtomic()

The ChecksumBlobStoreFormat.writeAtomic() method writes a blob using a
temporary name and then moves the blob to its final name. The move
operation can fail and in this case the temporary blob is deleted. If
this delete operation also fails, then the initial exception is lost.

This commit ensures that when something goes wrong during the move
operation the initial exception is kept and thrown, and if the delete
operation also fails then this additional exception is added
as a suppressed exception to the initial one.
---
 .../blobstore/ChecksumBlobStoreFormat.java    |  6 +-
 .../snapshots/BlobStoreFormatIT.java          | 62 +++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java b/core/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java
index 0cb38d9976d2d..a959cd0efb87e 100644
--- a/core/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java
+++ b/core/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java
@@ -138,7 +138,11 @@ public void writeAtomic(T obj, BlobContainer blobContainer, String name) throws
             blobContainer.move(tempBlobName, blobName);
         } catch (IOException ex) {
             // Move failed - try cleaning up
-            blobContainer.deleteBlob(tempBlobName);
+            try {
+                blobContainer.deleteBlob(tempBlobName);
+            } catch (Exception e) {
+                ex.addSuppressed(e);
+            }
             throw ex;
         }
     }
diff --git a/core/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java b/core/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java
index 5c1fe5d2c2c22..65926234d45c0 100644
--- a/core/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java
+++ b/core/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java
@@ -38,6 +38,7 @@
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.translog.BufferedChecksumStreamOutput;
 import org.elasticsearch.repositories.blobstore.ChecksumBlobStoreFormat;
+import org.elasticsearch.snapshots.mockstore.BlobContainerWrapper;
 
 import java.io.EOFException;
 import java.io.IOException;
@@ -210,6 +211,67 @@ public Void call() throws Exception {
         }
     }
 
+    public void testAtomicWriteFailures() throws Exception {
+        final String name = randomAlphaOfLength(10);
+        final BlobObj blobObj = new BlobObj("test");
+        final ChecksumBlobStoreFormat<BlobObj> checksumFormat = new ChecksumBlobStoreFormat<>(BLOB_CODEC, "%s", BlobObj::fromXContent,
+            xContentRegistry(), randomBoolean(), randomBoolean() ? XContentType.SMILE : XContentType.JSON);
+
+        final BlobStore blobStore = createTestBlobStore();
+        final BlobContainer blobContainer = blobStore.blobContainer(BlobPath.cleanPath());
+
+        {
+            IOException writeBlobException = expectThrows(IOException.class, () -> {
+                BlobContainer wrapper = new BlobContainerWrapper(blobContainer) {
+                    @Override
+                    public void writeBlob(String blobName, InputStream inputStream, long blobSize) throws IOException {
+                        throw new IOException("Exception thrown in writeBlob() for " + blobName);
+                    }
+                };
+                checksumFormat.writeAtomic(blobObj, wrapper, name);
+            });
+
+            assertEquals("Exception thrown in writeBlob() for pending-" + name, writeBlobException.getMessage());
+            assertEquals(0, writeBlobException.getSuppressed().length);
+        }
+        {
+            IOException moveException = expectThrows(IOException.class, () -> {
+                BlobContainer wrapper = new BlobContainerWrapper(blobContainer) {
+                    @Override
+                    public void move(String sourceBlobName, String targetBlobName) throws IOException {
+                        throw new IOException("Exception thrown in move() for " + sourceBlobName);
+                    }
+                };
+                checksumFormat.writeAtomic(blobObj, wrapper, name);
+            });
+            assertEquals("Exception thrown in move() for pending-" + name, moveException.getMessage());
+            assertEquals(0, moveException.getSuppressed().length);
+        }
+        {
+            IOException moveThenDeleteException = expectThrows(IOException.class, () -> {
+                BlobContainer wrapper = new BlobContainerWrapper(blobContainer) {
+                    @Override
+                    public void move(String sourceBlobName, String targetBlobName) throws IOException {
+                        throw new IOException("Exception thrown in move() for " + sourceBlobName);
+                    }
+
+                    @Override
+                    public void deleteBlob(String blobName) throws IOException {
+                        throw new IOException("Exception thrown in deleteBlob() for " + blobName);
+                    }
+                };
+                checksumFormat.writeAtomic(blobObj, wrapper, name);
+            });
+
+            assertEquals("Exception thrown in move() for pending-" + name, moveThenDeleteException.getMessage());
+            assertEquals(1, moveThenDeleteException.getSuppressed().length);
+
+            final Throwable suppressedThrowable = moveThenDeleteException.getSuppressed()[0];
+            assertTrue(suppressedThrowable instanceof IOException);
+            assertEquals("Exception thrown in deleteBlob() for pending-" + name, suppressedThrowable.getMessage());
+        }
+    }
+
     protected BlobStore createTestBlobStore() throws IOException {
         Settings settings = Settings.builder().build();
         return new FsBlobStore(settings, randomRepoPath());