Correctly propagate errors and abort the build for certain remote err…

…ors. Fixes #23473. Closes #23656. PiperOrigin-RevId: 677788155 Change-Id: I974c8412fa3be775ec645e4001bf736e8ba93932
bazelbuild · Sep 23, 2024 · 3f50414 · 3f50414
1 parent e6be637
commit 3f50414
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 18 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java b/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java
@@ -36,7 +36,7 @@ public EnvironmentalExecException(IOException cause, FailureDetails.Execution.Co
         FailureDetail.newBuilder().setExecution(Execution.newBuilder().setCode(code)).build();
   }
 
-  public EnvironmentalExecException(Exception cause, FailureDetail failureDetail) {
+  public EnvironmentalExecException(Throwable cause, FailureDetail failureDetail) {
     super(failureDetail.getMessage(), cause);
     this.failureDetail = failureDetail;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java b/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java
@@ -30,10 +30,10 @@
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement;
+import com.google.devtools.build.lib.remote.common.RemoteExecutionCapabilitiesException;
 import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.RxFutures;
-import com.google.devtools.build.lib.remote.util.Utils;
 import io.grpc.ClientInterceptor;
 import io.grpc.ManagedChannel;
 import io.reactivex.rxjava3.core.Single;
@@ -153,18 +153,14 @@ public void onSuccess(ServerCapabilities result) {
 
             @Override
             public void onFailure(Throwable error) {
-              String message =
-                  "Failed to query remote execution capabilities: "
-                      + Utils.grpcAwareErrorMessage(error, verboseFailures);
-              reporter.handle(Event.error(message));
-
-              IOException exception;
-              if (error instanceof IOException ioException) {
-                exception = ioException;
+              Throwable cause;
+              if (!(error instanceof IOException)
+                  && error.getCause() instanceof IOException ioException) {
+                cause = ioException;
               } else {
-                exception = new IOException(error);
+                cause = error;
               }
-              serverCapabilities.setException(exception);
+              serverCapabilities.setException(new RemoteExecutionCapabilitiesException(cause));
             }
           },
           directExecutor());

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
@@ -15,6 +15,8 @@
 
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_DOWNLOAD;
+import static com.google.devtools.build.lib.remote.util.Utils.createExecExceptionForCredentialHelperException;
+import static com.google.devtools.build.lib.remote.util.Utils.createExecExceptionFromRemoteExecutionCapabilitiesException;
 import static com.google.devtools.build.lib.remote.util.Utils.createSpawnResult;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -27,6 +29,7 @@
 import com.google.devtools.build.lib.actions.SpawnMetrics;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
+import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.SpawnCache;
@@ -39,6 +42,7 @@
 import com.google.devtools.build.lib.remote.common.BulkTransferException;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
+import com.google.devtools.build.lib.remote.common.RemoteExecutionCapabilitiesException;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.Utils;
@@ -165,6 +169,10 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
           }
         } catch (CacheNotFoundException e) {
           // Intentionally left blank
+        } catch (CredentialHelperException e) {
+          throw createExecExceptionForCredentialHelperException(e);
+        } catch (RemoteExecutionCapabilitiesException e) {
+          throw createExecExceptionFromRemoteExecutionCapabilitiesException(e);
         } catch (IOException e) {
           if (BulkTransferException.allCausedByCacheNotFoundException(e)) {
             // Intentionally left blank

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
@@ -22,6 +22,8 @@
 import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_QUEUE;
 import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_SETUP;
 import static com.google.devtools.build.lib.profiler.ProfilerTask.UPLOAD_TIME;
+import static com.google.devtools.build.lib.remote.util.Utils.createExecExceptionForCredentialHelperException;
+import static com.google.devtools.build.lib.remote.util.Utils.createExecExceptionFromRemoteExecutionCapabilitiesException;
 import static com.google.devtools.build.lib.remote.util.Utils.createSpawnResult;
 import static java.lang.Math.max;
 
@@ -42,6 +44,7 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
+import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperException;
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.clock.BlazeClock.MillisSinceEpochToNanosConverter;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -62,6 +65,7 @@
 import com.google.devtools.build.lib.remote.circuitbreaker.CircuitBreakerFactory;
 import com.google.devtools.build.lib.remote.common.BulkTransferException;
 import com.google.devtools.build.lib.remote.common.OperationObserver;
+import com.google.devtools.build.lib.remote.common.RemoteExecutionCapabilitiesException;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.Utils;
@@ -241,6 +245,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
           }
         }
       }
+    } catch (CredentialHelperException e) {
+      throw createExecExceptionForCredentialHelperException(e);
     } catch (IOException e) {
       return execLocallyAndUploadOrFail(action, spawn, context, uploadLocalResults, e);
     }
