publish target summaries after vacuous test summaries

target summaries must publish after test summaries but can't know a priori when test summaries are vacuous and therefore not published to BEP. Without this change, BEP can contain extraneous "aborted" test summary events when setting --experimental_bep_target_summary

PiperOrigin-RevId: 367263589

src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java

@@ -369,6 +369,7 @@ public void beforeCommand(CommandEnvironment cmdEnv) {
         new BuildEventStreamer.Builder()
             .buildEventTransports(bepTransports)
             .besStreamOptions(besStreamOptions)
+            .publishTargetSummaries(bepOptions.publishTargetSummary)
             .artifactGroupNamer(artifactGroupNamer)
             .oomMessage(parsingResult.getOptions(CommonCommandOptions.class).oomMessage)
             .build();

src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java

@@ -82,8 +82,16 @@
  */
 @ThreadSafe
 public class BuildEventStreamer {
+  /** Return value for {@link #routeBuildEvent}. */
+  private enum RetentionDecision {
+    BUFFERED,
+    DISCARD,
+    POST
+  }
+
   private final Collection<BuildEventTransport> transports;
   private final BuildEventStreamOptions besOptions;
+  private final boolean publishTargetSummaries;
 
   @GuardedBy("this")
   private Set<BuildEventId> announcedEvents;
@@ -158,10 +166,12 @@ public interface OutErrProvider {
   private BuildEventStreamer(
       Collection<BuildEventTransport> transports,
       BuildEventStreamOptions options,
+      boolean publishTargetSummaries,
       CountingArtifactGroupNamer artifactGroupNamer,
       String oomMessage) {
     this.transports = transports;
     this.besOptions = options;
+    this.publishTargetSummaries = publishTargetSummaries;
     this.announcedEvents = null;
     this.progressCount = 0;
     this.artifactGroupNamer = artifactGroupNamer;
@@ -438,8 +448,17 @@ public void buildEvent(BuildEvent event) {
       }
     }
 
-    if (shouldIgnoreBuildEvent(event)) {
-      return;
+    switch (routeBuildEvent(event)) {
+      case DISCARD:
+        // Check if there are pending events waiting on this event
+        maybePostPendingEventsBeforeDiscarding(event);
+        return; // bail: we're dropping this event
+      case BUFFERED:
+        // Bail: the event was buffered and the BuildEventStreamer is now responsible for eventually
+        // posting (or discarding) it
+        return;
+      case POST:
+        break; // proceed
     }
 
     if (event instanceof BuildStartingEvent) {
@@ -520,6 +539,38 @@ private static boolean isIncomplete(BuildCompleteEvent event) {
         && event.getResult().getStopOnFirstFailure();
   }
 
+  /**
+   * Given an event that will be discarded (not buffered), publishes any events waiting on the given
+   * event.
+   *
+   * @param event event that is being discarded (not buffered)
+   */
+  private void maybePostPendingEventsBeforeDiscarding(BuildEvent event) {
+    if (publishTargetSummaries && isVacuousTestSummary(event)) {
+      // Target summaries should "post after" test summaries, but we can't a priori know whether
+      // test summaries will be vacuous (as that depends on test execution progress). So check for
+      // and publish any pending (target summary) events here. If we don't do this then
+      // clearPendingEvents() will publish "aborted" test_summary events for the very events we're
+      // discarding here (b/184580877), followed by the pending target_summary events, which is not
+      // only confusing but also delays target_summary events until the end of the build.
+      //
+      // Technically it seems we should do this with all events we're dropping but that would be
+      // a lot of extra locking e.g. for every ActionExecutedEvent and it's only necessary to
+      // check for this where events are configured to "post after" events that may be discarded.
+      BuildEventId eventId = event.getEventId();
+      Collection<BuildEvent> toReconsider;
+      synchronized (this) {
+        toReconsider = pendingEvents.removeAll(eventId);
+        // Pretend we posted this event so a target summary arriving after this test summary (which
+        // is common) doesn't get erroneously buffered in bufferUntilPrerequisitesReceived().
+        postedEvents.add(eventId);
+      }
+      for (BuildEvent freedEvent : toReconsider) {
+        buildEvent(freedEvent);
+      }
+    }
+  }
+
   private synchronized BuildEvent flushStdoutStderrEvent(String out, String err) {
     BuildEvent updateEvent = ProgressEvent.progressUpdate(progressCount, out, err);
     progressCount++;
@@ -657,21 +708,27 @@ private synchronized void buildComplete(ChainableEvent event) {
     }
   }
 
-  /** Returns whether a {@link BuildEvent} should be ignored. */
-  private boolean shouldIgnoreBuildEvent(BuildEvent event) {
+  /** Returns whether a {@link BuildEvent} should be ignored or was buffered. */
+  private RetentionDecision routeBuildEvent(BuildEvent event) {
     if (event instanceof ActionExecutedEvent
         && !shouldPublishActionExecutedEvent((ActionExecutedEvent) event)) {
-      return true;
+      return RetentionDecision.DISCARD;
     }
 
-    if (bufferUntilPrerequisitesReceived(event) || isVacuousTestSummary(event)) {
-      return true;
+    if (bufferUntilPrerequisitesReceived(event)) {
+      return RetentionDecision.BUFFERED;
+    }
+
+    if (isVacuousTestSummary(event)) {
+      return RetentionDecision.DISCARD;
     }
 
     if (isTestCommand && event instanceof BuildCompleteEvent) {
       // In case of "bazel test" ignore the BuildCompleteEvent, as it will be followed by a
       // TestingCompleteEvent that contains the correct exit code.
-      return !isCrash((BuildCompleteEvent) event);
+      return isCrash((BuildCompleteEvent) event)
+          ? RetentionDecision.POST
+          : RetentionDecision.DISCARD;
     }
 
     if (event instanceof TargetParsingCompleteEvent) {
@@ -680,11 +737,13 @@ private boolean shouldIgnoreBuildEvent(BuildEvent event) {
       // TODO(b/109727414): This is brittle. It would be better to always post one PatternExpanded
       // event for each pattern given on the command line instead of one event for all of them
       // combined.
-      return ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1
-          && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty();
+      boolean discard =
+          ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1
+              && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty();
+      return discard ? RetentionDecision.DISCARD : RetentionDecision.POST;
     }
 
-    return false;
+    return RetentionDecision.POST;
   }
 
   /** Returns whether an {@link ActionExecutedEvent} should be published. */
@@ -783,6 +842,7 @@ private synchronized String getAbortReasonDetails() {
   public static final class Builder {
     private Set<BuildEventTransport> buildEventTransports;
     private BuildEventStreamOptions besStreamOptions;
+    private boolean publishTargetSummaries;
     private CountingArtifactGroupNamer artifactGroupNamer;
     private String oomMessage;
 
@@ -796,6 +856,11 @@ public Builder besStreamOptions(BuildEventStreamOptions value) {
       return this;
     }
 
+    public Builder publishTargetSummaries(boolean publishTargetSummaries) {
+      this.publishTargetSummaries = publishTargetSummaries;
+      return this;
+    }
+
     public Builder artifactGroupNamer(CountingArtifactGroupNamer value) {
       this.artifactGroupNamer = value;
       return this;
@@ -810,6 +875,7 @@ public BuildEventStreamer build() {
       return new BuildEventStreamer(
           checkNotNull(buildEventTransports),
           checkNotNull(besStreamOptions),
+          publishTargetSummaries,
           checkNotNull(artifactGroupNamer),
           nullToEmpty(oomMessage));
     }

src/test/shell/integration/build_event_stream_test.sh

@@ -755,7 +755,7 @@ function test_aspect_analysis_failure_no_target_summary() {
   expect_log_once '^completed '  # target completes due to -k
   # One "aborted" for failed aspect analysis, another for target_summary_id
   # announced by "completed" event asserted above
-  expect_log_n '^aborted ' 2
+  expect_log_n 'aborted' 2
   expect_not_log '^target_summary '  # no summary due to analysis failure
 }
 
@@ -1063,7 +1063,7 @@ function test_test_fails_to_build() {
   (bazel test --experimental_bep_target_summary \
          --build_event_text_file=$TEST_log \
          pkg:test_that_fails_to_build && fail "test failure expected") || true
-  expect_not_log '^test_summary'
+  expect_not_log 'test_summary'  # no test_summary events or references to them
   expect_log_once '^target_summary '
   expect_not_log 'overall_build_success'
   expect_log 'last_message: true'