Fix Postable forwarding and replay

We were previously duplicate-posting Postable events posted to the
Skyframe environment.

PiperOrigin-RevId: 162323598

6 files changed, 98 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index 6bd01b538f..568e08f01d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadHostile;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import java.io.PrintStream;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -165,5 +166,15 @@ public interface MemoizingEvaluator {
    * {@code EmittedEventState} first and pass it to the graph during creation. This allows them to
    * determine whether or not to replay events.
    */
-  class EmittedEventState extends NestedSetVisitor.VisitedState<TaggedEvents> {}
+  class EmittedEventState {
+    final NestedSetVisitor.VisitedState<TaggedEvents> eventState =
+        new NestedSetVisitor.VisitedState<>();
+    final NestedSetVisitor.VisitedState<Postable> postableState =
+        new NestedSetVisitor.VisitedState<>();
+
+    public void clear() {
+      eventState.clear();
+      postableState.clear();
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index beb4a2548b..aac9277a1d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -23,7 +23,6 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.util.BlazeClock;
@@ -1088,9 +1087,9 @@ public final class ParallelEvaluator implements Evaluator {
         continue;
       }
       SkyValue value = valueWithMetadata.getValue();
-      for (Postable post : valueWithMetadata.getTransitivePostables()) {
-        evaluatorContext.getReporter().post(post);
-      }
+      evaluatorContext
+          .getReplayingNestedSetPostableVisitor()
+          .visit(valueWithMetadata.getTransitivePostables());
       // TODO(bazel-team): Verify that message replay is fast and works in failure
       // modes [skyframe-core]
       // Note that replaying events here is only necessary on null builds, because otherwise we
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
index 2f585ff679..9bafd6ebf7 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState;
 import com.google.devtools.build.skyframe.QueryableGraph.Reason;
@@ -46,6 +47,7 @@ class ParallelEvaluatorContext {
   private final ImmutableMap<SkyFunctionName, ? extends SkyFunction> skyFunctions;
   private final ExtendedEventHandler reporter;
   private final NestedSetVisitor<TaggedEvents> replayingNestedSetEventVisitor;
+  private final NestedSetVisitor<Postable> replayingNestedSetPostableVisitor;
   private final boolean keepGoing;
   private final DirtyTrackingProgressReceiver progressReceiver;
   private final EventFilter storedEventFilter;
@@ -76,7 +78,10 @@ class ParallelEvaluatorContext {
     this.skyFunctions = skyFunctions;
     this.reporter = reporter;
     this.replayingNestedSetEventVisitor =
-        new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState);
+        new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState.eventState);
+    this.replayingNestedSetPostableVisitor =
+        new NestedSetVisitor<>(
+            new NestedSetPostableReceiver(reporter), emittedEventState.postableState);
     this.keepGoing = keepGoing;
     this.progressReceiver = Preconditions.checkNotNull(progressReceiver);
     this.storedEventFilter = storedEventFilter;
@@ -109,7 +114,10 @@ class ParallelEvaluatorContext {
     this.skyFunctions = skyFunctions;
     this.reporter = reporter;
     this.replayingNestedSetEventVisitor =
-        new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState);
+        new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState.eventState);
+    this.replayingNestedSetPostableVisitor =
+        new NestedSetVisitor<>(
+            new NestedSetPostableReceiver(reporter), emittedEventState.postableState);
     this.keepGoing = keepGoing;
     this.progressReceiver = Preconditions.checkNotNull(progressReceiver);
     this.storedEventFilter = storedEventFilter;
@@ -192,6 +200,10 @@ class ParallelEvaluatorContext {
     return replayingNestedSetEventVisitor;
   }
 
+  NestedSetVisitor<Postable> getReplayingNestedSetPostableVisitor() {
+    return replayingNestedSetPostableVisitor;
+  }
+
   ExtendedEventHandler getReporter() {
     return reporter;
   }
@@ -225,4 +237,19 @@ class ParallelEvaluatorContext {
       }
     }
   }
