Skip to content

Reference chains: recycled leak-tag identity and Java integration tests - #800

Merged
jbachorik merged 28 commits into
mainfrom
jb/rc-6-java-tests
Oct 19, 2026
Merged

jbachorik merged 28 commits into
mainfrom
jb/rc-6-java-tests

Conversation

@jbachorik

@jbachorik jbachorik commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?:

Java integration-test layer for reference chains, plus a production identity fix for recycled leak tags discovered by hardening tests.

Production: recycled leak-tag identity

Problem. Leak tags are recycled once their object dies. A cached chain matched by tag alone can belong to a previous owner of that tag, so the ReferenceChain.targetTag → HeapLiveObject.leakTag join attributes a dead object's chain to the tag's new owner.

Fix.

  • LivenessTracker keeps a per-slot ownership-period stamp, bumped on every ownership transition. The bump reads the stamp with an atomic load — a plain read raced the lock-free acquire readers.
  • The frontier entry and the cached chain capture the stamp when the tag is bound.
  • Pending-chain emission drops a chain whose target tag has since been re-acquired by another object.

Related fix. Adopting a class-map generation (the start() rebuild and the population-reset seam) invalidates the tracking table's cached class IDs. IDs from the wiped dictionary namespace would alias unrelated classes.

Test seams (DEBUG builds only)

All serialized on _engine_lock, so they never observe the BFS thread mid-mutation:

  • frontier state — an object's actual expansion state
  • resolved target tag — leak-tag lookups verify the ownership stamp, not just tag equality
  • pending position — an object's position in the search's pending work
  • class-map re-baseline — adopt the current generation after seeding

The heap-floor ring (the aggregate heap-usage history behind the urgent-OOM gate) has its own lock-free seeding seams outside the engine lock: seed samples, fake max heap, suppress real-GC recording, and reset the ring between tests. The reset refuses to run while recording is enabled — the two index stores are not atomic as a pair, so the recording-disabled precondition is the only thing that excludes a concurrent reader.

Test layer

All assertions go through the public profiler API:

Area What it proves
End-to-end leak scenarios a thread-local leak (incl. a thread that starts leaking only after profiling starts) and a static-field leak growing across recording rounds
Leak-tag correlation a chain recorded in one recording matches the object observed in the next, and the correlation keeps the best-evidenced scan instead of the latest one
External process the profiler runs in a fresh JVM, so the pass cannot depend on in-process state
Engine scheduling gates incl. the urgent-OOM bypass, asserted on the BFS thread because the one-shot urgency flag races a direct query; the ring reset itself is asserted through a rising-then-empty read
JFR parsing reference-chain events parse as recorded; the roundtrip gtest writes the seeded values to a sidecar next to the recording, so the producer and the Java parser assert against one set of constants
C++ gtests stamp semantics: ownership transitions, invalidation, restart identity, stale-search rejection, dead-owner chains refused even after the tag is recycled

The scenario infrastructure fails loud on absent identity fields (a missing leakTag/leakTagIncarnation accessor throws instead of degrading the correlation to tag-only matching) and attributes persistent-allocator outcomes per run via a generation token, so an abandoned batch's result can never be attributed to the next one.

Motivation:

The engine and JFR layers landed in earlier PRs with unit tests only. The recycled-tag identity problem surfaced while writing the correlation tests: a pass built on numeric tag equality can match another object's chain. That is why the fix is production code, not test scaffolding.

Additional Notes:

  • The external-process static-field scenario sets its own search cadence (painbudget=100). The production default (5% of wall-clock) can starve the search engine on a loaded host for longer than the scenario's observation window, which made the test a load lottery.
  • 4 pre-existing macOS test failures (NativeThreadTest, DynamicNativeThread, ThreadEntryDetectionTest, VtableReceiverFrameTest) are unrelated and reproduce on a clean tree.

How to test the change?:

./gradlew :ddprof-test:testDebug      # fast suites
./gradlew :ddprof-test:testSlowDebug  # @Tag("slow") scenario tests

ReferenceChainJfrParserTest assumes the roundtrip gtest's recording; run :ddprof-lib:gtestDebug_referenceChainJfrRoundtrip_ut first (or any full testDebug, which orders the gtests ahead of the Java tests), otherwise the test is skipped with an assumption.

For Datadog employees:

  • This PR doesn't touch any of that.
  • JIRA: PROF-15341

@dd-octo-sts

dd-octo-sts Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmlun5p
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Thu Sep 17 20:12:32 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs4
C++ move semantics
Use-after-move1
Logic error
Dereference of null pointer1
Result of operation is garbage or undefined1
Unused code
Dead increment1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Unused codeDead incrementreferenceChains.cppcollectStaticFieldAnchorsForRotation36771
Logic errorDereference of null pointerfaultInjection.cppcrashNow242
Logic errorResult of operation is garbage or undefinedlivenessTracker.cppsecondsToOOM138324
C++ move semanticsUse-after-movereferenceChains.cppbuildCanaryChainEvent608368

