Bzlmod lockfile does not track repository mappings of extension .bzl files

[fmeum]

Description of the bug:

When the .bzl files transitively loaded by a module extension don't change, but their repo mapping does, a module extension locked in the lockfile can return stale results.

Which category does this issue belong to?

External Dependency

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

# MODULE.bazel
bazel_dep(name = "rules_go", version = "0.40.0")

ext = use_extension("//:ext.bzl", "ext")
ext.tag(label_str = "@rules_go//go")
use_repo(ext, "repo")

other_ext = use_extension("//:other_ext.bzl", "other_ext")
use_repo(other_ext, "other_repo")

# BUILD
load("@repo//:defs.bzl", "LABEL")
load("@other_repo//:defs.bzl", "STR")
print(LABEL)
print(STR)

# ext.bzl
def _repo_impl(ctx):
    ctx.file("WORKSPACE")
    ctx.file("BUILD")
    ctx.file("defs.bzl", "LABEL = Label({})".format(repr(ctx.attr.label_str)))

_repo = repository_rule(
    implementation=_repo_impl,
    attrs={"label_str": attr.string(mandatory=True)},
)

_tag = tag_class(
    attrs={"label_str": attr.string(mandatory=True)},
)

def _ext_impl(ctx):
    for module in ctx.modules:
        for tag in module.tags.tag:
            _repo(name = "repo", label_str = tag.label_str)

ext = module_extension(
    implementation=_ext_impl,
    tag_classes={"tag": _tag},
)

# other_ext.bzl
load("@repo//:defs.bzl", "LABEL")

def _other_repo_impl(ctx):
    ctx.file("WORKSPACE")
    ctx.file("BUILD")
    ctx.file("defs.bzl", "STR = {}".format(repr(ctx.attr.str)))

_other_repo = repository_rule(
    implementation = _other_repo_impl,
    attrs = {
        "str": attr.string(),
    },
)

def _other_ext_impl(_):
    _other_repo(name = "other_repo", str = str(LABEL))

other_ext = module_extension(
    implementation = _other_ext_impl,
)

$ bazel build //...
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:3:6: @@rules_go~0.40.0//go:go
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:4:6: @@rules_go~0.40.0//go:go
$ buildozer 'set version 0.41.0' MODULE.bazel:rules_go
$ bazel build //...
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:3:6: @@rules_go~0.41.0//go:go
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:4:6: @@rules_go~0.40.0//go:go
$ bazel build //... --lockfile_mode=off
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:3:6: @@rules_go~0.41.0//go:go
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:4:6: @@rules_go~0.41.0//go:go
$ buildozer 'set version 0.40.0' MODULE.bazel:rules_go
$ bazel build //... --lockfile_mode=off
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:3:6: @@rules_go~0.40.0//go:go
DEBUG: /home/fhenneke/tmp/repo-mapping-lock/BUILD:4:6: @@rules_go~0.40.0//go:go

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

7.0.0 and HEAD

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 master; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

This is a regression in 7.0.0 since the lockfile is now enabled by default.

Have you found anything relevant by searching the web?

