Handle exceptions encountered resolving packages during the execution phase

Currently we may do lookups of not-already-cached packages during the
execution phase for actions that discover inputs. Exceptions encountered
during this would go unhandled and result in a crash. Here we introduce
PackageRootResolutionException which wraps these exceptions and triggers
an ActionExecutionException which is cleanly handled in the exec phase.

As part of this change SkyframeActionExecutor#getArtifactRoots(...) will
fail properly on errors getting package roots.

--
MOS_MIGRATED_REVID=95578891

15 files changed, 133 insertions, 48 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 ee42bcaa82..5fa354e76c 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
@@ -112,7 +112,8 @@ public abstract class AbstractAction implements Action {
   @Nullable
   @Override
   public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver,
-      PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
+      PackageRootResolver resolver, Collection<PathFragment> inputPaths)
+          throws PackageRootResolutionException {
     throw new IllegalStateException(
         "Method must be overridden for actions that may have unknown inputs.");
   }
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 3581ae0a31..af0ed613ac 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
@@ -120,11 +120,12 @@ public interface Action extends ActionMetadata, Describable {
    * @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.
+   * @throws PackageRootResolutionException on failure to determine package roots of inputPaths
    */
   @Nullable
   Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver, PackageRootResolver resolver,
-      Collection<PathFragment> inputPaths);
+      Collection<PathFragment> inputPaths) throws PackageRootResolutionException;
 
   /**
    * Informs the action that its inputs are {@code inputs}, and that its inputs are now known. Can
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 40eabeb9de..c9c1ecbec3 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
@@ -241,7 +241,8 @@ public class ActionCacheChecker {
   }
 
   @Nullable
-  public Iterable<Artifact> getCachedInputs(Action action, PackageRootResolver resolver) {
+  public Iterable<Artifact> getCachedInputs(Action action, PackageRootResolver resolver)
+      throws PackageRootResolutionException {
     ActionCache.Entry entry = getCacheEntry(action);
     if (entry == null || entry.isCorrupted()) {
       return ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index f9c0e75fe7..db94639861 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -255,7 +255,8 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
 
   @Override
   public synchronized Map<PathFragment, Artifact> resolveSourceArtifacts(
-      Iterable<PathFragment> execPaths, PackageRootResolver resolver) {
+      Iterable<PathFragment> execPaths, PackageRootResolver resolver)
+          throws PackageRootResolutionException {
     Map<PathFragment, Artifact> result = new HashMap<>();
     ArrayList<PathFragment> unresolvedPaths = new ArrayList<>();
 
@@ -346,15 +347,17 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    * cannot be created. Unfortunately, we currently need this in some cases.
    *
    * @param execPath the exec path of the artifact
+   * @throws PackageRootResolutionException on failure to determine the package roots of
+   *    {@code execPath}
    */
