Automated rollback of commit 4465dae23de989f1452e93d0a88ac2a289103dd9.

*** Reason for rollback ***

The no-cache tag is not respected (see b/77857812) and thus this breaks remote caching for all projects with symlink outputs.

*** Original change description ***

Only allow regular files and directories spawn outputs to be uploaded to a remote cache.

The remote cache protocol only knows about regular files and
directories. Currently, during action output upload, symlinks are
resolved into regular files. This means cached "executions" of an
action may have different output file types than the original
execution, which can be a footgun. This CL bans symlinks from cachable
spawn outputs and fixes http...

***

PiperOrigin-RevId: 193338629

8 files changed, 92 insertions, 97 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index dd4cdea7f4..0a0a2a811b 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -41,6 +41,7 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.SortedMap;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -94,7 +95,8 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte
           // Actual execution.
           spawnResult = spawnRunner.exec(spawn, policy);
           if (cacheHandle.willStore()) {
-            cacheHandle.store(spawnResult);
+            cacheHandle.store(
+                spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot()));
           }
         }
       }
@@ -118,6 +120,19 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte
     return ImmutableList.of(spawnResult);
   }
 
+  private List<Path> listExistingOutputFiles(Spawn spawn, Path execRoot) {
+    ArrayList<Path> outputFiles = new ArrayList<>();
+    for (ActionInput output : spawn.getOutputFiles()) {
+      Path outputPath = execRoot.getRelative(output.getExecPathString());
+      // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead
+      // of statting the files here again.
+      if (outputPath.exists()) {
+        outputFiles.add(outputPath);
+      }
+    }
+    return outputFiles;
+  }
+
   private final class SpawnExecutionPolicyImpl implements SpawnExecutionPolicy {
     private final Spawn spawn;
     private final ActionExecutionContext actionExecutionContext;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
index 206aa58bfc..1374b47fd8 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
@@ -19,8 +19,10 @@ import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.Closeable;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.NoSuchElementException;
 
 /**
@@ -31,31 +33,32 @@ import java.util.NoSuchElementException;
  */
 public interface SpawnCache extends ActionContext {
   /** A no-op implementation that has no result, and performs no upload. */
-  public static CacheHandle NO_RESULT_NO_STORE =
-      new CacheHandle() {
-        @Override
-        public boolean hasResult() {
-          return false;
-        }
-
-        @Override
-        public SpawnResult getResult() {
-          throw new NoSuchElementException();
-        }
-
-        @Override
-        public boolean willStore() {
-          return false;
-        }
-
-        @Override
-        public void store(SpawnResult result) throws InterruptedException, IOException {
-          // Do nothing.
-        }
-
-        @Override
-        public void close() {}
-      };
+  public static CacheHandle NO_RESULT_NO_STORE = new CacheHandle() {
+    @Override
+    public boolean hasResult() {
+      return false;
+    }
+
+    @Override
+    public SpawnResult getResult() {
+      throw new NoSuchElementException();
+    }
+
+    @Override
+    public boolean willStore() {
+      return false;
+    }
+
+    @Override
+    public void store(SpawnResult result, Collection<Path> files)
+        throws InterruptedException, IOException {
+      // Do nothing.
+    }
+
+    @Override
+    public void close() {
+    }
+  };
 
   /**
    * Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance.
@@ -78,12 +81,14 @@ public interface SpawnCache extends ActionContext {
       }
 
       @Override
-      public void store(SpawnResult result) throws InterruptedException, IOException {
+      public void store(SpawnResult result, Collection<Path> files)
+          throws InterruptedException, IOException {
         throw new IllegalStateException();
       }
 
       @Override
-      public void close() {}
+      public void close() {
+      }
     };
   }
 
@@ -143,7 +148,8 @@ public interface SpawnCache extends ActionContext {
      * <p>If the current thread is interrupted, then this method should return as quickly as
      * possible with an {@link InterruptedException}.
      */
-    void store(SpawnResult result) throws ExecException, InterruptedException, IOException;
+    void store(SpawnResult result, Collection<Path> files)
+        throws InterruptedException, IOException;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
index 3bd935ca63..be81ad6cfe 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -15,16 +15,13 @@ package com.google.devtools.build.lib.remote;
 
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
-import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Dirent;
-import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.remoteexecution.v1test.ActionResult;
 import com.google.devtools.remoteexecution.v1test.Command;
 import com.google.devtools.remoteexecution.v1test.Digest;
@@ -96,7 +93,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
       Collection<Path> files,
       FileOutErr outErr,
       boolean uploadAction)
-      throws ExecException, IOException, InterruptedException;
+      throws IOException, InterruptedException;
 
   /**
    * Download a remote blob to a local destination.
@@ -256,9 +253,10 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
     }
   }
 
-  /** UploadManifest adds output metadata to a {@link ActionResult}. */
-  static class UploadManifest {
-    private final DigestUtil digestUtil;
+  /**
+   * The UploadManifest is used to mutualize upload between the RemoteActionCache implementations.
+   */
+  public class UploadManifest {
     private final ActionResult.Builder result;
     private final Path execRoot;
     private final Map<Digest, Path> digestToFile;
@@ -268,8 +266,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
      * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult
      * builder is populated through a call to {@link #addFile(Digest, Path)}.
      */
-    public UploadManifest(DigestUtil digestUtil, ActionResult.Builder result, Path execRoot) {
-      this.digestUtil = digestUtil;
+    public UploadManifest(ActionResult.Builder result, Path execRoot) {
       this.result = result;
       this.execRoot = execRoot;
 
@@ -278,31 +275,24 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
     }
 
     /**
-     * Add a collection of files or directories to the UploadManifest. Adding a directory has the
+     * Add a collection of files (and directories) to the UploadManifest. Adding a directory has the
      * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the
      * directory, including the descendants, can be reconstructed and 2) uploading all the
      * non-directory descendant files.
-     *
-     * <p>Attempting to a upload symlink results in a {@link
-     * com.google.build.lib.actions.ExecException}, since cachable actions shouldn't emit symlinks.
      */
-    public void addFiles(Collection<Path> files)
-        throws ExecException, IOException, InterruptedException {
+    public void addFiles(Collection<Path> files) throws IOException, InterruptedException {
       for (Path file : files) {
         // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and
         // rely on the local spawn runner to stat the files, instead of statting here.
-        FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW);
-        if (stat == null) {
+        if (!file.exists()) {
           // We ignore requested results that have not been generated by the action.
           continue;
         }
-        if (stat.isDirectory()) {
+        if (file.isDirectory()) {
           addDirectory(file);
-        } else if (stat.isFile()) {
-          Digest digest = digestUtil.compute(file, stat.getSize());
-          addFile(digest, file);
         } else {
-          illegalOutput(file);
+          Digest digest = digestUtil.compute(file);
+          addFile(digest, file);
         }
       }
     }
@@ -331,7 +321,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
       digestToFile.put(digest, file);
     }
 
-    private void addDirectory(Path dir) throws ExecException, IOException {
+    private void addDirectory(Path dir) throws IOException {
       Tree.Builder tree = Tree.newBuilder();
       Directory root = computeDirectory(dir, tree);
       tree.setRoot(root);
@@ -350,8 +340,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
       digestToChunkers.put(chunker.digest(), chunker);
     }
 
-    private Directory computeDirectory(Path path, Tree.Builder tree)
-        throws ExecException, IOException {
+    private Directory computeDirectory(Path path, Tree.Builder tree) throws IOException {
       Directory.Builder b = Directory.newBuilder();
 
       List<Dirent> sortedDirent = new ArrayList<>(path.readdir(TreeNodeRepository.SYMLINK_POLICY));
@@ -364,27 +353,15 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
           Directory dir = computeDirectory(child, tree);
           b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir));
           tree.addChildren(dir);
-        } else if (dirent.getType() == Dirent.Type.FILE) {
+        } else {
           Digest digest = digestUtil.compute(child);
           b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
           digestToFile.put(digest, child);
-        } else {
-          illegalOutput(child);
         }
       }
 
       return b.build();
     }
-
-    private void illegalOutput(Path what) throws ExecException, IOException {
-      String kind = what.isSymbolicLink() ? "symbolic link" : "special file";
-      throw new UserExecException(
-          String.format(
-              "Output %s is a %s. Only regular files and directories may be "
-                  + "uploaded to a remote cache. "
-                  + "Change the file type or add the \"no-cache\" tag/execution requirement.",
-              what.relativeTo(execRoot), kind));
-    }
   }
 
   /** Release resources associated with the cache. The cache may not be used after calling this. */
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index a29d7d23c4..3f5a48176b 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -25,7 +25,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.remote.Retrier.RetryException;
@@ -241,7 +240,7 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache {
       Collection<Path> files,
       FileOutErr outErr,
       boolean uploadAction)
-      throws ExecException, IOException, InterruptedException {
+      throws IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
     upload(execRoot, files, outErr, result);
     if (!uploadAction) {
@@ -267,8 +266,8 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache {
   }
 
   void upload(Path execRoot, Collection<Path> files, FileOutErr outErr, ActionResult.Builder result)
-      throws ExecException, IOException, InterruptedException {
-    UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot);
+      throws IOException, InterruptedException {
+    UploadManifest manifest = new UploadManifest(result, execRoot);
     manifest.addFiles(files);
 
     List<Chunker> filesToUpload = new ArrayList<>();
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
index 01cdabaca9..0cb40b167b 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -161,8 +161,8 @@ final class RemoteSpawnCache implements SpawnCache {
         }
 
         @Override
-        public void store(SpawnResult result)
-            throws ExecException, InterruptedException, IOException {
+        public void store(SpawnResult result, Collection<Path> files)
+            throws InterruptedException, IOException {
           if (options.experimentalGuardAgainstConcurrentChanges) {
             try {
               checkForConcurrentModifications();
@@ -176,8 +176,6 @@ final class RemoteSpawnCache implements SpawnCache {
                   && Status.SUCCESS.equals(result.status())
                   && result.exitCode() == 0;
           Context previous = withMetadata.attach();
-          Collection<Path> files =
-              RemoteSpawnRunner.resolveActionInputs(execRoot, spawn.getOutputFiles());
           try {
             remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr(), uploadAction);
           } catch (IOException e) {
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
index d6d879642e..cbbae37dfa 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.remote;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
@@ -442,12 +441,12 @@ class RemoteSpawnRunner implements SpawnRunner {
         return result;
       }
     }
-    boolean uploadAction =
-        Spawns.mayBeCached(spawn)
-            && Status.SUCCESS.equals(result.status())
-            && result.exitCode() == 0;
-    Collection<Path> outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles());
+    List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn);
     try {
+      boolean uploadAction =
+          Spawns.mayBeCached(spawn)
+              && Status.SUCCESS.equals(result.status())
+              && result.exitCode() == 0;
       remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr(), uploadAction);
     } catch (IOException e) {
       if (verboseFailures) {
@@ -471,12 +470,16 @@ class RemoteSpawnRunner implements SpawnRunner {
     }
   }
 
-  /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */
-  static Collection<Path> resolveActionInputs(
-      Path execRoot, Collection<? extends ActionInput> actionInputs) {
-    return actionInputs
-        .stream()
-        .map((inp) -> execRoot.getRelative(inp.getExecPath()))
-        .collect(ImmutableList.toImmutableList());
+  static List<Path> listExistingOutputFiles(Path execRoot, Spawn spawn) {
+    ArrayList<Path> outputFiles = new ArrayList<>();
+    for (ActionInput output : spawn.getOutputFiles()) {
+      Path outputPath = execRoot.getRelative(output.getExecPathString());
+      // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead
+      // of statting the files here again.
+      if (outputPath.exists()) {
+        outputFiles.add(outputPath);
+      }
+    }
+    return outputFiles;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index 845cf227b8..2d4f4e9bbc 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.remote;
 
 import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -115,7 +114,7 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache
       Collection<Path> files,
       FileOutErr outErr,
       boolean uploadAction)
-      throws ExecException, IOException, InterruptedException {
+      throws IOException, InterruptedException {
     ActionResult.Builder result = ActionResult.newBuilder();
     upload(result, execRoot, files);
     if (outErr.getErrorPath().exists()) {
@@ -132,8 +131,8 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache
   }
 
   public void upload(ActionResult.Builder result, Path execRoot, Collection<Path> files)
-      throws ExecException, IOException, InterruptedException {
-    UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot);
+      throws IOException, InterruptedException {
+    UploadManifest manifest = new UploadManifest(result, execRoot);
     manifest.addFiles(files);
 
     for (Map.Entry<Digest, Path> entry : manifest.getDigestToFile().entrySet()) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
index e4dc92a46f..afd31ccdeb 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
@@ -60,11 +60,9 @@ public class DigestUtil {
   }
 
   public Digest compute(Path file) throws IOException {
-    return compute(file, file.getFileSize());
-  }
-
-  public Digest compute(Path file, long fileSize) throws IOException {
-    return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), fileSize);
+    long fileSize = file.getFileSize();
+    byte[] digest = DigestUtils.getDigestOrFail(file, fileSize);
+    return buildDigest(digest, fileSize);
   }
 
   public Digest compute(VirtualActionInput input) throws IOException {