From 5e1bb9d456bcf1eb13e593b1316767334b538376 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 25 Jul 2026 12:15:18 +0200 Subject: [PATCH 1/4] fix: restore per-client settings isolation, queue stack sizing and ASM version (#920) Three fixes that had been merged and were then overwritten by a release-prep commit rebased on a stale checkout, plus guards so the next overwrite fails the build rather than shipping. - agent: give each accepted remote connection its own SharedSettings copy. RemoteClient applies SET_PARAMS by mutating the context's settings in place, so sharing SharedSettings.GLOBAL let one client's debug, dumpDir, outputFile, granted permissions and trusted flag reach every other client and the global transformer. trusted is deliberately non-downgrading, making such a leak permanent for the lifetime of the agent. ClientContext now rejects the global instance outright, and the accept path is reachable from a unit test because inside the loop it sits under a catch that would downgrade the failure to a per-connection warning. - agent: restore the 4 MB command-queue processor stack on class-file major 70 and above, where ClassFile API's StackMapGenerator can exhaust the default depth. Older JVMs keep the JVM-chosen default. The sizing policy is now a pure function so it can be tested across versions. - client: build agent arguments through a single helper. The agent parses each comma-separated entry by splitting on = and matching the key, so a misplaced separator is not rejected -- the setting is dropped in silence. That is how cmdQueueLimit was lost. The helper rejects malformed keys, and the tests assert a populated argument string parses with no empty key. - gradle-plugin: return ASM to 9.10.1, matching the root version catalog. The plugin is an included build and cannot read the catalog, so AsmVersionSyncTest compares the two files and fails when they drift. Verified: btrace-agent, btrace-client, btrace-core and btrace-gradle-plugin unit tests, spotlessCheck, and the BTraceFunctionalTests, prepared-mode authentication and runtime-hardening integration suites. Each new guard was confirmed to fail against the pre-fix code. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/io/btrace/agent/ClientContext.java | 9 + .../src/main/java/io/btrace/agent/Main.java | 62 ++++++- .../agent/ClientSettingsIsolationTest.java | 172 ++++++++++++++++++ .../agent/QueueProcessorStackSizeTest.java | 71 ++++++++ .../main/java/io/btrace/client/Client.java | 52 ++++-- .../io/btrace/client/ClientAgentArgsTest.java | 118 ++++++++++++ btrace-gradle-plugin/build.gradle | 12 +- .../io/btrace/gradle/AsmVersionSyncTest.java | 125 +++++++++++++ 8 files changed, 599 insertions(+), 22 deletions(-) create mode 100644 btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java create mode 100644 btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java create mode 100644 btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java create mode 100644 btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java diff --git a/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java b/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java index c958856d2..3b76bdf3e 100644 --- a/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java +++ b/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java @@ -34,6 +34,15 @@ class ClientContext { ClientContext( Instrumentation instr, BTraceTransformer transformer, ArgsMap args, SharedSettings settings) { + // A client's SET_PARAMS command mutates these settings in place, so a context must never be + // built on the agent-wide instance: that would let one client's debug/dumpDir/outputFile, + // granted permissions and non-downgradable `trusted` flag leak into every other client and + // into the global transformer. Seed a per-client copy with Main#newClientSettings instead. + if (settings == SharedSettings.GLOBAL) { + throw new IllegalArgumentException( + "ClientContext must not share SharedSettings.GLOBAL; use Main#newClientSettings(..)" + + " to seed an isolated per-client copy"); + } this.instr = instr; this.transformer = transformer; this.args = args; diff --git a/btrace-agent/src/main/java/io/btrace/agent/Main.java b/btrace-agent/src/main/java/io/btrace/agent/Main.java index 7e546bddc..1cd3df3f4 100644 --- a/btrace-agent/src/main/java/io/btrace/agent/Main.java +++ b/btrace-agent/src/main/java/io/btrace/agent/Main.java @@ -180,6 +180,27 @@ public final class Main { private static final SharedSettings settings = SharedSettings.GLOBAL; private static final BTraceTransformer transformer = new BTraceTransformer(new DebugSupport(settings)); + // ClassFile API's StackMapGenerator can exhaust the default stack depth when processing + // complex methods in JDK 26+ (class-file major >= 70). Use 4 MB on those JVMs only; on + // older JVMs keep the default (0 = JVM-chosen) to avoid any OS-level stack-allocation + // overhead that was intermittently delaying test startup on JDK 8/11/21 CI machines. + static final long QUEUE_PROCESSOR_STACK_SIZE = resolveQueueProcessorStackSize(); + + static long resolveQueueProcessorStackSize() { + return resolveQueueProcessorStackSize(System.getProperty("java.class.version", "52.0")); + } + + // Visible for testing. + static long resolveQueueProcessorStackSize(String classVersion) { + try { + int dot = classVersion.indexOf('.'); + int major = Integer.parseInt(dot >= 0 ? classVersion.substring(0, dot) : classVersion); + return major >= 70 ? 4L * 1024 * 1024 : 0L; + } catch (RuntimeException ignored) { + return 0L; + } + } + // #BTRACE-42: Non-daemon thread prevents traced application from exiting // Deliberately an anonymous class, not a lambda: this static field initializer runs in // Main's , unconditionally, before -javaagent premain()'s body even starts -- see @@ -188,7 +209,8 @@ public final class Main { new ThreadFactory() { @Override public Thread newThread(Runnable r) { - Thread result = new Thread(r, "BTrace Command Queue Processor"); + Thread result = + new Thread(null, r, "BTrace Command Queue Processor", QUEUE_PROCESSOR_STACK_SIZE); result.setDaemon(true); return result; } @@ -1457,8 +1479,7 @@ private static boolean loadBTraceScript(String filePath, boolean traceToStdOut) return false; } - SharedSettings clientSettings = new SharedSettings(); - clientSettings.from(settings); + SharedSettings clientSettings = newClientSettings(settings); clientSettings.setClientName(scriptName); if (traceToStdOut) { clientSettings.setOutputFile("::stdout"); @@ -1511,7 +1532,7 @@ private static void runServer(ControlServer endpoint) { log.debug("client accepted from {}", sock.getRemoteSocketAddress()); } authenticationToken = endpoint.copyAuthenticationToken(); - ClientContext ctx = new ClientContext(inst, transformer, argMap, settings); + ClientContext ctx = newRemoteClientContext(); Client client = RemoteClient.getClient( ctx, @@ -1569,6 +1590,39 @@ private static synchronized void shutdownServer() { } } + /** + * Seeds a fresh per-client settings instance from the agent baseline. + * + *

Every client - local and remote alike - must get its own copy. A client's {@code SET_PARAMS} + * command is applied by mutating its {@link ClientContext}'s settings in place, so handing out + * the shared {@link SharedSettings#GLOBAL} instance lets one client's {@code debug}, {@code + * dumpDir}, {@code outputFile}, granted permissions and - most importantly - {@code trusted} flag + * leak into every other client and into the global transformer. {@code trusted} is deliberately + * non-downgrading, so such a leak is permanent for the lifetime of the agent. + * + * @param baseline the agent-wide settings to seed from + * @return a new settings instance, independent of {@code baseline} + */ + static SharedSettings newClientSettings(SharedSettings baseline) { + SharedSettings clientSettings = new SharedSettings(); + clientSettings.from(baseline); + return clientSettings; + } + + /** + * Builds the context for a newly accepted remote connection. + * + *

Extracted from the accept loop so the settings isolation it depends on is reachable from a + * unit test: inside the loop it sits under a {@code catch (RuntimeException | IOException)} that + * would turn a re-introduced sharing bug into a per-connection warning rather than a test + * failure. + * + * @return a context carrying settings private to this connection + */ + static ClientContext newRemoteClientContext() { + return new ClientContext(inst, transformer, argMap, newClientSettings(settings)); + } + private static Future handleNewClient(Client client) { // Deliberately an anonymous class, not a lambda: this method can run reachable from // -javaagent premain(), before the JVM's own java.lang.invoke bootstrap is guaranteed diff --git a/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java b/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java new file mode 100644 index 000000000..32d4c1e6c --- /dev/null +++ b/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.btrace.agent; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.btrace.core.ArgsMap; +import io.btrace.core.SharedSettings; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Every client must trace against its own {@link SharedSettings} copy. + * + *

A client's {@code SET_PARAMS} command is applied by {@code RemoteClient} as {@code + * ctx.getSettings().from(params)} - an in-place mutation. If contexts share the agent-wide + * instance, one client's parameters silently become every client's parameters. {@code trusted} is + * the dangerous case: {@link SharedSettings#from(Map)} ORs it in and never clears it, so a single + * client asking for {@code trusted} would permanently elevate the whole agent. + * + *

This isolation has been implemented and then accidentally reverted once already, with nothing + * failing. These tests exist so the next revert breaks the build. + */ +class ClientSettingsIsolationTest { + + @Test + @DisplayName("per-client settings are a copy, not the agent-wide instance") + void clientSettingsAreDistinctFromBaseline() { + SharedSettings baseline = new SharedSettings(); + SharedSettings clientSettings = Main.newClientSettings(baseline); + + assertNotSame(baseline, clientSettings, "client settings must not alias the baseline"); + assertNotSame( + SharedSettings.GLOBAL, + clientSettings, + "client settings must not alias the global instance"); + } + + @Test + @DisplayName("per-client settings inherit the agent baseline") + void clientSettingsInheritBaseline() { + SharedSettings baseline = new SharedSettings(); + baseline.setDebug(true); + baseline.setOutputFile("agent-baseline.btrace"); + + SharedSettings clientSettings = Main.newClientSettings(baseline); + + assertTrue(clientSettings.isDebug(), "baseline debug flag should be inherited"); + assertEquals("agent-baseline.btrace", clientSettings.getOutputFile()); + } + + @Test + @DisplayName("one client's SET_PARAMS does not leak into the agent baseline") + void setParamsDoesNotLeakToBaseline() { + SharedSettings baseline = new SharedSettings(); + SharedSettings clientSettings = Main.newClientSettings(baseline); + + // Mirrors RemoteClient's handling of Command.SET_PARAMS. + clientSettings.from(setParams()); + + assertTrue(clientSettings.isDebug(), "the requesting client should see its own parameters"); + assertFalse(baseline.isDebug(), "debug leaked into the agent baseline"); + assertFalse(baseline.isTrusted(), "trusted leaked into the agent baseline"); + assertEquals(null, baseline.getDumpDir(), "dumpDir leaked into the agent baseline"); + } + + @Test + @DisplayName("one client's SET_PARAMS does not leak into another client") + void setParamsDoesNotLeakBetweenClients() { + SharedSettings baseline = new SharedSettings(); + SharedSettings first = Main.newClientSettings(baseline); + SharedSettings second = Main.newClientSettings(baseline); + + first.from(setParams()); + + assertTrue(first.isTrusted(), "the requesting client should see its own parameters"); + assertFalse(second.isTrusted(), "trusted leaked into a concurrently connected client"); + assertFalse(second.isDebug(), "debug leaked into a concurrently connected client"); + } + + @Test + @DisplayName("trusted cannot be downgraded, so a leak would be permanent") + void trustedIsNonDowngrading() { + SharedSettings settings = new SharedSettings(); + settings.from(setParams()); + assertTrue(settings.isTrusted()); + + Map untrust = new HashMap<>(); + untrust.put(SharedSettings.TRUSTED_KEY, Boolean.FALSE); + settings.from(untrust); + + assertTrue( + settings.isTrusted(), + "trusted is deliberately non-downgrading - which is exactly why sharing the instance" + + " between clients is unsafe"); + } + + @Test + @DisplayName("a ClientContext cannot be built on the agent-wide settings") + void clientContextRejectsGlobalSettings() { + IllegalArgumentException failure = + assertThrows( + IllegalArgumentException.class, + () -> new ClientContext(null, null, new ArgsMap(), SharedSettings.GLOBAL)); + + assertTrue( + failure.getMessage().contains("newClientSettings"), + "the failure should point at the supported way to seed per-client settings"); + } + + @Test + @DisplayName("a ClientContext accepts an isolated copy") + void clientContextAcceptsIsolatedSettings() { + ClientContext ctx = + new ClientContext(null, null, new ArgsMap(), Main.newClientSettings(SharedSettings.GLOBAL)); + + assertNotSame(SharedSettings.GLOBAL, ctx.getSettings()); + } + + @Test + @DisplayName("the remote accept path builds contexts on isolated settings") + void remoteContextDoesNotShareGlobalSettings() { + ClientContext ctx = Main.newRemoteClientContext(); + + assertNotSame( + SharedSettings.GLOBAL, + ctx.getSettings(), + "the remote accept path handed out the agent-wide settings instance"); + } + + @Test + @DisplayName("two remote connections do not share settings with each other") + void remoteContextsAreIndependent() { + ClientContext first = Main.newRemoteClientContext(); + ClientContext second = Main.newRemoteClientContext(); + + assertNotSame(first.getSettings(), second.getSettings()); + + first.getSettings().from(setParams()); + + assertFalse(second.getSettings().isTrusted(), "trusted leaked between remote connections"); + assertFalse(SharedSettings.GLOBAL.isTrusted(), "trusted leaked into the agent-wide settings"); + } + + private static Map setParams() { + Map params = new HashMap<>(); + params.put(SharedSettings.DEBUG_KEY, Boolean.TRUE); + params.put(SharedSettings.TRUSTED_KEY, Boolean.TRUE); + params.put(SharedSettings.DUMP_DIR_KEY, "/tmp/leaked-dump-dir"); + return params; + } +} diff --git a/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java b/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java new file mode 100644 index 000000000..09799c2e1 --- /dev/null +++ b/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.btrace.agent; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * The command-queue processor thread needs an enlarged stack on JDK 26+. + * + *

ClassFile API's {@code StackMapGenerator} can exhaust the default stack depth while processing + * complex methods on class-file major 70 and above. Older JVMs keep the JVM-chosen default, because + * an explicit stack size there was intermittently delaying test startup on JDK 8/11/21 CI machines. + * + *

This sizing has been dropped by an accidental revert once already; these tests pin both halves + * of the trade-off. + */ +class QueueProcessorStackSizeTest { + + private static final long FOUR_MB = 4L * 1024 * 1024; + + @Test + @DisplayName("JDK 26+ class files get an enlarged stack") + void enlargedStackOnJdk26AndLater() { + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("70.0"), "JDK 26 (major 70)"); + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("71.0"), "JDK 27 (major 71)"); + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("99"), "far-future JVM"); + } + + @Test + @DisplayName("older JVMs keep the JVM-chosen default") + void defaultStackBelowJdk26() { + assertEquals(0L, Main.resolveQueueProcessorStackSize("52.0"), "JDK 8 (major 52)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("55.0"), "JDK 11 (major 55)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("65.0"), "JDK 21 (major 65)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("69.0"), "JDK 25 (major 69)"); + } + + @Test + @DisplayName("an unparseable class version falls back to the default") + void malformedClassVersionFallsBack() { + assertEquals(0L, Main.resolveQueueProcessorStackSize("")); + assertEquals(0L, Main.resolveQueueProcessorStackSize("not-a-version")); + assertEquals(0L, Main.resolveQueueProcessorStackSize(".7")); + } + + @Test + @DisplayName("the constant is wired to the running JVM's class version") + void constantMatchesRunningJvm() { + assertEquals( + Main.resolveQueueProcessorStackSize(System.getProperty("java.class.version", "52.0")), + Main.QUEUE_PROCESSOR_STACK_SIZE, + "QUEUE_PROCESSOR_STACK_SIZE must be derived from java.class.version"); + } +} diff --git a/btrace-client/src/main/java/io/btrace/client/Client.java b/btrace-client/src/main/java/io/btrace/client/Client.java index 90a52b789..0bd7bfb07 100644 --- a/btrace-client/src/main/java/io/btrace/client/Client.java +++ b/btrace-client/src/main/java/io/btrace/client/Client.java @@ -233,6 +233,28 @@ private static void warnIfDeprecatedTargetJvm(String pid, String targetJavaVersi } } + /** + * Appends a single {@code key=value} entry to a comma-separated agent argument string. + * + *

The agent splits each entry on its first {@code =} and matches on the key, so an entry whose + * key is empty or misplaced is not rejected - it is silently ignored, and the setting it carried + * is lost with no diagnostic. A hand-rolled {@code ",=" + KEY + value} typo did exactly that to + * {@code cmdQueueLimit} for two releases. Routing every entry through here keeps the shape + * uniform and lets {@code ClientAgentArgsTest} assert it. + * + * @param agentArgs the arguments accumulated so far; empty for the first entry + * @param key the argument name, which must be non-empty and free of {@code =} and {@code ,} + * @param value the argument value, rendered with {@link String#valueOf(Object)} + * @return {@code agentArgs} with the new entry appended + */ + static String appendAgentArg(String agentArgs, String key, Object value) { + if (key == null || key.isEmpty() || key.indexOf('=') >= 0 || key.indexOf(',') >= 0) { + throw new IllegalArgumentException("invalid agent argument key: " + key); + } + String entry = key + "=" + String.valueOf(value); + return agentArgs.isEmpty() ? entry : agentArgs + "," + entry; + } + private static boolean isPortAvailable(int port) { Socket clSocket = null; try { @@ -772,25 +794,25 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th log.debug("attached to {}", pid); log.debug("loading {}", agentPath); } - String agentArgs = Args.PORT + "=" + port; + String agentArgs = appendAgentArg("", Args.PORT, port); if (statsdDef != null) { - agentArgs += "," + Args.STATSD + "=" + statsdDef; + agentArgs = appendAgentArg(agentArgs, Args.STATSD, statsdDef); } if (debug) { - agentArgs += "," + Args.DEBUG + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.DEBUG, "true"); } if (trusted) { - agentArgs += "," + Args.TRUSTED + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.TRUSTED, "true"); } if (dumpClasses) { - agentArgs += "," + Args.DUMP_CLASSES + "=true"; - agentArgs += "," + Args.DUMP_DIR + "=" + dumpDir; + agentArgs = appendAgentArg(agentArgs, Args.DUMP_CLASSES, "true"); + agentArgs = appendAgentArg(agentArgs, Args.DUMP_DIR, dumpDir); } if (trackRetransforms) { - agentArgs += "," + Args.TRACK_RETRANSFORMS + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.TRACK_RETRANSFORMS, "true"); } if (bootCp != null) { - agentArgs += "," + Args.BOOT_CLASS_PATH + "=" + bootCp; + agentArgs = appendAgentArg(agentArgs, Args.BOOT_CLASS_PATH, bootCp); } String toolsPath = getToolsJarPath( @@ -800,12 +822,12 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th } else { sysCp = sysCp + File.pathSeparator + toolsPath; } - agentArgs += "," + Args.SYSTEM_CLASS_PATH + "=" + sysCp; + agentArgs = appendAgentArg(agentArgs, Args.SYSTEM_CLASS_PATH, sysCp); String cmdQueueLimit = System.getProperty(BTraceRuntime.CMD_QUEUE_LIMIT_KEY, null); if (cmdQueueLimit != null) { - agentArgs += "," + Args.CMD_QUEUE_LIMIT + "=" + cmdQueueLimit; + agentArgs = appendAgentArg(agentArgs, Args.CMD_QUEUE_LIMIT, cmdQueueLimit); } - agentArgs += "," + Args.PROBE_DESC_PATH + "=" + probeDescPath; + agentArgs = appendAgentArg(agentArgs, Args.PROBE_DESC_PATH, probeDescPath); // Pass-through selected system properties as agent system props via "$" args // so the agent can read them at startup. These become system properties in the target JVM. @@ -818,28 +840,28 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th throw new IllegalArgumentException( "System property btrace.feature.manifestLibs must not contain ','"); } - agentArgs += "," + "$btrace.feature.manifestLibs" + "=" + manifestLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.feature.manifestLibs", manifestLibs); } if (sysAppendJar != null && !sysAppendJar.isEmpty()) { if (sysAppendJar.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.system.appendJar must not contain ','"); } - agentArgs += "," + "$btrace.system.appendJar" + "=" + sysAppendJar; + agentArgs = appendAgentArg(agentArgs, "$btrace.system.appendJar", sysAppendJar); } if (allowExternalLibs != null && !allowExternalLibs.isEmpty()) { if (allowExternalLibs.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.allowExternalLibs must not contain ','"); } - agentArgs += "," + "$btrace.allowExternalLibs" + "=" + allowExternalLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.allowExternalLibs", allowExternalLibs); } if (testSkipLibs != null && !testSkipLibs.isEmpty()) { if (testSkipLibs.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.test.skipLibs must not contain ','"); } - agentArgs += "," + "$btrace.test.skipLibs" + "=" + testSkipLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.test.skipLibs", testSkipLibs); } if (log.isDebugEnabled()) { log.debug("agent args: {}", agentArgs); diff --git a/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java b/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java new file mode 100644 index 000000000..d010fddc0 --- /dev/null +++ b/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.btrace.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.btrace.core.Args; +import io.btrace.core.ArgsMap; +import java.util.Map; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * The agent argument string the client hands to {@code VirtualMachine#loadAgent} must survive the + * agent's own parsing. + * + *

{@link ArgsMap} splits each comma-separated entry on {@code =} and dispatches on the key, so a + * malformed entry is not an error - it parses into a key nothing matches and the setting is dropped + * in silence. {@code cmdQueueLimit} was shipped that way, fixed, then reverted back by an unrelated + * commit, because nothing asserted the wire shape. These tests assert it. + */ +class ClientAgentArgsTest { + + @Test + @DisplayName("the first entry carries no leading separator") + void firstEntryHasNoLeadingComma() { + assertEquals("port=2020", Client.appendAgentArg("", Args.PORT, 2020)); + } + + @Test + @DisplayName("subsequent entries are comma separated") + void subsequentEntriesAreCommaSeparated() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.DEBUG, "true"); + + assertEquals("port=2020,debug=true", args); + } + + @Test + @DisplayName("cmdQueueLimit round-trips through the agent's parser") + void cmdQueueLimitRoundTripsThroughArgsMap() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.CMD_QUEUE_LIMIT, "512"); + + ArgsMap parsed = new ArgsMap(args.split(",")); + + assertEquals( + "512", + parsed.get(Args.CMD_QUEUE_LIMIT), + "the agent must see cmdQueueLimit as a key, not as part of a value"); + } + + @Test + @DisplayName("a fully populated argument string parses without an empty key") + void noEntryParsesToAnEmptyKey() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.STATSD, "localhost:8125"); + args = Client.appendAgentArg(args, Args.DEBUG, "true"); + args = Client.appendAgentArg(args, Args.TRUSTED, "true"); + args = Client.appendAgentArg(args, Args.DUMP_CLASSES, "true"); + args = Client.appendAgentArg(args, Args.DUMP_DIR, "/tmp/dump"); + args = Client.appendAgentArg(args, Args.TRACK_RETRANSFORMS, "true"); + args = Client.appendAgentArg(args, Args.CMD_QUEUE_LIMIT, "512"); + args = Client.appendAgentArg(args, Args.PROBE_DESC_PATH, "."); + args = Client.appendAgentArg(args, "$btrace.system.appendJar", "/tmp/x.jar"); + + ArgsMap parsed = new ArgsMap(args.split(",")); + + for (Map.Entry entry : parsed) { + assertFalse( + entry.getKey().isEmpty(), + "an empty key means an entry was built with a misplaced '='; the agent silently drops it"); + } + assertEquals("512", parsed.get(Args.CMD_QUEUE_LIMIT)); + assertEquals("localhost:8125", parsed.get(Args.STATSD)); + assertEquals("/tmp/x.jar", parsed.get("$btrace.system.appendJar")); + } + + @Test + @DisplayName("a malformed key is rejected rather than silently dropped downstream") + void malformedKeysAreRejected() { + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "", "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", null, "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "=bad", "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "ba,d", "v")); + } + + /** + * Pins the failure mode the guard exists for: the historical {@code ",=" + KEY + value} typo + * parses into an empty key, which no {@code case} in the agent matches. + */ + @Test + @DisplayName("the historical malformed shape does parse to an empty key") + void malformedShapeParsesToEmptyKey() { + String malformed = "port=2020" + ",=" + Args.CMD_QUEUE_LIMIT + "512"; + + ArgsMap parsed = new ArgsMap(malformed.split(",")); + + assertEquals(null, parsed.get(Args.CMD_QUEUE_LIMIT), "the setting is lost, not reported"); + assertEquals(Args.CMD_QUEUE_LIMIT + "512", parsed.get(""), "it lands under an empty key"); + } +} diff --git a/btrace-gradle-plugin/build.gradle b/btrace-gradle-plugin/build.gradle index 3cf5ef53b..b8ec7bf34 100644 --- a/btrace-gradle-plugin/build.gradle +++ b/btrace-gradle-plugin/build.gradle @@ -26,9 +26,11 @@ repositories { dependencies { implementation gradleApi() implementation localGroovy() - // Match versions commonly cached in this repository to avoid extra fetches - implementation 'org.ow2.asm:asm:9.9.1' - implementation 'org.ow2.asm:asm-tree:9.9.1' + // This is an included build and cannot access the root version catalog, so the ASM version is + // duplicated here. It must stay in sync with version('asm', ...) in the root settings.gradle; + // AsmVersionSyncTest fails the build if the two drift apart. + implementation 'org.ow2.asm:asm:9.10.1' + implementation 'org.ow2.asm:asm-tree:9.10.1' // This is an included build and cannot access the root version catalog. Keep its tests on the // last Java 8-compatible JUnit line; the production plugin remains Java 11-compatible. @@ -47,6 +49,10 @@ test { javaLauncher = javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(17) } + // AsmVersionSyncTest compares this build's hard-coded ASM version against the root version + // catalog; hand it both files rather than let it guess from the working directory. + systemProperty 'btrace.test.pluginBuildFile', file('build.gradle').absolutePath + systemProperty 'btrace.test.rootSettingsFile', rootDir.parentFile.toPath().resolve('settings.gradle').toString() } gradlePlugin { diff --git a/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java b/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java new file mode 100644 index 000000000..6db8b5d6e --- /dev/null +++ b/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.btrace.gradle; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Guards the one dependency version this build is forced to duplicate. + * + *

{@code btrace-gradle-plugin} is an included build, so it cannot resolve the root build's + * version catalog and has to hard-code its ASM coordinates. That duplication has already drifted + * once: a release-prep commit rebased on a stale checkout silently reverted the plugin's ASM to an + * older version than the catalog, and nothing failed. This test turns that silent drift into a + * build failure. + */ +class AsmVersionSyncTest { + + private static final Pattern CATALOG_ASM_VERSION = + Pattern.compile("version\\s*\\(\\s*'asm'\\s*,\\s*'([^']+)'\\s*\\)"); + private static final Pattern PLUGIN_ASM_COORDINATE = + Pattern.compile("['\"]org\\.ow2\\.asm:(asm[\\w-]*):([^'\"]+)['\"]"); + + @Test + @DisplayName("plugin ASM version matches the root version catalog") + void asmVersionMatchesRootCatalog() throws IOException { + Path rootSettings = requiredPath("btrace.test.rootSettingsFile"); + Assumptions.assumeTrue( + Files.isRegularFile(rootSettings), + "root settings.gradle not reachable - plugin is being built standalone"); + + String catalogVersion = catalogAsmVersion(read(rootSettings)); + Set pluginVersions = pluginAsmVersions(read(pluginBuildFile())); + + assertTrue(!pluginVersions.isEmpty(), "no org.ow2.asm coordinates found in the plugin build"); + assertEquals( + 1, + pluginVersions.size(), + "btrace-gradle-plugin declares conflicting ASM versions: " + pluginVersions); + assertEquals( + catalogVersion, + pluginVersions.iterator().next(), + "btrace-gradle-plugin's hard-coded ASM version has drifted from version('asm', ...) " + + "in the root settings.gradle. Update btrace-gradle-plugin/build.gradle to " + + "match, or update the catalog if the plugin is intentionally pinned."); + } + + @Test + @DisplayName("every ASM artifact in the plugin shares one version") + void allAsmArtifactsShareOneVersion() throws IOException { + String pluginBuild = read(pluginBuildFile()); + Matcher m = PLUGIN_ASM_COORDINATE.matcher(pluginBuild); + Set artifacts = new LinkedHashSet<>(); + Set versions = new LinkedHashSet<>(); + while (m.find()) { + artifacts.add(m.group(1)); + versions.add(m.group(2)); + } + assertTrue(artifacts.contains("asm"), "expected an org.ow2.asm:asm dependency"); + assertTrue(artifacts.contains("asm-tree"), "expected an org.ow2.asm:asm-tree dependency"); + assertEquals(1, versions.size(), "ASM artifacts must share a single version: " + versions); + } + + private static String catalogAsmVersion(String settings) { + Matcher m = CATALOG_ASM_VERSION.matcher(settings); + assertTrue(m.find(), "version('asm', ...) not found in the root settings.gradle"); + return m.group(1); + } + + private static Set pluginAsmVersions(String buildFile) { + Matcher m = PLUGIN_ASM_COORDINATE.matcher(buildFile); + Set versions = new LinkedHashSet<>(); + while (m.find()) { + versions.add(m.group(2)); + } + return versions; + } + + private static Path pluginBuildFile() { + Path buildFile = requiredPath("btrace.test.pluginBuildFile"); + assertTrue(Files.isRegularFile(buildFile), "cannot locate btrace-gradle-plugin/build.gradle"); + return buildFile; + } + + private static Path requiredPath(String property) { + String value = System.getProperty(property); + assertTrue( + value != null && !value.isEmpty(), + "system property " + property + " is not set; it is wired up by the test task"); + return Paths.get(value); + } + + private static String read(Path path) throws IOException { + List lines = Files.readAllLines(path, StandardCharsets.UTF_8); + return String.join("\n", lines); + } +} From 69cf631600e952e989fc6de647a66c7ee164bd09 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 25 Jul 2026 13:30:19 +0200 Subject: [PATCH 2/4] fix(client): derive console handling from the running JVM, not JAVA_HOME (#887) The Java 22 console workaround was keyed off a `release` file located through JAVA_HOME. That is the wrong source: the console quirk belongs to the JVM running the client, and JAVA_HOME is a launcher convention that need not point at it. With JAVA_HOME set to a different JDK than the one executing the client, the check read the wrong version and skipped the workaround, reproducing the stdout-to-stderr behaviour it exists to avoid. Read java.specification.version through the existing JavaVersionCheck helper instead. This also removes the surrounding failure modes rather than guarding them: no file read, so no InvalidPathException, and no getProperty("JAVA_VERSION").replace(..) NPE when the key is absent -- both were unchecked throws inside a static initializer, where they surface as ExceptionInInitializerError and stop the CLI from starting at all. Behaviour is unchanged wherever it was already correct: an undeterminable version still uses the console, non-22 still uses the console, 22 still bypasses it. Comparing the parsed feature version also handles 22 and 22.0.1 uniformly, where startsWith("22") was a prefix match. Also replaces the fully qualified JavaVersionCheck references in Client with an import, per the repository convention. Verified: btrace-agent, btrace-client and btrace-core unit tests plus spotlessCheck with JAVA_HOME unset and set, and the BTraceFunctionalTests and prepared-mode authentication integration suites. Co-Authored-By: Claude Opus 5 (1M context) --- .../main/java/io/btrace/client/Client.java | 13 +-- .../src/main/java/io/btrace/client/Main.java | 37 ++++----- .../btrace/client/ConsoleSelectionTest.java | 81 +++++++++++++++++++ 3 files changed, 103 insertions(+), 28 deletions(-) create mode 100644 btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java diff --git a/btrace-client/src/main/java/io/btrace/client/Client.java b/btrace-client/src/main/java/io/btrace/client/Client.java index 0bd7bfb07..0132d08c8 100644 --- a/btrace-client/src/main/java/io/btrace/client/Client.java +++ b/btrace-client/src/main/java/io/btrace/client/Client.java @@ -23,6 +23,7 @@ import io.btrace.compiler.Compiler; import io.btrace.core.Args; import io.btrace.core.BTraceRuntime; +import io.btrace.core.JavaVersionCheck; import io.btrace.core.SharedSettings; import io.btrace.core.annotations.DTrace; import io.btrace.core.annotations.DTraceRef; @@ -211,21 +212,21 @@ public Client( */ private static void warnIfDeprecatedTargetJvm(String pid, String targetJavaVersion) { try { - int featureVersion = io.btrace.core.JavaVersionCheck.parseFeatureVersion(targetJavaVersion); - if (io.btrace.core.JavaVersionCheck.isDeprecated(featureVersion) - && !Boolean.getBoolean(io.btrace.core.JavaVersionCheck.SUPPRESS_PROP)) { + int featureVersion = JavaVersionCheck.parseFeatureVersion(targetJavaVersion); + if (JavaVersionCheck.isDeprecated(featureVersion) + && !Boolean.getBoolean(JavaVersionCheck.SUPPRESS_PROP)) { System.err.println( "[BTrace] WARNING: The target JVM (PID " + pid + ") is Java " + featureVersion + ". Running BTrace on Java versions older than " - + io.btrace.core.JavaVersionCheck.DEPRECATION_FLOOR + + JavaVersionCheck.DEPRECATION_FLOOR + " is deprecated and support will be removed in the next major release. " + "Please upgrade the target JVM to Java " - + io.btrace.core.JavaVersionCheck.DEPRECATION_FLOOR + + JavaVersionCheck.DEPRECATION_FLOOR + " or newer. Suppress this warning with -D" - + io.btrace.core.JavaVersionCheck.SUPPRESS_PROP + + JavaVersionCheck.SUPPRESS_PROP + "=true."); } } catch (Throwable ignored) { diff --git a/btrace-client/src/main/java/io/btrace/client/Main.java b/btrace-client/src/main/java/io/btrace/client/Main.java index a4a661077..c008aad5f 100644 --- a/btrace-client/src/main/java/io/btrace/client/Main.java +++ b/btrace-client/src/main/java/io/btrace/client/Main.java @@ -22,6 +22,7 @@ import io.btrace.compiler.oneliner.OnelinerParser; import io.btrace.compiler.oneliner.OnelinerValidator; import io.btrace.core.DebugSupport; +import io.btrace.core.JavaVersionCheck; import io.btrace.core.Messages; import io.btrace.core.comm.Command; import io.btrace.core.comm.CommandListener; @@ -32,9 +33,6 @@ import java.io.File; import java.io.IOException; import java.io.PrintWriter; -import java.nio.file.Files; -import java.nio.file.Paths; -import java.util.Properties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sun.misc.Signal; @@ -84,28 +82,23 @@ public final class Main { if (log.isDebugEnabled()) log.debug("dumpDir is {}", DUMP_DIR); } PROBE_DESC_PATH = System.getProperty("com.sun.btrace.probeDescPath", "."); - String javaVersion = getJavaVersion(); - // In Java 22 the console will write standard output to stderr :shrug: - con = javaVersion == null || !javaVersion.startsWith("22") ? System.console() : null; + con = suppressConsole(JavaVersionCheck.javaFeatureVersion()) ? null : System.console(); out = getOutWriter(con); } - private static String getJavaVersion() { - String javaHome = System.getenv("JAVA_HOME"); - if (javaHome == null || javaHome.trim().isEmpty()) { - // Fall back to the JVM the client itself is running on; JAVA_HOME is not required to be set. - javaHome = System.getProperty("java.home"); - } - if (javaHome == null || javaHome.trim().isEmpty()) { - return null; - } - Properties props = new Properties(); - try { - props.load(Files.newInputStream(Paths.get(javaHome, "release"))); - return props.getProperty("JAVA_VERSION").replace("\"", ""); - } catch (IOException ignored) { - return null; - } + /** + * Whether {@link System#console()} must be bypassed on the given Java feature version. + * + *

Java 22's console writes standard output to stderr, so the client falls back to {@link + * System#out} there. The version that matters is the one of the JVM running this client - not + * {@code JAVA_HOME}, which need not point at it - so this is decided from {@code + * java.specification.version} rather than by reading a {@code release} file off disk. + * + * @param featureVersion a Java feature version, or {@code -1} when it can not be determined + * @return {@code true} to bypass the console + */ + static boolean suppressConsole(int featureVersion) { + return featureVersion == 22; } @SuppressWarnings("DefaultCharset") diff --git a/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java b/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java new file mode 100644 index 000000000..031b06efb --- /dev/null +++ b/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.btrace.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.btrace.core.JavaVersionCheck; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * The client bypasses {@link System#console()} on Java 22 only, because that release writes + * standard output to stderr. + * + *

The decision must follow the JVM actually running the client. It used to be derived from a + * {@code release} file located through {@code JAVA_HOME}, which is not required to be set, need not + * point at the running JVM, and cost a file read plus two unchecked-exception surfaces inside a + * static initializer - where any throw becomes an {@code ExceptionInInitializerError} that stops + * the CLI from starting at all. + */ +class ConsoleSelectionTest { + + @Test + @DisplayName("Java 22 bypasses the console") + void java22BypassesConsole() { + assertTrue(Main.suppressConsole(22)); + } + + @ParameterizedTest(name = "Java {0} uses the console") + @ValueSource(ints = {8, 11, 17, 21, 23, 24, 25, 26, 27}) + @DisplayName("every other release uses the console") + void otherVersionsUseConsole(int featureVersion) { + assertFalse( + Main.suppressConsole(featureVersion), + "only Java 22 has the stderr redirection; " + featureVersion + " must keep the console"); + } + + @Test + @DisplayName("an undeterminable version falls back to using the console") + void unknownVersionUsesConsole() { + assertFalse( + Main.suppressConsole(-1), + "an unknown version must behave as before: use the console rather than guess"); + } + + @Test + @DisplayName("the running JVM's version is resolved without JAVA_HOME") + void runningVersionResolvesIndependentlyOfJavaHome() { + // The suite runs both with and without JAVA_HOME set; neither may change the answer, and + // neither may leave the version undeterminable. + int featureVersion = JavaVersionCheck.javaFeatureVersion(); + + assertTrue( + featureVersion >= 8, + "the running JVM's feature version must be resolvable from system properties alone," + + " independently of JAVA_HOME; got " + + featureVersion); + assertEquals( + JavaVersionCheck.parseFeatureVersion(System.getProperty("java.specification.version")), + featureVersion, + "the version must come from the running JVM, not from an external installation"); + } +} From 56437df85686f7923ba484db7e58d0343ef4ad36 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sun, 26 Jul 2026 14:46:03 +0200 Subject: [PATCH 3/4] fix(build): mirror the released extension set and derive the staged ASM version Two follow-ups to the ASM bump, both instances of the same duplication problem the bump was meant to remove. Issue884PublishedFatAgentE2ETest held a third copy of the ASM version, with a comment noting it had to be kept in step with btrace-gradle-plugin by hand. It stages that version into a temporary repository which is the external consumer build's only source, so once the plugin moved to 9.10.1 the consumer could no longer resolve its ASM dependency and the test failed. The version is now read from version('asm', ..) in the root settings.gradle, the same value the plugin is held to, so it cannot drift again. copyExtensions was a Copy into the distribution's extensions/ directory, which never removes anything. Archives and exploded directories for extensions since withdrawn from the release set survived incremental builds, so a developer build kept shipping btrace-ext-test and the Spark/Hadoop examples and the release allow-list silently stopped being enforced. Sync makes the destination mirror the allow-list exactly; it also clears the exploded directories, which explodeExtensions recreates in the same build. No new test for the packaging change: BTraceJarPackagingTest already asserts the directory holds exactly the maintained extensions as both archives and exploded directories. It was passing only because CI builds from a clean tree. Verified: seeded stale btrace-ext-test and btrace-spark archives and exploded directories, then confirmed an incremental build removes them and leaves the seven maintained extensions intact; btrace-agent, btrace-client, btrace-core and btrace-dist unit tests plus spotlessCheck; Issue884PublishedFatAgentE2ETest end to end. Co-Authored-By: Claude Opus 5 (1M context) --- btrace-dist/build.gradle | 9 +++-- .../Issue884PublishedFatAgentE2ETest.java | 33 ++++++++++++++++--- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/btrace-dist/build.gradle b/btrace-dist/build.gradle index 67fd1d35b..e2b7aa5b0 100644 --- a/btrace-dist/build.gradle +++ b/btrace-dist/build.gradle @@ -509,9 +509,14 @@ task copyDtraceLib(type: Copy) { into "${distTarget}/libs/" } -task copyExtensions(type: Copy) { +// Sync, not Copy: the allow-list below is only meaningful if the destination mirrors it exactly. +// A Copy leaves behind archives and exploded directories belonging to extensions that have since +// been withdrawn from the release set, so an incremental build keeps shipping them and the +// allow-list silently stops being enforced. Sync removes anything not produced by this build, +// including the subdirectories explodeExtensions creates, which that task then recreates. +task copyExtensions(type: Sync) { group 'Build' - description 'Copy extension ZIP archives to extensions/ directory' + description 'Mirror the released extension ZIP archives into the extensions/ directory' into "${distTarget}/extensions/" } diff --git a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java index 8a84679dd..dc0a08059 100644 --- a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java +++ b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java @@ -28,6 +28,8 @@ import java.util.concurrent.TimeUnit; import java.util.jar.Attributes; import java.util.jar.JarFile; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -37,8 +39,14 @@ class Issue884PublishedFatAgentE2ETest { private static final String VERSION = "3.0.0"; - /** Must match the ASM version the Gradle plugin declares in {@code btrace-gradle-plugin}. */ - private static final String ASM_VERSION = "9.9.1"; + /** + * Matches {@code version('asm', ..)} in the root {@code settings.gradle}, which is also what + * {@code btrace-gradle-plugin} declares. The external consumer build resolves from the staged + * repository alone, so an ASM version staged here that the plugin does not depend on leaves the + * consumer unable to resolve it. Deriving the value keeps that from drifting. + */ + private static final Pattern CATALOG_ASM_VERSION = + Pattern.compile("version\\s*\\(\\s*'asm'\\s*,\\s*'([^']+)'\\s*\\)"); @TempDir Path temporaryDirectory; @@ -106,8 +114,25 @@ private void publishFixtureRepository(Path root, Path repository) throws Excepti publishPluginArtifacts(root, repository); publishArtifact(repository, "io/btrace", "btrace", VERSION, engineJar); - publishDependency(repository, "org.ow2.asm", "asm", ASM_VERSION); - publishDependency(repository, "org.ow2.asm", "asm-tree", ASM_VERSION); + String asmVersion = catalogAsmVersion(root); + publishDependency(repository, "org.ow2.asm", "asm", asmVersion); + publishDependency(repository, "org.ow2.asm", "asm-tree", asmVersion); + } + + /** + * Reads the ASM version from the root version catalog rather than restating it here. + * + * @param root the repository root + * @return the ASM version the Gradle plugin resolves against + */ + private String catalogAsmVersion(Path root) throws IOException { + Path settings = root.resolve("settings.gradle"); + assertTrue(Files.isRegularFile(settings), "missing root settings.gradle: " + settings); + Matcher matcher = + CATALOG_ASM_VERSION.matcher( + new String(Files.readAllBytes(settings), StandardCharsets.UTF_8)); + assertTrue(matcher.find(), "version('asm', ..) not found in " + settings); + return matcher.group(1); } private void publishPluginArtifacts(Path root, Path repository) throws Exception { From b06262e029c2d05a9b87444510eea6a0c165e455 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sun, 26 Jul 2026 15:35:41 +0200 Subject: [PATCH 4/4] test(integration): remove the attach/teardown race and unify JDK resolution Three robustness problems in the integration harness, all load-sensitive and all capable of failing a run for reasons unrelated to the code under test. testOnExit gated on output that cannot appear until after the gate returns. @OnExit fires only when the target JVM exits, and the harness signals that shutdown after the completion gate completes, so waiting for "onexit" always burned the full timeout by design. On a loaded machine the client had not finished attaching when that timeout expired, and the teardown then removed the target underneath it: the client reported "No such process" on stderr and the assertion on empty stderr failed. It now gates on the probe actually being live, which is deterministic and faster; the harness no longer reports a completion timeout for this test at all. testOSMBean already documented this same race and resolved it the same way. Issue884PublishedFatAgentE2ETest allowed 90 seconds for the nested Gradle builds it forks. Those resolve plugins and dependencies and may compile from a cold cache, so the limit expired on a loaded machine and surfaced as "publication did not finish" rather than as anything diagnostic. The budget is now a named constant, generous enough to distinguish a hung build from a slow one. Nine call sites resolved the test JDK independently, with three different chains: most omitted the java.home fallback and one also omitted the JAVA_TEST_HOME alias, so an environment without JAVA_HOME failed in some code paths while working in others. They now share resolveTestJavaHome(). testTraceAll and testJfr deliberately do not use that helper. They read a release file to learn the *target* JDK's version, and a java.home fallback would make them read from disk to rediscover the version of the JVM already running them - the same pattern removed from the client in this branch, missing release key included. They use targetJdkVersion(), which consults only an explicitly configured test JDK and otherwise reports java.runtime.version. Verified: testOnExit passes under deliberate CPU saturation with no completion timeout logged; the full integration suite passes; the suite also passes with none of TEST_JAVA_HOME, JAVA_TEST_HOME or JAVA_HOME set, which previously threw before running anything; unit tests and spotlessCheck pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/tests/BTraceFunctionalTests.java | 71 ++++++++++------- .../Issue884PublishedFatAgentE2ETest.java | 18 ++++- .../src/test/java/tests/RuntimeTest.java | 77 ++++++++----------- 3 files changed, 92 insertions(+), 74 deletions(-) diff --git a/integration-tests/src/test/java/tests/BTraceFunctionalTests.java b/integration-tests/src/test/java/tests/BTraceFunctionalTests.java index 4c361dd7e..4e296c0a8 100644 --- a/integration-tests/src/test/java/tests/BTraceFunctionalTests.java +++ b/integration-tests/src/test/java/tests/BTraceFunctionalTests.java @@ -21,7 +21,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -import static org.junit.jupiter.api.Assumptions.assumeFalse; import static org.junit.jupiter.api.Assumptions.assumeTrue; import io.btrace.client.Client; @@ -62,6 +61,13 @@ * @author Jaroslav Bachorik */ public class BTraceFunctionalTests extends RuntimeTest { + /** + * The client logs this once a probe is installed and running. Tests whose expected output can + * only appear after the target is torn down gate on it, so the teardown never races an attach + * that is still in progress. + */ + private static final String PROBE_STARTED_MARKER = "Successfully started BTrace probe"; + @BeforeAll public static void setup() throws Exception { classSetup(); @@ -73,6 +79,31 @@ public void reset() { super.reset(); } + /** + * The feature version of the JDK under test. + * + *

Deliberately not routed through {@link RuntimeTest#resolveTestJavaHome()}: that method's + * {@code java.home} fallback would make this read a {@code release} file off disk purely to + * rediscover the version of the JVM already executing this code, which is what {@code + * java.runtime.version} reports directly. Only an explicitly configured test JDK can differ. + * + * @return the version string of the JDK the target application will run under + */ + private static String targetJdkVersion() throws IOException { + String configured = System.getenv("TEST_JAVA_HOME"); + if (configured == null) { + configured = System.getenv("JAVA_TEST_HOME"); + } + String rtVersion = System.getProperty("java.runtime.version", ""); + if (configured == null) { + return rtVersion; + } + Properties releaseProps = new Properties(); + releaseProps.load( + Files.newInputStream(new File(configured + File.separator + "release").toPath())); + return releaseProps.getProperty("JAVA_VERSION", rtVersion).replace("\"", ""); + } + @Test public void testOSMBean() throws Exception { isUnsafe = true; @@ -174,12 +205,20 @@ public void validate(String stdout, String stderr, int retcode, String jfrFile) @Test public void testOnExit() throws Exception { - timeout = 3500; + // @OnExit can only fire once the target JVM exits, and the harness signals that shutdown only + // after the completion gate returns - so waiting for "onexit" here can never be satisfied and + // always burns the full timeout. Worse, when the machine is loaded the client may still be + // attaching when that timeout expires, and the teardown then pulls the target out from under + // it: the client reports "No such process" on stderr and the run fails. Gate on the probe + // being live instead, which is both deterministic and quicker. testDynamic( "resources.Main", "btrace/OnExitTest.java", - Completion.untilContains("onexit"), + Completion.untilContains(PROBE_STARTED_MARKER), (stdout, stderr, retcode, jfrFile) -> { + assertTrue( + stdout.contains(PROBE_STARTED_MARKER), + "probe did not start before the target was torn down; stdout: " + stdout); assertFalse(stdout.contains("FAILED"), "Script should not have failed"); assertTrue(stderr.isEmpty(), "Non-empty stderr"); assertTrue(stdout.contains("onexit")); @@ -272,21 +311,7 @@ public void locateTracePrefersCurrentBuildOutput() { @Test public void testTraceAll() throws Exception { - String testJavaHome = System.getenv().get("TEST_JAVA_HOME"); - if (testJavaHome == null) testJavaHome = System.getenv().get("JAVA_TEST_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getProperty("java.home"); - } - } - - assumeFalse(testJavaHome == null); - - Properties releaseProps = new Properties(); - releaseProps.load( - Files.newInputStream(new File(testJavaHome + File.separator + "release").toPath())); - String rtVersion = releaseProps.getProperty("JAVA_VERSION").replace("\"", ""); + String rtVersion = targetJdkVersion(); if (!isVersionSafeForTraceAll(rtVersion)) { System.err.println("Skipping test for JDK " + rtVersion); return; @@ -444,15 +469,7 @@ public void validate(String stdout, String stderr, int retcode, String jfrFile) @Test public void testJfr() throws Exception { - String rtVersion = System.getProperty("java.runtime.version", ""); - String testJavaHome = System.getenv().get("TEST_JAVA_HOME"); - if (testJavaHome == null) testJavaHome = System.getenv().get("JAVA_TEST_HOME"); - if (testJavaHome != null) { - Properties releaseProps = new Properties(); - releaseProps.load( - Files.newInputStream(new File(testJavaHome + File.separator + "release").toPath())); - rtVersion = releaseProps.getProperty("JAVA_VERSION").replace("\"", ""); - } + String rtVersion = targetJdkVersion(); if (!isVersionSafeForJfr(rtVersion)) { // skip the test for 8.0.* because of missing support // skip all non-LTS versions (except the last one) diff --git a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java index dc0a08059..86d72ef79 100644 --- a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java +++ b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java @@ -48,6 +48,16 @@ class Issue884PublishedFatAgentE2ETest { private static final Pattern CATALOG_ASM_VERSION = Pattern.compile("version\\s*\\(\\s*'asm'\\s*,\\s*'([^']+)'\\s*\\)"); + /** + * Budget for the nested Gradle builds this test forks. + * + *

Each fork resolves plugins and dependencies and may compile from a cold cache. 90 seconds + * was enough on a warm, idle machine but expired routinely on a loaded one, surfacing as + * "publication did not finish" rather than as anything diagnostic. This bounds a hung build + * without racing a merely slow one. + */ + private static final long NESTED_BUILD_TIMEOUT_SECONDS = 300; + @TempDir Path temporaryDirectory; @BeforeAll @@ -147,7 +157,9 @@ private void publishPluginArtifacts(Path root, Path repository) throws Exception .directory(root.toFile()) .redirectErrorStream(true) .start(); - assertTrue(process.waitFor(90, TimeUnit.SECONDS), "plugin publication did not finish"); + assertTrue( + process.waitFor(NESTED_BUILD_TIMEOUT_SECONDS, TimeUnit.SECONDS), + "plugin publication did not finish"); String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); assertTrue(process.exitValue() == 0, output); } @@ -251,7 +263,9 @@ private Path buildExternalConsumer(Path root, Path repository) throws Exception .directory(root.toFile()) .redirectErrorStream(true) .start(); - assertTrue(process.waitFor(90, TimeUnit.SECONDS), "external consumer did not finish"); + assertTrue( + process.waitFor(NESTED_BUILD_TIMEOUT_SECONDS, TimeUnit.SECONDS), + "external consumer did not finish"); String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); assertTrue(process.exitValue() == 0, output); assertTrue(output.contains("Staged masked BTrace engine"), output); diff --git a/integration-tests/src/test/java/tests/RuntimeTest.java b/integration-tests/src/test/java/tests/RuntimeTest.java index afad9a18f..31fe2a1f7 100644 --- a/integration-tests/src/test/java/tests/RuntimeTest.java +++ b/integration-tests/src/test/java/tests/RuntimeTest.java @@ -115,6 +115,33 @@ public abstract class RuntimeTest { private ProtocolSettings clientProtocolSettings = FORCED_V2_PROTOCOL; private ProtocolSettings agentProtocolSettings = FORCED_V2_PROTOCOL; + /** + * Resolves the JDK that the target application and the BTrace client run under. + * + *

{@code TEST_JAVA_HOME} wins, then its {@code JAVA_TEST_HOME} alias, then {@code JAVA_HOME}, + * and finally the JVM running the tests. Every call site used to inline this chain with a + * different tail - most omitted {@code java.home}, one also omitted the alias - so a bare + * environment failed in some code paths while succeeding in others. + * + * @return the resolved JDK home + */ + protected static String resolveTestJavaHome() { + String[] candidates = { + System.getenv("TEST_JAVA_HOME"), + System.getenv("JAVA_TEST_HOME"), + System.getenv("JAVA_HOME"), + System.getProperty("java.home") + }; + for (String candidate : candidates) { + if (candidate != null && !candidate.trim().isEmpty()) { + return candidate; + } + } + throw new IllegalStateException( + "Cannot resolve a JDK for the integration tests: none of TEST_JAVA_HOME, JAVA_TEST_HOME," + + " JAVA_HOME or the java.home system property is set"); + } + public static void classSetup() { if (System.getProperty("btrace.comm.protocol") == null) { System.setProperty("btrace.comm.protocol", "2"); @@ -161,22 +188,7 @@ public static void classSetup() { Assertions.assertNotNull(clientClassPath); String toolsjar = null; - // Accept both TEST_JAVA_HOME (preferred) and JAVA_TEST_HOME as aliases - // TEST_JAVA_HOME has the highest precedence - javaHome = System.getenv("TEST_JAVA_HOME"); - if (javaHome == null) { - javaHome = System.getenv("JAVA_TEST_HOME"); - } - if (javaHome == null) { - javaHome = System.getenv("JAVA_HOME"); - } - if (javaHome == null) { - javaHome = System.getProperty("java.home"); - } - if (javaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } - javaHome = javaHome.replace("/jre", ""); + javaHome = resolveTestJavaHome().replace("/jre", ""); Path toolsJarPath = Paths.get(javaHome, "lib", "tools.jar"); if (Files.exists(toolsJarPath)) { @@ -304,14 +316,7 @@ public void testDynamicOneliner( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); System.out.println("===> test java: " + testJavaHome); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", cp)); @@ -500,14 +505,7 @@ public void testDynamic( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); System.out.println("===> test java: " + testJavaHome); String jfrFile = null; List args = @@ -702,14 +700,7 @@ public void testStartup( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", targetAppCp)); @@ -1046,11 +1037,7 @@ private TestApp launchTestAppInternal( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", cp)); if (attachDebugger) {