ActionMetadataHandler: proper metadata even for the volatile workspace status

This is part of cleaning up the ActionInputFileCache / MetadataHandler split.
Both classes generally store and return identical information, except the
MetadataHandler lies about constant metadata artifacts. Instead of lying here,
update the ActionCacheChecker, which is the only place where we actually want
constant metadata.

Additionally, remove getMetadataMaybe; again, the only caller that needs
special casing is the ActionCacheChecker, so we move the special casing there
instead of having it in the MetadataHandler.

PiperOrigin-RevId: 159665345

3 files changed, 43 insertions, 58 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index a7f0b82fa1..57abd35703 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -131,14 +131,15 @@ public class ActionCacheChecker {
    *
    * @return true if at least one artifact has changed, false - otherwise.
    */
-  private boolean validateArtifacts(Entry entry, Action action,
-      Iterable<Artifact> actionInputs, MetadataHandler metadataHandler, boolean checkOutput) {
+  private boolean validateArtifacts(
+      Entry entry, Action action, Iterable<Artifact> actionInputs, MetadataHandler metadataHandler,
+      boolean checkOutput) {
     Iterable<Artifact> artifacts = checkOutput
         ? Iterables.concat(action.getOutputs(), actionInputs)
         : actionInputs;
     Map<String, Metadata> mdMap = new HashMap<>();
     for (Artifact artifact : artifacts) {
-      mdMap.put(artifact.getExecPathString(), metadataHandler.getMetadataMaybe(artifact));
+      mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact));
     }
     return !DigestUtils.fromMetadata(mdMap).equals(entry.getFileDigest());
   }
@@ -290,6 +291,27 @@ public class ActionCacheChecker {
     return false; // cache hit
   }
 
+  private static Metadata getMetadataOrConstant(MetadataHandler metadataHandler, Artifact artifact)
+      throws IOException {
+    if (artifact.isConstantMetadata()) {
+      return Metadata.CONSTANT_METADATA;
+    } else {
+      return metadataHandler.getMetadata(artifact);
+    }
+  }
+
+  // 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 Metadata getMetadataMaybe(MetadataHandler metadataHandler, Artifact artifact) {
+    try {
+      return getMetadataOrConstant(metadataHandler, artifact);
+    } catch (IOException e) {
+      return null;
+    }
+  }
+
   public void afterExecution(
       Action action, Token token, MetadataHandler metadataHandler, Map<String, String> clientEnv)
       throws IOException {
@@ -313,14 +335,17 @@ public class ActionCacheChecker {
         actionCache.remove(execPath);
       }
       if (!metadataHandler.artifactOmitted(output)) {
-        // Output files *must* exist and be accessible after successful action execution.
-        Metadata metadata = metadataHandler.getMetadata(output);
+        // Output files *must* exist and be accessible after successful action execution. We use 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.
+        Metadata metadata = getMetadataOrConstant(metadataHandler, output);
         Preconditions.checkState(metadata != null);
         entry.addFile(output.getExecPath(), metadata);
       }
     }
     for (Artifact input : action.getInputs()) {
-      entry.addFile(input.getExecPath(), metadataHandler.getMetadataMaybe(input));
+      entry.addFile(input.getExecPath(), getMetadataMaybe(metadataHandler, input));
     }
     entry.getFileDigest();
     actionCache.put(key, entry);
@@ -392,18 +417,16 @@ public class ActionCacheChecker {
   }
 
   /**
-   * Special handling for the MiddlemanAction. Since MiddlemanAction output
-   * artifacts are purely fictional and used only to stay within dependency
-   * graph model limitations (action has to depend on artifacts, not on other
-   * actions), we do not need to validate metadata for the outputs - only for
-   * inputs. We also do not need to validate MiddlemanAction key, since action
-   * cache entry key already incorporates that information for the middlemen
-   * and we will experience a cache miss when it is different. Whenever it
-   * encounters middleman artifacts as input artifacts for other actions, it
-   * consults with the aggregated middleman digest computed here.
+   * Special handling for the MiddlemanAction. Since MiddlemanAction output artifacts are purely
+   * fictional and used only to stay within dependency graph model limitations (action has to depend
+   * on artifacts, not on other actions), we do not need to validate metadata for the outputs - only
+   * for inputs. We also do not need to validate MiddlemanAction key, since action cache entry key
+   * already incorporates that information for the middlemen and we will experience a cache miss
+   * when it is different. Whenever it encounters middleman artifacts as input artifacts for other
+   * actions, it consults with the aggregated middleman digest computed here.
    */
