Split MetadataProvider.getMetadata() into two methods.

One for getting metadata for the inputs of the action and one for its outputs.

The principal difference between the two is that when the action starts
executing, the metadata for its inputs should be known, so no expensive I/O
should be needed. This would in principle allow one to not throw IOException
there. It remains for two reasons:

* Some conditions are signaled by throwing FileNotFoundException (I haven't
  investigated very deeply)
* Some implementations still do I/O for reasons unknown

At the very least, this will allow one to not throw InterruptedException in
getInputMetadata().

Work towards fixing #17009.

RELNOTES: None.
PiperOrigin-RevId: 523318267
Change-Id: Ib3af4c099a0faafb9a8b6d75d04928af47bfbcd1

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

@@ -553,7 +553,7 @@ protected void checkInputsForDirectories(
       // Assume that if the file did not exist, we would not have gotten here.
       try {
         if (input.isSourceArtifact()
-            && metadataProvider.getMetadata(input).getType().isDirectory()) {
+            && metadataProvider.getInputMetadata(input).getType().isDirectory()) {
           // TODO(ulfjack): What about dependency checking of special files?
           eventHandler.handle(
               Event.warn(
@@ -583,7 +583,7 @@ protected void checkOutputsForDirectories(ActionExecutionContext actionExecution
         continue;
       }
       try {
-        metadata = metadataHandler.getMetadata(output);
+        metadata = metadataHandler.getOutputMetadata(output);
       } catch (IOException e) {
         logger.atWarning().withCause(e).log("Error getting metadata for %s", output);
         metadata = null;

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

@@ -198,14 +198,14 @@ private static boolean validateArtifacts(
       for (Artifact artifact : action.getOutputs()) {
         FileArtifactValue metadata = getCachedMetadata(cachedOutputMetadata, artifact);
         if (metadata == null) {
-          metadata = getMetadataMaybe(metadataHandler, artifact);
+          metadata = getOutputMetadataMaybe(metadataHandler, artifact);
         }
 
         mdMap.put(artifact.getExecPathString(), metadata);
       }
     }
     for (Artifact artifact : actionInputs.toList()) {
-      mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact));
+      mdMap.put(artifact.getExecPathString(), getInputMetadataMaybe(metadataHandler, artifact));
     }
     return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest());
   }
@@ -396,7 +396,7 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
 
         FileArtifactValue localMetadata;
         try {
-          localMetadata = getMetadataOrConstant(metadataHandler, artifact);
+          localMetadata = getOutputMetadataOrConstant(metadataHandler, artifact);
         } catch (FileNotFoundException ignored) {
           localMetadata = null;
         } catch (IOException e) {
@@ -571,22 +571,43 @@ private boolean mustExecute(
     return false;
   }
 
-  private static FileArtifactValue getMetadataOrConstant(
+  private static FileArtifactValue getInputMetadataOrConstant(
       MetadataHandler metadataHandler, Artifact artifact) throws IOException {
-    FileArtifactValue metadata = metadataHandler.getMetadata(artifact);
+    FileArtifactValue metadata = metadataHandler.getInputMetadata(artifact);
     return (metadata != null && artifact.isConstantMetadata())
         ? ConstantMetadataValue.INSTANCE
         : metadata;
   }
 
+  private static FileArtifactValue getOutputMetadataOrConstant(
+      MetadataHandler metadataHandler, Artifact artifact) throws IOException {
+    FileArtifactValue metadata = metadataHandler.getOutputMetadata(artifact);
+    return (metadata != null && artifact.isConstantMetadata())
+        ? ConstantMetadataValue.INSTANCE
+        : metadata;
+  }
+
+  // TODO(ulfjack): It's unclear to me why we're ignoring all IOExceptions. In some cases, we want
+  // to trigger a re-execution, so we should catch the IOException explicitly there. In others, we
+  // should propagate the exception, because it is unexpected (e.g., bad file system state).
+  @Nullable
+  private static FileArtifactValue getInputMetadataMaybe(
+      MetadataHandler metadataHandler, Artifact artifact) {
+    try {
+      return getInputMetadataOrConstant(metadataHandler, artifact);
+    } catch (IOException e) {
+      return null;
+    }
+  }
+
   // TODO(ulfjack): It's unclear to me why we're ignoring all IOExceptions. In some cases, we want
   // to trigger a re-execution, so we should catch the IOException explicitly there. In others, we
   // should propagate the exception, because it is unexpected (e.g., bad file system state).
   @Nullable
-  private static FileArtifactValue getMetadataMaybe(
+  private static FileArtifactValue getOutputMetadataMaybe(
       MetadataHandler metadataHandler, Artifact artifact) {
     try {
-      return getMetadataOrConstant(metadataHandler, artifact);
+      return getOutputMetadataOrConstant(metadataHandler, artifact);
     } catch (IOException e) {
       return null;
     }
@@ -632,7 +653,7 @@ public void updateActionCache(
           // the 'constant' metadata for the volatile workspace status output. The volatile output
           // contains information such as timestamps, and even when --stamp is enabled, we don't
           // want to rebuild everything if only that file changes.
-          FileArtifactValue metadata = getMetadataOrConstant(metadataHandler, output);
+          FileArtifactValue metadata = getOutputMetadataOrConstant(metadataHandler, output);
           checkState(metadata != null);
           entry.addOutputFile(output, metadata, cacheConfig.storeOutputMetadata());
         }
@@ -650,7 +671,7 @@ public void updateActionCache(
     for (Artifact input : action.getInputs().toList()) {
       entry.addInputFile(
           input.getExecPath(),
-          getMetadataMaybe(metadataHandler, input),
+          getInputMetadataMaybe(metadataHandler, input),
           /* saveExecPath= */ !excludePathsFromActionCache.contains(input));
     }
     entry.getFileDigest();
@@ -769,7 +790,9 @@ private void checkMiddlemanAction(
       entry = new ActionCache.Entry("", ImmutableMap.of(), false, OutputPermissions.READONLY);
       for (Artifact input : action.getInputs().toList()) {
         entry.addInputFile(
-            input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true);
+            input.getExecPath(),
+            getInputMetadataMaybe(metadataHandler, input),
+            /* saveExecPath= */ true);
       }
     }
 

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

@@ -406,6 +406,44 @@ public void close() throws IOException {
     }
   }
 
+  /**
+   * Creates a new {@link ActionExecutionContext} whose {@link MetadataProvider} has the given
+   * {@link ActionInput}s as inputs.
+   *
+   * <p>Each {@link ActionInput} must be an output of the current {@link ActionExecutionContext} and
+   * it must already have been built.
+   */
+  public ActionExecutionContext withOutputsAsInputs(
+      Iterable<? extends ActionInput> additionalInputs) throws IOException {
+    ImmutableMap.Builder<ActionInput, FileArtifactValue> additionalInputMap =
+        ImmutableMap.builder();
+
+    for (ActionInput input : additionalInputs) {
+      additionalInputMap.put(input, getMetadataHandler().getOutputMetadata(input));
+    }
+    StaticMetadataProvider additionalInputMetadata =
+        new StaticMetadataProvider(additionalInputMap.buildOrThrow());
+
+    return new ActionExecutionContext(
+        executor,
+        new DelegatingPairMetadataProvider(actionInputFileCache, additionalInputMetadata),
+        actionInputPrefetcher,
+        actionKeyContext,
+        metadataHandler,
+        rewindingEnabled,
+        lostInputsCheck,
+        fileOutErr,
+        eventHandler,
+        clientEnv,
+        topLevelFilesets,
+        artifactExpander,
+        env,
+        actionFileSystem,
+        skyframeDepsResult,
+        discoveredModulesPruner,
+        syscallCache,
+        threadStateReceiverForMetrics);
+  }
   /**
    * Allows us to create a new context that overrides the FileOutErr with another one. This is
    * useful for muting the output for example.

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

@@ -174,7 +174,7 @@ private int getIndex(String execPathString) {
 
   @Nullable
   @Override
-  public FileArtifactValue getMetadata(ActionInput input) {
+  public FileArtifactValue getInputMetadata(ActionInput input) {
     if (input instanceof TreeFileArtifact) {
       TreeFileArtifact treeFileArtifact = (TreeFileArtifact) input;
       int treeIndex = getIndex(treeFileArtifact.getParent().getExecPathString());

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

@@ -20,12 +20,22 @@
 
 /** Prefetches files to local disk. */
 public interface ActionInputPrefetcher {
+  /**
+   * Returns the metadata for an {@link ActionInput}.
+   *
+   * <p>This will generally call through to a {@link MetadataProvider} and ask for the metadata of
+   * either an input or an output artifact.
+   */
+  public interface MetadataSupplier {
+    FileArtifactValue getMetadata(ActionInput actionInput) throws IOException;
+  }
+
   public static final ActionInputPrefetcher NONE =
       new ActionInputPrefetcher() {
         @Override
         public ListenableFuture<Void> prefetchFiles(
             Iterable<? extends ActionInput> inputs,
-            MetadataProvider metadataProvider,
+            MetadataSupplier metadataSupplier,
             Priority priority) {
           // Do nothing.
           return immediateVoidFuture();
@@ -69,7 +79,7 @@ public enum Priority {
    * @return future success if prefetch is finished or {@link IOException}.
    */
   ListenableFuture<Void> prefetchFiles(
-      Iterable<? extends ActionInput> inputs, MetadataProvider metadataProvider, Priority priority);
+      Iterable<? extends ActionInput> inputs, MetadataSupplier metadataSupplier, Priority priority);
 
   /**
    * Whether the prefetcher requires the metadata for a tree artifact to be available whenever one

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

@@ -111,7 +111,7 @@ public ActionInputMap getImportantInputMap() {
 
   @Nullable
   public FileArtifactValue getFileArtifactValue(Artifact artifact) {
-    return importantInputMap.getMetadata(artifact);
+    return importantInputMap.getInputMetadata(artifact);
   }
 
   /** Returns true if the given artifact is guaranteed to be a file (and not a directory). */
@@ -136,7 +136,7 @@ public void visitArtifacts(Iterable<Artifact> artifacts, ArtifactReceiver receiv
                   : RelativeSymlinkBehaviorWithoutError.RESOLVE);
         }
       } else if (artifact.isTreeArtifact()) {
-        FileArtifactValue treeArtifactMetadata = importantInputMap.getMetadata(artifact);
+        FileArtifactValue treeArtifactMetadata = importantInputMap.getInputMetadata(artifact);
         if (treeArtifactMetadata == null) {
           BugReport.sendBugReport(
               new IllegalStateException(

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

@@ -0,0 +1,43 @@
+// Copyright 2023 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.lib.actions;
+
+import java.io.IOException;
+
+/** A {@link MetadataProvider} implementation that consults two others in a given order. */
+public final class DelegatingPairMetadataProvider implements MetadataProvider {
+
+  private final MetadataProvider primary;
+  private final MetadataProvider secondary;
+
+  public DelegatingPairMetadataProvider(MetadataProvider primary, MetadataProvider secondary) {
+    this.primary = primary;
+    this.secondary = secondary;
+  }
+
+  @Override
+  public FileArtifactValue getInputMetadata(ActionInput input) throws IOException {
+    FileArtifactValue metadata = primary.getInputMetadata(input);
+    return (metadata != null) && (metadata != FileArtifactValue.MISSING_FILE_MARKER)
+        ? metadata
+        : secondary.getInputMetadata(input);
+  }
+
+  @Override
+  public ActionInput getInput(String execPath) {
+    ActionInput input = primary.getInput(execPath);
+    return input != null ? input : secondary.getInput(execPath);
+  }
+}

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

@@ -22,16 +22,9 @@
 /** Provides {@link ActionInput} metadata. */
 @ThreadSafe
 public interface MetadataProvider {
-
   /**
    * Returns a {@link FileArtifactValue} for the given {@link ActionInput}.
    *
-   * <p>If the given input is an output {@link Artifact} of an action, then the returned value is
-   * current as of the action's most recent execution time (which may be from a prior build, in
-   * which case the value may or may not be up to date for the current build). The returned value
-   * can vary across calls, for example if the action executes between calls and produces different
-   * outputs than its previous execution.
-   *
    * <p>The returned {@link FileArtifactValue} instance corresponds to the final target of a symlink
    * and therefore must not have a type of {@link FileStateType#SYMLINK}.
    *
@@ -43,10 +36,10 @@ public interface MetadataProvider {
    * @param input the input to retrieve the digest for
    * @return the artifact's digest or null if digest cannot be obtained (due to artifact
    *     non-existence, lookup errors, or any other reason)
-   * @throws IOException if the input cannot be digested
+   * @throws IOException if the action input cannot be digested
    */
   @Nullable
-  FileArtifactValue getMetadata(ActionInput input) throws IOException;
+  FileArtifactValue getInputMetadata(ActionInput input) throws IOException;
 
   /** Looks up an input from its exec path. */
   @Nullable
@@ -67,8 +60,8 @@ default FileSystem getFileSystemForInputResolution() {
   }
 
   /**
-   * Indicates whether calls to {@link #getMetadata} with a {@link Artifact.DerivedArtifact} may
-   * require a skyframe lookup.
+   * Indicates whether calls to {@link #getInputMetadata} with a {@link Artifact.DerivedArtifact}
+   * may require a skyframe lookup.
    */
   default boolean mayGetGeneratingActionsFromSkyframe() {
     return false;

src/main/java/com/google/devtools/build/lib/remote/util/StaticMetadataProvider.java → src/main/java/com/google/devtools/build/lib/actions/StaticMetadataProvider.java

@@ -11,12 +11,9 @@
 // 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.remote.util;
+package com.google.devtools.build.lib.actions;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.MetadataProvider;
 import java.util.Collection;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -50,7 +47,7 @@ private static ImmutableMap<String, ActionInput> constructExecPathToInputMap(
 
   @Nullable
   @Override
-  public FileArtifactValue getMetadata(ActionInput input) {
+  public FileArtifactValue getInputMetadata(ActionInput input) {
     return inputToMetadata.get(input);
   }
 

src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java

@@ -19,27 +19,35 @@
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import java.io.IOException;
-import javax.annotation.Nullable;
 
 /** Handles metadata of inputs and outputs during the execution phase. */
 public interface MetadataHandler extends MetadataProvider, MetadataInjector {
 
   /**
-   * {@inheritDoc}
+   * Returns a {@link FileArtifactValue} for the given {@link ActionInput}.
+   *
+   * <p>If the metadata of the given {@link ActionInput} is not known, it's computed. This may
+   * result in a significant amount of I/O.
+   *
+   * <p>The returned {@link FileArtifactValue} instance corresponds to the final target of a symlink
+   * and therefore must not have a type of {@link FileStateType#SYMLINK}.
    *
    * <p>Freshly created output files (i.e. from an action that just executed) that require a stat to
    * obtain the metadata will first be set read-only and executable during this call. This ensures
    * that the returned metadata has an appropriate ctime, which is affected by chmod. Note that this
    * does not apply to outputs injected via {@link #injectFile} or {@link #injectTree} since a stat
    * is not required for them.
+   *
+   * @param output the output to retrieve the digest for
+   * @return the artifact's digest or null the artifact is not a known output of the action
+   * @throws IOException if the action input cannot be digested
    */
-  @Override
-  @Nullable
-  FileArtifactValue getMetadata(ActionInput input) throws IOException;
+  FileArtifactValue getOutputMetadata(ActionInput output) throws IOException;
 
   /** Sets digest for virtual artifacts (e.g. middlemen). {@code digest} must not be null. */
   void setDigestForVirtualArtifact(Artifact artifact, byte[] digest);

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -255,7 +255,7 @@ public ListenableFuture<Void> prefetchInputs()
                 .prefetchFiles(
                     getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true)
                         .values(),
-                    getMetadataProvider(),
+                    getMetadataProvider()::getInputMetadata,
                     Priority.MEDIUM),
             BulkTransferException.class,
             (BulkTransferException e) -> {

src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java

@@ -55,7 +55,10 @@ public SingleBuildFileCache(String cwd, FileSystem fs, XattrProvider xattrProvid
   }
 
   @Override
-  public FileArtifactValue getMetadata(ActionInput input) throws IOException {
+  public FileArtifactValue getInputMetadata(ActionInput input) throws IOException {
+    // TODO(lberki): It would be nice to assert that only source files are passed here.
+    // Unfortunately, that's not quite true at the moment and an unknown amount of work would be
+    // needed to make that true.
     return pathToMetadata
         .get(
             input.getExecPathString(),