Make ActionMetadataHandler stricter

Don't store metadata for output artifacts that are not declared outputs
of the action.

This solves the issue with a625561.

The ActionExecutionFunction uses a per-build cache for the entire build,
and a per-action cache for each action. It runs each action with the
DelegatingPairFileCache, which first asks the the per-action cache, and then
the per-build cache for data.

When we do action input discovery (aka include scanning), the per-action
cache does not contain input header files, including input header files
that are outputs of dependent actions.

The (Google-internal) remote execution plugin first asks the cache for
metadata for each input file to construct the remote execution request.
The initial request may fail if the remote system does not have one or
more of the input files. In that case, it reports the metadata of the
missing input files, _but not the names_. The plugin then looks up the
name from the metadata from a local hash map. However, it requires the
ActionInput object in order to be able to upload the file. In order to
find the ActionInput, it asks the cache for the object using the name.

Before a625561, the PerActionFileCache consulted the
ActionInputMap first to get the metadata and later to resolve the
ActionInput from the name.

After a625561, the ActionMetadataHandler does a more involved
lookup. In particular, if the ActionInput corresponds to an output
Artifact, then it first consults the ActionInputMap, but if the output
is not there, it assumes that the ActionInput is an output of the
current action and stats the file on disk to obtain the metadata.

This would not be a problem, except that the DelegatingPairFileCache
uses the presence of metadata in the ActionInput lookup call to decide
which of the two caches to consult. If the ActionMetadataHandler has
metadata, then the DelegatingPairFileCache also asks the
ActionMetadataHandler for the ActionInput. However, the
ActionMetadataHandler only consults the ActionInputMap to look up the
ActionInput from the file name, which doesn't know about the Artifact
since it's neither an input nor an output of the current action.

By making the ActionMetadataHandler stricter, we avoid this situation -
if it's asked for metadata for an artifact that is neither an input nor
an output of the current action, it simply returns null unless we're in
strict mode, i.e., outside of include scanning. In the latter case, it
throws an exception.

PiperOrigin-RevId: 217531155

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

