Ignore unevaluated dirtied/changed nodes in FrontierSerializer.
These nodes are effectively disconnected from the graph for the purposes of serializing the frontier deps of a particular build. Because they're invalidated but not evaluated, by definition, their values are not necessary to produce the top level Skyframe value of the current build, and therefore not necessary to be serialized.
PiperOrigin-RevId: 688919651
Change-Id: I365d28b0472890aa8a87a26fa191abf15c998673
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java
index 068dc7b..5a6f46d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java
@@ -268,6 +268,17 @@
}
InMemoryNodeEntry node = checkNotNull(graph.getIfPresent(root), root);
+ if (!node.isDone()) {
+ // This node was marked dirty or changed in the most recent build, but its value was not
+ // necessary by any node in that evaluation, so it was never evaluated. Because this node was
+ // never evaluated, it doesn't need to be added to the active set -- it is essentially
+ // disconnected from the current graph evaluation.
+ //
+ // However, this node's direct deps may still be frontier candidates, but only if they are
+ // reachable from another active node, and candidate selection will be handled by them.
+ return;
+ }
+
for (SkyKey dep : node.getDirectDeps()) {
if (!(dep instanceof ActionLookupKey child)) {
continue;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java
index d115dc9..edee016 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java
@@ -492,6 +492,48 @@
assertThat(graph.getIfPresent(ctKeyOfLabel.apply("//bar:I"))).isNull();
}
+ @Test
+ public void undoneNodesFromIncrementalChanges_ignoredForSerialization() throws Exception {
+ setupScenarioWithConfiguredTargets();
+
+ write(
+ "foo/PROJECT.scl",
+ """
+active_directories = { "default": ["foo"] }
+""");
+
+ addOptions("--experimental_remote_analysis_cache_mode=upload");
+ buildTarget("//foo:A");
+
+ var serializedConfiguredTargetCount =
+ getCommandEnvironment()
+ .getRemoteAnalysisCachingEventListener()
+ .getSkyfunctionCounts()
+ .count(SkyFunctions.CONFIGURED_TARGET);
+
+ // Make a small change to the //foo:A's dep graph by cutting the dep on //foo:D.
+ // By changing this file, //foo:D will be invalidated as a transitive reverse dependency, but
+ // because evaluating //foo:A no longer needs //foo:D's value, it will remain as an un-done
+ // value. FrontierSerializer will try to mark //foo:D as active because it's in 'foo', but
+ // realizes that it's not done, so it will be ignored.
+ write(
+ "foo/BUILD",
+ """
+filegroup(name = "A", srcs = [":B", "//bar:C"]) # unchanged.
+filegroup(name = "B", srcs = ["//bar:E", "//bar:F"]) # changed: cut dep on D.
+filegroup(name = "D", srcs = ["//bar:H"]) # unchanged.
+filegroup(name = "G") # unchanged.
+""");
+
+ // This will pass only if FrontierSerializer only processes nodes that have finished evaluating.
+ buildTarget("//foo:A");
+ // //bar:H is not serialized because it was only reachable from //foo:D, so we expect
+ // exactly one fewer serialized CT.
+ assertThat(
+ getCommandEnvironment().getRemoteAnalysisCachingEventListener().getSkyfunctionCounts())
+ .hasCount(SkyFunctions.CONFIGURED_TARGET, serializedConfiguredTargetCount - 1);
+ }
+
protected void setupScenarioWithConfiguredTargets() throws Exception {
// ┌───────┐ ┌───────┐
// │ bar:C │ ◀── │ foo:A │
