A bunch of unrelated cleanups:

-Have SkylarkImportLookupFunction include causes in the SkyFunctionExceptions it throws.
-Better transitive skyframe error declarations in ASTFileLookupFunction.
-Have ErrorInfo differentiate between direct and transitive transience.
-Introduce ErrorInfoManager and have ParallelEvaluator/ParallelEvaluatorContext use it.

RELNOTES: None
PiperOrigin-RevId: 159163186

19 files changed, 207 insertions, 75 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index 30a9c08b16..f6385f2bb4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -85,7 +85,10 @@ public class ASTFileLookupFunction implements SkyFunction {
     try {
       fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class,
           FileSymlinkException.class, InconsistentFilesystemException.class);
-    } catch (IOException | FileSymlinkException e) {
+    } catch (IOException e) {
+      throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
+          Transience.PERSISTENT);
+    } catch (FileSymlinkException e) {
       throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
           Transience.PERSISTENT);
     } catch (InconsistentFilesystemException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
index af4f039769..cf6904a0b6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
@@ -13,9 +13,20 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
+import java.io.IOException;
+
 /** Indicates some sort of IO error while dealing with a Skylark extension. */
 public class ErrorReadingSkylarkExtensionException extends Exception {
-  public ErrorReadingSkylarkExtensionException(Exception e) {
+  public ErrorReadingSkylarkExtensionException(BuildFileNotFoundException e) {
+    super(e.getMessage(), e);
+  }
+
+  public ErrorReadingSkylarkExtensionException(IOException e) {
+    super(e.getMessage(), e);
+  }
+
+  public ErrorReadingSkylarkExtensionException(FileSymlinkException e) {
     super(e.getMessage(), e);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index b5cb5b827e..15e21e3bb9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Throwables;
 import com.google.common.cache.Cache;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
@@ -633,6 +634,16 @@ public class PackageFunction implements SkyFunction {
     return buildFileValue;
   }
 
+  private static BuildFileContainsErrorsException propagateSkylarkImportFailedException(
+      PackageIdentifier packageId, SkylarkImportFailedException e)
+          throws BuildFileContainsErrorsException {
+    Throwable rootCause = Throwables.getRootCause(e);
+    throw (rootCause instanceof IOException)
+        ? new BuildFileContainsErrorsException(
+            packageId, e.getMessage(), (IOException) rootCause)
+        : new BuildFileContainsErrorsException(packageId, e.getMessage());
+  }
+
   /**
    * Fetch the skylark loads for this BUILD file. If any of them haven't been computed yet,
    * returns null.
@@ -667,7 +678,7 @@ public class PackageFunction implements SkyFunction {
         return null;
       }
     } catch (SkylarkImportFailedException e) {
-      throw new BuildFileContainsErrorsException(packageId, e.getMessage());
+      throw propagateSkylarkImportFailedException(packageId, e);
     }
 
     // Look up and load the imports.
@@ -721,7 +732,7 @@ public class PackageFunction implements SkyFunction {
 
       }
     } catch (SkylarkImportFailedException e) {
-      throw new BuildFileContainsErrorsException(packageId, e.getMessage());
+      throw propagateSkylarkImportFailedException(packageId, e);
     } catch (InconsistentFilesystemException e) {
       throw new NoSuchPackageException(packageId, e.getMessage(), e);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 7d3a196124..92daf4428a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -133,7 +133,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       astLookupValue = (ASTFileLookupValue) env.getValueOrThrow(astLookupKey,
           ErrorReadingSkylarkExtensionException.class, InconsistentFilesystemException.class);
     } catch (ErrorReadingSkylarkExtensionException e) {
-      throw SkylarkImportFailedException.errorReadingFile(filePath, e.getMessage());
+      throw SkylarkImportFailedException.errorReadingFile(filePath, e);
     }
     if (astLookupValue == null) {
       return null;
@@ -441,16 +441,20 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       super(errorMessage);
     }
 
+    private SkylarkImportFailedException(String errorMessage, Exception cause) {
+      super(errorMessage, cause);
+    }
+
     private SkylarkImportFailedException(InconsistentFilesystemException e) {
-      super(e.getMessage());
+      this(e.getMessage(), e);
     }
 
     private SkylarkImportFailedException(BuildFileNotFoundException e) {
-      super(e.getMessage());
+      this(e.getMessage(), e);
     }
 
     private SkylarkImportFailedException(LabelSyntaxException e) {
-      super(e.getMessage());
+      this(e.getMessage(), e);
     }
 
     static SkylarkImportFailedException errors(PathFragment file) {
@@ -458,9 +462,14 @@ public class SkylarkImportLookupFunction implements SkyFunction {
           String.format("Extension file '%s' has errors", file));
     }
 
-    static SkylarkImportFailedException errorReadingFile(PathFragment file, String error) {
+    static SkylarkImportFailedException errorReadingFile(
+        PathFragment file, ErrorReadingSkylarkExtensionException cause) {
       return new SkylarkImportFailedException(
-          String.format("Encountered error while reading extension file '%s': %s", file, error));
+          String.format(
+              "Encountered error while reading extension file '%s': %s",
+              file,
+              cause.getMessage()),
+          cause);
     }
 
     static SkylarkImportFailedException noFile(String reason) {
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
index 418a4cfe1d..bd90d8afa0 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -42,6 +42,7 @@ public class ErrorInfo {
         Preconditions.checkNotNull(rootCauseException, "Cause null %s", rootCauseException),
         rootCauseSkyKey,
         /*cycles=*/ ImmutableList.<CycleInfo>of(),
+        skyFunctionException.isTransient(),
         isTransitivelyTransient || skyFunctionException.isTransient(),
         skyFunctionException.isCatastrophic());
   }
@@ -53,7 +54,8 @@ public class ErrorInfo {
         /*exception=*/ null,
         /*rootCauseOfException=*/ null,
         ImmutableList.of(cycleInfo),
-        /*isTransient=*/ false,
+        /*isDirectlyTransient=*/ false,
+        /*isTransitivelyTransient=*/ false,
         /*isCatastrophic=*/ false);
   }
 
@@ -66,7 +68,7 @@ public class ErrorInfo {
     ImmutableList.Builder<CycleInfo> cycleBuilder = ImmutableList.builder();
     Exception firstException = null;
     SkyKey firstChildKey = null;
-    boolean isTransient = false;
+    boolean isTransitivelyTransient = false;
     boolean isCatastrophic = false;
     for (ErrorInfo child : childErrors) {
       if (firstException == null) {
@@ -76,7 +78,7 @@ public class ErrorInfo {
       }
       rootCausesBuilder.addTransitive(child.rootCauses);
       cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles));
-      isTransient |= child.isTransient();
+      isTransitivelyTransient |= child.isTransitivelyTransient();
       isCatastrophic |= child.isCatastrophic();
     }
 
@@ -85,7 +87,8 @@ public class ErrorInfo {
         firstException,
         firstChildKey,
         cycleBuilder.build(),
-        isTransient,
+        /*isDirectlyTransient=*/ false,
+        isTransitivelyTransient,
         isCatastrophic);
   }
 
