Stop requiring that a catastrophe remains a catastrophe when it's bub…

…bled up. The salient characteristic of a catastrophe in Skyframe is that it shuts down a keep-going build. How it's transformed during error bubbling is not a concern.

PiperOrigin-RevId: 409222485

src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -597,8 +596,9 @@ <T extends SkyValue> EvaluationResult<T> constructResult(
  bubbleErrorInfo);
  EvaluationResult.Builder<T> result = EvaluationResult.builder();
  List<SkyKey> cycleRoots = new ArrayList<>();
- boolean nonCycleErrorFound = false;
+ boolean haveKeys = false;
  for (SkyKey skyKey : skyKeys) {
+ haveKeys = true;
  SkyValue unwrappedValue =
  maybeGetValueFromError(
  skyKey, graph.get(null, Reason.PRE_OR_POST_EVALUATION, skyKey), bubbleErrorInfo);
@@ -618,15 +618,13 @@ <T extends SkyValue> EvaluationResult<T> constructResult(
  Preconditions.checkState(value != null || errorInfo != null, skyKey);
  if (!evaluatorContext.keepGoing() && errorInfo != null) {
  // value will be null here unless the value was already built on a prior keepGoing build.
- nonCycleErrorFound = true;
  result.addError(skyKey, errorInfo);
  continue;
  }
  if (value == null) {
  // Note that we must be in the keepGoing case. Only make this value an error if it doesn't
  // have a value. The error shouldn't matter to the caller since the value succeeded after a
  // fashion.
- nonCycleErrorFound = true;
  result.addError(skyKey, errorInfo);
  } else {
  result.addResult(skyKey, value);
@@ -635,50 +633,35 @@ <T extends SkyValue> EvaluationResult<T> constructResult(
  if (!cycleRoots.isEmpty()) {
  cycleDetector.checkForCycles(cycleRoots, result, evaluatorContext);
  }
- if (catastrophe && bubbleErrorInfo != null && !result.hasCatastrophe()) {
- // We may not have a top-level node completed. Inform the caller of at least one catastrophic
- // exception that shut down the evaluation so that it has some context.
- // TODO(b/159006108): Sometimes we get here and not every exception is catastrophic, so we
- // alert when that happens. If we didn't need to guard against that case, we could simply
- // take the last element of bubbleErrorInfo#values() and make that the catastrophe.
- boolean catastropheFound = false;
- @Nullable Exception nonCatastrophicExceptionForBugHandler = null;
- for (ValueWithMetadata valueWithMetadata : bubbleErrorInfo.values()) {
+ if (haveKeys && result.isEmpty()) {
+ Preconditions.checkState(
+ catastrophe && bubbleErrorInfo != null,
+ "No top-level keys were present but we did not have catastrophic error bubbling: %s %s %s"
+ + " %s",
+ skyKeys,
+ catastrophe,
+ bubbleErrorInfo,
+ cycleRoots);
+ boolean exceptionFound = false;
+ for (ValueWithMetadata valueWithMetadata :
+ ImmutableList.copyOf(bubbleErrorInfo.values()).reverse()) {
  ErrorInfo errorInfo =
  Preconditions.checkNotNull(
  valueWithMetadata.getErrorInfo(),
  "bubbleErrorInfo should have contained element with errorInfo: %s",
  bubbleErrorInfo);
- if (errorInfo.isCatastrophic()) {
- if (!result.hasCatastrophe()) {
- result.setCatastrophe(errorInfo.getException());
- }
- catastropheFound = true;
- } else {
- // Alert for the known bug of a non-catastrophic exception.
- BugReport.sendBugReport(
- new IllegalStateException(
- String.format(
- "bubbleErrorInfo should have contained element with catastrophe: %s"
- + " (bubbleErrorInfo: %s)",
- valueWithMetadata, bubbleErrorInfo)));
- if (errorInfo.getException() != null) {
- nonCatastrophicExceptionForBugHandler = errorInfo.getException();
- }
+ if (errorInfo.getException() != null) {
+ result.setCatastrophe(errorInfo.getException());
+ exceptionFound = true;
+ break;
  }
  }
- if (!catastropheFound && !nonCycleErrorFound) {
- Preconditions.checkNotNull(
- nonCatastrophicExceptionForBugHandler,
- "There were no exceptions in bubbleErrorInfo despite a catastrophic failure (%s)",
- bubbleErrorInfo);
- // Alert for the never-seen bug of *no* catastrophic exceptions.
- BugReport.sendBugReport(
- new IllegalStateException(
- "No element in bubbleErrorInfo was catastrophic: " + bubbleErrorInfo,
- nonCatastrophicExceptionForBugHandler));
- result.setCatastrophe(nonCatastrophicExceptionForBugHandler);
- }
+ Preconditions.checkState(
+ exceptionFound,
+ "Evaluation of %s terminated early with no exception (%s %s %s)",
+ skyKeys,
+ bubbleErrorInfo,
+ result);
  }
  EvaluationResult<T> builtResult = result.build();
  Preconditions.checkState(

src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java

@@ -196,8 +196,8 @@ public Builder<T> setCatastrophe(Exception catastrophe) {
  return this;
  }
 
- boolean hasCatastrophe() {
- return this.catastrophe != null;
+ boolean isEmpty() {
+ return this.result.isEmpty() && this.errors.isEmpty();
  }
  }
 }

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

@@ -72,6 +72,7 @@ java_library(
  "//third_party:junit4",
  "//third_party:mockito",
  "//third_party:truth",
+ "@com_google_testparameterinjector//:testparameterinjector",
  ],
 )
 

src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java

@@ -19,6 +19,7 @@
 import static com.google.devtools.build.lib.testutil.EventIterableSubjectFactory.assertThatEvents;
 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.skyKey;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 
@@ -52,6 +53,8 @@
 import com.google.devtools.build.skyframe.NotifyingHelper.Listener;
 import com.google.devtools.build.skyframe.NotifyingHelper.Order;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -66,16 +69,14 @@
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 import org.junit.After;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 import org.mockito.Mockito;
 
-/**
- * Tests for {@link ParallelEvaluator}.
- */
-@RunWith(JUnit4.class)
+/** Tests for {@link ParallelEvaluator}. */
+@RunWith(TestParameterInjector.class)
 public class ParallelEvaluatorTest {
  private static final SkyFunctionName CHILD_TYPE = SkyFunctionName.createHermetic("child");
  private static final SkyFunctionName PARENT_TYPE = SkyFunctionName.createHermetic("parent");
@@ -850,21 +851,10 @@ public void shouldBuildOneTarget() throws Exception {
  }
 
  @Test
- public void catastropheHaltsBuild_KeepGoing_KeepEdges() throws Exception {
- catastrophicBuild(true, true);
- }
-
- @Test
- public void catastropheHaltsBuild_KeepGoing_NoKeepEdges() throws Exception {
- catastrophicBuild(true, false);
- }
-
- @Test
- public void catastropheInBuild_NoKeepGoing_KeepEdges() throws Exception {
- catastrophicBuild(false, true);
- }
+ public void catastrophicBuild(@TestParameter boolean keepGoing, @TestParameter boolean keepEdges)
+ throws Exception {
+ Assume.assumeTrue(keepGoing || keepEdges);
 
- private void catastrophicBuild(boolean keepGoing, boolean keepEdges) throws Exception {
  graph = new InMemoryGraphImpl(keepEdges);
 
  SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe");
@@ -949,6 +939,67 @@ public String extractTag(SkyKey skyKey) {
  assertThat(result.getCatastrophe()).isEqualTo(catastrophe);
  }
 
+ @Test
+ public void catastropheBubblesIntoNonCatastrophe() throws Exception {
+ graph = new InMemoryGraphImpl();
+ SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe");
+ Exception catastrophe = new SomeErrorException("bad");
+ tester
+ .getOrCreate(catastropheKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Nullable
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
+ throw new SkyFunctionException(catastrophe, Transience.PERSISTENT) {
+ @Override
+ public boolean isCatastrophic() {
+ return true;
+ }
+ };
+ }
+
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ SkyKey topKey = skyKey("top");
+ tester
+ .getOrCreate(topKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Nullable
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws SkyFunctionException, InterruptedException {
+ try {
+ env.getValueOrThrow(catastropheKey, SomeErrorException.class);
+ } catch (SomeErrorException e) {
+ throw new SkyFunctionException(
+ new SomeErrorException("We got: " + e.getMessage()), Transience.PERSISTENT) {
+ @Override
+ public boolean isCatastrophic() {
+ return false;
+ }
+ };
+ }
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ EvaluationResult<StringValue> result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey));
+
+ assertThat(result.getError(topKey).getException()).isInstanceOf(SomeErrorException.class);
+ assertThat(result.getError(topKey).getException()).hasMessageThat().isEqualTo("We got: bad");
+ }
+
  @Test
  public void incrementalCycleWithCatastropheAndFailedBubbleUp() throws Exception {
  SkyKey topKey = GraphTester.toSkyKey("top");