Rollback persisted configuration on blob store add/modify failures

When adding or modifying a blob store, if an exception occurs after the
configuration has been persisted to disk, the stored XML file could be left
in an invalid state that prevents application startup on restart.

This prevents GeoServer to store invalid blob store configurations, for
as long as the BlobStore throws UnsuitableStorageException, in order not
to change the current logic.

This fix ensures that if save() or handleAddBlobStore()/handleModifyBlobStore()
fail, the persisted configuration is rolled back by re-saving the previous
valid state. Makes loadConfiguration() package-private with @VisibleForTesting
to allow tests to verify the persisted state matches expectations after rollback.

Author: groldan

geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java

@@ -15,6 +15,7 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.thoughtworks.xstream.XStream;
 import com.thoughtworks.xstream.io.xml.DomReader;
 import java.io.FileNotFoundException;
@@ -290,7 +291,8 @@ public void setDefaultValues(TileLayer layer) {
         }
     }
 
-    private GeoWebCacheConfiguration loadConfiguration() throws ConfigurationException {
+    @VisibleForTesting
+    GeoWebCacheConfiguration loadConfiguration() throws ConfigurationException {
         Assert.isTrue(resourceProvider.hasInput(), "Resource provider must have an input");
         try {
             try (InputStream in = resourceProvider.in()) {
@@ -969,19 +971,44 @@ public synchronized void addBlobStore(BlobStoreInfo info) {
         } catch (IOException ioe) {
             // save failed, roll back the add
             blobStores.remove(info);
-            throw new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), ioe);
+            ConfigurationPersistenceException thrown =
+                    new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), ioe);
+
+            try { // reverting the stored config by re-saving
+                save();
+            } catch (IOException suppressed) {
+                log.log(
+                        Level.WARNING,
+                        "Unable to revert stored configuration after failure to store BlobStore " + info,
+                        suppressed);
+                thrown.addSuppressed(thrown);
+            }
+            throw thrown;
         }
         try {
             blobStoreListeners.safeForEach(listener -> {
                 listener.handleAddBlobStore(info);
             });
         } catch (IOException | GeoWebCacheException e) {
+            ConfigurationPersistenceException thrown;
             if (ExceptionUtils.isOrSuppresses(e, UnsuitableStorageException.class)) {
                 // Can't store here, roll back
                 blobStores.remove(info);
-                throw new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), e);
+                thrown = new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), e);
+                try { // reverting the stored config by re-saving
+                    save();
+                    log.log(Level.CONFIG, "Configuration reverted after failure to add BlobStore " + info);
+                } catch (IOException suppress) {
+                    log.log(
+                            Level.WARNING,
+                            "Unexpected error saving the reverted configuration after failure to add BlobStore " + info,
+                            suppress);
+                    thrown.addSuppressed(thrown);
+                }
+            } else {
+                thrown = new ConfigurationPersistenceException(e);
             }
-            throw new ConfigurationPersistenceException(e);
+            throw thrown;
         }
     }
 
@@ -1050,7 +1077,21 @@ public synchronized void modifyBlobStore(BlobStoreInfo info) {
                 // Can't store here, roll back
                 blobStores.remove(info);
                 blobStores.add(infoToRemove);
-                throw new ConfigurationPersistenceException("Unable to modify BlobStoreInfo \"%s\"".formatted(info), e);
+                ConfigurationPersistenceException thrown = new ConfigurationPersistenceException(
+                        "Unable to modify BlobStoreInfo \"%s\"".formatted(info), e);
+                try {
+                    // roll-back stored config, leaving an invalid stored config will prevent application startup
+                    save();
+                    log.log(Level.CONFIG, "Configuration reverted after failure to modify BlobStore " + info);
+                } catch (IOException suppress) {
+                    log.log(
+                            Level.WARNING,
+                            "Unexpected error saving the reverted configuration after failure to modify BlobStore "
+                                    + info,
+                            suppress);
+                    thrown.addSuppressed(suppress);
+                }
+                throw thrown;
             }
             throw new ConfigurationPersistenceException(e);
         }

geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java

@@ -16,6 +16,7 @@
 import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.replay;
+import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.greaterThan;
@@ -31,12 +32,15 @@
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import java.io.File;
 import java.io.FileInputStream;
+import java.io.FilterOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -67,13 +71,16 @@
 import org.geowebcache.grid.SRS;
 import org.geowebcache.layer.TileLayer;
 import org.geowebcache.layer.wms.WMSLayer;
+import org.geowebcache.storage.UnsuitableStorageException;
 import org.geowebcache.util.TestUtils;
 import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.mockito.Mockito;
 import org.springframework.context.ApplicationContext;
+import org.springframework.web.context.WebApplicationContext;
 import org.xml.sax.SAXParseException;
 
 public class XMLConfigurationTest {
@@ -477,7 +484,7 @@ public void testNoBlobStores() throws Exception {
     }
 
     @Test
-    public void testSaveBlobStores() throws Exception {
+    public void testAddBlobStores() throws Exception {
         FileBlobStoreInfo store1 = new FileBlobStoreInfo();
         store1.setName("store1");
         store1.setDefault(true);
@@ -517,6 +524,150 @@ public void testSaveBlobStores() throws Exception {
         assertEquals(store2, stores.get(1));
     }
 
+    @Test
+    public void testAddBlobStoreExceptionSaving() throws Exception {
+
+        XMLFileResourceProvider resourceProvider =
+                new XMLFileResourceProvider(
+                        XMLConfiguration.DEFAULT_CONFIGURATION_FILE_NAME,
+                        (WebApplicationContext) null,
+                        this.configDir.getAbsolutePath(),
+                        null) {
+
+                    // throw an ioexception the first time close() is called, the second time is the roll-back
+                    private boolean thrown = false;
+
+                    @Override
+                    public OutputStream out() throws IOException {
+                        OutputStream real = super.out();
+                        return new FilterOutputStream(real) {
+
+                            @Override
+                            public void close() throws IOException {
+                                real.close();
+                                if (thrown) {
+                                    return;
+                                }
+                                thrown = true;
+                                throw new IOException("forced io exception");
+                            }
+                        };
+                    }
+                };
+
+        config = new XMLConfiguration(null, resourceProvider);
+        config.setGridSetBroker(gridSetBroker);
+
+        FileBlobStoreInfo store1 = new FileBlobStoreInfo();
+        store1.setName("store1");
+        store1.setDefault(true);
+        store1.setEnabled(true);
+        store1.setFileSystemBlockSize(8096);
+        store1.setBaseDirectory("/tmp/test");
+
+        assertThrows(ConfigurationPersistenceException.class, () -> config.addBlobStore(store1));
+        assertEquals(0, config.getBlobStoreCount());
+        GeoWebCacheConfiguration configuration = config.loadConfiguration();
+        assertTrue("store shouldn't be saved", configuration.getBlobStores().isEmpty());
+    }
+
+    /**
+     * Verifies the blobstore configuration is rolled back from the persisted configuration if a
+     * {@link BlobStoreConfigurationListener#handleAddBlobStore} throws an {@link UnsuitableStorageException}.
+     *
+     * <p>Note I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not on
+     * IOException or GeoWebCacheException
+     */
+    @Test
+    public void testAddBlobStoreExceptionFromListener() throws Exception {
+        FileBlobStoreInfo store1 = new FileBlobStoreInfo();
+        store1.setName("store1");
+        store1.setDefault(true);
+        store1.setEnabled(true);
+        store1.setFileSystemBlockSize(8096);
+        store1.setBaseDirectory("/tmp/test");
+
+        BlobStoreConfigurationListener listener;
+
+        listener = mock(BlobStoreConfigurationListener.class);
+        doThrow(new UnsuitableStorageException("fake")).when(listener).handleAddBlobStore(Mockito.any());
+        config.addBlobStoreListener(listener);
+
+        assertAddBlobStoreFails(store1, UnsuitableStorageException.class);
+
+        // note, I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not
+        // on IOException or GeoWebCacheException
+        // doThrow(new IOException("fake")).when(listener).handleAddBlobStore(Mockito.any());
+        // assertAddBlobStoreFails(store1, IOException.class);
+        //
+        // doThrow(new GeoWebCacheException("fake")).when(listener).handleAddBlobStore(Mockito.any());
+        // assertAddBlobStoreFails(store1, GeoWebCacheException.class);
+    }
+
+    private void assertAddBlobStoreFails(FileBlobStoreInfo store, Class<? extends Exception> expectedCause)
+            throws ConfigurationException {
+        ConfigurationPersistenceException expected;
+        expected = assertThrows(ConfigurationPersistenceException.class, () -> config.addBlobStore(store));
+        assertThat(expected.getCause(), instanceOf(expectedCause));
+        assertEquals(0, config.getBlobStoreCount());
+        GeoWebCacheConfiguration configuration = config.loadConfiguration();
+        assertTrue("store shouldn't be saved", configuration.getBlobStores().isEmpty());
+    }
+
+    /**
+     * Verifies the blobstore configuration is rolled back from the persisted configuration if a
+     * {@link BlobStoreConfigurationListener#handleModifyBlobStore(BlobStoreInfo)} throws an
+     * {@link UnsuitableStorageException}.
+     *
+     * <p>Note I'm not sure why XMLConfiguration.modifyBobstore() only rolls-back on UnsuitableStorageException and not
+     * on IOException or GeoWebCacheException
+     */
+    @Test
+    public void testModifyBlobStoreExceptionFromListener() throws Exception {
+        FileBlobStoreInfo original = new FileBlobStoreInfo();
+        original.setName("store1");
+        original.setDefault(true);
+        original.setEnabled(true);
+        original.setFileSystemBlockSize(8096);
+        original.setBaseDirectory("/tmp/test");
+
+        config.addBlobStore(original);
+
+        BlobStoreConfigurationListener listener;
+
+        listener = mock(BlobStoreConfigurationListener.class);
+        doThrow(new UnsuitableStorageException("fake")).when(listener).handleModifyBlobStore(Mockito.any());
+        config.addBlobStoreListener(listener);
+
+        assertModifyBlobStoreFails(original, UnsuitableStorageException.class);
+
+        // note, I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not
+        // on IOException or GeoWebCacheException
+        // doThrow(new IOException("fake")).when(listener).handleModifyBlobStore(Mockito.any());
+        // assertModifyBlobStoreFails(original, IOException.class);
+        //
+        // doThrow(new GeoWebCacheException("fake")).when(listener).handleModifyBlobStore(Mockito.any());
+        // assertModifyBlobStoreFails(original, GeoWebCacheException.class);
+    }
+
+    private void assertModifyBlobStoreFails(FileBlobStoreInfo original, Class<? extends Exception> expectedCause)
+            throws ConfigurationException {
+
+        FileBlobStoreInfo modified = (FileBlobStoreInfo) original.clone();
+        modified.setBaseDirectory("/tmp/test2");
+
+        assertEquals(1, config.getBlobStoreCount());
+
+        ConfigurationPersistenceException expected;
+        expected = assertThrows(ConfigurationPersistenceException.class, () -> config.modifyBlobStore(modified));
+        assertThat(expected.getCause(), instanceOf(expectedCause));
+        GeoWebCacheConfiguration reloaded = config.loadConfiguration();
+        assertEquals(1, reloaded.getBlobStores().size());
+
+        BlobStoreInfo stored = reloaded.getBlobStores().get(0);
+        assertEquals("store shouldn't be saved", original, stored);
+    }
+
     @Test
     public void testSaveCurrentVersion() throws Exception {