Skip to content

Commit ba35ba6

Browse files
committed
test(refchain): tighten test hygiene per review round 3
1 parent 87e9977 commit ba35ba6

3 files changed

Lines changed: 93 additions & 32 deletions

File tree

‎ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/AggressiveLeakReferenceChainTest.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,21 @@ private static GateEvaluation evaluateGateAfterSearchReset() {
135135
// candidateSeen bracket honest.
136136
int[] candidatesBefore = ReferenceChainTestHooks.selectLeakCandidateKlassIds0();
137137
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
138-
int passesBefore = ReferenceChainTestHooks.referenceChainPassesRunForTest0();
138+
// Sampled AFTER the reset, so not a pre-reset baseline: a value > 0 means a pass ENDED
139+
// between the reset and this read (the reset zeroes the counter only at pass END, so a
140+
// long in-flight pass is still invisible to this read - the candidate bracket plus
141+
// checkNoCandidateFreeActivity's retry compensates for that blind spot).
142+
int passesAfterReset = ReferenceChainTestHooks.referenceChainPassesRunForTest0();
139143
GateEvaluation e = new GateEvaluation();
140144
e.gateOpen = ReferenceChainTestHooks.shouldRunPassForTest0();
141145
int[] candidatesAfter = ReferenceChainTestHooks.selectLeakCandidateKlassIds0();
146+
// Sampled after the gate call: a BFS pass racing between the gate evaluation and this read
147+
// is conservatively attributed to the evaluation window (safe direction: it only widens
148+
// the observed activity window, never masks a gate violation).
142149
e.passesAfter = ReferenceChainTestHooks.referenceChainPassesRunForTest0();
143150
e.candidateSeen = !isEmpty(candidatesBefore) || !isEmpty(candidatesAfter);
144151
e.detail = "candidatesBefore=" + Arrays.toString(candidatesBefore) + " candidatesAfter="
145-
+ Arrays.toString(candidatesAfter) + " passesBefore=" + passesBefore
152+
+ Arrays.toString(candidatesAfter) + " passesAfterReset=" + passesAfterReset
146153
+ " passesAfter=" + e.passesAfter;
147154
return e;
148155
}

