Fixing #3380: output stack traces and informative messages.

Also added tests specifically for the output, to ensure we don't break it again.

TESTED=remote worker, unit tests
RELNOTES: fixes #3380
PiperOrigin-RevId: 163283558

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.exec;
 
-import com.google.common.base.Throwables;
 import com.google.common.eventbus.EventBus;
 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.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.Spawns;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.SpawnResult.Status;
 import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
@@ -144,15 +142,6 @@ public void report(ProgressStatus state, String name) {
     try {
       result = spawnRunner.exec(spawn, policy);
     } catch (IOException e) {
-      if (verboseFailures) {
-        actionExecutionContext
-            .getEventHandler()
-            .handle(
-                Event.warn(
-                    spawn.getMnemonic()
-                        + " remote work failed:\n"
-                        + Throwables.getStackTraceAsString(e)));
-      }
       throw new EnvironmentalExecException("Unexpected IO error.", e);
     }
 

src/main/java/com/google/devtools/build/lib/remote/CacheNotFoundException.java

@@ -15,24 +15,21 @@
 package com.google.devtools.build.lib.remote;
 
 import com.google.devtools.remoteexecution.v1test.Digest;
+import java.io.IOException;
 
 /**
  * An exception to indicate cache misses.
  * TODO(olaola): have a class of checked RemoteCacheExceptions.
  */
-public final class CacheNotFoundException extends Exception {
+public final class CacheNotFoundException extends IOException {
   private final Digest missingDigest;
 
   CacheNotFoundException(Digest missingDigest) {
+    super("Missing digest: " + missingDigest);
     this.missingDigest = missingDigest;
   }
 
   public Digest getMissingDigest() {
     return missingDigest;
   }
-
-  @Override
-  public String toString() {
-    return "Missing digest: " + missingDigest;
-  }
 }

src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java

@@ -19,7 +19,6 @@
 import com.google.bytestream.ByteStreamProto.ReadRequest;
 import com.google.bytestream.ByteStreamProto.ReadResponse;
 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.common.util.concurrent.ListeningScheduledExecutorService;
@@ -29,7 +28,6 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.remote.Digests.ActionKey;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
-import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -54,6 +52,7 @@
 import io.grpc.Status;
 import io.grpc.StatusRuntimeException;
 import io.grpc.protobuf.StatusProto;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.ArrayList;
@@ -197,7 +196,7 @@ private void downloadTree(Digest rootDigest, Path rootLocation) {
    */
   @Override
   public void download(ActionResult result, Path execRoot, FileOutErr outErr)
-      throws IOException, InterruptedException, CacheNotFoundException {
+      throws IOException, InterruptedException {
     for (OutputFile file : result.getOutputFilesList()) {
       Path path = execRoot.getRelative(file.getPath());
       FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
@@ -211,25 +210,17 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
             file.getContent().writeTo(stream);
           }
         } else {
-          try {
-            retrier.execute(
-                () -> {
-                  try (OutputStream stream = path.getOutputStream()) {
-                    Iterator<ReadResponse> replies = readBlob(digest);
-                    while (replies.hasNext()) {
-                      replies.next().getData().writeTo(stream);
-                    }
-                  }
-                  Digest receivedDigest = Digests.computeDigest(path);
-                  if (!receivedDigest.equals(digest)) {
-                    throw new IOException(
-                        "Digest does not match " + receivedDigest + " != " + digest);
-                  }
-                  return null;
-                });
-          } catch (RetryException e) {
-            Throwables.throwIfInstanceOf(e.getCause(), CacheNotFoundException.class);
-            throw e;
+          retrier.execute(
+              () -> {
+                try (OutputStream stream = path.getOutputStream()) {
+                  readBlob(digest, stream);
+                }
+                return null;
+              });
+          Digest receivedDigest = Digests.computeDigest(path);
+          if (!receivedDigest.equals(digest)) {
+            throw new IOException(
+                "Digest does not match " + receivedDigest + " != " + digest);
           }
         }
       }
@@ -243,7 +234,7 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
   }
 
   private void downloadOutErr(ActionResult result, FileOutErr outErr)
-      throws IOException, InterruptedException, CacheNotFoundException {
+      throws IOException, InterruptedException {
     if (!result.getStdoutRaw().isEmpty()) {
       result.getStdoutRaw().writeTo(outErr.getOutputStream());
       outErr.getOutputStream().flush();
@@ -268,21 +259,24 @@ private void downloadOutErr(ActionResult result, FileOutErr outErr)
    * {@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}.
+   * <p>This method also converts any NOT_FOUND code returned from the server into a
+   * {@link CacheNotFoundException}. TODO(olaola): this is not enough. NOT_FOUND can also be raised
+   * by execute, in which case the server should return the missing digest in the Status.details
+   * field. This should be part of the API.
    */
-  private Iterator<ReadResponse> readBlob(Digest digest)
-      throws CacheNotFoundException, StatusRuntimeException {
+  private void readBlob(Digest digest, OutputStream stream)
+      throws IOException, StatusRuntimeException {
     String resourceName = "";
     if (!options.remoteInstanceName.isEmpty()) {
       resourceName += options.remoteInstanceName + "/";
     }
     resourceName += "blobs/" + digest.getHash() + "/" + digest.getSizeBytes();
     try {
-      return bsBlockingStub()
+      Iterator<ReadResponse> replies = bsBlockingStub()
           .read(ReadRequest.newBuilder().setResourceName(resourceName).build());
+      while (replies.hasNext()) {
+        replies.next().getData().writeTo(stream);
+      }
     } catch (StatusRuntimeException e) {
       if (e.getStatus().getCode() == Status.Code.NOT_FOUND) {
         throw new CacheNotFoundException(digest);
@@ -405,29 +399,16 @@ Digest uploadBlob(byte[] blob) throws IOException, InterruptedException {
   }
 
   byte[] downloadBlob(Digest digest)
-      throws IOException, InterruptedException, CacheNotFoundException {
+      throws IOException, InterruptedException {
     if (digest.getSizeBytes() == 0) {
       return new byte[0];
     }
-    byte[] result = new byte[(int) digest.getSizeBytes()];
-    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;
+    return retrier.execute(
+        () -> {
+          ByteArrayOutputStream stream = new ByteArrayOutputStream((int) digest.getSizeBytes());
+          readBlob(digest, stream);
+          return stream.toByteArray();
+        });
   }
 
   // Execution Cache API

src/main/java/com/google/devtools/build/lib/remote/RemoteActionCache.java

@@ -55,10 +55,12 @@ void ensureInputsPresent(
   /**
    * Download the output files and directory trees of a remotely executed action to the local
    * machine, as well stdin / stdout to the given files.
+   *
+   * @throws CacheNotFoundException in case of a cache miss.
    */
   // TODO(olaola): will need to amend to include the TreeNodeRepository for updating.
   void download(ActionResult result, Path execRoot, FileOutErr outErr)
-      throws IOException, InterruptedException, CacheNotFoundException;
+      throws IOException, InterruptedException;
   /**
    * Attempts to look up the given action in the remote cache and return its result, if present.
    * Returns {@code null} if there is no such entry. Note that a successful result from this method

src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java

@@ -56,6 +56,7 @@ public Iterable<? extends ActionContext> getActionContexts() {
         env.getExecRoot(),
         remoteOptions,
         createFallbackRunner(env),
+        executionOptions.verboseFailures,
         cache,
         executor);
     RemoteSpawnStrategy spawnStrategy =

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

@@ -14,6 +14,7 @@
 
 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.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
@@ -39,6 +40,7 @@
 import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
 import com.google.devtools.remoteexecution.v1test.Platform;
 import com.google.protobuf.Duration;
+import io.grpc.Status.Code;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -58,6 +60,7 @@ final class RemoteSpawnRunner implements SpawnRunner {
   // TODO(olaola): This will be set on a per-action basis instead.
   private final Platform platform;
   private final SpawnRunner fallbackRunner;
+  private final boolean verboseFailures;
 
   @Nullable private final RemoteActionCache remoteCache;
   @Nullable private final GrpcRemoteExecutor remoteExecutor;
@@ -66,6 +69,7 @@ final class RemoteSpawnRunner implements SpawnRunner {
       Path execRoot,
       RemoteOptions options,
       SpawnRunner fallbackRunner,
+      boolean verboseFailures,
       @Nullable RemoteActionCache remoteCache,
       @Nullable GrpcRemoteExecutor remoteExecutor) {
     this.execRoot = execRoot;
@@ -74,6 +78,7 @@ final class RemoteSpawnRunner implements SpawnRunner {
     this.fallbackRunner = fallbackRunner;
     this.remoteCache = remoteCache;
     this.remoteExecutor = remoteExecutor;
+    this.verboseFailures = verboseFailures;
   }
 
   @Override
@@ -149,21 +154,20 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
         return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
       }
 
-      io.grpc.Status grpcStatus = io.grpc.Status.fromThrowable(e);
-      final String message;
-      if (io.grpc.Status.UNAVAILABLE.getCode().equals(grpcStatus.getCode())) {
-        message = "The remote executor/cache is unavailable: " + grpcStatus.getDescription();
+      String message = "";
+      if (e instanceof RetryException
+          && ((RetryException) e).causedByStatusCode(Code.UNAVAILABLE)) {
+        message = "The remote executor/cache is unavailable";
+      } else if (e instanceof CacheNotFoundException) {
+        message = "Failed to download from remote cache";
       } else {
-        message = "I/O Error in remote cache/executor: " + e.getMessage();
+        message = "Error in remote cache/executor";
       }
-      throw new EnvironmentalExecException(message, true);
-    } catch (CacheNotFoundException e) {
-      if (options.remoteLocalFallback) {
-        return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
+      // TODO(olaola): reuse the ErrorMessage class for these errors.
+      if (verboseFailures) {
+        message += "\n" + Throwables.getStackTraceAsString(e);
       }
-
-      String message = "Failed to download from remote cache: " + e.getMessage();
-      throw new EnvironmentalExecException(message, true);
+      throw new EnvironmentalExecException(message, e, true);
     }
   }
 

src/main/java/com/google/devtools/build/lib/remote/RetryException.java

@@ -24,7 +24,7 @@ public final class RetryException extends IOException {
   private final int attempts;
 
   RetryException(Throwable cause, int retryAttempts) {
-    super(cause);
+    super(String.format("after %d attempts: %s", retryAttempts + 1, cause), cause);
     this.attempts = retryAttempts + 1;
   }
 
@@ -40,9 +40,4 @@ public boolean causedByStatusCode(Code code) {
     }
     return false;
   }
-
-  @Override
-  public String toString() {
-    return String.format("after %d attempts: %s", attempts, getCause());
-  }
 }

src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java

@@ -79,7 +79,7 @@ public void ensureInputsPresent(
   }
 
   public void downloadTree(Digest rootDigest, Path rootLocation)
-      throws IOException, CacheNotFoundException, InterruptedException {
+      throws IOException, InterruptedException {
     Directory directory = Directory.parseFrom(downloadBlob(rootDigest));
     for (FileNode file : directory.getFilesList()) {
       downloadFileContents(
@@ -110,7 +110,7 @@ private Digest uploadFileContents(
 
   @Override
   public void download(ActionResult result, Path execRoot, FileOutErr outErr)
-      throws IOException, CacheNotFoundException, InterruptedException {
+      throws IOException, InterruptedException {
     for (OutputFile file : result.getOutputFilesList()) {
       if (!file.getContent().isEmpty()) {
         createFile(
@@ -129,7 +129,7 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
   }
 
   private void downloadOutErr(ActionResult result, FileOutErr outErr)
-          throws IOException, CacheNotFoundException, InterruptedException {
+          throws IOException, InterruptedException {
     if (!result.getStdoutRaw().isEmpty()) {
       result.getStdoutRaw().writeTo(outErr.getOutputStream());
       outErr.getOutputStream().flush();
@@ -202,7 +202,7 @@ public void uploadOutErr(ActionResult.Builder result, byte[] stdout, byte[] stde
   }
 
   private void downloadFileContents(Digest digest, Path dest, boolean executable)
-      throws IOException, CacheNotFoundException, InterruptedException {
+      throws IOException, InterruptedException {
     FileSystemUtils.createDirectoryAndParents(dest.getParentDirectory());
     try (OutputStream out = dest.getOutputStream()) {
       downloadBlob(digest, out);
@@ -248,7 +248,7 @@ public Digest uploadBlob(Digest digest, InputStream in)
   }
 
   public void downloadBlob(Digest digest, OutputStream out)
-      throws IOException, CacheNotFoundException, InterruptedException {
+      throws IOException, InterruptedException {
     if (digest.getSizeBytes() == 0) {
       return;
     }
@@ -259,7 +259,7 @@ public void downloadBlob(Digest digest, OutputStream out)
   }
 
   public byte[] downloadBlob(Digest digest)
-      throws IOException, CacheNotFoundException, InterruptedException {
+      throws IOException, InterruptedException {
     if (digest.getSizeBytes() == 0) {
       return new byte[0];
     }