Simplify further and add comments

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

@@ -219,14 +219,13 @@ public ImmutableList<SpawnResult> exec(
     }
 
     if (spawnResult.status() != Status.SUCCESS) {
-      Path execRoot = actionExecutionContext.getExecRoot();
-      checkArgument(spawnResult.status() != Status.SUCCESS);
+      String cwd = actionExecutionContext.getExecRoot().getPathString();
       String resultMessage = spawnResult.getFailureMessage();
       String message =
           !Strings.isNullOrEmpty(resultMessage)
               ? resultMessage
               : CommandFailureUtils.describeCommandFailure(
-                  executionOptions.verboseFailures, execRoot.getPathString(), spawn);
+                  executionOptions.verboseFailures, cwd, spawn);
       throw new SpawnExecException(message, spawnResult, /* forciblyRunRemotely= */ false);
     }
     return ImmutableList.of(spawnResult);

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -239,9 +239,7 @@ java_library(
     srcs = ["SpawnExecException.java"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/util:command",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
-        "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/protobuf:failure_details_java_proto",
         "//third_party:guava",
     ],

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

@@ -86,7 +86,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/exec:module_action_context_registry",
         "//src/main/java/com/google/devtools/build/lib/exec:remote_local_fallback_registry",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_cache",
-        "//src/main/java/com/google/devtools/build/lib/exec:spawn_exec_exception",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_input_expander",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
@@ -113,7 +112,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
-        "//src/main/java/com/google/devtools/build/lib/util:command",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:string",

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

@@ -867,10 +867,7 @@ private void deletePartialDownloadedOutputs(
     }
   }
 
