From df2d368786eb0b4d11bc2a59c38b246203316fa4 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Tue, 1 Sep 2026 16:09:21 -0400 Subject: [PATCH 01/12] Add @ForegroundSafe / @BackgroundOnly marker annotations Documentation-and-tooling markers declaring whether code is cheap enough for application (foreground) threads or must be confined to a background thread the tracer paces itself. No application to real code yet and no checker -- just the annotation types, following the Strategy/StrategyConsumer marker convention (APMLP-1543). --- .../trace/api/function/BackgroundOnly.java | 33 +++++++++++++++++ .../trace/api/function/ForegroundSafe.java | 36 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java create mode 100644 internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java diff --git a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java new file mode 100644 index 00000000000..242652be53d --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java @@ -0,0 +1,33 @@ +package datadog.trace.api.function; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks code that must be confined to a background thread the tracer owns and paces itself -- e.g. + * serialization, stats aggregation, or eviction -- and must never be reached from an application + * thread (the foreground; see {@link ForegroundSafe}), where its cost would become customer-visible + * latency instead. + * + *

This is a documentation-and-tooling marker; it changes no behavior. It exists to telegraph the + * constraint to readers and to give a future checker (see {@code APMLP-1645}) something to verify + * -- that no {@code @BackgroundOnly} code is reachable from a foreground call site. The discipline + * it names is not yet enforced; hold to it by hand until the checker lands. + * + *

The two markers are not symmetric -- see {@link ForegroundSafe} for why it, not this + * one, is the strictly stronger guarantee. + * + *