@@ -96,11 +99,17 @@ public class ErrorInfo {
 
   private final ImmutableList<CycleInfo> cycles;
 
-  private final boolean isTransient;
+  private final boolean isDirectlyTransient;
+  private final boolean isTransitivelyTransient;
   private final boolean isCatastrophic;
 
-  public ErrorInfo(NestedSet<SkyKey> rootCauses, @Nullable Exception exception,
-      SkyKey rootCauseOfException, ImmutableList<CycleInfo> cycles, boolean isTransient,
+  public ErrorInfo(
+      NestedSet<SkyKey> rootCauses,
+      @Nullable Exception exception,
+      SkyKey rootCauseOfException,
+      ImmutableList<CycleInfo> cycles,
+      boolean isDirectlyTransient,
+      boolean isTransitivelyTransient,
       boolean isCatostrophic) {
     Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles),
         "At least one of exception and cycles must be non-null/empty, respectively");
@@ -112,7 +121,8 @@ public class ErrorInfo {
     this.exception = exception;
     this.rootCauseOfException = rootCauseOfException;
     this.cycles = cycles;
-    this.isTransient = isTransient;
+    this.isDirectlyTransient = isDirectlyTransient;
+    this.isTransitivelyTransient = isTransitivelyTransient;
     this.isCatastrophic = isCatostrophic;
   }
 
@@ -162,11 +172,19 @@ public class ErrorInfo {
   }
 
   /**
+   * Returns true iff the error is directly transient, i.e. if there was a transient error
+   * encountered during the computation itself.
+   */
+  public boolean isDirectlyTransient() {
+    return isDirectlyTransient;
+  }
+
+  /**
    * Returns true iff the error is transitively transient, i.e. if retrying the same computation
    * could lead to a different result.
    */
-  public boolean isTransient() {
-    return isTransient;
+  public boolean isTransitivelyTransient() {
+    return isTransitivelyTransient;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfoManager.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfoManager.java
new file mode 100644
index 0000000000..01ca34b232
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfoManager.java
@@ -0,0 +1,65 @@
+// Copyright 2017 The Bazel Authors. 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.skyframe;
+
+import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException;
+import java.util.Collection;
+import javax.annotation.Nullable;
+
+/** Used by {@link ParallelEvaluator} to produce and consume {@link ErrorInfo} instances. */
+public interface ErrorInfoManager {
+  ErrorInfo fromException(
+      SkyKey key,
+      ReifiedSkyFunctionException skyFunctionException,
+      boolean isTransitivelyTransient);
+
+  /**
+   * Returns the {@link ErrorInfo} to use when there isn't currently one because
+   * {@link SkyFunction#compute} didn't throw a {@link SkyFunctionException} and there was no cycle.
+   */
+  @Nullable
+  ErrorInfo getErrorInfoToUse(
+      SkyKey skyKey,
+      boolean hasValue,
+      Collection<ErrorInfo> childErrorInfos);
+
+  /**
+   * Trivial {@link ErrorInfoManager} implementation whose {@link #fromException} simply uses
+   * {@link ErrorInfo#fromException} and whose {@link #getErrorInfoToUse} makes an {@link ErrorInfo}
+   * from the given {@code childErrorInfos}.
+   */
+  static class UseChildErrorInfoIfNecessary implements ErrorInfoManager {
+    public static final UseChildErrorInfoIfNecessary INSTANCE = new UseChildErrorInfoIfNecessary();
+
+    private UseChildErrorInfoIfNecessary() {
+    }
+
+    @Override
+    public ErrorInfo fromException(
+        SkyKey key,
+        ReifiedSkyFunctionException skyFunctionException,
+        boolean isTransitivelyTransient) {
+      return ErrorInfo.fromException(skyFunctionException, isTransitivelyTransient);
+    }
+
+    @Override
+    @Nullable
+    public ErrorInfo getErrorInfoToUse(
+        SkyKey skyKey,
+        boolean hasValue,
+        Collection<ErrorInfo> childErrorInfos) {
+      return !childErrorInfos.isEmpty() ? ErrorInfo.fromChildErrors(skyKey, childErrorInfos) : null;
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index 4ebde56624..504de3a10e 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -176,6 +176,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
               eventHandler,
               emittedEventState,
               DEFAULT_STORED_EVENT_FILTER,
+              ErrorInfoManager.UseChildErrorInfoIfNecessary.INSTANCE,
               keepGoing,
               numThreads,
               progressReceiver);
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index 0904405a17..beb4a2548b 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -105,6 +105,7 @@ public final class ParallelEvaluator implements Evaluator {
       final ExtendedEventHandler reporter,
       EmittedEventState emittedEventState,
       EventFilter storedEventFilter,
+      ErrorInfoManager errorInfoManager,
       boolean keepGoing,
       int threadCount,
       DirtyTrackingProgressReceiver progressReceiver) {
@@ -117,9 +118,9 @@ public final class ParallelEvaluator implements Evaluator {
             reporter,
             emittedEventState,
             keepGoing,
-            /*storeErrorsAlongsideValues=*/ true,
             progressReceiver,
             storedEventFilter,
+            errorInfoManager,
             createEvaluateRunnable(),
             threadCount);
     cycleDetector = new SimpleCycleDetector();
@@ -132,14 +133,13 @@ public final class ParallelEvaluator implements Evaluator {
       final ExtendedEventHandler reporter,
       EmittedEventState emittedEventState,
       EventFilter storedEventFilter,
+      ErrorInfoManager errorInfoManager,
       boolean keepGoing,
-      boolean storeErrorsAlongsideValues,
       DirtyTrackingProgressReceiver progressReceiver,
       ForkJoinPool forkJoinPool,
       CycleDetector cycleDetector) {
     this.graph = graph;
     this.cycleDetector = cycleDetector;
-    Preconditions.checkState(storeErrorsAlongsideValues || keepGoing);
     evaluatorContext =
         new ParallelEvaluatorContext(
             graph,
@@ -148,9 +148,9 @@ public final class ParallelEvaluator implements Evaluator {
             reporter,
             emittedEventState,
             keepGoing,
-            storeErrorsAlongsideValues,
             progressReceiver,
             storedEventFilter,
+            errorInfoManager,
             createEvaluateRunnable(),
             Preconditions.checkNotNull(forkJoinPool));
   }
@@ -449,15 +449,16 @@ public final class ParallelEvaluator implements Evaluator {
               }
               ErrorInfo depError = depEntry.getErrorInfo();
               if (depError != null) {
-                isTransitivelyTransient |= depError.isTransient();
+                isTransitivelyTransient |= depError.isTransitivelyTransient();
               }
             }
-            ErrorInfo errorInfo =
-                ErrorInfo.fromException(reifiedBuilderException, isTransitivelyTransient);
+            ErrorInfo errorInfo = evaluatorContext.getErrorInfoManager().fromException(
+                skyKey,
+                reifiedBuilderException,
+                isTransitivelyTransient);
             registerNewlyDiscoveredDepsForDoneEntry(
                 skyKey, state, newlyRequestedDeps, oldDeps, env);
-            env.setError(
-                state, errorInfo, /*isDirectlyTransient=*/ reifiedBuilderException.isTransient());
+            env.setError(state, errorInfo);
             env.commit(
                 state,
                 evaluatorContext.keepGoing()
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
index 49782ce513..2f585ff679 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
@@ -47,9 +47,10 @@ class ParallelEvaluatorContext {
   private final ExtendedEventHandler reporter;
   private final NestedSetVisitor<TaggedEvents> replayingNestedSetEventVisitor;
   private final boolean keepGoing;
-  private final boolean storeErrorsAlongsideValues;
   private final DirtyTrackingProgressReceiver progressReceiver;
   private final EventFilter storedEventFilter;
+  private final ErrorInfoManager errorInfoManager;
+
   /**
    * The visitor managing the thread pool. Used to enqueue parents when an entry is finished, and,
    * during testing, to block until an exception is thrown if a node builder requests that.
@@ -65,9 +66,9 @@ class ParallelEvaluatorContext {
       ExtendedEventHandler reporter,
       EmittedEventState emittedEventState,
       boolean keepGoing,
-      boolean storeErrorsAlongsideValues,
       final DirtyTrackingProgressReceiver progressReceiver,
       EventFilter storedEventFilter,
+      ErrorInfoManager errorInfoManager,
       final Function<SkyKey, Runnable> runnableMaker,
       final int threadCount) {
     this.graph = graph;
@@ -77,9 +78,9 @@ class ParallelEvaluatorContext {
     this.replayingNestedSetEventVisitor =
         new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState);
     this.keepGoing = keepGoing;
-    this.storeErrorsAlongsideValues = storeErrorsAlongsideValues;
     this.progressReceiver = Preconditions.checkNotNull(progressReceiver);
     this.storedEventFilter = storedEventFilter;
+    this.errorInfoManager = errorInfoManager;
     visitorSupplier =
         Suppliers.memoize(
             new Supplier<NodeEntryVisitor>() {
@@ -98,9 +99,9 @@ class ParallelEvaluatorContext {
       ExtendedEventHandler reporter,
       EmittedEventState emittedEventState,
       boolean keepGoing,
-      boolean storeErrorsAlongsideValues,
       final DirtyTrackingProgressReceiver progressReceiver,
       EventFilter storedEventFilter,
+      ErrorInfoManager errorInfoManager,
       final Function<SkyKey, Runnable> runnableMaker,
       final ForkJoinPool forkJoinPool) {
     this.graph = graph;
@@ -110,9 +111,9 @@ class ParallelEvaluatorContext {
     this.replayingNestedSetEventVisitor =
         new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState);
     this.keepGoing = keepGoing;
-    this.storeErrorsAlongsideValues = storeErrorsAlongsideValues;
     this.progressReceiver = Preconditions.checkNotNull(progressReceiver);
     this.storedEventFilter = storedEventFilter;
+    this.errorInfoManager = errorInfoManager;
     visitorSupplier =
         Suppliers.memoize(
             new Supplier<NodeEntryVisitor>() {
@@ -203,8 +204,8 @@ class ParallelEvaluatorContext {
     return storedEventFilter;
   }
 
-  boolean storeErrorsAlongsideValues() {
-    return storeErrorsAlongsideValues;
+  ErrorInfoManager getErrorInfoManager() {
+    return errorInfoManager;
   }
 
   /** Receives the events from the NestedSet and delegates to the reporter. */
diff --git a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
index 2adbbc54ed..4a8acc165f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
@@ -160,8 +160,7 @@ class SimpleCycleDetector implements CycleDetector {
                 directDeps,
                 Sets.difference(entry.getAllRemainingDirtyDirectDeps(), removedDeps),
                 evaluatorContext);
-        env.setError(
-            entry, ErrorInfo.fromChildErrors(key, errorDeps), /*isDirectlyTransient=*/ false);
+        env.setError(entry, ErrorInfo.fromChildErrors(key, errorDeps));
         env.commit(entry, EnqueueParentBehavior.SIGNAL);
       } else {
         entry = evaluatorContext.getGraph().get(null, Reason.CYCLE_CHECKING, key);
@@ -220,7 +219,7 @@ class SimpleCycleDetector implements CycleDetector {
           CycleInfo cycleInfo = new CycleInfo(cycle);
           // Add in this cycle.
           allErrors.add(ErrorInfo.fromCycle(cycleInfo));
-          env.setError(entry, ErrorInfo.fromChildErrors(key, allErrors), /*isTransient=*/ false);
+          env.setError(entry, ErrorInfo.fromChildErrors(key, allErrors));
           env.commit(entry, EnqueueParentBehavior.SIGNAL);
           continue;
         } else {
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
index 00f4fa8e7c..427ec944c9 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -226,27 +226,6 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     return postBuilder.build();
   }
 
-  /**
-   * If this node has an error, that is, if errorInfo is non-null, do nothing. Otherwise, set
-   * errorInfo to the union of the child errors that were recorded earlier by getValueOrException,
-   * if there are any.
-   *
-   * <p>Child errors are remembered, if there are any and yet the parent recovered without error, so
-   * that subsequent noKeepGoing evaluations can stop as soon as they encounter a node whose
-   * (transitive) children had experienced an error, even if that (transitive) parent node had been
-   * able to recover from it during a keepGoing build. This behavior can be suppressed by setting
-   * {@link ParallelEvaluatorContext#storeErrorsAlongsideValues} to false, which will cause nodes
-   * with values to have no stored error info. This may be useful if this graph will only ever be
-   * used for keepGoing builds, since in that case storing errors from recovered nodes is pointless.
-   */
-  private void finalizeErrorInfo() {
-    if (errorInfo == null
-        && (evaluatorContext.storeErrorsAlongsideValues() || value == null)
-        && !childErrorInfos.isEmpty()) {
-      errorInfo = ErrorInfo.fromChildErrors(skyKey, childErrorInfos);
-    }
-  }
-
   void setValue(SkyValue newValue) {
     Preconditions.checkState(
         errorInfo == null && bubbleErrorInfo == null,
@@ -264,12 +243,11 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
    * dependencies of this node <i>must</i> already have been registered, since this method may
    * register a dependence on the error transience node, which should always be the last dep.
    */
-  void setError(NodeEntry state, ErrorInfo errorInfo, boolean isDirectlyTransient)
-      throws InterruptedException {
+  void setError(NodeEntry state, ErrorInfo errorInfo)  throws InterruptedException {
     Preconditions.checkState(value == null, "%s %s %s", skyKey, value, errorInfo);
     Preconditions.checkState(this.errorInfo == null, "%s %s %s", skyKey, this.errorInfo, errorInfo);
 
-    if (isDirectlyTransient) {
+    if (errorInfo.isDirectlyTransient()) {
       NodeEntry errorTransienceNode =
           Preconditions.checkNotNull(
               evaluatorContext
@@ -541,7 +519,10 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
   void commit(NodeEntry primaryEntry, EnqueueParentBehavior enqueueParents)
       throws InterruptedException {
     // Construct the definitive error info, if there is one.
-    finalizeErrorInfo();
+    if (errorInfo == null) {
+      errorInfo = evaluatorContext.getErrorInfoManager().getErrorInfoToUse(
+          skyKey, value != null, childErrorInfos);
+    }
 
     // We have the following implications:
     // errorInfo == null => value != null => enqueueParents.
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java
index 54ad3c7657..0db7fa63d7 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java
@@ -78,7 +78,7 @@ public abstract class SkyFunctionException extends Exception {
   }
 
   @Nullable
-  final SkyKey getRootCauseSkyKey() {
+  public final SkyKey getRootCauseSkyKey() {
     return rootCause;
   }
 
@@ -124,9 +124,17 @@ public abstract class SkyFunctionException extends Exception {
     private final boolean isCatastrophic;
 
     public ReifiedSkyFunctionException(SkyFunctionException e, SkyKey key) {
-      super(e.getCause(), e.transience, Preconditions.checkNotNull(e.getRootCauseSkyKey() == null
-          ? key : e.getRootCauseSkyKey()));
-      this.isCatastrophic = e.isCatastrophic();
+      this(e.getCause(), e.transience, key, e.getRootCauseSkyKey(), e.isCatastrophic());
+    }
+
+    protected ReifiedSkyFunctionException(
+        Exception cause,
+        Transience transience,
+        SkyKey key,
+        @Nullable SkyKey rootCauseSkyKey,
+        boolean isCatastrophic) {
+      super(cause, transience, rootCauseSkyKey == null ? key : rootCauseSkyKey);
+      this.isCatastrophic = isCatastrophic;
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index f9ff08e22d..ea00194bab 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -681,6 +681,24 @@ public class PackageFunctionTest extends BuildViewTestCase {
     assertThat(errorInfo.getException()).hasCauseThat().isSameAs(exn);
   }
 
+  @Test
+  public void testPackageLoadingErrorOnIOExceptionReadingBzlFile() throws Exception {
+    scratch.file("foo/BUILD", "load('//foo:bzl.bzl', 'x')");
+    Path fooBzlFilePath = scratch.file("foo/bzl.bzl");
+    IOException exn = new IOException("nope");
+    fs.throwExceptionOnGetInputStream(fooBzlFilePath, exn);
+
+    SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
+    EvaluationResult<PackageValue> result = SkyframeExecutorTestUtils.evaluate(
+        getSkyframeExecutor(), skyKey, /*keepGoing=*/false, reporter);
+    assertThat(result.hasError()).isTrue();
+    ErrorInfo errorInfo = result.getError(skyKey);
+    String errorMessage = errorInfo.getException().getMessage();
+    assertThat(errorMessage).contains("nope");
+    assertThat(errorInfo.getException()).isInstanceOf(NoSuchPackageException.class);
+    assertThat(errorInfo.getException()).hasCauseThat().isSameAs(exn);
+  }
+
   private static class CustomInMemoryFs extends InMemoryFileSystem {
     private abstract static class FileStatusOrException {
       abstract FileStatus get() throws IOException;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 2d121cbe1e..e42ac6efb5 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -811,7 +811,7 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe
     EvaluationResult<SkyValue> result = eval(key);
     assertThat(result.hasError()).isTrue();
     ErrorInfo error = result.getError(key);
-    assertThat(error.isTransient()).isFalse();
+    assertThat(error.isTransitivelyTransient()).isFalse();
     assertThat(error.getException())
         .hasMessageThat()
         .contains("Generated directory a/b/c conflicts with package under the same path.");
diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
index d91e5ca187..95781b6d0b 100644
--- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -140,6 +140,7 @@ public class EagerInvalidatorTest {
             reporter,
             new MemoizingEvaluator.EmittedEventState(),
             InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER,
+            ErrorInfoManager.UseChildErrorInfoIfNecessary.INSTANCE,
             keepGoing,
             200,
             new DirtyTrackingProgressReceiver(null));
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
index 455149445d..e3fd258b11 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java
@@ -46,13 +46,13 @@ public class ErrorInfoSubject extends Subject<ErrorInfoSubject, ErrorInfo> {
   }
 
   public void isTransient() {
-    if (!getSubject().isTransient()) {
+    if (!getSubject().isTransitivelyTransient()) {
       fail("is transient");
     }
   }
 
   public void isNotTransient() {
-    if (getSubject().isTransient()) {
+    if (getSubject().isTransitivelyTransient()) {
       fail("is not transient");
     }
   }
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
index 6dbeede700..a00391481a 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
@@ -59,7 +59,9 @@ public class ErrorInfoTest {
     assertThat(errorInfo.getException()).isSameAs(exception);
     assertThat(errorInfo.getRootCauseOfException()).isSameAs(causeOfException);
     assertThat(errorInfo.getCycleInfo()).isEmpty();
-    assertThat(errorInfo.isTransient()).isEqualTo(isDirectlyTransient || isTransitivelyTransient);
+    assertThat(errorInfo.isDirectlyTransient()).isEqualTo(isDirectlyTransient);
+    assertThat(errorInfo.isTransitivelyTransient()).isEqualTo(
+        isDirectlyTransient || isTransitivelyTransient);
     assertThat(errorInfo.isCatastrophic()).isFalse();
   }
 
@@ -95,7 +97,7 @@ public class ErrorInfoTest {
     assertThat(errorInfo.getRootCauses()).isEmpty();
     assertThat(errorInfo.getException()).isNull();
     assertThat(errorInfo.getRootCauseOfException()).isNull();
-    assertThat(errorInfo.isTransient()).isFalse();
+    assertThat(errorInfo.isTransitivelyTransient()).isFalse();
     assertThat(errorInfo.isCatastrophic()).isFalse();
   }
 
@@ -141,7 +143,7 @@ public class ErrorInfoTest {
         new CycleInfo(
             ImmutableList.of(currentKey, Iterables.getOnlyElement(cycle.getPathToCycle())),
             cycle.getCycle()));
-    assertThat(errorInfo.isTransient()).isTrue();
+    assertThat(errorInfo.isTransitivelyTransient()).isTrue();
     assertThat(errorInfo.isCatastrophic()).isTrue();
   }
 
@@ -154,6 +156,7 @@ public class ErrorInfoTest {
           /*rootCauseOfException=*/ null,
           ImmutableList.<CycleInfo>of(),
           false,
+          false,
           false);
     } catch (IllegalStateException e) {
       // Brittle, but confirms we failed for the right reason.
@@ -171,6 +174,7 @@ public class ErrorInfoTest {
           /*rootCauseOfException=*/ null,
           ImmutableList.<CycleInfo>of(),
           false,
+          false,
           false);
     } catch (IllegalStateException e) {
       // Brittle, but confirms we failed for the right reason.
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 8842f84b21..314d4ac05d 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -1200,7 +1200,7 @@ public class MemoizingEvaluatorTest {
     if (errorsStoredAlongsideValues) {
       // The parent should be transitively transient, since it transitively depends on a transient
       // error.
-      assertThat(errorInfo.isTransient()).isTrue();
+      assertThat(errorInfo.isTransitivelyTransient()).isTrue();
       assertThat(errorInfo.getException()).hasMessage(NODE_TYPE.getName() + ":errorKey");
       assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey);
     } else {
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
index 109b2fbac1..094f75ac1c 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -99,6 +99,7 @@ public class ParallelEvaluatorTest {
         new Reporter(new EventBus(), eventCollector),
         new MemoizingEvaluator.EmittedEventState(),
         storedEventFilter,
+        ErrorInfoManager.UseChildErrorInfoIfNecessary.INSTANCE,
         keepGoing,
         150,
         revalidationReceiver);