Refactor RemoteSpawn{Strategy,Runner}

- Make the RemoteSpawnRunner match RemoteSpawnStrategy functionality, including
  local fallback, remote caching, and execution. This is done so we can
  actually finish the migration to the SpawnRunner API, for which I've had a
  pending change for several months now.

- Never throw StatusRuntimeException from the GrpcRemoteCache or the
  GrpcExecutor. We almost never do, since the Retrier catches them implcitly,
  so a number of catch blocks were already unreachable. Carefully document
  the cases where we still need to handle it.

- RemoteSpawnStrategy / RemoteSpawnRunner no longer catch gRPC-specific
  exceptions; they should be able to handle any reasonable remote caching /
  execution implementation (except we don't have a common interface for
  GrpcRemoteExecutor yet), with no dependency on gRPC as such. Note that the
  RemoteSpawnStrategy class will actually go away after the SpawnRunner
  migration (eventually).

- However, ensure that we _are_ actually throwing CacheNotFoundException;
  the retrier implicitly catches that also, so we need to manually unwrap
  from RetryException.

- Don't call into the EventHandler from RemoteSpawnStrategy; instead, throw
  an exception with the message, and let the higher levels handle the
  reporting (we only allow this for exception + local fallback, for which
  there's no good reporting API right now).

PiperOrigin-RevId: 161195666

7 files changed, 212 insertions, 125 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index 036fa93f4f..55ee38ab6e 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -22,6 +22,7 @@ import com.google.bytestream.ByteStreamProto.ReadResponse;
 import com.google.bytestream.ByteStreamProto.WriteRequest;
 import com.google.bytestream.ByteStreamProto.WriteResponse;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -209,16 +210,21 @@ public class GrpcRemoteCache implements RemoteActionCache {
             file.getContent().writeTo(stream);
           }
         } else {
-          retrier.execute(
-              () -> {
-                try (OutputStream stream = path.getOutputStream()) {
-                  Iterator<ReadResponse> replies = readBlob(digest);
-                  while (replies.hasNext()) {
-                    replies.next().getData().writeTo(stream);
+          try {
+            retrier.execute(
+                () -> {
+                  try (OutputStream stream = path.getOutputStream()) {
+                    Iterator<ReadResponse> replies = readBlob(digest);
+                    while (replies.hasNext()) {
+                      replies.next().getData().writeTo(stream);
+                    }
+                    return null;
                   }
-                  return null;
-                }
-              });
+                });
+          } catch (RetryException e) {
+            Throwables.throwIfInstanceOf(e.getCause(), CacheNotFoundException.class);
+            throw e;
+          }
         }
       }
       path.setExecutable(file.getIsExecutable());
@@ -250,7 +256,19 @@ public class GrpcRemoteCache implements RemoteActionCache {
     }
   }
 
