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");