[7.1.0] Fix a hanging issue with skymeld & --combined_report=lcov. (#…

…21271)

With Skymeld, the combined report is done right before the end of the
build. This works fine except for a buggy case:
- `--nokeep_going`
- when there's an error that the Coverage action transitively depends on
- The error would try to interrupt the build, and since we evaluate
coverage actions uninterruptibly (consistent with the noskymeld
behavior), we're stuck in an infinite loop [1].

This happened because we did not fail fast like we should have. This CL
fixes the issue by checking, before generating the combined report,
whether we should fail fast.

Fixes #21154

[1]
https://github.com/bazelbuild/bazel/blob/026f493a5a403fa5d770cae0b3a3baf8dcf33488/src/main/java/com/google/devtools/build/lib/concurrent/Uninterruptibles.java#L33-L39

Commit
37247d5

PiperOrigin-RevId: 605572100
Change-Id: I8d57dacca58358771799161352819ec65bef6ac2

Co-authored-by: Googler <leba@google.com>

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java

@@ -584,7 +584,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           BuildFailedException,
           TestExecException {
     Stopwatch analysisWorkTimer = Stopwatch.createStarted();
-    EvaluationResult<SkyValue> evaluationResult;
+    EvaluationResult<SkyValue> mainEvaluationResult;
 
     var newKeys =
         ImmutableSet.<ActionLookupKey>builderWithExpectedSize(
@@ -663,7 +663,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
             Profiler.instance().profile("skyframeExecutor.evaluateBuildDriverKeys")) {
           // Will be disabled later by the AnalysisOperationWatcher upon conclusion of analysis.
           enableAnalysis(true);
-          evaluationResult =
+          mainEvaluationResult =
               skyframeExecutor.evaluateBuildDriverKeys(
                   eventHandler,
                   buildDriverCTKeys,
@@ -689,8 +689,10 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
         }
 
         // The exclusive tests whose analysis succeeded i.e. those that can be run.
-        ImmutableSet<ConfiguredTarget> exclusiveTestsToRun = getExclusiveTests(evaluationResult);
-        boolean continueWithExclusiveTests = !evaluationResult.hasError() || keepGoing;
+        ImmutableSet<ConfiguredTarget> exclusiveTestsToRun =
+            getExclusiveTests(mainEvaluationResult);
+        boolean continueWithExclusiveTests = !mainEvaluationResult.hasError() || keepGoing;
+        boolean hasExclusiveTestsError = false;
 
         if (continueWithExclusiveTests && !exclusiveTestsToRun.isEmpty()) {
           skyframeExecutor.getIsBuildingExclusiveArtifacts().set(true);
@@ -707,6 +709,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
                     keepGoing,
                     executors.executionParallelism());
             if (testRunResult.hasError()) {
+              hasExclusiveTestsError = true;
               detailedExitCodes.add(
                   SkyframeErrorProcessor.processErrors(
                           testRunResult,
@@ -721,29 +724,33 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
             }
           }
         }
+
         // Coverage report generation should only be requested after all tests have executed.
         // We could generate baseline coverage artifacts earlier; it is only the timing of the
         // combined report that matters.
-        ImmutableSet<Artifact> coverageArtifacts =
-            coverageReportActionsWrapperSupplier.getCoverageArtifacts(
-                buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
-        eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts));
-        additionalArtifactsResult =
-            skyframeExecutor.evaluateSkyKeys(
-                eventHandler, Artifact.keys(coverageArtifacts), keepGoing);
-        eventBus.post(new CoverageActionFinishedEvent());
-        if (additionalArtifactsResult.hasError()) {
-          detailedExitCodes.add(
-              SkyframeErrorProcessor.processErrors(
-                      additionalArtifactsResult,
-                      skyframeExecutor.getCyclesReporter(),
-                      eventHandler,
-                      keepGoing,
-                      skyframeExecutor.tracksStateForIncrementality(),
-                      eventBus,
-                      bugReporter,
-                      /* includeExecutionPhase= */ true)
-                  .executionDetailedExitCode());
+        // When --nokeep_going and there's an earlier error, we should skip this and fail fast.
+        if ((!mainEvaluationResult.hasError() && !hasExclusiveTestsError) || keepGoing) {
+          ImmutableSet<Artifact> coverageArtifacts =
+              coverageReportActionsWrapperSupplier.getCoverageArtifacts(
+                  buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
+          eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts));
+          additionalArtifactsResult =
+              skyframeExecutor.evaluateSkyKeys(
+                  eventHandler, Artifact.keys(coverageArtifacts), keepGoing);
+          eventBus.post(new CoverageActionFinishedEvent());
+          if (additionalArtifactsResult.hasError()) {
+            detailedExitCodes.add(
+                SkyframeErrorProcessor.processErrors(
+                        additionalArtifactsResult,
+                        skyframeExecutor.getCyclesReporter(),
+                        eventHandler,
+                        keepGoing,
+                        skyframeExecutor.tracksStateForIncrementality(),
+                        eventBus,
+                        bugReporter,
+                        /* includeExecutionPhase= */ true)
+                    .executionDetailedExitCode());
+          }
         }
       } finally {
         // No more action execution beyond this point.
@@ -752,35 +759,35 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
         resourceManager.resetResourceUsage();
       }
 
