fix(backup): handle S3-style object stores in config restore

changelog/unreleased/fix-s3-restore-missing-directory-marker.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Remove S3 directory marker dependency from backup/restore — use virtual-directory convention instead
+type: fixed
+authors:
+  - name: Samuel Verstraete
+links:
+  - name: GitHub PR #4182
+    url: https://github.com/apache/solr/pull/4182

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

@@ -198,15 +198,13 @@ private URI createAndValidateBackupPath(
       throws IOException {
     final URI backupNamePath = repository.resolveDirectory(location, backupName);
 
-    if ((!incremental) && repository.exists(backupNamePath)) {
+    if ((!incremental) && repository.listAllOrEmpty(backupNamePath).length > 0) {
       throw new SolrException(
           SolrException.ErrorCode.BAD_REQUEST,
           "The backup directory already exists: " + backupNamePath);
     }
 
-    if (!repository.exists(backupNamePath)) {
-      repository.createDirectory(backupNamePath);
-    } else if (incremental) {
+    if (incremental && repository.listAllOrEmpty(backupNamePath).length > 0) {
       final String[] directoryContents = repository.listAll(backupNamePath);
       if (directoryContents.length == 1) {
         String directoryContentsName = directoryContents[0];
@@ -238,9 +236,6 @@ private URI createAndValidateBackupPath(
     // Incremental backups have an additional directory named after the collection that needs
     // created
     final URI backupPathWithCollection = repository.resolveDirectory(backupNamePath, collection);
-    if (!repository.exists(backupPathWithCollection)) {
-      repository.createDirectory(backupPathWithCollection);
-    }
     BackupFilePaths incBackupFiles = new BackupFilePaths(repository, backupPathWithCollection);
     incBackupFiles.createIncrementalBackupFolders();
     return backupPathWithCollection;

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

@@ -79,14 +79,15 @@ public URI getBackupLocation() {
   }
 
   /**
-   * Create all locations required to store an incremental backup.
+   * Previously created directory marker objects at the incremental backup locations. On S3-style
+   * object stores no explicit directory marker is needed; files can be written directly under any
+   * prefix. This method is kept for interface compatibility but performs no operations.
    *
    * @throws IOException for issues encountered using repository to create directories
    */
   public void createIncrementalBackupFolders() throws IOException {
-    repository.createDirectory(backupLoc);
-    repository.createDirectory(getIndexDir());
-    repository.createDirectory(getShardBackupMetadataDir());
+    // S3 does not require directory markers. On local FS, parent directories are created
+    // implicitly when files are written. No action needed.
   }
 
   /**

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

@@ -131,7 +131,7 @@ public static BackupManager forRestore(
     Objects.requireNonNull(repository);
     Objects.requireNonNull(stateReader);
 
-    if (!repository.exists(backupPath)) {
+    if (repository.listAllOrEmpty(backupPath).length == 0) {
       throw new SolrException(
           ErrorCode.SERVER_ERROR, "Couldn't restore since doesn't exist: " + backupPath);
     }
@@ -258,7 +258,7 @@ public void uploadConfigDir(
       String sourceConfigName, String targetConfigName, ConfigSetService configSetService)
       throws IOException {
     URI source = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, sourceConfigName);
-    if (!repository.exists(source)) {
+    if (repository.listAll(source).length == 0) {
       throw new IllegalArgumentException("Configset expected at " + source + " does not exist");
     }
     uploadConfigToSolrCloud(configSetService, source, targetConfigName, "");
@@ -275,9 +275,6 @@ public void uploadConfigDir(
   public void downloadConfigDir(String configName, ConfigSetService configSetService)
       throws IOException {
     URI dest = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, configName);
-    repository.createDirectory(repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR));
-    repository.createDirectory(dest);
-
     downloadConfigToRepo(configSetService, configName, dest);
   }
 
@@ -368,9 +365,8 @@ private void downloadConfigToRepo(ConfigSetService configSetService, String conf
           }
         }
       } else {
-        if (!repository.exists(uri)) {
-          repository.createDirectory(uri);
-        }
+        // Directory entries from getAllConfigFiles are implicit on object stores;
+        // no explicit directory creation is needed.
       }
     }
   }
@@ -431,6 +427,7 @@ private URI getZkStateDir() {
   }
 
   public void createZkStateDir() throws IOException {
-    repository.createDirectory(getZkStateDir());
+    // S3 does not require directory marker objects to be written before files are stored
+    // under a prefix. This is kept as a no-op for interface compatibility.
   }
 }

solr/core/src/java/org/apache/solr/handler/SnapShooter.java

@@ -162,12 +162,12 @@ protected void deleteSnapAsync(final ReplicationHandler replicationHandler) {
   public void validateCreateSnapshot() throws IOException {
     // Note - Removed the current behavior of creating the directory hierarchy.
     // Do we really need to provide this support?
-    if (!backupRepo.exists(baseSnapDirPath)) {
+    if (backupRepo.listAllOrEmpty(baseSnapDirPath).length == 0) {
       throw new SolrException(
           SolrException.ErrorCode.BAD_REQUEST, " Directory does not exist: " + snapshotDirPath);
     }
 
-    if (backupRepo.exists(snapshotDirPath)) {
+    if (backupRepo.listAllOrEmpty(snapshotDirPath).length > 0) {
       throw new SolrException(
           SolrException.ErrorCode.BAD_REQUEST,
           "Snapshot directory already exists: " + snapshotDirPath);

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

@@ -0,0 +1,179 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import org.apache.lucene.store.IOContext;
+import org.apache.lucene.store.IndexInput;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.backup.repository.BackupRepository;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Unit tests for {@link BackupManager#uploadConfigDir} covering the case where an object-store
+ * backup (e.g. S3) has no explicit directory marker but the config files are still present.
+ */
+public class BackupManagerUploadConfigTest extends SolrTestCaseJ4 {
+
+  @BeforeClass
+  public static void setUpClass() {
+    assumeWorkingMockito();
+  }
+
+  private static final URI BACKUP_PATH = URI.create("s3://bucket/backup/");
+  private static final URI ZK_STATE_URI = URI.create("s3://bucket/backup/zk_backup/");
+  private static final URI CONFIG_DIR_URI =
+      URI.create("s3://bucket/backup/zk_backup/configs/myconfig/");
+  private static final URI SOLRCONFIG_URI =
+      URI.create("s3://bucket/backup/zk_backup/configs/myconfig/solrconfig.xml");
+
+  /**
+   * When the directory marker object is absent (S3 "no-marker" scenario) but config files exist
+   * under the prefix, {@code uploadConfigDir} must succeed and upload those files.
+   *
+   * <p>This is the regression test for the case where a backup was copied through a local
+   * filesystem and re-uploaded to S3, losing the zero-byte directory marker objects in the process.
+   */
+  @Test
+  public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent()
+      throws Exception {
+    BackupRepository mockRepo = mock(BackupRepository.class);
+    ZkStateReader mockZkStateReader = mock(ZkStateReader.class);
+    ConfigSetService mockConfigSetService = mock(ConfigSetService.class);
+
+    // Simulate S3: no marker object → listAll() finds the files
+    when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"});
+
+    // resolveDirectory chain used by getZkStateDir() and uploadConfigDir()
+    when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR))
+        .thenReturn(ZK_STATE_URI);
+    when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig"))
+        .thenReturn(CONFIG_DIR_URI);
+
+    // resolve() used by uploadConfigToSolrCloud() to build the per-file URI
+    when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI);
+    when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE);
+
+    // Return minimal valid XML bytes so FileTypeMagicUtil does not reject the file
+    byte[] configBytes = "<config/>".getBytes(StandardCharsets.UTF_8);
+    IndexInput mockInput = mock(IndexInput.class);
+    when(mockInput.length()).thenReturn((long) configBytes.length);
+    doAnswer(
+            invocation -> {
+              byte[] dest = invocation.getArgument(0);
+              int offset = invocation.getArgument(1);
+              System.arraycopy(configBytes, 0, dest, offset, configBytes.length);
+              return null;
+            })
+        .when(mockInput)
+        .readBytes(any(), eq(0), eq(configBytes.length));
+    when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT))
+        .thenReturn(mockInput);
+
+    BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH);
+
+    // Must not throw even though exists() returned false
+    backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService);
+
+    // The config file must have been uploaded to the target configset
+    verify(mockConfigSetService)
+        .uploadFileToConfig(
+            eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false));
+  }
+
+  /**
+   * When neither a directory marker nor any files exist under the config prefix, {@code
+   * uploadConfigDir} must throw {@link IllegalArgumentException}.
+   */
+  @Test
+  public void testUploadConfigDir_throwsWhenTrulyAbsent() throws Exception {
+    BackupRepository mockRepo = mock(BackupRepository.class);
+    ZkStateReader mockZkStateReader = mock(ZkStateReader.class);
+    ConfigSetService mockConfigSetService = mock(ConfigSetService.class);
+
+    URI missingConfigDir = URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/");
+
+    when(mockRepo.listAll(missingConfigDir)).thenReturn(new String[0]);
+
+    when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR))
+        .thenReturn(ZK_STATE_URI);
+    when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "nonexistent"))
+        .thenReturn(missingConfigDir);
+
+    BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH);
+
+    assertThrows(
+        IllegalArgumentException.class,
+        () -> backupManager.uploadConfigDir("nonexistent", "target", mockConfigSetService));
+  }
+
+  /**
+   * When the directory marker IS present (the normal case), {@code uploadConfigDir} uses the fast
+   * path via {@code exists()} and must still upload files correctly.
+   */
+  @Test
+  public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Exception {
+    BackupRepository mockRepo = mock(BackupRepository.class);
+    ZkStateReader mockZkStateReader = mock(ZkStateReader.class);
+    ConfigSetService mockConfigSetService = mock(ConfigSetService.class);
+
+    // listAll() is the sole existence check
+    when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"});
+
+    when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR))
+        .thenReturn(ZK_STATE_URI);
+    when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig"))
+        .thenReturn(CONFIG_DIR_URI);
+
+    when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI);
+    when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE);
+
+    byte[] configBytes = "<config/>".getBytes(StandardCharsets.UTF_8);
+    IndexInput mockInput = mock(IndexInput.class);
+    when(mockInput.length()).thenReturn((long) configBytes.length);
+    doAnswer(
+            invocation -> {
+              byte[] dest = invocation.getArgument(0);
+              int offset = invocation.getArgument(1);
+              System.arraycopy(configBytes, 0, dest, offset, configBytes.length);
+              return null;
+            })
+        .when(mockInput)
+        .readBytes(any(), eq(0), eq(configBytes.length));
+    when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT))
+        .thenReturn(mockInput);
+
+    BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH);
+
+    backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService);
+
+    verify(mockConfigSetService)
+        .uploadFileToConfig(
+            eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false));
+  }
+}

solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java

@@ -285,7 +285,6 @@ public void copyIndexFileFrom(
         throw new CorruptIndexException("file is too small:" + indexInput.length(), indexInput);
       }
 
-      client.createDirectory(getS3Path(dest));
       try (OutputStream outputStream = client.pushStream(s3Path)) {
 
         byte[] buffer = new byte[CHUNK_SIZE];