Update GetActionResult for disk cache to check referenced files when …

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

@@ -44,11 +44,12 @@ public static RemoteCacheClient createDiskAndRemoteClient(
       Path workingDirectory,
       PathFragment diskCachePath,
       boolean remoteVerifyDownloads,
+      boolean checkActionResult,
       DigestUtil digestUtil,
       RemoteCacheClient remoteCacheClient)
       throws IOException {
-    DiskCacheClient diskCacheClient =
-        createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
+    DiskCacheClient diskCacheClient = createDiskCache(workingDirectory, diskCachePath,
+        remoteVerifyDownloads, checkActionResult, digestUtil);
     return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
   }
 
@@ -68,8 +69,8 @@ public static RemoteCacheClient create(
       return createHttp(options, creds, authAndTlsOptions, digestUtil);
     }
     if (isDiskCache(options)) {
-      return createDiskCache(
-          workingDirectory, options.diskCache, options.remoteVerifyDownloads, digestUtil);
+      return createDiskCache(workingDirectory, options.diskCache, options.remoteVerifyDownloads,
+          !options.remoteOutputsMode.downloadAllOutputs(), digestUtil);
     }
     throw new IllegalArgumentException(
         "Unrecognized RemoteOptions configuration: remote Http cache URL and/or local disk cache"
@@ -128,14 +129,15 @@ private static DiskCacheClient createDiskCache(
       Path workingDirectory,
       PathFragment diskCachePath,
       boolean verifyDownloads,
+      boolean checkActionResult,
       DigestUtil digestUtil)
       throws IOException {
     Path cacheDir =
         workingDirectory.getRelative(Preconditions.checkNotNull(diskCachePath, "diskCachePath"));
     if (!cacheDir.exists()) {
       cacheDir.createDirectoryAndParents();
     }
-    return new DiskCacheClient(cacheDir, verifyDownloads, digestUtil);
+    return new DiskCacheClient(cacheDir, verifyDownloads, checkActionResult, digestUtil);
   }
 
   private static RemoteCacheClient createDiskAndHttpCache(
@@ -153,8 +155,8 @@ private static RemoteCacheClient createDiskAndHttpCache(
     }
 
     RemoteCacheClient httpCache = createHttp(options, cred, authAndTlsOptions, digestUtil);
-    return createDiskAndRemoteClient(
-        workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
+    return createDiskAndRemoteClient(workingDirectory, diskCachePath, options.remoteVerifyDownloads,
+        !options.remoteOutputsMode.downloadAllOutputs(), digestUtil, httpCache);
   }
 
   public static boolean isDiskCache(RemoteOptions options) {

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

@@ -585,6 +585,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
                   env.getWorkingDirectory(),
                   remoteOptions.diskCache,
                   remoteOptions.remoteVerifyDownloads,
+                  !remoteOptions.remoteOutputsMode.downloadAllOutputs(),
                   digestUtil,
                   cacheClient);
         } catch (IOException e) {
@@ -644,6 +645,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
                   env.getWorkingDirectory(),
                   remoteOptions.diskCache,
                   remoteOptions.remoteVerifyDownloads,
+                  !remoteOptions.remoteOutputsMode.downloadAllOutputs(),
                   digestUtil,
                   cacheClient);
         } catch (IOException e) {

src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java

@@ -15,6 +15,8 @@
 
 import build.bazel.remote.execution.v2.ActionResult;
 import build.bazel.remote.execution.v2.Digest;
+import build.bazel.remote.execution.v2.Directory;
+import build.bazel.remote.execution.v2.Tree;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.ByteStreams;
 import com.google.common.util.concurrent.Futures;
@@ -43,20 +45,27 @@ public class DiskCacheClient implements RemoteCacheClient {
 
   private final Path root;
   private final boolean verifyDownloads;
+  private final boolean checkActionResult;
   private final DigestUtil digestUtil;
 
-  public DiskCacheClient(Path root, boolean verifyDownloads, DigestUtil digestUtil) {
+  public DiskCacheClient(Path root, boolean verifyDownloads, boolean checkActionResult,
+      DigestUtil digestUtil) {
     this.root = root;
     this.verifyDownloads = verifyDownloads;
+    this.checkActionResult = checkActionResult;
     this.digestUtil = digestUtil;
   }
 
-  /** Returns {@code true} if the provided {@code key} is stored in the CAS. */
+  /**
+   * Returns {@code true} if the provided {@code key} is stored in the CAS.
+   */
   public boolean contains(Digest digest) {
     return toPath(digest.getHash(), /* actionResult= */ false).exists();
   }
 
-  /** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */
+  /**
+   * Returns {@code true} if the provided {@code key} is stored in the Action Cache.
+   */
   public boolean containsActionResult(ActionKey actionKey) {
     return toPath(actionKey.getDigest().getHash(), /* actionResult= */ true).exists();
   }
@@ -102,13 +111,57 @@ public ListenableFuture<Void> downloadBlob(
         MoreExecutors.directExecutor());
   }
 
+  private void checkDigestExists(Digest digest) throws CacheNotFoundException {
+    if (digest.getSizeBytes() == 0) {
+      return;
+    }
+
+    if (!toPath(digest.getHash(), /* isActionCache= */ false).exists()) {
+      throw new CacheNotFoundException(digest);
+    }
+  }
+
+  private void checkOutputDirectory(Directory dir) throws CacheNotFoundException {
+    for (var file : dir.getFilesList()) {
+      checkDigestExists(file.getDigest());
+    }
+  }
+
+  private void checkActionResult(ActionResult actionResult) throws IOException {
+    for (var outputFile : actionResult.getOutputFilesList()) {
+      checkDigestExists(outputFile.getDigest());
+    }
+
+    for (var outputDirectory : actionResult.getOutputDirectoriesList()) {
+      var treeDigest = outputDirectory.getTreeDigest();
+      checkDigestExists(treeDigest);
+
+      var treePath = toPath(treeDigest.getHash(), /* isActionCache= */ false);
+      var tree = Tree.parseFrom(treePath.getInputStream());
+      checkOutputDirectory(tree.getRoot());
+      for (var dir : tree.getChildrenList()) {
+        checkOutputDirectory(dir);
+      }
+    }
+  }
+
   @Override
   public ListenableFuture<CachedActionResult> downloadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
-    return Futures.transform(
+    return Futures.transformAsync(
         Utils.downloadAsActionResult(
             actionKey, (digest, out) -> download(digest, out, /* isActionCache= */ true)),
-        CachedActionResult::disk,
+        actionResult -> {
+          if (actionResult == null) {
+            return Futures.immediateFuture(null);
+          }
+
+          if (checkActionResult) {
+            checkActionResult(actionResult);
+          }
+
+          return Futures.immediateFuture(CachedActionResult.disk(actionResult));
+        },
         MoreExecutors.directExecutor());
   }
 

src/test/java/com/google/devtools/build/lib/remote/BUILD

@@ -40,6 +40,7 @@ java_test(
         exclude = NATIVE_SSL_TEST + [
             "BuildWithoutTheBytesIntegrationTest.java",
             "BuildWithoutTheBytesIntegrationTestBase.java",
+            "DiskCacheIntegrationTest.java",
         ],
     ) + NATIVE_SSL_TEST_MAYBE,
     test_class = "com.google.devtools.build.lib.AllTests",
@@ -163,3 +164,26 @@ java_test(
         "//third_party:truth",
     ],
 )
+
+java_test(
+    name = "DiskCacheIntegrationTest",
+    srcs = ["DiskCacheIntegrationTest.java"],
+    runtime_deps = [
+        "//third_party/grpc-java:grpc-jar",
+    ],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib:runtime",
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/remote",
+        "//src/main/java/com/google/devtools/build/lib/standalone",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/test/java/com/google/devtools/build/lib/buildtool/util",
+        "//src/test/java/com/google/devtools/build/lib/remote/util:integration_test_utils",
+        "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
+        "//third_party:guava",
+        "//third_party:junit4",
+        "//third_party:truth",
+    ],
+)

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