On a type ({@link ElementType#TYPE}): every method of this type is background-only + * unless a method-level {@link ForegroundSafe} widens it. + * + *

On a method ({@link ElementType#METHOD}): this method specifically is background-only, + * regardless of what the enclosing type declares -- a method-level marker always wins over the + * type-level one. + */ +@Documented +@Retention(RetentionPolicy.SOURCE) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface BackgroundOnly {} diff --git a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java new file mode 100644 index 00000000000..b1c0985dc3c --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java @@ -0,0 +1,36 @@ +package datadog.trace.api.function; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks code cheap enough to call from an application thread (the foreground) -- the request or + * transaction thread the instrumented application itself is running, where any added cost is + * customer-visible latency, as opposed to a background thread the tracer owns and paces itself (see + * {@link BackgroundOnly}). + * + *

This is a documentation-and-tooling marker; it changes no behavior. It exists to telegraph the + * guarantee to readers and to give a future checker (see {@code APMLP-1645}) something to verify -- + * that no {@link BackgroundOnly} code is reachable from a foreground call site. The discipline it + * names is not yet enforced; hold to it by hand until the checker lands. + * + *

The two markers are not symmetric. {@code @ForegroundSafe} is the strictly stronger + * guarantee: code cheap enough for the foreground is automatically fine to call from a background + * thread too, so a {@code @ForegroundSafe} type or method may be called from either. {@link + * BackgroundOnly} code carries no such guarantee and must never be reached from a foreground call + * site. + * + *

On a type ({@link ElementType#TYPE}): every method of this type is foreground-safe + * unless a method-level {@link BackgroundOnly} narrows it. + * + *

On a method ({@link ElementType#METHOD}): this method specifically is foreground-safe, + * regardless of what the enclosing type declares -- a method-level marker always wins over the + * type-level one. + */ +@Documented +@Retention(RetentionPolicy.SOURCE) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface ForegroundSafe {} From 7792b1de81b9244b2024db892a6485c13a597f0a Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 12:32:47 -0400 Subject: [PATCH 02/12] Annotate DDCache/SimpleUtf8Cache/GenerationalUtf8Cache with @ForegroundSafe / @BackgroundOnly DDCache is cheap (small, bounded probe count) -- safe to call from application threads. SimpleUtf8Cache and GenerationalUtf8Cache trade CPU for allocation savings that only pay off when run on a background thread, so they're confined to the background serializer thread. APMLP-1544 Co-Authored-By: Claude Sonnet 5 --- .../communication/serialization/GenerationalUtf8Cache.java | 6 ++++++ .../communication/serialization/SimpleUtf8Cache.java | 6 ++++++ .../src/main/java/datadog/trace/api/cache/DDCache.java | 7 +++++++ 3 files changed, 19 insertions(+) diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index 099ff56e0e0..9dd1c3b2818 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -1,5 +1,6 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.nio.charset.StandardCharsets; import javax.annotation.concurrent.ThreadSafe; @@ -15,6 +16,11 @@ * String#getBytes(java.nio.charset.Charset)}. * *

The cache is thread safe. + * + *

{@link BackgroundOnly}: the eden/tenured promotion and recalibration bookkeeping costs more + * than the {@code getBytes} call it replaces, so the saving only shows up as reduced allocation/GC + * pressure on the thread that pays it -- confine it to the background serializer thread, not an + * application thread. */ /* * Cache works by using a 2-level promotion based scheme. diff --git a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java index 8eb12d48465..4af30539053 100644 --- a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java @@ -1,5 +1,6 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; import java.nio.charset.StandardCharsets; import javax.annotation.concurrent.ThreadSafe; @@ -13,6 +14,11 @@ * String#getBytes(java.nio.charset.Charset)}. * *

The cache is thread safe. + * + *

{@link BackgroundOnly}: the bookkeeping (hit counting, LFU eviction scan) costs more than the + * {@code getBytes} call it replaces, so the saving only shows up as reduced allocation/GC pressure + * on the thread that pays it -- confine it to the background serializer thread, not an application + * thread. */ /* * Thread safety is achieved through using CacheEntry objects where the key data diff --git a/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java b/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java index d1cd8024847..17888187a83 100644 --- a/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java +++ b/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java @@ -1,8 +1,15 @@ package datadog.trace.api.cache; +import datadog.trace.api.function.ForegroundSafe; import java.util.function.BiConsumer; import java.util.function.Function; +/** + * {@link ForegroundSafe}: every implementation looks up a key via a small, bounded number of probes + * (no growth, no eviction bookkeeping beyond overwriting a slot) -- cheap enough to call from an + * application thread. + */ +@ForegroundSafe public interface DDCache { /** * Look up or create and store a value in the cache. From 1ccbd8f51c192f900d99d92fa0459fd013429e13 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 12:43:15 -0400 Subject: [PATCH 03/12] Add J16: flag foreground calls into @BackgroundOnly-declared code Ports the @ForegroundSafe/@BackgroundOnly declared-contract check (originally drafted for the now-retired /perf-review skill) into the dd-apm-sdk-review performance override, plus a matching .llm-validation case exercising it against a synthetic Byte Buddy advice call into a @BackgroundOnly cache. Co-Authored-By: Claude Sonnet 5 --- .../reviewers/performance.md | 5 +- .llm-validation/suites/dd-apm-sdk-review.yaml | 49 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index 9ad44dc33f9..fdba7e5b583 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -1,6 +1,6 @@ Override for `reviewers/performance.md` (in the core skill folder) — read that file first, then this. -Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md,checks.md}` (the previously-shipped, now-retired Java performance rubric; the skill folder was removed to avoid two competing performance-review entry points). Everything language-agnostic from that rubric moved to the core `reviewers/performance.md` (confidence axis, severity model, domain-adjusted severity, universal checks); everything Java-specific is the addenda below (J1-J15), the instrumentation idioms, the deterministic-lint candidates, and the bootstrap note. +Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md,checks.md}` (the previously-shipped, now-retired Java performance rubric; the skill folder was removed to avoid two competing performance-review entry points). Everything language-agnostic from that rubric moved to the core `reviewers/performance.md` (confidence axis, severity model, domain-adjusted severity, universal checks); everything Java-specific is the addenda below (J1-J16), the instrumentation idioms, the deterministic-lint candidates, and the bootstrap note. # Performance — dd-trace-java specifics @@ -30,10 +30,11 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input), but also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. A non-capturing lambda is a cached singleton (zero-alloc); a capturing lambda (closes over a local or `this`) is a new instance per evaluation — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. +- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence — **SEV-1** — a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup. **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. Currently-annotated classes: `DDCache` (`@ForegroundSafe`), `SimpleUtf8Cache` and `GenerationalUtf8Cache` (`@BackgroundOnly`, both `datadog.communication.serialization`). **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. -**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. +**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`), `ForegroundSafe`/`BackgroundOnly` (`datadog.trace.api.function`, scoped only to the three classes named in J16 above). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. ## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 2cd716ed13e..e2d6965f772 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -368,3 +368,52 @@ cases: - "Notes the flag exists but misses that nothing re-initializes it per payload." - "Suggests resetting the field at the start of every `map()` (or only \"make it volatile\" / \"add logging\")." - "Treats this as a crash rather than silent data loss from leaked state." + + - id: java-perf-foreground-calls-background-only-001 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + `SimpleUtf8Cache.getUtf8(...)` is declared `@BackgroundOnly` (see its Javadoc below) — + its bookkeeping only pays off when it is only ever called from the single background + serializer thread. `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice that runs + synchronously on the calling application thread for every instrumented + `HttpURLConnection` request (a foreground/app-thread call site, not annotated + `@BackgroundOnly` or `@ForegroundSafe`). + + ```java + // datadog/communication/serialization/SimpleUtf8Cache.java (existing, unchanged) + /** + * {@link BackgroundOnly}: bookkeeping to reuse the encoded byte[] only pays off when this + * is confined to the single background serializer thread that owns it. + */ + @BackgroundOnly + public final class SimpleUtf8Cache implements EncodingCache { + public byte[] getUtf8(String value) { /* ... */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/httpurlconnection/HttpUrlConnectionAdvice.java b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/httpurlconnection/HttpUrlConnectionAdvice.java + @@ -40,6 +40,11 @@ public class HttpUrlConnectionAdvice { + + // PR #99999: tag the outgoing request's first header value for a new debug flag. + + @Advice.OnMethodExit + + public static void onExit(@Advice.This HttpURLConnection connection, + + @Advice.Argument(0) String headerValue, @Advice.Local("span") AgentSpan span) { + + byte[] utf8 = SHARED_UTF8_CACHE.getUtf8(headerValue); + + span.setTag("debug.header_bytes_len", utf8.length); + + } + ``` + expected_criteria: + - "Flags that `onExit` — Byte Buddy advice running on the application thread for every instrumented `HttpURLConnection` call — reaches `SimpleUtf8Cache.getUtf8(...)`, a method declared `@BackgroundOnly`, per J16's declared-contract check." + - "Classifies this as SEV-1 / P0, not a soft nudge or generic allocation note." + - "Recommends removing the call from advice or moving the work to the background/serializer thread that legitimately owns `SimpleUtf8Cache`, rather than caching the value differently in place." + bad_signals: + - "Treats this as an ordinary allocation or contention finding without naming the declared `@BackgroundOnly` contract violation." + - "Assumes the cache is safe to call from advice because it is described as a \"cache\" without checking the annotation." + - "Misses that `onExit` runs on the application/foreground thread, not the serializer thread." From a87a8bbd8ae507fff0d32e69ca8bbec03d98120b Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 13:02:11 -0400 Subject: [PATCH 04/12] Actually apply @BackgroundOnly to the Utf8 cache classes The prior commit documented the constraint in a Javadoc @link but never applied the annotation itself, so J16's grep-based resolution had nothing to match. APMLP-1544 Co-Authored-By: Claude Sonnet 5 --- .../communication/serialization/GenerationalUtf8Cache.java | 1 + .../datadog/communication/serialization/SimpleUtf8Cache.java | 1 + 2 files changed, 2 insertions(+) diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index 9dd1c3b2818..d0911907116 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -70,6 +70,7 @@ * provide better cache utilization. */ @ThreadSafe +@BackgroundOnly @SuppressFBWarnings( value = "IS2_INCONSISTENT_SYNC", justification = diff --git a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java index 4af30539053..2398c4d4e05 100644 --- a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java @@ -49,6 +49,7 @@ * a LFU: least frequently used eviction policy is used to free up a slot. */ @ThreadSafe +@BackgroundOnly public final class SimpleUtf8Cache implements EncodingCache { static final int MAX_CAPACITY = 1024; From 384b4301ed9b5058549f1304fb7a221fad47154d Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 13:04:24 -0400 Subject: [PATCH 05/12] Wire J16 test case into the gate preset, fix fixture accuracy - Add java-perf-foreground-calls-background-only-001 to the gate preset so it actually runs in CI instead of only under --level full. - Add the missing core performance.md to its files: list. - Fix the fixture: annotation is class-level not method-level, and the diff now points at the real HttpUrlConnectionAdvice location instead of a nonexistent module path. Co-Authored-By: Claude Sonnet 5 --- .llm-validation/README.md | 2 +- .llm-validation/config.yaml | 1 + .llm-validation/suites/dd-apm-sdk-review.yaml | 38 +++++++++++-------- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/.llm-validation/README.md b/.llm-validation/README.md index 00dcf8202fd..35b1c598f9f 100644 --- a/.llm-validation/README.md +++ b/.llm-validation/README.md @@ -56,7 +56,7 @@ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level full --runs 1 -# CI-shaped set (8 cases) +# CI-shaped set (9 cases) docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level gate --runs 1 diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index 1e4632ba076..e1ec7a8c448 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -33,6 +33,7 @@ presets: - java-maintainability-resource-leak-streams - java-correctness-span-events-list-only - java-correctness-mapper-state-leak + - java-perf-foreground-calls-background-only-001 runs: 2 minimum: cases: ["java-perf-lens-wrong-collection-001"] diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index e2d6965f772..39fd63a2f7f 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -372,6 +372,7 @@ cases: - id: java-perf-foreground-calls-background-only-001 files: - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md input: | Apply this repo's performance-review criteria (specifically the J16 declared-contract @@ -379,12 +380,12 @@ cases: reviewers/_common.md) to the following change. No git checkout available — this diff is the entire change to review. - `SimpleUtf8Cache.getUtf8(...)` is declared `@BackgroundOnly` (see its Javadoc below) — - its bookkeeping only pays off when it is only ever called from the single background - serializer thread. `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice that runs - synchronously on the calling application thread for every instrumented - `HttpURLConnection` request (a foreground/app-thread call site, not annotated - `@BackgroundOnly` or `@ForegroundSafe`). + `SimpleUtf8Cache` is declared `@BackgroundOnly` at the class level (see its Javadoc + below) — its bookkeeping only pays off when it is only ever called from the single + background serializer thread that owns it. `HttpUrlConnectionAdvice.onExit` is Byte + Buddy advice that runs synchronously on the calling application thread for every + instrumented `HttpURLConnection` request (a foreground/app-thread call site, not + annotated `@BackgroundOnly` or `@ForegroundSafe`). ```java // datadog/communication/serialization/SimpleUtf8Cache.java (existing, unchanged) @@ -394,23 +395,28 @@ cases: */ @BackgroundOnly public final class SimpleUtf8Cache implements EncodingCache { + public static final SimpleUtf8Cache SHARED = new SimpleUtf8Cache(); + public byte[] getUtf8(String value) { /* ... */ } } ``` ```diff - diff --git a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/httpurlconnection/HttpUrlConnectionAdvice.java b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/httpurlconnection/HttpUrlConnectionAdvice.java - @@ -40,6 +40,11 @@ public class HttpUrlConnectionAdvice { - + // PR #99999: tag the outgoing request's first header value for a new debug flag. - + @Advice.OnMethodExit - + public static void onExit(@Advice.This HttpURLConnection connection, - + @Advice.Argument(0) String headerValue, @Advice.Local("span") AgentSpan span) { - + byte[] utf8 = SHARED_UTF8_CACHE.getUtf8(headerValue); - + span.setTag("debug.header_bytes_len", utf8.length); - + } + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,12 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + // PR #99999: tag the outgoing request's first header value for a new debug flag. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit( + + @Advice.This final HttpURLConnection thiz, + + @Advice.FieldValue("connected") final boolean connected, + + @Advice.Local("headerValue") final String headerValue) { + + byte[] utf8 = SimpleUtf8Cache.SHARED.getUtf8(headerValue); + + thiz.setRequestProperty("X-Debug-Header-Bytes-Len", Integer.toString(utf8.length)); + + } ``` expected_criteria: - - "Flags that `onExit` — Byte Buddy advice running on the application thread for every instrumented `HttpURLConnection` call — reaches `SimpleUtf8Cache.getUtf8(...)`, a method declared `@BackgroundOnly`, per J16's declared-contract check." + - "Flags that `onExit` — Byte Buddy advice running on the application thread for every instrumented `HttpURLConnection` call — reaches `SimpleUtf8Cache.getUtf8(...)`, a class declared `@BackgroundOnly`, per J16's declared-contract check." - "Classifies this as SEV-1 / P0, not a soft nudge or generic allocation note." - "Recommends removing the call from advice or moving the work to the background/serializer thread that legitimately owns `SimpleUtf8Cache`, rather than caching the value differently in place." bad_signals: From 6f4702b25f01ec1018fccf3f22453d3697e7bd52 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 13:37:26 -0400 Subject: [PATCH 06/12] Make J16 severity follow the generic cost-based model Flat SEV-1 skipped the shared SEV->P mapping in reviewers/performance.md, so a reviewer who correctly applied the mapping would report P1 and fail the test's own hardcoded "SEV-1/P0" criterion. J16 now escalates from SEV-2/3 toward SEV-1/2 based on call-site frequency/cost, matching J13-J15, and gets the Fix: clause the others already have. Test case graded accordingly, plus a bad_signals entry for a future silent severity downgrade. Co-Authored-By: Claude Sonnet 5 --- .agents/dd-apm-sdk-review-overrides/reviewers/performance.md | 2 +- .llm-validation/suites/dd-apm-sdk-review.yaml | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index fdba7e5b583..631738aa984 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -30,7 +30,7 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input), but also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. A non-capturing lambda is a cached singleton (zero-alloc); a capturing lambda (closes over a local or `this`) is a new instance per evaluation — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. -- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence — **SEV-1** — a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup. **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. Currently-annotated classes: `DDCache` (`@ForegroundSafe`), `SimpleUtf8Cache` and `GenerationalUtf8Cache` (`@BackgroundOnly`, both `datadog.communication.serialization`). +- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. Currently-annotated classes: `DDCache` (`@ForegroundSafe`), `SimpleUtf8Cache` and `GenerationalUtf8Cache` (`@BackgroundOnly`, both `datadog.communication.serialization`). Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 39fd63a2f7f..81d6a85dfe3 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -417,9 +417,10 @@ cases: ``` expected_criteria: - "Flags that `onExit` — Byte Buddy advice running on the application thread for every instrumented `HttpURLConnection` call — reaches `SimpleUtf8Cache.getUtf8(...)`, a class declared `@BackgroundOnly`, per J16's declared-contract check." - - "Classifies this as SEV-1 / P0, not a soft nudge or generic allocation note." + - "Escalates severity toward SEV-1/2 (mapping to P1, absent OOM/SLA evidence) because the call site is a hot per-request path (every instrumented HttpURLConnection call), per J16's graduated severity guidance and the core SEV→P mapping — not a hardcoded top severity independent of call-site frequency." - "Recommends removing the call from advice or moving the work to the background/serializer thread that legitimately owns `SimpleUtf8Cache`, rather than caching the value differently in place." bad_signals: - "Treats this as an ordinary allocation or contention finding without naming the declared `@BackgroundOnly` contract violation." - "Assumes the cache is safe to call from advice because it is described as a \"cache\" without checking the annotation." - "Misses that `onExit` runs on the application/foreground thread, not the serializer thread." + - "Dismisses or downgrades the contract violation itself (e.g. calls it optional, stylistic, or something to fix \"eventually\") instead of treating a proven `@BackgroundOnly` crossing as a real, actionable finding regardless of which P-level it lands on." From c6f2fd453e1a3efff5ce91e630da1d8ace7979be Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 13:57:02 -0400 Subject: [PATCH 07/12] Match @NoEscape's Javadoc shape and clarify @ThreadSafe orthogonality Add the Checker contract section (Trigger/Not-a-trigger/Violation/ Compliant/Out-of-scope) that @NoEscape already uses, so the rule is machine-checkable straight from the annotation's own Javadoc without needing J16's prose. Also note that @BackgroundOnly is orthogonal to JSR-305 @ThreadSafe -- one governs concurrent-caller safety, the other which category of caller is valid at all -- since both now land on the same cache classes. APMLP-1543 Co-Authored-By: Claude Sonnet 5 --- .../trace/api/function/BackgroundOnly.java | 29 +++++++++++++++++++ .../trace/api/function/ForegroundSafe.java | 5 ++++ 2 files changed, 34 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java index 242652be53d..3c63c885672 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java +++ b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java @@ -20,12 +20,41 @@ *

The two markers are not symmetric -- see {@link ForegroundSafe} for why it, not this * one, is the strictly stronger guarantee. * + *

Orthogonal to {@code @ThreadSafe} (JSR-305): that annotation says concurrent calls from + * multiple callers are safe; this one says which category of caller is valid at all -- background + * threads (there may be more than one), never the foreground/application thread. A class can, and + * often will, carry both. + * *

On a type ({@link ElementType#TYPE}): every method of this type is background-only * unless a method-level {@link ForegroundSafe} widens it. * *

On a method ({@link ElementType#METHOD}): this method specifically is background-only, * regardless of what the enclosing type declares -- a method-level marker always wins over the * type-level one. + * + *

Checker contract. The rule below is written to be machine-checkable -- by a future + * static checker, or in the meantime by an AI reviewer (see the {@code dd-apm-sdk-review} skill's + * performance override, addendum J16) -- without needing to read this class's prose above. + * + *

*/ @Documented @Retention(RetentionPolicy.SOURCE) diff --git a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java index b1c0985dc3c..f25526097a8 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java +++ b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java @@ -29,6 +29,11 @@ *

On a method ({@link ElementType#METHOD}): this method specifically is foreground-safe, * regardless of what the enclosing type declares -- a method-level marker always wins over the * type-level one. + * + *

Checker contract. This annotation is not itself a trigger -- it is what makes a call + * site not suspect. See {@link BackgroundOnly}'s checker contract for the actual rule: + * code marked {@code @ForegroundSafe} (or carrying no marker) is exactly the caller side of the + * violation that contract flags when it reaches a {@code @BackgroundOnly} symbol. */ @Documented @Retention(RetentionPolicy.SOURCE) From 1c583b4c003cda21073f5c0676bde5fed935d9bb Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 14:03:06 -0400 Subject: [PATCH 08/12] Remove the header-value tangent from the J16 fixture Caching a caller-supplied HTTP header value pulled in an unrelated data-retention question. The violation under test is purely which thread reaches @BackgroundOnly code, so swap in a fixed, non-sensitive instrumentation-internal string instead. Co-Authored-By: Claude Sonnet 5 --- .llm-validation/suites/dd-apm-sdk-review.yaml | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 81d6a85dfe3..8485693c079 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -383,9 +383,11 @@ cases: `SimpleUtf8Cache` is declared `@BackgroundOnly` at the class level (see its Javadoc below) — its bookkeeping only pays off when it is only ever called from the single background serializer thread that owns it. `HttpUrlConnectionAdvice.onExit` is Byte - Buddy advice that runs synchronously on the calling application thread for every - instrumented `HttpURLConnection` request (a foreground/app-thread call site, not - annotated `@BackgroundOnly` or `@ForegroundSafe`). + Buddy advice, which always runs synchronously on the calling application thread — for + every instrumented `HttpURLConnection` request, not annotated `@BackgroundOnly` or + `@ForegroundSafe`. The value passed to the cache here is a fixed, non-sensitive + instrumentation-internal string (the integration name) — the violation is purely about + which thread reaches `@BackgroundOnly` code, not what data flows through it. ```java // datadog/communication/serialization/SimpleUtf8Cache.java (existing, unchanged) @@ -403,16 +405,15 @@ cases: ```diff diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java - @@ -95,6 +95,12 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { public static class HttpUrlConnectionAdvice { - + // PR #99999: tag the outgoing request's first header value for a new debug flag. + + private static final String INTEGRATION_NAME = "http-url-connection"; + + + + // PR #99999: pre-encode the integration name once so debug logging doesn't re-encode it. + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - + public static void onExit( - + @Advice.This final HttpURLConnection thiz, - + @Advice.FieldValue("connected") final boolean connected, - + @Advice.Local("headerValue") final String headerValue) { - + byte[] utf8 = SimpleUtf8Cache.SHARED.getUtf8(headerValue); - + thiz.setRequestProperty("X-Debug-Header-Bytes-Len", Integer.toString(utf8.length)); + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + byte[] utf8 = SimpleUtf8Cache.SHARED.getUtf8(INTEGRATION_NAME); + + log.debug("traced {} bytes via {}", utf8.length, thiz); + } ``` expected_criteria: From 7d5de5a49d3bdecc2f4949b8f9f84d496016a962 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 14:11:23 -0400 Subject: [PATCH 09/12] J16: keep only 2 illustrative examples, not an exhaustive class inventory Listing every currently-annotated class implied a complete, closed set; reframe as a non-exhaustive sample and point at grep for the real answer. Co-Authored-By: Claude Sonnet 5 --- .agents/dd-apm-sdk-review-overrides/reviewers/performance.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index 631738aa984..9150c4b18d4 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -30,11 +30,11 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input), but also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. A non-capturing lambda is a cached singleton (zero-alloc); a capturing lambda (closes over a local or `this`) is a new instance per evaluation — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. -- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. Currently-annotated classes: `DDCache` (`@ForegroundSafe`), `SimpleUtf8Cache` and `GenerationalUtf8Cache` (`@BackgroundOnly`, both `datadog.communication.serialization`). Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. +- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. For a feel of the shape (not an exhaustive list — grep for the current, complete set): `DDCache` carries `@ForegroundSafe`, `SimpleUtf8Cache` carries `@BackgroundOnly`. Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. -**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`), `ForegroundSafe`/`BackgroundOnly` (`datadog.trace.api.function`, scoped only to the three classes named in J16 above). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. +**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`), `ForegroundSafe`/`BackgroundOnly` (`datadog.trace.api.function`, scoped only to whatever J16's grep currently turns up — see J16 above, don't assume repo-wide coverage). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. ## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes From bfc2e98790671cfb88c78be40e1ac23a0abbe532 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 14:14:31 -0400 Subject: [PATCH 10/12] Add J16 negative test cases: unannotated callee and method-level override Covers two guard behaviors the existing violation-only case couldn't: absence of either marker on the callee is not itself a finding, and a method-level @ForegroundSafe correctly widens a @BackgroundOnly type for that method. Both wired into the gate preset (9 -> 11 cases). Co-Authored-By: Claude Sonnet 5 --- .llm-validation/README.md | 4 +- .llm-validation/config.yaml | 2 + .llm-validation/suites/dd-apm-sdk-review.yaml | 96 +++++++++++++++++++ 3 files changed, 100 insertions(+), 2 deletions(-) diff --git a/.llm-validation/README.md b/.llm-validation/README.md index 35b1c598f9f..de98811629c 100644 --- a/.llm-validation/README.md +++ b/.llm-validation/README.md @@ -56,7 +56,7 @@ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level full --runs 1 -# CI-shaped set (9 cases) +# CI-shaped set (11 cases) docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level gate --runs 1 @@ -110,7 +110,7 @@ that level already selected. | Level | Cases | Default runs | Use | |---|---|---|---| | `minimum` | **1** (`java-perf-lens-wrong-collection-001`) | 2 | First smoke | -| `gate` (default) | **8** listed in `config.yaml` | 2 | CI-shaped | +| `gate` (default) | **11** listed in `config.yaml` | 2 | CI-shaped | | `full` | **every** case in `suites/` | 2 | Broader pass | So this command runs **one** case once, not the whole suite: diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index e1ec7a8c448..a77b1574aca 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -34,6 +34,8 @@ presets: - java-correctness-span-events-list-only - java-correctness-mapper-state-leak - java-perf-foreground-calls-background-only-001 + - java-perf-foreground-calls-unannotated-002 + - java-perf-foreground-calls-method-override-003 runs: 2 minimum: cases: ["java-perf-lens-wrong-collection-001"] diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 8485693c079..225df01aef7 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -425,3 +425,99 @@ cases: - "Assumes the cache is safe to call from advice because it is described as a \"cache\" without checking the annotation." - "Misses that `onExit` runs on the application/foreground thread, not the serializer thread." - "Dismisses or downgrades the contract violation itself (e.g. calls it optional, stylistic, or something to fix \"eventually\") instead of treating a proven `@BackgroundOnly` crossing as a real, actionable finding regardless of which P-level it lands on." + + - id: java-perf-foreground-calls-unannotated-002 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + `LookupCache` carries neither `@ForegroundSafe` nor `@BackgroundOnly` — it predates both + annotations and has never been classified. `HttpUrlConnectionAdvice.onExit` is Byte Buddy + advice, which always runs synchronously on the calling application thread — for every + instrumented `HttpURLConnection` request. + + ```java + // datadog/communication/serialization/LookupCache.java (existing, unchanged, unannotated) + public final class LookupCache implements EncodingCache { + public static final LookupCache SHARED = new LookupCache(); + + public byte[] getUtf8(String value) { /* ... */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + private static final String INTEGRATION_NAME = "http-url-connection"; + + + + // PR #99998: pre-encode the integration name once so debug logging doesn't re-encode it. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + byte[] utf8 = LookupCache.SHARED.getUtf8(INTEGRATION_NAME); + + log.debug("traced {} bytes via {}", utf8.length, thiz); + + } + ``` + expected_criteria: + - "Does not raise a J16 declared-contract finding for this call — `LookupCache` carries neither marker annotation, and J16 is explicit that the absence of an annotation on the callee is not itself a finding." + - "If the response mentions `LookupCache` at all, it either says plainly that it is unclassified/unannotated and out of J16's scope, or raises a *different*, independently-justified finding (e.g. an ordinary per-call-allocation or hot-path concern under another addendum) rather than treating this as a `@BackgroundOnly` contract violation." + bad_signals: + - "Flags this as a `@BackgroundOnly` (or `@ForegroundSafe`) contract violation despite `LookupCache` carrying no such annotation." + - "Assumes `LookupCache` is background-only by analogy to `SimpleUtf8Cache` (same shape, similar name) rather than checking for the actual annotation." + - "Treats the mere presence of a cache-like class called from advice as sufficient grounds for a declared-contract finding, independent of whether any marker annotation is actually present." + + - id: java-perf-foreground-calls-method-override-003 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + `StatsCache` is declared `@BackgroundOnly` at the class level, but its `sizeHint()` method + carries its own method-level `@ForegroundSafe`, which overrides the type-level marker for + that method specifically per J16 (a method-level annotation always wins over the + enclosing type's). `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice, which always runs + synchronously on the calling application thread — for every instrumented + `HttpURLConnection` request — and calls only `sizeHint()`, never any other method on + `StatsCache`. + + ```java + // datadog/communication/serialization/StatsCache.java (existing, unchanged) + @BackgroundOnly + public final class StatsCache { + public static final StatsCache SHARED = new StatsCache(); + + @ForegroundSafe + public int sizeHint() { /* O(1) volatile field read, no allocation */ } + + public void recordSample(long value) { /* background bookkeeping */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,10 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + // PR #99997: size the debug-log buffer up front using the cache's current size hint. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + log.debug("cache sizeHint={} via {}", StatsCache.SHARED.sizeHint(), thiz); + + } + ``` + expected_criteria: + - "Does not raise a J16 declared-contract finding for this call — `sizeHint()` carries its own method-level `@ForegroundSafe`, which per J16 overrides the enclosing type's `@BackgroundOnly` for that method specifically, so calling it from foreground advice is compliant, not a violation." + - "If it discusses the annotations at all, correctly states that method-level markers win over type-level ones, and that this is exactly the compliant case, not merely an unclassified or borderline one." + bad_signals: + - "Flags this as a `@BackgroundOnly` violation by looking only at the class-level annotation on `StatsCache` and ignoring the method-level `@ForegroundSafe` on `sizeHint()`." + - "Treats the presence of `@BackgroundOnly` anywhere on the type as disqualifying every method on it, contradicting J16's explicit method-overrides-type rule." + - "Recommends removing or moving the `sizeHint()` call as if it were a real contract crossing." From a6310c50782c28fb5ddc0743c6025b3a589d9eba Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 15:26:28 -0400 Subject: [PATCH 11/12] Document inheritance-narrowing rule, switch to CLASS retention, add J16 test coverage Adds the declaration-site inheritance-narrowing rule to both annotations' Javadoc and to the performance lens's J16 addendum (now slugged background-only-contract for stable cross-referencing), switches both annotations from SOURCE to CLASS retention so a future bytecode checker (APMLP-1645) can see them, adds llm-validation fixtures covering the interface-hop and inheritance-widening cases, and documents the foreground/background contract in instrumentation_design_guidelines.md. Co-Authored-By: Claude Sonnet 5 --- .../reviewers/performance.md | 13 +- .llm-validation/README.md | 4 +- .llm-validation/config.yaml | 2 + .llm-validation/suites/dd-apm-sdk-review.yaml | 132 ++++++++++++++---- .../serialization/EncodingCache.java | 7 + docs/instrumentation_design_guidelines.md | 16 +++ .../java/datadog/trace/api/cache/DDCache.java | 7 - .../trace/api/cache/FixedSizeCache.java | 6 + .../trace/api/function/BackgroundOnly.java | 22 ++- .../trace/api/function/ForegroundSafe.java | 9 +- 10 files changed, 179 insertions(+), 39 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index 9150c4b18d4..0993d791df2 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -30,7 +30,18 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input), but also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. A non-capturing lambda is a cached singleton (zero-alloc); a capturing lambda (closes over a local or `this`) is a new instance per evaluation — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. -- **J16 — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`SOURCE`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. For a feel of the shape (not an exhaustive list — grep for the current, complete set): `DDCache` carries `@ForegroundSafe`, `SimpleUtf8Cache` carries `@BackgroundOnly`. Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. +- **J16 (slug: `background-only-contract`) — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`CLASS`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. + + **Inheritance direction is a second, independent trigger — checked at the declaration, not the call site.** The full rule and rationale live in `BackgroundOnly`'s Javadoc ("Inheritance direction" section, mirrored in `ForegroundSafe`'s) — don't restate it here, just apply it: an override may only narrow a supertype's declared cost, never widen it. The checker-facing consequence: overriding a `@ForegroundSafe` or unannotated supertype method and marking the override `@BackgroundOnly` is flag-with-confidence the moment it appears in the diff, whether or not any foreground call site currently reaches it — the reverse direction (narrowing `@BackgroundOnly` to `@ForegroundSafe`) is compliant, not a finding. + + Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **This is local type-tracking (creation point to usage point within a class/file), not cross-file data-flow.** Extend resolution exactly two hops beyond the direct case, both anchored to files already in hand — never the reverse (enumerating every implementer of an interface to see whether any one of them is `@BackgroundOnly`); that search is unbounded, repo-wide, and belongs to APMLP-1645's real resolver, not this grep-based check: + + 1. **Concrete → declared interface, method-scoped.** When a foreground call site is typed to an interface `I` rather than the concrete class, and some implementation `C` of `I` — already in hand, i.e. visible in the diff or files handed to you, never found by searching the repo for implementers — is `@BackgroundOnly` (type- or method-level), read `C`'s `implements`/`extends` clause and match `C`'s `@Override` methods against `I`'s method signatures. Propagate the marker only to *that signature* on `I` — not to every method `I` declares. A foreground call to a different, unrelated method on `I` is not a finding. + 2. **Default method transitively calling a flagged signature.** If `I` declares a default method `d()` whose body calls one of the signatures flagged in step 1, `d()` inherits the flag too. This is a single read of `I`'s own source — do not chase the call further than one interface file. + + Neither hop proves the call site's interface reference actually resolves to `C` at runtime (that's still unprovable by grep) — treat a hit from either hop as flag-with-confidence the same as the direct case, since a call site that would only ever reach a *safe* implementer of `I` is the rare case, not the default one to assume. + +**Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. For a feel of the shape (not an exhaustive list — grep for the current, complete set): `FixedSizeCache` carries `@ForegroundSafe` (bounded-probe implementation only — the `DDCache` interface itself and its other implementations, e.g. `CHMCache`, are deliberately unannotated since their cost isn't uniformly bounded), `SimpleUtf8Cache` carries `@BackgroundOnly`. Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. diff --git a/.llm-validation/README.md b/.llm-validation/README.md index de98811629c..be299ca5176 100644 --- a/.llm-validation/README.md +++ b/.llm-validation/README.md @@ -56,7 +56,7 @@ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level full --runs 1 -# CI-shaped set (11 cases) +# CI-shaped set (13 cases) docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level gate --runs 1 @@ -110,7 +110,7 @@ that level already selected. | Level | Cases | Default runs | Use | |---|---|---|---| | `minimum` | **1** (`java-perf-lens-wrong-collection-001`) | 2 | First smoke | -| `gate` (default) | **11** listed in `config.yaml` | 2 | CI-shaped | +| `gate` (default) | **13** listed in `config.yaml` | 2 | CI-shaped | | `full` | **every** case in `suites/` | 2 | Broader pass | So this command runs **one** case once, not the whole suite: diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index a77b1574aca..81e4b195a4b 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -36,6 +36,8 @@ presets: - java-perf-foreground-calls-background-only-001 - java-perf-foreground-calls-unannotated-002 - java-perf-foreground-calls-method-override-003 + - java-perf-foreground-calls-interface-hop-004 + - java-perf-inheritance-widen-violation-005 runs: 2 minimum: cases: ["java-perf-lens-wrong-collection-001"] diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 225df01aef7..eb9cdc07c2a 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -380,17 +380,10 @@ cases: reviewers/_common.md) to the following change. No git checkout available — this diff is the entire change to review. - `SimpleUtf8Cache` is declared `@BackgroundOnly` at the class level (see its Javadoc - below) — its bookkeeping only pays off when it is only ever called from the single - background serializer thread that owns it. `HttpUrlConnectionAdvice.onExit` is Byte - Buddy advice, which always runs synchronously on the calling application thread — for - every instrumented `HttpURLConnection` request, not annotated `@BackgroundOnly` or - `@ForegroundSafe`. The value passed to the cache here is a fixed, non-sensitive - instrumentation-internal string (the integration name) — the violation is purely about - which thread reaches `@BackgroundOnly` code, not what data flows through it. - ```java - // datadog/communication/serialization/SimpleUtf8Cache.java (existing, unchanged) + // datadog/communication/serialization/SimpleUtf8Cache.java (hypothetical shape, for + // illustration — annotation and bookkeeping rationale match the real class; the no-arg + // constructor and SHARED field below are simplified for this example) /** * {@link BackgroundOnly}: bookkeeping to reuse the encoded byte[] only pays off when this * is confined to the single background serializer thread that owns it. @@ -413,7 +406,7 @@ cases: + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void onExit(@Advice.This final HttpURLConnection thiz) { + byte[] utf8 = SimpleUtf8Cache.SHARED.getUtf8(INTEGRATION_NAME); - + log.debug("traced {} bytes via {}", utf8.length, thiz); + + log.debug("traced {} bytes via connection", utf8.length); + } ``` expected_criteria: @@ -437,13 +430,8 @@ cases: reviewers/_common.md) to the following change. No git checkout available — this diff is the entire change to review. - `LookupCache` carries neither `@ForegroundSafe` nor `@BackgroundOnly` — it predates both - annotations and has never been classified. `HttpUrlConnectionAdvice.onExit` is Byte Buddy - advice, which always runs synchronously on the calling application thread — for every - instrumented `HttpURLConnection` request. - ```java - // datadog/communication/serialization/LookupCache.java (existing, unchanged, unannotated) + // datadog/communication/serialization/LookupCache.java (hypothetical, for illustration) public final class LookupCache implements EncodingCache { public static final LookupCache SHARED = new LookupCache(); @@ -461,7 +449,7 @@ cases: + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void onExit(@Advice.This final HttpURLConnection thiz) { + byte[] utf8 = LookupCache.SHARED.getUtf8(INTEGRATION_NAME); - + log.debug("traced {} bytes via {}", utf8.length, thiz); + + log.debug("traced {} bytes via connection", utf8.length); + } ``` expected_criteria: @@ -483,16 +471,12 @@ cases: reviewers/_common.md) to the following change. No git checkout available — this diff is the entire change to review. - `StatsCache` is declared `@BackgroundOnly` at the class level, but its `sizeHint()` method - carries its own method-level `@ForegroundSafe`, which overrides the type-level marker for - that method specifically per J16 (a method-level annotation always wins over the - enclosing type's). `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice, which always runs - synchronously on the calling application thread — for every instrumented - `HttpURLConnection` request — and calls only `sizeHint()`, never any other method on - `StatsCache`. + `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice, which always runs synchronously on + the calling application thread — for every instrumented `HttpURLConnection` request — and + calls only `sizeHint()`, never any other method on `StatsCache`. ```java - // datadog/communication/serialization/StatsCache.java (existing, unchanged) + // datadog/communication/serialization/StatsCache.java (hypothetical, for illustration) @BackgroundOnly public final class StatsCache { public static final StatsCache SHARED = new StatsCache(); @@ -511,7 +495,7 @@ cases: + // PR #99997: size the debug-log buffer up front using the cache's current size hint. + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void onExit(@Advice.This final HttpURLConnection thiz) { - + log.debug("cache sizeHint={} via {}", StatsCache.SHARED.sizeHint(), thiz); + + log.debug("cache sizeHint={}", StatsCache.SHARED.sizeHint()); + } ``` expected_criteria: @@ -521,3 +505,97 @@ cases: - "Flags this as a `@BackgroundOnly` violation by looking only at the class-level annotation on `StatsCache` and ignoring the method-level `@ForegroundSafe` on `sizeHint()`." - "Treats the presence of `@BackgroundOnly` anywhere on the type as disqualifying every method on it, contradicting J16's explicit method-overrides-type rule." - "Recommends removing or moving the `sizeHint()` call as if it were a real contract crossing." + + - id: java-perf-foreground-calls-interface-hop-004 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + ```java + // datadog/communication/serialization/TagCache.java (hypothetical, for illustration) + public interface TagCache { + byte[] encode(String s); + } + ``` + + ```java + // datadog/communication/serialization/StatsTagCache.java (hypothetical, for illustration) + @BackgroundOnly + public final class StatsTagCache implements TagCache { + public static final StatsTagCache SHARED = new StatsTagCache(); + + @Override + public byte[] encode(String s) { /* background-only bookkeeping */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + private static final String INTEGRATION_NAME = "http-url-connection"; + + // PR #99996: cache is referenced through its interface type here, not the concrete class. + + private static final TagCache CACHE = StatsTagCache.SHARED; + + + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit() { + + byte[] utf8 = CACHE.encode(INTEGRATION_NAME); + + } + ``` + expected_criteria: + - "Flags this as a J16 declared-contract violation even though the call site (`CACHE.encode(...)`) is typed to the unannotated interface `TagCache`, not the `@BackgroundOnly` concrete class `StatsTagCache` — by reading `StatsTagCache`'s `implements TagCache` clause and matching its `@Override encode(String)` against `TagCache.encode(String)`, per J16's concrete-to-interface, method-scoped propagation hop." + - "Identifies the `onExit` advice body as the foreground call site, since Byte Buddy advice always runs synchronously on the application thread." + bad_signals: + - "Stays silent because the declared type of `CACHE` (`TagCache`) carries no annotation, without checking whether a `@BackgroundOnly` implementer of that interface is present in the same files." + - "Treats this as unresolvable because the call site isn't textually adjacent to the `@BackgroundOnly` annotation, instead of applying the one-hop concrete-to-interface propagation J16 describes." + - "Over-generalizes by claiming every method on `TagCache` is now background-only, rather than propagating the flag only to the specific `encode(String)` signature `StatsTagCache` overrides." + + - id: java-perf-inheritance-widen-violation-005 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. There is no foreground call site to this new override anywhere + in this diff or elsewhere in the codebase yet. + + ```java + // datadog/communication/serialization/TagCache.java (hypothetical, for illustration) + public interface TagCache { + @ForegroundSafe + byte[] encode(String s); + } + ``` + + ```diff + diff --git a/datadog/communication/serialization/HeavyTagCache.java b/datadog/communication/serialization/HeavyTagCache.java + new file mode 100644 + @@ -0,0 +1,10 @@ + +package datadog.communication.serialization; + + + +public final class HeavyTagCache implements TagCache { + + + + @Override + + @BackgroundOnly + + public byte[] encode(String s) { + + // heavy bookkeeping, safe only from the background serializer thread + + return null; + + } + +} + ``` + expected_criteria: + - "Flags this as a J16 declared-contract violation at the declaration site itself — `HeavyTagCache.encode` overrides `TagCache.encode`, which is `@ForegroundSafe`, and marks the override `@BackgroundOnly`, which widens the contract rather than narrowing it." + - "States plainly that this finding does not require any current foreground call site to exist — every existing and future caller holding a `TagCache`-typed reference already assumed the `@ForegroundSafe` guarantee and would be broken by this override, per J16's inheritance-direction rule." + bad_signals: + - "Stays silent because no foreground call site reaches `HeavyTagCache.encode` yet, treating the declaration-site rule as if it only applied once a call path exists." + - "Approves this as a normal narrowing override (confusing the direction), missing that narrowing only goes the other way: `@BackgroundOnly` supertype to `@ForegroundSafe` override is fine, the reverse is not." + - "Flags a different, unrelated concern (e.g. plain per-call allocation) without ever naming the inheritance-direction violation that J16 specifically calls out." diff --git a/communication/src/main/java/datadog/communication/serialization/EncodingCache.java b/communication/src/main/java/datadog/communication/serialization/EncodingCache.java index 4833eb8695a..d0b6bc086b3 100644 --- a/communication/src/main/java/datadog/communication/serialization/EncodingCache.java +++ b/communication/src/main/java/datadog/communication/serialization/EncodingCache.java @@ -1,6 +1,13 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; + // TODO @FunctionalInterface +/** + * {@link BackgroundOnly}: every implementation ({@link SimpleUtf8Cache}, {@link + * GenerationalUtf8Cache}) is only safe to call from the writer/serializer thread that owns it. + */ +@BackgroundOnly public interface EncodingCache { byte[] encode(CharSequence s); diff --git a/docs/instrumentation_design_guidelines.md b/docs/instrumentation_design_guidelines.md index 305ff651439..6e4676e49d1 100644 --- a/docs/instrumentation_design_guidelines.md +++ b/docs/instrumentation_design_guidelines.md @@ -81,3 +81,19 @@ Context ctx = Context.current(); // GOOD - use the bytecode bridge, static-imported Context ctx = currentContext(); ``` + +### 4. Foreground/background thread cost contract + +**Why it matters:** + +- Advice bodies and span lifecycle methods run on the application (foreground) thread, where added cost is + customer-visible latency, as opposed to a background thread the tracer owns and paces itself +- Code meant to run only on a background thread must never become reachable from a foreground call site + +**What to do:** + +Mark code with `datadog.trace.api.function.ForegroundSafe` (cheap enough for either thread) or `.BackgroundOnly` +(must never be reached from the foreground) where the distinction matters, and see their Javadoc for the full +contract, including the inheritance-narrowing rule for overrides. This is a documentation-and-tooling convention +today (not yet compiler- or runtime-enforced); it is checked by hand and by the `dd-apm-sdk-review` skill's +performance lens. diff --git a/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java b/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java index 17888187a83..d1cd8024847 100644 --- a/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java +++ b/internal-api/src/main/java/datadog/trace/api/cache/DDCache.java @@ -1,15 +1,8 @@ package datadog.trace.api.cache; -import datadog.trace.api.function.ForegroundSafe; import java.util.function.BiConsumer; import java.util.function.Function; -/** - * {@link ForegroundSafe}: every implementation looks up a key via a small, bounded number of probes - * (no growth, no eviction bookkeeping beyond overwriting a slot) -- cheap enough to call from an - * application thread. - */ -@ForegroundSafe public interface DDCache { /** * Look up or create and store a value in the cache. diff --git a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java index a8f5fc5f3fa..7818e67ce29 100644 --- a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java +++ b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java @@ -1,6 +1,7 @@ package datadog.trace.api.cache; import datadog.trace.api.Pair; +import datadog.trace.api.function.ForegroundSafe; import java.util.Arrays; import java.util.function.BiConsumer; import java.util.function.Function; @@ -16,9 +17,14 @@ * computeIfAbsent is idempotent, or otherwise you might not get back the value you expect * from a cache lookup. * + *

{@link ForegroundSafe}: {@code computeIfAbsent} always does a small, bounded number of probes + * against a fixed-size array -- no growth, no eviction sweep -- cheap enough to call from an + * application thread. + * * @param key type * @param value type */ +@ForegroundSafe abstract class FixedSizeCache implements DDCache { static final int MAXIMUM_CAPACITY = 1 << 30; diff --git a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java index 3c63c885672..c20e80df70c 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java +++ b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java @@ -32,6 +32,17 @@ * regardless of what the enclosing type declares -- a method-level marker always wins over the * type-level one. * + *

Inheritance direction. An override may only narrow a supertype's declared cost, never + * widen it -- the same variance rule as a covariant return type, applied to a cost contract instead + * of a value type. A method overriding a {@code @BackgroundOnly} supertype/interface method may + * itself be marked {@link ForegroundSafe} if that override happens to be cheap: every caller + * holding a reference typed to the supertype already assumed the worse (background-only) case, so a + * cheaper override can't surprise them. The reverse can't be done safely: overriding a {@link + * ForegroundSafe} or unannotated supertype method and marking the override {@code @BackgroundOnly} + * breaks the promise for every existing caller holding a supertype-typed reference, without their + * code changing at all -- this is a violation at the declaration site itself, independent of + * whether any foreground call site currently exists in the diff. + * *

Checker contract. The rule below is written to be machine-checkable -- by a future * static checker, or in the meantime by an AI reviewer (see the {@code dd-apm-sdk-review} skill's * performance override, addendum J16) -- without needing to read this class's prose above. @@ -51,12 +62,21 @@ * {@code @BackgroundOnly}. *

  • Compliant example: the same call made only from the background serializer thread * that owns the cache, or from a method itself marked {@code @BackgroundOnly}. + *
  • Also a trigger, at the declaration site: a method overriding a {@link + * ForegroundSafe} or unannotated supertype/interface method that marks the override + * {@code @BackgroundOnly} -- see "Inheritance direction" above. Flag this the moment it + * appears; it does not require a foreground call site to exist yet. *
  • Out of scope (v1): resolution is grep-only today (there is no APT-generated manifest * yet -- {@code APMLP-1645}), so a callee outside the diff costs a grep per unfamiliar symbol * rather than a lookup; reflection and dynamic-proxy call sites are not resolved at all. + *
  • Propagation across an interface boundary (concrete implementation to the interface + * method it implements, and one hop further into a default method that calls it) is a bounded + * extension of the direct-call trigger above, not a separate rule -- see the {@code + * dd-apm-sdk-review} skill's performance override, addendum {@code background-only-contract}, + * for its exact two-hop scope. * */ @Documented -@Retention(RetentionPolicy.SOURCE) +@Retention(RetentionPolicy.CLASS) @Target({ElementType.TYPE, ElementType.METHOD}) public @interface BackgroundOnly {} diff --git a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java index f25526097a8..71fe982d21c 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java +++ b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java @@ -30,12 +30,19 @@ * regardless of what the enclosing type declares -- a method-level marker always wins over the * type-level one. * + *

    Inheritance direction: an override may narrow a {@link BackgroundOnly} supertype/ + * interface method to {@code @ForegroundSafe} (a cheaper override can't surprise a caller who + * already assumed the worse case) but may never widen a {@code @ForegroundSafe} or unannotated + * supertype method to {@link BackgroundOnly} -- see {@link BackgroundOnly}'s "Inheritance + * direction" for the full rule and why the reverse direction is a declaration-site violation on its + * own. + * *

    Checker contract. This annotation is not itself a trigger -- it is what makes a call * site not suspect. See {@link BackgroundOnly}'s checker contract for the actual rule: * code marked {@code @ForegroundSafe} (or carrying no marker) is exactly the caller side of the * violation that contract flags when it reaches a {@code @BackgroundOnly} symbol. */ @Documented -@Retention(RetentionPolicy.SOURCE) +@Retention(RetentionPolicy.CLASS) @Target({ElementType.TYPE, ElementType.METHOD}) public @interface ForegroundSafe {} From 8163b8974127e9d72130936654c685fd81ff4b09 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 11 Sep 2026 15:45:34 -0400 Subject: [PATCH 12/12] Address Codex findings: scope FixedSizeCache's cost claim, fix J16 false-positive FixedSizeCache's @ForegroundSafe Javadoc overclaimed: computeIfAbsent's producer and visit()'s consumer are caller-supplied and can be arbitrarily expensive, so the annotation now scopes its guarantee to the cache's own bookkeeping only. J16's interface-propagation hop-1 flagged any interface-typed call site reachable to a @BackgroundOnly implementer C, even when the diff visibly binds the receiver to a different, safe implementation -- guaranteed unreachable. Hop-1 now checks for a visible binding first and only falls back to flag-with-confidence when the receiver's concrete type isn't resolvable from the diff. Co-Authored-By: Claude Sonnet 5 --- .../reviewers/performance.md | 4 ++-- .../java/datadog/trace/api/cache/FixedSizeCache.java | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index 0993d791df2..546602a91dc 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -36,10 +36,10 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **This is local type-tracking (creation point to usage point within a class/file), not cross-file data-flow.** Extend resolution exactly two hops beyond the direct case, both anchored to files already in hand — never the reverse (enumerating every implementer of an interface to see whether any one of them is `@BackgroundOnly`); that search is unbounded, repo-wide, and belongs to APMLP-1645's real resolver, not this grep-based check: - 1. **Concrete → declared interface, method-scoped.** When a foreground call site is typed to an interface `I` rather than the concrete class, and some implementation `C` of `I` — already in hand, i.e. visible in the diff or files handed to you, never found by searching the repo for implementers — is `@BackgroundOnly` (type- or method-level), read `C`'s `implements`/`extends` clause and match `C`'s `@Override` methods against `I`'s method signatures. Propagate the marker only to *that signature* on `I` — not to every method `I` declares. A foreground call to a different, unrelated method on `I` is not a finding. + 1. **Concrete → declared interface, method-scoped.** When a foreground call site is typed to an interface `I` rather than the concrete class, and some implementation `C` of `I` — already in hand, i.e. visible in the diff or files handed to you, never found by searching the repo for implementers — is `@BackgroundOnly` (type- or method-level), read `C`'s `implements`/`extends` clause and match `C`'s `@Override` methods against `I`'s method signatures. Propagate the marker only to *that signature* on `I` — not to every method `I` declares. A foreground call to a different, unrelated method on `I` is not a finding. **First check whether the call site's receiver is visibly bound in the diff** (a local `new`/factory-method assignment, a constructor/field initializer, a DI wire-up you can see) **to a specific implementation of `I` other than `C`.** When it is, and that other implementation is not itself `@BackgroundOnly`, resolve against what's actually visible instead of `C` — this is not a finding, since `C` cannot be what that particular reference calls. Reserve hop 1 for the case where the receiver's concrete type isn't visible in hand (a parameter, a field populated elsewhere, a return value from outside the diff) and `C` is the only candidate implementation you have to reason from. 2. **Default method transitively calling a flagged signature.** If `I` declares a default method `d()` whose body calls one of the signatures flagged in step 1, `d()` inherits the flag too. This is a single read of `I`'s own source — do not chase the call further than one interface file. - Neither hop proves the call site's interface reference actually resolves to `C` at runtime (that's still unprovable by grep) — treat a hit from either hop as flag-with-confidence the same as the direct case, since a call site that would only ever reach a *safe* implementer of `I` is the rare case, not the default one to assume. + Neither hop proves the call site's interface reference actually resolves to `C` at runtime when the receiver's concrete type isn't visible (that's still unprovable by grep) — treat a hit from either hop as flag-with-confidence in that case, same as the direct case, since a call site that would only ever reach a *safe* implementer of `I` is the rare case, not the default one to assume absent evidence. But when the diff *does* show the receiver bound to a specific, different, safe implementation, that visible evidence overrides the default assumption — don't flag against a `C` the call site provably can't reach. **Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. For a feel of the shape (not an exhaustive list — grep for the current, complete set): `FixedSizeCache` carries `@ForegroundSafe` (bounded-probe implementation only — the `DDCache` interface itself and its other implementations, e.g. `CHMCache`, are deliberately unannotated since their cost isn't uniformly bounded), `SimpleUtf8Cache` carries `@BackgroundOnly`. Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. diff --git a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java index 7818e67ce29..3f832874b5c 100644 --- a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java +++ b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java @@ -17,9 +17,13 @@ * computeIfAbsent is idempotent, or otherwise you might not get back the value you expect * from a cache lookup. * - *

    {@link ForegroundSafe}: {@code computeIfAbsent} always does a small, bounded number of probes - * against a fixed-size array -- no growth, no eviction sweep -- cheap enough to call from an - * application thread. + *

    {@link ForegroundSafe}: the cache's own bookkeeping -- {@code computeIfAbsent}'s probing, + * {@code clear()}, {@code visit()}'s traversal -- always does a small, bounded amount of work + * against a fixed-size array: no growth, no eviction sweep. This guarantee covers only that + * bookkeeping, not the caller-supplied {@code producer} passed to {@code computeIfAbsent} or the + * {@code consumer} passed to {@code visit()} -- their cost is the caller's responsibility, exactly + * as with any higher-order method; a slow or blocking producer/consumer is not made safe by this + * annotation. * * @param key type * @param value type