Report what RemoteSpawnCache is doing.

Post ProgressStatus.CHECKING_CACHE if RemoteSpawnCache is checking the cache.

The UI sees CHECKING_CACHE exactly the same as EXECUTING because no UIs
currently have any special behavior for actions in cache-lookup state. This is
still a UX improvement with --experimental_spawn_cache because EXECUTING is
generally more correct than the old action state, which varies from harmless but
unhelpful (no known state) to just wrong (C++ compile actions claimed they were
doing include scanning during cache lookups).

Closes #5130.

Change-Id: I77421c3667c180875216f937fe0713f0e9415a7a
PiperOrigin-RevId: 195233123

4 files changed, 54 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index d9ae236622..d8c42ff140 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -252,6 +252,7 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte
       EventBus eventBus = actionExecutionContext.getEventBus();
       switch (state) {
         case EXECUTING:
+        case CHECKING_CACHE:
           eventBus.post(ActionStatusMessage.runningStrategy(action, name));
           break;
         case SCHEDULING:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index 72766ced3e..2e31f83b48 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.SpawnCache;
+import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -91,6 +92,12 @@ final class RemoteSpawnCache implements SpawnCache {
   @Override
   public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       throws InterruptedException, IOException, ExecException {
+    boolean checkCache = options.remoteAcceptCached && Spawns.mayBeCached(spawn);
+
+    if (checkCache) {
+      context.report(ProgressStatus.CHECKING_CACHE, "remote-cache");
+    }
+
     // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
     TreeNodeRepository repository =
         new TreeNodeRepository(execRoot, context.getActionInputFileCache(), digestUtil);
@@ -106,42 +113,42 @@ final class RemoteSpawnCache implements SpawnCache {
             spawn.getExecutionPlatform(),
             context.getTimeout(),
             Spawns.mayBeCached(spawn));
-
     // Look up action cache, and reuse the action output if it is found.
     final ActionKey actionKey = digestUtil.computeActionKey(action);
+
     Context withMetadata =
         TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey);
-    // Metadata will be available in context.current() until we detach.
-    // This is done via a thread-local variable.
-    Context previous = withMetadata.attach();
-    try {
-      ActionResult result =
-          this.options.remoteAcceptCached && Spawns.mayBeCached(spawn)
-              ? remoteCache.getCachedActionResult(actionKey)
-              : null;
-      if (result != null) {
-        // We don't cache failed actions, so we know the outputs exist.
-        // For now, download all outputs locally; in the future, we can reuse the digests to
-        // just update the TreeNodeRepository and continue the build.
-        remoteCache.download(result, execRoot, context.getFileOutErr());
-        SpawnResult spawnResult =
-            new SpawnResult.Builder()
-                .setStatus(Status.SUCCESS)
-                .setExitCode(result.getExitCode())
-                .setCacheHit(true)
-                .setRunnerName("remote cache hit")
-                .build();
-        return SpawnCache.success(spawnResult);
+
+    if (checkCache) {
+      // Metadata will be available in context.current() until we detach.
+      // This is done via a thread-local variable.
+      Context previous = withMetadata.attach();
+      try {
+        ActionResult result = remoteCache.getCachedActionResult(actionKey);
+        if (result != null) {
+          // We don't cache failed actions, so we know the outputs exist.
+          // For now, download all outputs locally; in the future, we can reuse the digests to
+          // just update the TreeNodeRepository and continue the build.
+          remoteCache.download(result, execRoot, context.getFileOutErr());
+          SpawnResult spawnResult =
+              new SpawnResult.Builder()
+                  .setStatus(Status.SUCCESS)
+                  .setExitCode(result.getExitCode())
+                  .setCacheHit(true)
+                  .setRunnerName("remote cache hit")
+                  .build();
+          return SpawnCache.success(spawnResult);
+        }
+      } catch (CacheNotFoundException e) {
+        // There's a cache miss. Fall back to local execution.
+      } catch (IOException e) {
+        // There's an IO error. Fall back to local execution.
+        reportOnce(
+            Event.warn(
+                "Some artifacts failed to be downloaded from the remote cache: " + e.getMessage()));
+      } finally {
+        withMetadata.detach(previous);
       }
-    } catch (CacheNotFoundException e) {
-      // There's a cache miss. Fall back to local execution.
-    } catch (IOException e) {
-      // There's an IO error. Fall back to local execution.
-      reportOnce(
-          Event.warn(
-              "Some artifacts failed to be downloaded from the remote cache: " + e.getMessage()));
-    } finally {
-      withMetadata.detach(previous);
     }
     if (options.remoteUploadLocalResults) {
       return new CacheHandle() {
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index c626bf2436..fb3d52c6ff 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1237,6 +1237,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:exitcode-external",
         "//src/main/java/com/google/devtools/build/lib:io",
+        "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
         "//src/main/java/com/google/devtools/build/lib/clock",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 997d3dc67d..8baaa26564 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -48,6 +48,7 @@ import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
@@ -61,7 +62,9 @@ import com.google.devtools.remoteexecution.v1test.Command;
 import com.google.devtools.remoteexecution.v1test.RequestMetadata;
 import java.io.IOException;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 import java.util.SortedMap;
 import org.junit.Before;
 import org.junit.Test;
@@ -92,6 +95,7 @@ public class RemoteSpawnCacheTest {
   @Mock private AbstractRemoteActionCache remoteCache;
   private RemoteSpawnCache cache;
   private FileOutErr outErr;
+  private final List<Pair<ProgressStatus, String>> progressUpdates = new ArrayList();
 
   private StoredEventHandler eventHandler = new StoredEventHandler();
 
@@ -146,7 +150,7 @@ public class RemoteSpawnCacheTest {
 
         @Override
         public void report(ProgressStatus state, String name) {
-          // TODO(ulfjack): Test that the right calls are made.
+          progressUpdates.add(Pair.of(state, name));
         }
       };
 
@@ -242,6 +246,8 @@ public class RemoteSpawnCacheTest {
     // We expect the CachedLocalSpawnRunner to _not_ write to outErr at all.
     assertThat(outErr.hasRecordedOutput()).isFalse();
     assertThat(outErr.hasRecordedStderr()).isFalse();
+    assertThat(progressUpdates)
+        .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
 
   @Test
@@ -270,6 +276,8 @@ public class RemoteSpawnCacheTest {
     entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
+    assertThat(progressUpdates)
+        .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
 
   @Test
@@ -299,6 +307,7 @@ public class RemoteSpawnCacheTest {
     entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
+    assertThat(progressUpdates).containsExactly();
   }
 
   @Test
@@ -318,6 +327,8 @@ public class RemoteSpawnCacheTest {
     entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
+    assertThat(progressUpdates)
+        .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
 
   @Test
@@ -345,5 +356,7 @@ public class RemoteSpawnCacheTest {
     assertThat(evt.getKind()).isEqualTo(EventKind.WARNING);
     assertThat(evt.getMessage()).contains("fail");
     assertThat(evt.getMessage()).contains("upload");
+    assertThat(progressUpdates)
+        .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
   }
 }