fix: address scholic-chorus review findings across agent, extension, and plugins

Applies 16 fixes surfaced by a multi-scholic review of the jb/configurations
branch. Each item below was verified against current HEAD before editing.

agent:
- Main.loadEmbeddedProbe: close the probe InputStream via try-with-resources
  and stop returning true from a stub that installs nothing; log an operator
  warn so the probes= argument no longer silently succeeds.
- Main.appendBootJar / appendSystemJar: synchronize the
  contains/appendTo/add block on the respective Set so two threads cannot
  both pass the dedup check and double-append the same jar.
- Main libs deprecation: emit at log.warn (was hidden behind isDebugEnabled).
- AgentManifestLibs.scanLibTree: close the Files.walk stream via
  try-with-resources (Files.walk is backed by a DirectoryStream).
- AgentManifestLibs.locateAgentPath: broaden the catch to include
  IllegalArgumentException and FileSystemNotFoundException (both thrown by
  Paths.get(URI) for non-file code sources), and drop the dead
  Files.isDirectory/else branch that returned the same value in both arms.
- AgentManifestLibs.filterAndNormalize: when toRealPath() fails, compare np
  against home.toAbsolutePath().normalize() instead of the raw home path so
  the fallback is at the same normalization level as np, and emit a warn so
  operators know symlink resolution was bypassed.
- RemoteClient.reconnect: publish this.disconnecting=false last so the
  volatile write acts as a release fence for the prior sock/ois/oos writes.

extension:
- EmbeddedExtensionRepository.readManifestIndex: trim each id after splitting
  the BTrace-Embedded-Extensions manifest value so a human-written
  "ext1, ext2" no longer produces a leading-space id that isValidExtensionId
  rejects.
- EmbeddedExtensionRepository.discoverBundledProbes: read the bundled probe
  list from the extension's properties file (probes=Probe1,Probe2) instead
  of always returning an empty list, so ExtensionDescriptorDTO actually
  carries the probes the extension declares.
- MethodHandleCache: add a negative cache so a repeated lookup for a missing
  method fails fast via the cached LookupRuntimeException instead of
  re-entering the reflective machinery on every call.

core:
- ProbeConfiguration.setOutput: use toLowerCase(Locale.ROOT) instead of
  default-locale toLowerCase() so Turkish dotted/dotless-I does not make
  "JFR" / "FILE" fall through to IllegalArgumentException.

maven plugin:
- FatAgentMojo.execute: wrap the staging-directory lifetime in try-finally
  so deleteDirectory runs on every exit path, not just the happy one.

gradle plugin:
- BTraceFatAgentExtension Maven/File extractFromZip: switch from
  project.buildDir (deprecated in Gradle 7, removed in Gradle 9) to
  project.layout.buildDirectory.dir(...).get().asFile.

housekeeping:
- Remove the accidentally-committed top-level compile.log and add it to
  .gitignore so it cannot be reintroduced.

Tests: full builds of btrace-core, btrace-extension, btrace-agent,
btrace-maven-plugin, and btrace-gradle-plugin are green, including
FatAgentMojoTest, AgentManifestLibsTest, SecurityValidationTest,
MainTest, and ExtensionBridgeImplPolicyTest.

The ClassDataLoader double-define race also surfaced in the review but the
chosen fix is still under discussion (see inline PR comment on
ClassDataLoader.findClass re: registerAsParallelCapable vs. synchronized
block on getClassLoadingLock) and is deliberately not included in this
commit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: jbachorik

.gitignore

@@ -41,3 +41,4 @@ gradle.properties
 gradle-wrapper.properties
 
 /.java.versions
+compile.log

btrace-agent/src/main/java/org/openjdk/btrace/agent/AgentManifestLibs.java

