[8.5.0] Treat action results with missing mandatory outputs as cache misses (bazelbuild#27380)

This allows builds to recover from a polluted remote cache that contains
an action result with an exit code of 0 that hasn't produced the
required output files.

The check added in unknown commit is moved closer to the lookup so that
the cached result can be ignored instead of resulting in an error, with
forced re-execution in the case of remote execution.

Closes bazelbuild#27105.

PiperOrigin-RevId: 821651398
Change-Id: I067c668598f9ee83e670b4636854185282112f86 (cherry picked from
commit 29d442b)

Closes bazelbuild#27363

Author: fmeum

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

@@ -57,6 +57,7 @@
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.vfs.inmemoryfs.FileInfo;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryContentInfo;
@@ -579,15 +580,16 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
   }
 
   @Override
-  protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
       throws IOException {
     linkPath = resolveSymbolicLinksForParent(linkPath);
 
     if (isOutput(linkPath)) {
-      remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment);
+      remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment, type);
     }
 
-    localFs.getPath(linkPath).createSymbolicLink(targetFragment);
+    localFs.getPath(linkPath).createSymbolicLink(targetFragment, type);
   }
 
   @Override

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

@@ -117,6 +117,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.util.StringEncoding;
 import com.google.devtools.build.lib.util.TempPathGenerator;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -149,6 +150,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
@@ -807,6 +809,32 @@ public boolean success() {
       return actionResult.getExitCode() == 0;
     }
 
+    /**
+     * Returns an output that is mandatory for the given spawn but missing from this result, if any.
+     */
+    public Optional<? extends ActionInput> maybeGetMissingMandatoryOutput(RemoteAction action) {
+      var outputFiles = Iterables.transform(getOutputFiles(), OutputFile::getPath);
+      var outputDirPaths = Iterables.transform(getOutputDirectories(), OutputDirectory::getPath);
+      var outputSymlinkPaths =
+          Iterables.transform(
+              Iterables.concat(
+                  getOutputSymlinks(), getOutputFileSymlinks(), getOutputDirectorySymlinks()),
+              OutputSymlink::getPath);
+      ImmutableSet<String> allOutputPaths =
+          ImmutableSet.copyOf(
+              Iterables.transform(
+                  Iterables.concat(outputFiles, outputDirPaths, outputSymlinkPaths),
+                  StringEncoding::unicodeToInternal));
+      // Check that all mandatory outputs are created.
+      var spawn = action.getSpawn();
+      var remotePathResolver = action.getRemotePathResolver();
+      return spawn.getOutputFiles().stream()
+          .filter(spawn::isMandatoryOutput)
+          .filter(
+              output -> !allOutputPaths.contains(remotePathResolver.localPathToOutputPath(output)))
+          .findFirst();
+    }
+
     /** Returns {@code true} if this result is from a cache. */
     public boolean cacheHit() {
       if (executeResponse == null) {
@@ -1487,23 +1515,11 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
     if (result.success()) {
       // Check that all mandatory outputs are created.
-      for (ActionInput output : action.getSpawn().getOutputFiles()) {
-        if (action.getSpawn().isMandatoryOutput(output)) {
-          // In the past, remote execution did not create output directories if the action didn't do
-          // this explicitly. This check only remains so that old remote cache entries that do not
-          // include empty output directories remain valid.
-          if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
-            continue;
-          }
-
-          Path localPath = execRoot.getRelative(output.getExecPath());
-          if (!metadata.files.containsKey(localPath)
-              && !metadata.directories.containsKey(localPath)
-              && !metadata.symlinks.containsKey(localPath)) {
-            throw new IOException(
-                String.format("mandatory output %s was not created", prettyPrint(output)));
-          }
-        }
+      var missingMandatoryOutput = result.maybeGetMissingMandatoryOutput(action);
+      if (missingMandatoryOutput.isPresent()) {
+        throw new IOException(
+            "mandatory output %s was not created"
+                .formatted(prettyPrint(missingMandatoryOutput.get())));
       }
 
       if (result.executeResponse != null && !knownMissingCasDigests.isEmpty()) {

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

@@ -147,9 +147,11 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
               prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) {
             result = remoteExecutionService.lookupCache(action);
           }
-          // In case the remote cache returned a failed action (exit code != 0) we treat it as a
-          // cache miss
-          if (result != null && result.getExitCode() == 0) {
+          // In case the remote cache returned a failed action (exit code != 0) or failed to create
+          // a mandatory output, we treat it as a cache miss.
+          if (result != null
+              && result.getExitCode() == 0
+              && result.maybeGetMissingMandatoryOutput(action).isEmpty()) {
             if (thisExecution != null) {
               thisExecution.close();
             }

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

@@ -78,10 +78,13 @@
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.longrunning.Operation;
+import com.google.protobuf.Any;
 import com.google.protobuf.Timestamp;
 import com.google.protobuf.util.Durations;
 import com.google.protobuf.util.Timestamps;
+import com.google.rpc.RetryInfo;
 import io.grpc.Status.Code;
+import io.grpc.protobuf.StatusProto;
 import java.io.IOException;
 import java.time.Duration;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -217,7 +220,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
       }
 
       if (cachedResult != null) {
-        if (cachedResult.getExitCode() != 0) {
+        if (cachedResult.getExitCode() != 0
+            || cachedResult.maybeGetMissingMandatoryOutput(action).isPresent()) {
           // Failed actions are treated as a cache miss mostly in order to avoid caching flaky
           // actions (tests).
           // Set acceptCachedResult to false in order to force the action re-execution
@@ -312,6 +316,19 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
             // It's already late at this stage, but we should at least report once.
             reporter.reportExecutingIfNot();
 
+            if (result.cacheHit()
+                && (!result.success()
+                    || result.maybeGetMissingMandatoryOutput(action).isPresent())) {
+              // Instead of failing in downloadAndFinalizeSpawnResult, retry with forced execution.
+              useCachedResult.set(false);
+              var status =
+                  com.google.rpc.Status.newBuilder()
+                      .setCode(com.google.rpc.Code.NOT_FOUND_VALUE)
+                      .addDetails(Any.pack(RetryInfo.getDefaultInstance()))
+                      .build();
+              throw StatusProto.toStatusRuntimeException(status);
+            }
+
             maybePrintExecutionMessages(context, result.getMessage(), result.success());
 
             profileAccounting(clampTimeNanos, result.getExecutionMetadata());

src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java

@@ -29,6 +29,7 @@
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
@@ -390,7 +391,8 @@ public void createDirectoryAndParents(PathFragment path) throws IOException {
   }
 
   @Override
-  protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
       throws IOException {
     var comp = Blocker.begin();
     try {

src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java

@@ -569,14 +569,27 @@ protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) thro
   protected abstract boolean isSpecialFile(PathFragment path, boolean followSymlinks);
 
   /**
-   * Creates a symbolic link. See {@link Path#createSymbolicLink(Path)} for specification.
+   * Creates a symbolic link. See {@link Path#createSymbolicLink(Path, SymlinkTargetType)} for
+   * specification.
    *
    * <p>Note: for {@link FileSystem}s where {@link #supportsSymbolicLinksNatively(PathFragment)}
    * returns false, this method will throw an {@link UnsupportedOperationException}
    */
-  protected abstract void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected abstract void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType hint)
       throws IOException;
 
+  /**
+   * Creates a symbolic link. See {@link Path#createSymbolicLink(Path)} for specification.
+   *
+   * <p>Note: for {@link FileSystem}s where {@link #supportsSymbolicLinksNatively(PathFragment)}
+   * returns false, this method will throw an {@link UnsupportedOperationException}
+   */
+  protected final void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+      throws IOException {
+    createSymbolicLink(linkPath, targetFragment, SymlinkTargetType.UNSPECIFIED);
+  }
+
   /**
    * Returns the target of a symbolic link. See {@link Path#readSymbolicLink} for specification.
    *

src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java

@@ -279,10 +279,12 @@ private boolean linkExists(File file) {
   }
 
   @Override
-  protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
       throws IOException {
     java.nio.file.Path nioPath = getNioPath(linkPath);
     try {
+      // Files.createSymbolicLink does not let us specify the target type.
       Files.createSymbolicLink(
           nioPath,
           Paths.get(StringEncoding.internalToPlatform(targetFragment.getSafePathString())));

src/main/java/com/google/devtools/build/lib/vfs/Path.java

@@ -486,11 +486,43 @@ public Path createTempDirectory(String prefix) throws IOException {
    * referent of the created symlink is is the absolute path "target"; it is not possible to create
    * relative symbolic links via this method.
    *
+   * <p>The {@code type} argument denotes the file type of the target, if known. Some filesystems
+   * require this information to correctly create a symlink. This argument may be ignored if the
+   * target can be observed to exist and is of a different type.
+   *
+   * @param type the target file type
    * @throws IOException if the creation of the symbolic link was unsuccessful for any reason
    */
-  public void createSymbolicLink(Path target) throws IOException {
+  public void createSymbolicLink(Path target, SymlinkTargetType type) throws IOException {
     checkSameFileSystem(target);
-    fileSystem.createSymbolicLink(asFragment(), target.asFragment());
+    fileSystem.createSymbolicLink(asFragment(), target.asFragment(), type);
+  }
+
+  /**
+   * Creates a symbolic link with the name of the current path, following symbolic links. The
+   * referent of the created symlink is is the absolute path "target"; it is not possible to create
+   * relative symbolic links via this method.
+   *
+   * @throws IOException if the creation of the symbolic link was unsuccessful for any reason
+   */
+  public void createSymbolicLink(Path target) throws IOException {
+    createSymbolicLink(target, SymlinkTargetType.UNSPECIFIED);
+  }
+
+  /**
+   * Creates a symbolic link with the name of the current path, following symbolic links. The
+   * referent of the created symlink is is the path fragment "target", which may be absolute or
+   * relative.
+   *
+   * <p>The {@code type} argument denotes the file type of the target, if known. Some filesystems
+   * require this information to correctly create a symlink. This argument may be ignored if the
+   * target can be observed to exist and is of a different type.
+   *
+   * @param type the target file type
+   * @throws IOException if the creation of the symbolic link was unsuccessful for any reason
+   */
+  public void createSymbolicLink(PathFragment target, SymlinkTargetType type) throws IOException {
+    fileSystem.createSymbolicLink(asFragment(), target, type);
   }
 
   /**
@@ -501,7 +533,7 @@ public void createSymbolicLink(Path target) throws IOException {
    * @throws IOException if the creation of the symbolic link was unsuccessful for any reason
    */
   public void createSymbolicLink(PathFragment target) throws IOException {
-    fileSystem.createSymbolicLink(asFragment(), target);
+    createSymbolicLink(target, SymlinkTargetType.UNSPECIFIED);
   }
 
   /**

src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java

@@ -119,9 +119,10 @@ protected boolean isSpecialFile(PathFragment path, boolean followSymlinks) {
   }
 
   @Override
-  protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
       throws IOException {
-    delegateFs.createSymbolicLink(toDelegatePath(linkPath), targetFragment);
+    delegateFs.createSymbolicLink(toDelegatePath(linkPath), targetFragment, type);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java

@@ -81,7 +81,8 @@ public void createDirectoryAndParents(PathFragment path) throws IOException {
   }
 
   @Override
-  protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
+  protected void createSymbolicLink(
+      PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
       throws IOException {
     throw modificationException();
   }