Allow banning symlink action outputs from being uploaded to a remote cache.

This is mostly a roll-forward of 4465dae23de989f1452e93d0a88ac2a289103dd9, which
was reverted by fa36d2f48965b127e8fd397348d16e991135bfb6. The main difference is
that the new behavior is now gated behind the --noremote_allow_symlink_upload
flag.

https://docs.google.com/document/d/1gnOYszitgrLVet3sQk-TKGqIcpkkDsc6aw-izoo-d64
is a design proposal to support symlinks in the remote cache, which would render
this change moot. I'd like to be able to prevent incorrect cache behavior until
that change is implemented, though.

This fixes https://github.com/bazelbuild/bazel/issues/4840 (again).

Closes #5122.

Change-Id: I2136cfe82c2e1a8a9f5856e12a37d42cabd0e299
PiperOrigin-RevId: 195261827

2 files changed, 21 insertions, 9 deletions

diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
index e9ea69df1d..0912b1f375 100644
--- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
+++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
@@ -24,6 +24,7 @@ import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.remote.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.ExecutionStatusException;
 import com.google.devtools.build.lib.remote.SimpleBlobStoreActionCache;
@@ -252,6 +253,7 @@ final class ExecutionServer extends ExecutionImplBase {
         (cmdResult != null && cmdResult.getTerminationStatus().timedOut())
             || wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime);
     final int exitCode;
+    Status errStatus = null;
     ExecuteResponse.Builder resp = ExecuteResponse.newBuilder();
     if (wasTimeout) {
       final String errMessage =
@@ -259,11 +261,11 @@ final class ExecutionServer extends ExecutionImplBase {
               "Command:\n%s\nexceeded deadline of %f seconds.",
               Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0);
       logger.warning(errMessage);
-      resp.setStatus(
+      errStatus =
           Status.newBuilder()
               .setCode(Code.DEADLINE_EXCEEDED.getNumber())
               .setMessage(errMessage)
-              .build());
+              .build();
       exitCode = LOCAL_EXEC_ERROR;
     } else if (cmdResult == null) {
       exitCode = LOCAL_EXEC_ERROR;
@@ -272,19 +274,29 @@ final class ExecutionServer extends ExecutionImplBase {
     }
 
     ActionResult.Builder result = ActionResult.newBuilder();
-    cache.upload(result, execRoot, outputs);
+    try {
+      cache.upload(result, execRoot, outputs);
+    } catch (ExecException e) {
+      if (errStatus == null) {
+        errStatus =
+            Status.newBuilder()
+                .setCode(Code.FAILED_PRECONDITION.getNumber())
+                .setMessage(e.getMessage())
+                .build();
+      }
+    }
     byte[] stdout = cmdResult.getStdout();
     byte[] stderr = cmdResult.getStderr();
     cache.uploadOutErr(result, stdout, stderr);
     ActionResult finalResult = result.setExitCode(exitCode).build();
-    if (exitCode == 0 && !action.getDoNotCache()) {
+    resp.setResult(finalResult);
+    if (errStatus != null) {
+      resp.setStatus(errStatus);
+      throw new ExecutionStatusException(errStatus, resp.build());
+    } else if (exitCode == 0 && !action.getDoNotCache()) {
       ActionKey actionKey = digestUtil.computeActionKey(action);
       cache.setCachedActionResult(actionKey, finalResult);
     }
-    resp.setResult(finalResult);
-    if (wasTimeout) {
-      throw new ExecutionStatusException(resp.getStatus(), resp.build());
-    }
     return finalResult;
   }
 
diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java
index 5bf8cbbad9..a55d2d9a6a 100644
--- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java
+++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java
@@ -272,7 +272,7 @@ public final class RemoteWorker {
         new RemoteWorker(
             fs,
             remoteWorkerOptions,
-            new SimpleBlobStoreActionCache(blobStore, digestUtil),
+            new SimpleBlobStoreActionCache(remoteOptions, blobStore, digestUtil),
             sandboxPath,
             digestUtil);