SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures

changelog/unreleased/gcs-copyIndexFileTo-fix.yml

@@ -0,0 +1,10 @@
+title: >
+  Fixed GCS backup restores silently swallowing failures and stopping early on zero-byte reads (SOLR-18250).
+type: fixed
+authors:
+  - name: Prithvi S
+links:
+  - name: SOLR-18250
+    url: https://issues.apache.org/jira/browse/SOLR-18250
+  - name: PR#4726
+    url: https://github.com/apache/solr/pull/4726

solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java

@@ -361,24 +361,26 @@ public void copyIndexFileFrom(
   public void copyIndexFileTo(
       URI sourceRepo, String sourceFileName, Directory dest, String destFileName)
       throws IOException {
-    try {
-      String blobName = sourceRepo.toString();
-      blobName = appendTrailingSeparatorIfNecessary(blobName);
-      blobName += sourceFileName;
-      final BlobId blobId = BlobId.of(bucketName, blobName);
-      try (final ReadChannel readChannel = storage.reader(blobId);
-          IndexOutput output =
-              dest.createOutput(destFileName, DirectoryFactory.IOCONTEXT_NO_CACHE)) {
-        ByteBuffer buffer = ByteBuffer.allocate(readBufferSizeBytes);
-        while (readChannel.read(buffer) > 0) {
-          buffer.flip();
-          byte[] arr = buffer.array();
-          output.writeBytes(arr, buffer.position(), buffer.limit() - buffer.position());
-          buffer.clear();
-        }
+    String blobName = sourceRepo.toString();
+    blobName = appendTrailingSeparatorIfNecessary(blobName);
+    blobName += sourceFileName;
+    final BlobId blobId = BlobId.of(bucketName, blobName);
+    try (final ReadChannel readChannel = storage.reader(blobId);
+        IndexOutput output = dest.createOutput(destFileName, DirectoryFactory.IOCONTEXT_NO_CACHE)) {
+      ByteBuffer buffer = ByteBuffer.allocate(readBufferSizeBytes);
+      while (readChannel.read(buffer) != -1) {
+        buffer.flip();
+        byte[] arr = buffer.array();
+        output.writeBytes(arr, buffer.position(), buffer.limit() - buffer.position());
+        buffer.clear();
       }
-    } catch (Exception e) {
-      log.info("Here's an exception e", e);
+    } catch (IOException e) {
+      log.error("Failed to copy index file from GCS: {}/{}", bucketName, blobName, e);
+      throw e;
+    } catch (RuntimeException e) {
+      log.error("Failed to copy index file from GCS: {}/{}", bucketName, blobName, e);
+      throw new IOException(
+          "Failed to copy index file from GCS: " + bucketName + "/" + blobName, e);
     }
   }
 

solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java

@@ -21,10 +21,26 @@
 import static org.apache.solr.gcs.GCSConfigParser.GCS_BUCKET_ENV_VAR_NAME;
 import static org.apache.solr.gcs.GCSConfigParser.GCS_CREDENTIAL_ENV_VAR_NAME;
 
+import com.google.cloud.ReadChannel;
+import com.google.cloud.storage.BlobId;
+import com.google.cloud.storage.BlobInfo;
+import com.google.cloud.storage.Storage;
+import com.google.cloud.storage.StorageException;
+import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper;
+import java.io.IOException;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.ByteBuffer;
 import java.util.HashMap;
 import java.util.Map;
+import org.apache.lucene.store.ByteBuffersDirectory;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.store.IOContext;
+import org.apache.lucene.store.IndexInput;
 import org.apache.solr.cloud.api.collections.AbstractBackupRepositoryTest;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.core.backup.repository.BackupRepository;
@@ -77,4 +93,176 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
 
     gcsBackupRepository.init(new NamedList<>(config));
   }
+
+  @Test
+  public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+    Storage failingStorage = createFailingStorage();
+    GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+    try (Directory dest = new ByteBuffersDirectory()) {
+      URI sourceDir = repo.resolve(getBaseUri(), "backup");
+      IOException thrown =
+          expectThrows(
+              IOException.class,
+              () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+      assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+      assertNotNull(thrown.getCause());
+      assertTrue(thrown.getCause() instanceof StorageException);
+      assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+    }
+  }
+
+  @Test
+  public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+    Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+    byte[] data = new byte[100];
+    random().nextBytes(data);
+    // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+    String bucketName = "solrBackupsBucket";
+
+    GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+    URI sourceDir = repo.resolve(getBaseUri(), "backup");
+    BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+    realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+    Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+    GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+    try (Directory dest = new ByteBuffersDirectory()) {
+      proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+      try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+        assertEquals(data.length, in.length());
+        byte[] read = new byte[data.length];
+        in.readBytes(read, 0, data.length);
+        assertArrayEquals(data, read);
+      }
+    }
+  }
+
+  @Test
+  public void testCopyIndexFileToCopiesFile() throws Exception {
+    Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+    byte[] data = new byte[100];
+    random().nextBytes(data);
+    // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+    String bucketName = "solrBackupsBucket";
+
+    GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+    URI sourceDir = repo.resolve(getBaseUri(), "backup");
+    BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+    realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+    try (Directory dest = new ByteBuffersDirectory()) {
+      repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+      try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+        assertEquals(data.length, in.length());
+        byte[] read = new byte[data.length];
+        in.readBytes(read, 0, data.length);
+        assertArrayEquals(data, read);
+      }
+    }
+  }
+
+  /** Storage proxy that fails on {@code reader} so we can assert copy errors are propagated. */
+  private static Storage createFailingStorage() {
+    Storage delegate = LocalStorageHelper.customOptions(false).getService();
+    return (Storage)
+        Proxy.newProxyInstance(
+            Storage.class.getClassLoader(),
+            new Class<?>[] {Storage.class},
+            (proxy, method, args) -> {
+              if ("reader".equals(method.getName())) {
+                throw new StorageException(0, "simulated GCS read failure");
+              }
+              return invokeAndUnwrap(method, delegate, args);
+            });
+  }
+
+  /**
+   * Storage proxy whose {@code reader} first returns a zero-byte {@link ReadChannel}, so we can
+   * assert that {@code copyIndexFileTo} retries instead of treating 0 as EOF.
+   */
+  private static Storage createZeroReturningStorage(Storage delegate) {
+    return (Storage)
+        Proxy.newProxyInstance(
+            Storage.class.getClassLoader(),
+            new Class<?>[] {Storage.class},
+            (proxy, method, args) -> {
+              if ("reader".equals(method.getName())
+                  && args != null
+                  && args.length == 1
+                  && args[0] instanceof BlobId) {
+                ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+                return createZeroFirstReadChannel(realChannel);
+              }
+              return invokeAndUnwrap(method, delegate, args);
+            });
+  }
+
+  /**
+   * {@link ReadChannel} that returns 0 on the first {@code read} (a short/empty read, not EOF),
+   * then delegates later reads to {@code delegate}. Used to reproduce the restore-truncation bug
+   * where a 0-byte read was treated as end-of-stream.
+   */
+  private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
+    return (ReadChannel)
+        Proxy.newProxyInstance(
+            ReadChannel.class.getClassLoader(),
+            new Class<?>[] {ReadChannel.class},
+            new InvocationHandler() {
+              private boolean returnedZero = false;
+
+              @Override
+              public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+                if ("read".equals(method.getName())
+                    && args != null
+                    && args.length == 1
+                    && args[0] instanceof ByteBuffer) {
+                  if (!returnedZero) {
+                    returnedZero = true;
+                    return 0;
+                  }
+                }
+                return invokeAndUnwrap(method, delegate, args);
+              }
+            });
+  }
+
+  /**
+   * Forwards a JDK proxy call to {@code target}. {@link Method#invoke} wraps checked exceptions in
+   * {@link InvocationTargetException}; unwrap so tests and {@code copyIndexFileTo} see the real GCS
+   * / channel exception instead of a reflection wrapper.
+   */
+  private static Object invokeAndUnwrap(Method method, Object target, Object[] args)
+      throws Throwable {
+    try {
+      return method.invoke(target, args);
+    } catch (InvocationTargetException e) {
+      throw e.getCause();
+    }
+  }
+
+  private GCSBackupRepository createRepositoryWithStorage(Storage storage) {
+    TestGCSBackupRepository repo = new TestGCSBackupRepository(storage);
+    repo.init(getBaseBackupRepositoryConfiguration());
+    return repo;
+  }
+
+  /**
+   * Test-only repository that injects a given {@link Storage} instead of creating a real GCS
+   * client. Lets tests simulate failures and unusual read behavior without talking to GCS.
+   */
+  private static class TestGCSBackupRepository extends GCSBackupRepository {
+    private final Storage testStorage;
+
+    TestGCSBackupRepository(Storage testStorage) {
+      this.testStorage = testStorage;
+    }
+
+    @Override
+    protected Storage initStorage() {
+      this.storage = testStorage;
+      return testStorage;
+    }
+  }
 }