Broken AC entries are written to disk cache with --remote_download_minimal

[exoson]

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 #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 ?

-

Have you found anything relevant by searching the web?

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

[exoson]

Forgot to add the place in code that seems to be the writing the broken entry to the disk

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

Line 202 in 3748bcc

diskCache.uploadActionResult(

[brentleyjones]

@coeuvre @tjgq

[brentleyjones]

@bazel-io flag

[coeuvre]

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.

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

Line 190 in 3748bcc

&& diskCache.containsActionResult(actionKey)) {

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.

[brentleyjones]

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?

[coeuvre]

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.

[brentleyjones]

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.

[coeuvre]

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.