Force synchronous upload and reuse of possibly modified spawn outputs

[fmeum]

When an action may modify a spawn's outputs after execution, the upload of outputs to the cache and reuse for deduplicated actions need to happen synchronously directly after spawn execution to avoid a race.

This commit implements this for cache uploads by marking all actions with this property and simply disabling async upload for all spawns executed by such actions.

For output reuse, all executions deduplicated against the first one register atomically upon deduplication and cause the cache upload to wait for all of them to complete reuse.

Fixes #22501
Fixes #23288
Work towards #21578
Closes #23307 (no longer needed)

[fmeum]

FYI @aranguyen: We don't need to modify anything about how paths are rewritten in Java actions to benefit from execution deduplication with this change. We should still look into getting rid of the in-place modification for direct classpath header compilation to avoid the synchronous fallback introduced by this change.

[fmeum]

@tjgq I have no idea how to test this short of running a large build in CI. I tested it manually on Bazel itself, with promising results: I no longer get any failures and build times improve noticeably.

Representative build results for bazel build //src/main/java/... with an initially empty disk cache on an M3 MacBook:

# Baseline
INFO: Elapsed time: 102.879s, Critical Path: 24.86s
INFO: 5128 processes: 145 internal, 3076 darwin-sandbox, 1907 worker.

# --experimental_worker_multiplex_sandboxing
INFO: Elapsed time: 115.551s, Critical Path: 29.83s
INFO: 5128 processes: 145 internal, 3076 darwin-sandbox, 1907 worker.

# --experimental_worker_multiplex_sandboxing --experimental_output_paths=strip, with deduplication patched out
INFO: Elapsed time: 95.024s, Critical Path: 27.01s
INFO: 5128 processes: 1438 disk cache hit, 145 internal, 2375 darwin-sandbox, 1170 worker.

# --experimental_worker_multiplex_sandboxing --experimental_output_paths=strip
INFO: Elapsed time: 80.242s, Critical Path: 22.04s
INFO: 5128 processes: 1367 disk cache hit, 145 internal, 2171 darwin-sandbox, 459 deduplicated, 986 worker.

I expect the sandboxing overhead to be lower on Linux, but even with the overhead, path mapping is substantially faster.

[tjgq]

This is great! I think we should be in good shape to flip --experimental_remote_cache_async once it's in.

[tjgq]

I'm not going to insist too much because I know this might be a lot of effort, but: is there some way we can write a test to verify that a fast, async-unsafe spawn waits for a slow, concurrent one to reuse its outputs before proceeding?

[fmeum]

I'm not going to insist too much because I know this might be a lot of effort, but: is there some way we can write a test to verify that a fast, async-unsafe spawn waits for a slow, concurrent one to reuse its outputs before proceeding?

I added a Java test that relies on mocking to make waitForAndReuseOutputs controllably slow. I don't think it's guaranteed to catch the issue if it comes back, but I verified that removing the call to awaitAllOutputReuse makes it fail.

The problem is that we can't just make the other spawn slow (it doesn't run anyway), we need to make its output reuse slow. A more realistic test would be possible if we could somehow delay file system operations. Is that something that the test filesystem supports (say, stall updates to /exec/root/bazel-bin/k8-opt/bin/output on a synchronization barrier)?

[tjgq]

I'm not going to insist too much because I know this might be a lot of effort, but: is there some way we can write a test to verify that a fast, async-unsafe spawn waits for a slow, concurrent one to reuse its outputs before proceeding?

I added a Java test that relies on mocking to make waitForAndReuseOutputs controllably slow. I don't think it's guaranteed to catch the issue if it comes back, but I verified that removing the call to awaitAllOutputReuse makes it fail.

The problem is that we can't just make the other spawn slow (it doesn't run anyway), we need to make its output reuse slow. A more realistic test would be possible if we could somehow delay file system operations. Is that something that the test filesystem supports (say, stall updates to /exec/root/bazel-bin/k8-opt/bin/output on a synchronization barrier)?

It might be possible with a custom FileSystem implementation; see https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java;l=134;drc=3fe227ff418ef81eb5f221625bfd90f52ac70150 for inspiration.

[fmeum]

Thanks for the pointer, that looks pretty interesting. Since the main code path here goes through FileSystemUtils.copyFile and not a fixed method on Path, I think that the current approach may even be more robust though (I am looking into changing the implementation to use NIO).

I added an assert that should make it less likely that the test fails open.

[tjgq]

Can we instead use a latch to explicitly synchronize with the call to uploadOutputs (and avoid unnecessarily slowing down the test)?

[fmeum]

Done. Note that with the latch we can no longer assert that the thread would naturally wait (in the call to awaitAllOutputReuse), so I removed that assert.

[fmeum]

@bazel-io fork 7.4.0