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

tjgq · copybara-github · commit 454f11de856c · 2023-01-18T06:01:40.000-08:00
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 #16086. PiperOrigin-RevId: 502854490 Change-Id: Id2d6a1728630fffea9399b406378c7f173b247bd
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

Original file line number	Diff line number	Diff line change
`@@ -350,7 +350,7 @@ private static String executionStatusExceptionErrorMessage(ExecutionStatusExcept`
`350`	`350`	`+ errorDetailsMessage(status.getDetailsList());`
`351`	`351`	`}`
`352`	`352`
`353`		`- public static String grpcAwareErrorMessage(IOException e) {`
	`353`	`+ private static String grpcAwareErrorMessage(IOException e) {`
`354`	`354`	`io.grpc.Status errStatus = io.grpc.Status.fromThrowable(e);`
`355`	`355`	`if (e.getCause() instanceof ExecutionStatusException) {`
`356`	`356`	`// Display error message returned by the remote service.`