Do not record events announced after the build-completing event.

Doing so results in a race condition: last_message is set when there are no events that are announced but not posted. Progress events always announce the subsequent progress event so if there is a progress event after the build-completing event, last_message is never set.

The UI is updated on a separate thread and writing to the console results in a progress event so the above scenario is entirely possible.

RELNOTES: None.
PiperOrigin-RevId: 636810448
Change-Id: I86e1b9d37b338b3d574a6e27a7d72e81a6ecdffa

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

@@ -130,8 +130,8 @@ private enum RetentionDecision {
   // After #buildComplete is called, contains the set of events that the streamer is expected to
   // process. The streamer will fully close after seeing them. This field is null until
   // #buildComplete is called.
-  // Thread-safety note: finalEventsToCome is only non-null in the final, sequential phase of the
-  // build (all final events are issued from the main thread).
+  // Thread-safety note: in the final, sequential phase of the build, we ignore any events that are
+  // announced by events posted after #buildComplete is called.
   private Set<BuildEventId> finalEventsToCome = null;
 
   // True, if we already closed the stream.
@@ -189,6 +189,24 @@ public void registerOutErrProvider(OutErrProvider outErrProvider) {
     this.outErrProvider = outErrProvider;
   }
 
+  // This exists to nop out the announcement of new events after #buildComplete
+  private synchronized void maybeRegisterAnnouncedEvent(BuildEventId id) {
+    if (finalEventsToCome != null) {
+      return;
+    }
+
+    announcedEvents.add(id);
+  }
+
+  // This exists to nop out the announcement of new events after #buildComplete
+  private synchronized void maybeRegisterAnnouncedEvents(Collection<BuildEventId> ids) {
+    if (finalEventsToCome != null) {
+      return;
+    }
+
+    announcedEvents.addAll(ids);
+  }
+
   /**
    * Post a new event to all transports; simultaneously keep track of the events we announce to
    * still come.
@@ -211,15 +229,15 @@ private void post(BuildEvent event) {
         // The very first event of a stream is implicitly announced by the convention that
         // a complete stream has to have at least one entry. In this way we keep the invariant
         // that the set of posted events is always a subset of the set of announced events.
-        announcedEvents.add(id);
+        maybeRegisterAnnouncedEvent(id);
         if (!event.getChildrenEvents().contains(ProgressEvent.INITIAL_PROGRESS_UPDATE)) {
           BuildEvent progress = ProgressEvent.progressChainIn(progressCount, event.getEventId());
           linkEvents = ImmutableList.of(progress);
           progressCount++;
-          announcedEvents.addAll(progress.getChildrenEvents());
+          maybeRegisterAnnouncedEvents(progress.getChildrenEvents());
           // the new first event in the stream, implicitly announced by the fact that complete
           // stream may not be empty.
-          announcedEvents.add(progress.getEventId());
+          maybeRegisterAnnouncedEvent(progress.getEventId());
           postedEvents.add(progress.getEventId());
         }
 
@@ -248,7 +266,7 @@ private void post(BuildEvent event) {
                     ProgressEvent.progressChainIn(progressCount, id, out, err);
                 finalLinkEvents.add(progressEvent);
                 progressCount++;
-                announcedEvents.addAll(progressEvent.getChildrenEvents());
+                maybeRegisterAnnouncedEvents(progressEvent.getChildrenEvents());
                 postedEvents.add(progressEvent.getEventId());
               });
         }
@@ -263,7 +281,7 @@ private void post(BuildEvent event) {
       }
 
       postedEvents.add(id);
-      announcedEvents.addAll(event.getChildrenEvents());
+      maybeRegisterAnnouncedEvents(event.getChildrenEvents());
       // We keep as an invariant that postedEvents is a subset of announced events, so this is a
       // cheaper test for equality
       if (announcedEvents.size() == postedEvents.size()) {
@@ -590,7 +608,7 @@ private void maybePostPendingEventsBeforeDiscarding(BuildEvent event) {
   private synchronized BuildEvent flushStdoutStderrEvent(String out, String err) {
     BuildEvent updateEvent = ProgressEvent.progressUpdate(progressCount, out, err);
     progressCount++;
-    announcedEvents.addAll(updateEvent.getChildrenEvents());
+    maybeRegisterAnnouncedEvents(updateEvent.getChildrenEvents());
     postedEvents.add(updateEvent.getEventId());
     return updateEvent;
   }

src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java

@@ -1194,6 +1194,25 @@ public void testEarlyAbort() {
     assertThat(transport.getEventProtos().get(3).getLastMessage()).isTrue();
   }
 
+  @Test
+  public void testEventAfterBuildCompleteEvent() {
+    BuildEventId lateId = testId("late");
+    BuildEvent startEvent =
+        new GenericBuildEvent(
+            testId("initial"),
+            ImmutableSet.of(
+                ProgressEvent.INITIAL_PROGRESS_UPDATE, BuildEventIdUtil.buildFinished()));
+    BuildEvent lateEvent = new GenericBuildEvent(lateId, ImmutableSet.of(testId("nonexistent")));
+    BuildEvent finishedEvent = new BuildCompleteEvent(new BuildResult(0), ImmutableList.of(lateId));
+
+    streamer.buildEvent(startEvent);
+    streamer.buildEvent(finishedEvent);
+    streamer.buildEvent(lateEvent);
+    assertThat(streamer.isClosed()).isTrue();
+    assertThat(transport.getEventProtos()).hasSize(4);
+    assertThat(transport.getEventProtos().get(3).getLastMessage()).isTrue();
+  }
+
   @Test
   public void testFinalEventsLate() {
     // Verify that we correctly handle late events (i.e., events coming only after the