-  private Iterator<ReadResponse> readBlob(Digest digest) throws CacheNotFoundException {
+  /**
+   * This method can throw {@link StatusRuntimeException}, but the RemoteCache interface does not
+   * allow throwing such an exception. Any caller must make sure to catch the
+   * {@link StatusRuntimeException}. Note that the retrier implicitly catches it, so if this is used
+   * in the context of {@link Retrier#execute}, that's perfectly safe.
+   *
+   * <p>On the other hand, this method can also throw {@link CacheNotFoundException}, but the
+   * retrier also implicitly catches that and wraps it in a {@link RetryException}, so any caller
+   * that wants to propagate the {@link CacheNotFoundException} needs to catch
+   * {@link RetryException} and rethrow the cause if it is a {@link CacheNotFoundException}.
+   */
+  private Iterator<ReadResponse> readBlob(Digest digest)
+      throws CacheNotFoundException, StatusRuntimeException {
     String resourceName = "";
     if (!options.remoteInstanceName.isEmpty()) {
       resourceName += options.remoteInstanceName + "/";
@@ -282,10 +300,12 @@ public class GrpcRemoteCache implements RemoteActionCache {
                           .setActionDigest(actionKey.getDigest())
                           .setActionResult(result)
                           .build()));
-    } catch (StatusRuntimeException e) {
-      if (e.getStatus().getCode() != Status.Code.UNIMPLEMENTED) {
-        throw e;
+    } catch (RetryException e) {
+      if (e.causedByStatusCode(Status.Code.UNIMPLEMENTED)) {
+        // Silently return without upload.
+        return;
       }
+      throw e;
     }
   }
 
@@ -469,24 +489,28 @@ public class GrpcRemoteCache implements RemoteActionCache {
       return new byte[0];
     }
     byte[] result = new byte[(int) digest.getSizeBytes()];
-    retrier.execute(
-        () -> {
-          Iterator<ReadResponse> replies = readBlob(digest);
-          int offset = 0;
-          while (replies.hasNext()) {
-            ByteString data = replies.next().getData();
-            data.copyTo(result, offset);
-            offset += data.size();
-          }
-          Preconditions.checkState(digest.getSizeBytes() == offset);
-          return null;
-        });
+    try {
+      retrier.execute(
+          () -> {
+            Iterator<ReadResponse> replies = readBlob(digest);
+            int offset = 0;
+            while (replies.hasNext()) {
+              ByteString data = replies.next().getData();
+              data.copyTo(result, offset);
+              offset += data.size();
+            }
+            Preconditions.checkState(digest.getSizeBytes() == offset);
+            return null;
+          });
+    } catch (RetryException e) {
+      Throwables.throwIfInstanceOf(e.getCause(), CacheNotFoundException.class);
+      throw e;
+    }
     return result;
   }
 
   // Execution Cache API
 
