Resolve inputs that were stored in action cache ahead of time so metadata for them can be retrieved with other inputs, avoiding a restart.

This also allows us to delete the UndeclaredInputsHandler, since we eagerly cache metadata before entering any ActionCacheChecker methods.

This should also allow us to split FileAndMetadataCache into two separate classes, as well as asserting that SingleBuildFileCache only sees non-artifact ActionInputs. To be done in follow-ups.

--
MOS_MIGRATED_REVID=89718712

3 files changed, 257 insertions, 278 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 02ef3b334e..9861aa1831 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
@@ -15,7 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
-import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.PackageIdentifier;
@@ -45,26 +44,32 @@ import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
 
+import javax.annotation.Nullable;
+
 /**
- * A builder for {@link ActionExecutionValue}s.
+ * A {@link SkyFunction} that creates {@link ActionExecutionValue}s. There are four points where
+ * this function can abort due to missing values in the graph:
+ * <ol>
+ *   <li>For actions that discover inputs, if missing metadata needed to resolve an artifact from a
+ *   string input in the action cache.</li>
+ *   <li>If missing metadata for artifacts in inputs (including the artifacts above).</li>
+ *   <li>For actions that discover inputs, if missing metadata for inputs discovered prior to
+ *   execution.</li>
+ *   <li>For actions that discover inputs, but do so during execution, if missing metadata for
+ *   inputs discovered during execution.</li>
+ * </ol>
  */
 public class ActionExecutionFunction implements SkyFunction, CompletionReceiver {
-
-  private static final Predicate<Artifact> IS_SOURCE_ARTIFACT = new Predicate<Artifact>() {
-    @Override
-    public boolean apply(Artifact input) {
-      return input.isSourceArtifact();
-    }
-  };
-
   private final SkyframeActionExecutor skyframeActionExecutor;
   private final TimestampGranularityMonitor tsgm;
   private ConcurrentMap<Action, ContinuationState> stateMap;
@@ -80,20 +85,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
   public SkyValue compute(SkyKey skyKey, Environment env) throws ActionExecutionFunctionException,
       InterruptedException {
     Action action = (Action) skyKey.argument();
-    Map<Artifact, FileArtifactValue> inputArtifactData = null;
-    Map<Artifact, Collection<Artifact>> expandedMiddlemen = null;
-    boolean alreadyRan = skyframeActionExecutor.probeActionExecution(action);
-    try {
-      Pair<Map<Artifact, FileArtifactValue>, Map<Artifact, Collection<Artifact>>> checkedInputs =
-          checkInputs(env, action, alreadyRan); // Declare deps on known inputs to action.
-
-      if (checkedInputs != null) {
-        inputArtifactData = checkedInputs.first;
-        expandedMiddlemen = checkedInputs.second;
-      }
-    } catch (ActionExecutionException e) {
-      throw new ActionExecutionFunctionException(e);
-    }
     // TODO(bazel-team): Non-volatile NotifyOnActionCacheHit actions perform worse in Skyframe than
     // legacy when they are not at the top of the action graph. In legacy, they are stored
     // separately, so notifying non-dirty actions is cheap. In Skyframe, they depend on the
@@ -103,14 +94,66 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       // Depending on the buildID ensure that these actions have a chance to execute.
       PrecomputedValue.BUILD_ID.get(env);
     }
+    // For restarts of this ActionExecutionFunction we use a ContinuationState variable, below, to
+    // avoid redoing work. However, if two actions are shared and the first one executes, when the
+    // second one goes to execute, we should detect that and short-circuit, even without taking
+    // ContinuationState into account.
+    boolean sharedActionAlreadyRan = skyframeActionExecutor.probeActionExecution(action);
+    ContinuationState state;
+    if (action.discoversInputs()) {
+      state = getState(action);
+    } else {
+      // Because this is a new state, all conditionals below about whether state has already done
+      // something will return false, and so we will execute all necessary steps.
+      state = new ContinuationState();
+    }
+    if (!state.hasCollectedInputs()) {
+      state.allInputs = collectInputs(action, env);
+      if (state.allInputs == null) {
+        // Missing deps.
+        return null;
+      }
+    } else if (state.allInputs.keysRequested != null) {
+      // Preserve the invariant that we ask for the same deps each build.
+      env.getValues(state.allInputs.keysRequested);
+      Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state);
+    }
+    Pair<Map<Artifact, FileArtifactValue>, Map<Artifact, Collection<Artifact>>> checkedInputs =
+        null;
+    try {
+      // Declare deps on known inputs to action. We do this unconditionally to maintain our
+      // invariant of asking for the same deps each build.
+      Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps
+          = env.getValuesOrThrow(toKeys(state.allInputs.getAllInputs(),
+                  action.discoversInputs() ? action.getMandatoryInputs() : null),
+              MissingInputFileException.class, ActionExecutionException.class);
+
+      if (!sharedActionAlreadyRan && !state.hasArtifactData()) {
+        // Do we actually need to find our metadata?
+        checkedInputs = checkInputs(env, action, inputDeps);
+      }
+    } catch (ActionExecutionException e) {
+      // Remove action from state map in case it's there (won't be unless it discovers inputs).
+      stateMap.remove(action);
+      throw new ActionExecutionFunctionException(e);
+    }
     if (env.valuesMissing()) {
+      // There was missing artifact metadata in the graph. Wait for it to be present.
       return null;
     }
 
+    if (checkedInputs != null) {
+      Preconditions.checkState(!state.hasArtifactData(), "%s %s", state, action);
+      state.inputArtifactData = checkedInputs.first;
+      state.expandedMiddlemen = checkedInputs.second;
+    }
+
     ActionExecutionValue result;
     try {
-      result = checkCacheAndExecuteIfNeeded(action, inputArtifactData, expandedMiddlemen, env);
+      result = checkCacheAndExecuteIfNeeded(action, state, env);
     } catch (ActionExecutionException e) {
+      // Remove action from state map in case it's there (won't be unless it discovers inputs).
+      stateMap.remove(action);
       // In this case we do not report the error to the action reporter because we have already
       // done it in SkyframeExecutor.reportErrorIfNotAbortingMode() method. That method
       // prints the error in the top-level reporter and also dumps the recorded StdErr for the
@@ -119,17 +162,70 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     }
 
     if (env.valuesMissing()) {
+      Preconditions.checkState(stateMap.containsKey(action), action);
       return null;
     }
 
+    // Remove action from state map in case it's there (won't be unless it discovers inputs).
+    stateMap.remove(action);
     return result;
   }
-  
+
+  /**
+   * An action's inputs needed for execution. May not just be the result of Action#getInputs(). If
+   * the action cache's view of this action contains additional inputs, it will request metadata for
+   * them, so we consider those inputs as dependencies of this action as well. Returns null if some
+   * dependencies were missing and this ActionExecutionFunction needs to restart.
+   */
+  @Nullable
+  private AllInputs collectInputs(Action action, Environment env) {
+    if (action.inputsKnown()) {
+      return new AllInputs(action.getInputs());
+    }
+    Preconditions.checkState(action.discoversInputs(), action);
+    PackageRootResolverWithEnvironment resolver = new PackageRootResolverWithEnvironment(env);
+    Iterable<Artifact> actionCacheInputs =
+        skyframeActionExecutor.getActionCachedInputs(action, resolver);
+    if (actionCacheInputs == null) {
+      Preconditions.checkState(env.valuesMissing(), action);
+      return null;
+    }
+    return new AllInputs(action.getInputs(), actionCacheInputs, resolver.keysRequested);
+  }
+
+  private static class AllInputs {
+    final Iterable<Artifact> defaultInputs;
+    @Nullable
+    final Iterable<Artifact> actionCacheInputs;
+    @Nullable
+    final List<SkyKey> keysRequested;
+
+    AllInputs(Iterable<Artifact> defaultInputs) {
+      this.defaultInputs = Preconditions.checkNotNull(defaultInputs);
+      this.actionCacheInputs = null;
+      this.keysRequested = null;
+    }
+
+    AllInputs(Iterable<Artifact> defaultInputs, Iterable<Artifact> actionCacheInputs,
+        List<SkyKey> keysRequested) {
+      this.defaultInputs = Preconditions.checkNotNull(defaultInputs);
+      this.actionCacheInputs = Preconditions.checkNotNull(actionCacheInputs);
+      this.keysRequested = keysRequested;
+    }
+
+    Iterable<Artifact> getAllInputs() {
+      return actionCacheInputs == null
+          ? defaultInputs
+          : Iterables.concat(defaultInputs, actionCacheInputs);
+    }
+  }
+
   /**
    * Skyframe implementation of {@link PackageRootResolver}. Should be used only from SkyFunctions,
    * because it uses SkyFunction.Environment for evaluation of ContainingPackageLookupValue.
    */
   private static class PackageRootResolverWithEnvironment implements PackageRootResolver {
+    final List<SkyKey> keysRequested = new ArrayList<>();
     private final Environment env;
 
     public PackageRootResolverWithEnvironment(Environment env) {
@@ -138,11 +234,15 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
     @Override
     public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths) {
-      Map<PathFragment, SkyKey> depKeys = new HashMap<>(); 
+      Preconditions.checkState(keysRequested.isEmpty(),
+          "resolver should only be called once: %s %s", keysRequested, execPaths);
+      Map<PathFragment, SkyKey> depKeys = new HashMap<>();
       // Create SkyKeys list based on execPaths.
       for (PathFragment path : execPaths) {
-        depKeys.put(path,
-            ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(path)));
+        SkyKey depKey =
+            ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(path));
+        depKeys.put(path, depKey);
+        keysRequested.add(depKey);
       }
       Map<SkyKey, SkyValue> values = env.getValues(depKeys.values());
       if (env.valuesMissing()) {
@@ -170,54 +270,28 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
   private ActionExecutionValue checkCacheAndExecuteIfNeeded(
       Action action,
-      Map<Artifact, FileArtifactValue> inputArtifactData,
-      Map<Artifact, Collection<Artifact>> expandedMiddlemen,
+      ContinuationState state,
       Environment env) throws ActionExecutionException, InterruptedException {
-    // If this is the second time we are here (because the action discovers inputs, and we had
-    // to restart the value builder after declaring our dependence on newly discovered inputs),
-    // the result returned here is the already-computed result from the first run.
-    // Similarly, if this is a shared action and the other action is the one that executed, we
-    // must use that other action's value, provided here, since it is populated with metadata
-    // for the outputs.
-    if (inputArtifactData == null) {
+    // If this is a shared action and the other action is the one that executed, we must use that
+    // other action's value, provided here, since it is populated with metadata for the outputs.
+    if (!state.hasArtifactData()) {
       return skyframeActionExecutor.executeAction(action, null, -1, null);
     }
-    ContinuationState state;
-    if (action.discoversInputs()) {
-      state = getState(action);
-    } else {
-      state = new ContinuationState();
-    }
     // This may be recreated if we discover inputs.
     FileAndMetadataCache fileAndMetadataCache = new FileAndMetadataCache(
-          inputArtifactData,
-          expandedMiddlemen,
+          state.inputArtifactData,
+          state.expandedMiddlemen,
           skyframeActionExecutor.getExecRoot(),
           action.getOutputs(),
-          // Only give the metadata cache the ability to look up Skyframe values if the action
-          // might have undeclared inputs. If those undeclared inputs are generated, they are
-          // present in Skyframe, so we can save a stat by looking them up directly.
-          action.discoversInputs() ? env : null,
-          tsgm);
-    MetadataHandler metadataHandler =
-          skyframeActionExecutor.constructMetadataHandler(fileAndMetadataCache);
+        tsgm);
     long actionStartTime = System.nanoTime();
     // We only need to check the action cache if we haven't done it on a previous run.
-    if (!state.hasDiscoveredInputs()) {
-      Token token = skyframeActionExecutor.checkActionCache(action, metadataHandler,
-          new PackageRootResolverWithEnvironment(env), actionStartTime);
-      if (token == Token.NEED_TO_RERUN) {
-        // Sadly, there is no state that we can preserve here across this restart.
-        return null;
-      }
-      state.token = token;
+    if (!state.hasCheckedActionCache()) {
+      state.token = skyframeActionExecutor.checkActionCache(action, fileAndMetadataCache,
+          actionStartTime, state.allInputs.actionCacheInputs);
     }
 
     if (state.token == null) {
-      if (action.discoversInputs()) {
-        // Action may have had its inputs updated. Keep track of those new inputs.
-        declareAdditionalDependencies(env, action);
-      }
       // We got a hit from the action cache -- no need to execute.
       return new ActionExecutionValue(
           fileAndMetadataCache.getOutputData(),
@@ -226,11 +300,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
     // This may be recreated if we discover inputs.
     ActionExecutionContext actionExecutionContext =
-        skyframeActionExecutor.constructActionExecutionContext(fileAndMetadataCache,
-            metadataHandler);
+        skyframeActionExecutor.constructActionExecutionContext(fileAndMetadataCache);
     boolean inputsDiscoveredDuringActionExecution = false;
-    ActionExecutionValue result;
-    Token token;
+    Map<Artifact, FileArtifactValue> metadataFoundDuringActionExecution = null;
     try {
       if (action.discoversInputs()) {
         if (!state.hasDiscoveredInputs()) {
@@ -242,35 +314,31 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
             inputsDiscoveredDuringActionExecution = true;
           }
         }
+        // state.discoveredInputs can be null even after include scanning if action discovers them
+        // during execution.
         if (state.discoveredInputs != null
-            && !inputArtifactData.keySet().containsAll(state.discoveredInputs)) {
-          inputArtifactData = addDiscoveredInputs(inputArtifactData, state.discoveredInputs,
-              env);
+            && !state.inputArtifactData.keySet().containsAll(state.discoveredInputs)) {
+          Map<Artifact, FileArtifactValue> inputArtifactData =
+              addDiscoveredInputs(state.inputArtifactData, state.discoveredInputs, env);
           if (env.valuesMissing()) {
-            // This is the only place that we actually preserve meaningful state across restarts.
             return null;
           }
+          state.inputArtifactData = inputArtifactData;
           fileAndMetadataCache = new FileAndMetadataCache(
-              inputArtifactData,
-              expandedMiddlemen,
+              state.inputArtifactData,
+              state.expandedMiddlemen,
               skyframeActionExecutor.getExecRoot(),
               action.getOutputs(),
-              null,
               tsgm
           );
-          actionExecutionContext = skyframeActionExecutor.constructActionExecutionContext(
-              fileAndMetadataCache, fileAndMetadataCache);
+          actionExecutionContext =
+              skyframeActionExecutor.constructActionExecutionContext(fileAndMetadataCache);
         }
       }
-      // Clear state before actual execution of action. It will never be needed again because
-      // skyframeActionExecutor is guaranteed to have a result after this.
-      token = state.token;
-      if (action.discoversInputs()) {
-        removeState(action);
+      if (!state.hasExecutedAction()) {
+        state.value = skyframeActionExecutor.executeAction(action,
+            fileAndMetadataCache, actionStartTime, actionExecutionContext);
       }
-      state = null;
-      result = skyframeActionExecutor.executeAction(action,
-          fileAndMetadataCache, actionStartTime, actionExecutionContext);
     } finally {
       try {
         actionExecutionContext.getFileOutErr().close();
@@ -278,11 +346,32 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         // Nothing we can do here.
       }
       if (inputsDiscoveredDuringActionExecution) {
-        declareAdditionalDependencies(env, action);
+        metadataFoundDuringActionExecution =
+            declareAdditionalDependencies(env, action, state.inputArtifactData.keySet());
+        state.discoveredInputs = metadataFoundDuringActionExecution.keySet();
       }
     }
-    skyframeActionExecutor.afterExecution(action, fileAndMetadataCache, token);
-    return result;
+    if (env.valuesMissing()) {
+      return null;
+    }
+    if (inputsDiscoveredDuringActionExecution && !metadataFoundDuringActionExecution.isEmpty()) {
+      // We are in the interesting case of an action that discovered its inputs during execution,
+      // and found some new ones, but the new ones were already present in the graph. We must
+      // therefore cache the metadata for those new ones.
+      Map<Artifact, FileArtifactValue> inputArtifactData = new HashMap<>();
+      inputArtifactData.putAll(state.inputArtifactData);
+      inputArtifactData.putAll(metadataFoundDuringActionExecution);
+      state.inputArtifactData = inputArtifactData;
+      fileAndMetadataCache = new FileAndMetadataCache(
+          state.inputArtifactData,
+          state.expandedMiddlemen,
+          skyframeActionExecutor.getExecRoot(),
+          action.getOutputs(),
+          tsgm
+      );
+    }
+    skyframeActionExecutor.afterExecution(action, fileAndMetadataCache, state.token);
+    return state.value;
   }
 
   private static Map<Artifact, FileArtifactValue> addDiscoveredInputs(
@@ -307,11 +396,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     if (env.valuesMissing()) {
       return null;
     }
-    for (Map.Entry<SkyKey, SkyValue> depsEntry : data.entrySet()) {
-      Artifact input = ArtifactValue.artifact(depsEntry.getKey());
-      result.put(input,
-          Preconditions.checkNotNull((FileArtifactValue) depsEntry.getValue(), input));
-    }
+    result.putAll(transformArtifactMetadata(data));
     return result;
   }
 
@@ -341,18 +426,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
    * missing. Some inputs may not yet be in the graph, in which case the builder should abort.
    */
   private Pair<Map<Artifact, FileArtifactValue>, Map<Artifact, Collection<Artifact>>> checkInputs(
-      Environment env, Action action, boolean alreadyRan) throws ActionExecutionException {
-    Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps =
-        env.getValuesOrThrow(toKeys(action.getInputs(), action.discoversInputs()
-            ? action.getMandatoryInputs() : null), MissingInputFileException.class,
-            ActionExecutionException.class);
-
-    // If the action was already run, then break out early. This avoids the cost of constructing the
-    // input map and expanded middlemen if they're not going to be used.
-    if (alreadyRan) {
-      return null;
-    }
-
+      Environment env, Action action,
+      Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps)
+      throws ActionExecutionException {
     int missingCount = 0;
     int actionFailures = 0;
     boolean catastrophe = false;
@@ -426,12 +502,27 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         Collections.unmodifiableMap(expandedMiddlemen));
   }
 
-  private static void declareAdditionalDependencies(Environment env, Action action) {
-    if (action.discoversInputs()) {
-      // TODO(bazel-team): Should this be all inputs, or just source files?
-      env.getValues(toKeys(Iterables.filter(action.getInputs(), IS_SOURCE_ARTIFACT),
-          action.getMandatoryInputs()));
+  /**
+   * Returns a map of artifact to artifact metadata for any of {@code action}s inputs that are not
+   * already in {@code knownInputs}. If some metadata was not available yet, the artifact is still
+   * present in the map but with null metadata.
+   */
+  private static Map<Artifact, FileArtifactValue> declareAdditionalDependencies(Environment env,
+      Action action, Set<Artifact> knownInputs) {
+    Preconditions.checkState(action.discoversInputs(), action);
+    Iterable<Artifact> newArtifacts =
+        Iterables.filter(action.getInputs(), Predicates.not(Predicates.in(knownInputs)));
+    return transformArtifactMetadata(
+        env.getValues(toKeys(newArtifacts, action.getMandatoryInputs())));
+  }
+
+  private static Map<Artifact, FileArtifactValue> transformArtifactMetadata(
+      Map<SkyKey, SkyValue> map) {
+    Map<Artifact, FileArtifactValue> result = new HashMap<>(); // May contain null values.
+    for (Map.Entry<SkyKey, SkyValue> entry : map.entrySet()) {
+      result.put(ArtifactValue.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
     }
+    return result;
   }
 
   /**
@@ -462,29 +553,61 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     return state;
   }
 
-  private void removeState(Action action) {
-    Preconditions.checkNotNull(stateMap.remove(action), action);
-  }
-
   /**
-   * State to save work across restarts of ActionExecutionFunction due to missing discovered inputs.
+   * State to save work across restarts of ActionExecutionFunction due to missing values in the
+   * graph for actions that discover inputs. There are three places where we save work, all for
+   * actions that discover inputs:
+   * <ol>
+   *   <li>If not all known input metadata (coming from Action#getInputs) is available yet, then the
+   *   calculated set of inputs (including the inputs resolved from the action cache) is saved.</li>
+   *   <li>If not all discovered inputs' metadata is available yet, then the known input metadata
+   *   together with the set of discovered inputs is saved, as well as the Token used to identify
+   *   this action to the action cache.</li>
+   *   <li>If, after execution, new inputs are discovered whose metadata is not yet available, then
+   *   the same data as in the previous case is saved, along with the actual result of execution.
+   *   </li>
+   * </ol>
    */
   private static class ContinuationState {
+    AllInputs allInputs;
+    Map<Artifact, FileArtifactValue> inputArtifactData = null;
+    Map<Artifact, Collection<Artifact>> expandedMiddlemen = null;
     Token token = null;
     Collection<Artifact> discoveredInputs = null;
+    ActionExecutionValue value = null;
+
+    boolean hasCollectedInputs() {
+      return allInputs != null;
+    }
+
+    boolean hasArtifactData() {
+      boolean result = inputArtifactData != null;
+      Preconditions.checkState(result == (expandedMiddlemen != null), this);
+      return result;
+    }
 
     // This will always be false for actions that don't discover their inputs, but we never restart
     // those actions in any case. For actions that do discover their inputs, they either discover
     // them before execution, in which case discoveredInputs will be non-null if that has already
-    // happened, or after execution, in which case they returned null when Action#discoverInputs()
-    // was called, and won't restart due to missing dependencies before execution.
+    // happened, or after execution, in which case we set discoveredInputs then.
     boolean hasDiscoveredInputs() {
       return discoveredInputs != null;
     }
 
+    boolean hasCheckedActionCache() {
+      // If token is null because there was an action cache hit, this method is never called again
+      // because we return immediately.
+      return token != null;
+    }
+
+    boolean hasExecutedAction() {
+      return value != null;
+    }
+
     @Override
     public String toString() {
-      return token + ", " + discoveredInputs;
+      return token + ", " + value + ", " + allInputs + ", " + inputArtifactData + ", "
+          + discoveredInputs;
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java
index 74a57968b5..b9b1284c64 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java
@@ -35,7 +35,6 @@ import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
-import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.protobuf.ByteString;
 
 import java.io.File;
@@ -82,20 +81,19 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
       new ConcurrentHashMap<>();
   private final Set<Artifact> injectedArtifacts = Sets.newConcurrentHashSet();
   private final ImmutableSet<Artifact> outputs;
-  @Nullable private final SkyFunction.Environment env;
   private final TimestampGranularityMonitor tsgm;
 
   private static final Interner<ByteString> BYTE_INTERNER = Interners.newWeakInterner();
 
+  @VisibleForTesting
   public FileAndMetadataCache(Map<Artifact, FileArtifactValue> inputArtifactData,
       Map<Artifact, Collection<Artifact>> expandedInputMiddlemen, File execRoot,
-      Iterable<Artifact> outputs, @Nullable SkyFunction.Environment env,
+      Iterable<Artifact> outputs,
       TimestampGranularityMonitor tsgm) {
     this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
     this.expandedInputMiddlemen = Preconditions.checkNotNull(expandedInputMiddlemen);
     this.execRoot = Preconditions.checkNotNull(execRoot);
     this.outputs = ImmutableSet.copyOf(outputs);
-    this.env = env;
     this.tsgm = tsgm;
   }
 
@@ -131,47 +129,10 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
 
   @Nullable
   private FileArtifactValue getInputFileArtifactValue(ActionInput input) {
-    FileArtifactValue value = inputArtifactData.get(input);
-    if (value != null) {
-      return value;
-    }
-    if (outputs.contains(input)) {
-      // When this method is called to calculate the metadata of an artifact, the artifact may be an
-      // output artifact. Don't try to do anything then.
-      return null;
-    }
-    if (!(input instanceof Artifact)) {
-      // Maybe we're being asked for some strange constructed ActionInput coming from runfiles or
-      // similar. We have no information about such things.
-      return null;
-    }
-    // TODO(bazel-team): Remove this codepath once Skyframe has native input discovery, so all
-    // inputs will already have metadata known.
-    // ActionExecutionFunction may have passed in null environment if this action does not
-    // discover inputs. In which case we should not have gotten here.
-    Preconditions.checkNotNull(env, input);
-    Artifact artifact = (Artifact) input;
-    if (artifact.isSourceArtifact()) {
-      // We might have no artifact data for discovered source inputs, and it's not worth storing
-      // it in this cache, because it won't be reused across actions -- while we could request an
-      // artifact from the graph, we would have to be tolerant to it not yet being present in the
-      // graph yet, which adds complexity. Instead, we let the undeclared inputs handler stat it, so
-      // it can be reused.
+    if (outputs.contains(input) || !(input instanceof Artifact)) {
       return null;
-    } else {
-      // This getValue call is not expected to return null, because if the artifact is a
-      // transitive dependency of this action (as it should be), it will already have been built,
-      // so this call will return a value.
-      // This getValue call is theoretically less efficient for a subsequent incremental build
-      // than it would be to do a bulk getValues call after action execution, as is done for
-      // source inputs. However, since almost all nodes requested here are transitive deps of an
-      // already-declared dependency, change pruning will request these values serially, but they
-      // will already have been built. So the only penalty is restarting ParallelEvaluator#run, as
-      // opposed to traversing the entire downward transitive closure on a single thread.
-      value = (FileArtifactValue) env.getValue(
-          FileArtifactValue.key(artifact, /*argument ignored for derived artifacts*/false));
-      return value;
     }
+    return Preconditions.checkNotNull(inputArtifactData.get(input), input);
   }
 
   /**
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 b0a2cfb7b5..4dc14788e7 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
@@ -50,9 +50,6 @@ import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.TargetOutOfDateException;
-import com.google.devtools.build.lib.actions.cache.Digest;
-import com.google.devtools.build.lib.actions.cache.DigestUtils;
-import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.concurrent.ExecutorShutdownUtil;
 import com.google.devtools.build.lib.concurrent.Sharder;
@@ -65,7 +62,6 @@ import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.util.io.OutErr;
-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.Symlinks;
@@ -104,7 +100,6 @@ public final class SkyframeActionExecutor {
   private Executor executorEngine;
   private ActionLogBufferPathGenerator actionLogBufferPathGenerator;
   private ActionCacheChecker actionCacheChecker;
-  private ConcurrentMap<Artifact, Metadata> undeclaredInputsMetadata = new ConcurrentHashMap<>();
   private final Profiler profiler = Profiler.instance();
   private boolean explain;
 
@@ -186,102 +181,6 @@ public final class SkyframeActionExecutor {
   }
 
   /**
-   * Basic implementation of {@link MetadataHandler} that delegates to Skyframe for metadata and
-   * caches missing source artifacts (which must be undeclared inputs: discovered headers) to avoid
-   * excessive filesystem access. The discovered-header cache is available across actions.
-   */
-  // TODO(bazel-team): remove when include scanning is skyframe-native.
-  private static class UndeclaredInputHandler implements MetadataHandler {
-    private final ConcurrentMap<Artifact, Metadata> undeclaredInputsMetadata;
-    private final MetadataHandler perActionHandler;
-
-    UndeclaredInputHandler(MetadataHandler perActionHandler,
-        ConcurrentMap<Artifact, Metadata> undeclaredInputsMetadata) {
-      // Shared across all UndeclaredInputHandlers in this build.
-      this.undeclaredInputsMetadata = undeclaredInputsMetadata;
-      this.perActionHandler = perActionHandler;
-    }
-
-    @Override
-    public Metadata getMetadataMaybe(Artifact artifact) {
-      try {
-        return getMetadata(artifact);
-      } catch (IOException e) {
-        return null;
-      }
-    }
-
-    @Override
-    public Metadata getMetadata(Artifact artifact) throws IOException {
-      Metadata metadata = perActionHandler.getMetadata(artifact);
-      if (metadata != null) {
-        return metadata;
-      }
-      // Skyframe stats all generated artifacts, because either they are outputs of the action being
-      // executed or they are generated files already present in the graph.
-      Preconditions.checkState(artifact.isSourceArtifact(), artifact);
-      metadata = undeclaredInputsMetadata.get(artifact);
-      if (metadata != null) {
-        return metadata;
-      }
-      FileStatus stat = artifact.getPath().stat();
-      if (DigestUtils.useFileDigest(artifact, stat.isFile(), stat.getSize())) {
-        metadata = new Metadata(Preconditions.checkNotNull(
-            DigestUtils.getDigestOrFail(artifact.getPath(), stat.getSize()), artifact));
-      } else {
-        metadata = new Metadata(stat.getLastModifiedTime());
-      }
-      // Cache for other actions that may also include without declaring.
-      Metadata oldMetadata = undeclaredInputsMetadata.put(artifact, metadata);
-      FileAndMetadataCache.checkInconsistentData(artifact, oldMetadata, metadata);
-      return metadata;
-    }
-
-    @Override
-    public void setDigestForVirtualArtifact(Artifact artifact, Digest digest) {
-      perActionHandler.setDigestForVirtualArtifact(artifact, digest);
-    }
-
-    @Override
-    public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
-      perActionHandler.injectDigest(output, statNoFollow, digest);
-    }
-
-    @Override
-    public boolean artifactExists(Artifact artifact) {
-      return perActionHandler.artifactExists(artifact);
-    }
-
-    @Override
-    public boolean isRegularFile(Artifact artifact) {
-      return perActionHandler.isRegularFile(artifact);
-    }
-
-    @Override
-    public boolean isInjected(Artifact artifact) throws IOException {
-      return perActionHandler.isInjected(artifact);
-    }
-
-    @Override
-    public void discardMetadata(Collection<Artifact> artifactList) {
-      // This input handler only caches undeclared inputs, which never need to be discarded
-      // intra-build.
-      perActionHandler.discardMetadata(artifactList);
-    }
-
-    @Override
-    public void markOmitted(ActionInput output) {
-      perActionHandler.markOmitted(output);
-    }
-
-    @Override
-    public boolean artifactOmitted(Artifact artifact) {
-      return perActionHandler.artifactOmitted(artifact);
-    }
-
-  }
-
-  /**
    * Find conflicts between generated artifacts. There are two ways to have conflicts. First, if
    * two (unshareable) actions generate the same output artifact, this will result in an {@link
    * ActionConflictException}. Second, if one action generates an artifact whose path is a prefix of
@@ -440,7 +339,6 @@ public final class SkyframeActionExecutor {
     this.hadExecutionError = false;
     this.actionCacheChecker = Preconditions.checkNotNull(actionCacheChecker);
     // Don't cache possibly stale data from the last build.
-    undeclaredInputsMetadata = new ConcurrentHashMap<>();
     this.explain = explain;
   }
 
@@ -525,14 +423,14 @@ public final class SkyframeActionExecutor {
    * pass the returned context to {@link #executeAction}, and any other method that needs to execute
    * tasks related to that action.
    */
-  ActionExecutionContext constructActionExecutionContext(final FileAndMetadataCache graphFileCache,
-      MetadataHandler metadataHandler) {
+  ActionExecutionContext constructActionExecutionContext(
+      final FileAndMetadataCache graphFileCache) {
     // TODO(bazel-team): this should be closed explicitly somewhere.
     FileOutErr fileOutErr = actionLogBufferPathGenerator.generate();
     return new ActionExecutionContext(
         executorEngine,
         new DelegatingPairFileCache(graphFileCache, perBuildFileCache),
-        metadataHandler,
+        graphFileCache,
         fileOutErr,
         new MiddlemanExpander() {
           @Override
@@ -550,24 +448,16 @@ public final class SkyframeActionExecutor {
   }
 
   /**
-   * Returns a MetadataHandler for use when executing a particular action. The caller can pass the
-   * returned handler in whenever a MetadataHandler is needed in the course of executing the action.
-   */
-  MetadataHandler constructMetadataHandler(MetadataHandler graphFileCache) {
-    return new UndeclaredInputHandler(graphFileCache, undeclaredInputsMetadata);
-  }
-
-  /**
    * Checks the action cache to see if {@code action} needs to be executed, or is up to date.
    * Returns a token with the semantics of {@link ActionCacheChecker#getTokenIfNeedToExecute}: null
    * if the action is up to date, and non-null if it needs to be executed, in which case that token
    * should be provided to the ActionCacheChecker after execution.
    */
   Token checkActionCache(Action action, MetadataHandler metadataHandler,
-      PackageRootResolver resolver, long actionStartTime) {
+      long actionStartTime, Iterable<Artifact> resolvedCacheArtifacts) {
     profiler.startTask(ProfilerTask.ACTION_CHECK, action);
     Token token = actionCacheChecker.getTokenIfNeedToExecute(
-        action, explain ? reporter : null, metadataHandler, resolver);
+        action, resolvedCacheArtifacts, explain ? reporter : null, metadataHandler);
     profiler.completeTask(ProfilerTask.ACTION_CHECK);
     if (token == null) {
       boolean eventPosted = false;
@@ -603,6 +493,11 @@ public final class SkyframeActionExecutor {
     }
   }
 
+  @Nullable
+  Iterable<Artifact> getActionCachedInputs(Action action, PackageRootResolver resolver) {
+    return actionCacheChecker.getCachedInputs(action, resolver);
+  }
+
   /**
    * Perform dependency discovery for action, which must discover its inputs.
    *