Prevent broken cache entries on concurrent file changes

Local execution has an inherent race condition: if a user modifies a file while an action is executed, then it is impossible for Bazel to tell which version of the file was actually read during action execution. The file may have been modified before or after the tool has read it, or, in the worst case, the tool may have read both the original and the modified version. In addition, the file may be changed back to the original state before Bazel can check the file, so computing the digest before / after may not be sufficient.

This is a concern for both local and remote caches, although the cost of poisoning a shared remote cache is significantly higher, and is what has triggered this work.

Fixes #3360.

We solve this by keeping a reference to the FileContentsProxy, and using that to check for modificaitons before storing the cache entry. We output a warning if this check fails.

This change does not increase memory consumption; Java objects are always allocated in multiples of 8 bytes, we use compressed oops, and the FileArtifactValue currently has 12 bytes worth of fields (excl. object overhead), so adding another pointer is effectively free.

As a possible performance optimization on purely local builds, we could also consider not computing digests at all, and only use the FileContentsProxy for caching.

PiperOrigin-RevId: 182510358

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

@@ -68,24 +68,20 @@ public ActionInputMetadata load(ActionInput input) {
                           ? ((Artifact) input).getPath()
                           : execRoot.getRelative(input.getExecPath());
                   try {
-                    byte[] digest = path.getDigest();
+                    FileArtifactValue metadata = FileArtifactValue.create(path);
+                    if (metadata.getType().isDirectory()) {
+                      throw new DigestOfDirectoryException(
+                          "Input is a directory: " + input.getExecPathString());
+                    }
                     BaseEncoding hex = BaseEncoding.base16().lowerCase();
                     ByteString hexDigest =
-                        ByteString.copyFrom(hex.encode(digest).getBytes(US_ASCII));
+                        ByteString.copyFrom(hex.encode(metadata.getDigest()).getBytes(US_ASCII));
                     // Inject reverse mapping. Doing this unconditionally in getDigest() showed up
                     // as a hotspot in CPU profiling.
                     digestToPath.put(hexDigest, input);
-                    return new ActionInputMetadata(digest, path.getFileSize());
+                    return new ActionInputMetadata(metadata);
                   } catch (IOException e) {
-                    if (path.isDirectory()) {
-                      // TODO(bazel-team): This is rather presumptuous- it could have been another
-                      // type of IOException.
-                      return new ActionInputMetadata(
-                          new DigestOfDirectoryException(
-                              "Input is a directory: " + input.getExecPathString()));
-                    } else {
-                      return new ActionInputMetadata(e);
-                    }
+                    return new ActionInputMetadata(e);
                   }
                 }
               });
