Properly handle exceptions that RecursivePkgFunction can encounter, instead of silently swallowing them.

The old behavior was simply incorrect on --keep_going builds because it meant any directory with an uncaught error caused all of its ancestors to not have any values. It wasn't noticed because SkyframeTargetPatternEvaluator was overly permissive in the errors it expects.

We also use a singleton for the empty RecursivePkgValue which might have a negligible (beneficial) memory impact.

--
MOS_MIGRATED_REVID=95037551

5 files changed, 53 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index c3faec8e40..68cb59a813 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -354,6 +354,10 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
           }
           result.put(pattern, targetsBuilder.build());
         } else {
+          // Because the graph was always initialized via a keep_going build, we know that the
+          // exception stored here must be a TargetParsingException. Thus the comment in
+          // SkyframeTargetPatternEvaluator#parseTargetPatternKeys describing the situation in which
+          // the exception acceptance must be looser does not apply here.
           targetParsingException =
               (TargetParsingException)
                   Preconditions.checkNotNull(graph.getException(patternKey), pattern);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 3aeda39cea..938c8d4113 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -93,8 +93,11 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
     if (lookup == null) {
       // Typically a null value from Environment.getValue(k) means that either the key k is missing
       // a dependency or an exception was thrown during evaluation of k. Here, if this getValue
-      // call returns null, it can only mean a missing dependency, because
+      // call returns null in a keep_going build, it can only mean a missing dependency, because
       // RecursivePkgFunction#compute never throws.
+      // In a nokeep_going build, a lower-level exception that RecursivePkgFunction ignored may
+      // bubble up to here, but we ignore it and depend on the top-level caller to be flexible in
+      // the exception types it can accept.
       throw new MissingDepException();
     }
     // TODO(bazel-team): Make RecursivePkgValue return NestedSet<PathFragment> so this transform is
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
index b814319840..30ad2bbc02 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
@@ -18,6 +18,7 @@ import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.PackageIdentifier;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -31,6 +32,7 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.io.IOException;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -57,25 +59,36 @@ public class RecursivePkgFunction implements SkyFunction {
     Set<PathFragment> excludedPaths = recursivePkgKey.getExcludedPaths();
 
     SkyKey fileKey = FileValue.key(rootedPath);
-    FileValue fileValue = (FileValue) env.getValue(fileKey);
+    FileValue fileValue = null;
+    try {
+      fileValue = (FileValue) env.getValueOrThrow(fileKey, InconsistentFilesystemException.class,
+          FileSymlinkCycleException.class, IOException.class);
+    } catch (InconsistentFilesystemException | FileSymlinkCycleException  | IOException e) {
+      return reportErrorAndReturn(e, rootRelativePath, env.getListener());
+    }
     if (fileValue == null) {
       return null;
     }
 
     if (!fileValue.isDirectory()) {
-      return new RecursivePkgValue(NestedSetBuilder.<String>emptySet(ORDER));
+      return RecursivePkgValue.EMPTY;
     }
 
     if (fileValue.isSymlink()) {
       // We do not follow directory symlinks when we look recursively for packages. It also
       // prevents symlink loops.
-      return new RecursivePkgValue(NestedSetBuilder.<String>emptySet(ORDER));
+      return RecursivePkgValue.EMPTY;
     }
 
     PackageIdentifier packageId = PackageIdentifier.createInDefaultRepo(
         rootRelativePath.getPathString());
-    PackageLookupValue pkgLookupValue =
-        (PackageLookupValue) env.getValue(PackageLookupValue.key(packageId));
+    PackageLookupValue pkgLookupValue;
+    try {
+      pkgLookupValue = (PackageLookupValue) env.getValueOrThrow(PackageLookupValue.key(packageId),
+          NoSuchPackageException.class, InconsistentFilesystemException.class);
+    } catch (NoSuchPackageException | InconsistentFilesystemException e) {
+      return reportErrorAndReturn(e, rootRelativePath, env.getListener());
+    }
     if (pkgLookupValue == null) {
       return null;
     }
@@ -171,7 +184,15 @@ public class RecursivePkgFunction implements SkyFunction {
         packages.addTransitive(((RecursivePkgValue) childValue).getPackages());
       }
     }
-    return new RecursivePkgValue(packages.build());
+    return RecursivePkgValue.create(packages);
+  }
+
+  // Ignore all errors in traversal and just say there are no packages here.
+  private static RecursivePkgValue reportErrorAndReturn(Exception e, PathFragment rootRelativePath,
+      EventHandler handler) {
+    handler.handle(Event.warn("Finding packages under " + rootRelativePath + " failed, skipping: "
+        + e.getMessage()));
+    return RecursivePkgValue.EMPTY;
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
index 56ed0e504a..721ebc8f0c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -32,14 +34,23 @@ import java.util.Objects;
  */
 @Immutable
 @ThreadSafe
-public class RecursivePkgValue implements SkyValue {
+class RecursivePkgValue implements SkyValue {
+  static final RecursivePkgValue EMPTY =
+      new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER));
 
   private final NestedSet<String> packages;
 
-  public RecursivePkgValue(NestedSet<String> packages) {
+  private RecursivePkgValue(NestedSet<String> packages) {
     this.packages = packages;
   }
 
+  static RecursivePkgValue create(NestedSetBuilder<String> packages) {
+    if (packages.isEmpty()) {
+      return EMPTY;
+    }
+    return new RecursivePkgValue(packages.build());
+  }
+
   /**
    * Create a transitive package lookup request.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index d166126038..e504f88001 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -180,6 +180,11 @@ final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator {
           continue;
         }
         if (error.getException() != null) {
+          // This exception may not be a TargetParsingException because in a nokeep_going build, the
+          // target pattern parser may swallow a NoSuchPackageException but the framework will
+          // bubble it up anyway.
+          Preconditions.checkArgument(!keepGoing
+              || error.getException() instanceof TargetParsingException, error);
           errorMessage = error.getException().getMessage();
         } else if (!Iterables.isEmpty(error.getCycleInfo())) {
           errorMessage = "cycles detected during target parsing";