[5.1] Remote: Action should not be successful and cached if outputs w…

…ere not created (#15071)

* Add a method, Spawn#isOutputMandatory, to indicate whether a Spawn must create an output by the end of its execution. If not, a Spawn (like a test) may succeed without necessarily creating that output.

The 4 categories of actions that do this are:

1. Tests (tests can create XML and other files, but may not).
2. Java compilations with reduced classpaths (the initial compilation with a reduced classpath may fail, but produce a usable jdeps proto file, so the Spawn will claim that it succeeded so that the action can process the proto file).
3. Extra actions with a dummy output that is produced locally, not by the Spawn. However, by changing the outputs of the Spawn to be empty, this situation is avoided.
4. C++ compilations with coverage (a .gcno coverage file is not produced for an empty translation unit).

In particular, all SpawnActions' Spawns have #isMandatoryOutput always return true, and there should be no new cases of #isMandatoryOutput returning false besides the ones added in this CL.

PiperOrigin-RevId: 425616085

* Remote: Don't upload action result if declared outputs are not created.

Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry.

Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread.

Fixes #14543.

Closes #15016.

PiperOrigin-RevId: 434448255

* Remote: Check declared outputs when downloading outputs.

An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build.

This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue.

Also fixes an issue introduced by #15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`.

Fixes #14543.

Closes #15051.

PiperOrigin-RevId: 435307260

Co-authored-by: janakr <[email protected]>

src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java

@@ -16,11 +16,11 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -113,7 +113,7 @@ public NestedSet<? extends ActionInput> getInputFiles() {
   }
 
   @Override
-  public Collection<? extends ActionInput> getOutputFiles() {
+  public ImmutableSet<Artifact> getOutputFiles() {
     return action.getOutputs();
   }
 

src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java

@@ -29,7 +29,7 @@ public class DelegateSpawn implements Spawn {
 
   private final Spawn spawn;
 
-  public DelegateSpawn(Spawn spawn){
+  public DelegateSpawn(Spawn spawn) {
     this.spawn = spawn;
   }
 
@@ -73,6 +73,11 @@ public Collection<? extends ActionInput> getOutputFiles() {
     return spawn.getOutputFiles();
   }
 
+  @Override
+  public boolean isMandatoryOutput(ActionInput output) {
+    return spawn.isMandatoryOutput(output);
+  }
+
   @Override
   public ActionExecutionMetadata getResourceOwner() {
     return spawn.getResourceOwner();

src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java

@@ -22,13 +22,11 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import java.util.Set;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
-/**
- * Immutable implementation of a Spawn that does not perform any processing on the parameters.
- * Prefer this over all other Spawn implementations.
- */
+/** Immutable implementation of a Spawn that does not perform any processing on the parameters. */
 @Immutable
 public final class SimpleSpawn implements Spawn {
   private final ActionExecutionMetadata owner;
@@ -41,6 +39,8 @@ public final class SimpleSpawn implements Spawn {
   private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
   private final ImmutableList<? extends ActionInput> outputs;
   private final ResourceSet localResources;
+  // If null, all outputs are mandatory.
+  @Nullable private final Set<? extends ActionInput> mandatoryOutputs;
 
   public SimpleSpawn(
       ActionExecutionMetadata owner,
@@ -52,6 +52,7 @@ public SimpleSpawn(
       NestedSet<? extends ActionInput> inputs,
       NestedSet<? extends ActionInput> tools,
       ImmutableSet<? extends ActionInput> outputs,
+      @Nullable Set<? extends ActionInput> mandatoryOutputs,
       ResourceSet localResources) {
     this.owner = Preconditions.checkNotNull(owner);
     this.arguments = Preconditions.checkNotNull(arguments);
@@ -63,6 +64,7 @@ public SimpleSpawn(
         runfilesSupplier == null ? EmptyRunfilesSupplier.INSTANCE : runfilesSupplier;
     this.filesetMappings = filesetMappings;
     this.outputs = Preconditions.checkNotNull(outputs).asList();
+    this.mandatoryOutputs = mandatoryOutputs;
     this.localResources = Preconditions.checkNotNull(localResources);
   }
 
@@ -73,7 +75,7 @@ public SimpleSpawn(
       ImmutableMap<String, String> executionInfo,
       NestedSet<? extends ActionInput> inputs,
       ImmutableSet<? extends ActionInput> outputs,
-      ResourceSet localResources) {
+      ResourceSet resourceSet) {
     this(
         owner,
         arguments,
@@ -84,7 +86,8 @@ public SimpleSpawn(
         inputs,
         NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         outputs,
-        localResources);
+        /*mandatoryOutputs=*/ null,
+        resourceSet);
   }
 
   @Override
@@ -127,6 +130,11 @@ public ImmutableList<? extends ActionInput> getOutputFiles() {
     return outputs;
   }
 
+  @Override
+  public boolean isMandatoryOutput(ActionInput output) {
+    return mandatoryOutputs == null || mandatoryOutputs.contains(output);
+  }
+
   @Override
   public ActionExecutionMetadata getResourceOwner() {
     return owner;

src/main/java/com/google/devtools/build/lib/actions/Spawn.java

@@ -99,18 +99,35 @@ public interface Spawn extends DescribableExecutionUnit {
   NestedSet<? extends ActionInput> getInputFiles();
 
   /**
-   * Returns the collection of files that this command must write.  Callers should not mutate
-   * the result.
+   * Returns the collection of files that this command will write. Callers should not mutate the
+   * result.
    *
    * <p>This is for use with remote execution, so remote execution does not have to guess what
-   * outputs the process writes.  While the order does not affect the semantics, it should be
-   * stable so it can be cached.
+   * outputs the process writes. While the order does not affect the semantics, it should be stable
+   * so it can be cached.
    */
   Collection<? extends ActionInput> getOutputFiles();
 
   /**
-   * Returns the resource owner for local fallback.
+   * Returns true if {@code output} must be created for the action to succeed. Can be used by remote
+   * execution implementations to mark a command as failed if it did not create an output, even if
+   * the command itself exited with a successful exit code.
+   *
+   * <p>Some actions, like tests, may have optional files (like .xml files) that may be created, but
+   * are not required, so their spawns should return false for those optional files. Note that in
+   * general, every output in {@link ActionAnalysisMetadata#getOutputs} is checked for existence in
+   * {@link com.google.devtools.build.lib.skyframe.SkyframeActionExecutor#checkOutputs}, so
+   * eventually all those outputs must be produced by at least one {@code Spawn} for that action, or
+   * locally by the action in some cases.
+   *
+   * <p>This method should not be overridden by any new Spawns if possible: outputs should be
+   * mandatory.
    */
+  default boolean isMandatoryOutput(ActionInput output) {
+    return true;
+  }
+
+  /** Returns the resource owner for local fallback. */
   ActionExecutionMetadata getResourceOwner();
 
   /**

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

@@ -340,6 +340,11 @@ private ActionExecutionException createCommandLineException(CommandLineExpansion
     return new ActionExecutionException(e, this, /*catastrophe=*/ false, detailedExitCode);
   }
 
+  @VisibleForTesting
+  public ResourceSetOrBuilder getResourceSetOrBuilder() {
+    return resourceSetOrBuilder;
+  }
+
   /**
    * Returns a Spawn that is representative of the command that this Action will execute. This
    * function must not modify any state.
@@ -361,20 +366,22 @@ final Spawn getSpawn(NestedSet<Artifact> inputs)
         /*envResolved=*/ false,
         inputs,
         /*additionalInputs=*/ ImmutableList.of(),
-        /*filesetMappings=*/ ImmutableMap.of());
+        /*filesetMappings=*/ ImmutableMap.of(),
+        /*reportOutputs=*/ true);
   }
 
   /**
-   * Return a spawn that is representative of the command that this Action will execute in the given
-   * client environment.
+   * Returns a spawn that is representative of the command that this Action will execute in the
+   * given client environment.
    */
   public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
       throws CommandLineExpansionException, InterruptedException {
     return getSpawn(
         actionExecutionContext.getArtifactExpander(),
         actionExecutionContext.getClientEnv(),
         /*envResolved=*/ false,
-        actionExecutionContext.getTopLevelFilesets());
+        actionExecutionContext.getTopLevelFilesets(),
+        /*reportOutputs=*/ true);
   }
 
   /**
@@ -385,11 +392,12 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
    *     effective environment. Otherwise they will be used as client environment to resolve the
    *     action env.
    */
-  Spawn getSpawn(
+  protected Spawn getSpawn(
       ArtifactExpander artifactExpander,
       Map<String, String> env,
       boolean envResolved,
-      Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
+      Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
+      boolean reportOutputs)
       throws CommandLineExpansionException, InterruptedException {
     ExpandedCommandLines expandedCommandLines =
         commandLines.expand(artifactExpander, getPrimaryOutput().getExecPath(), commandLineLimits);
@@ -399,7 +407,8 @@ Spawn getSpawn(
         envResolved,
         getInputs(),
         expandedCommandLines.getParamFiles(),
-        filesetMappings);
+        filesetMappings,
+        reportOutputs);
   }
 
   Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
@@ -554,10 +563,11 @@ public Map<String, String> getExecutionInfo() {
   }
 
   /** A spawn instance that is tied to a specific SpawnAction. */
-  private class ActionSpawn extends BaseSpawn {
+  private final class ActionSpawn extends BaseSpawn {
     private final NestedSet<ActionInput> inputs;
     private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
     private final ImmutableMap<String, String> effectiveEnvironment;
+    private final boolean reportOutputs;
 
     /**
      * Creates an ActionSpawn with the given environment variables.
@@ -571,7 +581,8 @@ private ActionSpawn(
         boolean envResolved,
         NestedSet<Artifact> inputs,
         Iterable<? extends ActionInput> additionalInputs,
-        Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
+        Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
+        boolean reportOutputs)
         throws CommandLineExpansionException {
       super(
           arguments,
@@ -592,16 +603,15 @@ private ActionSpawn(
       this.inputs = inputsBuilder.build();
       this.filesetMappings = filesetMappings;
 
-      /**
-       * If the action environment is already resolved using the client environment, the given
-       * environment variables are used as they are. Otherwise, they are used as clientEnv to
-       * resolve the action environment variables
-       */
+      // If the action environment is already resolved using the client environment, the given
+      // environment variables are used as they are. Otherwise, they are used as clientEnv to
+      // resolve the action environment variables.
       if (envResolved) {
         effectiveEnvironment = ImmutableMap.copyOf(env);
       } else {
         effectiveEnvironment = SpawnAction.this.getEffectiveEnvironment(env);
       }
+      this.reportOutputs = reportOutputs;
     }
 
     @Override
@@ -618,6 +628,11 @@ public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMap
     public NestedSet<? extends ActionInput> getInputFiles() {
       return inputs;
     }
+
+    @Override
+    public ImmutableSet<Artifact> getOutputFiles() {
+      return reportOutputs ? super.getOutputFiles() : ImmutableSet.of();
+    }
   }
 
   /**

src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java

@@ -324,7 +324,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
         actionExecutionContext.getArtifactExpander(),
         getEffectiveEnvironment(actionExecutionContext.getClientEnv()),
         /*envResolved=*/ true,
-        actionExecutionContext.getTopLevelFilesets());
+        actionExecutionContext.getTopLevelFilesets(),
+        /*reportOutputs=*/ true);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java

@@ -26,12 +26,14 @@
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLine;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.CommandLines;
 import com.google.devtools.build.lib.actions.CommandLines.CommandLineLimits;
 import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -54,17 +56,8 @@ public final class ExtraAction extends SpawnAction {
   private final boolean createDummyOutput;
   private final NestedSet<Artifact> extraActionInputs;
 
-  /**
-   * A long way to say (ExtraAction xa) -> xa.getShadowedAction().
-   */
   public static final Function<ExtraAction, Action> GET_SHADOWED_ACTION =
-      new Function<ExtraAction, Action>() {
-        @Nullable
-        @Override
-        public Action apply(@Nullable ExtraAction extraAction) {
-          return extraAction != null ? extraAction.getShadowedAction() : null;
-        }
-      };
+      e -> e != null ? e.getShadowedAction() : null;
 
   ExtraAction(
       NestedSet<Artifact> extraActionInputs,
@@ -153,6 +146,20 @@ public NestedSet<Artifact> getAllowedDerivedInputs() {
     return shadowedAction.getAllowedDerivedInputs();
   }
 
+  @Override
+  public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
+      throws CommandLineExpansionException, InterruptedException {
+    if (!createDummyOutput) {
+      return super.getSpawn(actionExecutionContext);
+    }
+    return getSpawn(
+        actionExecutionContext.getArtifactExpander(),
+        actionExecutionContext.getClientEnv(),
+        /*envResolved=*/ false,
+        actionExecutionContext.getTopLevelFilesets(),
+        /*reportOutputs=*/ false);
+  }
+
   @Override
   protected void afterExecute(
       ActionExecutionContext actionExecutionContext, List<SpawnResult> spawnResults)
@@ -171,9 +178,7 @@ protected void afterExecute(
     }
   }
 
-  /**
-   * Returns the action this extra action is 'shadowing'.
-   */
+  /** Returns the action this extra action is 'shadowing'. */
   public Action getShadowedAction() {
     return shadowedAction;
   }

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

@@ -151,6 +151,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
                 ? action.getTools()
                 : NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             createSpawnOutputs(action),
+            /*mandatoryOutputs=*/ ImmutableSet.of(),
             localResourceUsage);
     Path execRoot = actionExecutionContext.getExecRoot();
     ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
@@ -558,13 +559,10 @@ private static Spawn createXmlGeneratingSpawn(
             Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
         /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         /*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
+        /*mandatoryOutputs=*/ null,
         SpawnAction.DEFAULT_RESOURCE_SET);
   }
 
-  /**
-   * A spawn to generate a test.xml file from the test log. This is only used if the test does not
-   * generate a test.xml file itself.
-   */
   private static Spawn createCoveragePostProcessingSpawn(
       ActionExecutionContext actionExecutionContext,
       TestRunnerAction action,
@@ -588,18 +586,19 @@ private static Spawn createCoveragePostProcessingSpawn(
         ImmutableMap.copyOf(testEnvironment),
         ImmutableMap.copyOf(action.getExecutionInfo()),
         action.getLcovMergerRunfilesSupplier(),
-        /* filesetMappings= */ ImmutableMap.of(),
-        /* inputs= */ NestedSetBuilder.<ActionInput>compileOrder()
+        /*filesetMappings=*/ ImmutableMap.of(),
+        /*inputs=*/ NestedSetBuilder.<ActionInput>compileOrder()
             .addTransitive(action.getInputs())
             .addAll(expandedCoverageDir)
             .add(action.getCollectCoverageScript())
             .add(action.getCoverageDirectoryTreeArtifact())
             .add(action.getCoverageManifest())
             .addTransitive(action.getLcovMergerFilesToRun().build())
             .build(),
-        /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
-        /* outputs= */ ImmutableSet.of(
+        /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+        /*outputs=*/ ImmutableSet.of(
             ActionInputHelper.fromPath(action.getCoverageData().getExecPath())),
+        /*mandatoryOutputs=*/ null,
         SpawnAction.DEFAULT_RESOURCE_SET);
   }