Remove PerActionFileCache

Instead, make ActionMetadataHandler implement the MetadataProvider interface.

This fixes an issue where an action that runs two spawns where one depends on
an output of the other was unable to get the metadata for the intermediate
output.

We don't currently have actions that do this, but we will have in a future
change (which will also implicitly act as a regression test).

PiperOrigin-RevId: 205629237

4 files changed, 57 insertions, 84 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 611e3f2059..aaa9221098 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -385,9 +385,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
           actionLookupData,
           /* inputDiscoveryRan= */ false);
     }
-    // This may be recreated if we discover inputs.
-    ActionMetadataHandler metadataHandler = new ActionMetadataHandler(state.inputArtifactData,
-        action.getOutputs(), tsgm.get(), pathResolver(state.actionFileSystem));
+    // The metadata handler is recreated below if the action performs input discovery.
+    ActionMetadataHandler metadataHandler = new ActionMetadataHandler(
+        state.inputArtifactData,
+        /* missingArtifactsAllowed= */ action.discoversInputs(),
+        action.getOutputs(),
+        tsgm.get(),
+        pathResolver(state.actionFileSystem));
     long actionStartTime = BlazeClock.nanoTime();
     // We only need to check the action cache if we haven't done it on a previous run.
     if (!state.hasCheckedActionCache()) {
@@ -422,19 +426,15 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     // Delete the metadataHandler's cache of the action's outputs, since they are being deleted.
     metadataHandler.discardOutputMetadata();
 
-    // This may be recreated if we discover inputs.
     // TODO(shahan): this isn't used when using ActionFileSystem so we can avoid creating some
     // unused objects.
-    PerActionFileCache perActionFileCache =
-        new PerActionFileCache(
-            state.inputArtifactData, /*missingArtifactsAllowed=*/ action.discoversInputs());
     if (action.discoversInputs()) {
       if (state.discoveredInputs == null) {
         try {
           state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler);
           state.discoveredInputs =
               skyframeActionExecutor.discoverInputs(
-                  action, perActionFileCache, metadataHandler, env, state.actionFileSystem);
+                  action, metadataHandler, env, state.actionFileSystem);
           Preconditions.checkState(state.discoveredInputs != null,
               "discoverInputs() returned null on action %s", action);
         } catch (MissingDepException e) {
@@ -447,8 +447,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       if (env.valuesMissing()) {
         return null;
       }
-      perActionFileCache =
-          new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
 
       // Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some
       // files with addDiscoveredInputs() and then have waited for those files to be available
@@ -467,11 +465,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         if (env.valuesMissing()) {
           return null;
         }
-        perActionFileCache =
-            new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
       }
       metadataHandler =
-          new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(),
+          new ActionMetadataHandler(
+              state.inputArtifactData,
+              /* missingArtifactsAllowed= */ false,
+              action.getOutputs(),
+              tsgm.get(),
               pathResolver(state.actionFileSystem));
       // Set the MetadataHandler to accept output information.
       metadataHandler.discardOutputMetadata();
@@ -503,7 +503,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler);
     try (ActionExecutionContext actionExecutionContext =
         skyframeActionExecutor.getContext(
-            perActionFileCache,
             metadataHandler,
             Collections.unmodifiableMap(state.expandedArtifacts),
             filesetMappings.build(),
@@ -545,7 +544,11 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         // markOmitted is only called for remote execution, and this code only gets executed for
         // local execution.
         metadataHandler =
-            new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(),
+            new ActionMetadataHandler(
+                state.inputArtifactData,
+                /*missingArtifactsAllowed=*/ false,
+                action.getOutputs(),
+                tsgm.get(),
                 pathResolver(state.actionFileSystem));
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 1d4012c60e..49159f401b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -83,6 +83,7 @@ public class ActionMetadataHandler implements MetadataHandler {
    * <p>This should never be read directly. Use {@link #getInputFileArtifactValue} instead.
    */
   private final ActionInputMap inputArtifactData;
+  private final boolean missingArtifactsAllowed;
 
   /** FileValues for each output Artifact. */
   private final ConcurrentMap<Artifact, FileValue> outputArtifactData =
@@ -135,10 +136,12 @@ public class ActionMetadataHandler implements MetadataHandler {
   @VisibleForTesting
   public ActionMetadataHandler(
       ActionInputMap inputArtifactData,
+      boolean missingArtifactsAllowed,
       Iterable<Artifact> outputs,
       TimestampGranularityMonitor tsgm,
       ArtifactPathResolver artifactPathResolver)  {
     this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
+    this.missingArtifactsAllowed = missingArtifactsAllowed;
     this.outputs = ImmutableSet.copyOf(outputs);
     this.tsgm = tsgm;
     this.artifactPathResolver = artifactPathResolver;
@@ -180,15 +183,25 @@ public class ActionMetadataHandler implements MetadataHandler {
   }
 
   @Override
-  public FileArtifactValue getMetadata(Artifact artifact) throws IOException {
+  public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException {
+    // TODO(shahan): is this bypass needed?
+    if (!(actionInput instanceof Artifact)) {
+      return null;
+    }
+
+    Artifact artifact = (Artifact) actionInput;
     FileArtifactValue value = getInputFileArtifactValue(artifact);
     if (value != null) {
       return metadataFromValue(value);
     }
+
     if (artifact.isSourceArtifact()) {
       // A discovered input we didn't have data for.
       // TODO(bazel-team): Change this to an assertion once Skyframe has native input discovery, so
       // all inputs will already have metadata known.
+      if (!missingArtifactsAllowed) {
+        throw new IllegalStateException(String.format("null for %s", artifact));
+      }
       return null;
     } else if (artifact.isMiddlemanArtifact()) {
       // A middleman artifact's data was either already injected from the action cache checker using
@@ -210,7 +223,11 @@ public class ActionMetadataHandler implements MetadataHandler {
       // Calling code depends on this particular exception.
       throw new FileNotFoundException(artifact + " not found");
     }
-    // It's an ordinary artifact.
+    // Fallthrough: the artifact must be a non-tree, non-middleman output artifact.
+
+    // Check for existing metadata. It may have been injected. In either case, this method is called
+    // from SkyframeActionExecutor to make sure that we have metadata for all action outputs, as the
+    // results are then stored in Skyframe (and the action cache).
     FileValue fileValue = outputArtifactData.get(artifact);
     if (fileValue != null) {
       // Non-middleman artifacts should only have additionalOutputData if they have
@@ -227,12 +244,28 @@ public class ActionMetadataHandler implements MetadataHandler {
       }
       return FileArtifactValue.createNormalFile(fileValue);
     }
-    // We do not cache exceptions besides nonexistence here, because it is unlikely that the file
-    // will be requested from this cache too many times.
+
+    // No existing metadata; this can happen if the output metadata is not injected after a spawn
+    // is executed. SkyframeActionExecutor.checkOutputs calls this method for every output file of
+    // the action, which hits this code path. Another possibility is that an action runs multiple
+    // spawns, and a subsequent spawn requests the metadata of an output of a previous spawn.
+    //
+    // Stat the file. All output artifacts of an action are deleted before execution, so if a file
+    // exists, it was most likely created by the current action. There is a race condition here if
+    // an external process creates (or modifies) the file between the deletion and this stat, which
+    // we cannot solve.
+    //
+    // We only cache nonexistence here, not file system errors. It is unlikely that the file will be
+    // requested from this cache too many times.
     fileValue = constructFileValue(artifact, /*statNoFollow=*/ null);
     return maybeStoreAdditionalData(artifact, fileValue, null);
   }
 
+  @Override
+  public ActionInput getInput(String execPath) {
+    return inputArtifactData.getInput(execPath);
+  }
+
   /**
    * Check that the new {@code data} we just calculated for an {@link Artifact} agrees with the
    * {@code oldData} (presumably calculated concurrently), if it was present.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
deleted file mode 100644
index f27bd369a9..0000000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
+++ /dev/null
@@ -1,61 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.skyframe;
-
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ActionInputMap;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.MetadataProvider;
-import javax.annotation.Nullable;
-
-/**
- * Cache provided by an {@link ActionExecutionFunction}, allowing Blaze to obtain artifact metadata
- * from the graph.
- *
- * <p>Data for the action's inputs is injected into this cache on construction, using the graph as
- * the source of truth.
- */
-class PerActionFileCache implements MetadataProvider {
-  private final ActionInputMap inputArtifactData;
-  private final boolean missingArtifactsAllowed;
-
-  /**
-   * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request.
-   * @param missingArtifactsAllowed whether to tolerate missing artifacts: can happen during input
-   *     discovery.
-   */
-  PerActionFileCache(ActionInputMap inputArtifactData, boolean missingArtifactsAllowed) {
-    this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
-    this.missingArtifactsAllowed = missingArtifactsAllowed;
-  }
-
-  @Nullable
-  @Override
-  public FileArtifactValue getMetadata(ActionInput input) {
-    // TODO(shahan): is this bypass needed?
-    if (!(input instanceof Artifact)) {
-      return null;
-    }
-    FileArtifactValue result = inputArtifactData.getMetadata(input);
-    Preconditions.checkState(missingArtifactsAllowed || result != null, "null for %s", input);
-    return result;
-  }
-
-  @Override
-  public ActionInput getInput(String execPath) {
-    return inputArtifactData.getInput(execPath);
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index a42fc65807..8cca0c45d4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -520,7 +520,6 @@ public final class SkyframeActionExecutor {
    * tasks related to that action.
    */
   public ActionExecutionContext getContext(
-      MetadataProvider graphFileCache,
       MetadataHandler metadataHandler,
       Map<Artifact, Collection<Artifact>> expandedInputs,
       ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
@@ -529,7 +528,7 @@ public final class SkyframeActionExecutor {
         ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot()));
     return new ActionExecutionContext(
         executorEngine,
-        createFileCache(graphFileCache, actionFileSystem),
+        createFileCache(metadataHandler, actionFileSystem),
         actionInputPrefetcher,
         actionKeyContext,
         metadataHandler,
@@ -649,7 +648,6 @@ public final class SkyframeActionExecutor {
    */
   Iterable<Artifact> discoverInputs(
       Action action,
-      PerActionFileCache graphFileCache,
       MetadataHandler metadataHandler,
       Environment env,
       @Nullable FileSystem actionFileSystem)
@@ -657,7 +655,7 @@ public final class SkyframeActionExecutor {
     ActionExecutionContext actionExecutionContext =
         ActionExecutionContext.forInputDiscovery(
             executorEngine,
-            createFileCache(graphFileCache, actionFileSystem),
+            createFileCache(metadataHandler, actionFileSystem),
             actionInputPrefetcher,
             actionKeyContext,
             metadataHandler,