SOLR-18249: Stage shard backup metadata before publication

changelog/unreleased/SOLR-18249.yml

@@ -0,0 +1,7 @@
+title: Stage shard backup metadata before publication and fail rather than fall back to a non-atomic local move.
+type: fixed
+authors:
+  - name: Nick Shanin
+links:
+  - name: SOLR-18249
+    url: https://issues.apache.org/jira/browse/SOLR-18249

solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteBackupCmd.java

@@ -27,7 +27,6 @@
 import java.net.URI;
 import java.nio.file.NoSuchFileException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -166,10 +165,19 @@ void deleteBackupIds(
     Set<String> referencedIndexFiles = new HashSet<>();
     List<ShardBackupId> shardBackupIdFileDeletes = new ArrayList<>();
 
-    List<ShardBackupId> shardBackupIds =
-        Arrays.stream(repository.listAllOrEmpty(shardBackupMetadataDir))
-            .map(sbi -> ShardBackupId.fromShardMetadataFilename(sbi))
-            .collect(Collectors.toList());
+    List<ShardBackupId> shardBackupIds = new ArrayList<>();
+    for (String filename : repository.listAllOrEmpty(shardBackupMetadataDir)) {
+      try {
+        shardBackupIds.add(ShardBackupId.fromShardMetadataFilename(filename));
+      } catch (IllegalArgumentException e) {
+        // The directory can hold files that are not shard metadata, such as the staged temp
+        // file an interrupted metadata write leaves behind. Such files belong to no backup
+        // point, so they are ignored here instead of failing the whole deletion.
+        if (log.isDebugEnabled()) {
+          log.debug("Ignoring file [{}] in shard backup metadata directory", filename);
+        }
+      }
+    }
     for (ShardBackupId shardBackupId : shardBackupIds) {
       final BackupId backupId = shardBackupId.getContainingBackupId();
 

solr/core/src/java/org/apache/solr/core/backup/ShardBackupMetadata.java

@@ -17,6 +17,7 @@
 
 package org.apache.solr.core.backup;
 
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -28,7 +29,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Set;
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexInput;
 import org.apache.solr.common.util.Utils;
@@ -104,20 +104,16 @@ public static ShardBackupMetadata from(
   }
 
   /**
-   * Storing ShardBackupMetadata at {@code folderURI} with name {@code filename}. If a file already
-   * existed there, overwrite it.
+   * Store this metadata at {@code folderURI} under the shard backup id's filename. The JSON is
+   * serialized completely before the repository is asked to write it; whether publication is atomic
+   * depends on the repository's {@link BackupRepository#writeBytes(URI, byte[])} implementation.
    */
   public void store(BackupRepository repository, URI folderURI, ShardBackupId shardBackupId)
       throws IOException {
     final String filename = shardBackupId.getBackupMetadataFilename();
-    URI fileURI = repository.resolve(folderURI, filename);
-    if (repository.exists(fileURI)) {
-      repository.delete(folderURI, Set.of(filename));
-    }
-
-    try (OutputStream os = repository.createOutput(repository.resolve(folderURI, filename))) {
-      store(os);
-    }
+    ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+    store(buffer);
+    repository.writeBytes(repository.resolve(folderURI, filename), buffer.toByteArray());
   }
 
   public Collection<String> listOriginalFileNames() {

solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java

@@ -147,6 +147,20 @@ default URI resolveDirectory(URI baseUri, String... pathComponents) {
    */
   OutputStream createOutput(URI path) throws IOException;
 
+  /**
+   * Write {@code data} to {@code path} using this repository's output semantics.
+   *
+   * <p>The default implementation writes directly through {@link #createOutput(URI)} and makes no
+   * atomicity guarantee: it does not stage the bytes, and a failed write may leave a partially
+   * written or replaced object. Repositories whose backing store supports it may override this
+   * method to stage the bytes and publish them atomically.
+   */
+  default void writeBytes(URI path, byte[] data) throws IOException {
+    try (OutputStream os = createOutput(path)) {
+      os.write(data);
+    }
+  }
+
   // TODO define whether this should also create any nonexistent parent directories. (i.e. is this
   // 'mkdir', or 'mkdir -p')
   /**

solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java

@@ -96,6 +96,11 @@ public OutputStream createOutput(URI path) throws IOException {
     return delegate.createOutput(path);
   }
 
+  @Override
+  public void writeBytes(URI path, byte[] data) throws IOException {
+    delegate.writeBytes(path, data);
+  }
+
   @Override
   public void createDirectory(URI path) throws IOException {
     delegate.createDirectory(path);

solr/core/src/java/org/apache/solr/core/backup/repository/LocalFileSystemRepository.java

@@ -25,8 +25,11 @@
 import java.nio.file.LinkOption;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.nio.file.StandardOpenOption;
 import java.util.Collection;
 import java.util.Objects;
+import java.util.UUID;
 import org.apache.commons.io.file.PathUtils;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.FSDirectory;
@@ -115,6 +118,37 @@ public OutputStream createOutput(URI path) throws IOException {
     return Files.newOutputStream(Path.of(path));
   }
 
+  /**
+   * Stage the bytes in a sibling file and publish them by moving the staged file onto {@code path}
+   * atomically, replacing any existing file.
+   *
+   * <p>This method does not fall back to a non-atomic move. If the provider cannot perform the
+   * atomic move, the failure is propagated and cleanup of the staged file is attempted.
+   *
+   * @throws IOException if writing or the requested atomic move fails
+   */
+  @Override
+  public void writeBytes(URI path, byte[] data) throws IOException {
+    Path dest = Path.of(path);
+    Path temp = dest.resolveSibling(dest.getFileName().toString() + ".tmp." + UUID.randomUUID());
+    try {
+      Files.write(temp, data, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE);
+      moveAtomically(temp, dest);
+    } catch (IOException | RuntimeException e) {
+      try {
+        Files.deleteIfExists(temp);
+      } catch (IOException | RuntimeException cleanupFailure) {
+        e.addSuppressed(cleanupFailure);
+      }
+      throw e;
+    }
+  }
+
+  /** Performs the atomic move used by {@link #writeBytes(URI, byte[])}. */
+  protected void moveAtomically(Path temp, Path dest) throws IOException {
+    Files.move(temp, dest, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+  }
+
   @Override
   public String[] listAll(URI dirPath) throws IOException {
     // It is better to check the existence of the directory first since

solr/core/src/test/org/apache/solr/cloud/api/collections/DeleteBackupCmdTest.java

@@ -17,13 +17,17 @@
 package org.apache.solr.cloud.api.collections;
 
 import java.io.IOException;
+import java.io.OutputStream;
 import java.net.URI;
 import java.util.Set;
 import java.util.UUID;
 import org.apache.solr.SolrTestCase;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.core.backup.BackupFilePaths;
 import org.apache.solr.core.backup.BackupId;
+import org.apache.solr.core.backup.Checksum;
+import org.apache.solr.core.backup.ShardBackupId;
+import org.apache.solr.core.backup.ShardBackupMetadata;
 import org.apache.solr.core.backup.repository.BackupRepository;
 import org.apache.solr.core.backup.repository.LocalFileSystemRepository;
 import org.junit.Before;
@@ -86,6 +90,68 @@ public void deleteDirectory(URI path) throws IOException {
     assertEquals("simulated repository failure", thrown.getMessage());
   }
 
+  @Test
+  public void testDeleteBackupIdsIgnoresStagedMetadataTempFile() throws Exception {
+    URI metadataDir = new BackupFilePaths(repository, backupUri).getShardBackupMetadataDir();
+    ShardBackupId shardBackupId = new ShardBackupId("shard1", BackupId.zero());
+    storeMetadata(metadataDir, shardBackupId);
+    String stagedFile = createStagedTempFile(metadataDir, shardBackupId);
+    URI metadataFile = repository.resolve(metadataDir, shardBackupId.getBackupMetadataFilename());
+    assertTrue(repository.exists(metadataFile));
+    assertTrue(repository.exists(repository.resolve(metadataDir, stagedFile)));
+
+    NamedList<Object> results = new NamedList<>();
+    new DeleteBackupCmd(null)
+        .deleteBackupIds(backupUri, repository, Set.of(BackupId.zero()), results);
+
+    assertNotNull(results.get("deleted"));
+    assertFalse(repository.exists(metadataFile));
+    // The staged file is not any backup point's metadata, so it is left in place.
+    assertTrue(repository.exists(repository.resolve(metadataDir, stagedFile)));
+  }
+
+  @Test
+  public void testKeepNumberOfBackupIgnoresStagedMetadataTempFile() throws Exception {
+    URI metadataDir = new BackupFilePaths(repository, backupUri).getShardBackupMetadataDir();
+    ShardBackupId oldest = new ShardBackupId("shard1", BackupId.zero());
+    ShardBackupId newest = new ShardBackupId("shard1", new BackupId(1));
+    storeMetadata(metadataDir, oldest);
+    storeMetadata(metadataDir, newest);
+    createStagedTempFile(metadataDir, oldest);
+    createBackupPropsFile(BackupId.zero());
+    createBackupPropsFile(new BackupId(1));
+
+    new DeleteBackupCmd(null).keepNumberOfBackup(repository, backupUri, 1, new NamedList<>());
+
+    assertFalse(
+        repository.exists(repository.resolve(metadataDir, oldest.getBackupMetadataFilename())));
+    assertTrue(
+        repository.exists(repository.resolve(metadataDir, newest.getBackupMetadataFilename())));
+  }
+
+  private void storeMetadata(URI metadataDir, ShardBackupId shardBackupId) throws IOException {
+    ShardBackupMetadata metadata = ShardBackupMetadata.empty();
+    metadata.addBackedFile("uniq_" + shardBackupId.getIdAsString(), "orig", new Checksum(1L, 10));
+    metadata.store(repository, metadataDir, shardBackupId);
+  }
+
+  private String createStagedTempFile(URI metadataDir, ShardBackupId shardBackupId)
+      throws IOException {
+    String stagedName = shardBackupId.getBackupMetadataFilename() + ".tmp." + UUID.randomUUID();
+    try (OutputStream out = repository.createOutput(repository.resolve(metadataDir, stagedName))) {
+      out.write('#');
+    }
+    return stagedName;
+  }
+
+  private void createBackupPropsFile(BackupId backupId) throws IOException {
+    try (OutputStream out =
+        repository.createOutput(
+            repository.resolve(backupUri, BackupFilePaths.getBackupPropsName(backupId)))) {
+      out.write('#');
+    }
+  }
+
   private URI zkStateDir(BackupId backupId) {
     return repository.resolveDirectory(backupUri, BackupFilePaths.getZkStateDir(backupId));
   }

solr/core/src/test/org/apache/solr/core/backup/ShardBackupMetadataOverwriteTest.java

@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core.backup;
+
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.backup.repository.BackupRepository;
+import org.apache.solr.core.backup.repository.DelegatingBackupRepository;
+import org.apache.solr.core.backup.repository.LocalFileSystemRepository;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Verifies that overwriting shard backup metadata never deletes the previous metadata file first.
+ * This test deliberately uses only the {@link BackupRepository} API that predates the {@code
+ * writeBytes} method, so it also compiles and runs against the code from before that change, where
+ * the overwrite deleted the existing file and this test fails.
+ */
+public class ShardBackupMetadataOverwriteTest extends SolrTestCase {
+
+  private LocalFileSystemRepository repository;
+  private URI folder;
+  private ShardBackupId shardBackupId;
+
+  @Before
+  public void setUpRepo() throws Exception {
+    repository = new LocalFileSystemRepository();
+    repository.init(new NamedList<>());
+    folder =
+        repository.createURI(createTempDir("shard-backup-metadata").toAbsolutePath().toString());
+    repository.createDirectory(folder);
+    shardBackupId = new ShardBackupId("shard1", BackupId.zero());
+  }
+
+  @Test
+  public void testStoreDoesNotDeleteExistingMetadata() throws Exception {
+    metadata("uniq1", "orig1", new Checksum(1L, 10)).store(repository, folder, shardBackupId);
+
+    RecordingBackupRepository recording = new RecordingBackupRepository(repository);
+    metadata("uniq2", "orig2", new Checksum(2L, 20)).store(recording, folder, shardBackupId);
+
+    assertTrue("overwrite must not delete the previous metadata file", recording.deleted.isEmpty());
+
+    ShardBackupMetadata loaded = ShardBackupMetadata.from(repository, folder, shardBackupId);
+    assertEquals(List.of("uniq2"), loaded.listUniqueFileNames());
+  }
+
+  private static ShardBackupMetadata metadata(
+      String uniqueFileName, String originalFileName, Checksum checksum) {
+    ShardBackupMetadata created = ShardBackupMetadata.empty();
+    created.addBackedFile(uniqueFileName, originalFileName, checksum);
+    return created;
+  }
+
+  private static class RecordingBackupRepository extends DelegatingBackupRepository {
+    final List<URI> deleted = new ArrayList<>();
+
+    RecordingBackupRepository(BackupRepository delegate) {
+      setDelegate(delegate);
+    }
+
+    @Override
+    public void delete(URI path, Collection<String> files) throws IOException {
+      for (String file : files) {
+        deleted.add(resolve(path, file));
+      }
+      super.delete(path, files);
+    }
+  }
+}