This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch branch_10x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_10x by this push:
new bc9c1791c67 SOLR-18250: GCSBackupRepository.copyIndexFileTo must not
swallow failures (#4726)
bc9c1791c67 is described below
commit bc9c1791c67382eeb2567404c9218a3f67211f24
Author: Prithvi S <[email protected]>
AuthorDate: Mon Aug 24 19:08:24 2026 +0530
SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures
(#4726)
Signed-off-by: prithvi <[email protected]>
(cherry picked from commit 7c02750192a494602b08c95be02828b6016052c5)
---
changelog/unreleased/gcs-copyIndexFileTo-fix.yml | 10 ++
.../org/apache/solr/gcs/GCSBackupRepository.java | 36 ++--
.../apache/solr/gcs/GCSBackupRepositoryTest.java | 188 +++++++++++++++++++++
3 files changed, 217 insertions(+), 17 deletions(-)
diff --git a/changelog/unreleased/gcs-copyIndexFileTo-fix.yml
b/changelog/unreleased/gcs-copyIndexFileTo-fix.yml
new file mode 100644
index 00000000000..9714825d5fb
--- /dev/null
+++ b/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
diff --git
a/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java
b/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java
index 3d176a5c3d2..ae767aa2e43 100644
---
a/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java
+++
b/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java
@@ -361,24 +361,26 @@ public class GCSBackupRepository extends
AbstractBackupRepository {
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);
}
}
diff --git
a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java
b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java
index 10d2acb2bd6..e4a2e084445 100644
---
a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java
+++
b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java
@@ -21,10 +21,26 @@ import static
org.apache.solr.common.params.CoreAdminParams.BACKUP_LOCATION;
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 class GCSBackupRepositoryTest extends
AbstractBackupRepositoryTest {
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;
+ }
+ }
}