-  /** Returns a cached result for a given Action digest, or null if not found in cache. */
   @Override
   public ActionResult getCachedActionResult(ActionKey actionKey)
       throws IOException, InterruptedException {
@@ -501,6 +525,7 @@ public class GrpcRemoteCache implements RemoteActionCache {
                           .build()));
     } catch (RetryException e) {
       if (e.causedByStatusCode(Status.Code.NOT_FOUND)) {
+        // Return null to indicate that it was a cache miss.
         return null;
       }
       throw e;
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
index e460c3ea7c..173f10ddb0 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.remote;
 
-import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
@@ -69,14 +68,14 @@ public class GrpcRemoteExecutor {
   }
 
   private @Nullable ExecuteResponse getOperationResponse(Operation op)
-      throws IOException, UserExecException {
+      throws IOException {
     if (op.getResultCase() == Operation.ResultCase.ERROR) {
       StatusRuntimeException e = StatusProto.toStatusRuntimeException(op.getError());
       if (e.getStatus().getCode() == Code.DEADLINE_EXCEEDED) {
         // This was caused by the command itself exceeding the timeout,
         // therefore it is not retriable.
         // TODO(olaola): this should propagate a timeout SpawnResult instead of raising.
-        throw new UserExecException("Remote execution time out", true);
+        throw new IOException("Remote execution time out");
       }
       throw e;
     }
@@ -92,7 +91,7 @@ public class GrpcRemoteExecutor {
   }
 
   public ExecuteResponse executeRemotely(ExecuteRequest request)
-      throws InterruptedException, IOException, UserExecException {
+      throws IOException, InterruptedException {
     Operation op = retrier.execute(() -> execBlockingStub().execute(request));
     ExecuteResponse resp = getOperationResponse(op);
     if (resp != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 0c1a6cf63f..7b6971623c 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -15,8 +15,8 @@
 package com.google.devtools.build.lib.remote;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.Spawns;
@@ -33,10 +33,11 @@ import com.google.devtools.remoteexecution.v1test.ActionResult;
 import com.google.devtools.remoteexecution.v1test.Command;
 import com.google.devtools.remoteexecution.v1test.Digest;
 import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
+import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
 import com.google.devtools.remoteexecution.v1test.Platform;
 import com.google.protobuf.Duration;
-import io.grpc.StatusRuntimeException;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.SortedMap;
@@ -49,68 +50,102 @@ final class RemoteSpawnRunner implements SpawnRunner {
   private final RemoteOptions options;
   // TODO(olaola): This will be set on a per-action basis instead.
   private final Platform platform;
+  private final SpawnRunner fallbackRunner;
 
-  private final GrpcRemoteExecutor executor;
-  private final GrpcRemoteCache remoteCache;
+  private final RemoteActionCache remoteCache;
+  private final GrpcRemoteExecutor remoteExecutor;
 
   RemoteSpawnRunner(
       Path execRoot,
       RemoteOptions options,
-      GrpcRemoteExecutor executor,
-      GrpcRemoteCache remoteCache) {
+      SpawnRunner fallbackRunner,
+      RemoteActionCache remoteCache,
+      GrpcRemoteExecutor remoteExecutor) {
     this.execRoot = execRoot;
     this.options = options;
     this.platform = options.parseRemotePlatformOverride();
-    this.executor = executor;
+    this.fallbackRunner = fallbackRunner;
     this.remoteCache = remoteCache;
+    this.remoteExecutor = remoteExecutor;
   }
 
   @Override
   public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
       throws ExecException, InterruptedException, IOException {
-    ActionExecutionMetadata owner = spawn.getResourceOwner();
-    if (owner.getOwner() != null) {
-      policy.report(ProgressStatus.EXECUTING);
+    if (!spawn.isRemotable() || remoteCache == null) {
+      return fallbackRunner.exec(spawn, policy);
     }
 
-    try {
-      // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
-      TreeNodeRepository repository =
-          new TreeNodeRepository(execRoot, policy.getActionInputFileCache());
-      SortedMap<PathFragment, ActionInput> inputMap = policy.getInputMapping();
-      TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
-      repository.computeMerkleDigests(inputRoot);
-      Command command = buildCommand(spawn.getArguments(), spawn.getEnvironment());
-      Action action =
-          buildAction(
-              spawn.getOutputFiles(),
-              Digests.computeDigest(command),
-              repository.getMerkleDigest(inputRoot),
-              Spawns.getTimeoutSeconds(spawn));
+    policy.report(ProgressStatus.EXECUTING);
+    // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
+    ActionInputFileCache inputFileCache = policy.getActionInputFileCache();
+    TreeNodeRepository repository = new TreeNodeRepository(execRoot, inputFileCache);
+    SortedMap<PathFragment, ActionInput> inputMap = policy.getInputMapping();
+    TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
+    repository.computeMerkleDigests(inputRoot);
+    Command command = buildCommand(spawn.getArguments(), spawn.getEnvironment());
+    Action action =
+        buildAction(
+            spawn.getOutputFiles(),
+            Digests.computeDigest(command),
+            repository.getMerkleDigest(inputRoot),
+            Spawns.getTimeoutSeconds(spawn));
 
-      ActionKey actionKey = Digests.computeActionKey(action);
+    // Look up action cache, and reuse the action output if it is found.
+    ActionKey actionKey = Digests.computeActionKey(action);
+    try {
+      boolean acceptCachedResult = options.remoteAcceptCached;
       ActionResult result =
-          options.remoteAcceptCached ? remoteCache.getCachedActionResult(actionKey) : null;
-      if (result == null) {
-        // Cache miss or we don't accept cache hits.
-        // Upload the command and all the inputs into the remote cache.
-        remoteCache.ensureInputsPresent(repository, execRoot, inputRoot, command);
-        // TODO(olaola): set BuildInfo and input total bytes as well.
-        ExecuteRequest.Builder request =
-            ExecuteRequest.newBuilder()
-                .setInstanceName(options.remoteInstanceName)
-                .setAction(action)
-                .setTotalInputFileCount(inputMap.size())
-                .setSkipCacheLookup(!options.remoteAcceptCached);
-        result = executor.executeRemotely(request.build()).getResult();
+          acceptCachedResult
+              ? remoteCache.getCachedActionResult(actionKey)
+              : null;
+      if (result != null) {
+        // We don't cache failed actions, so we know the outputs exist.
+        // For now, download all outputs locally; in the future, we can reuse the digests to
+        // just update the TreeNodeRepository and continue the build.
+        try {
+          remoteCache.download(result, execRoot, policy.getFileOutErr());
+          return new SpawnResult.Builder()
+              .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
+              .setExitCode(result.getExitCode())
+              .build();
+        } catch (CacheNotFoundException e) {
+          acceptCachedResult = false; // Retry the action remotely and invalidate the results.
+        }
+      }
+
+      if (remoteExecutor == null) {
+        return execLocally(spawn, policy, remoteCache, actionKey);
       }
 
+      // Upload the command and all the inputs into the remote cache.
+      remoteCache.ensureInputsPresent(repository, execRoot, inputRoot, command);
+      // TODO(olaola): set BuildInfo and input total bytes as well.
+      ExecuteRequest.Builder request =
+          ExecuteRequest.newBuilder()
+              .setInstanceName(options.remoteInstanceName)
+              .setAction(action)
+              .setTotalInputFileCount(inputMap.size())
+              .setSkipCacheLookup(!acceptCachedResult);
+      ExecuteResponse reply = remoteExecutor.executeRemotely(request.build());
+      result = reply.getResult();
+      if (options.remoteLocalFallback && result.getExitCode() != 0) {
+        return execLocally(spawn, policy, remoteCache, actionKey);
+      }
       remoteCache.download(result, execRoot, policy.getFileOutErr());
       return new SpawnResult.Builder()
           .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
           .setExitCode(result.getExitCode())
           .build();
-    } catch (StatusRuntimeException | CacheNotFoundException e) {
+    } catch (IOException e) {
+      if (options.remoteLocalFallback) {
+        return execLocally(spawn, policy, remoteCache, actionKey);
+      }
+      throw e;
+    } catch (CacheNotFoundException e) {
+      if (options.remoteLocalFallback) {
+        return execLocally(spawn, policy, remoteCache, actionKey);
+      }
       throw new IOException(e);
     }
   }
@@ -147,4 +182,32 @@ final class RemoteSpawnRunner implements SpawnRunner {
     }
     return command.build();
   }
+
+  /**
+   * Fallback: execute the spawn locally. If an ActionKey is provided, try to upload results to
+   * remote action cache.
+   */
+  private SpawnResult execLocally(
+      Spawn spawn,
+      SpawnExecutionPolicy policy,
+      RemoteActionCache remoteCache,
+      ActionKey actionKey)
+      throws ExecException, IOException, InterruptedException {
+    SpawnResult result = fallbackRunner.exec(spawn, policy);
+    if (options.remoteUploadLocalResults && remoteCache != null && actionKey != null) {
+      ArrayList<Path> outputFiles = new ArrayList<>();
+      for (ActionInput output : spawn.getOutputFiles()) {
+        Path outputFile = execRoot.getRelative(output.getExecPathString());
+        // Ignore non-existent files.
+        // TODO(ulfjack): This is not ideal - in general, all spawn strategies should stat the
+        // output files and return a list of existing files. We shouldn't re-stat the files here.
+        if (!outputFile.exists()) {
+          continue;
+        }
+        outputFiles.add(outputFile);
+      }
+      remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr());
+    }
+    return result;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
index 346d43e1bb..15545bdc44 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.remote;
 
-import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -27,7 +26,6 @@ import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.remote.Digests.ActionKey;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
@@ -43,7 +41,6 @@ import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
 import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
 import com.google.devtools.remoteexecution.v1test.Platform;
 import com.google.protobuf.Duration;
-import io.grpc.StatusRuntimeException;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -136,7 +133,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
       ActionExecutionContext actionExecutionContext,
       RemoteActionCache remoteCache,
       ActionKey actionKey)
-      throws ExecException, InterruptedException {
+      throws ExecException, IOException, InterruptedException {
     fallbackStrategy.exec(spawn, actionExecutionContext);
     if (remoteOptions.remoteUploadLocalResults && remoteCache != null && actionKey != null) {
       ArrayList<Path> outputFiles = new ArrayList<>();
@@ -150,22 +147,8 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
         }
         outputFiles.add(outputFile);
       }
-      try {
-        remoteCache.upload(
-            actionKey, execRoot, outputFiles, actionExecutionContext.getFileOutErr());
-      } catch (IOException e) {
-        throw new UserExecException("Unexpected IO error.", e);
-      } catch (UnsupportedOperationException e) {
-        actionExecutionContext
-            .getEventHandler()
-            .handle(
-                Event.warn(
-                    spawn.getMnemonic() + " unsupported operation for action cache (" + e + ")"));
-      } catch (StatusRuntimeException e) {
-        actionExecutionContext
-            .getEventHandler()
-            .handle(Event.warn(spawn.getMnemonic() + " failed uploading results (" + e + ")"));
-      }
+      remoteCache.upload(
+          actionKey, execRoot, outputFiles, actionExecutionContext.getFileOutErr());
     }
   }
 
@@ -178,9 +161,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
   @Override
   public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
-    ActionKey actionKey = null;
     String mnemonic = spawn.getMnemonic();
-    EventHandler eventHandler = actionExecutionContext.getEventHandler();
 
     if (!spawn.isRemotable() || remoteCache == null) {
       fallbackStrategy.exec(spawn, actionExecutionContext);
@@ -193,6 +174,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
         .getEventBus()
         .post(ActionStatusMessage.runningStrategy(spawn.getResourceOwner(), "remote"));
 
+    ActionKey actionKey = null;
     try {
       // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
       ActionInputFileCache inputFileCache = actionExecutionContext.getActionInputFileCache();
@@ -260,38 +242,24 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
                 verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
         throw new UserExecException(message + ": Exit " + result.getExitCode());
       }
-    } catch (RetryException e) {
-      String stackTrace = "";
-      if (verboseFailures) {
-        stackTrace = "\n" + Throwables.getStackTraceAsString(e);
-      }
-      eventHandler.handle(Event.warn(mnemonic + " remote work failed (" + e + ")" + stackTrace));
-      if (remoteOptions.remoteLocalFallback) {
-        execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
-      } else {
-        String cwd = actionExecutionContext.getExecRoot().getPathString();
-        String message =
-            CommandFailureUtils.describeCommandFailure(
-                verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
-        throw new UserExecException(message, e.getCause());
-      }
-    } catch (CacheNotFoundException e) {
+    } catch (CacheNotFoundException | IOException e) {
+      Exception reportedCause = e;
       // TODO(olaola): handle this exception by reuploading / reexecuting the action remotely.
-      eventHandler.handle(Event.warn(mnemonic + " remote work results cache miss (" + e + ")"));
       if (remoteOptions.remoteLocalFallback) {
-        execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
-      } else {
-        String cwd = actionExecutionContext.getExecRoot().getPathString();
-        String message =
-            CommandFailureUtils.describeCommandFailure(
-                verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
-        throw new UserExecException(message, e);
+        actionExecutionContext.getEventHandler().handle(
+            Event.warn(mnemonic + " remote work failed (" + e + ")"));
+        try {
+          execLocally(spawn, actionExecutionContext, remoteCache, actionKey);
+          return;
+        } catch (IOException e2) {
+          reportedCause = e2;
+        }
       }
-    } catch (IOException e) {
-      throw new UserExecException("Unexpected IO error.", e);
-    } catch (UnsupportedOperationException e) {
-      eventHandler.handle(
-          Event.warn(mnemonic + " unsupported operation for action cache (" + e + ")"));
+      String cwd = actionExecutionContext.getExecRoot().getPathString();
+      String message =
+          CommandFailureUtils.describeCommandFailure(
+              verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
+      throw new UserExecException(message, reportedCause);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java
index 981af46199..cd69ad4d0f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.util.Preconditions;
 import io.grpc.Status;
 import io.grpc.StatusException;
 import io.grpc.StatusRuntimeException;
+import java.io.IOException;
 import java.time.Duration;
 import java.util.List;
 import java.util.concurrent.Callable;
@@ -280,9 +281,11 @@ public class Retrier {
    * backoff/retry policy. Will raise the last encountered retriable error, or the first
    * non-retriable error.
    *
+   * <p>This method never throws {@link StatusRuntimeException} even if the passed-in Callable does.
+   *
    * @param c The callable to execute.
    */
-  public <T> T execute(Callable<T> c) throws InterruptedException, RetryException {
+  public <T> T execute(Callable<T> c) throws InterruptedException, IOException {
     Backoff backoff = backoffSupplier.get();
     while (true) {
       try {
@@ -290,8 +293,11 @@ public class Retrier {
       } catch (StatusException | StatusRuntimeException e) {
         onFailure(backoff, Status.fromThrowable(e));
       } catch (Exception e) {
-        // Generic catch because Callable is declared to throw Exception.
+        // Generic catch because Callable is declared to throw Exception, we rethrow any unchecked
+        // exception as well as any exception we declared above.
         Throwables.throwIfUnchecked(e);
+        Throwables.throwIfInstanceOf(e, IOException.class);
+        Throwables.throwIfInstanceOf(e, InterruptedException.class);
         throw new RetryException(e, backoff.getRetryAttempts());
       }
     }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index dd4237fc81..e299ba7873 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -173,7 +173,7 @@ public class GrpcRemoteExecutionClientTest {
     Channel channel = InProcessChannelBuilder.forName(fakeServerName).directExecutor().build();
     GrpcRemoteExecutor executor = new GrpcRemoteExecutor(channel, ChannelOptions.DEFAULT, options);
     GrpcRemoteCache remoteCache = new GrpcRemoteCache(channel, ChannelOptions.DEFAULT, options);
-    client = new RemoteSpawnRunner(execRoot, options, executor, remoteCache);
+    client = new RemoteSpawnRunner(execRoot, options, null, remoteCache, executor);
     inputDigest = fakeFileCache.createScratchInput(simpleSpawn.getInputFiles().get(0), "xyz");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
index ac54c26da7..9a8e464914 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
@@ -19,6 +19,7 @@ import static org.mockito.Mockito.when;
 
 import com.google.common.collect.Range;
 import io.grpc.Status;
+import java.io.IOException;
 import java.time.Duration;
 import org.junit.Before;
 import org.junit.Test;
@@ -71,7 +72,7 @@ public class RetrierTest {
     assertThat(backoff.nextDelayMillis()).isEqualTo(Retrier.Backoff.STOP);
   }
 
-  void assertThrows(Retrier retrier, int attempts) throws InterruptedException {
+  void assertThrows(Retrier retrier, int attempts) throws InterruptedException, IOException {
     try {
       retrier.execute(() -> fooMock.foo());
       fail();
@@ -108,5 +109,30 @@ public class RetrierTest {
     Mockito.verify(retrier, Mockito.times(2)).sleep(2000);
     Mockito.verify(fooMock, Mockito.times(6)).foo();
   }
-}
 
+  @Test
+  public void testInterruptedExceptionIsPassedThrough() throws Exception {
+    InterruptedException thrown = new InterruptedException();
+    try {
+      Retrier.NO_RETRIES.execute(() -> {
+        throw thrown;
+      });
+      fail();
+    } catch (InterruptedException expected) {
+      assertThat(expected).isSameAs(thrown);
+    }
+  }
+
+  @Test
+  public void testIOExceptionIsPassedThrough() throws Exception {
+    IOException thrown = new IOException();
+    try {
+      Retrier.NO_RETRIES.execute(() -> {
+        throw thrown;
+      });
+      fail();
+    } catch (IOException expected) {
+      assertThat(expected).isSameAs(thrown);
+    }
+  }
+}