@@ -328,6 +334,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
               throw e;
             }
           });
+    } catch (CredentialHelperException e) {
+      throw createExecExceptionForCredentialHelperException(e);
     } catch (IOException e) {
       return execLocallyAndUploadOrFail(action, spawn, context, uploadLocalResults, e);
     }
@@ -575,6 +583,9 @@ private SpawnResult execLocallyAndUploadOrFail(
   private SpawnResult handleError(
       RemoteAction action, IOException exception, SpawnExecutionContext context)
       throws ExecException, InterruptedException, IOException {
+    if (exception instanceof RemoteExecutionCapabilitiesException e) {
+      throw createExecExceptionFromRemoteExecutionCapabilitiesException(e);
+    }
     boolean remoteCacheFailed = BulkTransferException.allCausedByCacheNotFoundException(exception);
     if (exception.getCause() instanceof ExecutionStatusException e) {
       RemoteActionResult result = null;

diff --git a/...ava/com/google/devtools/build/lib/remote/common/RemoteExecutionCapabilitiesException.java b/...ava/com/google/devtools/build/lib/remote/common/RemoteExecutionCapabilitiesException.java
@@ -0,0 +1,23 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote.common;
+
+import java.io.IOException;
+
+/** An exception to indicate that Bazel failed to query remote execution capabilities. */
+public class RemoteExecutionCapabilitiesException extends IOException {
+  public RemoteExecutionCapabilitiesException(Throwable cause) {
+    super(cause);
+  }
+}
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
@@ -34,6 +34,8 @@
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
+import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnMetrics;
@@ -46,9 +48,11 @@
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
+import com.google.devtools.build.lib.remote.common.RemoteExecutionCapabilitiesException;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.protobuf.Any;
 import com.google.protobuf.ByteString;
@@ -666,4 +670,30 @@ public static ListenableFuture<Void> mergeBulkTransfer(
             },
             directExecutor());
   }
+
+  public static ExecException createExecExceptionForCredentialHelperException(
+      CredentialHelperException e) {
+    return new EnvironmentalExecException(
+        e,
+        FailureDetail.newBuilder()
+            .setRemoteOptions(
+                FailureDetails.RemoteOptions.newBuilder()
+                    .setCode(FailureDetails.RemoteOptions.Code.CREDENTIALS_READ_FAILURE)
+                    .build())
+            .setMessage("Exec failed due to CredentialHelperException")
+            .build());
+  }
+
+  public static ExecException createExecExceptionFromRemoteExecutionCapabilitiesException(
+      RemoteExecutionCapabilitiesException e) {
+    return new EnvironmentalExecException(
+        e.getCause(),
+        FailureDetail.newBuilder()
+            .setRemoteExecution(
+                RemoteExecution.newBuilder()
+                    .setCode(RemoteExecution.Code.CAPABILITIES_QUERY_FAILURE)
+                    .build())
+            .setMessage("Failed to query remote execution capabilities")
+            .build());
+  }
 }
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -97,11 +97,9 @@ function test_credential_helper_remote_cache() {
 
   bazel build \
       --remote_cache=grpc://localhost:${worker_port} \
-      //a:a >& $TEST_log || fail "Build without credentials should have succeeded"
+      //a:a >& $TEST_log && fail "Build without credentials should have failed"
   expect_log "Failed to query remote execution capabilities"
 
-  bazel clean
-
   # Helper shouldn't have been called yet.
   expect_credential_helper_calls 0
 

diff --git a/src/test/shell/bazel/remote/remote_execution_tls_test.sh b/src/test/shell/bazel/remote/remote_execution_tls_test.sh
@@ -101,7 +101,7 @@ function test_mtls_fails_if_client_has_no_cert() {
       --remote_cache=grpcs://localhost:${worker_port} \
       --tls_certificate="${cert_path}/ca.crt" \
       //a:foo &> $TEST_log \
-      || fail "Failed to build //a:foo"
+      && fail "Expected bazel to fail without a client cert" || true
   expect_log "Failed to query remote execution capabilities:"
 }
 
@@ -125,8 +125,8 @@ function test_remote_cache_with_incompatible_tls_enabled_removed_grpc_scheme() {
       --remote_cache=grpc://localhost:${worker_port} \
       --tls_certificate="${cert_path}/ca.crt" \
       ${client_mtls_flags} \
-      //a:foo >& $TEST_log \
-      || fail "Failed to build //a:foo" || true
+      //a:foo &> $TEST_log \
+      && fail "Expected test failure" || true
   expect_log "Failed to query remote execution capabilities:"
 }