Address review comments

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java

@@ -71,6 +71,7 @@ protected ModuleExtensionContext(
         timeoutScaling,
         processWrapper,
         starlarkSemantics,
+        ModuleExtensionEvaluationProgress.moduleExtensionEvaluationContextString(extensionId),
         remoteExecutor,
         /* allowWatchingPathsOutsideWorkspace= */ false);
     this.extensionId = extensionId;
@@ -83,17 +84,12 @@ public Path getWorkingDirectory() {
   }
 
   @Override
-  protected boolean shouldDeleteWorkingDirectory(boolean successful) {
+  protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) {
     // The contents of the working directory are purely ephemeral, only the repos instantiated by
     // the extension are considered its results.
     return true;
   }
 
-  @Override
-  protected String getIdentifyingStringForLogging() {
-    return ModuleExtensionEvaluationProgress.moduleExtensionEvaluationContextString(extensionId);
-  }
-
   @Override
   protected boolean isRemotable() {
     // Maybe we can some day support remote execution, but not today.

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java

@@ -145,6 +145,7 @@ private interface AsyncTask {
   protected final double timeoutScaling;
   @Nullable private final ProcessWrapper processWrapper;
   protected final StarlarkSemantics starlarkSemantics;
+  protected final String identifyingStringForLogging;
   private final HashMap<RepoRecordedInput.File, String> recordedFileInputs = new HashMap<>();
   private final HashMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs = new HashMap<>();
   private final HashSet<String> accumulatedEnvKeys = new HashSet<>();
@@ -164,6 +165,7 @@ protected StarlarkBaseExternalContext(
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
       StarlarkSemantics starlarkSemantics,
+      String identifyingStringForLogging,
       @Nullable RepositoryRemoteExecutor remoteExecutor,
       boolean allowWatchingPathsOutsideWorkspace) {
     this.workingDirectory = workingDirectory;
@@ -175,13 +177,14 @@ protected StarlarkBaseExternalContext(
     this.timeoutScaling = timeoutScaling;
     this.processWrapper = processWrapper;
     this.starlarkSemantics = starlarkSemantics;
+    this.identifyingStringForLogging = identifyingStringForLogging;
     this.remoteExecutor = remoteExecutor;
     this.asyncTasks = new ArrayList<>();
     this.allowWatchingPathsOutsideWorkspace = allowWatchingPathsOutsideWorkspace;
     this.executorService =
         Executors.newThreadPerTaskExecutor(
             Thread.ofVirtual()
-                .name("downloads-" + workingDirectory.getBaseName())
+                .name("downloads[" + identifyingStringForLogging + "]-", 0)
                 .factory());
   }
 
@@ -196,22 +199,21 @@ public final void markSuccessful() {
   @Override
   public final void close() throws EvalException, IOException {
     // Cancel all pending async tasks.
-    boolean hadPendingItems = ensureNoPendingAsyncTasks();
+    boolean hadPendingItems = cancelPendingAsyncTasks();
     // Wait for all (cancelled) async tasks to complete before cleaning up the working directory.
     // This is necessary because downloads may still be in progress and could end up writing to the
     // working directory during deletion, which would cause an error.
     executorService.close();
-    if (shouldDeleteWorkingDirectory(wasSuccessful)) {
+    if (shouldDeleteWorkingDirectoryOnClose(wasSuccessful)) {
       workingDirectory.deleteTree();
     }
     if (hadPendingItems && wasSuccessful) {
       throw Starlark.errorf(
-          "Pending asynchronous work after %s finished execution",
-          getIdentifyingStringForLogging());
+          "Pending asynchronous work after %s finished execution", identifyingStringForLogging);
     }
   }
 
-  private boolean ensureNoPendingAsyncTasks() {
+  private boolean cancelPendingAsyncTasks() {
     boolean hadPendingItems = false;
     for (AsyncTask task : asyncTasks) {
       if (!task.cancel()) {
@@ -223,7 +225,7 @@ private boolean ensureNoPendingAsyncTasks() {
                       task.getLocation(),
                       String.format(
                           "Work pending after %s finished execution: %s",
-                          getIdentifyingStringForLogging(), task.getDescription())));
+                          identifyingStringForLogging, task.getDescription())));
         }
       }
     }
@@ -237,11 +239,8 @@ protected final void registerAsyncTask(AsyncTask task) {
     asyncTasks.add(task);
   }
 
-  /** A string that can be used to identify this context object. Used for logging purposes. */
-  protected abstract String getIdentifyingStringForLogging();
-
   @ForOverride
-  protected abstract boolean shouldDeleteWorkingDirectory(boolean successful);
+  protected abstract boolean shouldDeleteWorkingDirectoryOnClose(boolean successful);
 
   /** Returns the file digests used by this context object so far. */
   public ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs() {
@@ -455,7 +454,7 @@ private Optional<Checksum> validateChecksum(String sha256, String integrity, Lis
         warnAboutChecksumError(urls, e.getMessage());
         throw new RepositoryFunctionException(
             Starlark.errorf(
-                "Checksum error in %s: %s", getIdentifyingStringForLogging(), e.getMessage()),
+                "Checksum error in %s: %s", identifyingStringForLogging, e.getMessage()),
             Transience.PERSISTENT);
       }
     }
