Automated rollback of commit ceece65.

*** Reason for rollback ***

Suspected as cause of b/214264446.

*** Original change description ***

Stop re-requesting ActionExecutionFunction deps after input discovery causes a Skyframe restart: we no longer have the invariant that a SkyFunction requests the same deps on each evaluation, and iterating over all the inputs of an action isn't trivial.

PiperOrigin-RevId: 421415386

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -28,8 +28,8 @@
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
-import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionCacheChecker.Token;
@@ -248,6 +248,11 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env)
         // Missing deps.
         return null;
       }
+    } else if (state.allInputs.packageLookupsRequested != null) {
+      // Preserve the invariant that we ask for the same deps each build. Because we know these deps
+      // were already retrieved successfully, we don't request them with exceptions.
+      env.getValues(state.allInputs.packageLookupsRequested);
+      Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state);
     }
 
     CheckInputResults checkedInputs = null;
@@ -257,22 +262,22 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env)
                 .actionFileSystemType()
                 .equals(ActionFileSystemType.STAGE_REMOTE_FILES));
 
+    Iterable<SkyKey> depKeys = getInputDepKeys(allInputs, state);
+    // We do this unconditionally to maintain our invariant of asking for the same deps each build.
+    List<
+            ValueOrException3<
+                SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>>
+        inputDeps =
+            env.getOrderedValuesOrThrow(
+                depKeys,
+                SourceArtifactException.class,
+                ActionExecutionException.class,
+                ArtifactNestedSetEvalException.class);
+
     try {
       if (previousExecution == null && !state.hasArtifactData()) {
         // Do we actually need to find our metadata?
-        Iterable<SkyKey> depKeys = getInputDepKeys(allInputs, state);
-        checkedInputs =
-            checkInputs(
-                env,
-                action,
-                env.getOrderedValuesOrThrow(
-                    depKeys,
-                    SourceArtifactException.class,
-                    ActionExecutionException.class,
-                    ArtifactNestedSetEvalException.class),
-                allInputs,
-                depKeys,
-                actionLookupData);
+        checkedInputs = checkInputs(env, action, inputDeps, allInputs, depKeys, actionLookupData);
       }
     } catch (ActionExecutionException e) {
       throw new ActionExecutionFunctionException(e);
@@ -334,14 +339,7 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env)
               actionStartTime);
     } catch (LostInputsActionExecutionException e) {
       return handleLostInputs(
-          e,
-          actionLookupData,
-          action,
-          actionStartTime,
-          env,
-          allInputs,
-          getInputDepKeys(allInputs, state),
-          state);
+          e, actionLookupData, action, actionStartTime, env, inputDeps, allInputs, depKeys, state);
     } catch (ActionExecutionException e) {
       // In this case we do not report the error to the action reporter because we have already
       // done it in SkyframeActionExecutor.reportErrorIfNotAbortingMode() method. That method
@@ -420,6 +418,12 @@ private SkyFunction.Restart handleLostInputs(
       Action action,
       long actionStartTime,
       Environment env,
+      List<
+              ValueOrException3<
+                  SourceArtifactException,
+                  ActionExecutionException,
+                  ArtifactNestedSetEvalException>>
+          inputDeps,
       NestedSet<Artifact> allInputs,
       Iterable<SkyKey> depKeys,
       InputDiscoveryState state)
@@ -432,7 +436,8 @@ private SkyFunction.Restart handleLostInputs(
     RewindPlan rewindPlan = null;
     try {
       ActionInputDepOwners inputDepOwners =
-          createAugmentedInputDepOwners(e, action, env, allInputs, actionLookupData);
+          createAugmentedInputDepOwners(
+              e, action, env, inputDeps, allInputs, depKeys, actionLookupData);
 
       // Collect the set of direct deps of this action which may be responsible for the lost inputs,
       // some of which may be discovered.
@@ -511,7 +516,14 @@ private ActionInputDepOwners createAugmentedInputDepOwners(
       LostInputsActionExecutionException e,
       Action action,
       Environment env,
+      List<
+              ValueOrException3<
+                  SourceArtifactException,
+                  ActionExecutionException,
+                  ArtifactNestedSetEvalException>>
+          inputDeps,
       NestedSet<Artifact> allInputs,
+      Iterable<SkyKey> requestedSkyKeys,
       ActionLookupData actionLookupDataForError)
       throws InterruptedException {
 
@@ -528,7 +540,9 @@ private ActionInputDepOwners createAugmentedInputDepOwners(
           getInputDepOwners(
               env,
               action,
+              inputDeps,
               allInputs,
+              requestedSkyKeys,
               lostInputsAndOwnersSoFar,
               actionLookupDataForError);
     } catch (ActionExecutionException unexpected) {
@@ -1085,7 +1099,14 @@ private CheckInputResults checkInputs(
   private ActionInputDepOwnerMap getInputDepOwners(
       Environment env,
       Action action,
+      List<
+              ValueOrException3<
+                  SourceArtifactException,
+                  ActionExecutionException,
+                  ArtifactNestedSetEvalException>>
+          inputDeps,
       NestedSet<Artifact> allInputs,
+      Iterable<SkyKey> requestedSkyKeys,
       Collection<ActionInput> lostInputs,
       ActionLookupData actionLookupDataForError)
       throws ActionExecutionException, InterruptedException {
@@ -1096,9 +1117,9 @@ private ActionInputDepOwnerMap getInputDepOwners(
     return accumulateInputs(
         env,
         action,
-        /*inputDeps=*/ null,
+        inputDeps,
         allInputs,
-        /*requestedSkyKeys=*/ null,
+        requestedSkyKeys,
         ignoredInputDepsSize -> new ActionInputDepOwnerMap(lostInputs),
         (actionInputMapSink,
             expandedArtifacts,
@@ -1135,56 +1156,48 @@ public boolean test(Artifact input) {
   /**
    * May return {@code null} if {@code allowValuesMissingEarlyReturn} and {@code
    * env.valuesMissing()} are true and no inputs result in {@link ActionExecutionException}s.
-   *
-   * <p>If {@code inputDeps} (and therefore {@code requestedSkyKeys}) is null, assumes that deps
-   * have already been checked for exceptions and inserted into the {@link
-   * ArtifactNestedSetFunction} cache, so those steps are skipped. (This codepath currently only
-   * used for action rewinding.)
    */
   @Nullable
   private <S extends ActionInputMapSink, R> R accumulateInputs(
       Environment env,
       Action action,
-      @Nullable
-          List<
-                  ValueOrException3<
-                      SourceArtifactException,
-                      ActionExecutionException,
-                      ArtifactNestedSetEvalException>>
-              inputDeps,
+      List<
+              ValueOrException3<
+                  SourceArtifactException,
+                  ActionExecutionException,
+                  ArtifactNestedSetEvalException>>
+          inputDeps,
       NestedSet<Artifact> allInputs,
-      @Nullable Iterable<SkyKey> requestedSkyKeys,
+      Iterable<SkyKey> requestedSkyKeys,
       IntFunction<S> actionInputMapSinkFactory,
       AccumulateInputResultsFactory<S, R> accumulateInputResultsFactory,
       boolean allowValuesMissingEarlyReturn,
       ActionLookupData actionLookupDataForError)
       throws ActionExecutionException, InterruptedException {
     Predicate<Artifact> isMandatoryInput = makeMandatoryInputPredicate(action);
-    ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = null;
-    if (inputDeps != null) {
-      actionExecutionFunctionExceptionHandler =
-          new ActionExecutionFunctionExceptionHandler(
-              Suppliers.memoize(
-                  () -> {
-                    ImmutableList<Artifact> allInputsList = allInputs.toList();
-                    SetMultimap<SkyKey, Artifact> skyKeyToArtifactSet =
-                        MultimapBuilder.hashKeys().hashSetValues().build();
-                    allInputsList.forEach(
-                        input -> {
-                          SkyKey key = Artifact.key(input);
-                          if (key != input) {
-                            skyKeyToArtifactSet.put(key, input);
-                          }
-                        });
-                    return skyKeyToArtifactSet;
-                  }),
-              inputDeps,
-              action,
-              isMandatoryInput,
-              requestedSkyKeys,
-              env.valuesMissing());
-      actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions();
-    }
+    ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler =
+        new ActionExecutionFunctionExceptionHandler(
+            Suppliers.memoize(
+                () -> {
+                  ImmutableList<Artifact> allInputsList = allInputs.toList();
+                  Multimap<SkyKey, Artifact> skyKeyToArtifactSet =
+                      MultimapBuilder.hashKeys().hashSetValues().build();
+                  allInputsList.forEach(
+                      input -> {
+                        SkyKey key = Artifact.key(input);
+                        if (key != input) {
+                          skyKeyToArtifactSet.put(key, input);
+                        }
+                      });
+                  return skyKeyToArtifactSet;
+                }),
+            inputDeps,
+            action,
+            isMandatoryInput,
+            requestedSkyKeys,
+            env.valuesMissing());
+
+    actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions();
 
     if (env.valuesMissing() && allowValuesMissingEarlyReturn) {
       return null;
@@ -1251,13 +1264,8 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
       }
       if (value instanceof MissingArtifactValue) {
         if (isMandatoryInput.test(input)) {
-          checkNotNull(
-                  actionExecutionFunctionExceptionHandler,
-                  "Missing artifact should have been caught already %s %s %s",
-                  input,
-                  value,
-                  action)
-              .accumulateMissingFileArtifactValue(input, (MissingArtifactValue) value);
+          actionExecutionFunctionExceptionHandler.accumulateMissingFileArtifactValue(
+              input, (MissingArtifactValue) value);
           continue;
         } else {
           value = FileArtifactValue.MISSING_FILE_MARKER;
@@ -1273,12 +1281,9 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
           value,
           env);
     }
-
-    if (actionExecutionFunctionExceptionHandler != null) {
-      // After accumulating the inputs, we might find some mandatory artifact with
-      // SourceFileInErrorArtifactValue.
-      actionExecutionFunctionExceptionHandler.maybeThrowException();
-    }
+    // After accumulating the inputs, we might find some mandatory artifact with
+    // SourceFileInErrorArtifactValue.
+    actionExecutionFunctionExceptionHandler.maybeThrowException();
 
     return accumulateInputResultsFactory.create(
         inputArtifactData,
@@ -1468,7 +1473,7 @@ public boolean isCatastrophic() {
 
   /** Helper subclass for the error-handling logic for ActionExecutionFunction#accumulateInputs. */
   private final class ActionExecutionFunctionExceptionHandler {
-    private final Supplier<SetMultimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions;
+    private final Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions;
     private final List<
             ValueOrException3<
                 SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>>
@@ -1482,7 +1487,7 @@ private final class ActionExecutionFunctionExceptionHandler {
     private ActionExecutionException firstActionExecutionException;
 
     ActionExecutionFunctionExceptionHandler(
-        Supplier<SetMultimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions,
+        Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions,
         List<
                 ValueOrException3<
                     SourceArtifactException,