@@ -119,8 +115,8 @@ private static class ActionInputMetadata {
     private final IOException exceptionOnAccess;
 
     /** Constructor for a successful lookup. */
-    ActionInputMetadata(byte[] digest, long size) {
-      this.metadata = FileArtifactValue.createNormalFile(digest, size);
+    ActionInputMetadata(Metadata metadata) {
+      this.metadata = metadata;
       this.exceptionOnAccess = null;
     }
 

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

@@ -211,4 +211,16 @@ public final class RemoteOptions extends OptionsBase {
     help = "A file path to a local disk cache."
   )
   public PathFragment experimentalLocalDiskCachePath;
+
+  @Option(
+    name = "experimental_guard_against_concurrent_changes",
+    defaultValue = "true",
+    category = "remote",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help = "Turn this off to disable checking the ctime of input files of an action before "
+        + "uploading it to a remote cache. There may be cases where the Linux kernel delays "
+        + "writing of files, which could cause false positives."
+  )
+  public boolean experimentalGuardAgainstConcurrentChanges;
 }

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

@@ -20,13 +20,15 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.Spawns;
+import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.SpawnCache;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
 import com.google.devtools.build.lib.remote.DigestUtil.ActionKey;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+import com.google.devtools.build.lib.skyframe.FileArtifactValue;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.remoteexecution.v1test.Action;
@@ -157,6 +159,14 @@ public boolean willStore() {
         @Override
         public void store(SpawnResult result, Collection<Path> files)
             throws InterruptedException, IOException {
+          if (options.experimentalGuardAgainstConcurrentChanges) {
+            try {
+              checkForConcurrentModifications();
+            } catch (IOException e) {
+              report(Event.warn(e.getMessage()));
+              return;
+            }
+          }
           boolean uploadAction =
               Spawns.mayBeCached(spawn)
                   && Status.SUCCESS.equals(result.status())
@@ -179,6 +189,19 @@ public void store(SpawnResult result, Collection<Path> files)
 
         @Override
         public void close() {}
+
+        private void checkForConcurrentModifications() throws IOException {
+          for (ActionInput input : inputMap.values()) {
+            Metadata metadata = policy.getActionInputFileCache().getMetadata(input);
+            if (metadata instanceof FileArtifactValue) {
+              FileArtifactValue artifactValue = (FileArtifactValue) metadata;
+              Path path = execRoot.getRelative(input.getExecPath());
+              if (artifactValue.wasModifiedSinceDigest(path)) {
+                throw new IOException(path + " was modified during execution");
+              }
+            }
+          }
+        }
       };
     } else {
       return SpawnCache.NO_RESULT_NO_STORE;

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

@@ -268,7 +268,7 @@ private static FileArtifactValue createSimpleFileArtifactValue(
     // Directories are special-cased because their mtimes are used, so should have been constructed
     // during execution of the action (in ActionMetadataHandler#maybeStoreAdditionalData).
     Preconditions.checkState(data.isFile(), "Unexpected not file %s (%s)", artifact, data);
-    return FileArtifactValue.createNormalFile(data.getDigest(), data.getSize());
+    return FileArtifactValue.createNormalFile(data);
   }
 
   private static AggregatingArtifactValue createAggregatingValue(

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

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
 import java.util.Arrays;
@@ -67,6 +68,11 @@ public long getModifiedTime() {
       return 0;
     }
 
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) throws IOException {
+      return false;
+    }
+
     @Override
     public String toString() {
       return "singleton marker artifact value (" + hashCode() + ")";
@@ -94,6 +100,11 @@ public long getModifiedTime() {
       throw new UnsupportedOperationException();
     }
 
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) throws IOException {
+      return false;
+    }
+
     @Override
     public String toString() {
       return "OMITTED_FILE_MARKER";
@@ -139,6 +150,11 @@ public long getSize() {
       return 0;
     }
 
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) throws IOException {
+      return false;
+    }
+
     @Override
     public String toString() {
       return MoreObjects.toStringHelper(this).add("mtime", mtime).toString();
@@ -147,10 +163,12 @@ public String toString() {
 
   private static final class RegularFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
+    @Nullable private final FileContentsProxy proxy;
     private final long size;
 
-    private RegularFileArtifactValue(byte[] digest, long size) {
+    private RegularFileArtifactValue(byte[] digest, @Nullable FileContentsProxy proxy, long size) {
       this.digest = Preconditions.checkNotNull(digest);
+      this.proxy = proxy;
       this.size = size;
     }
 
@@ -169,6 +187,15 @@ public long getSize() {
       return size;
     }
 
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) throws IOException {
+      if (proxy == null) {
+        return false;
+      }
+      FileStatus stat = path.statIfFound(Symlinks.FOLLOW);
+      return stat == null || !stat.isFile() || !proxy.equals(FileContentsProxy.create(stat));
+    }
+
     @Override
     public long getModifiedTime() {
       throw new UnsupportedOperationException(
@@ -181,51 +208,77 @@ public String toString() {
     }
   }
 
-  @VisibleForTesting
-  public static FileArtifactValue create(Artifact artifact) throws IOException {
-    return create(artifact.getPath());
-  }
-
   static FileArtifactValue create(Artifact artifact, FileValue fileValue) throws IOException {
     boolean isFile = fileValue.isFile();
-    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0,
+    FileContentsProxy proxy = getProxyFromFileStateValue(fileValue.realFileStateValue());
+    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0, proxy,
         isFile ? fileValue.getDigest() : null);
   }
 
   static FileArtifactValue create(
       Artifact artifact, FileValue fileValue, @Nullable byte[] injectedDigest) throws IOException {
     boolean isFile = fileValue.isFile();
-    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0, injectedDigest);
+    FileContentsProxy proxy = getProxyFromFileStateValue(fileValue.realFileStateValue());
+    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0, proxy,
+        injectedDigest);
+  }
+
+  @VisibleForTesting
+  public static FileArtifactValue create(Artifact artifact) throws IOException {
+    return create(artifact.getPath());
   }
 
   @VisibleForTesting
-  static FileArtifactValue create(Path path) throws IOException {
-    FileStatus stat = path.stat();
-    boolean isFile = stat.isFile();
-    return create(path, isFile, isFile ? stat.getSize() : 0, null);
+  public static FileArtifactValue create(Path path) throws IOException {
+    // Caution: there's a race condition between stating the file and computing the
+    // digest. We need to stat first, since we're using the stat to detect changes.
+    // We follow symlinks here to be consistent with getDigest.
+    FileStatus stat = path.stat(Symlinks.FOLLOW);
+    return create(path, stat.isFile(), stat.getSize(), FileContentsProxy.create(stat), null);
   }
 
   private static FileArtifactValue create(
-      Path path, boolean isFile, long size, @Nullable byte[] digest) throws IOException {
-    if (isFile && digest == null) {
-      digest = DigestUtils.getDigestOrFail(path, size);
-    }
+      Path path, boolean isFile, long size, FileContentsProxy proxy, @Nullable byte[] digest)
+          throws IOException {
     if (!isFile) {
       // In this case, we need to store the mtime because the action cache uses mtime for
       // directories to determine if this artifact has changed. We want this code path to go away
       // somehow (maybe by implementing FileSet in Skyframe).
       return new DirectoryArtifactValue(path.getLastModifiedTime());
     }
+    if (digest == null) {
+      digest = DigestUtils.getDigestOrFail(path, size);
+    }
     Preconditions.checkState(digest != null, path);
-    return createNormalFile(digest, size);
+    return new RegularFileArtifactValue(digest, proxy, size);
   }
 
-  public static FileArtifactValue createNormalFile(byte[] digest, long size) {
-    return new RegularFileArtifactValue(digest, size);
+  public static FileArtifactValue createForVirtualActionInput(byte[] digest, long size) {
+    return new RegularFileArtifactValue(digest, /*proxy=*/ null, size);
+  }
+
+  public static FileArtifactValue createNormalFile(
+      byte[] digest, @Nullable FileContentsProxy proxy, long size) {
+    return new RegularFileArtifactValue(digest, proxy, size);
   }
 
   static FileArtifactValue createNormalFile(FileValue fileValue) {
-    return new RegularFileArtifactValue(fileValue.getDigest(), fileValue.getSize());
+    FileContentsProxy proxy = getProxyFromFileStateValue(fileValue.realFileStateValue());
+    return new RegularFileArtifactValue(fileValue.getDigest(), proxy, fileValue.getSize());
+  }
+
+  private static FileContentsProxy getProxyFromFileStateValue(FileStateValue value) {
+    if (value instanceof FileStateValue.RegularFileStateValue) {
+      return ((FileStateValue.RegularFileStateValue) value).getContentsProxy();
+    } else if (value instanceof FileStateValue.SpecialFileStateValue) {
+      return ((FileStateValue.SpecialFileStateValue) value).getContentsProxy();
+    }
+    return null;
+  }
+
+  @VisibleForTesting
+  public static FileArtifactValue createNormalFile(byte[] digest, long size) {
+    return createNormalFile(digest, /*proxy=*/null, size);
   }
 
   public static FileArtifactValue createDirectory(long mtime) {
@@ -238,7 +291,7 @@ public static FileArtifactValue createDirectory(long mtime) {
    */
   static FileArtifactValue createProxy(byte[] digest) {
     Preconditions.checkNotNull(digest);
-    return createNormalFile(digest, /*size=*/ 0);
+    return createNormalFile(digest, /*proxy=*/ null, /*size=*/ 0);
   }
 
   @Override
@@ -254,6 +307,15 @@ static FileArtifactValue createProxy(byte[] digest) {
   @Override
   public abstract long getModifiedTime();
 
+  /**
+   * Provides a best-effort determination whether the file was changed since the digest was
+   * computed. This method performs file system I/O, so may be expensive. It's primarily intended to
+   * avoid storing bad cache entries in an action cache. It should return true if there is a chance
+   * that the file was modified since the digest was computed. Better not upload if we are not sure
+   * that the cache entry is reliable.
+   */
+  public abstract boolean wasModifiedSinceDigest(Path path) throws IOException;
+
   @Override
   public boolean equals(Object o) {
     if (this == o) {

src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java

@@ -21,8 +21,11 @@
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.skyframe.FileArtifactValue;
+import com.google.devtools.build.lib.skyframe.FileContentsProxy;
+import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.remoteexecution.v1test.Digest;
 import com.google.devtools.remoteexecution.v1test.Tree;
 import com.google.protobuf.ByteString;
@@ -43,9 +46,10 @@ final class FakeActionInputFileCache implements ActionInputFileCache {
   @Override
   public Metadata getMetadata(ActionInput input) throws IOException {
     String hexDigest = Preconditions.checkNotNull(cas.get(input), input);
+    Path path = execRoot.getRelative(input.getExecPath());
+    FileStatus stat = path.stat(Symlinks.FOLLOW);
     return FileArtifactValue.createNormalFile(
-        HashCode.fromString(hexDigest).asBytes(),
-        execRoot.getRelative(input.getExecPath()).getFileSize());
+        HashCode.fromString(hexDigest).asBytes(), FileContentsProxy.create(stat), stat.getSize());
   }
 
   void setDigest(ActionInput input, String digest) {