Make lost input ownership tracking internal to ActionRewindStrategy.

Removes the option for owners to be pre-calculated and stored in `LostInputsExecException` and `LostInputsActionExecutionException`. There was only one place that was doing this.

Since the calculation is now private to `ActionRewindStrategy`, we don't need `LostInputOwners` - the preconditions checks are unnecessary, since we know that `ActionRewindStrategy` calculates owners correctly.

PiperOrigin-RevId: 878563149
Change-Id: Ie3c757d86a87a7cc87d853d030d6cef4cabbb717

Author: justinhorvitz

src/main/java/com/google/devtools/build/lib/actions/BUILD

@@ -174,7 +174,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
-        "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:lost_input_owners",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
@@ -474,7 +473,6 @@ java_library(
         ":actions",
         ":artifacts",
         "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
-        "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:lost_input_owners",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",

src/main/java/com/google/devtools/build/lib/actions/ImportantOutputHandler.java

@@ -18,14 +18,12 @@
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.skyframe.DetailedException;
-import com.google.devtools.build.lib.skyframe.rewinding.LostInputOwners;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.time.Duration;
 import java.util.Collection;
 import java.util.Map;
-import java.util.Optional;
 
 /** Context to be informed of top-level outputs and their runfiles. */
 public interface ImportantOutputHandler extends ActionContext {
@@ -139,21 +137,14 @@ void processWorkspaceStatusOutputs(Path stableOutput, Path volatileOutput)
   void processTooLargeStdoutErr(Path stdoutErr)
       throws ImportantOutputException, InterruptedException;
 
-  /**
-   * Represents artifacts that need to be regenerated via action rewinding, optionally along with
-   * their owners if known. If {@code owners} is present, the ownership information must be
-   * complete.
-   */
-  record LostArtifacts(
-      ImmutableSetMultimap<String, ActionInput> byDigest, Optional<LostInputOwners> owners) {
+  /** Represents artifacts that need to be regenerated via action rewinding. */
+  record LostArtifacts(ImmutableSetMultimap<String, ActionInput> byDigest) {
 
     /** An empty instance of {@link LostArtifacts}. */
-    public static final LostArtifacts EMPTY =
-        new LostArtifacts(ImmutableSetMultimap.of(), Optional.of(new LostInputOwners()));
+    public static final LostArtifacts EMPTY = new LostArtifacts(ImmutableSetMultimap.of());
 
     public LostArtifacts {
       checkNotNull(byDigest);
-      checkNotNull(owners);
     }
 
     public boolean isEmpty() {
@@ -163,7 +154,7 @@ public boolean isEmpty() {
     /** Throws {@link LostInputsExecException} if this instance is not empty. */
     public void throwIfNotEmpty() throws LostInputsExecException {
       if (!isEmpty()) {
-        throw new LostInputsExecException(byDigest, owners, /* cause= */ null);
+        throw new LostInputsExecException(byDigest);
       }
     }
   }

src/main/java/com/google/devtools/build/lib/actions/LostInputsActionExecutionException.java

@@ -16,11 +16,9 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSetMultimap;
-import com.google.devtools.build.lib.skyframe.rewinding.LostInputOwners;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
-import java.util.Optional;
 import javax.annotation.Nullable;
 
 /**
@@ -32,14 +30,6 @@ public final class LostInputsActionExecutionException extends ActionExecutionExc
   /** Maps lost input digests to their {@link ActionInput}s. */
   private final ImmutableSetMultimap<String, ActionInput> lostInputs;
 
-  /**
-   * Optional mapping of lost inputs to their owning expansion artifacts (tree artifacts, filesets,
-   * runfiles).
-   *
-   * <p>See {@link LostInputsExecException} for details on whether this should be provided.
-   */
-  private final Optional<LostInputOwners> owners;
-
   /**
    * The {@link ActionLookupData} for the action whose evaluation failed. Used to distinguish
    * whether an action handling this exception was primary in its set of shared actions. Event
@@ -73,23 +63,17 @@ public final class LostInputsActionExecutionException extends ActionExecutionExc
   public LostInputsActionExecutionException(
       String message,
       ImmutableSetMultimap<String, ActionInput> lostInputs,
-      Optional<LostInputOwners> owners,
       Action action,
       Exception cause,
       DetailedExitCode detailedExitCode) {
     super(message, cause, action, /* catastrophe= */ false, detailedExitCode);
     this.lostInputs = lostInputs;
-    this.owners = owners;
   }
 
   public ImmutableSetMultimap<String, ActionInput> getLostInputs() {
     return lostInputs;
   }
 
-  public Optional<LostInputOwners> getOwners() {
-    return owners;
-  }
-
   public Path getPrimaryOutputPath() {
     return primaryOutputPath;
   }
@@ -149,6 +133,6 @@ public LostInputsExecException toExecException() {
     Preconditions.checkState(primaryOutputPath == null);
     Preconditions.checkState(fileOutErr == null);
     Preconditions.checkState(!fromInputDiscovery);
-    return new LostInputsExecException(lostInputs, owners, this);
+    return new LostInputsExecException(lostInputs, this);
   }
 }

src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java

@@ -15,16 +15,13 @@
 package com.google.devtools.build.lib.actions;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.devtools.build.lib.server.FailureDetails.Execution;
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.lib.skyframe.rewinding.LostInputOwners;
 import com.google.devtools.build.lib.util.DetailedExitCode;
-import java.util.Optional;
 import javax.annotation.Nullable;
 
 /**
@@ -36,50 +33,25 @@ public final class LostInputsExecException extends ExecException {
   /** Maps lost input digests to their {@link ActionInput}s. */
   private final ImmutableSetMultimap<String, ActionInput> lostInputs;
 
-  /**
-   * Optional mapping of lost inputs to their owning expansion artifacts (tree artifacts, filesets,
-   * runfiles).
-   *
-   * <p>If present, the mapping must be complete. Spawn runners should only provide this if they can
-   * do so correctly and efficiently. If not provided, {@link
-   * com.google.devtools.build.lib.skyframe.rewinding.ActionRewindStrategy} will calculate the
-   * ownership mappings - the only benefit of providing them here is the performance benefit of
-   * skipping that step.
-   */
-  private final Optional<LostInputOwners> owners;
-
   public LostInputsExecException(ImmutableSetMultimap<String, ActionInput> lostInputs) {
-    this(lostInputs, /* owners= */ Optional.empty());
-  }
-
-  public LostInputsExecException(
-      ImmutableSetMultimap<String, ActionInput> lostInputs, Optional<LostInputOwners> owners) {
-    this(lostInputs, owners, /* cause= */ null);
+    this(lostInputs, /* cause= */ null);
   }
 
   public LostInputsExecException(
-      ImmutableSetMultimap<String, ActionInput> lostInputs,
-      Optional<LostInputOwners> owners,
-      @Nullable Throwable cause) {
+      ImmutableSetMultimap<String, ActionInput> lostInputs, @Nullable Throwable cause) {
     super("lost inputs with digests: " + String.join(",", lostInputs.keySet()), cause);
     checkArgument(!lostInputs.isEmpty(), "No inputs were lost");
     this.lostInputs = lostInputs;
-    this.owners = checkNotNull(owners);
   }
 
   @VisibleForTesting
   public ImmutableSetMultimap<String, ActionInput> getLostInputs() {
     return lostInputs;
   }
 
-  @VisibleForTesting
-  public Optional<LostInputOwners> getOwners() {
-    return owners;
-  }
-
   ActionExecutionException fromExecException(String message, Action action, DetailedExitCode code) {
     return new LostInputsActionExecutionException(
-        message, lostInputs, owners, action, /* cause= */ this, code);
+        message, lostInputs, action, /* cause= */ this, code);
   }
 
   @Override
@@ -97,8 +69,7 @@ public LostInputsExecException combine(LostInputsExecException other) {
             .putAll(other.lostInputs)
             .build();
     LostInputsExecException combined =
-        new LostInputsExecException(
-            combinedLostInputs, /* owners= */ Optional.empty(), /* cause= */ this);
+        new LostInputsExecException(combinedLostInputs, /* cause= */ this);
     combined.addSuppressed(other);
     return combined;
   }

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -911,10 +911,7 @@ public void createHardLink(PathFragment linkPath, PathFragment originalPath) thr
   public void checkForLostInputs(Action action) throws LostInputsActionExecutionException {
     var mergedException =
         lostInputs.stream()
-            .map(
-                lostArtifacts ->
-                    new LostInputsExecException(
-                        lostArtifacts.byDigest(), lostArtifacts.owners(), /* cause= */ null))
+            .map(lostArtifacts -> new LostInputsExecException(lostArtifacts.byDigest()))
             .reduce(LostInputsExecException::combine);
     if (mergedException.isPresent()) {
       throw (LostInputsActionExecutionException)

src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java

@@ -26,7 +26,6 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.function.Function;
 
 /**
@@ -114,7 +113,7 @@ public LostArtifacts getLostArtifacts(Function<PathFragment, ActionInput> action
       byDigestBuilder.put(DigestUtil.toString(missingDigest), actionInput);
     }
     var byDigest = byDigestBuilder.build();
-    return new LostArtifacts(byDigest, Optional.empty());
+    return new LostArtifacts(byDigest);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

@@ -22,7 +22,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CompletionContext;
@@ -424,20 +423,6 @@ private RewindPlanResult informImportantOutputHandler(
         return null;
       }
 
-      var owners =
-          lostOutputs
-              .owners()
-              .orElseGet(
-                  () ->
-                      ActionRewindStrategy.calculateLostInputOwners(
-                          lostOutputs.byDigest().values(), metadataProvider));
-      // Filter out lost outputs from the set of built artifacts so that they are not reported. If
-      // rewinding is successful, we'll report them later on.
-      for (ActionInput lostOutput : lostOutputs.byDigest().values()) {
-        builtArtifacts.remove(lostOutput);
-        builtArtifacts.removeAll(owners.getOwners(lostOutput));
-      }
-
       Iterable<Artifact> artifactsRelevantForRewinding = importantArtifacts;
       if (importantOutputHandler.requiresHiddenOutputMetadata()) {
         var hiddenTopLevelArtifacts =
@@ -453,7 +438,8 @@ private RewindPlanResult informImportantOutputHandler(
           key,
           ImmutableSet.copyOf(Artifact.keys(artifactsRelevantForRewinding)),
           lostOutputs.byDigest(),
-          owners,
+          metadataProvider,
+          builtArtifacts,
           env);
     } catch (ActionRewindException | ImportantOutputException e) {
       LabelCause cause = new LabelCause(label, e.getDetailedExitCode());