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

2 files changed, 57 insertions, 10 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
index ccf282243f..7637abd3ba 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
@@ -19,6 +19,7 @@ import static org.junit.Assert.fail;
 
 import com.google.common.io.BaseEncoding;
 import com.google.common.testing.EqualsTester;
+import com.google.devtools.build.lib.testutil.ManualClock;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -30,11 +31,12 @@ import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
 public class FileArtifactValueTest {
-  private final FileSystem fs = new InMemoryFileSystem();
+  private final ManualClock clock = new ManualClock();
+  private final FileSystem fs = new InMemoryFileSystem(clock);
 
   private Path scratchFile(String name, long mtime, String content) throws IOException {
     Path path = fs.getPath(name);
-    FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
+    path.getParentDirectory().createDirectoryAndParents();
     FileSystemUtils.writeContentAsLatin1(path, content);
     path.setLastModifiedTime(mtime);
     return path;
@@ -42,7 +44,7 @@ public class FileArtifactValueTest {
 
   private Path scratchDir(String name, long mtime) throws IOException {
     Path path = fs.getPath(name);
-    FileSystemUtils.createDirectoryAndParents(path);
+    path.createDirectoryAndParents();
     path.setLastModifiedTime(mtime);
     return path;
   }
@@ -114,9 +116,7 @@ public class FileArtifactValueTest {
 
   @Test
   public void testDirectory() throws Exception {
-    Path path = fs.getPath("/dir");
-    FileSystemUtils.createDirectoryAndParents(path);
-    path.setLastModifiedTime(1L);
+    Path path = scratchDir("/dir", /*mtime=*/ 1L);
     FileArtifactValue value = create(path);
     assertThat(value.getDigest()).isNull();
     assertThat(value.getModifiedTime()).isEqualTo(1L);
@@ -153,7 +153,7 @@ public class FileArtifactValueTest {
       }
     };
     Path path = fs.getPath("/some/path");
-    FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
+    path.getParentDirectory().createDirectoryAndParents();
     FileSystemUtils.writeContentAsLatin1(path, "content");
     try {
       create(path);
@@ -162,4 +162,53 @@ public class FileArtifactValueTest {
       assertThat(e).isSameAs(exception);
     }
   }
+
+  @Test
+  public void testUptodateCheck() throws Exception {
+    Path path = scratchFile("/dir/artifact1", 0L, "content");
+    FileArtifactValue value = create(path);
+    clock.advanceMillis(1);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+    clock.advanceMillis(1);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+    clock.advanceMillis(1);
+    path.setLastModifiedTime(123); // Changing mtime implicitly updates ctime.
+    assertThat(value.wasModifiedSinceDigest(path)).isTrue();
+    clock.advanceMillis(1);
+    assertThat(value.wasModifiedSinceDigest(path)).isTrue();
+  }
+
+  @Test
+  public void testUptodateCheckDeleteFile() throws Exception {
+    Path path = scratchFile("/dir/artifact1", 0L, "content");
+    FileArtifactValue value = create(path);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+    path.delete();
+    assertThat(value.wasModifiedSinceDigest(path)).isTrue();
+  }
+
+  @Test
+  public void testUptodateCheckDirectory() throws Exception {
+    // For now, we don't attempt to detect changes to directories.
+    Path path = scratchDir("/dir", 0L);
+    FileArtifactValue value = create(path);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+    path.delete();
+    clock.advanceMillis(1);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+  }
+
+  @Test
+  public void testUptodateChangeFileToDirectory() throws Exception {
+    // For now, we don't attempt to detect changes to directories.
+    Path path = scratchFile("/dir/file", 0L, "");
+    FileArtifactValue value = create(path);
+    assertThat(value.wasModifiedSinceDigest(path)).isFalse();
+    // If we only check ctime, then we need to change the clock here, or we get a ctime match on the
+    // stat.
+    path.delete();
+    path.createDirectoryAndParents();
+    clock.advanceMillis(1);
+    assertThat(value.wasModifiedSinceDigest(path)).isTrue();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index 3c32a9df3d..4e3b242b5d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -102,9 +102,7 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase {
     // breaking changes.
     Map<String, Metadata> digestBuilder = new HashMap<>();
     for (PathFragment child : children) {
-      Metadata subdigest = FileArtifactValue.createNormalFile(
-          tree.getPath().getRelative(child).getDigest(),
-          tree.getPath().getRelative(child).getFileSize());
+      Metadata subdigest = FileArtifactValue.create(tree.getPath().getRelative(child));
       digestBuilder.put(child.getPathString(), subdigest);
     }
     assertThat(DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe())