Inject metadata when creating a filesystem symlink for a non-symlink …

…artifact.

A ctx.actions.symlink whose output is a declare_file or declare_directory
(as opposed to a declare_symlink) has "copy" semantics, i.e., the output
artifact is indistinguishable from the referent except for its name; the
symlink is just a filesystem-level optimization to avoid an actual copy,
and is transparently resolved when collecting the action output metadata.

When the symlink points to an artifact that was built remotely and without the
bytes, we currently must download it before executing the symlink action, so
that the output metadata can be constructed from the local filesystem. This
change short-circuits the filesystem traversal by injecting output metadata,
which is identical to the input plus a pointer to the original path. This is
used by the prefetcher to avoid downloading the same files multiple times when
they're symlinked more than once.

(An alternative would have been to teach all of the RemoteActionFileSystem
methods to resolve symlinks by patching together the local and remote metadata,
but that would have resulted in an awful lot of complexity.)

Fixes #15678.

src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java

@@ -84,6 +84,18 @@ TrieArtifact getOrPutSubFolder(String name) {
       return trieArtifact;
     }
 
+    @Nullable
+    TreeArtifactValue findTreeArtifactNode(PathFragment execPath) {
+      TrieArtifact current = this;
+      for (String segment : execPath.segments()) {
+        current = current.subFolders.get(segment);
+        if (current == null) {
+          return null;
+        }
+      }
+      return current.treeArtifactValue;
+    }
+
     @Nullable
     TreeArtifactValue findTreeArtifactNodeAtPrefix(PathFragment execPath) {
       TrieArtifact current = this;
@@ -244,6 +256,17 @@ private FileArtifactValue getMetadataFromTreeArtifacts(PathFragment execPath) {
     return entry != null ? entry.getValue() : null;
   }
 
+  @Nullable
+  @Override
+  public TreeArtifactValue getTreeMetadata(SpecialArtifact input) {
+    return getTreeMetadata(input.getExecPath());
+  }
+
+  @Nullable
+  public TreeArtifactValue getTreeMetadata(PathFragment execPath) {
+    return treeArtifactsRoot.findTreeArtifactNode(execPath);
+  }
+
   @Nullable
   @Override
   public ActionInput getInput(String execPathString) {

src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.actions;
 
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
@@ -34,8 +35,10 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.rmi.Remote;
 import java.util.Arrays;
 import java.util.Objects;
+import java.util.Optional;
 import javax.annotation.Nullable;
 
 /**
@@ -539,16 +542,27 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
     private final long size;
     private final int locationIndex;
     private final String actionId;
-
-    public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) {
+    @Nullable private final PathFragment symlinkTargetExecPath;
+
+    public RemoteFileArtifactValue(
+        byte[] digest,
+        long size,
+        int locationIndex,
+        String actionId,
+        @Nullable PathFragment symlinkTargetExecPath) {
       this.digest = Preconditions.checkNotNull(digest, actionId);
       this.size = size;
       this.locationIndex = locationIndex;
       this.actionId = actionId;
+      this.symlinkTargetExecPath = symlinkTargetExecPath;
+    }
+
+    public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) {
+      this(digest, size, locationIndex, actionId, /* symlinkTargetExecPath= */ null);
     }
 
     public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) {
-      this(digest, size, locationIndex, /* actionId= */ "");
+      this(digest, size, locationIndex, /* actionId= */ "", /* symlinkTargetExecPath= */ null);
     }
 
     @Override
@@ -561,12 +575,14 @@ public boolean equals(Object o) {
       return Arrays.equals(digest, that.digest)
           && size == that.size
           && locationIndex == that.locationIndex
-          && Objects.equals(actionId, that.actionId);
+          && Objects.equals(actionId, that.actionId)
+          && Objects.equals(symlinkTargetExecPath, that.symlinkTargetExecPath);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId);
+      return Objects.hash(
+          Arrays.hashCode(digest), size, locationIndex, actionId, symlinkTargetExecPath);
     }
 
     @Override
@@ -597,7 +613,7 @@ public String getActionId() {
     @Override
     public long getModifiedTime() {
       throw new UnsupportedOperationException(
-          "RemoteFileArifactValue doesn't support getModifiedTime");
+          "RemoteFileArtifactValue doesn't support getModifiedTime");
     }
 
     @Override
@@ -615,12 +631,25 @@ public boolean isRemote() {
       return true;
     }
 
+    /**
+     * Optional symlink target path.
+     *
+     * <p>If set, this artifact is a copy of another artifact. It is materialized in the local
+     * filesystem as a symlink to the original artifact, whose contents live at this location. This
+     * is used to implement zero-cost copies of remotely stored artifacts.
+     */
+    public Optional<PathFragment> getSymlinkTargetExecPath() {
+      return Optional.ofNullable(symlinkTargetExecPath);
+    }
+
     @Override
     public String toString() {
       return MoreObjects.toStringHelper(this)
           .add("digest", bytesToString(digest))
           .add("size", size)
           .add("locationIndex", locationIndex)
+          .add("actionId", actionId)
+          .add("symlinkTargetExecPath", symlinkTargetExecPath)
           .toString();
     }
   }

src/main/java/com/google/devtools/build/lib/actions/MetadataProvider.java

@@ -14,7 +14,9 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import java.io.IOException;
 import javax.annotation.Nullable;
@@ -48,6 +50,10 @@ public interface MetadataProvider {
   @Nullable
   FileArtifactValue getMetadata(ActionInput input) throws IOException;
 
+  /** Returns a {@link TreeArtifactValue} for the given {@link ActionInput}. */
+  @Nullable
+  TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException;
+
   /** Looks up an input from its exec path. */
   @Nullable
   ActionInput getInput(String execPath);

src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java

@@ -36,6 +36,7 @@
 import com.google.devtools.build.lib.util.VarInt;
 import com.google.devtools.build.lib.vfs.DigestUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import com.google.devtools.build.lib.vfs.UnixGlob;
 import java.io.ByteArrayOutputStream;
@@ -466,6 +467,14 @@ private static void encodeRemoteMetadata(
     VarInt.putVarInt(value.getLocationIndex(), sink);
 
     VarInt.putVarInt(indexer.getOrCreateIndex(value.getActionId()), sink);
+
+    Optional<PathFragment> symlinkTargetExecPath = value.getSymlinkTargetExecPath();
+    if (symlinkTargetExecPath.isPresent()) {
+      VarInt.putVarInt(1, sink);
+      VarInt.putVarInt(indexer.getOrCreateIndex(symlinkTargetExecPath.get().toString()), sink);
+    } else {
+      VarInt.putVarInt(0, sink);
+    }
   }
 
   private static final int MAX_REMOTE_METADATA_SIZE =
@@ -484,7 +493,14 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
 
     String actionId = getStringForIndex(indexer, VarInt.getVarInt(source));
 
-    return new RemoteFileArtifactValue(digest, size, locationIndex, actionId);
+    PathFragment symlinkTargetExecPath = null;
+    if (VarInt.getVarInt(source) != 0) {
+      symlinkTargetExecPath =
+          PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
+    }
+
+    return new RemoteFileArtifactValue(
+        digest, size, locationIndex, actionId, symlinkTargetExecPath);
   }
 
   /** @return action data encoded as a byte[] array. */

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -203,6 +203,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:caffeine",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java

@@ -17,9 +17,11 @@
 import com.github.benmanes.caffeine.cache.Caffeine;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.DigestOfDirectoryException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.Symlinks;
@@ -81,6 +83,14 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException {
         .getMetadata();
   }
 
