From 78db9ae9a545a9586dbb02d7831f5302594e01cb Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Tue, 17 Oct 2023 06:51:30 -0700
Subject: [PATCH] Fix unconditional Skyframe invalidation with
 `--lockfile_mode=update`

Ensure that `BazelLockFileModule` only updates `MODULE.bazel.lock` if the content of the file needs to change. Every such update changes the file's metadata, which results in Skyframe invalidation of, in particular, all configurations. This broke `bazel config`, which uses `MemoizingEvaluator#getDoneValues()` to directly observe Skyframe state.

Since this type of invalidation can also be caused by users and deviates from the usual "incremental correctness" guarantees provided by Bazel, let `config` show a descriptive error when no configurations are found.

Fixes #19823

Closes #19842.

PiperOrigin-RevId: 574133346
Change-Id: I5886c91fc6b7b938a7dee59ea75aa7b8afb5b161
---
 .../build/lib/bazel/bzlmod/BazelLockFileModule.java   |  9 +++++++--
 .../build/lib/runtime/commands/ConfigCommand.java     |  9 +++++++++
 src/test/shell/integration/configured_query_test.sh   | 11 ++++-------
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
index a664ae3437b5c2..4036eb3b994f42 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
@@ -106,8 +106,13 @@ public void afterCommand() throws AbruptExitException {
                 combineModuleExtensions(lockfile.getModuleExtensions(), oldExtensionUsages))
             .build();
 
-    // Write the new value to the file
-    updateLockfile(lockfilePath, lockfile);
+    // Write the new value to the file, but only if needed. This is not just a performance
+    // optimization: whenever the lockfile is updated, most Skyframe nodes will be marked as dirty
+    // on the next build, which breaks commands such as `bazel config` that rely on
+    // com.google.devtools.build.skyframe.MemoizingEvaluator#getDoneValues.
+    if (!lockfile.equals(oldLockfile)) {
+      updateLockfile(lockfilePath, lockfile);
+    }
     this.moduleResolutionEvent = null;
     this.extensionResolutionEventsMap.clear();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
index 53e43ca829fdd4..c0ddd7badb22bd 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java
@@ -332,6 +332,15 @@ public int hashCode() {
   public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
     ImmutableSortedMap<BuildConfigurationKey, BuildConfigurationValue> configurations =
         findConfigurations(env);
+    if (configurations.isEmpty()) {
+      String message =
+          "No configurations found. This can happen if the 'config' subcommand is used after "
+              + "files, including their metadata, have changed since the last invocation of "
+              + "another subcommand. Try running a 'build' or 'cquery' directly followed by "
+              + "'config'.";
+      env.getReporter().handle(Event.error(message));
+      return createFailureResult(message, Code.CONFIGURATION_NOT_FOUND);
+    }
 
     try (PrintWriter writer =
         new PrintWriter(
diff --git a/src/test/shell/integration/configured_query_test.sh b/src/test/shell/integration/configured_query_test.sh
index 8417dc11836c36..9a9cb52c26d8b5 100755
--- a/src/test/shell/integration/configured_query_test.sh
+++ b/src/test/shell/integration/configured_query_test.sh
@@ -60,10 +60,6 @@ fi
 
 add_to_bazelrc "build --package_path=%workspace%"
 
-# TODO: enable Bzlmod for this Test
-# https://github.com/bazelbuild/bazel/issues/19823
-disable_bzlmod
-
 #### TESTS #############################################################
 
 function test_basic_query() {
@@ -897,8 +893,8 @@ EOF
     --starlark:expr="str(target.label) + '%foo'" > output \
     2>"$TEST_log" || fail "Expected success"
 
-  assert_contains "^@//$pkg:pylib%foo$" output
-  assert_contains "^@//$pkg:pylibtwo%foo$" output
+  assert_contains "^@@\?//$pkg:pylib%foo$" output
+  assert_contains "^@@\?//$pkg:pylibtwo%foo$" output
 
   bazel cquery "//$pkg:all" --output=starlark \
     --noincompatible_unambiguous_label_stringification \
@@ -1294,7 +1290,7 @@ EOF
     2>"$TEST_log" || fail "Expected success"
   assert_contains "//$pkg:srcfile.txt:providers=.*FileProvider.*FilesToRunProvider.*LicensesProvider.*VisibilityProvider" \
     output
-  assert_contains "VisibilityProvider.label:@//$pkg:srcfile.txt" output
+  assert_contains "VisibilityProvider.label:@@\?//$pkg:srcfile.txt" output
 }
 
 function test_starlark_output_providers_starlark_provider() {
@@ -1394,6 +1390,7 @@ sh_library(name='japanese')
 EOF
 
   mkdir -p $dir/main
+  write_default_lockfile $dir/main/MODULE.bazel.lock
   cat > $dir/main/WORKSPACE <<EOF
 local_repository(name = "repo", path = "../repo")
 EOF