Avoid the spawn cache if executing dynamically.

Fixes #12364 (caused by 25e58ff) based on Ulf's approach, but avoiding secretly disabling some potential caches that should not be disabled.

RELNOTES: n/a
PiperOrigin-RevId: 341588598

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -114,14 +114,20 @@ public ImmutableList<SpawnResult> exec(
     SpawnExecutionContext context =
         new SpawnExecutionContextImpl(spawn, actionExecutionContext, stopConcurrentSpawns, timeout);
 
-    SpawnCache cache = actionExecutionContext.getContext(SpawnCache.class);
+    // Avoid caching for runners which handle caching internally e.g. RemoteSpawnRunner.
+    SpawnCache cache =
+        spawnRunner.handlesCaching()
+            ? SpawnCache.NO_CACHE
+            : actionExecutionContext.getContext(SpawnCache.class);
+
     // In production, the getContext method guarantees that we never get null back. However, our
     // integration tests don't set it up correctly, so cache may be null in testing.
     if (cache == null) {
       cache = SpawnCache.NO_CACHE;
     }
-    // Avoid caching for runners which handle caching internally e.g. RemoteSpawnRunner
-    if (spawnRunner.handlesCaching()) {
+
+    // Avoid using the remote cache of a dynamic execution setup for the local runner.
+    if (context.speculating() && !cache.usefulInDynamicExecution()) {
       cache = SpawnCache.NO_CACHE;
     }
     SpawnResult spawnResult;

src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java

@@ -165,4 +165,15 @@ interface CacheHandle extends Closeable {
    */
   CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       throws ExecException, IOException, InterruptedException;
+
+  /**
+   * Returns whether this cache implementation makes sense to use together with dynamic execution.
+   *
+   * <p>A cache that's part of the remote system used for dynamic execution should not also be used
+   * for the local speculative execution. However, a local cache or a separate remote cache-only
+   * system would be.
+   */
+  default boolean usefulInDynamicExecution() {
+    return true;
+  }
 }

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java

@@ -342,4 +342,9 @@ private static boolean useRemoteCache(RemoteOptions options) {
   private static boolean useDiskCache(RemoteOptions options) {
     return options.diskCache != null && !options.diskCache.isEmpty();
   }
+
+  @Override
+  public boolean usefulInDynamicExecution() {
+    return false;
+  }
 }

src/test/java/com/google/devtools/build/lib/BUILD

@@ -82,6 +82,7 @@ java_library(
         "//src/test/java/com/google/devtools/build/lib/testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestSuite",
         "//third_party:junit4",
+        "//third_party:mockito",
     ],
 )
 

src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java

@@ -20,6 +20,8 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
 import static org.mockito.Mockito.when;
 
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -183,6 +185,84 @@ public void testCacheMiss() throws Exception {
     verify(entry).store(eq(spawnResult));
   }
 
+  @Test
+  public void testExec_whenLocalCaches_usesNoCache() throws Exception {
+    when(spawnRunner.handlesCaching()).thenReturn(true);
+
+    SpawnCache cache = mock(SpawnCache.class);
+
+    when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache);
+    when(actionExecutionContext.getExecRoot()).thenReturn(execRoot);
+    SpawnResult spawnResult =
+        new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
+    when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class)))
+        .thenReturn(FutureSpawn.immediate(spawnResult));
+
+    List<SpawnResult> spawnResults =
+        new TestedSpawnStrategy(execRoot, spawnRunner).exec(SIMPLE_SPAWN, actionExecutionContext);
+
+    assertThat(spawnResults).containsExactly(spawnResult);
+
+    // Must only be called exactly once.
+    verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class));
+    verifyZeroInteractions(cache);
+  }
+
+  @Test
+  public void testExec_usefulCacheInDynamicExecution() throws Exception {
+    when(spawnRunner.handlesCaching()).thenReturn(false);
+
+    SpawnCache cache = mock(SpawnCache.class);
+    when(cache.usefulInDynamicExecution()).thenReturn(true);
+    CacheHandle entry = mock(CacheHandle.class);
+    when(cache.lookup(any(Spawn.class), any(SpawnExecutionContext.class))).thenReturn(entry);
+    when(entry.hasResult()).thenReturn(false);
+    when(entry.willStore()).thenReturn(true);
+
+    when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache);
+    when(actionExecutionContext.getExecRoot()).thenReturn(execRoot);
+    SpawnResult spawnResult =
+        new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
+    when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class)))
+        .thenReturn(FutureSpawn.immediate(spawnResult));
+
+    List<SpawnResult> spawnResults =
+        new TestedSpawnStrategy(execRoot, spawnRunner)
+            .exec(SIMPLE_SPAWN, actionExecutionContext, () -> {});
+
+    assertThat(spawnResults).containsExactly(spawnResult);
+
+    // Must only be called exactly once.
+    verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class));
+    verify(entry).store(eq(spawnResult));
+  }
+
+  @Test
+  public void testExec_nonUsefulCacheInDynamicExecution() throws Exception {
+    when(spawnRunner.handlesCaching()).thenReturn(false);
+
+    SpawnCache cache = mock(SpawnCache.class);
+    when(cache.usefulInDynamicExecution()).thenReturn(false);
+
+    when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache);
+    when(actionExecutionContext.getExecRoot()).thenReturn(execRoot);
+    SpawnResult spawnResult =
+        new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
+    when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class)))
+        .thenReturn(FutureSpawn.immediate(spawnResult));
+
+    List<SpawnResult> spawnResults =
+        new TestedSpawnStrategy(execRoot, spawnRunner)
+            .exec(SIMPLE_SPAWN, actionExecutionContext, () -> {});
+
+    assertThat(spawnResults).containsExactly(spawnResult);
+
+    // Must only be called exactly once.
+    verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class));
+    verify(cache).usefulInDynamicExecution();
+    verifyNoMoreInteractions(cache);
+  }
+
   @Test
   public void testCacheMissWithNonZeroExit() throws Exception {
     SpawnCache cache = mock(SpawnCache.class);