Skip to content

SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures #4726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iprithv wants to merge 1 commit into
base: main
Choose a base branch
from
+193 −17
Open

SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures #4726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/unreleased/gcs-copyIndexFileTo-fix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
title: >
Fixed GCS backup restores silently swallowing failures and stopping early on zero-byte reads (SOLR-18250).
type: fixed
authors:
- name: Prithvi S
Comment on lines +1 to +5
36 changes: 19 additions & 17 deletions solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
169 changes: 169 additions & 0 deletions solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -77,4 +93,157 @@ 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);
}
}
}

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);
});
}

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);
});
}

private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {

@epugh epugh Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise a doc, and I think it's cool how you are setting up specific scenarios.

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);
}
});
}

private static Object invokeAndUnwrap(Method method, Object target, Object[] args)

@epugh epugh Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh.. everytime I see these very advanced java method manipulation, I get slightly nervous, but maybe just a quick doc that says what we are using this for?

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;
}

private static class TestGCSBackupRepository extends GCSBackupRepository {

@epugh epugh Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just a bit of docs to cue the user that we are udisng this to be able to simulate certain behaviors?

private final Storage testStorage;

TestGCSBackupRepository(Storage testStorage) {
this.testStorage = testStorage;
}

@Override
protected Storage initStorage() {
this.storage = testStorage;
return testStorage;
}
}
}
Loading