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

1 files changed, 8 insertions, 10 deletions

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;
   }
 
   /**