Use a BugReporter in ActionRewindStrategy.

In the test case that covers ineffective rewinding (action lost input too many times), use a `RecordingBugReporter` instead of catching the crash thrown by the default bug report in tests. This allows us to test the code path for handling such an exception and assert that the detailed exit code is correct.

PiperOrigin-RevId: 604487369
Change-Id: I39f863b05d27ef7591449814d9243d73db85a668

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

@@ -718,7 +718,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) {
             syscallCache));
     map.put(SkyFunctions.BUILD_INFO, new WorkspaceStatusFunction(this::makeWorkspaceStatusAction));
     map.put(SkyFunctions.COVERAGE_REPORT, new CoverageReportFunction(actionKeyContext));
-    this.actionRewindStrategy = new ActionRewindStrategy();
+    this.actionRewindStrategy = new ActionRewindStrategy(bugReporter);
     map.put(SkyFunctions.ACTION_EXECUTION, newActionExecutionFunction());
     map.put(
         SkyFunctions.RECURSIVE_FILESYSTEM_TRAVERSAL,

src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java

@@ -45,7 +45,7 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.LostInputsActionExecutionException;
-import com.google.devtools.build.lib.bugreport.BugReport;
+import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.collect.nestedset.ArtifactNestedSetKey;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.server.FailureDetails.ActionRewinding;
@@ -81,12 +81,18 @@ public final class ActionRewindStrategy {
   @VisibleForTesting static final int MAX_ACTION_REWIND_EVENTS = 5;
   private static final int MAX_LOST_INPUTS_RECORDED = 5;
 
+  private final BugReporter bugReporter;
+
   // Note that these references are mutated only outside of Skyframe evaluations, and accessed only
   // inside of them. Their visibility piggybacks on Skyframe evaluation synchronizations.
   private ConcurrentHashMultiset<LostInputRecord> lostInputRecords =
       ConcurrentHashMultiset.create();
   private ConcurrentLinkedQueue<RewindPlanStats> rewindPlansStats = new ConcurrentLinkedQueue<>();
 
+  public ActionRewindStrategy(BugReporter bugReporter) {
+    this.bugReporter = checkNotNull(bugReporter);
+  }
+
   /**
    * Returns a {@link RewindPlan} specifying:
    *
@@ -266,7 +272,7 @@ private ImmutableList<LostInputRecord> checkIfActionLostInputTooManyTimes(
                 "lost input too many times (#%s) for the same action. lostInput: %s, "
                     + "lostInput digest: %s, failedAction: %.10000s",
                 priorLosses + 1, lostInputsByDigest.get(digest), digest, failedAction);
-        BugReport.sendBugReport(new IllegalStateException(message));
+        bugReporter.sendBugReport(new IllegalStateException(message));
         throw createActionExecutionException(
             lostInputsException, failedAction, message, Code.LOST_INPUT_TOO_MANY_TIMES);
       } else if (0 < priorLosses) {
@@ -279,7 +285,7 @@ private ImmutableList<LostInputRecord> checkIfActionLostInputTooManyTimes(
     return lostInputRecordsThisAction.build();
   }
 
-  private static Set<DerivedArtifact> getLostInputOwningDirectDeps(
+  private Set<DerivedArtifact> getLostInputOwningDirectDeps(
       ImmutableList<ActionInput> lostInputs,
       ActionInputDepOwners inputDepOwners,
       Set<SkyKey> failedActionDeps,
@@ -340,7 +346,7 @@ private static Set<DerivedArtifact> getLostInputOwningDirectDeps(
         // of the failed action. In this case, try resetting the failed action (and no other deps)
         // just in case that helps. If it does not help, then eventually the action will fail in
         // checkIfActionLostInputTooManyTimes.
-        BugReport.sendNonFatalBugReport(
+        bugReporter.sendNonFatalBugReport(
             new IllegalStateException(
                 String.format(
                     "Lost input not a dep of the failed action and can't be associated with such"
@@ -351,7 +357,7 @@ private static Set<DerivedArtifact> getLostInputOwningDirectDeps(
     return lostInputOwningDirectDeps;
   }
 
-  private static void checkDerived(
+  private void checkDerived(
       String lostInputQualifier,
       Artifact expectedDerived,
       Action failedAction,
@@ -365,7 +371,7 @@ private static void checkDerived(
         String.format(
             "Unexpected source artifact as lost input%s: %s %s",
             lostInputQualifier, expectedDerived, failedAction);
-    BugReport.sendBugReport(new IllegalStateException(message));
+    bugReporter.sendBugReport(new IllegalStateException(message));
     throw createActionExecutionException(
         lostInputsException, failedAction, message, Code.LOST_INPUT_IS_SOURCE);
   }
@@ -375,13 +381,12 @@ private static void checkDerived(
    * actions, and (in the case of {@link SkyframeAwareAction}s) other Skyframe nodes need to be
    * rewound. If this finds more actions to rewind, those actions are recursively checked too.
    */
-  private static void checkActions(
+  private void checkActions(
       ImmutableList<ActionAndLookupData> actionsToCheck,
       Environment env,
       MutableGraph<SkyKey> rewindGraph,
       ImmutableList.Builder<Action> depsToRewind)
       throws InterruptedException {
-
     ArrayDeque<ActionAndLookupData> uncheckedActions = new ArrayDeque<>(actionsToCheck);
     while (!uncheckedActions.isEmpty()) {
       ActionAndLookupData actionAndLookupData = uncheckedActions.removeFirst();
@@ -466,7 +471,7 @@ private static void addSkyframeAwareDepsAndGetNewlyVisitedArtifactsAndActions(
    * to {@code newlyVisitedArtifacts}, and add any {@link ActionLookupData}s to {@code
    * newlyVisitedActions}.
    */
-  private static void addPropagatingActionDepsAndGetNewlyVisitedArtifactsAndActions(
+  private void addPropagatingActionDepsAndGetNewlyVisitedArtifactsAndActions(
       MutableGraph<SkyKey> rewindGraph,
       ActionLookupData actionKey,
       Action action,
@@ -499,7 +504,7 @@ private static void addPropagatingActionDepsAndGetNewlyVisitedArtifactsAndAction
     // 2) the set of actions that "mayInsensitivelyPropagateInputs",
     // should have no overlap. Log a bug report if we see such an action:
     if (action.discoversInputs()) {
-      BugReport.sendBugReport(
+      bugReporter.sendBugReport(
           new IllegalStateException(
               String.format(
                   "Action insensitively propagates and discovers inputs. actionKey: %s, action: "

src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java

@@ -339,7 +339,7 @@ protected void finalizeBuildResult(BuildResult result) {
    *
    * <p>The server is reinitialized so that this change is picked up.
    */
-  protected final RecordingBugReporter recordBugReportsAndReinitialize() throws Exception {
+  public final RecordingBugReporter recordBugReportsAndReinitialize() throws Exception {
     RecordingBugReporter recordingBugReporter = new RecordingBugReporter();
     setCustomBugReporterAndReinitialize(recordingBugReporter);
     return recordingBugReporter;
@@ -1092,7 +1092,7 @@ public final void assertContainsError(String expectedError) {
   }
 
   /** {@link BugReporter} that stores bug reports for later inspection. */
-  protected static class RecordingBugReporter implements BugReporter {
+  public static final class RecordingBugReporter implements BugReporter {
     @GuardedBy("this")
     private final List<Throwable> exceptions = new ArrayList<>();
 

src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java

@@ -251,7 +251,7 @@ protected BuilderWithResult createBuilder(
                 .put(
                     SkyFunctions.ACTION_EXECUTION,
                     new ActionExecutionFunction(
-                        new ActionRewindStrategy(),
+                        new ActionRewindStrategy(BugReporter.defaultInstance()),
                         skyframeActionExecutor,
                         evaluatorRef::get,
                         directories,

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java

@@ -20,7 +20,6 @@
 import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
-import static com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.assertAndClearBugReporterStoredCrash;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContentAsLatin1;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.junit.Assert.assertThrows;
@@ -52,11 +51,13 @@
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
+import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase.RecordingBugReporter;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.ArtifactNestedSetKey;
 import com.google.devtools.build.lib.exec.SpawnExecException;
 import com.google.devtools.build.lib.runtime.KeepGoingOption;
 import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.server.FailureDetails.ActionRewinding;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
 import com.google.devtools.build.lib.testutil.ActionEventRecorder;
@@ -541,21 +542,23 @@ public final void runMultiplyLosingInputsFails() throws Exception {
           });
     }
 
+    RecordingBugReporter bugReporter = testCase.recordBugReportsAndReinitialize();
     List<SkyKey> rewoundKeys = collectOrderedRewoundKeys();
-    RuntimeException e =
-        assertThrows(RuntimeException.class, () -> testCase.buildTarget("//test:rule2"));
-    IllegalStateException illegalStateException = (IllegalStateException) e.getCause();
-    // Command thread is not interrupted because crash propagates up and is finally processed on
-    // command thread.
-    assertThat(Thread.currentThread().isInterrupted()).isFalse();
+    BuildFailedException e =
+        assertThrows(BuildFailedException.class, () -> testCase.buildTarget("//test:rule2"));
+    assertThat(e.getDetailedExitCode().getFailureDetail().getActionRewinding().getCode())
+        .isEqualTo(ActionRewinding.Code.LOST_INPUT_TOO_MANY_TIMES);
 
     String errorDetail =
         String.format(
             "lost input too many times (#%s) for the same action. lostInput: %s, "
                 + "lostInput digest: fakedigest, "
                 + "failedAction: action 'Executing genrule //test:rule2'",
             ActionRewindStrategy.MAX_REPEATED_LOST_INPUTS + 1, intermediate.get());
-    assertThat(illegalStateException).hasMessageThat().contains(errorDetail);
+    assertThat(e.getDetailedExitCode().getFailureDetail().getMessage()).contains(errorDetail);
+    assertThat(Iterables.getOnlyElement(bugReporter.getExceptions()))
+        .hasMessageThat()
+        .contains(errorDetail);
 
     assertThat(getExecutedSpawnDescriptions())
         .containsExactlyElementsIn(
@@ -587,7 +590,6 @@ public final void runMultiplyLosingInputsFails() throws Exception {
     assertOnlyActionsRewound(rewoundKeys);
     assertThat(Iterables.frequency(rewoundArtifactOwnerLabels(rewoundKeys), "//test:rule1"))
         .isEqualTo(ActionRewindStrategy.MAX_REPEATED_LOST_INPUTS);
-    assertAndClearBugReporterStoredCrash(RuntimeException.class);
   }
 
   /**