[7.5.0] Do not invalidate remote metadata during action dirtiness check

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

@@ -522,12 +522,17 @@ public String toString() {
 
     @Override
     protected boolean couldBeModifiedByMetadata(FileArtifactValue o) {
-      if (!(o instanceof RegularFileArtifactValue)) {
-        return true;
+      switch (o) {
+        case RegularFileArtifactValue lastKnown -> {
+          return size != lastKnown.size || !Objects.equals(proxy, lastKnown.proxy);
+        }
+        case RemoteFileArtifactValueWithMaterializationData lastKnown -> {
+          return size != lastKnown.getSize() || !Objects.equals(proxy, lastKnown.proxy);
+        }
+        default -> {
+          return true;
+        }
       }
-
-      RegularFileArtifactValue lastKnown = (RegularFileArtifactValue) o;
-      return size != lastKnown.size || !Objects.equals(proxy, lastKnown.proxy);
     }
   }
 
@@ -536,36 +541,25 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
     private final long size;
     private final int locationIndex;
-    @Nullable private final PathFragment materializationExecPath;
 
-    private RemoteFileArtifactValue(
-        byte[] digest,
-        long size,
-        int locationIndex,
-        @Nullable PathFragment materializationExecPath) {
-      this.digest = Preconditions.checkNotNull(digest);
+    private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) {
+      this.digest = checkNotNull(digest);
       this.size = size;
       this.locationIndex = locationIndex;
-      this.materializationExecPath = materializationExecPath;
     }
 
-    public static RemoteFileArtifactValue create(
-        byte[] digest, long size, int locationIndex, long expireAtEpochMilli) {
-      return create(
-          digest, size, locationIndex, expireAtEpochMilli, /* materializationExecPath= */ null);
+    public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) {
+      return new RemoteFileArtifactValue(digest, size, locationIndex);
     }
 
-    @VisibleForTesting
-    public static RemoteFileArtifactValue create(
+    public static RemoteFileArtifactValueWithMaterializationData createWithMaterializationData(
         byte[] digest,
         long size,
         int locationIndex,
         long expireAtEpochMilli,
         @Nullable PathFragment materializationExecPath) {
-      return expireAtEpochMilli < 0
-          ? new RemoteFileArtifactValue(digest, size, locationIndex, materializationExecPath)
-          : new RemoteFileArtifactValueWithExpiration(
-              digest, size, locationIndex, materializationExecPath, expireAtEpochMilli);
+      return new RemoteFileArtifactValueWithMaterializationData(
+          digest, size, locationIndex, materializationExecPath, expireAtEpochMilli);
     }
 
     /**
@@ -575,10 +569,10 @@ public static RemoteFileArtifactValue create(
     public static RemoteFileArtifactValue createFromExistingWithMaterializationPath(
         RemoteFileArtifactValue metadata, PathFragment materializationExecPath) {
       checkNotNull(materializationExecPath);
-      if (metadata.materializationExecPath != null) {
+      if (metadata.getMaterializationExecPath().isPresent()) {
         return metadata;
       }
-      return create(
+      return createWithMaterializationData(
           metadata.getDigest(),
           metadata.getSize(),
           metadata.getLocationIndex(),
@@ -598,13 +592,12 @@ public boolean equals(Object o) {
       RemoteFileArtifactValue that = (RemoteFileArtifactValue) o;
       return Arrays.equals(digest, that.digest)
           && size == that.size
-          && locationIndex == that.locationIndex
-          && Objects.equals(materializationExecPath, that.materializationExecPath);
+          && locationIndex == that.locationIndex;
     }
 
     @Override
-    public final int hashCode() {
-      return Objects.hash(Arrays.hashCode(digest), size, locationIndex, materializationExecPath);
+    public int hashCode() {
+      return Objects.hash(Arrays.hashCode(digest), size, locationIndex);
     }
 
     @Override
@@ -618,7 +611,7 @@ public final byte[] getDigest() {
     }
 
     @Override
-    public final FileContentsProxy getContentsProxy() {
+    public FileContentsProxy getContentsProxy() {
       throw new UnsupportedOperationException();
     }
 
@@ -638,11 +631,6 @@ public final int getLocationIndex() {
       return locationIndex;
     }
 
-    @Override
-    public final Optional<PathFragment> getMaterializationExecPath() {
-      return Optional.ofNullable(materializationExecPath);
-    }
-
     /**
      * Returns the time when the remote file expires in milliseconds since epoch. A negative value
      * means the remote is not known to expire.
@@ -674,28 +662,33 @@ public final boolean isRemote() {
     }
 
     @Override
-    public final String toString() {
+    public String toString() {
       return MoreObjects.toStringHelper(this)
           .add("digest", bytesToString(digest))
           .add("size", size)
           .add("locationIndex", locationIndex)
-          .add("materializationExecPath", materializationExecPath)
-          .add("expireAtEpochMilli", getExpireAtEpochMilli())
           .toString();
     }
   }
 
-  /** A remote artifact that expires at a particular time. */
-  private static final class RemoteFileArtifactValueWithExpiration extends RemoteFileArtifactValue {
+  /**
+   * A remote artifact that contains additional data for materialization. This is used when the
+   * output mode allows Bazel to materialize remote output to local filesystem.
+   */
+  public static final class RemoteFileArtifactValueWithMaterializationData
+      extends RemoteFileArtifactValue {
+    @Nullable private final PathFragment materializationExecPath;
     private long expireAtEpochMilli;
+    @Nullable private FileContentsProxy proxy;
 
-    private RemoteFileArtifactValueWithExpiration(
+    private RemoteFileArtifactValueWithMaterializationData(
         byte[] digest,
         long size,
         int locationIndex,
         PathFragment materializationExecPath,
         long expireAtEpochMilli) {
-      super(digest, size, locationIndex, materializationExecPath);
+      super(digest, size, locationIndex);
+      this.materializationExecPath = materializationExecPath;
       this.expireAtEpochMilli = expireAtEpochMilli;
     }
 
@@ -706,14 +699,75 @@ public long getExpireAtEpochMilli() {
 
     @Override
     public void extendExpireAtEpochMilli(long expireAtEpochMilli) {
+      if (expireAtEpochMilli < 0) {
+        return;
+      }
       Preconditions.checkState(expireAtEpochMilli > this.expireAtEpochMilli);
       this.expireAtEpochMilli = expireAtEpochMilli;
     }
 
+    /**
+     * Returns a non-null {@link FileContentsProxy} if this remote metadata is backed by a local
+     * file, e.g. the file is materialized after action execution.
+     */
+    @Override
+    public FileContentsProxy getContentsProxy() {
+      return proxy;
+    }
+
+    /**
+     * Sets the {@link FileContentsProxy} if the output backed by this remote metadata is
+     * materialized later.
+     */
+    public void setContentsProxy(FileContentsProxy proxy) {
+      this.proxy = proxy;
+    }
+
     @Override
     public boolean isAlive(Instant now) {
+      if (expireAtEpochMilli < 0) {
+        return true;
+      }
       return now.toEpochMilli() < expireAtEpochMilli;
     }
+
+    @Override
+    public Optional<PathFragment> getMaterializationExecPath() {
+      return Optional.ofNullable(materializationExecPath);
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof RemoteFileArtifactValueWithMaterializationData that)) {
+        return false;
+      }
+
+      return Arrays.equals(getDigest(), that.getDigest())
+          && getSize() == that.getSize()
+          && getLocationIndex() == that.getLocationIndex()
+          && Objects.equals(materializationExecPath, that.materializationExecPath);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(
+          Arrays.hashCode(getDigest()), getSize(), getLocationIndex(), materializationExecPath);
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this)
+          .add("digest", bytesToString(getDigest()))
+          .add("size", getSize())
+          .add("locationIndex", getLocationIndex())
+          .add("materializationExecPath", materializationExecPath)
+          .add("expireAtEpochMilli", expireAtEpochMilli)
+          .add("proxy", proxy)
+          .toString();
+    }
   }
 
   /** A {@link FileArtifactValue} representing a symlink that is not to be resolved. */

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

@@ -514,7 +514,11 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
           PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
     }
 
-    return RemoteFileArtifactValue.create(
+    if (expireAtEpochMilli < 0 && materializationExecPath == null) {
+      return RemoteFileArtifactValue.create(digest, size, locationIndex);
+    }
+
+    return RemoteFileArtifactValue.createWithMaterializationData(
         digest, size, locationIndex, expireAtEpochMilli, materializationExecPath);
   }
 

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

@@ -40,6 +40,9 @@
 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.FileArtifactValue.RemoteFileArtifactValueWithMaterializationData;
+import com.google.devtools.build.lib.actions.FileContentsProxy;
+import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.cache.OutputMetadataStore;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.events.Reporter;
@@ -531,7 +534,8 @@ private Completable downloadFileNoCheckRx(
                         directExecutor())
                     .doOnComplete(
                         () -> {
-                          finalizeDownload(tempPath, finalPath, dirsWithOutputPermissions);
+                          finalizeDownload(
+                              metadata, tempPath, finalPath, dirsWithOutputPermissions);
                           alreadyDeleted.set(true);
                         })
                     .doOnError(
@@ -553,7 +557,8 @@ private Completable downloadFileNoCheckRx(
             }));
   }
 