-      if (!evaluationResult.hasError() && detailedExitCodes.isEmpty()) {
+      if (!mainEvaluationResult.hasError() && detailedExitCodes.isEmpty()) {
         ImmutableMap<AspectKey, ConfiguredAspect> successfulAspects =
             getSuccessfulAspectMap(
                 topLevelAspectsKeys.size(),
-                evaluationResult,
+                mainEvaluationResult,
                 buildDriverAspectKeys,
-                /*topLevelActionConflictReport=*/ null);
+                /* topLevelActionConflictReport= */ null);
         var targetsWithConfiguration =
             ImmutableList.<TargetAndConfiguration>builderWithExpectedSize(ctKeys.size());
         ImmutableSet<ConfiguredTarget> successfulConfiguredTargets =
             getSuccessfulConfiguredTargets(
                 ctKeys.size(),
-                evaluationResult,
+                mainEvaluationResult,
                 buildDriverCTKeys,
                 labelTargetMap,
                 targetsWithConfiguration,
                 /* topLevelActionConflictReport= */ null);
 
         return SkyframeAnalysisAndExecutionResult.success(
             successfulConfiguredTargets,
-            evaluationResult.getWalkableGraph(),
+            mainEvaluationResult.getWalkableGraph(),
             successfulAspects,
             targetsWithConfiguration.build(),
             /* packageRoots= */ null);
       }
 
       ErrorProcessingResult errorProcessingResult =
           SkyframeErrorProcessor.processErrors(
-              evaluationResult,
+              mainEvaluationResult,
               skyframeExecutor.getCyclesReporter(),
               eventHandler,
               keepGoing,
@@ -795,7 +802,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           foundActionConflictInLatestCheck
               ? handleActionConflicts(
                   eventHandler,
-                  evaluationResult.getWalkableGraph(),
+                  mainEvaluationResult.getWalkableGraph(),
                   ctKeys,
                   topLevelAspectsKeys,
                   topLevelArtifactContext,
@@ -807,15 +814,15 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
       ImmutableMap<AspectKey, ConfiguredAspect> successfulAspects =
           getSuccessfulAspectMap(
               topLevelAspectsKeys.size(),
-              evaluationResult,
+              mainEvaluationResult,
               buildDriverAspectKeys,
               topLevelActionConflictReport);
       var targetsWithConfiguration =
           ImmutableList.<TargetAndConfiguration>builderWithExpectedSize(ctKeys.size());
       ImmutableSet<ConfiguredTarget> successfulConfiguredTargets =
           getSuccessfulConfiguredTargets(
               ctKeys.size(),
-              evaluationResult,
+              mainEvaluationResult,
               buildDriverCTKeys,
               labelTargetMap,
               targetsWithConfiguration,
@@ -826,7 +833,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           /* hasAnalysisError= */ errorProcessingResult.hasAnalysisError(),
           /* hasActionConflicts= */ foundActionConflictInLatestCheck,
           successfulConfiguredTargets,
-          evaluationResult.getWalkableGraph(),
+          mainEvaluationResult.getWalkableGraph(),
           successfulAspects,
           targetsWithConfiguration.build(),
           /* packageRoots= */ null,