Make ActionMetadataHandler stricter
Don't store metadata for output artifacts that are not declared outputs
of the action.
This solves the issue with https://github.com/bazelbuild/bazel/commit/a6255612e4892729d3758775c76085b26b9bc584.
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 https://github.com/bazelbuild/bazel/commit/a6255612e4892729d3758775c76085b26b9bc584, the PerActionFileCache consulted the
ActionInputMap first to get the metadata and later to resolve the
ActionInput from the name.
After https://github.com/bazelbuild/bazel/commit/a6255612e4892729d3758775c76085b26b9bc584, 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
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index 290ea13..efca8b0 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -619,7 +619,8 @@
* 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());
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index e05fbe1..9ddea36 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -144,17 +144,17 @@
@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 @@
}
// 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).
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
index 8c39997..9c2e5ef 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
+++ b/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 @@
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) {
+ }
+ }
}
