Repository: bazelbuild/bazel
Issue: Broken AC entries are written to disk cache with --remote_download_minimal
Labels: type: bug
P1
team-Remote-Exec

### Description of the bug:


Building something with build without bytes using remote exec, running bazel clean and then rebuilding fails due to missing digests at least for some targets. Seems that the ac entry gets written to the disk cache while, in general, the cas entries referenced are not written due to build without bytes. This seems to have been the case for some time now but it wasn't a problem until check for the ac entry integrity was added in https://github.com/bazelbuild/bazel/pull/16731.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


Setup the workspace:
```
touch WORKSPACE
echo 'int main() { return 0; }' > main.cc
echo 'cc_binary(name = "main", srcs = ["main.cc"])' > BUILD
```
Also setup a config called remote_exec_with_disk which will enable remote execution and disk cache when building this project.

Repro issue
```
bazel build --config=remote_exec_with_disk --remote_download_minimal //:main && bazel clean && bazel build --config=remote_exec_with_disk --remote_download_minimal //:main
```
The second build command should give you an error like
```
ERROR: /repro_project/BUILD:1:10: Linking main failed: (Exit 34): Missing digest: 377e9d2c78fac1172750ae43806f3d3e1ec7dff71c15cdadfbbad10019929b8c/7936
```

### Which operating system are you running Bazel on?


linux

### What is the output of `bazel info release`?


release 6.0.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


-

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


```text
-
```


### Have you found anything relevant by searching the web?

-

### Any other information, logs, or outputs that you want to share?

-

Issue comments:
Forgot to add the place in code that seems to be the writing the broken entry to the disk
https://github.com/bazelbuild/bazel/blob/3748bcc8bf0c45e34295583fafaf1f52449114b8/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java#L202
@coeuvre @tjgq 
@bazel-io flag
Thanks for the report!

The issue is tricky. When building without the bytes with disk cache alone, we want to check ac entry integrity for disk cache. Otherwise the build could fail if the any blob is missing when we are reading it later.

However, when combined with remote cache, if we hit remote cache, we only download ac and skip downloading of blobs. Hence the AC in the disk cache is always incomplete until all the referenced blobs are downloaded later. The current code didn't check the integrity before deciding using the disk part of a combined cache.
https://github.com/bazelbuild/bazel/blob/3748bcc8bf0c45e34295583fafaf1f52449114b8/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java#L190

We could fix that by checking the integrity when deciding which part to use of a combined cache. The problem is we will never hit the disk cache until all the referenced blobs are downloaded. The fix could reduce the cache-hit rate for disk cache. Considering 2 consecutive builds:

```
bazel build --config=remote_exec_with_disk --remote_download_minimal ...
bazel build --config=remote_exec_with_disk --remote_download_minimal ...
```

The second build will miss the disk cache because blobs referenced by AC are not downloaded (but can still hit the remote cache). On the other hand, this should the correct behaviour. Since remote cache could evict blobs. We shouldn't accept AC from disk cache if any referenced blob is missing, hoping we could get it from remote cache -- it could also be missing there.

Another fix is drop the integrity check for AC from disk cache when using combined cache. This will increase the disk-cache hit rate, but the chance to trigger "missing blobs" error is higher if the TTL of blobs on remote cache is low.

I will go with the second fix for now (for performance reason) and see how can we improve it with lease service.
>Another fix is drop the integrity check for AC from disk cache when using combined cache. This will increase the disk-cache hit rate, but the chance to trigger "missing blobs" error is higher if the TTL of blobs on remote cache is low.

Can I vote that we _don't_ do this? At least until the lease service is up and running. The change to have the AC check if it's entries are valid is a very good one, in particular with remote caching. If you need to disable the check, and you do it only when disk cache + remote exec, but not disk cache + remote cache?
I agree AC check is a good one. But if we enable it for a combined cache (either disk cache + remote exec, or disk cache + remote cache), we will probably never hit the disk cache when building without the bytes.

Taking the repro as an example, since we are using minimal mode, the outputs of `//:main` will nerve be downloaded which means we will never hit the disk cache if we check the integrity.

In real world builds, I believe there are actions whose outputs will never be downloaded. If we do AC check, these actions will never hit the disk cache.
I'm fine with the AC result being used in the combined cache, as long as Bazel calls FindMissingBlobs on the remote cache to verify that the result is valid. I think it would be cheaper if Bazel instead just asked for the AC result from the remote cache in that case.

The main benefit of the combined disk cache is the storing of the CAS items. There might be some local-only actions that do end up storing themselves, even with BwtB. I feel that BwtB + disk cache only is going to be very rare. The most common case will be the combined cache. And if we want the BwtB experience to be as nice as possible, the combined cache shouldn't degrade the experience. By that, I mean that if one was using just the remote cache, and `bazel clean --expunge` was run (until the lease service lands), Bazel wouldn't run into missing blobs errors because the remote cache wouldn't return AC results if they had missing CAS blobs. This change would degrade that because the disk cache could incorrectly return an invalid AC result.
I get you. From this perspective, not throwing an error is definitely a better experience than slowing the build. And yes, just check remote AC is probably not slower. Reusing CAS is the main benefit.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java
index 1fc6200cd9e9d7..37ccd2cd4b456a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java
+++ b/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;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
index d0d6bd406d7e9c..55d6b43581f8dd 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
+++ b/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));


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
index 7cef85b3320396..f438d4dce01bc2 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -171,6 +171,9 @@ 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",
@@ -178,8 +181,10 @@ java_test(
         "//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",
     ],
 )
diff --git a/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java
index ee64e175c495dd..5c6bf046b48529 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java
+++ b/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 {