Skip to content

Fix a batch of correctness, concurrency, and resource bugs across all modules#857

Merged
jbachorik merged 8 commits into
developfrom
claude/core-runtime-analysis-6qck5p
Jul 5, 2026
Merged

Fix a batch of correctness, concurrency, and resource bugs across all modules#857
jbachorik merged 8 commits into
developfrom
claude/core-runtime-analysis-6qck5p

Conversation

@jbachorik

@jbachorik jbachorik commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

A wide bug review across the whole codebase (core, runtime, agent/instr, compiler, client, dtrace, boot, MCP server) turned up a set of correctness, concurrency, and resource-management bugs. This PR fixes them. Each change was compiled per-module against the real dependencies (ASM 9.10.1, jctools 4.0.6, lanterna 3.1.5, slf4j, JDK compiler APIs) and the changed files are formatted with google-java-format.

Two consolidated analysis reports with the full findings (including deferred/latent items) are included under internal/analysis/.

⚠️ Action required before merge — regenerate instrumentation golden files.
The three instrumentor fixes in the last commit change the bytecode the instrumentors emit, so the btrace-instr golden data under btrace-instr/src/test/resources/instrumentorTestData/ will no longer match. Please regenerate and commit it:

./gradlew test -PupdateTestData
# then commit the updated files under btrace-instr/src/test/resources/instrumentorTestData/

I couldn't run this in my environment (the full distribution build needs the JDK 8/11 toolchains, and Gradle distribution / toolchain downloads were blocked). Everything else was verified by direct compilation and, where feasible, standalone behavioral tests.

What's fixed

Correctness — silently wrong results

  • @Level gating never worked. The MethodHandle guard read the thread-local runtime (the level-0 dummy at guard time) instead of the owning probe's runtime. Guards now resolve the probe's runtime by name. (BTraceRuntimes, HandlerRepositoryImpl)
  • @OnError handlers never ran and errors were swallowed. handleException invoked guarded error handlers while the thread was still entered, so their enter() prologue failed. They're now dispatched with the runtime escaped, like handleEvent. (BTraceRuntimeImplBase)
  • Profiler: snapshotAndReset() never reset, reset() corrupted the recorder when threads were mid-method (NPE), and direct recursion double-counted wall time. Fixed and covered by MethodInvocationRecorderTest.
  • @OnLowMemory was broken end to end (reflective arity in LowMemoryHandler, the runtime call site, and the Preprocessor codegen: constructor descriptor + trackUsage, array element type, null-LDC).
  • Compiler hung forever on any enum-declared method call (e.g. TimeUnit.NANOSECONDS.toMillis(x)) — the verifier's parent-type loop never advanced. (VerifierVisitor)
  • @Duration on a Kind.CALL handler always received 0, and adaptive-sampled CALL sites lost their timing feedback — onAfterCallMethod shadowed the timing context configured by onBeforeCallMethod. (Instrumentor)
  • Nested new (new Outer(new Inner())) missed Kind.NEW/@Return — a single boolean collapsed under nesting; now tracked on a stack. (ObjectAllocInstrumentor)
  • LineNumberInstrumentor reported line-1 instead of the actual previous line for non-consecutive line tables.
  • @TLS short / @Export short fields failed with a VerifyError (TypeUtils.isPrimitive omitted short).

Protocol / wire

  • V1 could not decode LIST_FAILED_EXTENSIONS (a command the client sends first), killing the session on both ends; also now throws IOException instead of a bare RuntimeException on unknown types.
  • V1 ObjectOutputStream reset/flush were unsynchronized against the command write, tearing the stream under concurrent writers.
  • Agent negotiation had no read timeout, so one idle/half-open connection wedged the single accept thread for all clients.
  • DTraceDropCommand.write serialized the stream instead of the event (out.writeObject(out)).
  • -Dio.btrace.core.cmdQueueLimit was silently dropped by the client (malformed agent-arg segment).

