Do the AC integrity check for disk part of the combined cache.

This will decrease the cache-hit rate for disk cache when building without the bytes. See bazelbuild#17080 for reasoning.

With lease service, the decrement will be fixed.

Fixes bazelbuild#17080.

Closes bazelbuild#17201.

PiperOrigin-RevId: 502818641
Change-Id: I1f73dd38d7e52e2f39b367e6114aab714de22d3c

Author: coeuvre

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

@@ -183,31 +183,53 @@ public ListenableFuture<Void> downloadBlob(
     }
   }
 
+  private ListenableFuture<CachedActionResult> downloadActionResultFromRemote(
+      RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
+    return Futures.transformAsync(
+        remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
+        (cachedActionResult) -> {
+          if (cachedActionResult == null) {
+            return immediateFuture(null);
+          } else {
+            return Futures.transform(
+                diskCache.uploadActionResult(context, actionKey, cachedActionResult.actionResult()),
+                v -> cachedActionResult,
+                directExecutor());
+          }
+        },
+        directExecutor());
+  }
+
   @Override
   public ListenableFuture<CachedActionResult> downloadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
-    if (context.getReadCachePolicy().allowDiskCache()
-        && diskCache.containsActionResult(actionKey)) {
-      return diskCache.downloadActionResult(context, actionKey, inlineOutErr);
+    ListenableFuture<CachedActionResult> future = immediateFuture(null);
+
+    if (context.getReadCachePolicy().allowDiskCache()) {
+      // If Build without the Bytes is enabled, the future will likely return null
+      // and fallback to remote cache because AC integrity check is enabled and referenced blobs are
+      // probably missing from disk cache due to BwoB.
+      //
+      // TODO(chiwang): With lease service, instead of doing the integrity check against local
+      // filesystem, we can check whether referenced blobs are alive in the lease service to
+      // increase the cache-hit rate for disk cache.
+      future = diskCache.downloadActionResult(context, actionKey, inlineOutErr);
     }
 
     if (context.getReadCachePolicy().allowRemoteCache()) {
-      return Futures.transformAsync(
-          remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
-          (cachedActionResult) -> {
-            if (cachedActionResult == null) {
-              return immediateFuture(null);
-            } else {
-              return Futures.transform(
-                  diskCache.uploadActionResult(
-                      context, actionKey, cachedActionResult.actionResult()),
-                  v -> cachedActionResult,
-                  directExecutor());
-            }
-          },
-          directExecutor());
-    } else {
-      return immediateFuture(null);
+      future =
+          Futures.transformAsync(
+              future,
+              (result) -> {
+                if (result == null) {
+                  return downloadActionResultFromRemote(context, actionKey, inlineOutErr);
+                } else {
+                  return immediateFuture(result);
+                }
+              },
+              directExecutor());
     }
+
+    return future;
   }
 }

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

@@ -68,11 +68,6 @@ 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. */
-  public boolean containsActionResult(ActionKey actionKey) {
-    return toPath(actionKey.getDigest().getHash(), /* actionResult= */ true).exists();
-  }
-
   public void captureFile(Path src, Digest digest, boolean isActionCache) throws IOException {
     Path target = toPath(digest.getHash(), isActionCache);
     target.getParentDirectory().createDirectoryAndParents();
@@ -161,7 +156,11 @@ public ListenableFuture<CachedActionResult> downloadActionResult(
           }
 
           if (checkActionResult) {
-            checkActionResult(actionResult);
+            try {
+              checkActionResult(actionResult);
+            } catch (CacheNotFoundException e) {
+              return Futures.immediateFuture(null);
+            }
           }
 
           return Futures.immediateFuture(CachedActionResult.disk(actionResult));

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

@@ -171,15 +171,20 @@ java_test(
 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/authandtls/credentialhelper:credential_module",
         "//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/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

@@ -13,11 +13,14 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
+import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule;
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
+import com.google.devtools.build.lib.remote.util.IntegrationTestUtils;
+import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.BlockWaitingModule;
@@ -32,6 +35,25 @@
 
 @RunWith(JUnit4.class)
 public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {
+  private WorkerInstance worker;
+
+  private void startWorker() throws Exception {
+    if (worker == null) {
+      worker = IntegrationTestUtils.startWorker();
+    }
+  }
+
+  private void enableRemoteExec(String... additionalOptions) {
+    assertThat(worker).isNotNull();
+    addOptions("--remote_executor=grpc://localhost:" + worker.getPort());
+    addOptions(additionalOptions);
+  }
+
+  private void enableRemoteCache(String... additionalOptions) {
+    assertThat(worker).isNotNull();
+    addOptions("--remote_cache=grpc://localhost:" + worker.getPort());
+    addOptions(additionalOptions);
+  }
 
   private static PathFragment getDiskCacheDir() {
     PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
@@ -48,6 +70,10 @@ protected void setupOptions() throws Exception {
   @After
   public void tearDown() throws IOException {
     getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();
+
+    if (worker != null) {
+      worker.stop();
+    }
   }
 
   @Override
@@ -93,7 +119,7 @@ public void hitDiskCache() throws Exception {
     events.assertContainsInfo("2 disk cache hit");
   }
 
-  private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOptions)
+  private void doBlobsReferencedInAcAreMissingFromCasIgnoresAc(String... additionalOptions)
       throws Exception {
     // Arrange: Prepare the workspace and populate disk cache
     write(
@@ -126,13 +152,72 @@ private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOption
   }
 
   @Test
-  public void blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
-    blobsReferencedInAAreMissingCasIgnoresAc();
+  public void blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
+    doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
+  }
+
+  @Test
+  public void bwob_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
+    doBlobsReferencedInAcAreMissingFromCasIgnoresAc("--remote_download_minimal");
   }
 
   @Test
-  public void bwob_blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
-    blobsReferencedInAAreMissingCasIgnoresAc("--remote_download_minimal");
+  public void bwobAndRemoteExec_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
+    startWorker();
+    enableRemoteExec("--remote_download_minimal");
+    doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
+  }
+
+  @Test
+  public void bwobAndRemoteCache_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
+    startWorker();
+    enableRemoteCache("--remote_download_minimal");
+    doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
+  }
+
+  private void doRemoteExecWithDiskCache(String... additionalOptions) throws Exception {
+    // Arrange: Prepare the workspace and populate disk cache
+    startWorker();
+    enableRemoteExec(additionalOptions);
+    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();
+
+    // Act: Do a clean build
+    enableRemoteExec("--remote_download_minimal");
+    buildTarget("//:foobar");
+  }
+
+  @Test
+  public void remoteExecWithDiskCache_hitDiskCache() throws Exception {
+    doRemoteExecWithDiskCache();
+
+    // Assert: Should hit the disk cache
+    events.assertContainsInfo("2 disk cache hit");
+  }
+
+  @Test
+  public void bwob_remoteExecWithDiskCache_hitRemoteCache() throws Exception {
+    doRemoteExecWithDiskCache("--remote_download_minimal");
+
+    // Assert: Should hit the remote cache because blobs referenced by the AC are missing from disk
+    // cache due to BwoB.
+    events.assertContainsInfo("2 remote cache hit");
   }
 
   private void cleanAndRestartServer() throws Exception {