Be resilient to outdated exec paths in action cache entries

[Wyverald]

60924fd changed the canonical repo name separator from ~ to +. Older repo names containing ~ now trigger a syntax error. If an action cache entry refers to an exec path from a previous version of Bazel that used ~, we need to be resilient and treat the cache entry as corrupted, rather than just crash.

Fixes #23180.

[Wyverald]

Tested with:

cd github/bazel
USE_BAZEL_VERSION=7.2.1 bazelisk build src:bazel  # succeeds
cp bazel-bin/src/bazel /tmp/bazel
USE_BAZEL_VERSION=last_green bazelisk build src:bazel  # crashes
/tmp/bazel build src:bazel  # succeeds

I'm still looking for a place to add tests. Any hints would be welcome :)

[Wyverald]

After digging around the code base a bit, it's still unclear where I should add tests for this.

The only thing I can think of is to have an integration test that builds with --noincompatible_use_plus_in_repo_names first, and then with --incompatible_use_plus_in_repo_names. But such a test could only work on release-7.3.0, since the flag is a no-op on master.

[justinhorvitz]

The issue is action cache sharing across bazel versions? Would it be easier to increment https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java;l=81;drc=f806a2ac39d1cf073207b8ecd5cb5cbcb8c4337b as a fix (#23224 is already going to do this)?

[Wyverald]

That would work, except that entries could be "poisoned" in the same version due to a flag flip. So unless we can "flip" the action cache version between two numbers (which is actually what we're doing with the MODULE.bazel.lock file version), this PR is still necessary.

[justinhorvitz]

I'm not very familiar with the callers of discoverFromExecPath. What happens after they ignore the empty Optional? Does the crashing build now succeed?

[Wyverald]

Does the crashing build now succeed?

Yes (see #23227 (comment)).

What happens after they ignore the empty Optional?

The caller of ActionExecutionFunction.PackageRootResolverWithEnvironment#findPackageRootsForFiles (this line) then tries to retrieve the package root for each unresolved path. So the sourceRoots.get(path) here will simply return null, which eventually leads to this comment which suggests that a null is fine.

[justinhorvitz]

Thanks, that explanation is very helpful.