Fix a bug with non-execution phase errors being considered

joeleba · copybara-github · commit 436d04d22347 · 2022-04-04T01:35:29.000-07:00
UndetailedExecutionCause.

Also, send a bug report for actual undetailed execution errors, instead of just
logging.

PiperOrigin-RevId: 439253033
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java
@@ -23,10 +23,11 @@
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.skyframe.WalkableGraph;
+import javax.annotation.Nullable;
 
 /** Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe. */
 public final class SkyframeAnalysisAndExecutionResult extends SkyframeAnalysisResult {
-  private final DetailedExitCode representativeExecutionExitCode;
+  @Nullable private final DetailedExitCode representativeExecutionExitCode;
 
   private SkyframeAnalysisAndExecutionResult(
       boolean hasLoadingError,
@@ -48,6 +49,7 @@ private SkyframeAnalysisAndExecutionResult(
     this.representativeExecutionExitCode = representativeExecutionExitCode;
   }
 
+  @Nullable
   public DetailedExitCode getRepresentativeExecutionExitCode() {
     return representativeExecutionExitCode;
   }
@@ -96,7 +98,7 @@ public static SkyframeAnalysisAndExecutionResult withErrors(
       WalkableGraph walkableGraph,
       ImmutableMap<AspectKey, ConfiguredAspect> aspects,
       PackageRoots packageRoots,
-      DetailedExitCode representativeExecutionExitCode) {
+      @Nullable DetailedExitCode representativeExecutionExitCode) {
     return new SkyframeAnalysisAndExecutionResult(
         hasLoadingError,
         hasAnalysisError,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
@@ -166,8 +166,7 @@ static ErrorProcessingResult processErrors(
     // turns out to be with execution.
     boolean hasAnalysisError = true;
     ViewCreationFailedException noKeepGoingAnalysisExceptionAspect = null;
-    DetailedExitCode detailedExitCode = null;
-    Throwable undetailedCause = null;
+    DetailedExitCode representativeExecutionDetailedExitCode = null;
     Map<ActionAnalysisMetadata, ConflictException> actionConflicts = new HashMap<>();
     for (Map.Entry<SkyKey, ErrorInfo> errorEntry : result.errorMap().entrySet()) {
       SkyKey errorKey = getErrorKey(errorEntry);
@@ -275,17 +274,21 @@ static ErrorProcessingResult processErrors(
         actionConflicts.putAll(tlce.getTransitiveActionConflicts());
         continue;
       } else if (isExecutionException(cause)) {
-        detailedExitCode =
+        DetailedExitCode detailedExitCode = DetailedException.getDetailedExitCode(cause);
+        if (detailedExitCode == null) {
+          detailedExitCode = createDetailedExitCodeForUndetailedExecutionCause(result, cause);
+        }
+        representativeExecutionDetailedExitCode =
             DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie(
-                detailedExitCode, ((DetailedException) cause).getDetailedExitCode());
+                representativeExecutionDetailedExitCode, detailedExitCode);
         rootCauses =
             cause instanceof ActionExecutionException
                 ? ((ActionExecutionException) cause).getRootCauses()
                 : NestedSetBuilder.emptySet(Order.STABLE_ORDER);
         hasAnalysisError = false;
       } else {
-        // TODO(ulfjack): Report something!
-        undetailedCause = cause;
+        BugReport.logUnexpected(
+            cause, "Unexpected cause encountered while evaluating: %s", errorKey);
       }
 
       if (!inTest) {
@@ -325,12 +328,11 @@ static ErrorProcessingResult processErrors(
       throw noKeepGoingAnalysisExceptionAspect;
     }
 
-    if (includeExecutionPhase && detailedExitCode == null) {
-      detailedExitCode = createDetailedExitCodeForUndetailedExecutionCause(result, undetailedCause);
-    }
-
     return ErrorProcessingResult.create(
-        hasLoadingError, hasAnalysisError, ImmutableMap.copyOf(actionConflicts), detailedExitCode);
+        hasLoadingError,
+        hasAnalysisError,
+        ImmutableMap.copyOf(actionConflicts),
+        representativeExecutionDetailedExitCode);
   }
 
   private static SkyKey getErrorKey(Entry<SkyKey, ErrorInfo> errorEntry) {
@@ -631,14 +633,14 @@ static void rethrow(
 
   private static DetailedExitCode createDetailedExitCodeForUndetailedExecutionCause(
       EvaluationResult<?> result, Throwable undetailedCause) {
-    // TODO(b/227660368): These warning logs should be a bug report, but tests currently fail.
     if (undetailedCause == null) {
-      logger.atWarning().log("No exceptions found despite error in %s", result);
+      BugReport.sendBugReport("No exceptions found despite error in %s", result);
       return createDetailedExecutionExitCode(
           "keep_going execution failed without an action failure",
           Execution.Code.NON_ACTION_EXECUTION_FAILURE);
     }
-    logger.atWarning().withCause(undetailedCause).log("No detailed exception found in %s", result);
+    BugReport.sendBugReport(
+        new IllegalStateException("No detailed exception found in " + result, undetailedCause));
     return createDetailedExecutionExitCode(
         "keep_going execution failed without an action failure: "
             + undetailedCause.getMessage()
