Fail build gracefully if an action discovers unexpected inputs.

Blaze discovers inputs for some actions when running with some
strategies. Those actions should not discover additional inputs
when they run, regardless of the strategy they end up using.

There are now no known legitimate cases of such additional input
discovery, so we should reinstate this check and find new ones :)

We also change the failure mode to be a normal error rather than
a crash. This error does indicate a tooling issue, and a small
chance of incorrect builds, but it doesn't create such an
inconsistent state that a crash is warranted.

--
Change-Id: I5d498d2fc1c5e23bfb5d77971f866c2027cbf03a
Reviewed-on: https://bazel-review.googlesource.com/#/c/2500/3
MOS_MIGRATED_REVID=109703508

1 files changed, 44 insertions, 36 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 76bb36e18d..cc40b8afd4 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
@@ -17,10 +17,10 @@ import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicates;
 import com.google.common.collect.Collections2;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
+import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionCacheChecker.Token;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -37,6 +37,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -56,6 +57,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
+import java.util.logging.Level;
 
 import javax.annotation.Nullable;
 
@@ -388,46 +390,52 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         // Include scanning didn't find anything beforehand -- these are the definitive discovered
         // inputs.
         state.discoveredInputs = metadataFoundDuringActionExecution.keySet();
-      } else {
-        // Sadly, even if we discovered inputs, sometimes the action runs and discovers more inputs.
-        // Technically, this means our pre-execution input discovery is buggy, but it turns out this
-        // is impractical to fix.
         if (env.valuesMissing()) {
-          // Any new inputs should already have been built -- this is a check that our input
-          // discovery code is not missing too much. It may have to be removed if further input
-          // discovery quirks are found.
-          Set<Artifact> missingArtifacts =
-              Maps.filterValues(metadataFoundDuringActionExecution, Predicates.isNull()).keySet();
-          throw new IllegalStateException(
-              "Missing artifacts: " + missingArtifacts + ", " + state + action);
+          return null;
         }
-        Set<FileArtifactValue> knownMetadata =
-            ImmutableSet.copyOf(state.inputArtifactData.values());
-        ImmutableSet.Builder<Artifact> discoveredInputBuilder =
-            ImmutableSet.<Artifact>builder().addAll(state.discoveredInputs);
-        for (Map.Entry<Artifact, FileArtifactValue> entry :
-            metadataFoundDuringActionExecution.entrySet()) {
-          Preconditions.checkState(knownMetadata.contains(entry.getValue()),
-              "%s %s", action, entry);
-          discoveredInputBuilder.add(entry.getKey());
+        if (!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;
+          metadataHandler =
+              new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm);
         }
-        state.discoveredInputs = discoveredInputBuilder.build();
-      }
-      if (env.valuesMissing()) {
-        return null;
-      }
-      if (!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;
-        metadataHandler =
-            new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm);
+      } else if (!metadataFoundDuringActionExecution.isEmpty()) {
+        // The action has run and discovered more inputs. This is a bug, probably the result of
+        // the action dynamically executing locally instead of remotely, and a discrepancy between
+        // our include scanning and the action's compiler. Fail the build so that the user notices,
+        // and also report the issue.
+        String errorMessage =
+            action.prettyPrint()
+                + " discovered unexpected inputs. This indicates a mismatch between "
+                + Constants.PRODUCT_NAME
+                + " and the action's compiler. Please report this issue. The ";
+        if (metadataFoundDuringActionExecution.size() > 10) {
+          errorMessage += "first ten ";
+        }
+        errorMessage += "additional inputs found were: ";
+        int artifactPrinted = 0;
+        for (Artifact extraArtifact : metadataFoundDuringActionExecution.keySet()) {
+          if (artifactPrinted >= 10) {
+            break;
+          }
+          if (artifactPrinted > 0) {
+            errorMessage += ", ";
+          }
+          artifactPrinted++;
+          errorMessage += extraArtifact.prettyPrint();
+        }
+        ActionExecutionException exception =
+            new ActionExecutionException(errorMessage, action, /*catastrophe=*/ false);
+        LoggingUtil.logToRemote(Level.SEVERE, errorMessage, exception);
+        throw exception;
       }
     }
+    Preconditions.checkState(!env.valuesMissing(), action);
     skyframeActionExecutor.afterExecution(action, metadataHandler, state.token);
     return state.value;
   }