fix: restore three silently reverted fixes and finish JAVA_HOME handling - #921
Merged
Merged
Conversation
…M 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) <noreply@anthropic.com>
…OME (#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) <noreply@anthropic.com>
…SM 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) <noreply@anthropic.com>
…lution 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change do?
Restores three fixes that had been merged and were then silently overwritten by release-prep commit
f298251e, which was authored against a stale checkout. Adds a guard for each so the next accidental overwrite fails the build instead of shipping. Also finishes off theJAVA_HOMEhandling that #905 started.1. Per-client settings isolation (agent) — the significant one
RemoteClientapplies a client'sSET_PARAMSby mutating itsClientContext's settings in place. The accept loop was handing every remote connectionSharedSettings.GLOBAL, so one client'sdebug,dumpDir,outputFile, granted permissions andtrustedflag reached every other client and the global transformer.SharedSettings.from(Map)ORstrustedin and never clears it, so such a leak is permanent for the lifetime of the agent. Authentication only exists in prepared mode, so on a normal attach nothing gates this.The local/premain path had always isolated correctly; only the remote path regressed.
ClientContextnow rejectsSharedSettings.GLOBALoutright, making the bug class unrepresentable rather than merely absent.Main#newRemoteClientContext(). This matters: inside the loop it sits undercatch (RuntimeException | IOException), which would downgrade a re-introduced sharing bug to a per-connection warning rather than a test failure.2. Command-queue processor stack size (agent)
Restores the 4 MB stack on class-file major >= 70, where ClassFile API's
StackMapGeneratorcan exhaust the default depth; older JVMs keep the JVM-chosen default, which was deliberate to avoid the startup delays seen on JDK 8/11/21 CI machines. Not a plain revert: a later commit made theThreadFactoryan anonymous class rather than a lambda for indy-freedom (BootstrapPathIndyFreedomTest), and that is preserved. The sizing policy is now a pure function so it can be tested across versions.3. Agent argument construction (client)
The agent parses each comma-separated entry by splitting on
=and matching the key, so a misplaced separator is not rejected — the entry lands under an empty key and the setting is dropped with no diagnostic. That is howcmdQueueLimitwas lost. All 16 entries now go through a singleappendAgentArg(..)helper that rejects malformed keys.4. ASM version (gradle plugin)
Back to 9.10.1, matching
version('asm', ..)in the rootsettings.gradle. The plugin is an included build and genuinely cannot read the catalog, soAsmVersionSyncTestcompares the two files and fails on drift. The stale "match versions commonly cached in this repository" comment was itself an artifact of the overwrite and is gone.5.
JAVA_HOMEno longer decides console handling (client)#905 stopped the startup crash but kept
JAVA_HOMEas the primary source. That is the wrong source: the Java 22 stdout-to-stderr quirk belongs to the JVM running the client, andJAVA_HOMEneed not point at it — withJAVA_HOME=<jdk17>and the client on JDK 22, the check read 17, skipped the workaround, and reproduced the bug it exists to prevent.getJavaVersion()is deleted in favour of the existingJavaVersionCheck.javaFeatureVersion(). This removes the remaining failure modes rather than guarding them: no file read, so noInvalidPathException, and nogetProperty("JAVA_VERSION").replace(..)NPE — both were unchecked throws inside a static initializer, where they surface asExceptionInInitializerErrorand stop the CLI from starting. Behaviour is unchanged wherever it was already correct.Related issue
Closes #920
Closes #887
Scope and compatibility
Compatibility or migration notes:
No API, CLI, probe, extension, protocol or packaging change. Two behavioural changes, both correcting existing bugs: remote clients no longer observe each other's
SET_PARAMS, and console selection follows the running JVM rather thanJAVA_HOME.Testing
Integration coverage was run specifically because the
agentArgsrefactor touches every attach;testDynamicAttachWithShippedProtocolDefaultsand the V1/V2 negotiation tests exercise that path end to end.Each new guard was confirmed to fail against the pre-fix code, not merely to pass against the fixed code:
remoteContextDoesNotShareGlobalSettingsandremoteContextsAreIndependentasmVersionMatchesRootCatalogNew tests:
ClientSettingsIsolationTest,QueueProcessorStackSizeTest,ClientAgentArgsTest,ConsoleSelectionTest,AsmVersionSyncTest.Notes for reviewers
ClientAgentArgsTestpins the historical malformed shape too, documenting why the guard exists.cmdQueueLimitconflict was resolved in favour of the helper form so all call sites stay uniform.RuntimeTest.java:307-314hard-requiresTEST_JAVA_HOME/JAVA_HOMEwith nojava.homefallback, unlikeRuntimeTest.java:166-175which has one. Pre-existing, and it means the integration suite cannot run in the bare environment the unit suite now tolerates.🤖 Generated with Claude Code
This change is