[8.8.0] Clarify the invalidation of REPO_CONTENTS_CACHE_DIRS FileStateValues

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -2566,6 +2566,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:constraints/top_level_constraint_semantics",
         "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",
+        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception",
         "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/causes",

src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java

@@ -165,8 +165,10 @@ private static boolean isCacheableType(FileType fileType) {
           return true;
         case EXTERNAL_REPO:
         case OUTPUT:
-        case REPO_CONTENTS_CACHE_DIRS:
           return false;
+        case REPO_CONTENTS_CACHE_DIRS:
+          throw new IllegalStateException(
+              "Repo contents cache dirs are not expected to be checked for dirtiness");
       }
       throw new AssertionError("Unknown type " + fileType);
     }

src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java

@@ -180,14 +180,16 @@ public enum FileType {
     /**
      * The directory containing the repo contents cache entries as well as direct children
      * corresponding to individual predeclared input hashes. These directories are created by Bazel
-     * but may be deleted when users delete the entire repo contents cache.
+     * but may be deleted when users delete the entire repo contents cache. However, they are always
+     * recreated by Bazel before they are used and/or depended on via Skyframe. They are thus
+     * immutably present from the perspective of Skyframe and don't require invalidation.
      *
-     * <p>These files' handling differs from EXTERNAL_REPO as they are never modified after they are
-     * created and don't live under the external directory, as well as from EXTERNAL as they
-     * can be recreated by Bazel after diff detection.
-     *
-     * <p>The contents of these directories are considered EXTERNAL as they carry UUID names
-     * and are thus never reused.
+     * <p>Note: If these directories ever need to be checked for dirtiness during diffing, they have
+     * to be made non-cacheable according to {@link
+     * DirtinessCheckerUtils.ExternalDirtinessChecker#isCacheableType} so that they are not locked
+     * in as non-existent if they have been removed. This would result in FileValues for files below
+     * them (the actual repo contents, of type EXTERNAL) being locked in as non-existent too,
+     * even after a refetch of the repo has added a new cache entry.
      */
     REPO_CONTENTS_CACHE_DIRS,
   }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.actions.TopLevelOutputException;
 import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
+import com.google.devtools.build.lib.bazel.bzlmod.ExternalDepsException;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
 import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics.TargetCompatibilityCheckException;
 import com.google.devtools.build.lib.bugreport.BugReport;
@@ -540,6 +541,13 @@ && isExecutionCycle(errorInfo.getCycleInfo())) {
               configurationIdMessage(ctKey.getConfigurationKey()),
               ((NoSuchThingException) exception).getDetailedExitCode());
       analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause);
+    } else if (exception instanceof ExternalDepsException externalDepsException) {
+      AnalysisFailedCause analysisFailedCause =
+          new AnalysisFailedCause(
+              topLevelLabel,
+              configurationIdMessage(ctKey.getConfigurationKey()),
+              externalDepsException.getDetailedExitCode());
+      analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause);
     } else if (exception instanceof TargetCompatibilityCheckException) {
       analysisRootCauses = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     } else if (isExecutionException(exception)) {
@@ -779,7 +787,8 @@ private static DetailedException convertToAnalysisException(Throwable cause) {
     // analyze with --nokeep_going.
     if (cause instanceof SaneAnalysisException
         || cause instanceof NoSuchTargetException
-        || cause instanceof NoSuchPackageException) {
+        || cause instanceof NoSuchPackageException
+        || cause instanceof ExternalDepsException) {
       return (DetailedException) cause;
     }
     return null;

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -3470,6 +3470,8 @@ protected void handleDiffsWithMissingDiffInformation(
         fileTypesToCheck.add(FileType.OUTPUT);
       }
       if (!fileTypesToCheck.isEmpty()) {
+        // FileType.REPO_CONTENTS_CACHE_DIRS is intentionally never checked here. See the comment on
+        // that enum constant for details.
         dirtinessCheckers =
             Iterables.concat(
                 dirtinessCheckers,

src/test/py/bazel/bzlmod/bazel_module_test.py

@@ -1540,6 +1540,36 @@ def testUnknownRegistryInModuleMirrors(self):
     )
     self.assertIn('https://unknown-registry.example.com', stderr)
 
+  def testInvalidRepoRuleReferencedByTargetDoesNotCrash(self):
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'repo = use_repo_rule("//:repo.bzl", "repo")',
+            'repo(name = "my_repo")',
+        ],
+    )
+    self.ScratchFile(
+        'BUILD',
+        [
+            'genrule(',
+            '  name = "gen",',
+            '  srcs = ["@my_repo//:a.txt"],',
+            '  outs = ["out.txt"],',
+            '  cmd = "cat $(SRCS) > $(OUTS)",',
+            ')',
+        ],
+    )
+    self.ScratchFile('repo.bzl', ['nonsense'])
+
+    exit_code, _, stderr = self.RunBazel(
+        ['build', '//:gen'],
+        allow_failure=True,
+    )
+    self.AssertNotExitCode(exit_code, 0, stderr)
+    stderr = '\n'.join(stderr)
+    self.assertNotIn('FATAL', stderr)
+    self.assertIn("compilation of module 'repo.bzl' failed", stderr)
+
 
 if __name__ == '__main__':
   absltest.main()

src/test/py/bazel/bzlmod/repo_contents_cache_test.py

@@ -503,77 +503,93 @@ def doTestRepoContentsCacheDeleted(self, check_external_repository_files):
             'repo(name = "my_repo")',
         ],
     )
-    self.ScratchFile('workspace/BUILD.bazel')
+    self.ScratchFile(
+        'workspace/BUILD.bazel',
+        [
+            'genrule(',
+            '  name = "gen",',
+            '  srcs = ["@my_repo//:haha", "in.txt"],',
+            '  outs = ["out.txt"],',
+            '  cmd = "cat $(SRCS) > $(OUTS)",',
+            ')',
+        ],
+    )
     self.ScratchFile(
         'workspace/repo.bzl',
         [
             'def _repo_impl(rctx):',
-            '  rctx.file("BUILD", "filegroup(name=\'haha\')")',
+            (
+                '  rctx.file("BUILD", "filegroup(name=\'haha\','
+                " srcs=['a.txt'], visibility=['//visibility:public'])\")"
+            ),
+            '  rctx.file("a.txt", "hello world")',
             '  print("JUST FETCHED")',
             '  return rctx.repo_metadata(reproducible=True)',
             'repo = repository_rule(_repo_impl)',
         ],
     )
     # First fetch: not cached
+    self.ScratchFile('workspace/in.txt', ['1'])
     _, _, stderr = self.RunBazel(
         [
             'build',
-            '@my_repo//:haha',
+            '//:gen',
         ]
         + extra_args,
         cwd=workspace,
     )
     self.assertIn('JUST FETCHED', '\n'.join(stderr))
+    with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f:
+      self.assertEqual(f.read(), 'hello world1\n')
 
     # Second fetch: cached
+    self.ScratchFile('workspace/in.txt', ['2'])
     _, _, stderr = self.RunBazel(
         [
             'build',
-            '@my_repo//:haha',
+            '//:gen',
         ]
         + extra_args,
         cwd=workspace,
     )
     self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
+    with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f:
+      self.assertEqual(f.read(), 'hello world2\n')
 
     # Delete the entire repo contents cache and fetch again: not cached
     # Avoid access denied on Windows due to files being read-only by moving to
     # a different location instead.
     os.rename(repo_contents_cache, repo_contents_cache + '_deleted')
+    self.ScratchFile('workspace/in.txt', ['3'])
     _, _, stderr = self.RunBazel(
-        [
-            'build',
-            '@my_repo//:haha',
-        ]
-        + extra_args,
+        ['build', '//:gen'] + extra_args,
         cwd=workspace,
     )
     stderr = '\n'.join(stderr)
     self.assertIn('JUST FETCHED', stderr)
     self.assertNotIn('WARNING', stderr)
+    with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f:
+      self.assertEqual(f.read(), 'hello world3\n')
 
     # Second fetch after deletion: cached
+    self.ScratchFile('workspace/in.txt', ['4'])
     _, _, stderr = self.RunBazel(
-        [
-            'build',
-            '@my_repo//:haha',
-        ]
-        + extra_args,
+        ['build', '//:gen'] + extra_args,
         cwd=workspace,
     )
     self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
     self.assertNotIn('WARNING', '\n'.join(stderr))
+    with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f:
+      self.assertEqual(f.read(), 'hello world4\n')
 
     # Delete the entire repo contents cache and fetch again with a different
     # path: not cached
     # Avoid access denied on Windows due to files being read-only by moving to
     # a different location instead.
     os.rename(repo_contents_cache, repo_contents_cache + '_deleted_again')
+    self.ScratchFile('workspace/in.txt', ['5'])
     _, _, stderr = self.RunBazel(
-        [
-            'build',
-            '@my_repo//:haha',
-        ]
+        ['build', '//:gen']
         + extra_args
         + [
             '--repo_contents_cache=%s' % repo_contents_cache + '2',
@@ -583,6 +599,8 @@ def doTestRepoContentsCacheDeleted(self, check_external_repository_files):
     stderr = '\n'.join(stderr)
     self.assertIn('JUST FETCHED', stderr)
     self.assertNotIn('WARNING', stderr)
+    with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f:
+      self.assertEqual(f.read(), 'hello world5\n')
 
   def testRepoContentsCacheDeleted_withCheckExternalRepositoryFiles(self):
     self.doTestRepoContentsCacheDeleted(check_external_repository_files=True)