Skip to content

Commit abbe49d

Browse files
rkennkeclaude
andcommitted
profiling(ddprof): fix two reapply ordering bugs found by PR review bots
- setContextValue: a rejected native write (e.g. >255-byte UTF-8) clears the native slot but left the Java snapshot untouched, so the attribute read as unset until the next span boundary silently resurrected the stale prior value. Reapply immediately on rejection so the prior value stays visible continuously, matching pre-migration DBB behavior. - setTraceContext: reapplyAppContext() ran unconditionally after the native call, so when profiling.context.attributes also names _dd.trace.operation/resource (with span-name/resource-name context enabled), the trailing reapply clobbered the span-derived value that setTraceContext just wrote to the same offset with a stale app-recorded one. reapplyAppContext now takes the operation/resource offsets to skip. Both were flagged independently by Codex and Datadog Autotest PR review bots on #11899, with a concrete repro for the first. Added regression coverage to DatadogProfilerTest#testContextRegistration for both (verified each new assertion fails without its corresponding fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 0bac29b commit abbe49d

2 files changed

Lines changed: 70 additions & 1 deletion

File tree

‎dd-java-agent/agent-profiling/profiling-ddprof/src/main/java/com/datadog/profiling/ddprof/DatadogProfiler.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,11 @@ public void setTraceContext(
524524
} catch (Throwable e) {
525525
log.debug("Failed to set trace context", e);
526526
}
527-
reapplyAppContext();
527+
// Skip operationOffset/resourceOffset: setTraceContext just wrote the span-derived values
528+
// there natively. If profiling.context.attributes also names _dd.trace.operation/resource,
529+
// that offset is app-owned too (see isAppOffset); reapplying it here would immediately
530+
// overwrite the fresh span-derived value with a stale app-recorded one.
531+
reapplyAppContext(operationOffset, resourceOffset);
528532
}
529533

530534
/** Per-deactivation clear; reapplies app-managed attributes afterwards (see setTraceContext). */
@@ -555,6 +559,10 @@ public boolean setContextValue(int offset, String value) {
555559
recordAppContextValue(offset, value);
556560
return true;
557561
}
562+
// Rejected (e.g. >255-byte UTF-8, dictionary full): the native call already cleared this
563+
// slot. Restore it from the still-current Java snapshot so a rejected write doesn't blank
564+
// the attribute until the next span boundary.
565+
reapplyAppContext();
558566
} catch (Throwable e) {
559567
log.debug("Failed to set context value", e);
560568
}
@@ -615,6 +623,16 @@ public boolean clearContextValue(int offset) {
615623
* (java-profiler PROF-15361); per-slot is adequate for the typical small app-attribute count.
616624
*/
617625
public void reapplyAppContext() {
626+
reapplyAppContext(-1, -1);
627+
}
628+
629+
/**
630+
* Same as {@link #reapplyAppContext()}, but leaves {@code skipOffset1}/{@code skipOffset2} alone.
631+
* Used by {@link #setTraceContext} to avoid clobbering the operation/resource offsets it just
632+
* wrote natively, in the edge case where those offsets are also app-owned (see {@link
633+
* #isAppOffset}). Pass -1 for either argument to skip nothing.
634+
*/
635+
private void reapplyAppContext(int skipOffset1, int skipOffset2) {
618636
if (!hasAppContext) {
619637
return;
620638
}
@@ -625,6 +643,9 @@ public void reapplyAppContext() {
625643
try {
626644
int remaining = snapshot.nonZeroCount();
627645
for (int i = 0; i < isAppOffset.length && remaining > 0; i++) {
646+
if (i == skipOffset1 || i == skipOffset2) {
647+
continue;
648+
}
628649
String s = snapshot.stringAt(i);
629650
if (s != null) {
630651
profiler.setContextValue(i, s);

‎dd-java-agent/agent-profiling/profiling-ddprof/src/test/java/com/datadog/profiling/ddprof/DatadogProfilerTest.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,54 @@ public void testContextRegistration() {
478478
profiler.snapshot()[fooOffset],
479479
"Guard: zero-span activation must degrade to a clean clear that still reapplies app context");
480480
profiler.clearContextValue("foo");
481+
482+
// Regression: a native setContextValue rejection (e.g. an oversized value, >255 UTF-8 bytes)
483+
// clears the native slot; the prior value must be resynced immediately instead of only
484+
// reappearing on the next span boundary (a flicker the pre-migration DBB path never had,
485+
// since it retained the prior value continuously).
486+
fooSetter.set("valid-before-reject");
487+
int validEncoding = profiler.snapshot()[fooOffset];
488+
assertNotEquals(0, validEncoding, "foo must be live before the rejected write");
489+
StringBuilder oversized = new StringBuilder();
490+
for (int i = 0; i < 300; i++) {
491+
oversized.append('x');
492+
}
493+
assertFalse(
494+
profiler.setContextValue("foo", oversized.toString()),
495+
"an oversized (>255 UTF-8 bytes) value must be rejected");
496+
assertEquals(
497+
validEncoding,
498+
profiler.snapshot()[fooOffset],
499+
"a rejected write must not blank the slot; the prior value must stay visible immediately");
500+
profiler.clearContextValue("foo");
501+
502+
// Regression: setTraceContext's trailing reapplyAppContext must not clobber the span-derived
503+
// value it just wrote natively to operationOffset/resourceOffset, even when that offset is
504+
// also app-owned (e.g. profiling.context.attributes names _dd.trace.operation/resource while
505+
// span-name/resource-name context is enabled). Reuses the "foo" app-owned offset as a stand-in
506+
// operationOffset — the profiler is a process-wide singleton and its context attributes can't
507+
// be reconfigured to register the real _dd.trace.operation/resource offsets here.
508+
profiler.setContextValue(fooOffset, "span-derived-value");
509+
int spanDerivedEncoding = profiler.snapshot()[fooOffset];
510+
assertNotEquals(0, spanDerivedEncoding, "fixture sanity: span-derived value must be live");
511+
profiler.clearContextValue("foo");
512+
513+
fooSetter.set("stale-app-value");
514+
int appEncoding = profiler.snapshot()[fooOffset];
515+
assertNotEquals(0, appEncoding, "foo app value must be recorded before the trace-context call");
516+
assertNotEquals(
517+
spanDerivedEncoding,
518+
appEncoding,
519+
"fixture sanity: span-derived and app values must have distinct encodings");
520+
521+
profiler.setTraceContext(1L, 1L, 0L, 1L, fooOffset, "span-derived-value", -1, null);
522+
assertEquals(
523+
spanDerivedEncoding,
524+
profiler.snapshot()[fooOffset],
525+
"setTraceContext's trailing reapplyAppContext must skip operationOffset/resourceOffset so"
526+
+ " it doesn't overwrite the span-derived value just written to an app-owned offset");
527+
profiler.clearSpanContext();
528+
profiler.clearContextValue("foo");
481529
}
482530

483531
private static ConfigProvider configProvider(

0 commit comments

Comments
 (0)