Add catastrophe field to EvaluationResult so that callers can identify the cause of a catastrophic failure (this is distinct from a crash).

Also clean up catastrophe logic in ParallelEvaluator -- the catastrophic nature of an exception is important only if the build is keep_going, and only if the exception is catastrophic can we have an exception in the first place.

--
MOS_MIGRATED_REVID=111293164

2 files changed, 51 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
index c398f7a9a7..64e1ff1475 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
@@ -40,6 +40,7 @@ import javax.annotation.Nullable;
 public class EvaluationResult<T extends SkyValue> {
 
   private final boolean hasError;
+  @Nullable private final Exception catastrophe;
 
   private final Map<SkyKey, T> resultMap;
   private final Map<SkyKey, ErrorInfo> errorMap;
@@ -48,13 +49,18 @@ public class EvaluationResult<T extends SkyValue> {
   /**
    * Constructor for the "completed" case. Used only by {@link Builder}.
    */
-  private EvaluationResult(Map<SkyKey, T> result, Map<SkyKey, ErrorInfo> errorMap,
-      boolean hasError, @Nullable WalkableGraph walkableGraph) {
+  private EvaluationResult(
+      Map<SkyKey, T> result,
+      Map<SkyKey, ErrorInfo> errorMap,
+      boolean hasError,
+      @Nullable Exception catastrophe,
+      @Nullable WalkableGraph walkableGraph) {
     Preconditions.checkState(errorMap.isEmpty() || hasError,
         "result=%s, errorMap=%s", result, errorMap);
     this.resultMap = Preconditions.checkNotNull(result);
     this.errorMap = Preconditions.checkNotNull(errorMap);
     this.hasError = hasError;
+    this.catastrophe = catastrophe;
     this.walkableGraph = walkableGraph;
   }
 
@@ -75,6 +81,12 @@ public class EvaluationResult<T extends SkyValue> {
     return hasError;
   }
 
+  /** @return catastrophic error encountered during evaluation, if any */
+  @Nullable
+  public Exception getCatastrophe() {
+    return catastrophe;
+  }
+
   /**
    * @return All successfully evaluated {@link SkyValue}s.
    */
@@ -152,6 +164,7 @@ public class EvaluationResult<T extends SkyValue> {
     private final Map<SkyKey, T> result = new HashMap<>();
     private final Map<SkyKey, ErrorInfo> errors = new HashMap<>();
     private boolean hasError = false;
+    @Nullable private Exception catastrophe = null;
     private WalkableGraph walkableGraph = null;
 
     /** Adds a value to the result. An error for this key must not already be present. */
@@ -184,11 +197,15 @@ public class EvaluationResult<T extends SkyValue> {
     }
 
     public EvaluationResult<T> build() {
-      return new EvaluationResult<>(result, errors, hasError, walkableGraph);
+      return new EvaluationResult<>(result, errors, hasError, catastrophe, walkableGraph);
     }
 
     public void setHasError(boolean hasError) {
       this.hasError = hasError;
     }
+
+    public void setCatastrophe(Exception catastrophe) {
+      this.catastrophe = catastrophe;
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index e2a6608210..54b3cfd18d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -1312,10 +1312,14 @@ public final class ParallelEvaluator implements Evaluator {
       // ErrorInfo could only be null if SchedulerException wrapped an InterruptedException, but
       // that should have been propagated.
       ErrorInfo errorInfo = Preconditions.checkNotNull(e.getErrorInfo(), errorKey);
-      catastrophe = errorInfo.isCatastrophic();
-      if (!catastrophe || !keepGoing) {
+      if (!keepGoing) {
         bubbleErrorInfo = bubbleErrorUp(errorInfo, errorKey, skyKeys, visitor);
       } else {
+        Preconditions.checkState(
+            errorInfo.isCatastrophic(),
+            "Scheduler exception only thrown for catastrophe in keep_going evaluation: %s",
+            e);
+        catastrophe = true;
         // Bubbling the error up requires that graph edges are present for done nodes. This is not
         // always the case in a keepGoing evaluation, since it is assumed that done nodes do not
         // need to be traversed. In this case, we hope the caller is tolerant of a possibly empty
@@ -1507,10 +1511,17 @@ public final class ParallelEvaluator implements Evaluator {
    * {@code skyKeys} are known to be in the DONE state ({@code entry.isDone()} returns true).
    */
   private <T extends SkyValue> EvaluationResult<T> constructResult(
-      @Nullable ValueVisitor visitor, Iterable<SkyKey> skyKeys,
-      Map<SkyKey, ValueWithMetadata> bubbleErrorInfo, boolean catastrophe) {
-    Preconditions.checkState(!keepGoing || catastrophe || bubbleErrorInfo == null,
-        "", skyKeys, bubbleErrorInfo);
+      @Nullable ValueVisitor visitor,
+      Iterable<SkyKey> skyKeys,
+      @Nullable Map<SkyKey, ValueWithMetadata> bubbleErrorInfo,
+      boolean catastrophe) {
+    Preconditions.checkState(
+        catastrophe == (keepGoing && bubbleErrorInfo != null),
+        "Catastrophe not consistent with keepGoing mode and bubbleErrorInfo: %s %s %s %s",
+        skyKeys,
+        catastrophe,
+        keepGoing,
+        bubbleErrorInfo);
     EvaluationResult.Builder<T> result = EvaluationResult.builder();
     List<SkyKey> cycleRoots = new ArrayList<>();
     boolean hasError = false;
@@ -1556,6 +1567,20 @@ public final class ParallelEvaluator implements Evaluator {
     Preconditions.checkState(bubbleErrorInfo == null || hasError,
         "If an error bubbled up, some top-level node must be in error", bubbleErrorInfo, skyKeys);
     result.setHasError(hasError);
+    if (catastrophe) {
+      // We may not have a top-level node completed. Inform the caller of the catastrophic exception
+      // that shut down the evaluation so that it has some context.
+      ErrorInfo errorInfo =
+          Preconditions.checkNotNull(
+              Iterables.getOnlyElement(bubbleErrorInfo.values()).getErrorInfo(),
+              "bubbleErrorInfo should have contained element with errorInfo: %s",
+              bubbleErrorInfo);
+      Preconditions.checkState(
+          errorInfo.isCatastrophic(),
+          "bubbleErrorInfo should have contained element with catastrophe: %s",
+          bubbleErrorInfo);
+      result.setCatastrophe(errorInfo.getException());
+    }
     return result.build();
   }