Improve the documentation for OutputMetadataStore.

Also make the getOutputMetadata argument an Artifact instead of an ActionInput, since we always cast it anyway.

PiperOrigin-RevId: 832479663
Change-Id: I092f6316d217add6528803a31bc135a017926eee

Author: tjgq

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

@@ -562,18 +562,18 @@ private ActionExecutionContext withInputMetadataProvider(
 
   /**
    * Creates a new {@link ActionExecutionContext} whose {@link InputMetadataProvider} has the given
-   * {@link ActionInput}s as inputs.
+   * {@link Artifact}s as inputs.
    *
-   * <p>Each {@link ActionInput} must be an output of the current {@link ActionExecutionContext} and
-   * it must already have been built.
+   * <p>Each {@link Artifact} 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, InterruptedException {
+  public ActionExecutionContext withOutputsAsInputs(Iterable<Artifact> outputs)
+      throws IOException, InterruptedException {
     ImmutableMap.Builder<ActionInput, FileArtifactValue> additionalInputMap =
         ImmutableMap.builder();
 
-    for (ActionInput input : additionalInputs) {
-      additionalInputMap.put(input, outputMetadataStore.getOutputMetadata(input));
+    for (Artifact output : outputs) {
+      additionalInputMap.put(output, outputMetadataStore.getOutputMetadata(output));
     }
 
     StaticInputMetadataProvider additionalInputMetadata =

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

@@ -13,13 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions.cache;
 
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import java.io.IOException;
+import javax.annotation.Nullable;
 
 /** Handles the metadata of the outputs of the action during its execution. */
 public interface OutputMetadataStore {
@@ -48,27 +48,47 @@ public interface OutputMetadataStore {
   void injectTree(SpecialArtifact output, TreeArtifactValue tree);
 
   /**
-   * Returns a {@link FileArtifactValue} for the given {@link ActionInput}.
+   * Returns a {@link FileArtifactValue} for the given {@link Artifact}.
    *
-   * <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>If the metadata of the given {@link Artifact} has not been injected via {@link #injectFile},
+   * it will be computed from the filesystem. This may result in a significant amount of I/O. The
+   * result will be cached for future calls to this method.
    *
-   * <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>For artifacts of non-symlink type (i.e., {@link Artifact#isSymlink} returns false), the
+   * returned {@link FileArtifactValue} corresponds to the final target of a symlink when one exists
+   * in the filesystem, and therefore will 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.
+   * <p>If a stat is required to obtain the metadata, the output will first be set read-only and
+   * executable by 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} 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
+   * @param artifact the artifact to retrieve metadata for
+   * @return the artifact metadata, or null the artifact is not a known output of the action
+   * @throws IOException if the metadata cannot be obtained from the filesystem
+   * @throws InterruptedException if the current thread is interrupted while computing the metadata
    */
-  FileArtifactValue getOutputMetadata(ActionInput output) throws IOException, InterruptedException;
+  @Nullable
+  FileArtifactValue getOutputMetadata(Artifact artifact) throws IOException, InterruptedException;
 
-  /** Retrieves the metadata for this tree artifact. Data should already be available. */
+  /**
+   * Returns a {@link TreeArtifactValue} for the given {@link SpecialArtifact}, which must be a tree
+   * artifact (i.e., {@link SpecialArtifact#isTreeArtifact} must return true).
+   *
+   * <p>If the metadata of the given {@link SpecialArtifact} has not been injected via {@link
+   * #injectTree}, it will be computed from the filesystem. This may result in a significant amount
+   * of I/O. The result will be cached for future calls to this method.
+   *
+   * <p>If a stat is required to obtain the metadata, the output will first be set read-only and
+   * executable by 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} since a stat is not required for them.
+   *
+   * @param treeArtifact the tree artifact to retrieve metadata for
+   * @return the tree artifact metadata
+   * @throws IOException if the metadata cannot be obtained from the filesystem
+   * @throws InterruptedException if the current thread is interrupted while computing the metadata
+   */
   TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact)
       throws IOException, InterruptedException;
 

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

@@ -23,9 +23,12 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 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.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -754,13 +757,13 @@ private TestAttemptResult runTestAttempt(
               .getOutputMetadataStore()
               .getOutputMetadata(testAction.getCoverageDirectoryTreeArtifact());
 
-      ImmutableSet<? extends ActionInput> expandedCoverageDir =
+      ImmutableSortedSet<TreeFileArtifact> expandedCoverageDir =
           actionExecutionContext
               .getOutputMetadataStore()
               .getTreeArtifactValue((SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact())
               .getChildren();
-      ImmutableSet<ActionInput> coverageSpawnMetadata =
-          ImmutableSet.<ActionInput>builder()
+      ImmutableSet<Artifact> coverageSpawnMetadata =
+          ImmutableSet.<Artifact>builder()
               .addAll(expandedCoverageDir)
               .add(testAction.getCoverageDirectoryTreeArtifact())
               .build();

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

@@ -90,8 +90,8 @@ enum DirectoryState {
   /**
    * Returns the metadata for an {@link ActionInput}.
    *
-   * <p>This will generally call through to a {@link InputMetadataProvider} and ask for the metadata
-   * of either an input or an output artifact.
+   * <p>This will generally call through to a {@link InputMetadataProvider} or {@link
+   * OutputMetadataStore} and ask for the metadata of either an input or an output artifact.
    */
   @VisibleForTesting
   public interface MetadataSupplier {
@@ -762,7 +762,7 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor
             prefetchFilesInterruptibly(
                 action,
                 outputsToDownload,
-                outputMetadataStore::getOutputMetadata,
+                output -> outputMetadataStore.getOutputMetadata((Artifact) output),
                 Priority.HIGH,
                 Reason.OUTPUTS));
       }

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

@@ -26,7 +26,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
@@ -71,9 +70,9 @@
  * files are set read-only and executable <em>before</em> statting them to ensure that the stat's
  * ctime is up to date.
  *
- * <p>After action execution, {@link #getOutputMetadata} should be called on each of the action's
- * outputs (except those that were {@linkplain #artifactOmitted omitted}) to ensure that declared
- * outputs were in fact created and are valid.
+ * <p>After action execution, {@link #getOutputMetadata} or {@link #getTreeArtifactValue} should be
+ * called on each of the action's outputs (except those that were {@linkplain #artifactOmitted
+ * omitted}) to ensure that declared outputs were in fact created and are valid.
  */
 final class ActionOutputMetadataStore implements OutputMetadataStore {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -176,18 +175,15 @@ private boolean isKnownOutput(Artifact artifact) {
 
   @Nullable
   @Override
-  public FileArtifactValue getOutputMetadata(ActionInput actionInput)
+  public FileArtifactValue getOutputMetadata(Artifact artifact)
       throws IOException, InterruptedException {
-    Artifact artifact = (Artifact) actionInput;
-    FileArtifactValue value;
-
     if (!isKnownOutput(artifact)) {
       return null;
     }
 
     if (artifact.isRunfilesTree()) {
       // Runfiles trees get a placeholder value, see the Javadoc of RUNFILES_TREE_MARKER as to why
-      value = artifactData.get(artifact);
+      FileArtifactValue value = artifactData.get(artifact);
       if (value != null) {
         return checkExists(value, artifact);
       }
@@ -202,11 +198,12 @@ public FileArtifactValue getOutputMetadata(ActionInput actionInput)
 
     if (artifact.isChildOfDeclaredDirectory()) {
       TreeArtifactValue tree = getTreeArtifactValue(artifact.getParent());
-      value = tree.getChildValues().getOrDefault(artifact, FileArtifactValue.MISSING_FILE_MARKER);
+      FileArtifactValue value =
+          tree.getChildValues().getOrDefault(artifact, FileArtifactValue.MISSING_FILE_MARKER);
       return checkExists(value, artifact);
     }
 
-    value = artifactData.get(artifact);
+    FileArtifactValue value = artifactData.get(artifact);
     if (value != null) {
       return checkExists(value, artifact);
     }

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

@@ -43,7 +43,6 @@
 import com.google.devtools.build.lib.actions.ActionExecutionContext.LostInputsCheck;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
@@ -147,7 +146,7 @@ public final class SkyframeActionExecutor {
   private static final OutputMetadataStore THROWING_OUTPUT_METADATA_STORE_FOR_ACTIONFS =
       new OutputMetadataStore() {
         @Override
-        public FileArtifactValue getOutputMetadata(ActionInput output) {
+        public FileArtifactValue getOutputMetadata(Artifact artifact) {
           throw new IllegalStateException();
         }
 

src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java

@@ -1855,12 +1855,8 @@ public FileArtifactValue getInputMetadata(ActionInput input) throws IOException
     }
 
     @Override
-    public FileArtifactValue getOutputMetadata(ActionInput input)
+    public FileArtifactValue getOutputMetadata(Artifact output)
         throws IOException, InterruptedException {
-      if (!(input instanceof Artifact output)) {
-        return null;
-      }
-
       if (output.isTreeArtifact()) {
         TreeArtifactValue treeArtifactValue = getTreeArtifactValue((SpecialArtifact) output);
         if (treeArtifactValue != null) {

src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java

@@ -1008,7 +1008,7 @@ public ImmutableList<RunfilesTree> getRunfilesTrees() {
     }
 
     @Override
-    public FileArtifactValue getOutputMetadata(ActionInput input)
+    public FileArtifactValue getOutputMetadata(Artifact artifact)
         throws IOException, InterruptedException {
       throw new UnsupportedOperationException();
     }

src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java

@@ -28,7 +28,6 @@
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -196,7 +195,7 @@ public Path getExecRoot() {
     }
 
     @Override
-    public ActionExecutionContext withOutputsAsInputs(Iterable<? extends ActionInput> inputs) {
+    public ActionExecutionContext withOutputsAsInputs(Iterable<Artifact> outputs) {
       return this;
     }