Proper action output checks for TreeArtifacts. Instead of crashing Bazel, we now handle failed TreeArtifact output checks gracefully.

--
MOS_MIGRATED_REVID=136627086

8 files changed, 82 insertions, 79 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
index f7a2706ac1..9838e032a8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -66,15 +66,6 @@ public interface MetadataHandler {
    */
   void markOmitted(ActionInput output);
 
-  /**
-   * Returns true iff artifact exists.
-   *
-   * <p>It is important to note that implementations may cache non-existence as a side effect of
-   * this method. If there is a possibility an artifact was intentionally omitted then {@link
-   * #artifactOmitted(Artifact)} should be checked first to avoid the side effect.
-   */
-  boolean artifactExists(Artifact artifact);
-
   /** Returns true iff artifact is a regular file. */
   boolean isRegularFile(Artifact artifact);
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index a7230a6eb1..9a82af3b61 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -26,7 +26,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.cache.Md5Digest;
 import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
-import com.google.devtools.build.lib.skyframe.TreeArtifactValue.TreeArtifactException;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -307,11 +306,7 @@ public class ActionMetadataHandler implements MetadataHandler {
       // should be single threaded and there should be no race condition.
       // The current design of ActionMetadataHandler makes this hard to enforce.
       Set<PathFragment> paths = null;
-      try {
-        paths = TreeArtifactValue.explodeDirectory(artifact);
-      } catch (TreeArtifactException e) {
-        throw new IllegalStateException(e);
-      }
+      paths = TreeArtifactValue.explodeDirectory(artifact);
       Set<TreeFileArtifact> diskFiles = ActionInputHelper.asTreeFileArtifacts(artifact, paths);
       if (!diskFiles.equals(registeredContents)) {
         // There might be more than one error here. We first look for missing output files.
@@ -322,13 +317,13 @@ public class ActionMetadataHandler implements MetadataHandler {
           // Currently it's hard to report this error without refactoring, since checkOutputs()
           // likes to substitute its own error messages upon catching IOException, and falls
           // through to unrecoverable error behavior on any other exception.
-          throw new IllegalStateException("Output file " + missingFiles.iterator().next()
+          throw new IOException("Output file " + missingFiles.iterator().next()
               + " was registered, but not present on disk");
         }
 
         Set<TreeFileArtifact> extraFiles = Sets.difference(diskFiles, registeredContents);
         // extraFiles cannot be empty
-        throw new IllegalStateException(
+        throw new IOException(
             "File " + extraFiles.iterator().next().getParentRelativePath()
             + ", present in TreeArtifact " + artifact + ", was not registered");
       }
@@ -381,11 +376,7 @@ public class ActionMetadataHandler implements MetadataHandler {
     }
 
     Set<PathFragment> paths = null;
-    try {
-      paths = TreeArtifactValue.explodeDirectory(artifact);
-    } catch (TreeArtifactException e) {
-      throw new IllegalStateException(e);
-    }
+    paths = TreeArtifactValue.explodeDirectory(artifact);
     // If you're reading tree artifacts from disk while outputDirectoryListings are being injected,
     // something has gone terribly wrong.
     Object previousDirectoryListing =
@@ -482,12 +473,6 @@ public class ActionMetadataHandler implements MetadataHandler {
   }
 
   @Override
-  public boolean artifactExists(Artifact artifact) {
-    Preconditions.checkState(!artifactOmitted(artifact), artifact);
-    return getMetadataMaybe(artifact) != null;
-  }
-
-  @Override
   public boolean isRegularFile(Artifact artifact) {
     // Currently this method is used only for genrule input directory checks. If we need to call
     // this on output artifacts too, this could be more efficient.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index 4b9263e0e6..9a94f3f30e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrappe
 import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.AutoProfiler.ElapsedTimeReceiver;
 import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker.DirtyResult;
-import com.google.devtools.build.lib.skyframe.TreeArtifactValue.TreeArtifactException;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -376,7 +375,7 @@ public class FilesystemValueChecker {
       Set<PathFragment> currentDirectoryValue = TreeArtifactValue.explodeDirectory(artifact);
       Set<PathFragment> valuePaths = value.getChildPaths();
       return !currentDirectoryValue.equals(valuePaths);
-    } catch (IOException | TreeArtifactException e) {
+    } catch (IOException e) {
       return true;
     }
   }
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 d69e6476fe..66ef70c03b 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
@@ -74,6 +74,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.protobuf.ByteString;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.HashSet;
@@ -717,8 +718,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
         handle = resourceManager.acquireResources(action, estimate);
       }
       boolean outputDumped = executeActionTask(action, context);
-      completeAction(action, context.getMetadataHandler(),
-          context.getFileOutErr(), outputDumped);
+      completeAction(action, context.getMetadataHandler(), context.getFileOutErr(), outputDumped);
     } finally {
       if (handle != null) {
         handle.close();
@@ -797,8 +797,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     return false;
   }
 
-  private void completeAction(Action action, MetadataHandler metadataHandler,
-      FileOutErr fileOutErr, boolean outputAlreadyDumped) throws ActionExecutionException {
+  private void completeAction(Action action, MetadataHandler metadataHandler, FileOutErr fileOutErr,
+      boolean outputAlreadyDumped) throws ActionExecutionException {
     try {
       Preconditions.checkState(action.inputsKnown(),
           "Action %s successfully executed, but inputs still not known", action);
@@ -806,7 +806,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
       profiler.startTask(ProfilerTask.ACTION_COMPLETE, action);
       try {
         if (!checkOutputs(action, metadataHandler)) {
-          reportError("not all outputs were created", null, action,
+          reportError("not all outputs were created or valid", null, action,
               outputAlreadyDumped ? null : fileOutErr);
         }
         // Prevent accidental stomping on files.
@@ -922,6 +922,19 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     }
   }
 
+  private void reportOutputTreeArtifactErrors(Action action, Artifact output, Reporter reporter,
+      IOException e) {
+    String errorMessage;
+    if (e instanceof FileNotFoundException) {
+      errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint());
+    } else {
+      errorMessage = String.format(
+          "Error while validating output TreeArtifact %s : %s", output, e.getMessage());
+    }
+
+    reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));
+  }
+
   /**
    * Validates that all action outputs were created or intentionally omitted.
    *
@@ -933,9 +946,18 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
       // artifactExists has the side effect of potentially adding the artifact to the cache,
       // therefore we only call it if we know the artifact is indeed not omitted to avoid any
       // unintended side effects.
-      if (!(metadataHandler.artifactOmitted(output) || metadataHandler.artifactExists(output))) {
-        reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink());
-        success = false;
+      if (!(metadataHandler.artifactOmitted(output))) {
+        try {
+          metadataHandler.getMetadata(output);
+        } catch (IOException e) {
+          success = false;
+          if (output.isTreeArtifact()) {
+            reportOutputTreeArtifactErrors(action, output, reporter, e);
+          } else {
+            // Are all exceptions caught due to missing files?
+            reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink());
+          }
+        }
       }
     }
     return success;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
index e37be07a5c..b523c92e37 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -180,29 +180,18 @@ class TreeArtifactValue implements SkyValue {
     }
   };
 
-  /**
-   * Exception used when the contents of a directory do not form a valid SetArtifact.
-   * (We cannot use IOException because ActionMetadataHandler, in some code paths,
-   * interprets IOExceptions as missing files.)
-   */
-  static class TreeArtifactException extends Exception {
-    TreeArtifactException(String message) {
-      super(message);
-    }
-  }
-
-  private static void explodeDirectory(Artifact rootArtifact,
+  private static void explodeDirectory(Artifact treeArtifact,
       PathFragment pathToExplode, ImmutableSet.Builder<PathFragment> valuesBuilder)
-      throws IOException, TreeArtifactException {
-    for (Path subpath : rootArtifact.getPath().getRelative(pathToExplode).getDirectoryEntries()) {
+      throws IOException {
+    for (Path subpath : treeArtifact.getPath().getRelative(pathToExplode).getDirectoryEntries()) {
       PathFragment canonicalSubpathFragment =
           pathToExplode.getChild(subpath.getBaseName()).normalize();
       if (subpath.isDirectory()) {
-        explodeDirectory(rootArtifact,
+        explodeDirectory(treeArtifact,
             pathToExplode.getChild(subpath.getBaseName()), valuesBuilder);
       } else if (subpath.isSymbolicLink()) {
-        throw new TreeArtifactException(
-            "A SetArtifact may not contain a symlink, found " + subpath);
+        throw new IOException(
+            "A TreeArtifact may not contain a symlink, found " + subpath);
       } else if (subpath.isFile()) {
         valuesBuilder.add(canonicalSubpathFragment);
       } else {
@@ -215,13 +204,12 @@ class TreeArtifactValue implements SkyValue {
   /**
    * Recursively get all child files in a directory
    * (excluding child directories themselves, but including all files in them).
-   * @throws IOException if one was thrown reading directory contents from disk.
-   * @throws TreeArtifactException if the on-disk directory is not a valid TreeArtifact.
+   * @throws IOException if there is any problem reading or validating outputs under the given
+   *     tree artifact.
    */
-  static Set<PathFragment> explodeDirectory(Artifact rootArtifact)
-      throws IOException, TreeArtifactException {
+  static Set<PathFragment> explodeDirectory(Artifact treeArtifact) throws IOException {
     ImmutableSet.Builder<PathFragment> explodedDirectory = ImmutableSet.builder();
-    explodeDirectory(rootArtifact, PathFragment.EMPTY_FRAGMENT, explodedDirectory);
+    explodeDirectory(treeArtifact, PathFragment.EMPTY_FRAGMENT, explodedDirectory);
     return explodedDirectory.build();
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
index 87d26bbc9f..a221496dab 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
@@ -98,11 +98,6 @@ public class PopulateTreeArtifactActionTest extends BuildViewTestCase {
     }
 
     @Override
-    public boolean artifactExists(Artifact artifact) {
-      throw new UnsupportedOperationException(artifact.prettyPrint());
-    }
-
-    @Override
     public boolean isRegularFile(Artifact artifact) {
       throw new UnsupportedOperationException(artifact.prettyPrint());
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ParallelBuilderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ParallelBuilderTest.java
index d06a544d1f..159c3c1a21 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ParallelBuilderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ParallelBuilderTest.java
@@ -51,12 +51,6 @@ import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -70,6 +64,10 @@ import java.util.concurrent.Callable;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Logger;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Test suite for ParallelBuilder.
@@ -311,7 +309,7 @@ public class ParallelBuilderTest extends TimestampBuilderTestCase {
       buildArtifacts(foo);
       fail("Expected to fail");
     } catch (BuildFailedException e) {
-      assertContainsEvent("not all outputs were created");
+      assertContainsEvent("not all outputs were created or valid");
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index b2359d92b4..d21c908dd3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import static com.google.common.base.Throwables.getRootCause;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.actions.ActionInputHelper.treeFileArtifact;
 import static org.junit.Assert.assertFalse;
@@ -22,9 +21,11 @@ import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.hash.Hashing;
@@ -47,6 +48,9 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.actions.util.TestAction;
 import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -88,7 +92,6 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
   TreeFileArtifact outTwoFileOne;
   TreeFileArtifact outTwoFileTwo;
   Button buttonTwo = new Button();
-
   @Before
   public void setUp() throws Exception {
     in = createSourceArtifact("input");
@@ -380,6 +383,18 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
 
   @Test
   public void testInvalidOutputRegistrations() throws Exception {
+    // Failure expected
+    StoredEventHandler storingEventHandler = new StoredEventHandler();
+    reporter.removeHandler(failFastHandler);
+    reporter.addHandler(storingEventHandler);
+
+    Predicate<Event> isErrorEvent = new Predicate<Event>() {
+        @Override
+        public boolean apply(Event event) {
+          return event.getKind().equals(EventKind.ERROR);
+        }
+    };
+
     TreeArtifactTestAction failureOne = new TreeArtifactTestAction(
         Runnables.doNothing(), outOneFileOne, outOneFileTwo) {
       @Override
@@ -400,8 +415,13 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
     try {
       buildArtifact(outOne);
       fail(); // Should have thrown
-    } catch (Exception e) {
-      assertThat(getRootCause(e).getMessage()).contains("not present on disk");
+    } catch (BuildFailedException e) {
+      //not all outputs were created
+      List<Event> errors = ImmutableList.copyOf(
+          Iterables.filter(storingEventHandler.getEvents(), isErrorEvent));
+      assertThat(errors).hasSize(2);
+      assertThat(errors.get(0).getMessage()).contains("not present on disk");
+      assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
     }
 
     TreeArtifactTestAction failureTwo = new TreeArtifactTestAction(
@@ -423,11 +443,16 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
     };
 
     registerAction(failureTwo);
+    storingEventHandler.clear();
     try {
       buildArtifact(outTwo);
       fail(); // Should have thrown
-    } catch (Exception e) {
-      assertThat(getRootCause(e).getMessage()).contains("not present on disk");
+    } catch (BuildFailedException e) {
+      List<Event> errors = ImmutableList.copyOf(
+          Iterables.filter(storingEventHandler.getEvents(), isErrorEvent));
+      assertThat(errors).hasSize(2);
+      assertThat(errors.get(0).getMessage()).contains("not present on disk");
+      assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
     }
   }
 
@@ -590,7 +615,7 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
       buildArtifact(artifact2);
       fail("Expected BuildFailedException");
     } catch (BuildFailedException e) {
-      assertThat(e.getMessage()).contains("not all outputs were created");
+      assertThat(e.getMessage()).contains("not all outputs were created or valid");
     }
   }