From 5cdb9e16dc44dc3cd868827659784c237f7ff42a Mon Sep 17 00:00:00 2001
From: Googler <leba@google.com>
Date: Tue, 10 Oct 2023 01:05:46 -0700
Subject: [PATCH] Ensure that pending events in BuildEventStreamer are resolved
 in FIFO order.

1. `TestAttempt` events would wait for the `TargetCompleteEvent` to be posted before being posted.

2. There was an implicit requirement for the `TestAttempt` events to be posted in a specific order.

3. This didn't break in the noskymeld case because we fulfilled this ordering by using the order of performing the attempts themselves. The sequence would look like:
    + post `TargetCompleteEvent`
    -> perform attempt #1
    -> post `TestAttempt` #1
    -> perform attempt #2
    -> post `TestAttempt` #2

4. With skymeld, however, it could happen like this:

    + defer `TargetCompleteEvent` to wait for `CoverageActionFinishedEvent`
    + perform attempt #1 -> defer posting `TestAttempt` #1 & wait for `TargetCompleteEvent`
    + perform attempt #2 -> defer posting `TestAttempt` #2 & wait for `TargetCompleteEvent`
    + `CoverageActionFinishedEvent` -> release & post `TargetCompleteEvent`
    + `TargetCompleteEvent` -> release & post `TestAttempt` #2
    + `TargetCompleteEvent` -> release & post `TestAttempt` #1

Due to (2), the undefined ordering in (4) would cause an issue.

This CL fixes that by ensuring a FIFO ordering of the deferred events.

PiperOrigin-RevId: 572165337
Change-Id: Iac4d023d946865b8b81f15b119417192dc4b5c53
---
 .../build/lib/runtime/BuildEventStreamer.java | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
index 778e7b4d2f7183..4a291c37036042 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
@@ -18,12 +18,12 @@
 import static com.google.common.base.Strings.nullToEmpty;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Multimap;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.common.eventbus.AllowConcurrentEvents;
 import com.google.common.eventbus.Subscribe;
@@ -106,8 +106,11 @@ private enum RetentionDecision {
   @GuardedBy("this")
   private List<Pair<String, String>> bufferedStdoutStderrPairs = new ArrayList<>();
 
+  // Use LinkedHashMultimap to maintain a FIFO ordering of pending events.
+  // This is important in case of Skymeld, so that the TestAttempt events are resolved in the
+  // correct order.
   @GuardedBy("this")
-  private final Multimap<BuildEventId, BuildEvent> pendingEvents = HashMultimap.create();
+  private final SetMultimap<BuildEventId, BuildEvent> pendingEvents = LinkedHashMultimap.create();
 
   @GuardedBy("this")
   private int progressCount;
@@ -493,11 +496,11 @@ public void buildEvent(BuildEvent event) {
     }
 
     // Reconsider all events blocked by the event just posted.
-    Collection<BuildEvent> toReconsider;
+    Set<BuildEvent> blockedEventsFifo;
     synchronized (this) {
-      toReconsider = pendingEvents.removeAll(event.getEventId());
+      blockedEventsFifo = pendingEvents.removeAll(event.getEventId());
     }
-    for (BuildEvent freedEvent : toReconsider) {
+    for (BuildEvent freedEvent : blockedEventsFifo) {
       buildEvent(freedEvent);
     }
 
@@ -558,14 +561,14 @@ private void maybePostPendingEventsBeforeDiscarding(BuildEvent event) {
       // 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;
+      Set<BuildEvent> blockedEventsFifo;
       synchronized (this) {
-        toReconsider = pendingEvents.removeAll(eventId);
+        blockedEventsFifo = 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) {
+      for (BuildEvent freedEvent : blockedEventsFifo) {
         buildEvent(freedEvent);
       }
     }