+
+  /** Receives the postables from the NestedSet and delegates to the reporter. */
+  private static class NestedSetPostableReceiver implements NestedSetVisitor.Receiver<Postable> {
+
+    private final ExtendedEventHandler reporter;
+
+    public NestedSetPostableReceiver(ExtendedEventHandler reporter) {
+      this.reporter = reporter;
+    }
+
+    @Override
+    public void accept(Postable post) {
+      reporter.post(post);
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
index 427ec944c9..fb51fdd789 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -387,9 +387,9 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
         if (bubbleErrorInfo == null) {
           addDep(depKey);
         }
-        for (Postable post : ValueWithMetadata.getPosts(depValue)) {
-          evaluatorContext.getReporter().post(post);
-        }
+        evaluatorContext
+            .getReplayingNestedSetPostableVisitor()
+            .visit(ValueWithMetadata.getPosts(depValue));
         evaluatorContext
             .getReplayingNestedSetEventVisitor()
             .visit(ValueWithMetadata.getEvents(depValue));
@@ -533,9 +533,6 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
 
     NestedSet<Postable> posts = buildPosts(primaryEntry);
     NestedSet<TaggedEvents> events = buildEvents(primaryEntry, /*missingChildren=*/ false);
-    for (ExtendedEventHandler.Postable post : posts) {
-      evaluatorContext.getReporter().post(post);
-    }
 
     Version valueVersion;
     SkyValue valueWithMetadata;
@@ -592,9 +589,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     evaluatorContext.signalValuesAndEnqueueIfReady(
         skyKey, reverseDeps, valueVersion, enqueueParents);
 
-    for (Postable post : posts) {
-      evaluatorContext.getReporter().post(post);
-    }
+    evaluatorContext.getReplayingNestedSetPostableVisitor().visit(posts);
     evaluatorContext.getReplayingNestedSetEventVisitor().visit(events);
   }
 
diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
index 455a36ec44..75ebadf4c9 100644
--- a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
+++ b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -103,6 +104,9 @@ public class GraphTester {
         if (builder.progress != null) {
           env.getListener().handle(Event.progress(builder.progress));
         }
+        if (builder.postable != null) {
+          env.getListener().post(builder.postable);
+        }
         Map<SkyKey, SkyValue> deps = new LinkedHashMap<>();
         boolean oneMissing = false;
         for (Pair<SkyKey, SkyValue> dep : builder.deps) {
@@ -175,6 +179,7 @@ public class GraphTester {
 
     private String warning;
     private String progress;
+    private Postable postable;
 
     private String tag;
 
@@ -260,6 +265,10 @@ public class GraphTester {
       return this;
     }
 
+    public TestFunction setPostable(Postable postable) {
+      this.postable = postable;
+      return this;
+    }
   }
 
   public static ImmutableList<SkyKey> toSkyKeys(String... names) {
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
index 094f75ac1c..db73be54fe 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -17,8 +17,6 @@ import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertContainsEvent;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents;
 import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
 import static com.google.devtools.build.skyframe.GraphTester.CONCATENATE;
 import static org.junit.Assert.fail;
@@ -33,11 +31,12 @@ import com.google.common.collect.Sets;
 import com.google.common.eventbus.EventBus;
 import com.google.common.util.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventCollector;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.testutil.TestThread;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.skyframe.GraphTester.StringValue;
@@ -70,14 +69,14 @@ public class ParallelEvaluatorTest {
   protected IntVersion graphVersion = IntVersion.of(0);
   protected GraphTester tester = new GraphTester();
 
-  private EventCollector eventCollector;
+  private StoredEventHandler storedEventHandler;
 
   private DirtyTrackingProgressReceiver revalidationReceiver =
       new DirtyTrackingProgressReceiver(null);
 
   @Before
   public void initializeReporter() {
-    eventCollector = new EventCollector();
+    storedEventHandler = new StoredEventHandler();
   }
 
   @After
@@ -96,7 +95,7 @@ public class ParallelEvaluatorTest {
         graph,
         oldGraphVersion,
         builders,
-        new Reporter(new EventBus(), eventCollector),
+        storedEventHandler,
         new MemoizingEvaluator.EmittedEventState(),
         storedEventFilter,
         ErrorInfoManager.UseChildErrorInfoIfNecessary.INSTANCE,
@@ -143,7 +142,8 @@ public class ParallelEvaluatorTest {
     tester.getOrCreate("ab").addDependency("a").addDependency("b").setComputedValue(CONCATENATE);
     StringValue value = (StringValue) eval(false, GraphTester.toSkyKey("ab"));
     assertThat(value.getValue()).isEqualTo("ab");
-    assertNoEvents(eventCollector);
+    assertThat(storedEventHandler.getEvents()).isEmpty();
+    assertThat(storedEventHandler.getPosts()).isEmpty();
   }
 
   /**
@@ -412,8 +412,8 @@ public class ParallelEvaluatorTest {
     set("a", "a").setWarning("warning on 'a'");
     StringValue value = (StringValue) eval(false, GraphTester.toSkyKey("a"));
     assertThat(value.getValue()).isEqualTo("a");
-    assertContainsEvent(eventCollector, "warning on 'a'");
-    assertEventCount(1, eventCollector);
+    assertContainsEvent(storedEventHandler.getEvents(), "warning on 'a'");
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
   }
 
   /** Regression test: events from already-done value not replayed. */
@@ -426,15 +426,36 @@ public class ParallelEvaluatorTest {
     tester.getOrCreate(top).addDependency(a).setComputedValue(CONCATENATE);
     // Build a so that it is already in the graph.
     eval(false, a);
-    assertEventCount(1, eventCollector);
-    eventCollector.clear();
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
+    storedEventHandler.clear();
     // Build top. The warning from a should be reprinted.
     eval(false, top);
-    assertEventCount(1, eventCollector);
-    eventCollector.clear();
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
+    storedEventHandler.clear();
     // Build top again. The warning should have been stored in the value.
     eval(false, top);
-    assertEventCount(1, eventCollector);
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
+  }
+
+  @Test
+  public void postableFromDoneChildRecorded() throws Exception {
+    graph = new InMemoryGraphImpl();
+    Postable post = new Postable() {};
+    set("a", "a").setPostable(post);
+    SkyKey a = GraphTester.toSkyKey("a");
+    SkyKey top = GraphTester.toSkyKey("top");
+    tester.getOrCreate(top).addDependency(a).setComputedValue(CONCATENATE);
+    // Build a so that it is already in the graph.
+    eval(false, a);
+    assertThat(storedEventHandler.getPosts()).containsExactly(post);
+    storedEventHandler.clear();
+    // Build top. The warning from a should be reprinted.
+    eval(false, top);
+    assertThat(storedEventHandler.getPosts()).containsExactly(post);
+    storedEventHandler.clear();
+    // Build top again. The warning should have been stored in the value.
+    eval(false, top);
+    assertThat(storedEventHandler.getPosts()).containsExactly(post);
   }
 
   @Test
@@ -476,16 +497,16 @@ public class ParallelEvaluatorTest {
             });
     evaluator.eval(ImmutableList.of(a));
     assertThat(evaluated.get()).isTrue();
-    assertEventCount(2, eventCollector);
-    assertContainsEvent(eventCollector, "boop");
-    assertContainsEvent(eventCollector, "beep");
-    eventCollector.clear();
+    assertThat(storedEventHandler.getEvents()).hasSize(2);
+    assertContainsEvent(storedEventHandler.getEvents(), "boop");
+    assertContainsEvent(storedEventHandler.getEvents(), "beep");
+    storedEventHandler.clear();
     evaluator = makeEvaluator(graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false);
     evaluated.set(false);
     evaluator.eval(ImmutableList.of(a));
     assertThat(evaluated.get()).isFalse();
-    assertEventCount(1, eventCollector);
-    assertContainsEvent(eventCollector, "boop");
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
+    assertContainsEvent(storedEventHandler.getEvents(), "boop");
   }
 
   @Test