Concurrency

  • CommandQueue.enqueue cleared the producing application thread's interrupt status and could drop commands while reporting success.
  • SharedSettings.GLOBAL leaked across clients (settings and a sticky trusted escalation); each remote client now gets its own copy, and the fields are volatile.
  • CircularBuffer re-delivered already-consumed commands after wrap-around and had no synchronization; rewritten and covered by tests.
  • ExtensionLoaderImpl re-opened/re-appended the API jar on every call (FD/path leak) and used plain HashMaps across threads.
  • Agent Client.WRITER_MAP (plain HashMap) was mutated from multiple threads; now guarded.
  • MethodTracker sampling arrays made volatile (hot readers vs. resize).

Resource management

  • Agent leaked a socket per non-instrument connection (-lp, -le, EXIT, malformed handshake).
  • MCP server crashed the whole process on one malformed JSON line, treated blank lines as EOF, and leaked client sockets in the list/exit/detach handlers.
  • CompilerHelper permanently set GLOBAL trusted and leaked a URLClassLoader per compiled class.

Robustness / API

  • Perf counters (perfInt/perfString) were unreachable — the jvmstat availability probe used a broken relative resource path and the reader class was package-private. Fixed the probe and made it instantiable.
  • JDK 11 JFR factory returned null where 8/9 return the EMPTY factory, NPE-ing probes on JFR-less JDK 11+.
  • @OnProbe-mapped Location was shared with a process-wide-cached template and mutated in place across sessions; now deep-copied.
  • BTraceTransformer regex-probe unregistration used a mismatched cache key, leaking filter entries.
  • Oneliner args[N] == null filter NPE, client CLI hang on an empty argument token, and a PolicyFile.save NPE on a bare relative path.

Deferred (documented in the analysis reports, not in this PR)

The Maven/Gradle plugin extension-packaging mismatches are packaging changes best validated with an actual plugin build; they're described in internal/analysis/2026-07-05-remaining-modules-analysis.md.

Commits

  1. Wide analysis report for core & runtime
  2. High-severity core/runtime fixes (16)
  3. Fixes across agent/instr, compiler, client, dtrace, MCP (16) + remaining-modules report
  4. Instrumentor fixes (CALL @Duration, nested-new alloc, line-number off-by-one) — requires golden-file regeneration

Testing

  • Per-module compilation against real dependencies on JDK 21 (all touched modules clean).
  • New JUnit test: MethodInvocationRecorderTest (profiler reset/recursion semantics).
  • New CircularBufferTest cases (no stale re-delivery, partial consumption, doNext).
  • Standalone behavioral verification of the CircularBuffer, profiler recorder, @Level MethodHandle composition, and nested-new allocation logic.
  • Not yet run: the full instrumentation golden-file suite — see the action-required note above.

🤖 Generated with Claude Code


This change is Reviewable

claude added 2 commits July 4, 2026 18:15
Consolidated findings from a full review of both modules (plus their
callers in btrace-agent/btrace-client/btrace-instr), covering outright
bugs, concurrency defects, cross-module inconsistencies, and API issues.

16 high-severity findings, including:
- @Level instrumentation guards always reading level 0 (dummy runtime)
- handleException enter/leave inversion silencing @onerror and errors
- CommandQueue.enqueue clearing app-thread interrupt status
- @OnLowMemory broken end-to-end (4 stacked defects)
- CircularBuffer stale-slot re-delivery + missing synchronization
- SharedSettings.GLOBAL cross-client leakage and sticky trusted flag
- Profiler snapshotAndReset never resetting; reset() corrupting state
- V1 protocol unable to decode LIST_FAILED_EXTENSIONS
- unsynchronized OOS reset tearing the V1 stream
- agent-side negotiation lacking a timeout (accept-thread wedge)
- embedded extensions bypassing the permission gate
- JDK11 JFR factory returning null where 8/9 return EMPTY
- perf counter built-ins unreachable by construction

Plus 40 medium and 31 low findings with file:line references and
concrete failure scenarios, a verified-clean list, and triage order.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qv2d3CN4yGzZAmXA7TXRvC
Addresses the 16 high-severity findings from the core/runtime analysis:

Correctness (silent wrong behavior):
- @Level guards: query the owning probe's runtime level by name instead
  of the thread-local (which is the level-0 dummy at guard time), so
  enableAt/setInstrumentationLevel gating works (BTraceRuntimes,
  HandlerRepositoryImpl).
- handleException: dispatch guarded @onerror handlers with the runtime
  escaped (doWithCurrent) so they can enter; errors no longer swallowed.
- Profiler recorder: honor the reset flag in getRecords (snapshotAndReset
  now resets), keep the stack consistent on reset (no NPE when threads are
  mid-method), and fix the direct-recursion off-by-one that double-counted
  wall time. Adds MethodInvocationRecorderTest.
- @OnLowMemory: fix reflective invocation arity in LowMemoryHandler and
  the runtime call site, and fix the Preprocessor emission (constructor
  descriptor + trackUsage, array element type, null-safe threshold LDC).

Concurrency / resource safety:
- CommandQueue.enqueue: don't clear the producing app thread's interrupt
  status; always attempt the offer; count drops. Adds CircularBuffer fix
  (correct ring semantics, no stale re-delivery) + synchronization, with
  tests.
- SharedSettings: per-client copy in the remote accept loop (no cross-client
  leak / sticky trusted escalation), volatile fields, non-downgrading
  legacy unsafe key.
- ExtensionLoaderImpl: dedup bootstrap API-jar appends (no FD/path leak),
  concurrent maps, guarded openApiJars.

Protocol / lifecycle:
- WireIO V1: decode LIST_FAILED_EXTENSIONS; throw IOException (not
  RuntimeException) on unknown types.
- JavaSerializationProtocol: synchronize reset/flush with the command write
  to avoid tearing the V1 stream.
- Agent negotiation: bound handshake reads with a socket timeout so an idle
  connection cannot wedge the accept thread.

Extensions / modern JDK:
- Embedded extensions: honor the declared permissions property (no consent
  gate bypass).
- BTraceRuntimeImpl_11.createEventFactory: return the EMPTY factory when JFR
  is unavailable (matches 8/9) instead of null.
- Perf reader: detect jvmstat via a real class-load probe and make
  PerfReaderImpl instantiable across packages.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qv2d3CN4yGzZAmXA7TXRvC
