Bazel uses out-of-date file contents when a bzl file is shared between BUILD and MODULE.bazel
- Dominant language
- Java
- Stars
- 25.8k
- Forks
- 4.6k
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 72
Description
### Description of the bug:
I have found a race condition that results in bazel using out-of-date file contents. This is only triggered under some tight timing conditions and when a bzl file is transitively loaded in both a BUILD file and the MODULE.bazel file. The race condition presents itself as bazel seeing incorrect file contents for the bzl file.
(caveat - I'm not an expert in Skyframe and this investigation was heavily assisted by claude, so please forgive any misuse of terminology)
This sequence of events are needed to trigger the race:
1. A bazel command is run, which starts warming the server.
2. The evaluation for `KeyForBuild` caches the parsed bzl file in `bzlCompileCache`
3. The evaluation for `KeyForBzlmod` starts and sees the bzl file in the `bzlCompileCache` and uses it. When this happens the `KeyForBzlmod` node does not get a dependency on the `FileValue` for the bzl file.
4. While the server is sitting warm without the correct SkyFrame dependency edges, a change is made to the source files in the repo that makes the old contents of the bzl file invalid (i.e we delete a target that it references, or delete another bzl that it `load`s)
5. Another bazel command is run and the Skyframe thread that evaluates the `KeyForBzlmod` doesn't get notified from the modified `FileValue` and uses a stale bzl value.
My proposed fix (pr incoming...) is to update `InliningAndCachingGetter.getBzlCompileValue` to still register a `FileValue` dep on the bzl file even if it gets a cache hit. This will ensure that regardless of if the cached value comes from the BUILD path or the MODULE.bazel path, there will not be a missing dependency edge.
https://github.com/bazelbuild/bazel/blob/c3a90dfded9504c0a496feabd9d050bc46be363c/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java#L1524-L1540
Alternative approaches:
1. Don't share the cache key across key types - i.e key the bzlCompileCache so that BUILD and MODULE path get separate keys. This eliminates the cross-key-type cache hit, but the both key types produce the same AST and should be able to share results
2. ??? - anything else?
I'm not familiar enough with how skyframe works and what should be considered when making a decision on this, so any help is appreciated!
### Which category does this issue belong to?
Core
### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
A reproduction repo can be found here - https://github.com/alex-torok/bazel-skyframe-race-condition-reproduction
The bug is pretty timing dependent and required injecting extra work into the MODULE.bazel resolution path so that the two skyframe threads aligned and experienced the race. It may be machine dependent, so if the reproduction doesn't work for you, some of the timing may need to be adjusted.
I have been able to reproduce this from 7.6 through main.
I'm guessing that some people may have gotten bitten by this from time to time, but the reproduction requirements are so convoluted, that it probably hasn't been a big problem. Something changed in my repo that introduced the dual-path scenario and the timing happened to be _just right_ for reliably reproducing this.
### Which operating system are you running Bazel on?
linux
### What is the output of `bazel info release`?
N/A - reproduces on all bazel versions
### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.
_No response_
### What's the output of `git remote get-url origin; git rev-parse HEAD` ?
```text
```
### If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.
I believe that this bug has existed since KeyForBzlmod was introduced in 348b53232e back in 2021.
### Have you found anything relevant by searching the web?
None
### Any other information, logs, or outputs that you want to share?
_No response_
Contributor guide
Research direction
Start in src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java, at InliningAndCachingGetter.getBzlCompileValue, and study the linked reproduction repository to understand the timing. Verify the BUILD/MODULE.bazel shared-cache scenario and ensure the resulting Skyframe dependency tracks bzl file changes so stale contents are not reused.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- build-system
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100