Always create a ResolvedFileArtifactValue for an artifact materialized as a symlink to another.

This will be useful in upcoming changes extending the concept of lazy materialization to non-remote artifacts.

PiperOrigin-RevId: 723477378
Change-Id: I704f13782c0dbfeb65f9e13e2dbaede745ec2202
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
index c55a589..7afddae 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
@@ -302,10 +302,12 @@
       }
     }
 
-    // Same rationale as for constructFileArtifactValue.
-    if (anyRemote.get()
-        && treeDir.isSymbolicLink()
-        && stat instanceof FileStatusWithMetadata statWithMetadata) {
+    // If the artifact was materialized in the filesystem as as symlink to another artifact, record
+    // the real path in the metadata so that it can be recreated as such later.
+    // See {@link FileArtifactValue#getResolvedPath} for why this is useful.
+    // TODO(tjgq): Actually check whether the path matches one of the action inputs. The presence
+    // of a FileStatusWithMetadata happens to coincide, but seems a little brittle.
+    if (stat instanceof FileStatusWithMetadata statWithMetadata && treeDir.isSymbolicLink()) {
       FileArtifactValue metadata = statWithMetadata.getMetadata();
       PathFragment resolvedPath = metadata.getResolvedPath();
       if (resolvedPath != null) {
@@ -409,21 +411,6 @@
             artifact.isConstantMetadata() ? null : tsgm);
     var value = statAndValue.fileArtifactValue();
 
-    // If the artifact is stored remotely but it was materialized in the filesystem as a symlink,
-    // store the original path in the metadata so it can later be recreated as such to avoid
-    // downloading multiple copies.
-    //
-    // This only affects Bazel when building without the bytes. In Blaze, without an action
-    // filesystem, metadata is always local (isRemote() is false); and with an action filesystem,
-    // createSymbolicLink() is implemented as a file copy (isSymbolicLink() is false).
-    if (value.isRemote()
-        && value.getResolvedPath() == null
-        && statAndValue.statNoFollow().isSymbolicLink()) {
-      value =
-          FileArtifactValue.createFromExistingWithResolvedPath(
-              value, statAndValue.realPath().asFragment());
-    }
-
     // Ensure that we don't have both an injected digest and a digest from the filesystem.
     byte[] fileDigest = value.getDigest();
 
@@ -556,6 +543,18 @@
     FileStatusWithDigest realStatWithDigest = FileStatusWithDigestAdapter.maybeAdapt(realStat);
     var fileArtifactValue =
         fileArtifactValueFromStat(realRootedPath, realStatWithDigest, xattrProvider, tsgm);
+
+    // If the artifact was materialized in the filesystem as as symlink to another artifact, record
+    // the real path in the metadata so that it can be recreated as such later.
+    // See {@link FileArtifactValue#getResolvedPath} for why this is useful.
+    // TODO(tjgq): Actually check whether the path matches one of the action inputs. The presence
+    // of a FileStatusWithMetadata happens to coincide, but seems a little brittle.
+    if (realStat instanceof FileStatusWithMetadata && fileArtifactValue.getResolvedPath() == null) {
+      fileArtifactValue =
+          FileArtifactValue.createFromExistingWithResolvedPath(
+              fileArtifactValue, realRootedPath.asPath().asFragment());
+    }
+
     return FileArtifactStatAndValue.create(pathNoFollow, realPath, statNoFollow, fileArtifactValue);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
index 1cfd0c0..8eaf7d2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.DigestUtils;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -67,27 +68,16 @@
 @RunWith(TestParameterInjector.class)
 public final class ActionOutputMetadataStoreTest {
 
+  private enum ArtifactType {
+    SOURCE,
+    OUTPUT
+  }
+
   private enum ResolvedPathDepth {
     SHALLOW,
     DEEP
   }
 
-  private enum FileLocation {
-    LOCAL,
-    REMOTE
-  }
-
-  private enum TreeComposition {
-    EMPTY,
-    FULLY_LOCAL,
-    FULLY_REMOTE,
-    MIXED;
-
-    boolean isPartiallyRemote() {
-      return this == FULLY_REMOTE || this == MIXED;
-    }
-  }
-
   private final Map<Path, Integer> chmodCalls = Maps.newConcurrentMap();
 
   private final Scratch scratch =
@@ -392,97 +382,63 @@
   }
 
   @Test
-  public void fileArtifactMaterializedAsSymlink(
-      @TestParameter ResolvedPathDepth depth, @TestParameter FileLocation location)
-      throws Exception {
-    Artifact targetArtifact =
-        ActionsTestUtil.createArtifactWithRootRelativePath(
-            outputRoot, PathFragment.create("target"));
-
+  public void fileArtifactMaterializedAsSymlinkToNonArtifact() throws Exception {
+    Path resolvedPath = outputRoot.getRoot().getRelative("resolved");
     Artifact outputArtifact =
         ActionsTestUtil.createArtifactWithRootRelativePath(
             outputRoot, PathFragment.create("output"));
 
-    PathFragment preexistingPath =
-        switch (depth) {
-          case SHALLOW -> null;
-          case DEEP -> outputRoot.getRoot().asPath().getRelative("preexisting").asFragment();
-        };
-
-    FileArtifactValue targetMetadata = createFileMetadataForSymlinkTest(location, preexistingPath);
-    ActionInputMap inputMap = new ActionInputMap(0);
-    inputMap.putWithNoDepOwner(targetArtifact, targetMetadata);
-
     RemoteActionFileSystem actionFs =
-        createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
+        createRemoteActionFileSystem(new ActionInputMap(0), ImmutableSet.of(outputArtifact));
 
     ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
     store.prepareForActionExecution();
 
-    // In a realistic scenario, files with local metadata should also exist on disk.
-    // However, the action filesystem is expected to obtain their metadata from the input map.
     actionFs
-        .getPath(outputArtifact.getPath().getParentDirectory().getPathString())
+        .getPath(outputArtifact.getPath().asFragment())
+        .getParentDirectory()
         .createDirectoryAndParents();
     actionFs
-        .getPath(outputArtifact.getPath().getPathString())
-        .createSymbolicLink(targetArtifact.getPath().asFragment());
-    if (preexistingPath != null) {
-      actionFs
-          .getPath(targetArtifact.getPath().getPathString())
-          .createSymbolicLink(preexistingPath);
-    }
-
-    PathFragment expectedResolvedPath = null;
-    if (location == FileLocation.REMOTE) {
-      expectedResolvedPath =
-          preexistingPath != null ? preexistingPath : targetArtifact.getPath().asFragment();
-    }
+        .getPath(outputArtifact.getPath().asFragment())
+        .createSymbolicLink(resolvedPath.asFragment());
+    FileSystemUtils.writeContentAsLatin1(resolvedPath, "foo");
 
     assertThat(store.getOutputMetadata(outputArtifact))
-        .isEqualTo(createFileMetadataForSymlinkTest(location, expectedResolvedPath));
-  }
-
-  private static FileArtifactValue createFileMetadataForSymlinkTest(
-      FileLocation location, @Nullable PathFragment resolvedPath) {
-    switch (location) {
-      case LOCAL:
-        return FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10);
-      case REMOTE:
-        {
-          FileArtifactValue metadata =
-              FileArtifactValue.createForRemoteFileWithMaterializationData(
-                  new byte[] {1, 2, 3}, 10, 1, null);
-          if (resolvedPath != null) {
-            metadata = FileArtifactValue.createFromExistingWithResolvedPath(metadata, resolvedPath);
-          }
-          return metadata;
-        }
-    }
-    throw new AssertionError();
+        .isEqualTo(FileArtifactValue.createForTesting(resolvedPath));
   }
 
   @Test
-  public void treeArtifactMaterializedAsSymlink(
-      @TestParameter ResolvedPathDepth depth, @TestParameter TreeComposition composition)
+  public void fileArtifactMaterializedAsSymlinkToFileArtifact(
+      @TestParameter ArtifactType artifactType, @TestParameter ResolvedPathDepth depth)
       throws Exception {
-    SpecialArtifact targetArtifact =
-        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "target");
-
-    SpecialArtifact outputArtifact =
-        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
-
-    PathFragment preexistingPath =
-        switch (depth) {
-          case SHALLOW -> null;
-          case DEEP -> outputRoot.getRoot().asPath().getRelative("preexisting").asFragment();
+    ArtifactRoot root =
+        switch (artifactType) {
+          case SOURCE -> sourceRoot;
+          case OUTPUT -> outputRoot;
         };
+    Artifact resolvedArtifact =
+        ActionsTestUtil.createArtifactWithRootRelativePath(root, PathFragment.create("resolved"));
+    Artifact inputArtifact =
+        switch (depth) {
+          case SHALLOW -> resolvedArtifact;
+          case DEEP ->
+              ActionsTestUtil.createArtifactWithRootRelativePath(
+                  outputRoot, PathFragment.create("input"));
+        };
+    Artifact outputArtifact =
+        ActionsTestUtil.createArtifactWithRootRelativePath(
+            outputRoot, PathFragment.create("output"));
 
-    TreeArtifactValue targetMetadata =
-        createTreeMetadataForSymlinkTest(targetArtifact, composition, preexistingPath);
+    FileArtifactValue inputMetadata =
+        FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, null, 123L);
+    if (!resolvedArtifact.equals(inputArtifact)) {
+      inputMetadata =
+          FileArtifactValue.createFromExistingWithResolvedPath(
+              inputMetadata, resolvedArtifact.getPath().asFragment());
+    }
 
     ActionInputMap inputMap = new ActionInputMap(0);
-    inputMap.putTreeArtifact(targetArtifact, targetMetadata, /* depOwner= */ null);
+    inputMap.putWithNoDepOwner(inputArtifact, inputMetadata);
 
     RemoteActionFileSystem actionFs =
         createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
@@ -490,66 +446,101 @@
     ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
     store.prepareForActionExecution();
 
-    // In a realistic scenario, files with local metadata should also exist on disk.
-    // However, the action filesystem is expected to obtain their metadata from the input map.
     actionFs
-        .getPath(outputArtifact.getPath().getParentDirectory().getPathString())
+        .getPath(outputArtifact.getPath().asFragment())
+        .getParentDirectory()
         .createDirectoryAndParents();
-    actionFs.getPath(targetArtifact.getPath().getPathString()).createDirectoryAndParents();
     actionFs
-        .getPath(outputArtifact.getPath().getPathString())
-        .createSymbolicLink(targetArtifact.getPath().asFragment());
+        .getPath(outputArtifact.getPath().asFragment())
+        .createSymbolicLink(inputArtifact.getPath().asFragment());
 
-    PathFragment expectedResolvedPath = null;
-    if (composition.isPartiallyRemote()) {
-      expectedResolvedPath =
-          preexistingPath != null ? preexistingPath : targetArtifact.getPath().asFragment();
-    }
+    assertThat(store.getOutputMetadata(outputArtifact))
+        .isEqualTo(
+            FileArtifactValue.createFromExistingWithResolvedPath(
+                inputMetadata, resolvedArtifact.getPath().asFragment()));
+  }
+
+  @Test
+  public void treeArtifactMaterializedAsSymlinkToNonArtifact() throws Exception {
+    Path resolvedPath = outputRoot.getRoot().getRelative("resolved");
+    SpecialArtifact outputArtifact =
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
+
+    RemoteActionFileSystem actionFs =
+        createRemoteActionFileSystem(new ActionInputMap(0), ImmutableSet.of(outputArtifact));
+
+    ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
+    store.prepareForActionExecution();
+
+    actionFs
+        .getPath(outputArtifact.getPath().asFragment())
+        .getParentDirectory()
+        .createDirectoryAndParents();
+    actionFs
+        .getPath(outputArtifact.getPath().asFragment())
+        .createSymbolicLink(resolvedPath.asFragment());
+    actionFs.getPath(resolvedPath.asFragment()).createDirectoryAndParents();
+    FileSystemUtils.writeContentAsLatin1(resolvedPath.getRelative("child"), "foo");
 
     assertThat(store.getTreeArtifactValue(outputArtifact))
         .isEqualTo(
-            createTreeMetadataForSymlinkTest(outputArtifact, composition, expectedResolvedPath));
+            TreeArtifactValue.newBuilder(outputArtifact)
+                .putChild(
+                    TreeFileArtifact.createTreeOutput(outputArtifact, "child"),
+                    FileArtifactValue.createForTesting(resolvedPath.getRelative("child")))
+                .build());
   }
 
