Include stack trace in all gRPC errors when --verbose_failures is set.

Also refactor a couple places where the stack trace was included in an ad-hoc manner, and force Utils.grpcAwareErrorMessage callers to be explicit to avoid future instances. Closes bazelbuild#16086. PiperOrigin-RevId: 502854490 Change-Id: Id2d6a1728630fffea9399b406378c7f173b247bd
fweikert · Jan 18, 2023 · 454f11d · 454f11d
1 parent b2976c5
commit 454f11d
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 19 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -532,7 +532,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
       }
     } catch (IOException e) {
       String errorMessage =
-          "Failed to query remote execution capabilities: " + Utils.grpcAwareErrorMessage(e);
+          "Failed to query remote execution capabilities: "
+              + Utils.grpcAwareErrorMessage(e, verboseFailures);
       if (remoteOptions.remoteLocalFallback) {
         if (verboseFailures) {
           errorMessage += System.lineSeparator() + Throwables.getStackTraceAsString(e);

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -19,7 +19,6 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Stopwatch;
-import com.google.common.base.Throwables;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -141,13 +140,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
         if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
           // Intentionally left blank
         } else {
-          String errorMessage;
-          if (!verboseFailures) {
-            errorMessage = Utils.grpcAwareErrorMessage(e);
-          } else {
-            // On --verbose_failures print the whole stack trace
-            errorMessage = "\n" + Throwables.getStackTraceAsString(e);
-          }
+          String errorMessage = Utils.grpcAwareErrorMessage(e, verboseFailures);
           if (isNullOrEmpty(errorMessage)) {
             errorMessage = e.getClass().getSimpleName();
           }

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
@@ -30,7 +30,6 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
-import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
@@ -566,11 +565,7 @@ private SpawnResult handleError(
       catastrophe = false;
     }
 
-    String errorMessage = Utils.grpcAwareErrorMessage(exception);
-    if (verboseFailures) {
-      // On --verbose_failures print the whole stack trace
-      errorMessage += "\n" + Throwables.getStackTraceAsString(exception);
-    }
+    String errorMessage = Utils.grpcAwareErrorMessage(exception, verboseFailures);
 
     if (exception.getCause() instanceof ExecutionStatusException) {
       ExecutionStatusException e = (ExecutionStatusException) exception.getCause();

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
@@ -350,7 +350,7 @@ private static String executionStatusExceptionErrorMessage(ExecutionStatusExcept
         + errorDetailsMessage(status.getDetailsList());
   }
 
-  public static String grpcAwareErrorMessage(IOException e) {
+  private static String grpcAwareErrorMessage(IOException e) {
     io.grpc.Status errStatus = io.grpc.Status.fromThrowable(e);
     if (e.getCause() instanceof ExecutionStatusException) {
       // Display error message returned by the remote service.

diff --git a/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java b/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java
@@ -36,14 +36,35 @@
 public class UtilsTest {
 
   @Test
-  public void testGrpcAwareErrorMessages() {
+  public void testGrpcAwareErrorMessage() {
     IOException ioError = new IOException("io error");
     IOException wrappedGrpcError =
         new IOException(
             "wrapped error", Status.ABORTED.withDescription("grpc error").asRuntimeException());
 
-    assertThat(Utils.grpcAwareErrorMessage(ioError)).isEqualTo("io error");
-    assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError)).isEqualTo("ABORTED: grpc error");
+    assertThat(Utils.grpcAwareErrorMessage(ioError, /* verboseFailures= */ false))
+        .isEqualTo("io error");
+    assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError, /* verboseFailures= */ false))
+        .isEqualTo("ABORTED: grpc error");
+  }
+
+  @Test
+  public void testGrpcAwareErrorMessage_verboseFailures() {
+    IOException ioError = new IOException("io error");
+    IOException wrappedGrpcError =
+        new IOException(
+            "wrapped error", Status.ABORTED.withDescription("grpc error").asRuntimeException());
+
+    assertThat(Utils.grpcAwareErrorMessage(ioError, /* verboseFailures= */ true))
+        .startsWith(
+            "io error\n"
+                + "java.io.IOException: io error\n"
+                + "\tat com.google.devtools.build.lib.remote.UtilsTest.testGrpcAwareErrorMessage_verboseFailures");
+    assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError, /* verboseFailures= */ true))
+        .startsWith(
+            "ABORTED: grpc error\n"
+                + "java.io.IOException: wrapped error\n"
+                + "\tat com.google.devtools.build.lib.remote.UtilsTest.testGrpcAwareErrorMessage_verboseFailures");
   }
 
   @Test