@@ -469,8 +468,7 @@ private Optional<Checksum> validateChecksum(String sha256, String integrity, Lis
     } catch (Checksum.InvalidChecksumException e) {
       warnAboutChecksumError(urls, e.getMessage());
       throw new RepositoryFunctionException(
-          Starlark.errorf(
-              "Checksum error in %s: %s", getIdentifyingStringForLogging(), e.getMessage()),
+          Starlark.errorf("Checksum error in %s: %s", identifyingStringForLogging, e.getMessage()),
           Transience.PERSISTENT);
     }
   }
@@ -728,7 +726,7 @@ public Object download(
             sha256,
             integrity,
             executable,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
 
@@ -759,7 +757,7 @@ public Object download(
               outputPath.getPath(),
               env.getListener(),
               envVariables,
-              getIdentifyingStringForLogging());
+              identifyingStringForLogging);
       download =
           new PendingDownload(
               executable,
@@ -933,7 +931,7 @@ public StructImpl downloadAndExtract(
             type,
             stripPrefix,
             renameFilesMap,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
 
     StarlarkPath outputPath = getPath("download_and_extract()", output);
@@ -961,7 +959,7 @@ public StructImpl downloadAndExtract(
               downloadDirectory,
               env.getListener(),
               envVariables,
-              getIdentifyingStringForLogging());
+              identifyingStringForLogging);
       // Ensure that the download is cancelled if the repo rule is restarted as it runs in its own
       // executor.
       PendingDownload pendingTask =
@@ -989,14 +987,14 @@ public StructImpl downloadAndExtract(
     }
     env.getListener().post(w);
     try (SilentCloseable c =
-        Profiler.instance().profile("extracting: " + getIdentifyingStringForLogging())) {
+        Profiler.instance().profile("extracting: " + identifyingStringForLogging)) {
       env.getListener()
           .post(
               new ExtractProgress(
                   outputPath.getPath().toString(), "Extracting " + downloadedPath.getBaseName()));
       DecompressorValue.decompress(
           DecompressorDescriptor.builder()
-              .setContext(getIdentifyingStringForLogging())
+              .setContext(identifyingStringForLogging)
               .setArchivePath(downloadedPath)
               .setDestinationPath(outputPath.getPath())
               .setPrefix(stripPrefix)
@@ -1103,7 +1101,7 @@ public void createFile(
             p.toString(),
             content,
             executable,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
     try {
@@ -1233,7 +1231,7 @@ public String readFile(Object path, String watch, StarlarkThread thread)
     StarlarkPath p = getPath("read()", path);
     WorkspaceRuleEvent w =
         WorkspaceRuleEvent.newReadEvent(
-            p.toString(), getIdentifyingStringForLogging(), thread.getCallerLocation());
+            p.toString(), identifyingStringForLogging, thread.getCallerLocation());
     env.getListener().post(w);
     maybeWatch(p, ShouldWatch.fromString(watch));
     if (p.isDir()) {
@@ -1410,7 +1408,7 @@ public void reportProgress(String status) {
             new FetchProgress() {
               @Override
               public String getResourceIdentifier() {
-                return getIdentifyingStringForLogging();
+                return identifyingStringForLogging;
               }
 
               @Override
@@ -1435,7 +1433,7 @@ public StarlarkOS getOS() {
     // manually inspect the code where this context object is used if they wish to find the
     // offending ctx.os expression.
     WorkspaceRuleEvent w =
-        WorkspaceRuleEvent.newOsEvent(getIdentifyingStringForLogging(), Location.BUILTIN);
+        WorkspaceRuleEvent.newOsEvent(identifyingStringForLogging, Location.BUILTIN);
     env.getListener().post(w);
     return osObject;
   }
@@ -1644,7 +1642,7 @@ public StarlarkExecutionResult execute(
             forceEnvVariables,
             workingDirectory.getPathString(),
             quiet,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
     createDirectory(workingDirectory);
@@ -1694,7 +1692,7 @@ public StarlarkExecutionResult execute(
   public StarlarkPath which(String program, StarlarkThread thread) throws EvalException {
     WorkspaceRuleEvent w =
         WorkspaceRuleEvent.newWhichEvent(
-            program, getIdentifyingStringForLogging(), thread.getCallerLocation());
+            program, identifyingStringForLogging, thread.getCallerLocation());
     env.getListener().post(w);
     if (program.contains("/") || program.contains("\\")) {
       throw Starlark.errorf(

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -77,7 +77,6 @@
             + " repository rule.")
 public class StarlarkRepositoryContext extends StarlarkBaseExternalContext {
   private final Rule rule;
-  private final RepositoryName repoName;
   private final PathPackageLocator packageLocator;
   private final StructImpl attrObject;
   private final ImmutableSet<PathFragment> ignoredPatterns;
@@ -112,10 +111,11 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext {
         timeoutScaling,
         processWrapper,
         starlarkSemantics,
+        RepositoryFetchProgress.repositoryFetchContextString(
+            RepositoryName.createUnvalidated(rule.getName())),
         remoteExecutor,
         /* allowWatchingPathsOutsideWorkspace= */ true);
     this.rule = rule;
-    this.repoName = RepositoryName.createUnvalidated(rule.getName());
     this.packageLocator = packageLocator;
     this.ignoredPatterns = ignoredPatterns;
     this.syscallCache = syscallCache;
@@ -132,15 +132,10 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext {
   }
 
   @Override
-  protected boolean shouldDeleteWorkingDirectory(boolean successful) {
+  protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) {
     return !successful;
   }
 
-  @Override
-  protected String getIdentifyingStringForLogging() {
-    return RepositoryFetchProgress.repositoryFetchContextString(repoName);
-  }
-
   public ImmutableMap<RepoRecordedInput.DirTree, String> getRecordedDirTreeInputs() {
     return ImmutableMap.copyOf(recordedDirTreeInputs);
   }
@@ -222,7 +217,7 @@ public void symlink(Object target, Object linkName, StarlarkThread thread)
         WorkspaceRuleEvent.newSymlinkEvent(
             targetPath.toString(),
             linkPath.toString(),
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
     try {
@@ -314,7 +309,7 @@ public void createFileFromTemplate(
             t.toString(),
             substitutionMap,
             executable,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
     if (t.isDir()) {
@@ -379,7 +374,7 @@ public boolean delete(Object pathObject, StarlarkThread thread)
     StarlarkPath starlarkPath = externalPath("delete()", pathObject);
     WorkspaceRuleEvent w =
         WorkspaceRuleEvent.newDeleteEvent(
-            starlarkPath.toString(), getIdentifyingStringForLogging(), thread.getCallerLocation());
+            starlarkPath.toString(), identifyingStringForLogging, thread.getCallerLocation());
     env.getListener().post(w);
     try {
       Path path = starlarkPath.getPath();
@@ -437,7 +432,7 @@ public void patch(Object patchFile, StarlarkInt stripI, String watchPatch, Starl
         WorkspaceRuleEvent.newPatchEvent(
             starlarkPath.toString(),
             strip,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
     if (starlarkPath.isDir()) {
@@ -548,7 +543,7 @@ public void extract(
             output.toString(),
             stripPrefix,
             renameFilesMap,
-            getIdentifyingStringForLogging(),
+            identifyingStringForLogging,
             thread.getCallerLocation());
     env.getListener().post(w);
 
@@ -558,7 +553,7 @@ public void extract(
                 outputPath.getPath().toString(), "Extracting " + archivePath.getBasename()));
     DecompressorValue.decompress(
         DecompressorDescriptor.builder()
-            .setContext(getIdentifyingStringForLogging())
+            .setContext(identifyingStringForLogging)
             .setArchivePath(archivePath.getPath())
             .setDestinationPath(outputPath.getPath())
             .setPrefix(stripPrefix)

src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java

@@ -781,7 +781,7 @@ public Path download(
       Map<String, String> clientEnv,
       String context)
       throws IOException, InterruptedException {
-    try (ExecutorService executorService = Executors.newSingleThreadExecutor()) {
+    try (ExecutorService executorService = Executors.newVirtualThreadPerTaskExecutor()) {
       Future<Path> future =
           downloadManager.startDownload(
               executorService,

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

@@ -1047,10 +1047,7 @@ def testPendingDownloadDetected(self):
     self.ScratchFile(
       'extensions.bzl',
       [
-        'def _rule_impl(ctx):',
-        '  ctx.file("REPO.bazel")',
-        '  ctx.file("BUILD")',
-        'repo_rule = repository_rule(_rule_impl)',
+        'repo_rule = repository_rule(lambda _: None)',
         'def ext_impl(module_ctx):',
         '  repo_rule(name = "ext")',
         '  module_ctx.download(url = "https://bcr.bazel.build", output = "download", block = False)',
@@ -1067,5 +1064,6 @@ def testPendingDownloadDetected(self):
       stderr,
     )
 
+
 if __name__ == '__main__':
   absltest.main()