[5.x] Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. (#14321)

* Remote: Invalidate actions if previous build used BwoB and remote cache is changed from enabled to disabled.

Part of #14252.

PiperOrigin-RevId: 410243297

* In `ArchivedTreeArtifact`, make the assumption that the relative output path is always a single segment, and use this to serialize less data.

This assumption holds because the origin of the relative output path (i.e. `bazel-out`) is `BlazeDirectories#getRelativeOutputPath`, which always returns a single-segment string. Instead of passing that around, just extract it from the tree artifact's exec path.

Additionally, the public `ArchivedTreeArtifact#create` method is removed in order to enforce a consistent naming pattern for all instances.

The codec supports custom derived tree roots even though there is currently no such serialization in skyframe (all serialized instances have the default `:archived_tree_artifacts`, but it was easy enough to be flexible).

PiperOrigin-RevId: 406382340

* Remote: Don't load remote metadata from action cache if remote cache is disabled.

Part of #14252.

Closes #14252.

PiperOrigin-RevId: 410448656

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

Author: coeuvre

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

@@ -401,19 +401,17 @@ public void repr(Printer printer) {
    *
    * @param execRoot the exec root in which this action is executed
    * @param bulkDeleter a helper to bulk delete outputs to avoid delegating to the filesystem
-   * @param outputPrefixForArchivedArtifactsCleanup derived output prefix to construct archived tree
-   *     artifacts to be cleaned up. If null, no cleanup is needed.
+   * @param cleanupArchivedArtifacts whether to clean up archived tree artifacts
    */
   protected final void deleteOutputs(
       Path execRoot,
       ArtifactPathResolver pathResolver,
       @Nullable BulkDeleter bulkDeleter,
-      @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
+      boolean cleanupArchivedArtifacts)
       throws IOException, InterruptedException {
     Iterable<Artifact> artifactsToDelete =
-        outputPrefixForArchivedArtifactsCleanup != null
-            ? Iterables.concat(
-                outputs, archivedTreeArtifactOutputs(outputPrefixForArchivedArtifactsCleanup))
+        cleanupArchivedArtifacts
+            ? Iterables.concat(outputs, archivedTreeArtifactOutputs())
             : outputs;
     Iterable<PathFragment> additionalPathOutputsToDelete = getAdditionalPathOutputsToDelete();
     Iterable<PathFragment> directoryOutputsToDelete = getDirectoryOutputsToDelete();
@@ -452,10 +450,10 @@ protected Iterable<PathFragment> getDirectoryOutputsToDelete() {
     return ImmutableList.of();
   }
 
-  private Iterable<Artifact> archivedTreeArtifactOutputs(PathFragment derivedPathPrefix) {
+  private Iterable<Artifact> archivedTreeArtifactOutputs() {
     return Iterables.transform(
         Iterables.filter(outputs, Artifact::isTreeArtifact),
-        tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree, derivedPathPrefix));
+        tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree));
   }
 
   /**
@@ -581,9 +579,9 @@ public void prepare(
       Path execRoot,
       ArtifactPathResolver pathResolver,
       @Nullable BulkDeleter bulkDeleter,
-      @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
+      boolean cleanupArchivedArtifacts)
       throws IOException, InterruptedException {
-    deleteOutputs(execRoot, pathResolver, bulkDeleter, outputPrefixForArchivedArtifactsCleanup);
+    deleteOutputs(execRoot, pathResolver, bulkDeleter, cleanupArchivedArtifacts);
   }
 
   @Override

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

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadCompatible;
 import com.google.devtools.build.lib.vfs.BulkDeleter;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -92,7 +91,7 @@ void prepare(
       Path execRoot,
       ArtifactPathResolver pathResolver,
       @Nullable BulkDeleter bulkDeleter,
-      @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
+      boolean cleanupArchivedArtifacts)
       throws IOException, InterruptedException;
 
   /**

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

@@ -22,6 +22,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.GoogleLogger;
+import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
@@ -72,7 +73,6 @@ public class ActionCacheChecker {
   private final Predicate<? super Action> executionFilter;
   private final ArtifactResolver artifactResolver;
   private final CacheConfig cacheConfig;
-  private final PathFragment derivedPathPrefix;
 
   @Nullable private final ActionCache actionCache; // Null when not enabled.
 
@@ -107,8 +107,7 @@ public ActionCacheChecker(
       ArtifactResolver artifactResolver,
       ActionKeyContext actionKeyContext,
       Predicate<? super Action> executionFilter,
-      @Nullable CacheConfig cacheConfig,
-      PathFragment derivedPathPrefix) {
+      @Nullable CacheConfig cacheConfig) {
     this.executionFilter = executionFilter;
     this.actionKeyContext = actionKeyContext;
     this.artifactResolver = artifactResolver;
@@ -125,7 +124,6 @@ public ActionCacheChecker(
     } else {
       this.actionCache = null;
     }
-    this.derivedPathPrefix = derivedPathPrefix;
   }
 
   public boolean isActionExecutionProhibited(Action action) {
@@ -310,10 +308,7 @@ private CachedOutputMetadata(
   }
 
   private static CachedOutputMetadata loadCachedOutputMetadata(
-      Action action,
-      ActionCache.Entry entry,
-      MetadataHandler metadataHandler,
-      PathFragment derivedPathPrefix) {
+      Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) {
     ImmutableMap.Builder<Artifact, RemoteFileArtifactValue> remoteFileMetadata =
         ImmutableMap.builder();
     ImmutableMap.Builder<SpecialArtifact, TreeArtifactValue> mergedTreeMetadata =
@@ -361,12 +356,9 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
               cachedTreeMetadata
                   .archivedFileValue()
                   .map(
-                      fileArtifactValue -> {
-                        Artifact.ArchivedTreeArtifact archivedTreeArtifact =
-                            Artifact.ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix);
-                        return ArchivedRepresentation.create(
-                            archivedTreeArtifact, fileArtifactValue);
-                      });
+                      fileArtifactValue ->
+                          ArchivedRepresentation.create(
+                              ArchivedTreeArtifact.createForTree(parent), fileArtifactValue));
         }
 
         TreeArtifactValue.Builder merged = TreeArtifactValue.newBuilder(parent);
@@ -422,7 +414,8 @@ public Token getTokenIfNeedToExecute(
       EventHandler handler,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
-      Map<String, String> remoteDefaultPlatformProperties)
+      Map<String, String> remoteDefaultPlatformProperties,
+      boolean isRemoteCacheEnabled)
       throws InterruptedException {
     // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that
     // produce only symlinks we should not check whether inputs are valid at all - all that matters
@@ -464,10 +457,12 @@ public Token getTokenIfNeedToExecute(
     }
     ActionCache.Entry entry = getCacheEntry(action);
     CachedOutputMetadata cachedOutputMetadata = null;
-    // load remote metadata from action cache
-    if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) {
-      cachedOutputMetadata =
-          loadCachedOutputMetadata(action, entry, metadataHandler, derivedPathPrefix);
+    if (entry != null
+        && !entry.isCorrupted()
+        && cacheConfig.storeOutputMetadata()
+        && isRemoteCacheEnabled) {
+      // load remote metadata from action cache
+      cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler);
     }
 
     if (mustExecute(

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

@@ -41,7 +41,6 @@
 import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
 import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.starlarkbuildapi.FileApi;
@@ -309,7 +308,7 @@ public ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact
   /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */
   public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact();
 
-  protected final ArtifactRoot root;
+  private final ArtifactRoot root;
 
   private final int hashCode;
   private final PathFragment execPath;
@@ -978,7 +977,7 @@ boolean ownersEqual(Artifact other) {
 
     @Override
     public PathFragment getRootRelativePath() {
-      return root.isExternal() ? getExecPath().subFragment(2) : getExecPath();
+      return getRoot().isExternal() ? getExecPath().subFragment(2) : getExecPath();
     }
 
     @Override
@@ -1168,10 +1167,10 @@ public SpecialArtifact deserialize(DeserializationContext context, CodedInputStr
    * TreeFileArtifact children} (and nothing else) of the tree artifact with their filesystem
    * structure, relative to the {@linkplain SpecialArtifact#getExecPath() tree artifact directory}.
    */
-  @AutoCodec
   public static final class ArchivedTreeArtifact extends DerivedArtifact {
-    private static final PathFragment ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT =
+    private static final PathFragment DEFAULT_DERIVED_TREE_ROOT =
         PathFragment.create(":archived_tree_artifacts");
+
     private final SpecialArtifact treeArtifact;
 
     private ArchivedTreeArtifact(
@@ -1180,6 +1179,8 @@ private ArchivedTreeArtifact(
         PathFragment execPath,
         Object generatingActionKey) {
       super(root, execPath, generatingActionKey);
+      Preconditions.checkArgument(
+          treeArtifact.isTreeArtifact(), "Not a tree artifact: %s", treeArtifact);
       this.treeArtifact = treeArtifact;
     }
 
@@ -1188,13 +1189,6 @@ public SpecialArtifact getParent() {
       return treeArtifact;
     }
 
-    /** Creates an archived tree artifact with a given {@code root} and {@code execPath}. */
-    public static ArchivedTreeArtifact create(
-        SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) {
-      return new ArchivedTreeArtifact(
-          treeArtifact, root, execPath, treeArtifact.getGeneratingActionKey());
-    }
-
     /**
      * Creates an {@link ArchivedTreeArtifact} for a given tree artifact at the path inferred from
      * the provided tree.
@@ -1206,13 +1200,12 @@ public static ArchivedTreeArtifact create(
      * {@linkplain ArchivedTreeArtifact artifact} of: {@code
      * bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/directory.zip}.
      */
-    public static ArchivedTreeArtifact createForTree(
-        SpecialArtifact treeArtifact, PathFragment derivedPathPrefix) {
-      return createWithCustomDerivedTreeRoot(
+    public static ArchivedTreeArtifact createForTree(SpecialArtifact treeArtifact) {
+      return createInternal(
           treeArtifact,
-          derivedPathPrefix,
-          ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT,
-          treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip"));
+          DEFAULT_DERIVED_TREE_ROOT,
+          treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip"),
+          treeArtifact.getGeneratingActionKey());
     }
 
     /**
@@ -1221,31 +1214,30 @@ public static ArchivedTreeArtifact createForTree(
      *
      * <p>Example: for a tree artifact with root of {@code bazel-out/k8-fastbuild/bin} returns an
      * {@linkplain ArchivedTreeArtifact artifact} of: {@code
-     * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code
-     * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
+     * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code
+     * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}.
+     *
+     * <p>Such artifacts should only be used as outputs of intermediate spawns. Action execution
+     * results must come from {@link #createForTree}.
      */
     public static ArchivedTreeArtifact createWithCustomDerivedTreeRoot(
+        SpecialArtifact treeArtifact, PathFragment derivedTreeRoot, PathFragment rootRelativePath) {
+      return createInternal(
+          treeArtifact, derivedTreeRoot, rootRelativePath, treeArtifact.getGeneratingActionKey());
+    }
+
+    private static ArchivedTreeArtifact createInternal(
         SpecialArtifact treeArtifact,
-        PathFragment derivedPathPrefix,
-        PathFragment customDerivedTreeRoot,
-        PathFragment rootRelativePath) {
-      ArtifactRoot artifactRoot =
-          createRootForArchivedArtifact(
-              treeArtifact.getRoot(), derivedPathPrefix, customDerivedTreeRoot);
-      return create(
-          treeArtifact, artifactRoot, artifactRoot.getExecPath().getRelative(rootRelativePath));
-    }
-
-    private static ArtifactRoot createRootForArchivedArtifact(
-        ArtifactRoot treeArtifactRoot,
-        PathFragment derivedPathPrefix,
-        PathFragment customDerivedTreeRoot) {
-      return ArtifactRoot.asDerivedRoot(
-          getExecRoot(treeArtifactRoot),
-          // e.g. bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin
-          RootType.Output,
-          getExecPathWithinCustomDerivedRoot(
-              derivedPathPrefix, customDerivedTreeRoot, treeArtifactRoot.getExecPath()));
+        PathFragment derivedTreeRoot,
+        PathFragment rootRelativePath,
+        Object generatingActionKey) {
+      ArtifactRoot treeRoot = treeArtifact.getRoot();
+      PathFragment archiveRoot = embedDerivedTreeRoot(treeRoot.getExecPath(), derivedTreeRoot);
+      return new ArchivedTreeArtifact(
+          treeArtifact,
+          ArtifactRoot.asDerivedRoot(getExecRoot(treeRoot), RootType.Output, archiveRoot),
+          archiveRoot.getRelative(rootRelativePath),
+          generatingActionKey);
     }
 
     /**
@@ -1255,23 +1247,22 @@ private static ArtifactRoot createRootForArchivedArtifact(
      * <p>Example: {@code bazel-out/k8-fastbuild/bin ->
      * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
      */
-    public static PathFragment getExecPathWithinArchivedArtifactsTree(
-        PathFragment derivedPathPrefix, PathFragment execPath) {
-      return getExecPathWithinCustomDerivedRoot(
-          derivedPathPrefix, ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT, execPath);
+    public static PathFragment getExecPathWithinArchivedArtifactsTree(PathFragment execPath) {
+      return embedDerivedTreeRoot(execPath, DEFAULT_DERIVED_TREE_ROOT);
     }
 
     /**
      * Translates provided output {@code execPath} to one under provided derived tree root.
      *
      * <p>Example: {@code bazel-out/k8-fastbuild/bin ->
-     * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}.
+     * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}.
      */
-    private static PathFragment getExecPathWithinCustomDerivedRoot(
-        PathFragment derivedPathPrefix, PathFragment customDerivedTreeRoot, PathFragment execPath) {
-      return derivedPathPrefix
-          .getRelative(customDerivedTreeRoot)
-          .getRelative(execPath.relativeTo(derivedPathPrefix));
+    private static PathFragment embedDerivedTreeRoot(
+        PathFragment execPath, PathFragment derivedTreeRoot) {
+      return execPath
+          .subFragment(0, 1)
+          .getRelative(derivedTreeRoot)
+          .getRelative(execPath.subFragment(1));
     }
 
     private static Path getExecRoot(ArtifactRoot artifactRoot) {
@@ -1284,16 +1275,42 @@ private static Path getExecRoot(ArtifactRoot artifactRoot) {
               0, rootPathFragment.segmentCount() - artifactRoot.getExecPath().segmentCount());
       return rootPath.getFileSystem().getPath(execRootPath);
     }
+  }
 
-    @VisibleForSerialization
-    @AutoCodec.Instantiator
-    static ArchivedTreeArtifact createForDeserialization(
-        SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) {
+  @SuppressWarnings("unused") // Codec used by reflection.
+  private static final class ArchivedTreeArtifactCodec
+      implements ObjectCodec<ArchivedTreeArtifact> {
+
+    @Override
+    public Class<ArchivedTreeArtifact> getEncodedClass() {
+      return ArchivedTreeArtifact.class;
+    }
+
+    @Override
+    public void serialize(
+        SerializationContext context, ArchivedTreeArtifact obj, CodedOutputStream codedOut)
+        throws SerializationException, IOException {
+      PathFragment derivedTreeRoot = obj.getRoot().getExecPath().subFragment(1, 2);
+
+      context.serialize(obj.getParent(), codedOut);
+      context.serialize(derivedTreeRoot, codedOut);
+      context.serialize(obj.getRootRelativePath(), codedOut);
+    }
+
+    @Override
+    public ArchivedTreeArtifact deserialize(
+        DeserializationContext context, CodedInputStream codedIn)
+        throws SerializationException, IOException {
+      SpecialArtifact treeArtifact = context.deserialize(codedIn);
+      PathFragment derivedTreeRoot = context.deserialize(codedIn);
+      PathFragment rootRelativePath = context.deserialize(codedIn);
       Object generatingActionKey =
           treeArtifact.hasGeneratingActionKey()
               ? treeArtifact.getGeneratingActionKey()
               : OMITTED_FOR_SERIALIZATION;
-      return new ArchivedTreeArtifact(treeArtifact, root, execPath, generatingActionKey);
+
+      return ArchivedTreeArtifact.createInternal(
+          treeArtifact, derivedTreeRoot, rootRelativePath, generatingActionKey);
     }
   }
 

src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java

@@ -113,7 +113,7 @@ private String getAdditionalWorkspaceStatus(
           } catch (IOException e) {
             throw createExecutionException(e, Code.STDERR_IO_EXCEPTION);
           }
-          return new String(stdoutStream.toByteArray(), UTF_8);
+          return stdoutStream.toString(UTF_8);
         }
       } catch (BadExitStatusException e) {
         throw createExecutionException(e, Code.NON_ZERO_EXIT);
@@ -159,7 +159,7 @@ public void prepare(
         Path execRoot,
         ArtifactPathResolver pathResolver,
         @Nullable BulkDeleter bulkDeleter,
-        @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup)
+        boolean cleanupArchivedArtifacts)
         throws IOException {
       // The default implementation of this method deletes all output files; override it to keep
       // the old stableStatus around. This way we can reuse the existing file (preserving its mtime)
@@ -356,8 +356,8 @@ public com.google.devtools.build.lib.shell.Command getCommand() {
   @Override
   public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) {
     return "build".equals(command.name())
-        ? ImmutableList.<Class<? extends OptionsBase>>of(WorkspaceStatusAction.Options.class)
-        : ImmutableList.<Class<? extends OptionsBase>>of();
+        ? ImmutableList.of(WorkspaceStatusAction.Options.class)
+        : ImmutableList.of();
   }
 
   @Override