The reproducer is a synthetic, simplified version of the original problem (https://bazelbuild.slack.com/archives/CDBP88Z0D/p1703062323776219), which affects rules_go.

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

No response

[fmeum]

@bazel-io flag

[fmeum]

Both #20722 and #20721 can be fixed with this patch:

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index 08a3ee4e2d..af90e5f306 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -829,6 +829,10 @@ public class BzlLoadFunction implements SkyFunction {
     if (predeclared == null) {
       return null;
     }
+
+    // Also include the repository mapping as the evaluation of a `.bzl` file depends on it through
+    // the Label function.
+    fp.addStringMap(Maps.transformValues(repoMapping.entries(), RepositoryName::getName));
     byte[] transitiveDigest = fp.digestAndReset();
 
     // The BazelModuleContext holds additional contextual info to be associated with the Module,

This does cause the invalidation of all repos generated by extension in a Bazel module when that module is updated though, which can be costly. A more fine-grained alternative would be to track Label calls similar to ctx.path calls.

CC @Wyverald

[fmeum]

@bazel-io fork 7.0.1

[Wyverald]

A more fine-grained alternative would be to track Label calls similar to ctx.path calls.

I think we can do this. IIUC, we just need to store the mapping entries for anything used by a Label() call. Storing all repo mapping entries is as you said way too sensitive to change.

[iancha1992]

@bazel-io fork 7.1.0

[fmeum]

@Wyverald I found a special case that isn't covered by the fix:

  def testExtensionRepoMappingChangeTopLevel(self):
    # Regression test for #20721
    self.main_registry.createCcModule('foo', '1.0')
    self.main_registry.createCcModule('foo', '2.0')
    self.main_registry.createCcModule('bar', '1.0')
    self.main_registry.createCcModule('bar', '2.0')
    self.ScratchFile(
      'MODULE.bazel',
      [
        'bazel_dep(name="foo",version="1.0")',
        'bazel_dep(name="bar",version="1.0")',
        'ext = use_extension(":ext.bzl", "ext")',
        'use_repo(ext, "repo")',
      ],
    )
    self.ScratchFile(
      'BUILD.bazel',
      [
        'load("@repo//:defs.bzl", "STR")',
        'print("STR="+STR)',
        'filegroup(name="lol")',
      ],
    )
    self.ScratchFile(
      'constant.bzl',
      [
        'LABEL = Label("@foo//:lib_foo")',
      ],
    )
    self.ScratchFile(
      'ext.bzl',
      [
        'load(":constant.bzl", "LABEL")',
        'def _repo_impl(rctx):',
        '  rctx.file("BUILD")',
        '  rctx.file("defs.bzl", "STR = " + repr(str(rctx.attr.value)))',
        'repo = repository_rule(_repo_impl,attrs={"value":attr.label()})',
        'def _ext_impl(mctx):',
        '  print("ran the extension!")',
        '  repo(name = "repo", value = LABEL)',
        'ext = module_extension(_ext_impl)',
      ],
    )

    _, _, stderr = self.RunBazel(['build', ':lol'])
    self.assertIn('STR=@@foo~1.0//:lib_foo', '\n'.join(stderr))

    # Shutdown bazel to make sure we rely on the lockfile and not skyframe
    self.RunBazel(['shutdown'])
    _, _, stderr = self.RunBazel(['build', ':lol'])
    self.assertNotIn('ran the extension!', '\n'.join(stderr))

    # Shutdown bazel to make sure we rely on the lockfile and not skyframe
    self.RunBazel(['shutdown'])
    # Now, for something spicy: upgrade foo to 2.0 and change nothing else.
    # The extension should rerun despite the lockfile being present, and no
    # usages or .bzl files having changed.
    self.ScratchFile(
      'MODULE.bazel',
      [
        'bazel_dep(name="foo",version="2.0")',
        'bazel_dep(name="bar",version="1.0")',
        'ext = use_extension(":ext.bzl", "ext")',
        'use_repo(ext, "repo")',
      ],
    )
    _, _, stderr = self.RunBazel(['build', ':lol'])
    self.assertIn('STR=@@foo~2.0//:lib_foo', '\n'.join(stderr))

    # Shutdown bazel to make sure we rely on the lockfile and not skyframe
    self.RunBazel(['shutdown'])
    # More spicy! upgrade bar to 2.0 and change nothing else.
    # The extension should NOT rerun, since it never used the @bar repo mapping.
    self.ScratchFile(
      'MODULE.bazel',
      [
        'bazel_dep(name="foo",version="2.0")',
        'bazel_dep(name="bar",version="2.0")',
        'ext = use_extension(":ext.bzl", "ext")',
        'use_repo(ext, "repo")',
      ],
    )
    _, _, stderr = self.RunBazel(['build', ':lol'])
    stderr = '\n'.join(stderr)
    self.assertNotIn('ran the extension!', stderr)
    self.assertIn('STR=@@foo~2.0//:lib_foo', stderr)

[Wyverald]

Ah, thanks! We'll need to keep track of the used repo mappings from BzlLoadFunction too.

[Wyverald]

Hmm, this is actually slightly trickier than I thought. In particular, we also need to deal with repo-relative Label() calls.

Consider this scenario:

• @@//:foo.bzl loads from @bar//:bar.bzl, which initially maps to @@bar~1.0//:bar.bzl
• we upgrade bar from 1.0 to 2.0, and change nothing else
• @@//:foo.bzl now actually loads from @@bar~2.0//:bar.bzl
• if bar.bzl actually contains a MY_CONSTANT = Label("//:whatever"), its value will have changed by the upgrade, despite no bzl digest having changed, or any repo mapping from @@bar~1.0 or @@bar~2.0 having changed

To deal with this scenario, we could either consider load()s a "use" of a repo mapping and track it just like a Label() call, or we could store the "source repo" for every .bzl file as part of its digest. The latter approach is easier to implement but has a larger blast radius (affects BUILD-time loads as well), so I think I'll go with the first approach.

---

Beyond that, we also need to be careful while verifying the up-to-dateness of stored repo mapping entries. In the scenario above, if @@bar~1.0//:bar.bzl contained a Label("@quux//:whatever) call, then the entry <@@bar~1.0, quux, @@quux~1.0> would be stored. After upgrading bar to 2.0, though, trying to verify this entry could cause a crash as @@bar~1.0 doesn't even exist anymore, so requesting its repo mapping just causes an exception.

[iancha1992]

A fix for this issue has been included in Bazel 7.0.1 RC2. Please test out the release candidate and report any issues as soon as possible. Thanks!

[phst]

Is this fixed in all cases? I'm still seeing it with Bazel 7.0.2 (e.g. https://github.com/phst/rules_elisp/actions/runs/7767854632/job/21185125063 - this works with --lockfile_mode=off but not otherwise).

[fmeum]

@phst Could you try rerunning the build after deleting the lock file once?

[phst]

Yeah, that indeed worked, thanks!
I guess there's still some bug lurking somewhere then? Or is it expected that users have to regularly delete and recreate their lockfiles?

[fmeum]

That's not expected. We should have bumped the lockfile version, but didn't think of that in time for the release. We will bump it for 7.1.0.

Cc @Wyverald

[Wyverald]

Indeed -- this was an oversight on my part. Although frequent lockfile version bumps are also very disruptive, it's still better than hard-to-diagnose correctness issues.

[iancha1992]

A fix for this issue has been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!