@@ -10,6 +10,7 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.nio.file.FileSystemNotFoundException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -122,12 +123,8 @@ private static Path locateAgentPath(Class<?> anchor) {
       URL url = anchor.getProtectionDomain().getCodeSource().getLocation();
       if (url == null) return null;
       URI uri = url.toURI();
-      Path p = Paths.get(uri);
-      if (Files.isDirectory(p)) {
-        return p; // exploded
-      }
-      return p;
-    } catch (URISyntaxException e) {
+      return Paths.get(uri);
+    } catch (URISyntaxException | IllegalArgumentException | FileSystemNotFoundException e) {
       if (log.isDebugEnabled()) log.debug("Failed to locate agent path: {}", e.toString());
       return null;
     }
@@ -168,10 +165,10 @@ static Path resolveEntry(String entry, Path baseDir) {
 
   // Package-private for testing
   static void scanLibTree(Path root, Set<Path> out) {
-    try {
-      if (root == null || !Files.exists(root)) return;
-      Files.walk(root)
-          .filter(f -> Files.isRegularFile(f))
+    if (root == null || !Files.exists(root)) return;
+    try (java.util.stream.Stream<Path> stream = Files.walk(root)) {
+      stream
+          .filter(Files::isRegularFile)
           .filter(f -> f.getFileName().toString().toLowerCase(Locale.ROOT).endsWith(".jar"))
           .forEach(out::add);
     } catch (IOException e) {
@@ -231,12 +228,21 @@ static List<Path> filterAndNormalize(Set<Path> entries, Path home, boolean allow
               continue;
             }
           } catch (IOException e) {
-            // toRealPath failed (file may not exist or path issue); fall back to non-canonical check
+            // toRealPath failed (file may not exist or path issue); fall back to a
+            // normalized non-canonical check. Use the same absolute+normalize pipeline as np
+            // so the comparison is apples-to-apples, and surface the weakened check so operators
+            // know symlink resolution could not be applied.
             if (log.isDebugEnabled()) log.debug("toRealPath failed for {}: {}", np, e.getMessage());
-            if (!np.startsWith(home)) {
-              log.warn("Rejecting manifest lib outside BTRACE_HOME: {}", np);
+            Path normalizedHome = home.toAbsolutePath().normalize();
+            if (!np.startsWith(normalizedHome)) {
+              log.warn(
+                  "Rejecting manifest lib outside BTRACE_HOME (symlink resolution failed, using normalized path): {}",
+                  np);
               continue;
             }
+            log.warn(
+                "Symlink resolution failed for manifest lib {}; accepted against normalized BTRACE_HOME only",
+                np);
           }
         }
         out.add(np);

btrace-agent/src/main/java/org/openjdk/btrace/agent/Main.java

@@ -443,37 +443,29 @@ private static void loadBundledProbes() {
 
   /**
    * Load an embedded probe from classpath resources.
+   *
+   * <p>Embedded probe activation is not yet wired through {@code BTraceProbeFactory} and the
+   * transformer pipeline, so this method currently verifies the resource exists and warns the
+   * operator that the {@code probes=} argument is a no-op. It returns {@code false} so the caller
+   * does not emit a misleading "Loaded bundled probe" info line.
    */
   private static boolean loadEmbeddedProbe(String resourcePath, String probeName, boolean traceToStdOut) {
-    try {
-      InputStream is = Main.class.getClassLoader().getResourceAsStream(resourcePath);
+    try (InputStream is = Main.class.getClassLoader().getResourceAsStream(resourcePath)) {
       if (is == null) {
         log.debug("Probe resource not found: {}", resourcePath);
         return false;
       }
-
       byte[] probeBytes = readAllBytes(is);
-      is.close();
-
-      // Create settings for this probe
-      SharedSettings probeSettings = new SharedSettings();
-      probeSettings.from(settings);
-      probeSettings.setClientName(probeName);
-      if (traceToStdOut) {
-        probeSettings.setOutputFile("::stdout");
-      }
-
-      ClientContext ctx = new ClientContext(inst, transformer, argMap, probeSettings);
-      // Load as bytes - similar to how Client handles compiled probes
-      // This is a simplified version; full implementation would use BTraceProbeFactory
-      log.debug("Loaded probe bytes for: {} ({} bytes)", probeName, probeBytes.length);
-
-      // For now, log that probe loading from embedded resources is available
-      // Full implementation would instantiate and activate the probe
-      return true;
-
-    } catch (Exception e) {
-      log.warn("Failed to load embedded probe {}: {}", probeName, e.getMessage());
+      log.warn(
+          "Embedded probe {} found at {} ({} bytes) but probe activation is not yet implemented; "
+              + "the 'probes=' agent argument is currently a no-op. Configure the probe via the "
+              + "client submit path instead.",
+          probeName,
+          resourcePath,
+          probeBytes.length);
+      return false;
+    } catch (IOException e) {
+      log.warn("Failed to read embedded probe {}: {}", probeName, e.getMessage());
       return false;
     }
   }
@@ -621,12 +613,10 @@ private static void parseArgs() {
 
     String libs = argMap.get(LIBS);
     if (libs != null && !libs.isEmpty()) {
-      if (log.isDebugEnabled()) {
-        log.debug(
-            "The 'libs' profile feature is deprecated and will be removed in a future release. "
-                + "Prefer packaging integrations as BTrace extensions (API on bootstrap, impl isolated). "
-                + "See docs/architecture/agent-manifest-libs.md for migration guidance.");
-      }
+      log.warn(
+          "The 'libs' profile feature is deprecated and will be removed in a future release. "
+              + "Prefer packaging integrations as BTrace extensions (API on bootstrap, impl isolated). "
+              + "See docs/architecture/agent-manifest-libs.md for migration guidance.");
     }
     String config = argMap.get(CONFIG);
     processClasspaths(libs);
@@ -995,16 +985,19 @@ private static JarFile asJarFile(File path) throws IOException {
   private static void appendBootJar(Path jarPath) {
     try {
       Path rp = jarPath.toAbsolutePath().normalize();
-      if (BOOT_ADDED.contains(rp)) {
-        if (log.isDebugEnabled()) log.debug("Skipping duplicate bootstrap jar: {}", rp);
-        return;
-      }
       if (!Files.exists(rp)) {
         if (log.isDebugEnabled()) log.debug("Bootstrap jar does not exist: {}", rp);
         return;
       }
-      inst.appendToBootstrapClassLoaderSearch(asJarFile(rp.toFile()));
-      BOOT_ADDED.add(rp);
+      // Synchronize the check-then-act so two threads cannot both pass the "not yet added"
+      // check and both call appendToBootstrapClassLoaderSearch for the same jar.
+      synchronized (BOOT_ADDED) {
+        if (!BOOT_ADDED.add(rp)) {
+          if (log.isDebugEnabled()) log.debug("Skipping duplicate bootstrap jar: {}", rp);
+          return;
+        }
+        inst.appendToBootstrapClassLoaderSearch(asJarFile(rp.toFile()));
+      }
       if (log.isDebugEnabled()) log.debug("Added to bootstrap: {}", rp);
     } catch (IOException e) {
       log.debug("Failed to append bootstrap jar {}: {}", jarPath, e.toString());
@@ -1014,16 +1007,17 @@ private static void appendBootJar(Path jarPath) {
   private static void appendSystemJar(Path jarPath) {
     try {
       Path rp = jarPath.toAbsolutePath().normalize();
-      if (SYSTEM_ADDED.contains(rp)) {
-        if (log.isDebugEnabled()) log.debug("Skipping duplicate system jar: {}", rp);
-        return;
-      }
       if (!Files.exists(rp)) {
         if (log.isDebugEnabled()) log.debug("System jar does not exist: {}", rp);
         return;
       }
-      inst.appendToSystemClassLoaderSearch(asJarFile(rp.toFile()));
-      SYSTEM_ADDED.add(rp);
+      synchronized (SYSTEM_ADDED) {
+        if (!SYSTEM_ADDED.add(rp)) {
+          if (log.isDebugEnabled()) log.debug("Skipping duplicate system jar: {}", rp);
+          return;
+        }
+        inst.appendToSystemClassLoaderSearch(asJarFile(rp.toFile()));
+      }
       if (log.isDebugEnabled()) log.debug("Added to system: {}", rp);
     } catch (IOException e) {
       log.debug("Failed to append system jar {}: {}", jarPath, e.toString());

btrace-agent/src/main/java/org/openjdk/btrace/agent/RemoteClient.java

@@ -391,10 +391,13 @@ protected void closeAll() throws IOException {
   }
 
   void reconnect(ObjectInputStream ois, ObjectOutputStream oos, Socket socket) throws IOException {
-    this.disconnecting = false;
     this.sock = socket;
     this.ois = ois;
     this.oos = oos;
+    // Publish last: the volatile write to `disconnecting` acts as a release fence so that any
+    // thread observing disconnecting==false via isDisconnected() is guaranteed to also see
+    // the updated socket triple above.
+    this.disconnecting = false;
     onCommand(Command.NULL);
   }
 }

btrace-core/src/main/java/org/openjdk/btrace/core/extensions/ProbeConfiguration.java

@@ -5,6 +5,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 
 /**
@@ -65,7 +66,7 @@ public ProbeConfiguration setOutput(String output) {
     if (output == null || output.isEmpty()) {
       return this;
     }
-    switch (output.toLowerCase()) {
+    switch (output.toLowerCase(Locale.ROOT)) {
       case "jfr":
         this.output = Output.JFR;
         break;

btrace-extension/src/main/java/org/openjdk/btrace/extension/impl/EmbeddedExtensionRepository.java

@@ -137,7 +137,14 @@ private List<String> readManifestIndex() {
           Attributes attrs = manifest.getMainAttributes();
           String embeddedExtensions = attrs.getValue(MANIFEST_ATTR);
           if (embeddedExtensions != null && !embeddedExtensions.trim().isEmpty()) {
-            return Arrays.asList(embeddedExtensions.split(","));
+            List<String> ids = new ArrayList<>();
+            for (String entry : embeddedExtensions.split(",")) {
+              String trimmed = entry.trim();
+              if (!trimmed.isEmpty()) {
+                ids.add(trimmed);
+              }
+            }
+            return ids;
           }
         }
       }
@@ -186,8 +193,8 @@ private ExtensionDescriptorDTO parseEmbeddedExtension(String extensionId) {
         configurator = null;
       }
 
-      // Discover bundled probes
-      List<String> bundledProbes = discoverBundledProbes(extensionId);
+      // Discover bundled probes (declared via the 'probes' property)
+      List<String> bundledProbes = discoverBundledProbes(extensionId, props);
 
       // For embedded extensions, jarPath points to a virtual path
       // The actual loading happens via ClassDataLoader
@@ -214,13 +221,23 @@ private ExtensionDescriptorDTO parseEmbeddedExtension(String extensionId) {
   }
 
   /**
-   * Discovers bundled probe class files in the extension's probes/ directory.
+   * Discovers bundled probe class names for an extension by reading the {@code probes}
+   * property from its {@code extension.properties}. The value is a comma-separated list
+   * of fully-qualified probe class names; entries are trimmed and validated.
    */
-  private List<String> discoverBundledProbes(String extensionId) {
-    // Note: Discovering resources without filesystem access is tricky.
-    // The probes list should be declared in extension.properties instead.
-    // For now, return empty and rely on extension.properties "probes" property.
-    return Collections.emptyList();
+  private List<String> discoverBundledProbes(String extensionId, Properties props) {
+    String probesStr = props.getProperty("probes", "");
+    if (probesStr.isEmpty()) {
+      return Collections.emptyList();
+    }
+    List<String> raw = new ArrayList<>();
+    for (String entry : probesStr.split(",")) {
+      String trimmed = entry.trim();
+      if (!trimmed.isEmpty()) {
+        raw.add(trimmed);
+      }
+    }
+    return validateClassNames(raw, "probe");
   }
 
   @Override

btrace-extension/src/main/java/org/openjdk/btrace/extension/util/MethodHandleCache.java

@@ -12,6 +12,7 @@
  */
 public final class MethodHandleCache {
   private final ConcurrentHashMap<Key, MethodHandle> cache = new ConcurrentHashMap<>();
+  private final ConcurrentHashMap<Key, LookupRuntimeException> negativeCache = new ConcurrentHashMap<>();
   private final MethodHandles.Lookup publicLookup = MethodHandles.publicLookup();
 
   public MethodHandleCache() {}
@@ -20,30 +21,44 @@ public MethodHandle findVirtual(Class<?> receiver, String name, Class<?> rtype,
       throws NoSuchMethodException, IllegalAccessException {
     MethodType mt = MethodType.methodType(rtype, ptypes);
     Key k = Key.of(receiver, name, mt, false);
-    return cache.computeIfAbsent(
-        k,
-        key -> {
-          try {
-            return publicLookup.findVirtual(receiver, name, mt);
-          } catch (NoSuchMethodException | IllegalAccessException e) {
-            throw new LookupRuntimeException(e);
-          }
-        });
+    LookupRuntimeException cachedFailure = negativeCache.get(k);
+    if (cachedFailure != null) throw cachedFailure;
+    try {
+      return cache.computeIfAbsent(
+          k,
+          key -> {
+            try {
+              return publicLookup.findVirtual(receiver, name, mt);
+            } catch (NoSuchMethodException | IllegalAccessException e) {
+              throw new LookupRuntimeException(e);
+            }
+          });
+    } catch (LookupRuntimeException e) {
+      negativeCache.putIfAbsent(k, e);
+      throw e;
+    }
   }
 
   public MethodHandle findStatic(Class<?> owner, String name, Class<?> rtype, Class<?>... ptypes)
       throws NoSuchMethodException, IllegalAccessException {
     MethodType mt = MethodType.methodType(rtype, ptypes);
     Key k = Key.of(owner, name, mt, true);
-    return cache.computeIfAbsent(
-        k,
-        key -> {
-          try {
-            return publicLookup.findStatic(owner, name, mt);
-          } catch (NoSuchMethodException | IllegalAccessException e) {
-            throw new LookupRuntimeException(e);
-          }
-        });
+    LookupRuntimeException cachedFailure = negativeCache.get(k);
+    if (cachedFailure != null) throw cachedFailure;
+    try {
+      return cache.computeIfAbsent(
+          k,
+          key -> {
+            try {
+              return publicLookup.findStatic(owner, name, mt);
+            } catch (NoSuchMethodException | IllegalAccessException e) {
+              throw new LookupRuntimeException(e);
+            }
+          });
+    } catch (LookupRuntimeException e) {
+      negativeCache.putIfAbsent(k, e);
+      throw e;
+    }
   }
 
   public static final class LookupRuntimeException extends RuntimeException {

btrace-gradle-plugin/src/main/groovy/org/openjdk/btrace/gradle/BTraceFatAgentExtension.groovy

@@ -351,7 +351,10 @@ class MavenExtensionSource extends ExtensionSource {
     }
 
     private ResolvedExtension extractFromZip(File zipFile) {
-        def tempDir = new File(project.buildDir, "fat-agent-staging/maven/${coordinates.replace(':', '_')}")
+        def tempDir = project.layout.buildDirectory
+            .dir("fat-agent-staging/maven/${coordinates.replace(':', '_')}")
+            .get()
+            .asFile
         tempDir.mkdirs()
 
         // Extract ZIP
@@ -417,7 +420,10 @@ class FileExtensionSource extends ExtensionSource {
     }
 
     private ResolvedExtension extractFromZip(File zipFile) {
-        def tempDir = new File(project.buildDir, "fat-agent-staging/file/${zipFile.name.replace('.zip', '')}")
+        def tempDir = project.layout.buildDirectory
+            .dir("fat-agent-staging/file/${zipFile.name.replace('.zip', '')}")
+            .get()
+            .asFile
         tempDir.mkdirs()
 
         project.copy {