Allow ParallelEvaluator to not store errors alongside values if nodes recover from errors. In the case of a single keep_going build, with no subsequent nokeep_going builds, storing the errors is unnecessary.

--
MOS_MIGRATED_REVID=114355846

4 files changed, 100 insertions, 13 deletions

diff --git a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
index a370cdc6a2..fd2f44b362 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.GraphTester.ValueComputer;
 import com.google.devtools.build.skyframe.ParallelEvaluator.SkyFunctionEnvironment;
@@ -26,7 +27,7 @@ import javax.annotation.Nullable;
  * {@link ValueComputer} that can be chained together with others of its type to synchronize the
  * order in which builders finish.
  */
-final class ChainedFunction implements SkyFunction {
+public final class ChainedFunction implements SkyFunction {
   @Nullable private final SkyValue value;
   @Nullable private final CountDownLatch notifyStart;
   @Nullable private final CountDownLatch waitToFinish;
@@ -34,9 +35,14 @@ final class ChainedFunction implements SkyFunction {
   private final boolean waitForException;
   private final Iterable<SkyKey> deps;
 
-  ChainedFunction(@Nullable CountDownLatch notifyStart, @Nullable CountDownLatch waitToFinish,
-      @Nullable CountDownLatch notifyFinish, boolean waitForException,
-      @Nullable SkyValue value, Iterable<SkyKey> deps) {
+  /** Do not use! Use {@link Builder} instead. */
+  ChainedFunction(
+      @Nullable CountDownLatch notifyStart,
+      @Nullable CountDownLatch waitToFinish,
+      @Nullable CountDownLatch notifyFinish,
+      boolean waitForException,
+      @Nullable SkyValue value,
+      Iterable<SkyKey> deps) {
     this.notifyStart = notifyStart;
     this.waitToFinish = waitToFinish;
     this.notifyFinish = notifyFinish;
@@ -80,6 +86,51 @@ final class ChainedFunction implements SkyFunction {
     }
   }
 
+  /** Builder for {@link ChainedFunction} objects. */
+  public static class Builder {
+    @Nullable private SkyValue value;
+    @Nullable private CountDownLatch notifyStart;
+    @Nullable private CountDownLatch waitToFinish;
+    @Nullable private CountDownLatch notifyFinish;
+    private boolean waitForException;
+    private Iterable<SkyKey> deps = ImmutableList.of();
+
+    public Builder setValue(SkyValue value) {
+      this.value = value;
+      return this;
+    }
+
+    public Builder setNotifyStart(CountDownLatch notifyStart) {
+      this.notifyStart = notifyStart;
+      return this;
+    }
+
+    public Builder setWaitToFinish(CountDownLatch waitToFinish) {
+      this.waitToFinish = waitToFinish;
+      return this;
+    }
+
+    public Builder setNotifyFinish(CountDownLatch notifyFinish) {
+      this.notifyFinish = notifyFinish;
+      return this;
+    }
+
+    public Builder setWaitForException(boolean waitForException) {
+      this.waitForException = waitForException;
+      return this;
+    }
+
+    public Builder setDeps(Iterable<SkyKey> deps) {
+      this.deps = Preconditions.checkNotNull(deps);
+      return this;
+    }
+
+    public SkyFunction build() {
+      return new ChainedFunction(
+          notifyStart, waitToFinish, notifyFinish, waitForException, value, deps);
+    }
+  }
+
   @Override
   public String extractTag(SkyKey skyKey) {
     throw new UnsupportedOperationException();
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
index 0f659bfc40..7a5f1d1100 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
@@ -43,4 +43,16 @@ public class ErrorInfoSubject extends Subject<ErrorInfoSubject, ErrorInfo> {
       fail("has root cause of exception " + key);
     }
   }
+
+  public void isTransient() {
+    if (!getSubject().isTransient()) {
+      fail("is transient");
+    }
+  }
+
+  public void isNotTransient() {
+    if (getSubject().isTransient()) {
+      fail("is not transient");
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java
index c0a5629aa8..7980ef5e95 100644
--- a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java
+++ b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java
@@ -13,8 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.truth.DefaultSubject;
 import com.google.common.truth.FailureStrategy;
+import com.google.common.truth.IterableSubject;
 import com.google.common.truth.Subject;
 import com.google.common.truth.Truth;
 
@@ -49,4 +51,9 @@ public class EvaluationResultSubject extends Subject<EvaluationResultSubject, Ev
         .named("Error entry for " + getDisplaySubject());
   }
 
+  public IterableSubject hasDirectDepsInGraphThat(SkyKey parent) {
+    return Truth.assertThat(
+            getSubject().getWalkableGraph().getDirectDeps(ImmutableList.of(parent)).get(parent))
+        .named("Direct deps for " + parent + " in " + getDisplaySubject());
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 108662aa97..94c9b819c3 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -18,6 +18,8 @@ import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertContainsEvent;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents;
+import static com.google.devtools.build.skyframe.ErrorInfoSubjectFactory.assertThatErrorInfo;
+import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
 import static com.google.devtools.build.skyframe.GraphTester.CONCATENATE;
 import static com.google.devtools.build.skyframe.GraphTester.COPY;
 import static com.google.devtools.build.skyframe.GraphTester.NODE_TYPE;
@@ -1066,8 +1068,12 @@ public class MemoizingEvaluatorTest {
     assertThat(cycleInfo.getPathToCycle()).isEmpty();
   }
 
-  @Test
-  public void parentOfCycleAndTransient() throws Exception {
+  /**
+   * {@link ParallelEvaluator} can be configured to not store errors alongside recovered values.
+   * In that case, transient errors that are recovered from do not make the parent transient.
+   */
+  protected void parentOfCycleAndTransientInternal(boolean errorsStoredAlongsideValues)
+      throws Exception {
     initializeTester();
     SkyKey cycleKey1 = GraphTester.toSkyKey("cycleKey1");
     SkyKey cycleKey2 = GraphTester.toSkyKey("cycleKey2");
@@ -1083,23 +1089,34 @@ public class MemoizingEvaluatorTest {
     tester.getOrCreate(top).setBuilder(new ChainedFunction(topEvaluated, null, null, false,
         new StringValue("unused"), ImmutableList.of(mid, cycleKey1)));
     EvaluationResult<StringValue> evalResult = tester.eval(true, top);
-    assertTrue(evalResult.hasError());
+    assertThatEvaluationResult(evalResult).hasError();
     ErrorInfo errorInfo = evalResult.getError(top);
     assertThat(topEvaluated.getCount()).isEqualTo(1);
-    // The parent should be transitively transient, since it transitively depends on a transient
-    // error.
-    assertThat(errorInfo.isTransient()).isTrue();
+    if (errorsStoredAlongsideValues) {
+      // The parent should be transitively transient, since it transitively depends on a transient
+      // error.
+      assertThat(errorInfo.isTransient()).isTrue();
+      assertThat(errorInfo.getException()).hasMessage(NODE_TYPE.getName() + ":errorKey");
+      assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey);
+    } else {
+      assertThatErrorInfo(errorInfo).isNotTransient();
+      assertThatErrorInfo(errorInfo).hasExceptionThat().isNull();
+    }
     assertWithMessage(errorInfo.toString())
         .that(errorInfo.getCycleInfo())
         .containsExactly(
             new CycleInfo(ImmutableList.of(top), ImmutableList.of(cycleKey1, cycleKey2)));
-    assertThat(errorInfo.getException()).hasMessage(NODE_TYPE.getName() + ":errorKey");
-    assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey);
     // But the parent itself shouldn't have a direct dep on the special error transience node.
-    assertThat(evalResult.getWalkableGraph().getDirectDeps(ImmutableList.of(top)).get(top))
+    assertThatEvaluationResult(evalResult)
+        .hasDirectDepsInGraphThat(top)
         .doesNotContain(ErrorTransienceValue.KEY);
   }
 
+  @Test
+  public void parentOfCycleAndTransient() throws Exception {
+    parentOfCycleAndTransientInternal(/*errorsStoredAlongsideValues=*/ true);
+  }
+
   /**
    * Regression test: IllegalStateException in BuildingState.isReady(). The ParallelEvaluator used
    * to assume during cycle-checking that all values had been built as fully as possible -- that