@@ -619,7 +619,8 @@ public static final class TreeFileArtifact extends Artifact {
      * TreeArtifact. The {@link ArtifactOwner} of the TreeFileArtifact is the {@link ArtifactOwner}
      * of the parent TreeArtifact.
      */
-    TreeFileArtifact(SpecialArtifact parent, PathFragment parentRelativePath) {
+    @VisibleForTesting
+    public TreeFileArtifact(SpecialArtifact parent, PathFragment parentRelativePath) {
       this(parent, parentRelativePath, parent.getArtifactOwner());
     }
 

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -144,17 +144,17 @@ private static FileArtifactValue metadataFromValue(FileArtifactValue value)
 
   @Nullable
   private FileArtifactValue getInputFileArtifactValue(Artifact input) {
-    if (outputs.contains(input)) {
+    if (isKnownOutput(input)) {
       return null;
     }
-
-    if (input.hasParent() && outputs.contains(input.getParent())) {
-      return null;
-    }
-
     return inputArtifactData.getMetadata(input);
   }
 
+  private boolean isKnownOutput(Artifact artifact) {
+    return outputs.contains(artifact)
+        || (artifact.hasParent() && outputs.contains(artifact.getParent()));
+  }
+
   @Override
   public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException {
     // TODO(shahan): is this bypass needed?
@@ -197,6 +197,15 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException
     }
     // Fallthrough: the artifact must be a non-tree, non-middleman output artifact.
 
+    // Don't store metadata for output artifacts that are not declared outputs of the action.
+    if (!isKnownOutput(artifact)) {
+      // Throw in strict mode.
+      if (!missingArtifactsAllowed) {
+        throw new IllegalStateException(String.format("null for %s", artifact));
+      }
+      return null;
+    }
+
     // Check for existing metadata. It may have been injected. In either case, this method is called
     // from SkyframeActionExecutor to make sure that we have metadata for all action outputs, as the
     // results are then stored in Skyframe (and the action cache).

src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java

@@ -21,12 +21,17 @@
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import java.io.FileNotFoundException;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -37,11 +42,13 @@
 public class ActionMetadataHandlerTest {
   private Scratch scratch;
   private ArtifactRoot sourceRoot;
+  private ArtifactRoot outputRoot;
 
   @Before
   public final void setRootDir() throws Exception  {
     scratch = new Scratch();
     sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/workspace")));
+    outputRoot = ArtifactRoot.asDerivedRoot(scratch.dir("/output"), scratch.dir("/output/bin"));
   }
 
   @Test
@@ -112,4 +119,133 @@ public void withUnknownSourceArtifact() throws Exception {
         new MinimalOutputStore());
     assertThat(handler.getMetadata(artifact)).isNull();
   }
+
+  @Test
+  public void withUnknownOutputArtifactMissingAllowed() throws Exception {
+    PathFragment path = PathFragment.create("foo/bar");
+    Artifact artifact = new Artifact(path, outputRoot);
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ true,
+        /* outputs= */ ImmutableList.of(),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    assertThat(handler.getMetadata(artifact)).isNull();
+  }
+
+  @Test
+  public void withUnknownOutputArtifactStatsFile() throws Exception {
+    scratch.file("/output/bin/foo/bar", "not empty");
+    Artifact artifact = new Artifact(PathFragment.create("foo/bar"), outputRoot);
+    assertThat(artifact.getPath().exists()).isTrue();
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ false,
+        /* outputs= */ ImmutableList.of(artifact),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    assertThat(handler.getMetadata(artifact)).isNotNull();
+  }
+
+  @Test
+  public void withUnknownOutputArtifactStatsFileFailsWithException() throws Exception {
+    Artifact artifact = new Artifact(PathFragment.create("foo/bar"), outputRoot);
+    assertThat(artifact.getPath().exists()).isFalse();
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ false,
+        /* outputs= */ ImmutableList.of(artifact),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    try {
+      handler.getMetadata(artifact);
+      fail();
+    } catch (FileNotFoundException expected) {
+    }
+  }
+
+  @Test
+  public void withUnknownOutputArtifactMissingDisallowed() throws Exception {
+    PathFragment path = PathFragment.create("foo/bar");
+    Artifact artifact = new Artifact(path, outputRoot);
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ false,
+        /* outputs= */ ImmutableList.of(),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    try {
+      handler.getMetadata(artifact);
+      fail();
+    } catch (IllegalStateException expected) {
+    }
+  }
+
+  @Test
+  public void withUnknownOutputArtifactMissingAllowedTreeArtifact() throws Exception {
+    PathFragment path = PathFragment.create("bin/foo/bar");
+    SpecialArtifact treeArtifact =
+        new SpecialArtifact(
+            outputRoot, path, ArtifactOwner.NullArtifactOwner.INSTANCE, SpecialArtifactType.TREE);
+    Artifact artifact = new TreeFileArtifact(treeArtifact, PathFragment.create("baz"));
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ true,
+        /* outputs= */ ImmutableList.of(),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    assertThat(handler.getMetadata(artifact)).isNull();
+  }
+
+  @Test
+  public void withUnknownOutputArtifactStatsFileTreeArtifact() throws Exception {
+    scratch.file("/output/bin/foo/bar/baz", "not empty");
+    PathFragment path = PathFragment.create("bin/foo/bar");
+    SpecialArtifact treeArtifact =
+        new SpecialArtifact(
+            outputRoot, path, ArtifactOwner.NullArtifactOwner.INSTANCE, SpecialArtifactType.TREE);
+    Artifact artifact = new TreeFileArtifact(treeArtifact, PathFragment.create("baz"));
+    assertThat(artifact.getPath().exists()).isTrue();
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ false,
+        /* outputs= */ ImmutableList.of(treeArtifact),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    assertThat(handler.getMetadata(artifact)).isNotNull();
+  }
+
+  @Test
+  public void withUnknownOutputArtifactMissingDisallowedTreeArtifact() throws Exception {
+    PathFragment path = PathFragment.create("bin/foo/bar");
+    SpecialArtifact treeArtifact =
+        new SpecialArtifact(
+            outputRoot, path, ArtifactOwner.NullArtifactOwner.INSTANCE, SpecialArtifactType.TREE);
+    Artifact artifact = new TreeFileArtifact(treeArtifact, PathFragment.create("baz"));
+    ActionInputMap map = new ActionInputMap(1);
+    ActionMetadataHandler handler = new ActionMetadataHandler(
+        map,
+        /* missingArtifactsAllowed= */ false,
+        /* outputs= */ ImmutableList.of(),
+        /* tsgm= */ null,
+        ArtifactPathResolver.IDENTITY,
+        new MinimalOutputStore());
+    try {
+      handler.getMetadata(artifact);
+      fail();
+    } catch (IllegalStateException expected) {
+    }
+  }
 }