@datadog-prod-us1-5

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #39299639079 | Commit: 6270d41 | Duration: 18m 8s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-amd64/debug
8 - ✅ - -
8-ibm - ✅ - -
8-j9 ✅ ✅ - -
8-librca - - ✅ ✅
8-orcl - ✅ - -
11 - ✅ - -
11-j9 ✅ ✅ - -
11-librca - - ✅ ✅
17 ✅ ✅ - -
17-graal ✅ ✅ - -
17-j9 ✅ ✅ - -
17-librca - - ✅ ✅
21 ✅ ✅ - -
21-graal ✅ ✅ - -
21-librca - - ✅ ✅
25 ✅ ✅ - -
25-graal ✅ ✅ - -
25-librca - - ✅ ✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-10-19 18:27:17 UTC

@dd-octo-sts

dd-octo-sts Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

✅ All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 4feba62d

@jbachorik
jbachorik added this pull request to stack #803 September 17, 2026 20:02
…ne_lock

frontierStateForTest() read _frontier with no lock while sibling seams take _engine_lock; resolvedChainTargetTagForTest() sampled _search_start_ns before taking _resolved_chains_lock. Take _engine_lock -> _resolved_chains_lock in both, cover the states, sentinels and lock ordering with gtests, and document the retired EDGE state and the -DDEBUG seam set.
…-opens on JDK 8

The scenario's native seams compile into debug/asan/tsan, so enable the seam path on all three (asserted via a stdout marker) instead of config==debug only. --add-opens is rejected by JDK 8's launcher, so add it only on JDK 9+.
Tail now watches a fresh chunk rotated on an 8-attempt maturation window,
re-pins the representative each attempt, and raises the safepoint pain
budget (painbudget=100) so a loaded box cannot freeze the search engine
for the whole tail window.
Trim comments across the branch to non-obvious intents and whys: remove
ticket/plan references, development-history wording, duplicated rationale
blocks, restatements of adjacent code, and dangling half-written fragments;
fix wrong cross-references. No code changes.
- Serialize leakTagIncarnation into HeapLiveObject and ReferenceChain so
  consumers join on (tag, stamp) across chunks; tags alone are recycled
- Erase chains whose leak-tag owner died during drain and reject them in
  cacheResolvedChain: a stamp mismatch can never become valid again
- Revert test seams to package-private; forward through ReferenceChainTestHooks
  in ddprof-test; restore the not-read-only warning on shouldRunPassForTest0
- Count drained chains per leaf class (drainReferenceChainEventCountForLeaf0)
- Accept debug and *san builds for test-hook gating (asan/tsan nightlies)
- Fix cache-leak test flake: persistent allocator thread, matched-scale seeds
- Verify owner identity in leak-tag correlation assertions
- Process-test hardening: bounded waits, synchronized log tail, child timeout
  budget, destroy on interrupt; copyright header
Both the referenceChainJfrRoundtrip_ut gtest and the ddprof-test
ReferenceChainJfrParserTest get the shared .jfr path from
DDPROF_REFERENCE_CHAIN_ROUNDTRIP_JFR; the producer deletes the file
before running so the consumer never parses a stale recording.
The shared path let a multi-configuration invocation delete or
overwrite another configuration's recording; both producer and
consumer now derive the configuration from the task name. Also
fail the build when no gtest*_referenceChainJfrRoundtrip_ut task
matches, instead of the parser test silently degrading to a skip.
@jbachorik
jbachorik requested a review from rkennke October 19, 2026 12:05
@jbachorik

Copy link
Copy Markdown
Collaborator Author

@DataDog review

@datadog-prod-us1-5 datadog-prod-us1-5 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The new allocation helper can discard completion signals when its worker finishes before the caller starts waiting, causing intermittent 30-second integration-test failures. A buffered completion queue preserves both task results and shutdown acknowledgements.

Open Bits AI session

🤖 Bits Code Review · Commit 981568f