-  private void finalizeDownload(Path tmpPath, Path finalPath, Set<Path> dirsWithOutputPermissions)
+  private void finalizeDownload(
+      FileArtifactValue metadata, Path tmpPath, Path finalPath, Set<Path> dirsWithOutputPermissions)
       throws IOException {
     Path parentDir = checkNotNull(finalPath.getParentDirectory());
 
@@ -585,6 +590,9 @@ private void finalizeDownload(Path tmpPath, Path finalPath, Set<Path> dirsWithOu
     // for artifacts produced by local actions.
     tmpPath.chmod(outputPermissions.getPermissionsMode());
     FileSystemUtils.moveFile(tmpPath, finalPath);
+    if (metadata instanceof RemoteFileArtifactValueWithMaterializationData remote) {
+      remote.setContentsProxy(FileContentsProxy.create(finalPath.stat()));
+    }
   }
 
   private interface TaskWithTempPath {

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

@@ -306,7 +306,12 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAt
       return;
     }
     var metadata =
-        RemoteFileArtifactValue.create(digest, size, /* locationIndex= */ 1, expireAtEpochMilli);
+        RemoteFileArtifactValue.createWithMaterializationData(
+            digest,
+            size,
+            /* locationIndex= */ 1,
+            expireAtEpochMilli,
+            /* materializationExecPath= */ null);
     remoteOutputTree.injectFile(path, metadata);
   }
 

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

@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static java.util.concurrent.TimeUnit.MINUTES;
 
 import com.google.common.base.Preconditions;
@@ -31,6 +32,7 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValueWithMaterializationData;
 import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.RemoteArtifactChecker;
 import com.google.devtools.build.lib.concurrent.ExecutorUtil;
@@ -476,21 +478,39 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
 
     // This could be improved by short-circuiting as soon as we see a child that is not present in
     // the TreeArtifactValue, but it doesn't seem to be a major source of overhead.
-    // visitTree() is called from multiple threads in parallel so this need to be a hash set
-    Set<PathFragment> currentChildren = Sets.newConcurrentHashSet();
+    // visitTree() is called from multiple threads in parallel so this need to be a concurrent set
+    Set<PathFragment> currentLocalChildren = Sets.newConcurrentHashSet();
     try {
       TreeArtifactValue.visitTree(
           path,
           (child, type, traversedSymlink) -> {
             if (type != Dirent.Type.DIRECTORY) {
-              currentChildren.add(child);
+              currentLocalChildren.add(child);
             }
           });
     } catch (IOException e) {
       return true;
     }
-    return !(currentChildren.isEmpty() && value.isEntirelyRemote())
-        && !currentChildren.equals(value.getChildPaths());
+
+    if (currentLocalChildren.isEmpty() && value.isEntirelyRemote()) {
+      return false;
+    }
+
+    var lastKnownLocalChildren =
+        value.getChildValues().entrySet().stream()
+            .filter(
+                entry -> {
+                  var metadata = entry.getValue();
+                  if (!(metadata
+                      instanceof RemoteFileArtifactValueWithMaterializationData remote)) {
+                    return true;
+                  }
+                  return remote.getContentsProxy() != null;
+                })
+            .map(entry -> entry.getKey().getParentRelativePath())
+            .collect(toImmutableSet());
+
+    return !currentLocalChildren.equals(lastKnownLocalChildren);
   }
 
   private boolean artifactIsDirtyWithDirectSystemCalls(

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

@@ -512,14 +512,14 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) {
   private RemoteFileArtifactValue createRemoteFileMetadata(
       String content, @Nullable PathFragment materializationExecPath) {
     byte[] bytes = content.getBytes(UTF_8);
-    return RemoteFileArtifactValue.create(
+    return RemoteFileArtifactValue.createWithMaterializationData(
         digest(bytes), bytes.length, 1, /* expireAtEpochMilli= */ -1, materializationExecPath);
   }
 
   private RemoteFileArtifactValue createRemoteFileMetadata(
       String content, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) {
     byte[] bytes = content.getBytes(UTF_8);
-    return RemoteFileArtifactValue.create(
+    return RemoteFileArtifactValue.createWithMaterializationData(
         digest(bytes), bytes.length, 1, expireAtEpochMilli, materializationExecPath);
   }
 

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

@@ -41,10 +41,7 @@
 public class CompletionContextTest {
   private static final FileArtifactValue DUMMY_METADATA =
       RemoteFileArtifactValue.create(
-          /* digest= */ new byte[0],
-          /* size= */ 0,
-          /* locationIndex= */ 0,
-          /* expireAtEpochMilli= */ -1);
+          /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 0);
 
   private Path execRoot;
   private ArtifactRoot outputRoot;