-  public Artifact deserializeArtifact(PathFragment execPath, PackageRootResolver resolver) {
+  public Artifact deserializeArtifact(PathFragment execPath, PackageRootResolver resolver)
+      throws PackageRootResolutionException {
     Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
     Path path = execRoot.getRelative(execPath);
     Root root = findDerivedRoot(path);
 
-    Artifact result;
     if (root != null) {
-      result = getDerivedArtifact(path.relativeTo(root.getPath()), root,
+      Artifact result = getDerivedArtifact(path.relativeTo(root.getPath()), root,
           Artifact.DESERIALIZED_MARKER_OWNER);
       Artifact oldResult = deserializedArtifacts.putIfAbsent(execPath, result);
       if (oldResult != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
index baebfec0f5..295dac22a6 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
@@ -72,8 +72,9 @@ public interface ArtifactResolver {
    *         existing or new source Artifacts for the given execPaths. The artifact is null if the
    *         root cannot be determined and the artifact did not exist before. Return null if any
    *         dependencies are missing.
+   * @throws PackageRootResolutionException failure to determine package roots of {@code execPaths}
    */
   @Nullable
   Map<PathFragment, Artifact> resolveSourceArtifacts(Iterable<PathFragment> execPaths,
-      PackageRootResolver resolver);
+      PackageRootResolver resolver) throws PackageRootResolutionException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
new file mode 100644
index 0000000000..ff16aaa789
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
@@ -0,0 +1,26 @@
+// Copyright 2015 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.actions;
+
+/**
+ * Exception signaling an error occurred determining package roots. See
+ * {@link PackageRootResolver#findPackageRoots(Iterable)} for further details.
+ */
+public class PackageRootResolutionException extends Exception {
+
+  public PackageRootResolutionException(String msg, Throwable cause) {
+    super(msg, cause);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
index 90af1361c7..5a06012291 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
@@ -26,9 +26,16 @@ import javax.annotation.Nullable;
 public interface PackageRootResolver {
 
   /**
-   * Returns mapping from execPath to Root. Some roots can equal null if the corresponding
-   * package can't be found. Returns null if for some reason we can't evaluate it.
+   * Returns mapping from execPath to Root. Root will be null if the path has no containing
+   * package.
+   *
+   * @param execPaths the paths to find {@link Root}s for
+   * @return mappings from {@code execPath} to {@link Root}, or null if for some reason we
+   *    cannot determine the result at this time (such as when used within a SkyFunction)
+   * @throws PackageRootResolutionException if unable to determine package roots or lack thereof,
+   *    typically caused by exceptions encountered while attempting to locate BUILD files
    */
   @Nullable
-  Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths);
+  Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+      throws PackageRootResolutionException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
index 50aeb9ae2b..db24b5ed1b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -34,7 +35,8 @@ public final class SkyframePackageRootResolver implements PackageRootResolver {
   }
 
   @Override
-  public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths) {
+  public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+      throws PackageRootResolutionException {
     return executor.getArtifactRoots(execPaths);
   }
 }
\ No newline at end of file
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 ded0cd39ec..b9db265ba3 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
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.MiddlemanExpander;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.extra.CppCompileInfo;
@@ -105,12 +106,12 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
   private static final int VALIDATION_DEBUG = 0;  // 0==none, 1==warns/errors, 2==all
   private static final boolean VALIDATION_DEBUG_WARN = VALIDATION_DEBUG >= 1;
-  
+
   /**
    * A string constant for the c compilation action.
    */
   public static final String C_COMPILE = "c-compile";
-  
+
   /**
    * A string constant for the c++ compilation action.
    */
@@ -120,18 +121,18 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
    * A string constant for the c++ header parsing.
    */
   public static final String CPP_HEADER_PARSING = "c++-header-parsing";
-  
+
   /**
    * A string constant for the c++ header preprocessing.
    */
   public static final String CPP_HEADER_PREPROCESSING = "c++-header-preprocessing";
-  
+
   /**
    * A string constant for the c++ module compilation action.
    * Note: currently we don't support C module compilation.
    */
   public static final String CPP_MODULE_COMPILE = "c++-module-compile";
-  
+
   /**
    * A string constant for the preprocessing assembler action.
    */
@@ -199,7 +200,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
    */
   protected CppCompileAction(ActionOwner owner,
       // TODO(bazel-team): Eventually we will remove 'features'; all functionality in 'features'
-      // will be provided by 'featureConfiguration'. 
+      // will be provided by 'featureConfiguration'.
       ImmutableList<String> features,
       FeatureConfiguration featureConfiguration,
       Artifact sourceFile,
@@ -521,7 +522,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     }
     return cmdlineIncludes.build();
   }
-  
+
   @Override
   public Artifact getMainIncludeScannerSource() {
     return CppFileTypes.CPP_MODULE_MAP.matches(getSourceFile().getPath())
@@ -914,7 +915,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   @Override
   public Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver, PackageRootResolver resolver,
-      Collection<PathFragment> inputPaths) {
+      Collection<PathFragment> inputPaths) throws PackageRootResolutionException {
     // Note that this method may trigger a violation of the desirable invariant that getInputs()
     // is a superset of getMandatoryInputs(). See bug about an "action not in canonical form"
     // error message and the integration test test_crosstool_change_and_failure().
@@ -931,7 +932,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       }
     }
 
-    Map<PathFragment, Artifact> resolvedArtifacts = 
+    Map<PathFragment, Artifact> resolvedArtifacts =
         artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
     if (resolvedArtifacts == null) {
       // We are missing some dependencies. We need to rerun this update later.
@@ -1175,7 +1176,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
     // The value of the BUILD_FDO_TYPE macro to be defined on command line
     @Nullable private final String fdoBuildStamp;
-    
+
     public CppCompileCommandLine(Artifact sourceFile, DotdFile dotdFile, CppModuleMap cppModuleMap,
         ImmutableList<String> copts, Predicate<String> coptsFilter,
         ImmutableList<String> pluginOpts, boolean isInstrumented,
@@ -1212,7 +1213,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
       return commandLine;
     }
-    
+
     private String getActionName() {
       PathFragment sourcePath = sourceFile.getExecPath();
       if (CppFileTypes.CPP_MODULE_MAP.matches(sourcePath)) {
@@ -1243,7 +1244,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     public List<String> getCompilerOptions() {
       List<String> options = new ArrayList<>();
       CppConfiguration toolchain = cppConfiguration;
-      
+
       // pluginOpts has to be added before defaultCopts because -fplugin must precede -plugin-arg.
       options.addAll(pluginOpts);
       addFilteredOptions(options, toolchain.getCompilerOptions(features));
@@ -1289,7 +1290,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       }
       if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
         buildVariables.addSequenceVariable("module_files", getHeaderModulePaths());
-      }      
+      }
       if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
         buildVariables.addSequenceVariable("include_paths",
             getSafePathStrings(context.getIncludeDirs()));
@@ -1389,7 +1390,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         // from targets that are built in both modes.
         if (usePic == filename.endsWith(".pic.pcm")) {
           result.add(artifact.getExecPathString());
-        }          
+        }
       }
       return result;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 78570210b0..15128fae8e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -28,6 +28,7 @@ import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
@@ -1795,8 +1796,14 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     // TODO(bazel-team): Remove the "relative" guard. sysroot should always be relative, and this
     // should be enforced in the creation of CppConfiguration.
     if (getSysroot() != null && !getSysroot().isAbsolute()) {
-      Root sysrootRoot = Iterables.getOnlyElement(
+      Root sysrootRoot;
+      try {
+        sysrootRoot = Iterables.getOnlyElement(
           resolver.findPackageRoots(ImmutableList.of(getSysroot())).entrySet()).getValue();
+      } catch (PackageRootResolutionException prre) {
+        throw new ViewCreationFailedException("Failed to determine sysroot", prre);
+      }
+
       PathFragment sysrootExecPath = sysroot.getRelative(BUILT_IN_INCLUDE_PATH_FRAGMENT);
       if (sysrootRoot.getPath().getRelative(sysrootExecPath).exists()) {
         builtInIncludeFile = Preconditions.checkNotNull(
@@ -1808,7 +1815,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       getFdoSupport().prepareToBuild(execRoot, genfilesPath, artifactFactory, resolver);
     } catch (ZipException e) {
       throw new ViewCreationFailedException("Error reading provided FDO zip file", e);
-    } catch (FdoException | IOException e) {
+    } catch (FdoException | IOException | PackageRootResolutionException e) {
       throw new ViewCreationFailedException("Error while initializing FDO support", e);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index dfabbed280..c10eea9edc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
@@ -255,11 +256,12 @@ public class FdoSupport implements Serializable {
    * FDO gcda zip file into a clean working directory under execRoot.
    *
    * @throws FdoException if the FDO ZIP contains a file of unknown type
+   * @throws PackageRootResolutionException
    */
   @ThreadHostile // must be called before starting the build
   public void prepareToBuild(Path execRoot, PathFragment genfilesPath,
       ArtifactFactory artifactDeserializer, PackageRootResolver resolver)
-      throws IOException, FdoException {
+      throws IOException, FdoException, PackageRootResolutionException {
     // The execRoot != null case is only there for testing. We cannot provide a real ZIP file in
     // tests because ZipFileSystem does not work with a ZIP on an in-memory file system.
     // IMPORTANT: Keep in sync with #declareSkyframeDependencies to avoid incrementality issues.
@@ -310,11 +312,13 @@ public class FdoSupport implements Serializable {
    *
    * @throws IOException if any of the I/O operations failed
    * @throws FdoException if the FDO ZIP contains a file of unknown type
+   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
   private void extractFdoZip(ArtifactFactory artifactFactory, Path sourceDir,
       Path targetDir, ImmutableSet.Builder<PathFragment> gcdaFilesBuilder,
       ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver) throws IOException, FdoException {
+      PackageRootResolver resolver)
+          throws IOException, FdoException, PackageRootResolutionException {
     for (Path sourceFile : sourceDir.getDirectoryEntries()) {
       Path targetFile = targetDir.getRelative(sourceFile.getBaseName());
       if (sourceFile.isDirectory()) {
@@ -340,10 +344,12 @@ public class FdoSupport implements Serializable {
    * Reads a .gcda.imports file and stores the imports information.
    *
    * @throws FdoException if an auxiliary LIPO input was not found
+   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
   private void readCoverageImports(ArtifactFactory artifactFactory, Path importsFile,
       ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver) throws IOException, FdoException {
+      PackageRootResolver resolver)
+          throws IOException, FdoException, PackageRootResolutionException {
     PathFragment key = importsFile.asFragment().relativeTo(ZIP_ROOT);
     String baseName = key.getBaseName();
     String ext = Iterables.getOnlyElement(CppFileTypes.COVERAGE_DATA_IMPORTS.getExtensions());
@@ -370,11 +376,12 @@ public class FdoSupport implements Serializable {
 
   /**
    * Reads a .afdo.imports file and stores the imports information.
+   * @throws PackageRootResolutionException
    */
   private ImmutableMultimap<PathFragment, Artifact> readAutoFdoImports(
       ArtifactFactory artifactFactory, Path importsFile, PathFragment genFilePath,
       PackageRootResolver resolver)
-          throws IOException, FdoException {
+          throws IOException, FdoException, PackageRootResolutionException {
     ImmutableMultimap.Builder<PathFragment, Artifact> importBuilder = ImmutableMultimap.builder();
     for (String line : FileSystemUtils.iterateLinesAsLatin1(importsFile)) {
       if (!line.isEmpty()) {
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 78b76ca0af..9aa9eb910b 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
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.ArtifactResolver;
 import com.google.devtools.build.lib.actions.DelegateSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -155,12 +156,12 @@ public final class ExtraAction extends SpawnAction {
   @Nullable
   @Override
   public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver,
-      PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
+      PackageRootResolver resolver, Collection<PathFragment> inputPaths)
+          throws PackageRootResolutionException {
     // We update the inputs directly from the shadowed action.
     Set<PathFragment> extraActionPathFragments =
         ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs));
-    return shadowedAction.resolveInputsFromCache(artifactResolver,
-        resolver,
+    return shadowedAction.resolveInputsFromCache(artifactResolver, resolver,
         Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments)));
   }
 
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 fec72255c2..9a62d39787 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
@@ -28,10 +28,12 @@ import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionExcep
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 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.packages.PackageIdentifier;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.Pair;
@@ -176,9 +178,11 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
    * the action cache's view of this action contains additional inputs, it will request metadata for
    * them, so we consider those inputs as dependencies of this action as well. Returns null if some
    * dependencies were missing and this ActionExecutionFunction needs to restart.
+   * @throws ActionExecutionFunctionException
    */
   @Nullable
-  private AllInputs collectInputs(Action action, Environment env) {
+  private AllInputs collectInputs(Action action, Environment env)
+      throws ActionExecutionFunctionException {
     Iterable<Artifact> allKnownInputs = Iterables.concat(
         action.getInputs(), action.getRunfilesSupplier().getArtifacts());
     if (action.inputsKnown()) {
@@ -187,8 +191,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
     Preconditions.checkState(action.discoversInputs(), action);
     PackageRootResolverWithEnvironment resolver = new PackageRootResolverWithEnvironment(env);
-    Iterable<Artifact> actionCacheInputs =
-        skyframeActionExecutor.getActionCachedInputs(action, resolver);
+    Iterable<Artifact> actionCacheInputs;
+    try {
+      actionCacheInputs = skyframeActionExecutor.getActionCachedInputs(action, resolver);
+    } catch (PackageRootResolutionException rre) {
+      throw new ActionExecutionFunctionException(
+          new ActionExecutionException("Failed to get cached inputs", rre, action, true));
+    }
     if (actionCacheInputs == null) {
       Preconditions.checkState(env.valuesMissing(), action);
       return null;
@@ -236,29 +245,38 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     }
 
     @Override
-    public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths) {
+    public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+        throws PackageRootResolutionException {
       Preconditions.checkState(keysRequested.isEmpty(),
           "resolver should only be called once: %s %s", keysRequested, execPaths);
-      Map<PathFragment, SkyKey> depKeys = new HashMap<>();
       // Create SkyKeys list based on execPaths.
+      Map<PathFragment, SkyKey> depKeys = new HashMap<>();
       for (PathFragment path : execPaths) {
         SkyKey depKey =
             ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(path));
         depKeys.put(path, depKey);
         keysRequested.add(depKey);
       }
-      Map<SkyKey, SkyValue> values = env.getValues(depKeys.values());
+
+      Map<SkyKey,
+          ValueOrException2<NoSuchPackageException, InconsistentFilesystemException>> values =
+              env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class,
+                  InconsistentFilesystemException.class);
       if (env.valuesMissing()) {
         // Some values are not computed yet.
         return null;
       }
+
       Map<PathFragment, Root> result = new HashMap<>();
       for (PathFragment path : execPaths) {
-        // TODO(bazel-team): Add check for errors here, when loading phase will be removed.
-        // For now all possible errors that ContainingPackageLookupFunction can generate
-        // are caught in previous phases.
-        ContainingPackageLookupValue value =
-            (ContainingPackageLookupValue) values.get(depKeys.get(path));
+        ContainingPackageLookupValue value;
+        try {
+          value = (ContainingPackageLookupValue) values.get(depKeys.get(path)).get();
+        } catch (NoSuchPackageException | InconsistentFilesystemException e) {
+          throw new PackageRootResolutionException("Could not determine containing package for "
+              + path, e);
+        }
+
         if (value.hasContainingPackage()) {
           // We have found corresponding root for current execPath.
           result.put(path, Root.asSourceRoot(value.getContainingPackageRoot()));
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 ed01fa979a..f16690eb55 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
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.actions.MapBasedActionGraph;
 import com.google.devtools.build.lib.actions.MutableActionGraph;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
@@ -508,7 +509,8 @@ public final class SkyframeActionExecutor {
   }
 
   @Nullable
-  Iterable<Artifact> getActionCachedInputs(Action action, PackageRootResolver resolver) {
+  Iterable<Artifact> getActionCachedInputs(Action action, PackageRootResolver resolver)
+      throws PackageRootResolutionException {
     return actionCacheChecker.getCachedInputs(action, resolver);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 575320f0e6..a4fde59889 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.Aspect;
@@ -601,7 +602,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   // TODO(bazel-team): Make this take a PackageIdentifier.
-  public Map<PathFragment, Root> getArtifactRoots(Iterable<PathFragment> execPaths) {
+  public Map<PathFragment, Root> getArtifactRoots(Iterable<PathFragment> execPaths)
+      throws PackageRootResolutionException {
     final List<SkyKey> packageKeys = new ArrayList<>();
     for (PathFragment execPath : execPaths) {
       Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
@@ -624,6 +626,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       throw new IllegalStateException(e);  // Should never happen.
     }
 
+    if (result.hasError()) {
+      throw new PackageRootResolutionException("Exception encountered determining package roots",
+          result.getError().getException());
+    }
+
     Map<PathFragment, Root> roots = new HashMap<>();
     for (PathFragment execPath : execPaths) {
       ContainingPackageLookupValue value = result.get(ContainingPackageLookupValue.key(