@@ -0,0 +1,127 @@
+package com.google.devtools.build.lib.remote;
+
+import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
+import com.google.devtools.build.lib.runtime.BlazeModule;
+import com.google.devtools.build.lib.runtime.BlazeRuntime;
+import com.google.devtools.build.lib.runtime.BlockWaitingModule;
+import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule;
+import com.google.devtools.build.lib.standalone.StandaloneModule;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
+import org.junit.After;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {
+
+  private static PathFragment getDiskCacheDir() {
+    PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
+    return testTmpDir.getRelative("disk_cache");
+  }
+
+  @Override
+  protected void setupOptions() throws Exception {
+    super.setupOptions();
+
+    addOptions("--disk_cache=" + getDiskCacheDir().toString());
+  }
+
+  @After
+  public void tearDown() throws IOException {
+    getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();
+  }
+
+  @Override
+  protected ImmutableList<BlazeModule> getSpawnModules() {
+    return ImmutableList.<BlazeModule>builder()
+        .addAll(super.getSpawnModules())
+        .add(new StandaloneModule())
+        .build();
+  }
+
+  @Override
+  protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception {
+    return super.getRuntimeBuilder()
+        .addBlazeModule(new RemoteModule())
+        .addBlazeModule(new BuildSummaryStatsModule())
+        .addBlazeModule(new BlockWaitingModule());
+  }
+
+  @Test
+  public void hitDiskCache() throws Exception {
+    write("BUILD",
+        "genrule(",
+        "  name = 'foo',",
+        "  srcs = ['foo.in'],",
+        "  outs = ['foo.out'],",
+        "  cmd = 'cat $(SRCS) > $@',",
+        ")",
+        "genrule(",
+        "  name = 'foobar',",
+        "  srcs = [':foo.out', 'bar.in'],",
+        "  outs = ['foobar.out'],",
+        "  cmd = 'cat $(SRCS) > $@',",
+        ")"
+    );
+    write("foo.in", "foo");
+    write("bar.in", "bar");
+    buildTarget("//:foobar");
+    cleanAndRestartServer();
+
+    buildTarget("//:foobar");
+
+    events.assertContainsInfo("2 disk cache hit");
+  }
+
+  private void blobsReferencedInACAreMissingCASIgnoresAc(Runnable setupOptions) throws Exception {
+    // Arrange: Prepare the workspace and populate disk cache
+    write("BUILD",
+        "genrule(",
+        "  name = 'foo',",
+        "  srcs = ['foo.in'],",
+        "  outs = ['foo.out'],",
+        "  cmd = 'cat $(SRCS) > $@',",
+        ")",
+        "genrule(",
+        "  name = 'foobar',",
+        "  srcs = [':foo.out', 'bar.in'],",
+        "  outs = ['foobar.out'],",
+        "  cmd = 'cat $(SRCS) > $@',",
+        ")"
+    );
+    write("foo.in", "foo");
+    write("bar.in", "bar");
+    setupOptions.run();
+    buildTarget("//:foobar");
+    cleanAndRestartServer();
+
+    // Act: Delete blobs in CAS from disk cache and do a clean build
+    getWorkspace().getFileSystem().getPath(getDiskCacheDir().getRelative("cas")).deleteTree();
+    setupOptions.run();
+    buildTarget("//:foobar");
+
+    // Assert: Should ignore the stale AC and rerun the generating action
+    events.assertDoesNotContainEvent("disk cache hit");
+  }
+
+  @Test
+  public void blobsReferencedInACAreMissingCAS_ignoresAc() throws Exception {
+    blobsReferencedInACAreMissingCASIgnoresAc(() -> {});
+  }
+
+  @Test
+  public void bwob_blobsReferencedInACAreMissingCAS_ignoresAc() throws Exception {
+    blobsReferencedInACAreMissingCASIgnoresAc(() -> addOptions("--remote_download_minimal"));
+  }
+
+  private void cleanAndRestartServer() throws Exception {
+    getOutputBase().getRelative("action_cache").deleteTreesBelow();
+    // Simulates a server restart
+    createRuntimeWrapper();
+  }
+}

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java

@@ -22,6 +22,7 @@
 import build.bazel.remote.execution.v2.RequestMetadata;
 import build.bazel.remote.execution.v2.UpdateActionResultRequest;
 import com.google.common.flogger.GoogleLogger;
+import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient.CachedActionResult;
@@ -59,6 +60,8 @@ public void getActionResult(
 
       responseObserver.onNext(result.actionResult());
       responseObserver.onCompleted();
+    } catch (CacheNotFoundException e) {
+      responseObserver.onError(StatusUtils.notFoundError(request.getActionDigest()));
     } catch (Exception e) {
       logger.atWarning().withCause(e).log("getActionResult request failed");
       responseObserver.onError(StatusUtils.internalError(e));

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java

@@ -44,7 +44,8 @@ class OnDiskBlobStoreCache extends RemoteCache {
   public OnDiskBlobStoreCache(RemoteOptions options, Path cacheDir, DigestUtil digestUtil) {
     super(
         CAPABILITIES,
-        new DiskCacheClient(cacheDir, /* verifyDownloads= */ true, digestUtil),
+        new DiskCacheClient(cacheDir, /* verifyDownloads= */ true, /* checkActionResult= */ true,
+            digestUtil),
         options,
         digestUtil);
   }