Teach the FilesystemValueChecker about remote outputs

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

@@ -51,6 +51,8 @@
  *   <li>a "middleman marker" object, which has a null digest, 0 size, and mtime of 0.
  *   <li>The "self data" of a TreeArtifact, where we would expect to see a digest representing the
  *       artifact's contents, and a size of 0.
+ *   <li>a file that's only stored by a remote caching/execution system, in which case we would
+ *       expect to see a digest and size.
  * </ul>
  */
 @Immutable
@@ -138,6 +140,13 @@ public final boolean isMarkerValue() {
    */
   public abstract boolean wasModifiedSinceDigest(Path path) throws IOException;
 
+  /**
+   * Returns {@code true} if the file only exists remotely.
+   */
+  public boolean isRemote() {
+    return false;
+  }
+
   @Override
   public boolean equals(Object o) {
     if (this == o) {
@@ -524,7 +533,12 @@ public int getLocationIndex() {
 
     @Override
     public boolean wasModifiedSinceDigest(Path path) {
-      throw new UnsupportedOperationException();
+      return false;
+    }
+
+    @Override
+    public boolean isRemote() {
+      return true;
     }
 
     @Override

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

@@ -26,6 +26,7 @@
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.concurrent.ExecutorUtil;
 import com.google.devtools.build.lib.concurrent.Sharder;
 import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper;
@@ -80,6 +81,7 @@ public class FilesystemValueChecker {
   private static final Predicate<SkyKey> ACTION_FILTER =
       SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION);
 
+  @Nullable
   private final TimestampGranularityMonitor tsgm;
   @Nullable
   private final Range<Long> lastExecutionTimeRange;
@@ -398,6 +400,9 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
     try {
       Set<PathFragment> currentDirectoryValue =
           TreeArtifactValue.explodeDirectory(artifact.getPath());
+      if (currentDirectoryValue.isEmpty() && value.isRemote()) {
+        return false;
+      }
       Set<PathFragment> valuePaths = value.getChildPaths();
       return !currentDirectoryValue.equals(valuePaths);
     } catch (IOException e) {
@@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act
         try {
           ArtifactFileMetadata fileMetadata =
               ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
+          FileArtifactValue fileValue = actionValue.getArtifactValue(file);
+          boolean lastSeenRemotely = fileValue != null && fileValue.isRemote();
+          if (!fileMetadata.exists() && lastSeenRemotely) {
+            // The output file does not exist in the output tree, but the last time we created it
+            // it was stored remotely so there is no need to invalidate it.
+            continue;
+          }
+
           if (!fileMetadata.equals(lastKnownData)) {
             updateIntraBuildModifiedCounter(
                 fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1,

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

@@ -15,6 +15,7 @@
 
 import com.google.common.base.Function;
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -56,27 +57,40 @@ public PathFragment apply(Artifact artifact) {
   private final byte[] digest;
   private final Map<TreeFileArtifact, FileArtifactValue> childData;
   private BigInteger valueFingerprint;
+  private final boolean isRemote;
 
   @AutoCodec.VisibleForSerialization
-  TreeArtifactValue(byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData) {
+  TreeArtifactValue(byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData, boolean isRemote) {
     this.digest = digest;
     this.childData = ImmutableMap.copyOf(childData);
+    this.isRemote = isRemote;
   }
 
   /**
    * Returns a TreeArtifactValue out of the given Artifact-relative path fragments
    * and their corresponding FileArtifactValues.
+   *
+   * <p>All {@code childFileValues} must return the same value for
+   * {@link FileArtifactValue#isRemote()}.
    */
   static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childFileValues) {
     Map<String, FileArtifactValue> digestBuilder =
         Maps.newHashMapWithExpectedSize(childFileValues.size());
+    Boolean isRemote = null;
     for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
-      digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue());
+      FileArtifactValue v = e.getValue();
+      if (isRemote == null) {
+        isRemote = v.isRemote();
+      }
+      Preconditions.checkArgument(v.isRemote() == isRemote, "files in a tree artifact must either"
+          + " be all remote or all local.");
+      digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), v);
     }
 
     return new TreeArtifactValue(
         DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(),
-        ImmutableMap.copyOf(childFileValues));
+        ImmutableMap.copyOf(childFileValues),
+        isRemote != null ? isRemote : false);
   }
 
   FileArtifactValue getSelfData() {
@@ -104,6 +118,13 @@ Map<TreeFileArtifact, FileArtifactValue> getChildValues() {
     return childData;
   }
 
+  /**
+   * Returns {@code true} if the @link TreeFileArtifact}s are only stored remotely.
+   */
+  public boolean isRemote() {
+    return isRemote;
+  }
+
   @Override
   public BigInteger getValueFingerprint() {
     if (valueFingerprint == null) {
@@ -150,7 +171,7 @@ public String toString() {
    * Java's concurrent collections disallow null members.
    */
   static final TreeArtifactValue MISSING_TREE_ARTIFACT =
-      new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>of()) {
+      new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>of(), false) {
         @Override
         FileArtifactValue getSelfData() {
           throw new UnsupportedOperationException();

src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java

@@ -21,8 +21,10 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.hash.HashCode;
 import com.google.common.util.concurrent.Runnables;
 import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
@@ -32,8 +34,10 @@
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
 import com.google.devtools.build.lib.actions.FileStateValue;
 import com.google.devtools.build.lib.actions.FileValue;
+import com.google.devtools.build.lib.actions.cache.DigestUtils;
 import com.google.devtools.build.lib.actions.util.TestAction;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ServerDirectories;
@@ -50,6 +54,7 @@
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.BatchStat;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
@@ -74,9 +79,11 @@
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
+import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -787,6 +794,113 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List<TreeFileArtifact>
         /*actionDependsOnBuildId=*/ false);
   }
 
+  private ActionExecutionValue actionValueWithRemoteTreeArtifact(SpecialArtifact output, Map<PathFragment, RemoteFileArtifactValue> children) {
+    ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> childFileValues = ImmutableMap.builder();
+    for (Map.Entry<PathFragment, RemoteFileArtifactValue> child : children.entrySet()) {
+      childFileValues.put(ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue());
+    }
+    TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build());
+    return ActionExecutionValue.create(Collections.emptyMap(),
+        Collections.singletonMap(output, treeArtifactValue), ImmutableMap.of(), null, null, false);
+  }
+
+  private ActionExecutionValue actionValueWithRemoteArtifact(Artifact output, RemoteFileArtifactValue value) {
+    return ActionExecutionValue.create(
+        Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(),
+        Collections.singletonMap(output, value), null, null, false);
+  }
+
+  private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) {
+    byte[] data = contents.getBytes();
+    DigestHashFunction hashFn = fs.getDigestFunction();
+    HashCode hash = hashFn.getHashFunction().hashBytes(data);
+    return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1);
+  }
+
+  @Test
+  public void testRemoteAndLocalArtifacts() throws Exception {
+    // Test that injected remote artifacts are trusted by the FileSystemValueChecker
+    // and that local files always takes preference over remote files.
+    ActionLookupKey actionLookupKey =
+        new ActionLookupKey() {
+          @Override
+          public SkyFunctionName functionName() {
+            return SkyFunctionName.FOR_TESTING;
+          }
+        };
+    SkyKey actionKey1 = ActionExecutionValue.key(actionLookupKey, 0);
+    SkyKey actionKey2 = ActionExecutionValue.key(actionLookupKey, 1);
+
+    Artifact out1 = createDerivedArtifact("foo");
+    Artifact out2 = createDerivedArtifact("bar");
+    Map<SkyKey, SkyValue> metadataToInject = new HashMap<>();
+    metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content")));
+    metadataToInject.put(actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content")));
+    differencer.inject(metadataToInject);
+
+    EvaluationContext evaluationContext =
+        EvaluationContext.newBuilder()
+            .setKeepGoing(false)
+            .setNumThreads(1)
+            .setEventHander(NullEventHandler.INSTANCE)
+            .build();
+    assertThat(driver.evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext).hasError()).isFalse();
+    assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+        null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty();
+
+    // Create the "out1" artifact on the filesystem and test that it invalidates the generating
+    // action's SkyKey.
+    FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content");
+    assertThat(
+        new FilesystemValueChecker(null, null)
+            .getDirtyActionValues(
+                evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED))
+        .containsExactly(actionKey1);
+  }
+
+  @Test
+  public void testRemoteAndLocalTreeArtifacts() throws Exception {
+    // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker
+    // and that local files always takes preference over remote files.
+    ActionLookupKey actionLookupKey =
+        new ActionLookupKey() {
+          @Override
+          public SkyFunctionName functionName() {
+            return SkyFunctionName.FOR_TESTING;
+          }
+        };
+    SkyKey actionKey = ActionExecutionValue.key(actionLookupKey, 0);
+
+    SpecialArtifact treeArtifact = createTreeArtifact("dir");
+    treeArtifact.getPath().createDirectoryAndParents();
+    Map<PathFragment, RemoteFileArtifactValue> treeArtifactMetadata = new HashMap<>();
+    treeArtifactMetadata.put(PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content"));
+    treeArtifactMetadata.put(PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content"));
+
+    Map<SkyKey, SkyValue> metadataToInject = new HashMap<>();
+    metadataToInject.put(actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata));
+    differencer.inject(metadataToInject);
+
+    EvaluationContext evaluationContext =
+        EvaluationContext.newBuilder()
+            .setKeepGoing(false)
+            .setNumThreads(1)
+            .setEventHander(NullEventHandler.INSTANCE)
+            .build();
+    assertThat(driver.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()).isFalse();
+    assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+        null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty();
+
+    // Create dir/foo on the local disk and test that it invalidates the associated sky key.
+    TreeFileArtifact fooArtifact = treeFileArtifact(treeArtifact, "foo");
+    FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "new-foo-content");
+    assertThat(
+        new FilesystemValueChecker(null, null)
+            .getDirtyActionValues(
+                evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED))
+        .containsExactly(actionKey);
+  }
+
   @Test
   public void testPropagatesRuntimeExceptions() throws Exception {
     Collection<SkyKey> values =