Remote: Cleanup the code that determines whether the spawn should accept/upload results from/to remote cache.

Remote cache (and execution) for an action can be controlled with following tags:
  - `no-remote-cache`: prevent remote caching, but allow local caching
  - `no-remote-exec`: prevent remote execution, but allow remote caching
  - `no-remote`: combine no-remote-cache and no-remote-exec
  - `no-cache`: extend no-remote-cache to also prevent local caching

A combined cache is treated as remote cache, hence is disabled for actions that are tagged with `no-remote-cache` unless `--incompatible_remote_results_ignore_disk` is set in which case local component is enabled.

Fixes #13621.

Closes #13769.

PiperOrigin-RevId: 388875505

Author: coeuvre

src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html

@@ -34,7 +34,8 @@
     Note: for the purposes of this tag, the disk-cache is considered a local cache, whereas
     the http and gRPC caches are considered remote.
     If a combined cache is specified (i.e. a cache with local and remote components),
-    it's treated as a remote cache and disabled entirely.
+    it's treated as a remote cache and disabled entirely unless <code>--incompatible_remote_results_ignore_disk</code>
+    is set in which case the local components will be used.
   </li>
 
   <li><code>no-remote-exec</code> keyword results in the action or test never being

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

@@ -158,7 +158,7 @@ public ActionResult upload(
         context, remotePathResolver, actionKey, action, command, outputs, outErr, resultBuilder);
     resultBuilder.setExitCode(exitCode);
     ActionResult result = resultBuilder.build();
-    if (exitCode == 0 && !action.getDoNotCache()) {
+    if (exitCode == 0) {
       cacheProtocol.uploadActionResult(context, actionKey, result);
     }
     return result;

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

@@ -16,6 +16,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
 import static com.google.common.util.concurrent.Futures.immediateFuture;
 import static com.google.common.util.concurrent.Futures.transform;
@@ -25,7 +26,13 @@
 import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
 import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath;
 import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromCombinedCache;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromDiskCache;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromRemoteCache;
 import static com.google.devtools.build.lib.remote.util.Utils.shouldDownloadAllSpawnOutputs;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToCombinedDisk;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToDiskCache;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToRemoteCache;
 
 import build.bazel.remote.execution.v2.Action;
 import build.bazel.remote.execution.v2.ActionResult;
@@ -266,19 +273,56 @@ public NetworkTime getNetworkTime() {
     }
   }
 
-  /** Returns {@code true} if the result of spawn may be cached remotely. */
-  public boolean mayBeCachedRemotely(Spawn spawn) {
-    return remoteCache != null && Spawns.mayBeCached(spawn) && Spawns.mayBeCachedRemotely(spawn);
+  private static boolean useRemoteCache(RemoteOptions options) {
+    return !isNullOrEmpty(options.remoteCache) || !isNullOrEmpty(options.remoteExecutor);
   }
 
-  /** Returns {@code true} if the result of spawn may be cached. */
-  public boolean mayBeCached(Spawn spawn) {
-    return remoteCache != null && Spawns.mayBeCached(spawn);
+  private static boolean useDiskCache(RemoteOptions options) {
+    return options.diskCache != null && !options.diskCache.isEmpty();
+  }
+
+  /** Returns {@code true} if the {@code spawn} should accept cached results from remote cache. */
+  public boolean shouldAcceptCachedResult(Spawn spawn) {
+    if (remoteCache == null) {
+      return false;
+    }
+
+    if (useRemoteCache(remoteOptions)) {
+      if (useDiskCache(remoteOptions)) {
+        return shouldAcceptCachedResultFromCombinedCache(remoteOptions, spawn);
+      } else {
+        return shouldAcceptCachedResultFromRemoteCache(remoteOptions, spawn);
+      }
+    } else {
+      return shouldAcceptCachedResultFromDiskCache(remoteOptions, spawn);
+    }
+  }
+
+  /**
+   * Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote
+   * cache.
+   */
+  public boolean shouldUploadLocalResults(Spawn spawn) {
+    if (remoteCache == null) {
+      return false;
+    }
+
+    if (useRemoteCache(remoteOptions)) {
+      if (useDiskCache(remoteOptions)) {
+        return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn);
+      } else {
+        return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn);
+      }
+    } else {
+      return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn);
+    }
   }
 
   /** Returns {@code true} if the spawn may be executed remotely. */
   public boolean mayBeExecutedRemotely(Spawn spawn) {
-    return remoteCache != null && remoteExecutor != null && Spawns.mayBeExecutedRemotely(spawn);
+    return remoteCache instanceof RemoteExecutionCache
+        && remoteExecutor != null
+        && Spawns.mayBeExecutedRemotely(spawn);
   }
 
   /** Creates a new {@link RemoteAction} instance from spawn. */
@@ -313,7 +357,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
         TracingMetadataUtils.buildMetadata(
             buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner());
     RemoteActionExecutionContext remoteActionExecutionContext =
-        RemoteActionExecutionContext.create(metadata);
+        RemoteActionExecutionContext.createForSpawn(spawn, metadata);
 
     return new RemoteAction(
         spawn,
@@ -434,11 +478,13 @@ public int hashCode() {
     }
   }
 
+
   /** Lookup the remote cache for the given {@link RemoteAction}. {@code null} if not found. */
   @Nullable
   public RemoteActionResult lookupCache(RemoteAction action)
       throws IOException, InterruptedException {
-    checkNotNull(remoteCache, "remoteCache can't be null");
+    checkState(shouldAcceptCachedResult(action.spawn), "spawn doesn't accept cached result");
+
     ActionResult actionResult =
         remoteCache.downloadActionResult(
             action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
@@ -944,7 +990,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
   /** Upload outputs of a remote action which was executed locally to remote cache. */
   public void uploadOutputs(RemoteAction action)
       throws InterruptedException, IOException, ExecException {
-    checkNotNull(remoteCache, "remoteCache can't be null");
+    checkState(shouldUploadLocalResults(action.spawn), "spawn shouldn't upload local result");
+
     Collection<Path> outputFiles =
         action.spawn.getOutputFiles().stream()
             .map((inp) -> execRoot.getRelative(inp.getExecPath()))
@@ -966,8 +1013,8 @@ public void uploadOutputs(RemoteAction action)
    */
   public void uploadInputsIfNotPresent(RemoteAction action)
       throws IOException, InterruptedException {
-    checkNotNull(remoteCache, "remoteCache can't be null");
-    checkState(remoteCache instanceof RemoteExecutionCache);
+    checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");
+
     RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache;
     // Upload the command and all the inputs into the remote cache.
     Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
@@ -986,7 +1033,7 @@ public void uploadInputsIfNotPresent(RemoteAction action)
   public RemoteActionResult executeRemotely(
       RemoteAction action, boolean acceptCachedResult, OperationObserver observer)
       throws IOException, InterruptedException {
-    checkNotNull(remoteExecutor, "remoteExecutor can't be null");
+    checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");
 
     ExecuteRequest.Builder requestBuilder =
         ExecuteRequest.newBuilder()

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

@@ -17,6 +17,7 @@
 import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_DOWNLOAD;
 import static com.google.devtools.build.lib.remote.util.Utils.createSpawnResult;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Throwables;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -81,15 +82,17 @@ final class RemoteSpawnCache implements SpawnCache {
     this.remoteExecutionService = remoteExecutionService;
   }
 
+  @VisibleForTesting
+  RemoteExecutionService getRemoteExecutionService() {
+    return remoteExecutionService;
+  }
+
   @Override
   public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       throws InterruptedException, IOException, ExecException, ForbiddenActionInputException {
-    boolean mayBeCached =
-        remoteExecutionService.mayBeCachedRemotely(spawn)
-            || (!useRemoteCache(options) && remoteExecutionService.mayBeCached(spawn));
-    if (!mayBeCached) {
-      // returning SpawnCache.NO_RESULT_NO_STORE in case the caching is disabled or in case
-      // the remote caching is disabled and the only configured cache is remote.
+    boolean shouldAcceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
+    boolean shouldUploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
+    if (!shouldAcceptCachedResult && !shouldUploadLocalResults) {
       return SpawnCache.NO_RESULT_NO_STORE;
     }
 
@@ -102,8 +105,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
             .setInputFiles(action.getInputFiles());
 
     Profiler prof = Profiler.instance();
-    if (options.remoteAcceptCached
-        || (options.incompatibleRemoteResultsIgnoreDisk && useDiskCache(options))) {
+    if (shouldAcceptCachedResult) {
       context.report(SPAWN_CHECKING_CACHE_EVENT);
       // Metadata will be available in context.current() until we detach.
       // This is done via a thread-local variable.
@@ -162,8 +164,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
 
     context.prefetchInputs();
 
-    if (options.remoteUploadLocalResults
-        || (options.incompatibleRemoteResultsIgnoreDisk && useDiskCache(options))) {
+    if (shouldUploadLocalResults) {
       return new CacheHandle() {
         @Override
         public boolean hasResult() {
@@ -249,14 +250,6 @@ private void report(Event evt) {
     }
   }
 
-  private static boolean useRemoteCache(RemoteOptions options) {
-    return !isNullOrEmpty(options.remoteCache) || !isNullOrEmpty(options.remoteExecutor);
-  }
-
-  private static boolean useDiskCache(RemoteOptions options) {
-    return options.diskCache != null && !options.diskCache.isEmpty();
-  }
-
   @Override
   public boolean usefulInDynamicExecution() {
     return false;

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

@@ -37,7 +37,6 @@
 import com.google.devtools.build.lib.actions.SpawnMetrics;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
-import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
@@ -170,12 +169,12 @@ public void reportExecutingIfNot() {
   public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
       throws ExecException, InterruptedException, IOException, ForbiddenActionInputException {
     Preconditions.checkArgument(
-        Spawns.mayBeExecutedRemotely(spawn), "Spawn can't be executed remotely. This is a bug.");
+        remoteExecutionService.mayBeExecutedRemotely(spawn),
+        "Spawn can't be executed remotely. This is a bug.");
 
     Stopwatch totalTime = Stopwatch.createStarted();
-    boolean spawnCacheableRemotely = Spawns.mayBeCachedRemotely(spawn);
-    boolean uploadLocalResults = remoteOptions.remoteUploadLocalResults && spawnCacheableRemotely;
-    boolean acceptCachedResult = remoteOptions.remoteAcceptCached && spawnCacheableRemotely;
+    boolean uploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn);
+    boolean acceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn);
 
     RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context);
     SpawnMetrics.Builder spawnMetrics =

src/main/java/com/google/devtools/build/lib/remote/common/BUILD

@@ -23,6 +23,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",
+        "//third_party:jsr305",
         "//third_party/protobuf:protobuf_java",
         "@googleapis//:google_longrunning_operations_java_proto",
         "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",

src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java

@@ -14,21 +14,35 @@
 package com.google.devtools.build.lib.remote.common;
 
 import build.bazel.remote.execution.v2.RequestMetadata;
+import com.google.devtools.build.lib.actions.Spawn;
+import javax.annotation.Nullable;
 
 /** A context that provide remote execution related information for executing an action remotely. */
 public interface RemoteActionExecutionContext {
 
-  /** Get the {@link RequestMetadata} for the action being executed. */
+  /** Returns the {@link Spawn} of the action being executed or {@code null}. */
+  @Nullable
+  Spawn getSpawn();
+
+  /** Returns the {@link RequestMetadata} for the action being executed. */
   RequestMetadata getRequestMetadata();
 
   /**
-   * Get the {@link NetworkTime} instance used to measure the network time during the action
+   * Returns the {@link NetworkTime} instance used to measure the network time during the action
    * execution.
    */
   NetworkTime getNetworkTime();
 
   /** Creates a {@link SimpleRemoteActionExecutionContext} with given {@link RequestMetadata}. */
   static RemoteActionExecutionContext create(RequestMetadata metadata) {
-    return new SimpleRemoteActionExecutionContext(metadata, new NetworkTime());
+    return new SimpleRemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime());
+  }
+
+  /**
+   * Creates a {@link SimpleRemoteActionExecutionContext} with given {@link Spawn} and {@link
+   * RequestMetadata}.
+   */
+  static RemoteActionExecutionContext createForSpawn(Spawn spawn, RequestMetadata metadata) {
+    return new SimpleRemoteActionExecutionContext(spawn, metadata, new NetworkTime());
   }
 }

src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java

@@ -14,19 +14,29 @@
 package com.google.devtools.build.lib.remote.common;
 
 import build.bazel.remote.execution.v2.RequestMetadata;
+import com.google.devtools.build.lib.actions.Spawn;
+import javax.annotation.Nullable;
 
 /** A {@link RemoteActionExecutionContext} implementation */
 public class SimpleRemoteActionExecutionContext implements RemoteActionExecutionContext {
 
+  private final Spawn spawn;
   private final RequestMetadata requestMetadata;
   private final NetworkTime networkTime;
 
   public SimpleRemoteActionExecutionContext(
-      RequestMetadata requestMetadata, NetworkTime networkTime) {
+      Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
+    this.spawn = spawn;
     this.requestMetadata = requestMetadata;
     this.networkTime = networkTime;
   }
 
+  @Nullable
+  @Override
+  public Spawn getSpawn() {
+    return spawn;
+  }
+
   @Override
   public RequestMetadata getRequestMetadata() {
     return requestMetadata;

src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java

@@ -13,6 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote.disk;
 
+import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromRemoteCache;
+import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToRemoteCache;
+
 import build.bazel.remote.execution.v2.ActionResult;
 import build.bazel.remote.execution.v2.Digest;
 import com.google.common.base.Preconditions;
@@ -54,7 +57,7 @@ public void uploadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult)
       throws IOException, InterruptedException {
     diskCache.uploadActionResult(context, actionKey, actionResult);
-    if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+    if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
       remoteCache.uploadActionResult(context, actionKey, actionResult);
     }
   }
@@ -70,7 +73,7 @@ public ListenableFuture<Void> uploadFile(
       RemoteActionExecutionContext context, Digest digest, Path file) {
     try {
       diskCache.uploadFile(context, digest, file).get();
-      if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+      if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
         remoteCache.uploadFile(context, digest, file).get();
       }
     } catch (ExecutionException e) {
@@ -86,7 +89,7 @@ public ListenableFuture<Void> uploadBlob(
       RemoteActionExecutionContext context, Digest digest, ByteString data) {
     try {
       diskCache.uploadBlob(context, digest, data).get();
-      if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+      if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
         remoteCache.uploadBlob(context, digest, data).get();
       }
     } catch (ExecutionException e) {
@@ -145,7 +148,7 @@ public ListenableFuture<Void> downloadBlob(
     final OutputStream tempOut;
     tempOut = new LazyFileOutputStream(tempPath);
 
-    if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteAcceptCached) {
+    if (shouldAcceptCachedResultFromRemoteCache(options, context.getSpawn())) {
       ListenableFuture<Void> download =
           closeStreamOnError(remoteCache.downloadBlob(context, digest, tempOut), tempOut);
       return Futures.transformAsync(
@@ -172,7 +175,7 @@ public ListenableFuture<ActionResult> downloadActionResult(
       return diskCache.downloadActionResult(context, actionKey, inlineOutErr);
     }
 
-    if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteAcceptCached) {
+    if (shouldAcceptCachedResultFromRemoteCache(options, context.getSpawn())) {
       return Futures.transformAsync(
           remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
           (actionResult) -> {