ParallelEvaluator: report events early for cache hits

In the case that a node is already done when evaluation starts, we now report
events and postables early, rather than waiting until the end of evaluation.
This makes reporting more timely, and ensures reporting even if the evaluation
is interrupted.

This caused a problem with moving the TargetCompleteEvent into Skyframe
(unknown commit). I added two unit tests at the Skyframe level to cover the
guarantees that we need for that.

Note that the replay call in constructResult can duplicate the events from
a cache hit - this is not a problem since the replaying visitor automatically
removes duplicates (and it wasn't obvious which keys correspond to cache
hits).

PiperOrigin-RevId: 179788157

3 files changed, 86 insertions, 13 deletions

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 27121bf54d..95cf3c0219 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -136,6 +136,13 @@ public class ParallelEvaluator extends AbstractParallelEvaluator implements Eval
           "%s should be at most %s in the version partial ordering",
           valueVersion,
           evaluatorContext.getGraphVersion());
+
+      if (value != null) {
+        ValueWithMetadata valueWithMetadata =
+            ValueWithMetadata.wrapWithMetadata(entry.getValueMaybeWithMetadata());
+        replay(valueWithMetadata);
+      }
+
       // For most nodes we do not inform the progress receiver if they were already done when we
       // retrieve them, but top-level nodes are presumably of more interest.
       // If valueVersion is not equal to graphVersion, it must be less than it (by the
@@ -491,6 +498,17 @@ public class ParallelEvaluator extends AbstractParallelEvaluator implements Eval
     return bubbleErrorInfo;
   }
 
+  private void replay(ValueWithMetadata valueWithMetadata) {
+    // TODO(bazel-team): Verify that message replay is fast and works in failure
+    // modes [skyframe-core]
+    evaluatorContext
+        .getReplayingNestedSetPostableVisitor()
+        .visit(valueWithMetadata.getTransitivePostables());
+    evaluatorContext
+        .getReplayingNestedSetEventVisitor()
+        .visit(valueWithMetadata.getTransitiveEvents());
+  }
+
   /**
    * Constructs an {@link EvaluationResult} from the {@link #graph}. Looks for cycles if there are
    * unfinished nodes but no error was already found through bubbling up (as indicated by {@code
@@ -529,17 +547,9 @@ public class ParallelEvaluator extends AbstractParallelEvaluator implements Eval
         }
         continue;
       }
+      // Replaying here is necessary for error bubbling and other cases.
+      replay(valueWithMetadata);
       SkyValue value = valueWithMetadata.getValue();
-      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
-      // would have already printed the transitive messages after building these values.
-      evaluatorContext
-          .getReplayingNestedSetEventVisitor()
-          .visit(valueWithMetadata.getTransitiveEvents());
       ErrorInfo errorInfo = valueWithMetadata.getErrorInfo();
       Preconditions.checkState(value != null || errorInfo != null, skyKey);
       if (!evaluatorContext.keepGoing() && errorInfo != null) {
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 bd4c322295..eb25426a4f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -103,6 +103,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
         @Override
         @SuppressWarnings("UnsynchronizedOverridesSynchronized") // only delegates to thread-safe.
         public void post(ExtendedEventHandler.Postable e) {
+          checkActive();
           if (e instanceof ExtendedEventHandler.ProgressLike) {
             evaluatorContext.getReporter().post(e);
           } else {
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 af09fcd1ea..558c5edab3 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -427,7 +427,7 @@ public class ParallelEvaluatorTest {
     eval(false, a);
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     storedEventHandler.clear();
-    // Build top. The warning from a should be reprinted.
+    // Build top. The warning from a should be printed.
     eval(false, top);
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     storedEventHandler.clear();
@@ -448,16 +448,78 @@ public class ParallelEvaluatorTest {
     eval(false, a);
     assertThat(storedEventHandler.getPosts()).containsExactly(post);
     storedEventHandler.clear();
-    // Build top. The warning from a should be reprinted.
+    // Build top. The post from a should be printed.
     eval(false, top);
     assertThat(storedEventHandler.getPosts()).containsExactly(post);
     storedEventHandler.clear();
-    // Build top again. The warning should have been stored in the value.
+    // Build top again. The post should have been stored in the value.
     eval(false, top);
     assertThat(storedEventHandler.getPosts()).containsExactly(post);
   }
 
   @Test
+  public void eventReportedTimely() throws Exception {
+    graph = new InMemoryGraphImpl();
+    set("a", "a").setWarning("warning on 'a'");
+    SkyKey a = GraphTester.toSkyKey("a");
+    SkyKey top = GraphTester.toSkyKey("top");
+    tester.getOrCreate(top).setBuilder(new SkyFunction() {
+      @Override
+      public SkyValue compute(SkyKey key, Environment env)
+          throws SkyFunctionException, InterruptedException {
+        // The event from a should already have been posted.
+        assertThat(storedEventHandler.getEvents()).hasSize(1);
+        return new StringValue("foo");
+      }
+
+      @Override
+      @Nullable
+      public String extractTag(SkyKey skyKey) {
+        return null;
+      }
+    });
+    // Build a so that it is already in the graph.
+    eval(false, a);
+    storedEventHandler.clear();
+    // Build top. The warning from a should be printed before evaluating top.
+    eval(false, ImmutableList.of(a, top));
+    assertThat(storedEventHandler.getEvents()).hasSize(1);
+    storedEventHandler.clear();
+  }
+
+  @Test
+  public void errorOfTopLevelTargetReported() throws Exception {
+    graph = new InMemoryGraphImpl();
+    SkyKey a = GraphTester.toSkyKey("a");
+    SkyKey b = GraphTester.toSkyKey("b");
+    tester.getOrCreate(b).setHasError(true);
+    Event errorEvent = Event.error("foobar");
+    tester.getOrCreate(a).setBuilder(new SkyFunction() {
+      @Override
+      public SkyValue compute(SkyKey key, Environment env)
+          throws SkyFunctionException, InterruptedException {
+        try {
+          if (env.getValueOrThrow(b, SomeErrorException.class) == null) {
+            return null;
+          }
+        } catch (SomeErrorException ignored) {
+          // Continue silently.
+        }
+        env.getListener().handle(errorEvent);
+        throw new SkyFunctionException(new SomeErrorException("bazbar"), Transience.PERSISTENT) {};
+      }
+
+      @Override
+      @Nullable
+      public String extractTag(SkyKey skyKey) {
+        return null;
+      }
+    });
+    eval(false, a);
+    assertThat(storedEventHandler.getEvents()).containsExactly(errorEvent);
+  }
+
+  @Test
   public void storedEventFilter() throws Exception {
     graph = new InMemoryGraphImpl();
     SkyKey a = GraphTester.toSkyKey("a");