-  /**
-   * Copies moves the locally created outputs from their temporary location to their declared
-   * location.
-   */
+  /** Moves the locally created outputs from their temporary location to their declared location. */
   private void moveOutputsToFinalLocation(
       Iterable<Path> localOutputs, Map<Path, Path> realToTmpPath) throws IOException {
     // Move the output files from their temporary name to the actual output file name. Executable
@@ -1332,21 +1329,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
-  public static final class InFlightExecution {
+  /** An ongoing local execution of a spawn. */
+  public static final class LocalExecution {
     private final RemoteAction action;
     private final SettableFuture<SpawnResult> spawnResultFuture;
 
-    private InFlightExecution(RemoteAction action) {
+    private LocalExecution(RemoteAction action) {
       this.action = action;
       this.spawnResultFuture = SettableFuture.create();
     }
 
+    /**
+     * Creates a new {@link LocalExecution} instance tracking the potential local execution of the
+     * given {@link RemoteAction} if there is a chance that the same action will be executed by a
+     * different Spawn.
+     */
     @Nullable
-    public static InFlightExecution createIfDeduplicatable(RemoteAction action) {
+    public static LocalExecution createIfDeduplicatable(RemoteAction action) {
       if (action.getSpawn().getPathMapper().isNoop()) {
         return null;
       }
-      return new InFlightExecution(action);
+      return new LocalExecution(action);
     }
 
     public void cancel() {
@@ -1357,7 +1360,7 @@ public void cancel() {
   /**
    * @return Whether the spawn result should be uploaded to the cache.
    */
-  public boolean shouldUpload(SpawnResult result, @Nullable InFlightExecution execution) {
+  public boolean shouldStore(SpawnResult result, @Nullable LocalExecution execution) {
     if (SpawnResult.Status.SUCCESS.equals(result.status()) && result.exitCode() == 0) {
       if (execution != null) {
         execution.spawnResultFuture.set(result);
@@ -1372,16 +1375,15 @@ public boolean shouldUpload(SpawnResult result, @Nullable InFlightExecution exec
   }
 
   /**
-   * Reuses the outputs of a concurrent non-remote execution of the same RemoteAction in a different
+   * Reuses the outputs of a concurrent local execution of the same RemoteAction in a different
    * spawn.
    *
    * <p>Since each output file is generated by a unique action and action's generally take care to
    * run a unique spawn for each output file, this method is only useful with path mapping enabled,
-   * which allows different spawns in a single build to generate the same RemoteAction.
+   * which allows different spawns in a single build to have the same RemoteAction.ActionKey.
    */
   @Nullable
-  public SpawnResult waitForAndReuseOutputs(
-      RemoteAction action, InFlightExecution previousExecution)
+  public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution previousExecution)
       throws InterruptedException, IOException {
     checkState(!shutdown.get(), "shutdown");
 
@@ -1390,6 +1392,9 @@ public SpawnResult waitForAndReuseOutputs(
       previousSpawnResult = previousExecution.spawnResultFuture.get();
     } catch (CancellationException | ExecutionException e) {
       Throwables.propagateIfPossible(e.getCause(), InterruptedException.class);
+      // The spawn this action was deduplicated against failed due to an exception or
+      // non-zero exit code. Since it isn't possible to transparently replay its failure for the
+      // current spawn, we rerun the action instead.
       return null;
     }
 
@@ -1419,6 +1424,7 @@ public SpawnResult waitForAndReuseOutputs(
           FileSystemUtils.copyFile(sourcePath, tmpPath);
         }
       } catch (FileNotFoundException e) {
+        // The spawn this action was deduplicated against failed to create an output file.
         return null;
       }
 

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

@@ -34,7 +34,7 @@
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
-import com.google.devtools.build.lib.remote.RemoteExecutionService.InFlightExecution;
+import com.google.devtools.build.lib.remote.RemoteExecutionService.LocalExecution;
 import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
 import com.google.devtools.build.lib.remote.common.BulkTransferException;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
@@ -57,8 +57,8 @@ final class RemoteSpawnCache implements SpawnCache {
   private final RemoteExecutionService remoteExecutionService;
   private final DigestUtil digestUtil;
   private final boolean verboseFailures;
-  private final ConcurrentHashMap<RemoteCacheClient.ActionKey, InFlightExecution>
-      inFlightExecutions = new ConcurrentHashMap<>();
+  private final ConcurrentHashMap<RemoteCacheClient.ActionKey, LocalExecution> inFlightExecutions =
+      new ConcurrentHashMap<>();
 
   RemoteSpawnCache(
       Path execRoot,
@@ -100,10 +100,16 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
     context.setDigest(digestUtil.asSpawnLogProto(action.getActionKey()));
 
     Profiler prof = Profiler.instance();
-    InFlightExecution thisExecution = null;
+    LocalExecution thisExecution = null;
     if (shouldAcceptCachedResult) {
-      InFlightExecution firstExecution = null;
-      thisExecution = InFlightExecution.createIfDeduplicatable(action);
+      // With path mapping enabled, different Spawns in a single build can have the same ActionKey.
+      // When their result isn't in the cache and two of them are scheduled concurrently, neither
+      // will result in a cache hit before the other finishes and uploads its result, which results
+      // in unnecessary work. To avoid this, we keep track of in-flight executions as long as their
+      // results haven't been uploaded to the cache yet and deduplicate all of them against the
+      // first one.
+      LocalExecution firstExecution = null;
+      thisExecution = LocalExecution.createIfDeduplicatable(action);
       if (shouldUploadLocalResults && thisExecution != null) {
         firstExecution = inFlightExecutions.putIfAbsent(action.getActionKey(), thisExecution);
       }
@@ -190,7 +196,7 @@ public SpawnMetrics getMetrics() {
     }
 
     if (shouldUploadLocalResults) {
-      final InFlightExecution thisExecutionFinal = thisExecution;
+      final LocalExecution thisExecutionFinal = thisExecution;
       return new CacheHandle() {
         @Override
         public boolean hasResult() {
@@ -209,7 +215,7 @@ public boolean willStore() {
 
         @Override
         public void store(SpawnResult result) throws ExecException, InterruptedException {
-          if (!remoteExecutionService.shouldUpload(result, thisExecutionFinal)) {
+          if (!remoteExecutionService.shouldStore(result, thisExecutionFinal)) {
             return;
           }
 
@@ -226,6 +232,10 @@ public void store(SpawnResult result) throws ExecException, InterruptedException
             }
           }
 
+          // As soon as the result is in the cache, actions can get the result from it instead of
+          // from the first in-flight execution. Not keeping in-flight executions around
+          // indefinitely is important to avoid excessive memory pressure - Spawns can be very
+          // large.
           remoteExecutionService.uploadOutputs(
               action, result, () -> inFlightExecutions.remove(action.getActionKey()));
         }
@@ -239,7 +249,7 @@ private void checkForConcurrentModifications()
             FileArtifactValue metadata = context.getInputMetadataProvider().getInputMetadata(input);
             Path path = execRoot.getRelative(input.getExecPath());
             if (metadata.wasModifiedSinceDigest(path)) {
-              throw new IOException(path + " was modified during thisExecution");
+              throw new IOException(path + " was modified during execution");
             }
           }
         }