A SynchronousQueue drops the worker's bare offer() when the batch
finishes between the handoff rendezvous and the caller entering
poll(), turning a completed batch (or its rethrown failure) into a
30-second poll timeout. A one-slot ArrayBlockingQueue retains it;
the protocol is single-in-flight so one slot suffices.
bumpLeakTagIncarnationLocked() read _leak_tag_incarnation with a plain
read while lock-free acquire readers race it; use an atomic load for the
read half of the release-store bump. Add a heapFloorResetForTest() seam
(empty head/fill, storeRelease like the producer) with JNI wiring, so the
AggressiveLeak tests can clear the seeded ring instead of leaving ~991MB
samples for later tests in the shared JVM. Correct getTid()'s javadoc:
getTid0() is compiled unconditionally (outside javaApi.cpp's #ifdef
DEBUG seam block) and works in release builds too.
Expose cacheResolvedChain()'s verdict through the test accessor and
assert dead-owner chains are refused (also after the tag is recycled for
a new owner with the dead owner's stamp). Add
FrontierChainFromSupersededSearchIsRejected: a frontier chain cached by
an earlier search must not answer the seam after a restart bumps
_search_start_ns, and nothing answers with no active search. Add
GenerationChangeClearsCachedKlassIdsWholeTable: every row below
_table_size clears its cached class id on a class-map generation change
and a canary row at _table_size stays untouched, pinning the loop bound.
PersistentAllocator.run() clears the completion slot before the handoff
so a stale outcome from a timed-out batch can never be attributed to the
next task (the worker is parked at handoff.take() at that point). The
AggressiveLeak tests reset the heap-floor ring in finally before
re-enabling recording. ThreadLocalLeakScenario: keepBest keeps an
in-pool chain over a later out-of-pool one, and an unresolved -1
incarnation fails loud or is a hard mismatch instead of degrading to
tag-only correlation. LeakTagCorrelationScenario: keepBest ranks scans
by the same evidence score scan() uses (owner-verified 2, in-pool 1),
so later degraded scans cannot displace the best correlation.
The roundtrip gtest writes its seeded values (class names, edge labels,
target tag, depth, incarnation) to a .expected sidecar next to the JFR
and the Java parser test asserts against it, removing the duplicated
hard-coded expectations across the two modules. Add
ReferenceChainAssertions.requiredTestConfig(): child-JVM launchers fail
loud when ddprof_test.config is unset instead of forwarding the literal
null that the child would classify as a release build.
ExternalProcessReferenceChainTest derives its expected seam path from
isDebugInstrumentedBuild(), the same single encoding the child uses.
heapFloorResetForTest() published two independent storeReleases, so a
concurrent reader could observe a torn old-fill/new-head window, and no
store ordering closes that (seeing the first store's new value while the
second is still old is possible either way). The documented precondition
- heap-floor recording disabled - is what excludes every reader, so
enforce it in DEBUG builds (where the flag exists): reset while
recording is enabled fails an assert instead of racing silently. Expose
heapFloorRisingForTest0() (JNI + hooks) and add
shouldEmptyHeapFloorRingOnReset(): rising-then-empty through the real
heapFloorRising() read, killing a no-op'd reset that previously survived
the whole suite unobserved.
completion.clear() before the handoff was a timing-only guard: after a
run() that timed out, the worker is not parked at handoff.take() yet, so
the abandoned task's outcome could land after the clear and be
attributed to the next task. Stamp each run with a monotonic generation
carried through the Job/Outcome pair and discard stale generations at
poll time; the stale entry is always offered before the current one's
verdict because the worker offers it before parking at take(), through
which alone the new Job is accepted.
Both leak-correlation scenarios silently degraded a missing leakTag (or,
in LeakTagCorrelation, leakTagIncarnation) accessor to -1, yielding an
empty correlation with no diagnostic - the failure mode the fail-loud
guards were added to prevent. Both scenarios now throw when either
identity accessor is absent. Consolidate LeakTagCorrelationScenario's
owner-verified/in-pool/out-of-pool ranking into evidenceRank() (used by
evidenceScore(), the within-scan candidate selection, and cross-referenced
by the marker cascade) so the three sites cannot drift apart. Document
that ThreadLocalLeakScenario's keepBest has nothing to merge from a
losing scan (no transient/noise evidence fields in that scenario).
start() rebuilds the leak-tag pool by comparing every
_leak_tag_in_use[i] against the table's owners; the singleton is
static-storage and zero-initialized regardless, but a non-statically
constructed tracker would read indeterminate bools there. Value-init
the array in the constructor's init list, matching _leak_tag_incarnation.
The worker's completion.offer() could silently drop THIS task's own
outcome: with one slot, a stale entry left by an abandoned (timed-out)
run() can still occupy the queue when the current task finishes, and the
dropped outcome turns a completed task into the caller's 30s poll
timeout. Use put() instead - the caller's generation loop drains the
stale entry, unblocking the worker; the same applies to close()'s exit
acknowledgement, which close()'s drain unblocks.

keepBest(): drop the dead other != null check (both scans are non-null
after the early returns). The roundtrip sidecar readers now name the
offending key and value instead of surfacing a bare
NumberFormatException (or "null" for a missing key).
Code bodies referenced java.util.Properties,
org.openjdk.jmc.common.{item,unit} types, java.lang.reflect.Method and
java.lang.management.ManagementFactory inline; import them and use the
simple names instead. Javadoc prose and message strings naming JDK
internals (chain path descriptions) are untouched.
@jbachorik
jbachorik merged commit dfc1ac6 into main Oct 19, 2026
116 checks passed
@jbachorik
jbachorik deleted the jb/rc-6-java-tests branch October 19, 2026 19:51
@github-actions github-actions Bot added this to the 1.52.0 milestone Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants