Repository navigation
Reference chains: recycled leak-tag identity and Java integration tests - #800
Merged
Merged
Conversation
2 tasks done
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This comment has been minimized.
This comment has been minimized.
Contributor
CI Test ResultsRun: #39299639079 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-19 18:27:17 UTC |
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 19:08
1916216 to
9ede839
Compare
Contributor
jbachorik
added this pull request to stack #803
September 17, 2026 20:02
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:05
9ede839 to
9fc9b0a
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:19
9fc9b0a to
1fad8a3
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:20
1fad8a3 to
4386c2b
Compare
2 tasks done
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 21:01
4386c2b to
d64a518
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 21:33
d64a518 to
4cd8612
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
2 times, most recently
from
September 18, 2026 05:52
19e586c to
aedf5bf
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 18, 2026 06:04
aedf5bf to
cec993f
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 18, 2026 07:06
cec993f to
1b8b709
Compare
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
force-pushed
the
jb/rc-6-java-tests
branch
from
October 19, 2026 10:53
afb4457 to
981568f
Compare
Collaborator
Author
|
@DataDog review |
There was a problem hiding this comment.
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.
🤖 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
added a commit
that referenced
this pull request
Oct 20, 2026
jbachorik
added a commit
that referenced
this pull request
Oct 20, 2026
jbachorik
added a commit
that referenced
this pull request
Oct 20, 2026
…ng (#801) * Java integration tests for reference chains End-to-end leak scenarios (thread-local leaks incl. late-thread and static-field-rooted leaks), leak-tag correlation across JFR round-trips, external-process launches, the search-gate behavior, reference-chain JFR parser tests, and the JavaProfiler test-seam widening they need. * javaApi: re-baseline class-map generation in the population reset seam selectLeakCandidates() returns nothing while classMap()->generation() differs from the tracker's baseline, and that baseline is only re-synced by cleanup_table()'s GC-driven sweep. A test JVM typically runs several recordings before a seeding test, each start() bumping the generation, and no GC may land between start() and the seeded select call. After the reset seam's wipe no stale-namespace population data remains, so adopt the current generation. * Add reference-chain repro, sweep, and reporting tooling * Address sphinx review findings on rc-6/rc-7 test tooling - ReferenceChainTestHooks: consolidate debug test seams behind one package-private facade; drop public visibility of the JavaProfiler seams back to package-private - JavaProfiler: restore the mutating-state warning on shouldRunPassForTest0 javadoc; getTid javadoc corrections - referencechains tests: await handoff publication in default mode, continue hysteresis epochs past the initial seeding, keepBest() preserves in-pool correlated results, clear seeded heap-floor-ring samples and chainTargetHolder in finally, bound the allocator-tid spin-wait, seed per-(klass,tid) ramps on the allocating thread - LeakTagCorrelationReferenceChainTest: assert at least one leak-tag interception in the tight-window variant - utils/sweep-*.sh: guard parse_run pipelines with || true, fix the JDK<=16 safepoint double-count, resolve swept-knob precedence against baked-in defaults - utils/run-refchains-repro.sh: stale budget comment, knob override - build-and-summarize: headline IndexError guard, zip pairing of failure headlines, multi-count test summaries, parser failure artifacts - ReferenceChainLeakAntagonist: hoist cache.size() out of the inner loop; ReferenceChainLeakDemo: malformed-duration usage message, rotating snapshot path - refchains-report-multi.py: warn on unparsable CSV cells * ReferenceChainJfrParserTest: skip when the gtest fixture is absent The roundtrip JFR fixture is produced by referenceChainJfrRoundtrip_ut, which does not run on CI runners without Google Test (e.g. the el7 testRelease job); asserting its existence failed the job there. Assume instead of assert so the test skips where the gtest artifact is unavailable and still executes wherever the gtest ran. * Compress comment density in the reference-chain test stack Comment-only rewrite of nine reference-chain test and hook files (assertions, seams test, scenarios, launcher, test hooks, JavaProfiler seam javadocs) to match the hand-written style of the rest of the suite. No code changes. Humanize pass over jb/rc-7-tooling. * Address light-review findings on the rc-7 test stack Fixes from the sphinx light review (9 items): - LeakTagCorrelationScenario.keepBest now merges violations in both directions; resultLine extracted and covered by the new LeakTagCorrelationScenarioKeepBestTest (7 tests, no profiler needed) - AggressiveLeakReferenceChainTest: hoist shouldRunPassForTest0() out of assertFalse, reset the search first - build-and-summarize: single write_unknown_summary() writer; Gradle exit status stays authoritative - run-refchains-repro.sh: bash 3.2 set -u guards for empty arrays - ReferenceChainTrackingTest: scratch JFRs via Files.createTempFile - ReferenceChainLeakDemo: seeding in main() before run(); heapUsedFraction falls back to Runtime.maxMemory() - referenceChainJfrRoundtrip fixture is consume-once (unlink before write); ReferenceChainJfrParserTest deletes it after reading - ReferenceChainJfrParserTest: assumeTrue fixture skip, re-applied from 218c2b7 (rc-7 forked before that fix) The touched Java files also carry the humanize comment rewrites for those files; splitting them would have left comments describing code that the intermediate commit did not contain. * Drop PersistentAllocator left behind by the #800 squash * Restore the dropped reference-chain test seams and fix the drain count native name Renaming the Java declaration to drainReferenceChainEventCount0() left no JNI symbol for it in any build; rename both javaApi.cpp definitions (no-arg, no leaf filter) instead, and restore the four removed seam declarations (frontier state, resolved target tag, heap-floor reset/rising) the tests need, plus the roundtrip gtest's stale env-var comment and its unread .expected sidecar writer. * Restore reverted reference-chain test fixes and debug-build guards Replaces the literal "debug" config checks with isDebugInstrumentedBuild() (debug/asan/tsan all compile the seams) and requiredTestConfig()'s fail-loud forwarding; restores the synchronized testLogTail guards, the frontier-state EXPANDED wait and target-tag matching, the ThreadLocalMap-hop filter and (tag, incarnation) correlation, the handoff-timeout marker, painbudget=100, the scaled child timeouts, the ActiveSetting id check, the matched population seeds, the pass-count baseline ordering, the held[0] reservation, the ring-reset mutation-killer test, and the roundtrip JFR env-var-first lookup. * Keep the chaos antagonist's population growing past the cap; cap repro snapshot retention A cache that freezes at CAP_ENTRIES reads a ~zero population slope for the rest of a long run, so the antagonist now recycles 25% of the cap and regrows instead. The run-forever repro keeps only the last 5 periodic snapshots so multi-hour sessions cannot fill the temp dir. * Fix the refchains sweep scripts' aggregation and bash 3.2 compatibility Aggregate every .jfr.snapshot.<round> file (a drained event is written once, so the final snapshot alone undercounts chains), drop the swept knob from fixed REFCHAINS_ARGS before each run (last-occurrence-wins would shadow it), guard the safepoint pipelines with || true under pipefail, and replace the associative array with a case function for macOS bash 3.2. * Reduce build-and-summarize to log capture and failure extraction The wrapper had grown a live-tail/awk pipeline, a 150-line embedded log parser, and summary artifacts - machinery from an era when builds were summarized by a billed subagent call. Harnesses truncate agent output and keep the full log on disk anyway, so the agent only needs the status line plus the failure section, extracted by grep. Update AGENTS.md's operating guide to match (no gradle-log-analyst delegation, no summary artifacts). * sphinx: address review feedback on PR #801 * Correlate leak tags on (tag, incarnation) and emit locale-stable metrics Leak tags are recycled, so tag-only correlation in LeakTagCorrelationScenario could accept a stale chunk that inherited the recycled tag; match the HeapLiveObject on both identity fields and fail loud when either accessor or stamp is missing. Format the repro's machine-readable metrics with Locale.ROOT so a comma-decimal locale cannot silently zero parsed values. * Quarantine TlsPrimingTest on el7-8-release-amd64 (PROF-16281) samples_dropped_thread_local is process-global and shared with every SIGNAL_HANDLER_GUARD_OR_DROP path, while the allowance is bounded by this test's own sample count, so co-located runner load in the sequential el7-8 suite run can trip the bound (observed 79/195, 50/645, 272/137 across the three RetryingTest attempts). * Drop sphinx spec notes from the PR
This was referenced Oct 20, 2026
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 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.leakTagjoin attributes a dead object's chain to the tag's new owner.Fix.
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: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:
The scenario infrastructure fails loud on absent identity fields (a missing
leakTag/leakTagIncarnationaccessor 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:
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.NativeThreadTest,DynamicNativeThread,ThreadEntryDetectionTest,VtableReceiverFrameTest) are unrelated and reproduce on a clean tree.How to test the change?:
ReferenceChainJfrParserTestassumes the roundtrip gtest's recording; run:ddprof-lib:gtestDebug_referenceChainJfrRoundtrip_utfirst (or any fulltestDebug, which orders the gtests ahead of the Java tests), otherwise the test is skipped with an assumption.For Datadog employees: