Handle deserialized ActionLookupValues without actions in more places.

* `SkyframeBuildView`: this is counting actions, which is safe to skip.
* `ArtifactConflictFinder`: this is potentially problematic for builds that opt into analysis caching, because one can introduce a new action that causes an artifact conflict with an action in the pruned analysis subgraph. However, any build later that doesn't use analysis caching will trigger a conflict check with the full graph. This includes the cache writer build.

Also add a test to verify that deserialized aspects don't have actions (129c89d made them transient).

PiperOrigin-RevId: 694066104
Change-Id: I387e5368d132d082f3417b1c9c9253cf6a9f887c

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

@@ -15,11 +15,17 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.skyframe.SkyValue;
+import javax.annotation.Nullable;
 
 /** Base interface for all values which can provide the generating action of an artifact. */
 public interface ActionLookupValue extends SkyValue {
 
-  /** Returns a list of actions registered by this {@link SkyValue}. */
+  /**
+   * Returns a list of actions registered by this {@link SkyValue}.
+   *
+   * <p>Null if this value was deserialized with analysis caching.
+   */
+  @Nullable
   ImmutableList<ActionAnalysisMetadata> getActions();
 
   /** Returns the {@link Action} with index {@code index} in this value. Never null. */
@@ -42,8 +48,15 @@ default ActionTemplate<?> getActionTemplate(int index) {
     return actionTemplate;
   }
 
-  /** Returns the number of {@link Action} objects present in this value. */
+  /**
+   * Returns the number of {@link Action} objects present in this value.
+   *
+   * <p>There are no actions for deserialized values.
+   */
   default int getNumActions() {
+    if (getActions() == null) {
+      return 0;
+    }
     return getActions().size();
   }
 }

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

@@ -13,17 +13,20 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.MoreObjects.ToStringHelper;
 import com.google.common.collect.ImmutableList;
+import javax.annotation.Nullable;
 
 /**
  * Basic implementation of {@link ActionLookupValue} where the value itself owns and maintains the
  * list of generating actions.
  */
 public class BasicActionLookupValue implements ActionLookupValue {
-  protected final transient ImmutableList<ActionAnalysisMetadata> actions;
+  @Nullable protected final transient ImmutableList<ActionAnalysisMetadata> actions;
 
   @VisibleForTesting
   public BasicActionLookupValue(ImmutableList<ActionAnalysisMetadata> actions) {
@@ -32,7 +35,7 @@ public BasicActionLookupValue(ImmutableList<ActionAnalysisMetadata> actions) {
 
   @Override
   public ImmutableList<ActionAnalysisMetadata> getActions() {
-    return actions;
+    return checkNotNull(actions, "actions are not available on deserialized instances");
   }
 
   protected ToStringHelper getStringHelper() {

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

@@ -119,6 +119,15 @@ private static void actionRegistration(
     List<Artifact> myArtifacts = new ArrayList<>(values.size());
 
     for (ActionLookupValue value : values) {
+      if (value.getActions() == null) {
+        // For remote analysis caching enabled builds with cache hits, deserialized
+        // ActionLookupValues do not contain actions.
+        //
+        // The full check will be delayed to the cache writing build, or any build
+        // that doesn't deserialize any remote ActionLookupValues.
+        continue;
+      }
+
       for (ActionAnalysisMetadata action : value.getActions()) {
         try {
           actionGraph.registerAction(action);

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

@@ -1513,6 +1513,11 @@ public void evaluated(
               return;
             }
           }
+          if (alv.getActions() == null) {
+            // No actions in deserialized action lookup values.
+            return;
+          }
+
           // During multithreaded operation, this is only set to true, so no concurrency issues.
           someActionLookupValueEvaluated = true;
           ImmutableList<ActionAnalysisMetadata> actions = alv.getActions();

src/test/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -2200,9 +2200,10 @@ java_test(
 )
 
 java_test(
-    name = "RemoteConfiguredTargetValueCodecTest",
-    srcs = ["RemoteConfiguredTargetValueCodecTest.java"],
+    name = "RemoteActionLookupValueSerializationTest",
+    srcs = ["RemoteActionLookupValueSerializationTest.java"],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
@@ -2218,6 +2219,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/skyframe/util:SkyframeExecutorTestUtils",
+        "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
     ],

src/test/java/com/google/devtools/build/lib/skyframe/RemoteConfiguredTargetValueCodecTest.java → src/test/java/com/google/devtools/build/lib/skyframe/RemoteActionLookupValueSerializationTest.java

@@ -16,13 +16,20 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.eventbus.EventBus;
+import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.analysis.AnalysisResult;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
+import com.google.devtools.build.lib.analysis.util.TestAspects.FileProviderAspect;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.skyframe.serialization.testutils.Dumper;
@@ -36,7 +43,51 @@
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
-public class RemoteConfiguredTargetValueCodecTest extends AnalysisTestCase {
+public class RemoteActionLookupValueSerializationTest extends AnalysisTestCase {
+
+  @Test
+  public void testDeserializedAspect_hasProvidersAndNoActions() throws Exception {
+    FileProviderAspect aspect = new FileProviderAspect();
+    setRulesAndAspectsAvailableInTests(ImmutableList.of(aspect), ImmutableList.of());
+    scratch.file(
+        "a/BUILD",
+        "load('//test_defs:foo_binary.bzl', 'foo_binary')",
+        "foo_binary(name = 'a', srcs = ['a.sh'])");
+
+    AnalysisResult analysisResult =
+        update(new EventBus(), defaultFlags(), ImmutableList.of(aspect.getName()), "//a:a");
+
+    ConfiguredAspect configuredAspect =
+        Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
+    assertThat(configuredAspect.getActions()).isNotEmpty();
+    assertThat(configuredAspect).isInstanceOf(ActionLookupValue.class);
+    assertThat(((ActionLookupValue) configuredAspect).getNumActions()).isAtLeast(1);
+
+    var tester = makeSerializationTester(configuredAspect);
+
+    tester.setVerificationFunction(
+        (in, out) -> {
+          // Assertions on actions.
+          assertThat(out).isInstanceOf(ActionLookupValue.class);
+          assertThat(out).isInstanceOf(ConfiguredAspect.class);
+          ActionLookupValue deserializedAlv = (ActionLookupValue) out;
+          var exception = assertThrows(NullPointerException.class, deserializedAlv::getActions);
+          assertThat(exception)
+              .hasMessageThat()
+              .contains("actions are not available on deserialized instances");
+
+          // Assertions on providers.
+          assertThat(in).isInstanceOf(ConfiguredAspect.class);
+          ConfiguredAspect deserializedAspect = (ConfiguredAspect) out;
+          assertThat(
+                  Dumper.dumpStructureWithEquivalenceReduction(
+                      ((ConfiguredAspect) in).getProviders()))
+              .isEqualTo(
+                  Dumper.dumpStructureWithEquivalenceReduction(deserializedAspect.getProviders()));
+        });
+
+    tester.runTests();
+  }
 
   @Test
   public void ruleConfiguredTargetValue_roundTripsToRemoteConfiguredTargetValue() throws Exception {
@@ -52,24 +103,13 @@ public void ruleConfiguredTargetValue_roundTripsToRemoteConfiguredTargetValue()
         SkyframeExecutorTestUtils.getExistingConfiguredTargetValue(
             skyframeExecutor, Label.parseCanonical("//a:a"), config);
 
-    var tester =
-        new SerializationTester(ctValue)
-            .makeMemoizingAndAllowFutureBlocking(true)
-            .addDependency(RuleClassProvider.class, ruleClassProvider)
-            .addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST)
-            .addDependency(FileSystem.class, scratch.getFileSystem())
-            .addDependency(
-                Root.RootCodecDependencies.class,
-                new Root.RootCodecDependencies(Root.absoluteRoot(scratch.getFileSystem())))
-            .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache())
-            .addDependency(PrerequisitePackageFunction.class, skyframeExecutor::getExistingPackage);
-
-    tester.addCodec(RemoteConfiguredTargetValue.codec());
+    var tester = makeSerializationTester(ctValue);
 
     tester.setVerificationFunction(
         (in, out) -> {
           assertThat(in).isInstanceOf(RuleConfiguredTargetValue.class);
           assertThat(out).isInstanceOf(RemoteConfiguredTargetValue.class);
+          assertThat(out).isNotInstanceOf(ActionLookupValue.class);
 
           RemoteConfiguredTargetValue remoteValue = (RemoteConfiguredTargetValue) out;
           RuleConfiguredTarget ruleTarget = ((RuleConfiguredTargetValue) in).getConfiguredTarget();
@@ -120,19 +160,7 @@ public void nonRuleConfiguredTargetValue_roundTripsToRemoteConfiguredTargetValue
             Label.parseCanonical("//a:a.generated"),
             getConfiguration(getConfiguredTarget("//a:a.generated")));
 
-    var tester =
-        new SerializationTester(inputCtValue, outputCtValue)
-            .makeMemoizingAndAllowFutureBlocking(true)
-            .addDependency(RuleClassProvider.class, ruleClassProvider)
-            .addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST)
-            .addDependency(FileSystem.class, scratch.getFileSystem())
-            .addDependency(
-                Root.RootCodecDependencies.class,
-                new Root.RootCodecDependencies(Root.absoluteRoot(scratch.getFileSystem())))
-            .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache())
-            .addDependency(PrerequisitePackageFunction.class, skyframeExecutor::getExistingPackage);
-
-    tester.addCodec(RemoteConfiguredTargetValue.codec());
+    var tester = makeSerializationTester(inputCtValue, outputCtValue);
 
     tester.setVerificationFunction(
         (in, out) -> {
@@ -152,4 +180,18 @@ public void nonRuleConfiguredTargetValue_roundTripsToRemoteConfiguredTargetValue
 
     tester.runTests();
   }
+
+  private SerializationTester makeSerializationTester(Object... subjects) {
+    return new SerializationTester(subjects)
+        .makeMemoizingAndAllowFutureBlocking(true)
+        .addDependency(RuleClassProvider.class, ruleClassProvider)
+        .addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST)
+        .addDependency(FileSystem.class, scratch.getFileSystem())
+        .addDependency(
+            Root.RootCodecDependencies.class,
+            new Root.RootCodecDependencies(Root.absoluteRoot(scratch.getFileSystem())))
+        .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache())
+        .addDependency(PrerequisitePackageFunction.class, skyframeExecutor::getExistingPackage)
+        .addCodec(RemoteConfiguredTargetValue.codec());
+  }
 }