-  protected void checkMiddlemanAction(Action action, EventHandler handler,
-      MetadataHandler metadataHandler) {
+  protected void checkMiddlemanAction(
+      Action action, EventHandler handler, MetadataHandler metadataHandler) {
     if (!cacheConfig.enabled()) {
       // Action cache is disabled, don't generate digests.
       return;
@@ -430,7 +453,7 @@ public class ActionCacheChecker {
       // it in the cache entry and just use empty string instead.
       entry = new ActionCache.Entry("", ImmutableMap.<String, String>of(), false);
       for (Artifact input : action.getInputs()) {
-        entry.addFile(input.getExecPath(), metadataHandler.getMetadataMaybe(input));
+        entry.addFile(input.getExecPath(), getMetadataMaybe(metadataHandler, input));
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
index 0874097ef7..57df4e2961 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -16,7 +16,6 @@ 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.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.MiddlemanAction;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import java.io.IOException;
 
@@ -30,18 +29,6 @@ import java.io.IOException;
  */
 public interface MetadataHandler {
   /**
-   * Returns metadata for the given artifact or null if it does not exist or is intentionally
-   * omitted.
-   *
-   * <p>This should always be used for the inputs to {@link MiddlemanAction}s instead of {@link
-   * #getMetadata(Artifact)} since we may allow non-existent inputs to middlemen.
-   *
-   * @param artifact artifact
-   * @return metadata instance or null if metadata cannot be obtained.
-   */
-  Metadata getMetadataMaybe(Artifact artifact);
-
-  /**
    * Returns metadata for the given artifact or throws an exception if the metadata could not be
    * obtained.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 99b4030e0f..47cd18f278 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -135,15 +135,6 @@ public class ActionMetadataHandler implements MetadataHandler {
     this.tsgm = tsgm;
   }
 
-  @Override
-  public Metadata getMetadataMaybe(Artifact artifact) {
-    try {
-      return getMetadata(artifact);
-    } catch (IOException e) {
-      return null;
-    }
-  }
-
   /**
    * Gets the {@link TimestampGranularityMonitor} to use for a given artifact.
    *
@@ -168,12 +159,6 @@ public class ActionMetadataHandler implements MetadataHandler {
     return value.isFile() ? new Metadata(value.getDigest()) : new Metadata(value.getModifiedTime());
   }
 
-  @Override
-  public Metadata getMetadata(Artifact artifact) throws IOException {
-    Metadata metadata = getRealMetadata(artifact);
-    return artifact.isConstantMetadata() ? Metadata.CONSTANT_METADATA : metadata;
-  }
-
   @Nullable
   private FileArtifactValue getInputFileArtifactValue(Artifact input) {
     if (outputs.contains(input)) {
@@ -187,18 +172,8 @@ public class ActionMetadataHandler implements MetadataHandler {
     return Preconditions.checkNotNull(inputArtifactData.get(input), input);
   }
 
-  /**
-   * Get the real (viz. on-disk) metadata for an Artifact.
-   * A key assumption is that getRealMetadata() will be called for every Artifact in this
-   * ActionMetadataHandler, to populate additionalOutputData and outputTreeArtifactData.
-   *
-   * <p>We cache data for constant-metadata artifacts, even though it is technically unnecessary,
-   * because the data stored in this cache is consumed by various parts of Blaze via the {@link
-   * ActionExecutionValue} (for now, {@link FilesystemValueChecker} and {@link ArtifactFunction}).
-   * It is simpler for those parts if every output of the action is present in the cache. However,
-   * we must not return the actual metadata for a constant-metadata artifact.
-   */
-  private Metadata getRealMetadata(Artifact artifact) throws IOException {
+  @Override
+  public Metadata getMetadata(Artifact artifact) throws IOException {
     FileArtifactValue value = getInputFileArtifactValue(artifact);
     if (value != null) {
       return metadataFromValue(value);