+  @Override
+  @Nullable
+  public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException {
+    // TODO(tjgq): Construct a TreeArtifactValue from the filesystem to mirror getMetadata?
+    // If so, we should share the logic with MetadataHandler#constructTreeArtifactFromFilesystem.
+    return null;
+  }
+
   @Override
   @Nullable
   public ActionInput getInput(String execPath) {

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -31,13 +31,16 @@
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.remote.util.AsyncTaskCache;
 import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult;
 import com.google.devtools.build.lib.remote.util.TempPathGenerator;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import io.reactivex.rxjava3.core.Completable;
 import io.reactivex.rxjava3.core.Flowable;
 import io.reactivex.rxjava3.core.Single;
@@ -118,6 +121,7 @@ protected ListenableFuture<Void> prefetchFiles(
       Priority priority) {
     Map<SpecialArtifact, List<TreeFileArtifact>> trees = new HashMap<>();
     List<ActionInput> files = new ArrayList<>();
+
     for (ActionInput input : inputs) {
       if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) {
         continue;
@@ -138,23 +142,55 @@ protected ListenableFuture<Void> prefetchFiles(
             .flatMapSingle(
                 entry ->
                     toTransferResult(
-                        prefetchInputTree(
+                        prefetchInputTreeOrSymlink(
                             metadataProvider, entry.getKey(), entry.getValue(), priority)));
+
     Flowable<TransferResult> fileDownloads =
         Flowable.fromIterable(files)
             .flatMapSingle(
-                input -> toTransferResult(prefetchInputFile(metadataProvider, input, priority)));
+                input ->
+                    toTransferResult(
+                        prefetchInputFileOrSymlink(metadataProvider, input, priority)));
+
     Flowable<TransferResult> transfers = Flowable.merge(treeDownloads, fileDownloads);
     Completable prefetch = mergeBulkTransfer(transfers).onErrorResumeNext(this::onErrorResumeNext);
     return toListenableFuture(prefetch);
   }
 
-  private Completable prefetchInputTree(
+  private Completable prefetchInputTreeOrSymlink(
       MetadataProvider provider,
       SpecialArtifact tree,
       List<TreeFileArtifact> treeFiles,
+      Priority priority)
+      throws IOException {
+
+    PathFragment execPath = tree.getExecPath();
+
+    TreeArtifactValue treeMetadata = provider.getTreeMetadata(tree);
+    if (treeMetadata == null) {
+      return Completable.complete();
+    }
+
+    PathFragment prefetchExecPath = treeMetadata.getSymlinkTargetExecPath().orElse(execPath);
+
+    Completable prefetch = prefetchInputTree(provider, prefetchExecPath, treeFiles, priority);
+
+    // If prefetching to a different path, plant a symlink into it.
+    if (!prefetchExecPath.equals(execPath)) {
+      Completable prefetchAndSymlink =
+          prefetch.doOnComplete(() -> createSymlink(execPath, prefetchExecPath));
+      return downloadCache.executeIfNot(execRoot.getRelative(execPath), prefetchAndSymlink);
+    }
+
+    return prefetch;
+  }
+
+  private Completable prefetchInputTree(
+      MetadataProvider provider,
+      PathFragment execPath,
+      List<TreeFileArtifact> treeFiles,
       Priority priority) {
-    Path treeRoot = execRoot.getRelative(tree.getExecPath());
+    Path treeRoot = execRoot.getRelative(execPath);
     HashMap<TreeFileArtifact, Path> treeFileTmpPathMap = new HashMap<>();
 
     Flowable<TransferResult> transfers =
@@ -182,10 +218,15 @@ private Completable prefetchInputTree(
                 () -> {
                   HashSet<Path> dirs = new HashSet<>();
 
-                  // Tree root is created by Bazel before action execution, but the permission is
-                  // changed to 0555 afterwards. We need to set it as writable in order to move
-                  // files into it.
-                  treeRoot.setWritable(true);
+                  if (!treeRoot.exists()) {
+                    // If the tree root doesn't already exist, create it now. This can happen if
+                    // we're prefetching to a different location and symlinking into it.
+                    treeRoot.createDirectory();
+                  } else {
+                    // If the tree root already exists, make it writable, so we can move files into
+                    // it.
+                    treeRoot.setWritable(true);
+                  }
                   dirs.add(treeRoot);
 
                   for (Map.Entry<TreeFileArtifact, Path> entry : treeFileTmpPathMap.entrySet()) {
@@ -230,20 +271,34 @@ private Completable prefetchInputTree(
     return downloadCache.executeIfNot(treeRoot, download);
   }
 
-  private Completable prefetchInputFile(
+  private Completable prefetchInputFileOrSymlink(
       MetadataProvider metadataProvider, ActionInput input, Priority priority) throws IOException {
     if (input instanceof VirtualActionInput) {
       prefetchVirtualActionInput((VirtualActionInput) input);
       return Completable.complete();
     }
 
     FileArtifactValue metadata = metadataProvider.getMetadata(input);
-    if (metadata == null) {
+    if (metadata == null || !metadata.isRemote()) {
       return Completable.complete();
     }
 
-    Path path = execRoot.getRelative(input.getExecPath());
-    return downloadFileRx(path, metadata, priority);
+    PathFragment execPath = input.getExecPath();
+
+    PathFragment prefetchExecPath =
+        ((RemoteFileArtifactValue) metadata).getSymlinkTargetExecPath().orElse(execPath);
+
+    Completable prefetch =
+        downloadFileRx(execRoot.getRelative(prefetchExecPath), metadata, priority);
+
+    // If prefetching to a different path, plant a symlink into it.
+    if (!prefetchExecPath.equals(execPath)) {
+      Completable prefetchAndSymlink =
+          prefetch.doOnComplete(() -> createSymlink(execPath, prefetchExecPath));
+      return downloadCache.executeIfNot(execRoot.getRelative(execPath), prefetchAndSymlink);
+    }
+
+    return prefetch;
   }
 
   /**
@@ -327,6 +382,15 @@ private void deletePartialDownload(Path path) {
     }
   }
 
+  private void createSymlink(PathFragment linkPath, PathFragment targetPath) throws IOException {
+    Path link = execRoot.getRelative(linkPath);
+    Path target = execRoot.getRelative(targetPath);
+    // Delete the link path if it already exists.
+    // This will happen for output directories, which get created before the action runs.
+    link.delete();
+    link.createSymbolicLink(target);
+  }
+
   public ImmutableSet<Path> downloadedFiles() {
     return downloadCache.getFinishedTasks();
   }