Avoid copying on disk cache hits if supported by the file system

[fmeum]

RELNOTES: Bazel no longer verifies the digests of disk cache entries upon a cache hit. This honors the description but not the previous behavior of the --remote_verify_downloads flag, which in fact controlled digest verification for both remote and disk caches.

In the process, LazyFileOutputStream#flush had to be changed to not open the stream if not open yet, otherwise it would override the existing contents.

Tested manually by running a fully cached build of //src:bazel and then verifying that du -kh bazel-bin/src/bazel prints 0B on macOS.

[gemini-code-assist]

Code Review

The pull request introduces the MaybePathBacked interface, which allows OutputStream implementations to indicate if they are backed by a file system Path. This enables an important optimization in DiskCacheClient to use FileSystemUtils.copyFile for direct file-to-file copies, potentially avoiding an in-memory buffer and improving efficiency. The changes are well-structured and utilize modern Java features like pattern matching for instanceof.

One area for improvement is ensuring consistent progress reporting when the optimized file copy path is taken, as the current implementation might bypass the DownloadProgressReporter for these operations.

[gemini-code-assist]

Code Review

This pull request introduces a significant performance optimization for disk cache hits by leveraging file system-level copy operations, avoiding redundant data transfer. This is enabled by a new MaybePathBacked interface to identify path-backed streams. A key part of this change is the intentional removal of digest verification for disk cache entries, which trades verification for speed. The implementation is generally clean, but I've identified a high-severity issue where the optimization inadvertently breaks download progress reporting. My review includes a detailed suggestion on how to fix this while keeping the performance benefits.

[john-flanagan]

I tested this PR rebased on to the 9.0.0 tag using https://github.com/john-flanagan/size_test and it looks great!

That test repo declares 50 iOS unit tests targets that all depend on the same framework containing a 2GB binary resource file.

Without this change hitting the disk cache results in a full file copy into each of the 50 targets and consumes ~100GB of disk space.

9.0.0

$ bazel clean --expunge
$ rm -rf ~/bazel_disk_cache_test
$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   549Gi     3%    453k  4.3G    0%   /

$ bazel build //...
INFO: Elapsed time: 128.151s, Critical Path: 54.48s

$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   546Gi     3%    453k  4.3G    0%   /

$ bazel clean
$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   547Gi     3%    453k  4.3G    0%   /

$ bazel build //...
INFO: Elapsed time: 88.520s, Critical Path: 28.19s

$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   449Gi     3%    453k  4.3G    0%   /

With this change hitting the disk cached is both much faster (25.8s vs 88.5s) and does not consume significant additional disk space.

Dev Build

$ bazel clean --expunge
$ rm -rf ~/bazel_disk_cache_test
$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   549Gi     3%    453k  4.3G    0%   /

$ ../bazel/bazel-bin/src/bazel-dev build //...
INFO: Elapsed time: 121.612s, Critical Path: 48.25s

$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   546Gi     3%    453k  4.3G    0%   /

$ ../bazel/bazel-bin/src/bazel-dev clean
$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   546Gi     3%    453k  4.3G    0%   /

$ ../bazel/bazel-bin/src/bazel-dev build //...
INFO: Elapsed time: 25.801s, Critical Path: 8.84s

$ df -h
Filesystem        Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk3s1s1   926Gi    11Gi   546Gi     3%    453k  4.3G    0%   /

[fmeum]

@bazel-io fork 9.1.0

[fmeum]

I will try to add a test for this that runs on macOS. Both Linux and Windows require special filesystem setups that would be difficult to prepare in CI.

[tjgq]

Can we drop the maybe? It seems that in practice all implementations are path-backed.

[fmeum]

ReportingOutputStream is only conditionally path backed (if the wrapped stream is), but we can't represent that on the type level. That's why I went with Maybe, but I can still drop the prefix if you think it's unnecessary.

[fmeum]

@tjgq I resolved the conflicts