Fail gracefully on conflicting actions generated by an aspect. These can come from Skylark, so we shouldn't crash. As a safety measure, subclasses of ActionLookupValue are now responsible for detecting action conflicts themselves.

PiperOrigin-RevId: 187095271

7 files changed, 45 insertions, 23 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java
index e2ddd4f55f..f6c5961941 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionLookupValueTest.java
@@ -20,6 +20,7 @@ import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -47,7 +48,7 @@ public class ActionLookupValueTest {
     Artifact artifact = mock(Artifact.class);
     when(action.getOutputs()).thenReturn(ImmutableSet.of(artifact));
     when(action.canRemoveAfterExecution()).thenReturn(true);
-    ActionLookupValue underTest = new ActionLookupValue(actionKeyContext, action, false);
+    ActionLookupValue underTest = new ActionLookupValue(action, false);
     assertThat(underTest.getGeneratingActionIndex(artifact)).isEqualTo(0);
     assertThat(underTest.getAction(0)).isSameAs(action);
     underTest.actionEvaluated(0, action);
@@ -55,7 +56,7 @@ public class ActionLookupValueTest {
   }
 
   @Test
-  public void testActionNotPresentAfterEvaluation() {
+  public void testActionNotPresentAfterEvaluation() throws ActionConflictException {
     Path execRoot = fs.getPath("/execroot");
     Path outputRootPath = execRoot.getRelative("blaze-out");
     ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, outputRootPath);
@@ -69,7 +70,9 @@ public class ActionLookupValueTest {
     when(persistentAction.canRemoveAfterExecution()).thenReturn(false);
     ActionLookupValue underTest =
         new ActionLookupValue(
-            actionKeyContext, ImmutableList.of(normalAction, persistentAction), true);
+            Actions.filterSharedActionsAndThrowActionConflict(
+                actionKeyContext, ImmutableList.of(normalAction, persistentAction)),
+            true);
     assertThat(underTest.getGeneratingActionIndex(normalArtifact)).isEqualTo(0);
     assertThat(underTest.getAction(0)).isSameAs(normalAction);
     assertThat(underTest.getGeneratingActionIndex(persistentOutput)).isEqualTo(1);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
index dba65ebbdc..ba1a3c2e3b 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -196,9 +196,7 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas
       ConfiguredTargetKey.of(Label.parseAbsoluteUnchecked("//foo:foo"), null);
 
   private List<Action> evaluate(SpawnActionTemplate spawnActionTemplate) throws Exception {
-    ConfiguredTargetValue ctValue =
-        createConfiguredTargetValue(
-            spawnActionTemplate.getOutputTreeArtifact(), spawnActionTemplate);
+    ConfiguredTargetValue ctValue = createConfiguredTargetValue(spawnActionTemplate);
 
     differencer.inject(CTKEY, ctValue);
     ActionTemplateExpansionKey templateKey = ActionTemplateExpansionValue.key(CTKEY, 0);
@@ -220,11 +218,10 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas
   }
 
   private static ConfiguredTargetValue createConfiguredTargetValue(
-      Artifact artifact, ActionTemplate<?> actionTemplate) {
+      ActionTemplate<?> actionTemplate) {
     return new ConfiguredTargetValue(
         Mockito.mock(ConfiguredTarget.class),
-        new Actions.GeneratingActions(
-            ImmutableList.of(actionTemplate), ImmutableMap.of(artifact, 0)),
+        Actions.GeneratingActions.fromSingleAction(actionTemplate),
         NestedSetBuilder.<Package>stableOrder().build(),
         /*removeActionsAfterEvaluation=*/ false);
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 0a8c460803..8a4f3af2e1 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -22,17 +22,18 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions;
 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.ArtifactRoot;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
 import com.google.devtools.build.lib.events.NullEventHandler;
@@ -318,18 +319,20 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
     return result.get(key);
   }
 
-  private void setGeneratingActions() throws InterruptedException {
+  private void setGeneratingActions() throws InterruptedException, ActionConflictException {
     if (evaluator.getExistingValue(ALL_OWNER) == null) {
       differencer.inject(
           ImmutableMap.of(
               ALL_OWNER,
               new ActionLookupValue(
-                  actionKeyContext, ImmutableList.<ActionAnalysisMetadata>copyOf(actions), false)));
+                  Actions.filterSharedActionsAndThrowActionConflict(
+                      actionKeyContext, ImmutableList.copyOf(actions)),
+                  false)));
     }
   }
 
   private <E extends SkyValue> EvaluationResult<E> evaluate(SkyKey... keys)
-      throws InterruptedException {
+      throws InterruptedException, ActionConflictException {
     setGeneratingActions();
     return driver.evaluate(
         Arrays.asList(keys),
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index 278f1d01af..d2aca56fe9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -38,10 +38,12 @@ import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.ActionResult;
+import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.BuildFailedException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.TestExecException;
@@ -233,12 +235,15 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
     PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
 
     return new Builder() {
-      private void setGeneratingActions() {
+      private void setGeneratingActions() throws ActionConflictException {
         if (evaluator.getExistingValue(ACTION_LOOKUP_KEY) == null) {
           differencer.inject(
               ImmutableMap.of(
                   ACTION_LOOKUP_KEY,
-                  new ActionLookupValue(actionKeyContext, ImmutableList.copyOf(actions), false)));
+                  new ActionLookupValue(
+                      Actions.filterSharedActionsAndThrowActionConflict(
+                          actionKeyContext, ImmutableList.copyOf(actions)),
+                      false)));
         }
       }
 
@@ -274,7 +279,11 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
           keys.add(ArtifactSkyKey.key(artifact, true));
         }
 
-        setGeneratingActions();
+        try {
+          setGeneratingActions();
+        } catch (ActionConflictException e) {
+          throw new IllegalStateException(e);
+        }
         EvaluationResult<SkyValue> result = driver.evaluate(keys, keepGoing, threadCount, reporter);
 
         if (result.hasError()) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
index 4f8b3bc4a0..0194819122 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
@@ -22,7 +22,7 @@ import static org.mockito.Mockito.when;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.testing.EqualsTester;
-import com.google.devtools.build.lib.actions.Actions;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -48,7 +48,7 @@ public class ToolchainResolutionFunctionTest extends ToolchainTestCase {
       ConfiguredTarget configuredTarget) {
     return new ConfiguredTargetValue(
         configuredTarget,
-        new Actions.GeneratingActions(ImmutableList.of(), ImmutableMap.of()),
+        GeneratingActions.EMPTY,
         NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         /*removeActionsAfterEvaluation=*/ false);
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index 411fef60fb..b15782c030 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -34,12 +34,14 @@ import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionResult;
+import com.google.devtools.build.lib.actions.Actions;
 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.ArtifactRoot;
 import com.google.devtools.build.lib.actions.BuildFailedException;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.actions.util.TestAction;
@@ -1232,7 +1234,12 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
 
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env) {
-      return new ActionTemplateExpansionValue(actionKeyContext, actions, false);
+      try {
+        return new ActionTemplateExpansionValue(
+            Actions.filterSharedActionsAndThrowActionConflict(actionKeyContext, actions), false);
+      } catch (ActionConflictException e) {
+        throw new IllegalStateException(e);
+      }
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index aaadc2295a..7a804e44d2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -24,16 +24,17 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions;
 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.ArtifactRoot;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.cache.DigestUtils;
 import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
@@ -222,18 +223,20 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase {
     return result.get(key);
   }
 
-  private void setGeneratingActions() throws InterruptedException {
+  private void setGeneratingActions() throws InterruptedException, ActionConflictException {
     if (evaluator.getExistingValue(ALL_OWNER) == null) {
       differencer.inject(
           ImmutableMap.of(
               ALL_OWNER,
               new ActionLookupValue(
-                  actionKeyContext, ImmutableList.<ActionAnalysisMetadata>copyOf(actions), false)));
+                  Actions.filterSharedActionsAndThrowActionConflict(
+                      actionKeyContext, ImmutableList.copyOf(actions)),
+                  false)));
     }
   }
 
   private <E extends SkyValue> EvaluationResult<E> evaluate(SkyKey... keys)
-      throws InterruptedException {
+      throws InterruptedException, ActionConflictException {
     setGeneratingActions();
     return driver.evaluate(
         Arrays.asList(keys), /*keepGoing=*/