return true;
}
try {
JarFile apiJarFile = new JarFile(apiJar.toFile());
claude added 2 commits July 5, 2026 07:10
Second wide-review round covering every production module outside
core/runtime. Fixes applied (compiled per-module against real deps on
JDK 21; changed files google-java-formatted):

HIGH:
- VerifierVisitor: enum-declared method calls (e.g. TimeUnit.toMillis)
  hung the compiler forever; resolve the declaring type once.
- DTraceDropCommand.write serialized the stream instead of the event
  (out.writeObject(out) -> out.writeObject(de)).
- Agent leaked a socket per non-instrument connection (EXIT, -lp, -le,
  malformed handshake); the accept loop now closes sockets it doesn't
  hand off to a live client.
- Client dropped -Dio.btrace.core.cmdQueueLimit via a malformed agent-arg
  segment (",=key value" -> ",key=value").
- MCP server died on one malformed JSON line and treated blank lines as
  EOF; parse errors now return JSON-RPC -32700 and the loop continues.

MEDIUM:
- Client CLI hung on an empty argument token (continue skipped count++).
- Agent Client.WRITER_MAP (plain HashMap) mutated cross-thread; guarded.
- @TLS/@export short fields VerifyError'd (isPrimitive missed 'S').
- @OnProbe-mapped Location shared across sessions; deep-copy in copyFrom
  (added a Location copy constructor).
- CompilerHelper permanently set GLOBAL trusted and leaked a
  URLClassLoader; save/restore + try-with-resources.
- MCP list/exit/detach handlers leaked client sockets; close them.
- Oneliner  filter NPE'd in the Filter constructor.
- BTraceTransformer regex probe unregistration key mismatch (add used
  replace("\\.","/"), remove used replace('.','/')).

LOW:
- MethodTracker sampling arrays made volatile (hot readers vs resize).
- PolicyFile.save guarded against a null parent path.

Deep instrumentation findings that change emitted bytecode (@duration on
Kind.CALL always 0; nested-new Kind.NEW/@return miss; LineNumber
off-by-one) and the plugin packaging fixes are documented in
internal/analysis/2026-07-05-remaining-modules-analysis.md as deferred:
they require regenerating instrumentorTestData golden files, which needs
the full dist build + JDK 8/11 toolchains.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qv2d3CN4yGzZAmXA7TXRvC
…off-by-one

These change the bytecode the instrumentors emit, so the
instrumentorTestData golden files must be regenerated (see PR notes).

- Instrumentor (CALL): onAfterCallMethod declared a local
  MethodTrackingContext that shadowed the field configured by
  onBeforeCallMethod (which holds the entry timestamp and sampler state).
  emitExit/emitTestSample ran on the fresh, un-timed local while
  emitDuration (in injectBtrace) read the field, so @duration on a
  Kind.CALL handler always received 0 and adaptive-sampled CALL sites
  lost their end-timestamp feedback. Reuse the field, matching the
  RETURN/ENTRY instrumentors.

- ObjectAllocInstrumentor: a single boolean instanceCreated collapsed
  under nested allocation, e.g.  — the inner
  <init> cleared the flag so the outer <init> was missed and
  afterObjectNew(Outer) never fired. Track pending NEW types on a stack
  (constructor args, and thus nested news, complete their <init> before
  the enclosing <init>, so initialization is LIFO; a constructor's own
  super()/this() delegation runs with an empty stack and correctly does
  not fire).

- LineNumberInstrumentor: visitLineNumber fired onAfterLine(line-1)
  instead of onAfterLine(lastLine); wrong whenever the line table is
  non-consecutive (a Kind.LINE/Where.AFTER probe on the real previous
  line was missed).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qv2d3CN4yGzZAmXA7TXRvC
@jbachorik jbachorik changed the title Add wide analysis report for btrace-core and btrace-runtime Fix a batch of correctness, concurrency, and resource bugs across all modules Jul 5, 2026
jbachorik added 2 commits July 5, 2026 17:05
…3addc)

The b33addc commit fixed a silent bug in the CALL instrumentor where
the @duration parameter always received 0 and adaptive-sampled CALL
sites lost their end-timestamp feedback (the fix reuses the
MethodTrackingContext field instead of shadowing it with a local).
This changes the emitted bytecode, so 10 golden files under
btrace-agent/src/test/resources/instrumentorTestData/dynamic/onmethod/
need to be regenerated to match the new (correct) output.

The new goldens were extracted from the CI test reports
(https://github.com/btraceio/btrace/actions/runs/28735908160) where
the test correctly produced the post-fix bytecode and only failed on
the assertEquals() against the stale golden.

Specifically updated:
- onmethod/MethodCallDuration
- onmethod/MethodCallDuration2
- onmethod/MethodCallDurationSampled
- onmethod/MethodCallDurationSampledMulti
- onmethod/MethodCallSampledAdaptive
- onmethod/leveled/MethodCallDuration
- onmethod/leveled/MethodCallDuration2
- onmethod/leveled/MethodCallDurationSampled
- onmethod/leveled/MethodCallDurationSampledMulti
- onmethod/leveled/MethodCallSampledAdaptive
…3addc) (#858)

The b33addc commit in #857 fixed a silent bug in the CALL instrumentor
where @duration always received 0 and adaptive-sampled CALL sites lost
their end-timestamp feedback. The fix reuses the `MethodTrackingContext`
field instead of shadowing it with a local, which changes the emitted
bytecode for `MethodCallDuration*` probes.

This PR regenerates the 10 golden files under
`btrace-agent/src/test/resources/instrumentorTestData/dynamic/onmethod/`
so the existing `OnMethodInstrumenterTest` assertions pass. The new
goldens were extracted from the CI test reports at
https://github.com/btraceio/btrace/actions/runs/28735908160 where the
test produced the post-fix bytecode and only failed on the
`assertEquals()` comparison against the stale golden.

Also merges `origin/develop` to pick up #851 (JDK compatibility matrix)
and #845 (JDK test version bump) and the doc-sync follow-up.

**Files updated (10):**
- `onmethod/MethodCallDuration` — now emits a second `System.nanoTime()`
call and passes the actual duration (`LLOAD 4`) instead of `LCONST_0`
- `onmethod/MethodCallDuration2`
- `onmethod/MethodCallDurationSampled` — now calls
`MethodTracker.getEndTs(2)` and uses `LSTORE 4` for the duration slot
- `onmethod/MethodCallDurationSampledMulti`
- `onmethod/MethodCallSampledAdaptive`
- `onmethod/leveled/MethodCallDuration`
- `onmethod/leveled/MethodCallDuration2`
- `onmethod/leveled/MethodCallDurationSampled`
- `onmethod/leveled/MethodCallDurationSampledMulti`
- `onmethod/leveled/MethodCallSampledAdaptive`

Once this lands, the `build` job in #857 should go green and the PR can
be merged.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg"
height="34" align="absmiddle"
alt="Reviewable"/>](https://reviewable.io/reviews/btraceio/btrace/858)
<!-- Reviewable:end -->
@jbachorik

Copy link
Copy Markdown
Collaborator Author

The Code Quality bot flagged btrace-core/src/main/java/io/btrace/extension/impl/ExtensionLoaderImpl.java:360 saying the JarFile is "not always closed on method exit". This is a false positive — the JarFile is intentionally retained in a class field, not leaked. The full lifecycle is:

1. Field declaration (line 59, with explanatory comment):

// Bootstrap-append bookkeeping, all guarded by bootstrapLock. appendedApiJars dedups the
// appendToBootstrapClassLoaderSearch calls so a hot path (one call per @Injected field per
// submitted script) does not reopen the same JAR and grow the bootstrap search path without
// bound; openApiJars keeps the JarFiles open for the loader's lifetime.
private final Object bootstrapLock = new Object();
private final Set<String> appendedApiJars = new HashSet<>();
private final List<JarFile> openApiJars = new ArrayList<>();

2. The method's own Javadoc:

The opened JarFile is retained for the loader's lifetime because HotSpot may need the descriptor to read class bytes after the append.

3. The class's close() method (lines 445–451) — which explicitly does not close the JarFiles:

@Override
public void close() {
  // openApiJars were registered with the bootstrap classloader via
  // appendToBootstrapClassLoaderSearch and must not be closed; the JVM may
  // continue reading class bytes from them after registration. The OS reclaims
  // file descriptors at JVM exit.
  openApiJars.clear();
}

This is a well-known idiom for Instrumentation.appendToBootstrapClassLoaderSearch(JarFile): the JarFile argument must remain open for the lifetime of the JVM, and closing it would corrupt the classloader's view of the bootstrap search path. The pre-PR code had the same pattern (the JarFile was created inline in ensureApiOnBootstrap and added to openApiJars); the ef953ac0 commit simply extracted that pattern into the new appendApiJarToBootstrap helper. The "leak" warning is identical in both versions.

The bot's static-analysis rule fires on new JarFile(...) without a try-with-resources in the same scope, but it cannot see that the JarFile is intentionally retained in a class field whose lifecycle is managed at the class level. Resolving as "not applicable — JarFile is deliberately retained in openApiJars for the loader's lifetime (and HotSpot requires this for bootstrap classloader search entries)".

The `diff()` method in InstrumentorTestBase returns a String with
`trim()` applied, so it has no trailing newline. The golden files
were being read with `new String(Files.readAllBytes(...))` which
preserves any trailing newline, causing `assertEquals(expected, diff)`
to fail by one character. Strip the trailing newline from each of the
10 regenerated goldens so the comparison matches.
@jbachorik
jbachorik merged commit d88e388 into develop Jul 5, 2026
13 checks passed
@jbachorik
jbachorik deleted the claude/core-runtime-analysis-6qck5p branch July 5, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants