Lazily create local symlinks with BwoB

Unconditionally creating symlinks results in unnecessary I/O and may not even be supported by the host (e.g. on Windows). Instead, only create it in the remote action file system and rely on the prefetcher to materialize it when needed by downstream actions.

Author: fmeum

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

@@ -164,6 +164,14 @@ public boolean isRemote() {
     return false;
   }
 
+  /**
+   * Returns whether the file contents are materialized lazily, for example, because they exist
+   * remotely.
+   */
+  public boolean isLazy() {
+    return isRemote();
+  }
+
   /** Returns the location index for remote files. For non-remote files, returns 0. */
   public int getLocationIndex() {
     return 0;
@@ -311,7 +319,13 @@ public static FileArtifactValue createForSourceArtifact(
 
   public static FileArtifactValue createFromInjectedDigest(
       FileArtifactValue metadata, @Nullable byte[] digest) {
-    return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
+    var normalFileValue =
+        createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
+    if (metadata.getResolvedPath() != null) {
+      return createFromExistingWithResolvedPath(normalFileValue, metadata.getResolvedPath());
+    } else {
+      return normalFileValue;
+    }
   }
 
   @VisibleForTesting
@@ -762,7 +776,6 @@ public void setContentsProxy(FileContentsProxy proxy) {
       this.proxy = proxy;
     }
 
-
     @Override
     public boolean equals(Object o) {
       if (this == o) {
@@ -890,6 +903,11 @@ public boolean isRemote() {
       return delegate.isRemote();
     }
 
+    @Override
+    public boolean isLazy() {
+      return true;
+    }
+
     @Override
     public int getLocationIndex() {
       return delegate.getLocationIndex();
@@ -1024,6 +1042,11 @@ public FileStateType getType() {
       return FileStateType.SYMLINK;
     }
 
+    @Override
+    public boolean isLazy() {
+      return true;
+    }
+
     @Override
     public byte[] getDigest() {
       return digest;

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

@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
@@ -41,6 +40,7 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileContentsProxy;
+import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.cache.OutputMetadataStore;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
@@ -196,15 +196,14 @@ void setOutputPermissions(Path dir) throws IOException {
   private final DirectoryTracker directoryTracker = new DirectoryTracker();
 
   /** A symlink in the output tree. */
-  record Symlink(PathFragment linkExecPath, PathFragment targetExecPath) {
+  record Symlink(Path linkPath, Path targetPath) {
     Symlink {
-      requireNonNull(linkExecPath, "linkExecPath");
-      requireNonNull(targetExecPath, "targetExecPath");
-      checkArgument(!linkExecPath.equals(targetExecPath));
+      requireNonNull(linkPath, "linkPath");
+      requireNonNull(targetPath, "targetPath");
     }
 
-    static Symlink of(PathFragment linkExecPath, PathFragment targetExecPath) {
-      return new Symlink(linkExecPath, targetExecPath);
+    static Symlink of(Path linkPath, Path targetPath) {
+      return new Symlink(linkPath, targetPath);
     }
   }
 
@@ -382,47 +381,56 @@ private ListenableFuture<Void> prefetchFile(
         return immediateVoidFuture();
       }
 
-      PathFragment execPath = input.getExecPath();
+      Path path = execRoot.getRelative(input.getExecPath());
 
       // Metadata may legitimately be missing, e.g. if this is an optional test output.
       FileArtifactValue metadata = metadataSupplier.getMetadata(input);
-      if (metadata == null || !canDownloadFile(execRoot.getRelative(execPath), metadata)) {
+      if (metadata == null || !metadata.isLazy()) {
         return immediateVoidFuture();
       }
 
       @Nullable Symlink symlink = maybeGetSymlink(input, metadata, metadataSupplier);
 
       if (symlink != null) {
-        checkState(execPath.startsWith(symlink.linkExecPath()));
-        execPath =
-            symlink.targetExecPath().getRelative(execPath.relativeTo(symlink.linkExecPath()));
+        checkState(path.startsWith(symlink.linkPath()));
+        path = symlink.targetPath().getRelative(path.relativeTo(symlink.linkPath()));
       }
 
       @Nullable PathFragment treeRootExecPath = maybeGetTreeRoot(input, metadataSupplier);
 
-      Completable result =
-          downloadFileNoCheckRx(
-                  action,
-                  execRoot.getRelative(execPath),
-                  treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null,
-                  dirsWithOutputPermissions,
-                  input,
-                  metadata,
-                  priority,
-                  reason)
-              .onErrorResumeNext(
-                  t -> {
-                    if (t instanceof CacheNotFoundException cacheNotFoundException) {
-                      // Only the symlink itself is guaranteed to be an input to the action, so
-                      // report its path for rewinding.
-                      cacheNotFoundException.setExecPath(input.getExecPath());
-                      return Completable.error(cacheNotFoundException);
-                    }
-                    return Completable.error(t);
-                  });
+      Completable result;
+      if (canDownloadFile(path, metadata)) {
+        result =
+            downloadFileNoCheckRx(
+                    action,
+                    path,
+                    treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null,
+                    dirsWithOutputPermissions,
+                    input,
+                    metadata,
+                    priority,
+                    reason)
+                .onErrorResumeNext(
+                    t -> {
+                      if (t instanceof CacheNotFoundException cacheNotFoundException) {
+                        // Only the symlink itself is guaranteed to be an input to the action, so
+                        // report its path for rewinding.
+                        cacheNotFoundException.setExecPath(input.getExecPath());
+                        return Completable.error(cacheNotFoundException);
+                      }
+                      return Completable.error(t);
+                    });
+      } else if (metadata.getType() == FileStateType.SYMLINK) {
+        result = plantRelativeSymlink(path, metadata.getUnresolvedSymlinkTarget());
+      } else {
+        // This is a symlink to a local file.
+        checkState(metadata.getResolvedPath() != null);
+        checkState(!metadata.isRemote());
+        result = Completable.complete();
+      }
 
       if (symlink != null) {
-        result = result.andThen(plantSymlink(symlink));
+        result = result.andThen(plantAbsoluteSymlink(symlink));
       }
 
       return toListenableFuture(result);
@@ -474,9 +482,7 @@ private PathFragment maybeGetTreeRoot(ActionInput input, MetadataSupplier metada
    */
   @Nullable
   private Symlink maybeGetSymlink(
-      ActionInput input,
-      FileArtifactValue metadata,
-      MetadataSupplier metadataSupplier)
+      ActionInput input, FileArtifactValue metadata, MetadataSupplier metadataSupplier)
       throws IOException, InterruptedException {
     if (input instanceof TreeFileArtifact treeFile) {
       SpecialArtifact treeArtifact = treeFile.getParent();
@@ -493,13 +499,13 @@ private Symlink maybeGetSymlink(
       }
       return maybeGetSymlink(treeArtifact, treeMetadata, metadataSupplier);
     }
-    PathFragment execPath = input.getExecPath();
-    PathFragment resolvedExecPath = execPath;
+    Path path = execRoot.getRelative(input.getExecPath());
+    Path resolvedPath = path;
     if (metadata.getResolvedPath() != null) {
-      resolvedExecPath = metadata.getResolvedPath().relativeTo(execRoot.asFragment());
+      resolvedPath = execRoot.getRelative(metadata.getResolvedPath());
     }
-    if (!resolvedExecPath.equals(execPath)) {
-      return Symlink.of(execPath, resolvedExecPath);
+    if (!resolvedPath.equals(path)) {
+      return Symlink.of(path, resolvedPath);
     }
     return null;
   }
@@ -683,17 +689,27 @@ private static void deletePartialDownload(Path path) {
     }
   }
 
-  private Completable plantSymlink(Symlink symlink) {
+  private Completable plantRelativeSymlink(Path linkPath, String target) {
+    return downloadCache.executeIfNot(
+        linkPath,
+        Completable.fromAction(
+            () -> {
+              // Delete the link path if it already exists. This is the case for tree artifacts,
+              // whose root directory is created before the action runs.
+              linkPath.delete();
+              linkPath.createSymbolicLink(PathFragment.create(target));
+            }));
+  }
+
+  private Completable plantAbsoluteSymlink(Symlink symlink) {
     return downloadCache.executeIfNot(
-        execRoot.getRelative(symlink.linkExecPath()),
+        symlink.linkPath(),
         Completable.defer(
             () -> {
-              Path link = execRoot.getRelative(symlink.linkExecPath());
-              Path target = execRoot.getRelative(symlink.targetExecPath());
               // Delete the link path if it already exists. This is the case for tree artifacts,
               // whose root directory is created before the action runs.
-              link.delete();
-              link.createSymbolicLink(target);
+              symlink.linkPath().delete();
+              symlink.linkPath().createSymbolicLink(symlink.targetPath());
               return Completable.complete();
             }));
   }
@@ -732,7 +748,7 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor
       }
 
       var metadata = outputMetadataStore.getOutputMetadata(output);
-      if (!metadata.isRemote()) {
+      if (!metadata.isLazy()) {
         continue;
       }
 

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

@@ -283,14 +283,17 @@ protected FileSystem getLocalFileSystem() {
 
   /** Returns whether a path is stored remotely. Follows symlinks. */
   boolean isRemote(Path path) throws IOException {
-    return isRemote(path.asFragment());
+    // Files in the local filesystem are non-remote by definition, so stat only in-memory sources.
+    var status = statInternal(path.asFragment(), FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY);
+    return status instanceof FileStatusWithMetadata fileStatusWithMetadata
+        && fileStatusWithMetadata.getMetadata().isRemote();
   }
 
-  private boolean isRemote(PathFragment path) throws IOException {
-    // Files in the local filesystem are non-remote by definition, so stat only in-memory sources.
+  private boolean isLazy(PathFragment path) throws IOException {
+    // Files in the local filesystem are non-lazy by definition, so stat only in-memory sources.
     var status = statInternal(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY);
     return status instanceof FileStatusWithMetadata fileStatusWithMetadata
-        && fileStatusWithMetadata.getMetadata().isRemote();
+        && fileStatusWithMetadata.getMetadata().isLazy();
   }
 
   public void updateContext(ActionExecutionMetadata action) {
@@ -368,7 +371,7 @@ protected boolean delete(PathFragment path) throws IOException {
   @Override
   protected InputStream getInputStream(PathFragment path) throws IOException {
     try {
-      downloadIfRemote(path);
+      materializeIfLazy(path);
     } catch (BulkTransferException e) {
       var newlyLostInputs = e.getLostArtifacts(inputArtifactData::getInput);
       if (!newlyLostInputs.isEmpty()) {
@@ -379,8 +382,8 @@ protected InputStream getInputStream(PathFragment path) throws IOException {
     return localFs.getPath(path).getInputStream();
   }
 
-  private void downloadIfRemote(PathFragment path) throws IOException {
-    if (!isRemote(path)) {
+  private void materializeIfLazy(PathFragment path) throws IOException {
+    if (!isLazy(path)) {
       return;
     }
     PathFragment execPath = path.relativeTo(execRoot);
@@ -592,9 +595,9 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag
 
     if (isOutput(linkPath)) {
       remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment);
+    } else {
+      localFs.getPath(linkPath).createSymbolicLink(targetFragment);
     }
-
-    localFs.getPath(linkPath).createSymbolicLink(targetFragment);
   }
 
   @Override

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

@@ -1005,26 +1005,36 @@ private void moveOutputsToFinalLocation(
     }
   }
 
-  private void createSymlinks(Iterable<SymlinkMetadata> symlinks) throws IOException {
+  private void createSymlinks(
+      RemoteActionResult result,
+      @Nullable RemoteActionFileSystem remoteActionFileSystem,
+      Iterable<SymlinkMetadata> symlinks,
+      @Nullable PathFragment treeRootExecPath)
+      throws IOException {
     for (SymlinkMetadata symlink : symlinks) {
-      Preconditions.checkNotNull(
-              symlink.path().getParentDirectory(),
-              "Failed creating directory and parents for %s",
-              symlink.path())
-          .createDirectoryAndParents();
-      // If a directory output is being materialized as a symlink, creating the symlink fails as we
-      // must first delete the preexisting empty directory. Since this is rare (and in the future
-      // BwoB may no longer eagerly create these directories), we don't delete the directory
-      // beforehand.
-      try {
-        symlink.path().createSymbolicLink(symlink.target());
-      } catch (IOException e) {
-        if (!symlink.path().isDirectory(Symlinks.NOFOLLOW)) {
-          throw e;
+      if (shouldDownload(result, symlink.path().relativeTo(execRoot), treeRootExecPath)) {
+        Preconditions.checkNotNull(
+                symlink.path().getParentDirectory(),
+                "Failed creating directory and parents for %s",
+                symlink.path())
+            .createDirectoryAndParents();
+        // If a directory output is being materialized as a symlink, creating the symlink fails as
+        // we must first delete the preexisting empty directory. Since this is rare (and in the
+        // future BwoB may no longer eagerly create these directories), we don't delete the
+        // directory beforehand.
+        try {
+          symlink.path().createSymbolicLink(symlink.target());
+        } catch (IOException e) {
+          if (!symlink.path().isDirectory(Symlinks.NOFOLLOW)) {
+            throw e;
+          }
+          // Retry after deleting the directory.
+          symlink.path().delete();
+          symlink.path().createSymbolicLink(symlink.target());
         }
-        // Retry after deleting the directory.
-        symlink.path().delete();
-        symlink.path().createSymbolicLink(symlink.target());
+      } else if (!(outputService instanceof BazelOutputService)) {
+        checkNotNull(remoteActionFileSystem)
+            .createSymbolicLink(symlink.path().asFragment(), symlink.target());
       }
     }
   }
@@ -1461,19 +1471,19 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
           Iterables.transform(finishedDownloads, FileMetadata::path), realToTmpPath);
     }
 
-    List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
-    for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
-      for (SymlinkMetadata symlink : entry.getValue().symlinks()) {
-        symlinksInDirectories.add(symlink);
-      }
-    }
-
-    Iterable<SymlinkMetadata> symlinks =
-        Iterables.concat(metadata.symlinks(), symlinksInDirectories);
-
     // Create the symbolic links after all downloads are finished, because dangling symlinks
     // might not be supported on all platforms.
-    createSymlinks(symlinks);
+    for (var symlink : metadata.symlinks()) {
+      createSymlinks(
+          result, remoteActionFileSystem, ImmutableList.of(symlink), /* treeRootExecPath= */ null);
+    }
+    for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
+      createSymlinks(
+          result,
+          remoteActionFileSystem,
+          entry.getValue().symlinks(),
+          entry.getKey().relativeTo(execRoot));
+    }
 
     if (result.success()) {
       // Check that all mandatory outputs are created.