-  private static TreeArtifactValue createTreeMetadataForSymlinkTest(
-      SpecialArtifact parent, TreeComposition composition, @Nullable PathFragment resolvedPath) {
-    TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent);
+  @Test
+  public void treeArtifactMaterializedAsSymlinkToAnotherTreeArtifact(
+      @TestParameter ResolvedPathDepth depth) throws Exception {
+    SpecialArtifact resolvedArtifact =
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "resolved");
+    SpecialArtifact inputArtifact =
+        switch (depth) {
+          case SHALLOW -> resolvedArtifact;
+          case DEEP -> ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "input");
+        };
+    SpecialArtifact outputArtifact =
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
 
-    TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
-    TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2");
-
-    FileArtifactValue localMetadata1 =
-        FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10);
-    FileArtifactValue localMetadata2 =
-        FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 20);
-
-    FileArtifactValue remoteMetadata1 =
-        FileArtifactValue.createForRemoteFile(new byte[] {1, 2, 3}, 10, 1);
-    FileArtifactValue remoteMetadata2 =
-        FileArtifactValue.createForRemoteFile(new byte[] {4, 5, 6}, 20, 1);
-
-    switch (composition) {
-      case EMPTY:
-        break;
-      case FULLY_LOCAL:
-        builder.putChild(child1, localMetadata1);
-        builder.putChild(child2, localMetadata2);
-        break;
-      case FULLY_REMOTE:
-        builder.putChild(child1, remoteMetadata1);
-        builder.putChild(child2, remoteMetadata2);
-        break;
-      case MIXED:
-        builder.putChild(child1, localMetadata1);
-        builder.putChild(child2, remoteMetadata2);
-        break;
+    TreeArtifactValue.Builder builder =
+        TreeArtifactValue.newBuilder(inputArtifact)
+            .putChild(
+                TreeFileArtifact.createTreeOutput(inputArtifact, "child"),
+                FileArtifactValue.createForNormalFile(
+                    new byte[] {1, 2, 3}, /* proxy= */ null, 123L));
+    if (depth == ResolvedPathDepth.DEEP) {
+      builder.setResolvedPath(resolvedArtifact.getPath().asFragment());
     }
+    TreeArtifactValue inputMetadata = builder.build();
 
-    if (resolvedPath != null) {
-      builder.setResolvedPath(resolvedPath);
-    }
+    ActionInputMap inputMap = new ActionInputMap(0);
+    inputMap.putTreeArtifact(inputArtifact, inputMetadata, /* depOwner= */ null);
 
-    return builder.build();
+    RemoteActionFileSystem actionFs =
+        createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
+
+    ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
+    store.prepareForActionExecution();
+
+    actionFs
+        .getPath(outputArtifact.getPath().asFragment())
+        .getParentDirectory()
+        .createDirectoryAndParents();
+    actionFs
+        .getPath(outputArtifact.getPath().asFragment())
+        .createSymbolicLink(inputArtifact.getPath().asFragment());
+
+    assertThat(store.getTreeArtifactValue(outputArtifact))
+        .isEqualTo(
+            TreeArtifactValue.newBuilder(outputArtifact)
+                .putChild(
+                    TreeFileArtifact.createTreeOutput(outputArtifact, "child"),
+                    FileArtifactValue.createForNormalFile(
+                        new byte[] {1, 2, 3}, /* proxy= */ null, 123L))
+                .setResolvedPath(resolvedArtifact.getPath().asFragment())
+                .build());
   }
 
   @Test