Move the chmod call from SkyframeActionExecutor to ActionMetadataHandler

This relies on the explicit state transition in MetadataHandler to decide
whether the chmod call is necessary (we must never call chmod if the action
was not executed).

This is a prerequisite for #1525. If we want to use ctime for detecting file
content changes (which is more reliable than just mtime), then we must call
chmod before stat, since chmod affects ctime.

Before this change, we were caching the stat in ActionMetadataHandler, but
calling chmod after action execution in SkyframeActionExecutor, which is the
wrong order of calls. However, we must be able to stat in ActionMetadataHandler
for cases where a single action runs multiple Spawns where one spawn's output
is a subsequent spawn's input.

Remove MetadataHandler.isInjected, which is no longer used anywhere.
PiperOrigin-RevId: 152387133

4 files changed, 63 insertions, 93 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java b/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java
index 858edba762..7ed0e2e406 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java
@@ -17,6 +17,9 @@ import com.google.devtools.build.lib.vfs.FileStatus;
 
 /**
  * A FileStatus corresponding to a file that is not determined by querying the file system.
+ *
+ * <p>Do not use this in combination with MetadataHandler or FileContentsProxy! FileContentsProxy
+ * may use ctime, which this class does not support.
  */
 public class InjectedStat implements FileStatus {
 
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 aa26ed5a12..0874097ef7 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
@@ -24,6 +24,9 @@ import java.io.IOException;
  * Retrieves {@link Metadata} of {@link Artifact}s, and inserts virtual metadata as well. Some
  * methods on this interface may only be called after a call to {@link #discardOutputMetadata}.
  * Calling them before such a call results in an {@link IllegalStateException}.
+ *
+ * <p>Note that implementations of this interface call chmod on output files if
+ * {@link #discardOutputMetadata} has been called.
  */
 public interface MetadataHandler {
   /**
@@ -87,16 +90,6 @@ public interface MetadataHandler {
   boolean artifactOmitted(Artifact artifact);
 
   /**
-   * @return Whether the artifact's data was injected.
-   * @throws IOException if implementation tried to stat the Artifact which threw an exception.
-   *     Technically, this means that the artifact could not have been injected, but by throwing
-   *     here we save the caller trying to stat this file on their own and throwing the same
-   *     exception. Implementations are not guaranteed to throw in this case if they are able to
-   *     determine that the artifact is not injected without statting it.
-   */
-  boolean isInjected(Artifact file) throws IOException;
-
-  /**
    * Discards all known output artifact metadata, presumably because outputs will be modified. May
    * only be called before any metadata is injected using {@link #injectDigest} or {@link
    * #markOmitted};
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 d2e8097e40..99b4030e0f 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
@@ -49,10 +49,14 @@ import javax.annotation.Nullable;
 
 /**
  * Cache provided by an {@link ActionExecutionFunction}, allowing Blaze to obtain data from the
- * graph and to inject data (e.g. file digests) back into the graph.
+ * graph and to inject data (e.g. file digests) back into the graph. The cache can be in one of two
+ * modes. After construction it acts as a cache for input and output metadata for the purpose of
+ * checking for an action cache hit. When {@link #discardOutputMetadata} is called, it switches to
+ * a mode where it calls chmod on output files before statting them. This is done here to ensure
+ * that the chmod always comes before the stat in order to ensure that the stat is up to date.
  *
- * <p>Data for the action's inputs is injected into this cache on construction, using the graph as
- * the source of truth.
+ * <p>Data for the action's inputs is injected into this cache on construction, using the Skyframe
+ * graph as the source of truth.
  *
  * <p>As well, this cache collects data about the action's output files, which is used in three
  * ways. First, it is served as requested during action execution, primarily by the {@code
@@ -321,6 +325,13 @@ public class ActionMetadataHandler implements MetadataHandler {
       return value;
     }
 
+    if (executionMode.get()) {
+      // Preserve existing behavior: we don't set non-TreeArtifact directories
+      // read only and executable. However, it's unusual for non-TreeArtifact outputs
+      // to be directories.
+      setTreeReadOnlyAndExecutable(artifact, PathFragment.EMPTY_FRAGMENT);
+    }
+
     Set<TreeFileArtifact> registeredContents = outputDirectoryListings.get(artifact);
     if (registeredContents != null) {
       // Check that our registered outputs matches on-disk outputs. Only perform this check
@@ -441,6 +452,8 @@ public class ActionMetadataHandler implements MetadataHandler {
     // Assumption: any non-Artifact output is 'virtual' and should be ignored here.
     if (output instanceof Artifact) {
       final Artifact artifact = (Artifact) output;
+      // We have to add the artifact to injectedFiles before calling constructFileValue to avoid
+      // duplicate chmod calls.
       Preconditions.checkState(injectedFiles.add(artifact), artifact);
       FileValue fileValue;
       try {
@@ -522,12 +535,6 @@ public class ActionMetadataHandler implements MetadataHandler {
     return artifact.getPath().isFile();
   }
 
-  @Override
-  public boolean isInjected(Artifact file) {
-    Preconditions.checkState(executionMode.get());
-    return injectedFiles.contains(file);
-  }
-
   /** @return data for output files that was computed during execution. */
   Map<Artifact, FileValue> getOutputArtifactData() {
     return outputArtifactData;
@@ -560,10 +567,20 @@ public class ActionMetadataHandler implements MetadataHandler {
     return additionalOutputData;
   }
 
-  /** Constructs a new FileValue, saves it, and checks inconsistent data. */
+  /**
+   * Constructs a new FileValue, saves it, and checks inconsistent data. This calls chmod on the
+   * file if we're in executionMode.
+   */
   private FileValue constructFileValue(
       Artifact artifact, @Nullable FileStatusWithDigest statNoFollow)
           throws IOException {
+    // We first chmod the output files before we construct the FileContentsProxy. The proxy may use
+    // ctime, which is affected by chmod.
+    if (executionMode.get()) {
+      Preconditions.checkState(!artifact.isTreeArtifact());
+      setPathReadOnlyAndExecutable(artifact);
+    }
+
     FileValue value = fileValueFromArtifact(artifact, statNoFollow,
         getTimestampGranularityMonitor(artifact));
     FileValue oldFsValue = outputArtifactData.putIfAbsent(artifact, value);
@@ -611,4 +628,31 @@ public class ActionMetadataHandler implements MetadataHandler {
     }
     return FileValue.value(rootedPath, fileStateValue, realRootedPath, realFileStateValue);
   }
+
+  private void setPathReadOnlyAndExecutable(Artifact artifact) throws IOException {
+    // If the metadata was injected, we assume the mode is set correct and bail out early to avoid
+    // the additional overhead of resetting it.
+    if (injectedFiles.contains(artifact)) {
+      return;
+    }
+    Path path = artifact.getPath();
+    if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only.
+      // We trust the files created by the execution engine to be non symlinks with expected
+      // chmod() settings already applied.
+      path.chmod(0555);  // Sets the file read-only and executable.
+    }
+  }
+
+  private void setTreeReadOnlyAndExecutable(Artifact parent, PathFragment subpath)
+      throws IOException {
+    Path path = parent.getPath().getRelative(subpath);
+    if (path.isDirectory()) {
+      path.chmod(0555);
+      for (Path child : path.getDirectoryEntries()) {
+        setTreeReadOnlyAndExecutable(parent, subpath.getChild(child.getBaseName()));
+      }
+    } else {
+      setPathReadOnlyAndExecutable(ActionInputHelper.treeFileArtifact(parent, subpath));
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index c8436576f1..09072e3fb7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
 import com.google.devtools.build.lib.actions.ActionGraph;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
-import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
@@ -810,14 +809,6 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
           reportError("not all outputs were created or valid", null, action,
               outputAlreadyDumped ? null : fileOutErr);
         }
-        // Prevent accidental stomping on files.
-        // This will also throw a FileNotFoundException
-        // if any of the output files doesn't exist.
-        try {
-          setOutputsReadOnlyAndExecutable(action, metadataHandler);
-        } catch (IOException e) {
-          reportError("failed to set outputs read-only", e, action, null);
-        }
       } finally {
         profiler.completeTask(ProfilerTask.ACTION_COMPLETE);
       }
@@ -843,68 +834,6 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     }
   }
 
-  private static void setPathReadOnlyAndExecutable(MetadataHandler metadataHandler,
-      Artifact artifact)
-      throws IOException {
-    // If the metadata was injected, we assume the mode is set correct and bail out early to avoid
-    // the additional overhead of resetting it.
-    if (metadataHandler.isInjected(artifact)) {
-      return;
-    }
-    Path path = artifact.getPath();
-    if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only.
-      // We trust the files created by the execution-engine to be non symlinks with expected
-      // chmod() settings already applied.
-      path.chmod(0555);  // Sets the file read-only and executable.
-    }
-  }
-
-  private static void setTreeReadOnlyAndExecutable(MetadataHandler metadataHandler, Artifact parent,
-      PathFragment subpath) throws IOException {
-    Path path = parent.getPath().getRelative(subpath);
-    if (path.isDirectory()) {
-      path.chmod(0555);
-      for (Path child : path.getDirectoryEntries()) {
-        setTreeReadOnlyAndExecutable(metadataHandler, parent,
-            subpath.getChild(child.getBaseName()));
-      }
-    } else {
-      setPathReadOnlyAndExecutable(
-          metadataHandler, ActionInputHelper.treeFileArtifact(parent, subpath));
-    }
-  }
-
-  /**
-   * For each of the action's outputs that is a regular file (not a symbolic link or directory),
-   * make it read-only and executable.
-   *
-   * <p>Making the outputs read-only helps preventing accidental editing of them (e.g. in case of
-   * generated source code), while making them executable helps running generated files (such as
-   * generated shell scripts) on the command line.
-   *
-   * <p>May execute in a worker thread.
-   *
-   * <p>Note: setting these bits maintains transparency regarding the locality of the build; because
-   * the remote execution engine sets them, they should be set for local builds too.
-   *
-   * @throws IOException if an I/O error occurred.
-   */
-  private static void setOutputsReadOnlyAndExecutable(
-      Action action, MetadataHandler metadataHandler) throws IOException {
-    Preconditions.checkState(!action.getActionType().isMiddleman());
-
-    for (Artifact output : action.getOutputs()) {
-      if (output.isTreeArtifact()) {
-        // Preserve existing behavior: we don't set non-TreeArtifact directories
-        // read only and executable. However, it's unusual for non-TreeArtifact outputs
-        // to be directories.
-        setTreeReadOnlyAndExecutable(metadataHandler, output, PathFragment.EMPTY_FRAGMENT);
-      } else {
-        setPathReadOnlyAndExecutable(metadataHandler, output);
-      }
-    }
-  }
-
   private static void reportMissingOutputFile(
       Action action, Artifact output, Reporter reporter, boolean isSymlink, IOException exception) {
     boolean genrule = action.getMnemonic().equals("Genrule");
@@ -943,7 +872,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
   }
 
   /**
-   * Validates that all action outputs were created or intentionally omitted.
+   * Validates that all action outputs were created or intentionally omitted. This can result in
+   * chmod calls on the output files; see {@link ActionMetadataHandler}.
    *
    * @return false if some outputs are missing, true - otherwise.
    */
@@ -953,7 +883,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
       // artifactExists has the side effect of potentially adding the artifact to the cache,
       // therefore we only call it if we know the artifact is indeed not omitted to avoid any
       // unintended side effects.
-      if (!(metadataHandler.artifactOmitted(output))) {
+      if (!metadataHandler.artifactOmitted(output)) {
         try {
           metadataHandler.getMetadata(output);
         } catch (IOException e) {