Add missing state reset on ActionExecutionFunction.complete for the d…

…elegator.

The crash in b/174837755 was caused by a stale
SkyframeEvalWithOrderedListAEFDelegator and as a result, a stale stateMap.

RELNOTES: None
PiperOrigin-RevId: 346367713

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

@@ -157,7 +157,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       if (skyframeEvalWithOrderedListAEFDelegator == null) {
         skyframeEvalWithOrderedListAEFDelegator =
             new SkyframeEvalWithOrderedListAEFDelegator(
-                skyframeActionExecutor, directories, tsgm, stateMap);
+                skyframeActionExecutor, directories, tsgm, stateMap, actionRewindStrategy);
       }
       return skyframeEvalWithOrderedListAEFDelegator.compute(skyKey, env);
     }
@@ -1418,6 +1418,11 @@ public void complete(ExtendedEventHandler eventHandler) {
     // Discard all remaining state (there should be none after a successful execution).
     stateMap = Maps.newConcurrentMap();
     actionRewindStrategy.reset(eventHandler);
+    if (skyframeEvalWithOrderedListAEFDelegator != null) {
+      skyframeEvalWithOrderedListAEFDelegator =
+          new SkyframeEvalWithOrderedListAEFDelegator(
+              skyframeActionExecutor, directories, tsgm, stateMap, actionRewindStrategy);
+    }
   }
 
   private ContinuationState getState(Action action) {

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

@@ -63,7 +63,6 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
-import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -129,21 +128,23 @@
  */
 // LINT.IfChange
 public class SkyframeEvalWithOrderedListAEFDelegator {
-  private final ActionRewindStrategy actionRewindStrategy = new ActionRewindStrategy();
+  private final ActionRewindStrategy actionRewindStrategy;
   private final SkyframeActionExecutor skyframeActionExecutor;
   private final BlazeDirectories directories;
   private final AtomicReference<TimestampGranularityMonitor> tsgm;
-  private ConcurrentMap<Action, ContinuationState> stateMap;
+  private final ConcurrentMap<Action, ContinuationState> stateMap;
 
   public SkyframeEvalWithOrderedListAEFDelegator(
       SkyframeActionExecutor skyframeActionExecutor,
       BlazeDirectories directories,
       AtomicReference<TimestampGranularityMonitor> tsgm,
-      ConcurrentMap<Action, ContinuationState> stateMap) {
+      ConcurrentMap<Action, ContinuationState> stateMap,
+      ActionRewindStrategy actionRewindStrategy) {
     this.skyframeActionExecutor = skyframeActionExecutor;
     this.directories = directories;
     this.tsgm = tsgm;
     this.stateMap = stateMap;
+    this.actionRewindStrategy = actionRewindStrategy;
   }
 
   public SkyValue compute(SkyKey skyKey, Environment env)
@@ -1329,17 +1330,6 @@ static LabelCause createLabelCause(
     return new LabelCause(inputLabel, missingValue.getDetailedExitCode());
   }
 
-  /**
-   * Should be called once execution is over, and the intra-build cache of in-progress computations
-   * should be discarded. If the cache is non-empty (due to an interrupted/failed build), failure to
-   * call complete() can both cause a memory leak and incorrect results on the subsequent build.
-   */
-  public void complete(ExtendedEventHandler eventHandler) {
-    // Discard all remaining state (there should be none after a successful execution).
-    stateMap = Maps.newConcurrentMap();
-    actionRewindStrategy.reset(eventHandler);
-  }
-
   private ContinuationState getState(Action action) {
     ContinuationState state = stateMap.get(action);
     if (state == null) {

src/test/shell/integration/execution_phase_tests.sh

@@ -393,4 +393,42 @@ function test_track_directory_crossing_package() {
   expect_log "WARNING: Directory artifact foo/dir crosses package boundary into"
 }
 
+# Regression test for b/174837755.
+# TODO(b/172462551) Clean this up after the experiment
+function test_skyframe_eval_with_ordered_list_incremental_with_error() {
+  export DONT_SANITY_CHECK_SERIALIZATION=1
+  mkdir -p foo
+  cat > foo/BUILD <<EOF
+cc_binary(
+    name = "main",
+    srcs = [
+        "main.cc",
+    ],
+    deps = [":lib"],
+)
+
+cc_library(
+    name = "lib",
+    srcs = [
+        "lib.cc",
+        "lib.h",
+    ],
+)
+EOF
+  touch foo/lib.h
+  touch foo/main.cc
+  echo "abc" > foo/lib.cc
+
+  bazel build --experimental_skyframe_eval_with_ordered_list //foo:main \
+    &> "$TEST_log" && fail "Expected failure"
+
+  bazel build --experimental_skyframe_eval_with_ordered_list //foo:main \
+    &> "$TEST_log" && fail "Expected failure"
+
+  # The incremental run shouldn't crash bazel.
+  exit_code="$?"
+  [[ "$exit_code" -eq 1 ]] || fail "Unexpected exit code: $exit_code"
+
+  true  # reset the last exit code so the test won't be considered failed
+}
 run_suite "Integration tests of ${PRODUCT_NAME} using the execution phase."