‎ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/ReferenceChainSchedulingTest.java‎

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ protected boolean isPlatformSupported() {
8383
* reading - so a closed result is final and only an open/matching result needs the retry.
8484
* The same real pass also produces the closed reading on its own, so a closed-direction probe
8585
* (a negated gate reading) must be bracketed by a check that the seeded state is still in force
86-
* (see {@link #rootEnumClosedWithSeedIntact(long)}) - otherwise a closed result caused by a
87-
* real pass would be accepted instead of retried.
86+
* (see {@link #rootEnumClosedWithSeedIntact(long, Runnable)}) - otherwise a closed result
87+
* caused by a real pass would be accepted instead of retried.
8888
*/
8989
private static boolean eventuallyTrue(String message, Runnable seed,
9090
BooleanSupplier probe)
@@ -102,16 +102,35 @@ private static boolean eventuallyTrue(String message, Runnable seed,
102102

103103
/**
104104
* Closed-direction root-enum probe (see eventuallyTrue): a closed gate counts only while the
105-
* seeded floor is still in force. A concurrent real enumeration also closes the gate (it
106-
* refreshes _last_root_enum_ns to "now"), so a bare closed reading cannot tell the seeded floor
107-
* apart from that unrelated close. The two JNI calls take the engine lock separately; a real
108-
* pass landing between them either changes the floor (the equality fails and the probe
109-
* retries) or leaves it unchanged (the closed reading is still what the seeded floor predicts),
110-
* so that window can cause a retry, never a false accept.
105+
* seeded state is still in force, verified by a floor + pass-counter bracket around the gate
106+
* read. A concurrent real enumeration also closes the gate (it refreshes _last_root_enum_ns to
107+
* "now"), so a bare closed reading cannot tell the seeded floor apart from that unrelated
108+
* close - and a floor-only bracket is not enough either: a real enumeration measuring a cost
109+
* that re-derives the same floor (e.g. ~0 ms on a small graph vs the seeded 0 cost) would
110+
* reproduce the seeded floor while the closed reading was actually caused by that enumeration.
111+
*
112+
* <p>The bracket is structurally airtight for pass-driven taint: real root enumeration runs
113+
* ONLY inside a BFS pass ({@code runPassSerialized()} holds {@code _engine_lock} across the
114+
* whole {@code runPass()}), and {@code _passes_run} increments only after the manual walk
115+
* returns - so enumeration state and the pass counter flip together, and the probe's
116+
* engine-lock-acquiring gate/floor reads can never observe one without the other: a completed
117+
* pass always flips {@code p0 != p1}, and an enumeration whose measured cost shifts the floor
118+
* flips {@code f1 != expectedFloorNs}. The probe also re-seeds immediately before reading,
119+
* undoing any real enumeration that already completed and clobbered the seeded state, so only
120+
* an enumeration landing DURING the read sequence can taint it - a microsecond window instead
121+
* of the seed-to-probe gap. Premise: if a future change ever runs root enumeration outside a
122+
* pass (outside {@code runPassSerialized()}'s lock hold), the pass-counter premise breaks and
123+
* this bracket degrades to probabilistic - revisit it then.
111124
*/
112-
private static boolean rootEnumClosedWithSeedIntact(long expectedFloorNs) {
113-
return !ReferenceChainTestHooks.shouldRunRootEnumForTest0(false)
114-
&& ReferenceChainTestHooks.rootEnumIntervalFloorNsForTest0() == expectedFloorNs;
125+
private static boolean rootEnumClosedWithSeedIntact(long expectedFloorNs, Runnable seed) {
126+
seed.run(); // re-seed immediately before reading: undoes any real enumeration that already
127+
// completed and clobbered the seeded state.
128+
int p0 = ReferenceChainTestHooks.referenceChainPassesRunForTest0();
129+
long f0 = ReferenceChainTestHooks.rootEnumIntervalFloorNsForTest0();
130+
boolean closed = !ReferenceChainTestHooks.shouldRunRootEnumForTest0(false);
131+
long f1 = ReferenceChainTestHooks.rootEnumIntervalFloorNsForTest0();
132+
int p1 = ReferenceChainTestHooks.referenceChainPassesRunForTest0();
133+
return closed && p0 == p1 && f0 == expectedFloorNs && f1 == expectedFloorNs;
115134
}
116135

117136
/** Seed relations the root-enum tests rely on, checked on the Java mirrors themselves. */
@@ -296,10 +315,11 @@ public void rootEnumWaitsForMinimumInterval() throws InterruptedException {
296315
assertRootEnumSeedAssumptions();
297316
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
298317
try {
318+
Runnable seed = () -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
319+
ROOT_ENUM_BEFORE_BASE_MS, 0);
299320
eventuallyTrue("Re-enumeration must wait out the base interval since the last attempt",
300-
() -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
301-
ROOT_ENUM_BEFORE_BASE_MS, 0),
302-
() -> rootEnumClosedWithSeedIntact(ROOT_ENUM_MIN_INTERVAL_NS));
321+
seed,
322+
() -> rootEnumClosedWithSeedIntact(ROOT_ENUM_MIN_INTERVAL_NS, seed));
303323
} finally {
304324
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
305325
}
@@ -353,6 +373,8 @@ public void rootEnumIntervalExtendsWithMeasuredSafepointCost() throws Interrupte
353373
assertRootEnumSeedAssumptions();
354374
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
355375
try {
376+
Runnable seed = () -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
377+
ROOT_ENUM_PAST_BASE_MS, ROOT_ENUM_EXTENDING_COST_MS);
356378
// Positive control: the past-base seed alone opens the gate, so the closed reading below
357379
// is attributable to the cost-extended floor.
358380
eventuallyTrue("A past-base seed with no measured cost must open the gate",
@@ -369,10 +391,9 @@ public void rootEnumIntervalExtendsWithMeasuredSafepointCost() throws Interrupte
369391
== ROOT_ENUM_EXTENDING_COST_MS * ROOT_ENUM_NS_PER_COST_MS);
370392
eventuallyTrue("Re-enumeration must wait out the cost-extended interval even though the "
371393
+ "base interval has elapsed",
372-
() -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
373-
ROOT_ENUM_PAST_BASE_MS, ROOT_ENUM_EXTENDING_COST_MS),
394+
seed,
374395
() -> rootEnumClosedWithSeedIntact(
375-
ROOT_ENUM_EXTENDING_COST_MS * ROOT_ENUM_NS_PER_COST_MS));
396+
ROOT_ENUM_EXTENDING_COST_MS * ROOT_ENUM_NS_PER_COST_MS, seed));
376397
} finally {
377398
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
378399
}
@@ -385,6 +406,8 @@ public void rootEnumIntervalCapsAtMaximum() throws InterruptedException {
385406
assertRootEnumSeedAssumptions();
386407
ReferenceChainTestHooks.resetReferenceChainSearchForTest0();
387408
try {
409+
Runnable seed = () -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
410+
ROOT_ENUM_PAST_BASE_MS, ROOT_ENUM_CAPPING_COST_MS);
388411
// Positive control, as in rootEnumIntervalExtendsWithMeasuredSafepointCost.
389412
eventuallyTrue("A past-base seed with no measured cost must open the gate",
390413
() -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
@@ -398,9 +421,8 @@ public void rootEnumIntervalCapsAtMaximum() throws InterruptedException {
398421
() -> ReferenceChainTestHooks.rootEnumIntervalFloorNsForTest0()
399422
== ROOT_ENUM_MAX_INTERVAL_NS);
400423
eventuallyTrue("Re-enumeration must wait out the capped interval",
401-
() -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(
402-
ROOT_ENUM_PAST_BASE_MS, ROOT_ENUM_CAPPING_COST_MS),
403-
() -> rootEnumClosedWithSeedIntact(ROOT_ENUM_MAX_INTERVAL_NS));
424+
seed,
425+
() -> rootEnumClosedWithSeedIntact(ROOT_ENUM_MAX_INTERVAL_NS, seed));
404426
// An uncapped floor would still hold this age off.
405427
eventuallyTrue("Re-enumeration must run once the capped interval has elapsed",
406428
() -> ReferenceChainTestHooks.setReferenceChainRootEnumStateForTest0(

‎ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/ReferenceChainTrackingTest.java‎

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,15 @@ public class ReferenceChainTrackingTest extends AbstractProfilerTest {
123123
private static final long ABANDON_DUMP_RETRY_SPACING_MS = 1_000;
124124
private static final long ABANDON_LAST_DUMP_MIN_DELAY_MS = 8_000;
125125

126+
// Seam-driven passes are front-loaded onto the first GRACE_DRIVE_ATTEMPTS grace-loop
127+
// attempts (8 passes each, 24 total); the remaining grace-loop attempts are poll-only
128+
// (organic BFS cadence + 1s sleep + dump + match), so CI load does not scale with the loop
129+
// width. Each driven pass serializes against the BFS thread via _engine_lock and feeds the
130+
// shared pause-controller cost model (_effective_cadence_ns, _canary_pass_ema_ms,
131+
// _cpu_pain_budget), and the deterministic-frontier claim needs only cumulative progress,
132+
// which the front-loaded burst far exceeds.
133+
private static final int GRACE_DRIVE_ATTEMPTS = 3;
134+
126135
/**
127136
* The {@code datadog.ReferenceChain} ActiveSetting's {@code id} (flightRecorder.cpp's
128137
* T_REFERENCE_CHAIN). ActiveSetting events from cpu/wall/memory/nativemem also read
@@ -467,10 +476,15 @@ public void shouldReconstructReferrerChainToGcRoot() throws Exception {
467476
// for why that is load-bearing; the seam stubs are no-ops in release builds). Ten attempts,
468477
// matching the cache-leak test's loop: this fixture's ArrayList resizes churn the walked
469478
// element path just like the cache's HashMap resizes, so late-round discovery needs the
470-
// same scheduling-independent margin.
479+
// same scheduling-independent margin. The seam burst is capped at the first
480+
// GRACE_DRIVE_ATTEMPTS attempts to bound synchronous heap-walk load on shared CI runners
481+
// (each pass serializes against the BFS thread via _engine_lock and feeds the
482+
// pause-controller cost model); the remaining attempts are poll-only on the organic cadence.
471483
JfrEvents lastEvents = null;
472484
for (int attempt = 0; match == null && attempt < 10; attempt++) {
473-
driveReferenceChainPasses(8);
485+
if (attempt < GRACE_DRIVE_ATTEMPTS) {
486+
driveReferenceChainPasses(8);
487+
}
474488
Thread.sleep(1000);
475489
dump(scratchDumpPath);
476490
lastEvents = verifyEvents(scratchDumpPath, "datadog.ReferenceChain", false);
@@ -691,10 +705,16 @@ public void shouldReconstructReferrerChainThroughUnboundedCacheLeak() throws Exc
691705
// duration). Driving passes here makes discovery depend on cumulative frontier progress,
692706
// which is deterministic, instead of on that scheduling. pollReferenceChainTargets0() after
693707
// each pass is what tags the candidate's tracked instances and polls the representative -
694-
// the same per-iteration tail the BFS thread runs.
708+
// the same per-iteration tail the BFS thread runs. The burst is capped at the first
709+
// GRACE_DRIVE_ATTEMPTS attempts to bound synchronous heap-walk load on shared CI runners
710+
// (each pass serializes against the BFS thread via _engine_lock and feeds the
711+
// pause-controller cost model); the remaining attempts are poll-only on the organic
712+
// cadence, which keeps this loop's load within the pre-seam 5-attempt shape.
695713
JfrEvents lastEvents = null;
696714
for (int attempt = 0; match == null && attempt < 10; attempt++) {
697-
driveReferenceChainPasses(8);
715+
if (attempt < GRACE_DRIVE_ATTEMPTS) {
716+
driveReferenceChainPasses(8);
717+
}
698718
Thread.sleep(1000);
699719
dump(scratchDumpPath);
700720
lastEvents = verifyEvents(scratchDumpPath, "datadog.ReferenceChain", false);
@@ -764,7 +784,13 @@ public void shouldReconstructReferrerChainThroughUnboundedCacheLeak() throws Exc
764784
* flakiness, observed in practice), the shared search can still be {@code RUNNING} with a large,
765785
* non-fresh frontier when this method starts - the fresh-search precondition is something this
766786
* method establishes, not something it presumes. Debug-only: the seam does not exist in a
767-
* release build.
787+
* release build, and the {@code assumeTrue} at the top of the method body skips this test
788+
* there - a JUnit assumption failure is reported as SKIPPED, not passed, so coverage reports
789+
* must not count it. By design, release-config runs (the {@code test:release} label)
790+
* therefore exercise none of the {@code buildAbandonedEvent()} →
791+
* {@code FlightRecorder::recordReferenceChainAbandoned()} write path; if the candidate-gate
792+
* contract ever changes so a release build could reach {@code ABANDONED} without the debug
793+
* population seams, this assumption must be revisited.
768794
*
769795
* <p><b>Why this method also seeds a klass population trend, despite requesting no
770796
* {@code generations=true} of its own:</b> {@code LivenessTracker::_gc_generations} is only ever
@@ -860,20 +886,25 @@ public void shouldReportAbandonedSearchOnTinyFrontierCap() throws Exception {
860886
// the frontier cap/ttl cutoff inside whatever wall-clock time this shared JVM's scheduling
861887
// grants it. The abandoned event only reaches the recording through a dump(), so each check
862888
// needs a fresh dump - and each dump rewrites the whole shared recording. Dumps are
863-
// therefore capped at ABANDON_MAX_DUMPS and paced so the last one lands at least
864-
// ABANDON_LAST_DUMP_MIN_DELAY_MS after the grace period started; the passesRun probe above
865-
// absorbs most of the scheduling slack without any I/O.
889+
// therefore capped at ABANDON_MAX_DUMPS and paced so that, when the loop runs to
890+
// completion, the last one lands at or after ABANDON_LAST_DUMP_MIN_DELAY_MS after the
891+
// grace period started (on the final attempt attemptsLeft == 1, so the sleep is
892+
// Math.max(SPACING, full remaining); an early success break can land earlier, which is
893+
// fine because the event was already found and the floor is moot); the passesRun probe
894+
// above absorbs most of the scheduling slack without any I/O.
866895
long lastDumpEarliestNs =
867896
graceStartNs + TimeUnit.MILLISECONDS.toNanos(ABANDON_LAST_DUMP_MIN_DELAY_MS);
868897
JfrEvents abandoned = null;
869898
int dumpsTaken = 0;
899+
long lastDumpAtNs = 0;
870900
for (int attempt = 0; attempt < ABANDON_MAX_DUMPS; attempt++) {
871901
if (attempt > 0) {
872902
int attemptsLeft = ABANDON_MAX_DUMPS - attempt; // includes the dump about to be taken
873903
long remainingMs =
874904
TimeUnit.NANOSECONDS.toMillis(lastDumpEarliestNs - System.nanoTime());
875905
Thread.sleep(Math.max(ABANDON_DUMP_RETRY_SPACING_MS, remainingMs / attemptsLeft));
876906
}
907+
lastDumpAtNs = System.nanoTime();
877908
dump(dumpPath);
878909
dumpsTaken++;
879910
abandoned = verifyEvents(dumpPath, "datadog.ReferenceChainAbandoned", false);
@@ -885,8 +916,9 @@ public void shouldReportAbandonedSearchOnTinyFrontierCap() throws Exception {
885916
"Expected a datadog.ReferenceChainAbandoned event within " + dumpsTaken + " of at most "
886917
+ ABANDON_MAX_DUMPS + " dumps (after 3 GCs and a 1500ms grace period; the pass "
887918
+ "probe waited up to " + ABANDON_PASS_WAIT_MS + "ms, passesRun="
888-
+ ReferenceChainTestHooks.referenceChainPassesRunForTest0() + "; last dump >= "
889-
+ ABANDON_LAST_DUMP_MIN_DELAY_MS + "ms after grace start)");
919+
+ ReferenceChainTestHooks.referenceChainPassesRunForTest0() + "; last dump "
920+
+ TimeUnit.NANOSECONDS.toMillis(lastDumpAtNs - graceStartNs)
921+
+ "ms after grace start, nominal floor " + ABANDON_LAST_DUMP_MIN_DELAY_MS + "ms)");
890922
} finally {
891923
Files.deleteIfExists(dumpPath);
892924
}

0 commit comments

Comments
 (0)