Delay updating inputs of an action when processing the action cache until it is known that the action is a cache hit.

This adds momentary memory overhead when checking the action cache, but should prevent a host of potential errors.

Note that this cl assumes that an action that discovers its inputs does *not* take the names of its inputs into account when calculating its key, which is already stated as part of the javadoc of Action#getKey.

--
MOS_MIGRATED_REVID=88971626

6 files changed, 68 insertions, 63 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 09457474bd..324d0b4608 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -36,6 +36,8 @@ import com.google.devtools.build.lib.vfs.Symlinks;
 import java.io.IOException;
 import java.util.Collection;
 
+import javax.annotation.Nullable;
+
 /**
  * Abstract implementation of Action which implements basic functionality: the
  * inputs, outputs, and toString method.  Both input and output sets are
@@ -98,13 +100,20 @@ public abstract class AbstractAction implements Action {
         + " since it does not discover inputs");
   }
 
+  @Nullable
   @Override
-  public boolean updateInputsFromCache(ArtifactResolver artifactResolver,
+  public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver,
       PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
     throw new IllegalStateException(
         "Method must be overridden for actions that may have unknown inputs.");
   }
 
+  @Override
+  public void updateInputs(Iterable<Artifact> inputs) {
+    throw new IllegalStateException(
+        "Method must be overridden for actions that may have unknown inputs.");
+  }
+
   /**
    * Should only be overridden by actions that need to optionally insert inputs. Actions that
    * discover their inputs should use {@link #setInputs} to set the new iterable of inputs when they
@@ -119,8 +128,8 @@ public abstract class AbstractAction implements Action {
    * Set the inputs of the action. May only be used by an action that {@link #discoversInputs()}.
    * The iterable passed in is automatically made immutable.
    */
-  public void setInputs(Iterable<Artifact> inputs) {
-    Preconditions.checkState(discoversInputs());
+  protected void setInputs(Iterable<Artifact> inputs) {
+    Preconditions.checkState(discoversInputs(), this);
     this.inputs = CollectionUtils.makeImmutable(inputs);
     cachedInputCount = -1;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index 09c50fb1fc..3581ae0a31 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -107,26 +107,33 @@ public interface Action extends ActionMetadata, Describable {
       throws ActionExecutionException, InterruptedException;
 
   /**
-   * Method used to update action inputs based on the information contained in
+   * Method used to resolve action inputs based on the information contained in
    * the action cache. It will be called iff inputsKnown() is false for the
    * given action instance and there is a related cache entry in the action
    * cache.
    *
    * Method must be redefined for any action that may return
-   * inputsKnown() == false. It also expects that implementation will ensure
-   * that inputsKnown() returns true after call to this method.
+   * inputsKnown() == false.
    *
    * @param artifactResolver the artifact factory that can be used to manufacture artifacts
-   * @param inputPaths List of relative (to the execution root) input paths
    * @param resolver object which helps to resolve some of the artifacts
-   * @return false if some dependencies are missing and we need to update again later,
-   * otherwise true.
+   * @param inputPaths List of relative (to the execution root) input paths
+   * @return List of Artifacts corresponding to inputPaths, or null if some dependencies were
+   * missing and we need to try again later.
    */
-  boolean updateInputsFromCache(
+  @Nullable
+  Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver, PackageRootResolver resolver,
       Collection<PathFragment> inputPaths);
 
   /**
+   * Informs the action that its inputs are {@code inputs}, and that its inputs are now known. Can
+   * only be called for actions that discover inputs. After this method is called,
+   * {@link ActionMetadata#inputsKnown} should return true.
+   */
+  void updateInputs(Iterable<Artifact> inputs);
+
+  /**
    * Return a best-guess estimate of the operation's resource consumption on the
    * local host itself for use in scheduling.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index f40003b1a5..cda9cb99ad 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -18,6 +18,7 @@ import com.google.common.base.Predicate;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action.MiddlemanType;
 import com.google.devtools.build.lib.actions.cache.ActionCache;
+import com.google.devtools.build.lib.actions.cache.ActionCache.Entry;
 import com.google.devtools.build.lib.actions.cache.Digest;
 import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
@@ -85,17 +86,19 @@ public class ActionCacheChecker {
    *
    * @param entry cached action information.
    * @param action action to be validated.
+   * @param actionInputs the inputs of the action. Normally just the result of action.getInputs(),
+   * but if this action doesn't yet know its inputs, we check the inputs from the cache.
    * @param metadataHandler provider of metadata for the artifacts this action interacts with.
    * @param checkOutput true to validate output artifacts, Otherwise, just
    *                    validate inputs.
    *
    * @return true if at least one artifact has changed, false - otherwise.
    */
-  private boolean validateArtifacts(ActionCache.Entry entry, Action action,
-      MetadataHandler metadataHandler, boolean checkOutput) {
+  private boolean validateArtifacts(Entry entry, Action action,
+      Iterable<Artifact> actionInputs, MetadataHandler metadataHandler, boolean checkOutput) {
     Iterable<Artifact> artifacts = checkOutput
-        ? Iterables.concat(action.getOutputs(), action.getInputs())
-        : action.getInputs();
+        ? Iterables.concat(action.getOutputs(), actionInputs)
+        : actionInputs;
     Map<String, Metadata> mdMap = new HashMap<>();
     for (Artifact artifact : artifacts) {
       mdMap.put(artifact.getExecPathString(), metadataHandler.getMetadataMaybe(artifact));
@@ -150,41 +153,36 @@ public class ActionCacheChecker {
       }
       return null;
     }
-    ActionCache.Entry entry = null; // Populated lazily.
-
-    // Update action inputs from cache, if necessary.
+    Iterable<Artifact> actionInputs = action.getInputs();
+    ActionCache.Entry entry = getCacheEntry(action);
+    // Resolve action inputs from cache, if necessary.
     boolean inputsKnown = action.inputsKnown();
-    if (!inputsKnown) {
-      Preconditions.checkState(action.discoversInputs());
-      entry = getCacheEntry(action);
-      boolean ranSuccessfully = updateActionInputs(action, entry, resolver);
-      // If during update of inputs skyframe was missing some dependencies (for example,
-      // ContainingPackageLookupValue inside of ArtifactFactory.resolveSourceArtifact), we need to
-      // wait for those dependencies to be resolved. So next time when we will call corresponding
-      // ActionExecutionFunction all those dependencies will have been resolved and we can continue
-      // action execution process.
-      if (!ranSuccessfully) {
+    if (entry != null && !entry.isCorrupted() && !inputsKnown) {
+      Preconditions.checkState(action.discoversInputs(), action);
+      actionInputs = resolveCachedActionInputs(action, entry, resolver);
+      if (actionInputs == null) {
+        // Not all inputs could be resolved. Try again next time.
         return Token.NEED_TO_RERUN;
       }
     }
-    if (mustExecute(action, entry, handler, metadataHandler)) {
+    if (mustExecute(action, entry, handler, metadataHandler, actionInputs)) {
       return new Token(getKeyString(action));
     }
+
+    if (!inputsKnown) {
+      action.updateInputs(actionInputs);
+    }
     return null;
   }
 
   protected boolean mustExecute(Action action, @Nullable ActionCache.Entry entry,
-      EventHandler handler, MetadataHandler metadataHandler) {
+      EventHandler handler, MetadataHandler metadataHandler, Iterable<Artifact> actionInputs) {
     // Unconditional execution can be applied only for actions that are allowed to be executed.
     if (unconditionalExecution(action)) {
       Preconditions.checkState(action.isVolatile());
       reportUnconditionalExecution(handler, action);
       return true; // must execute - unconditional execution is requested.
     }
-
-    if (entry == null) {
-      entry = getCacheEntry(action);
-    }
     if (entry == null) {
       reportNewAction(handler, action);
       return true; // must execute -- no cache entry (e.g. first build)
@@ -193,7 +191,7 @@ public class ActionCacheChecker {
     if (entry.isCorrupted()) {
       reportCorruptedCacheEntry(handler, action);
       return true; // cache entry is corrupted - must execute
-    } else if (validateArtifacts(entry, action, metadataHandler, true)) {
+    } else if (validateArtifacts(entry, action, actionInputs, metadataHandler, true)) {
       reportChanged(handler, action);
       return true; // files have changed
     } else if (!entry.getActionKey().equals(action.getKey())){
@@ -230,11 +228,10 @@ public class ActionCacheChecker {
     actionCache.put(key, entry);
   }
 
-  protected boolean updateActionInputs(Action action, ActionCache.Entry entry,
+  private Iterable<Artifact> resolveCachedActionInputs(Action action, ActionCache.Entry entry,
       PackageRootResolver resolver) {
-    if (entry == null || entry.isCorrupted()) {
-      return true;
-    }
+    Preconditions.checkNotNull(entry, action);
+    Preconditions.checkState(!entry.isCorrupted(), action);
 
     List<PathFragment> outputs = new ArrayList<>();
     for (Artifact output : action.getOutputs()) {
@@ -249,7 +246,7 @@ public class ActionCacheChecker {
         inputs.add(execPath);
       }
     }
-    return action.updateInputsFromCache(artifactResolver, resolver, inputs);
+    return action.resolveInputsFromCache(artifactResolver, resolver, inputs);
   }
 
   /**
@@ -273,7 +270,7 @@ public class ActionCacheChecker {
       if (entry.isCorrupted()) {
         reportCorruptedCacheEntry(handler, action);
         changed = true;
-      } else if (validateArtifacts(entry, action, metadataHandler, false)) {
+      } else if (validateArtifacts(entry, action, action.getInputs(), metadataHandler, false)) {
         reportChanged(handler, action);
         changed = true;
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 5d2857a995..1d49d676f3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -836,7 +836,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   }
 
   @Override
-  public boolean updateInputsFromCache(
+  public Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver, PackageRootResolver resolver,
       Collection<PathFragment> inputPaths) {
     // Note that this method may trigger a violation of the desirable invariant that getInputs()
@@ -859,7 +859,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
     if (resolvedArtifacts == null) {
       // We are missing some dependencies. We need to rerun this update later.
-      return false;
+      return null;
     }
 
     for (PathFragment execPath : unresolvedPaths) {
@@ -873,11 +873,15 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         inputs.add(artifact);
       }
     }
+    return inputs;
+  }
+
+  @Override
+  public synchronized void updateInputs(Iterable<Artifact> inputs) {
     inputsKnown = true;
     synchronized (this) {
       setInputs(inputs);
     }
-    return true;
   }
 
   private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
index a1f82a9785..343b44302c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
@@ -152,28 +152,26 @@ public final class ExtraAction extends SpawnAction {
     return result.addAll(extraActionInputs).build();
   }
 
-  private void updateInputs(Iterable<Artifact> shadowedActionInputs) {
+  @Override
+  public void updateInputs(Iterable<Artifact> shadowedActionInputs) {
+    shadowedAction.updateInputs(shadowedActionInputs);
+    Preconditions.checkArgument(shadowedAction.inputsKnown(), "%s %s", this, shadowedAction);
     synchronized (this) {
       setInputs(createInputs(shadowedActionInputs, extraActionInputs));
       inputsKnown = true;
     }
   }
 
+  @Nullable
   @Override
-  public boolean updateInputsFromCache(ArtifactResolver artifactResolver,
+  public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver,
       PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
     // We update the inputs directly from the shadowed action.
     Set<PathFragment> extraActionPathFragments =
         ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs));
-    boolean noMissingDependencies = shadowedAction.updateInputsFromCache(artifactResolver, resolver,
+    return shadowedAction.resolveInputsFromCache(artifactResolver,
+        resolver,
         Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments)));
-    if (!noMissingDependencies) {
-      // This update needs to be rerun.
-      return false;
-    }
-    Preconditions.checkState(shadowedAction.inputsKnown(), "%s %s", this, shadowedAction);
-    updateInputs(shadowedAction.getInputs());
-    return true;
   }
 
   /**
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 812d1fa89b..2958c6e3c7 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
@@ -254,16 +254,6 @@ public class ActionExecutionFunction implements SkyFunction {
       for (Artifact artifact : inputs) {
         discoveredArtifacts.add(ArtifactValue.key(artifact, mandatory.contains(artifact)));
       }
-
-      // In case the action violates the invariant that getInputs() is a superset of
-      // getMandatoryInputs(), explicitly add the mandatory inputs. See bug about an
-      // "action not in canonical form" error message. Also note that we may add Skyframe edges on
-      // these potentially stale deps due to the way loading inputs from the action cache functions.
-      // In practice, this is safe since C++ actions (the only ones which discover inputs) only add
-      // possibly stale inputs on source artifacts, which we treat as non-mandatory.
-      for (Artifact artifact : mandatory) {
-        discoveredArtifacts.add(ArtifactValue.key(artifact, true));
-      }
       return discoveredArtifacts;
     }
   }