Add a perf-review check for @BackgroundOnly code reached from the foreground (AI perf review)

.agents/skills/perf-review/references/checks.md

@@ -27,6 +27,7 @@ Format: **pattern** — *expensive when (the interprocedural condition to trace)
 7. **Escape / allocation-elision defeated** *(Java/Go/.NET/V8 all have a version)* — *a refactor makes a previously-local object escape (stored, returned, captured by a closure, passed to a virtual/non-inlined call) → silent heap allocation on a hot path* — **flag-as-measure** ("may now escape and allocate; verify with an allocation profiler") — SEV-2/3 — fix: keep it local; avoid the escaping store/capture.
 8. **`@NoEscape` field-storage violation** — *a field (instance or static, directly or as a generic type argument) declared with an `@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`), **or** initialized directly from a call to an `@NoEscape`-annotated method — with no comment at the declaration justifying the retention*. The type form marks a concrete type (e.g. `SubSequence`); the method form exists for a return value whose concrete type can't itself carry the annotation (an anonymous class or lambda implementing a JDK interface). The annotation's own javadoc carries a self-contained "Checker contract" section (trigger / not-a-trigger / violation example / compliant example, both forms) written so this can be checked from the diff alone, with no other context needed. The underlying rule is "should", not "must" (RFC-2119 sense): a trigger is a presumptive finding, not an automatic failure — compliant if the declaration carries either a `// Retained on purpose: <reason>`-style comment, or `@SuppressPerfContract(value = NoEscape.class, reason = "...")` (`datadog.perfcontract.SuppressPerfContract` — the generic exemption mechanism shared by every perf-contract marker, not `@NoEscape`-specific), or a named canned-exception annotation itself meta-annotated `@SuppressPerfContract` with `NoEscape.class` among its `value` (e.g. a hypothetical `@Borrowed`) — treat the meta-annotation's own `reason` as satisfying the requirement. Don't flag a `SuppressPerfContract`/canned-exception use as non-compliant just for being terse; that mirrors the annotation's own Checker contract. **flag-with-confidence** — SEV-2/3 (SEV-1 if the annotated type/method shares backing storage with something large, per its own javadoc). Current wearers: `SubSequence`, `Maybe` (type form; see J7 below for `SubSequence`'s specific retention-vs-transient discriminator). **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.**
 9. **`@StaticLifetime` field-lifetime violation** — *a field annotated `@StaticLifetime` (`datadog.perfcontract.StaticLifetime`), **or** an unannotated field whose declared type is listed in the contract's `@ImpliedFor` (`datadog.perfcontract.ImpliedFor`; read the types from the `@ImpliedFor` on `StaticLifetime` itself, subtypes included), that is neither `static final` nor a `final` instance field declared on a class annotated `@Singleton` (`datadog.perfcontract.Singleton`)*. Declaration-scan only, same shape as the `@NoEscape` field-storage check above — it does not attempt to prove a field is actually reused enough to be worth caching, and it does not catch an unannotated per-instance cache of a non-implied type; it only guards fields that opt in or whose type implies the contract. A deliberate per-instance holder of an implied type is compliant with `@SuppressPerfContract(value = StaticLifetime.class, reason = "...")`. `@Singleton`, applied to a class, is a trusted, unverified declaration (v1 posture — the same stance the other perf-contract markers take toward `static final` itself) that the class has exactly one process-wide instance; declaring it satisfies `@StaticLifetime` for that class's `final` instance fields. **flag-with-confidence** — SEV-2/3, motivated by a real production defect (a per-request-constructed cache that never actually amortized anything — see the annotation's own javadoc for the full trigger/accepted-form table). Refines the "per-call cache / expensive-object creation that should be static/once" deterministic-lint candidate below by giving that pattern's opted-in subset an explicit annotation and trigger; the unannotated general case is still that line's job. **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.**
+10. **`@BackgroundOnly` reached from the foreground** — *foreground code that directly calls background-only code, as defined by the checker contract in `@BackgroundOnly`'s Javadoc (`datadog.trace.api.function.BackgroundOnly`; `@ForegroundSafe` marks where the foreground starts)*. Background-only work (serialization, stats aggregation, eviction) is paced by a tracer-owned thread; run on an application thread, the same cost becomes customer-visible latency. Direct calls only: a call that reaches `@BackgroundOnly` code through unmarked intermediate methods is out of scope for the contract. Two levels of confidence: **flag-with-confidence** when the caller is marked `@ForegroundSafe` (or sits on a `@ForegroundSafe` type), or is an advice method (`@Advice.OnMethodEnter` / `@Advice.OnMethodExit`), because both ends are declared — ByteBuddy inlines advice into the instrumented application method, so it is foreground by construction; **flag-as-measure** when the caller is unmarked but sits on the hot-path map above (decorators, span create/setTag/finish, tag-map ops, propagation), because "foreground" is then inferred rather than declared. Advice on a thread the tracer owns is the rare exception; it takes `@SuppressPerfContract(value = BackgroundOnly.class, reason = "...")`. Not a trigger: handing the work to a background thread (enqueueing it) instead of running it; background-only code calling `@ForegroundSafe` code; a calling method with `@SuppressPerfContract(value = BackgroundOnly.class, reason = "...")`. SEV-2 (SEV-1 if the background-only work blocks — I/O, a contended lock, or an unbounded loop) — fix: defer the work to the background thread that owns it (enqueue, or record and let the background drain compute). **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands (APMLP-1645) and it can migrate down.**
 
 ## Deterministic-lint candidates (DON'T spend AI budget — make these real lints)
 Fixed-signature, mechanically checkable:

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;
@@ -12,7 +13,9 @@
  *
  * <p>NOTE: The aim of this cache is to reduce allocation overhead -- not CPU overhead. Using the
  * cache has higher CPU overhead than simply calling {@link
- * String#getBytes(java.nio.charset.Charset)}.
+ * String#getBytes(java.nio.charset.Charset)}. That trade only pays off on a serializer thread, so
+ * the cache is {@link BackgroundOnly}: on an application thread, the extra CPU becomes request
+ * latency.
  *
  * <p>The cache is thread safe.
  */
@@ -64,6 +67,7 @@
  * provide better cache utilization.
  */
 @ThreadSafe
+@BackgroundOnly
 @SuppressFBWarnings(
     value = "IS2_INCONSISTENT_SYNC",
     justification =

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;
 
@@ -10,7 +11,9 @@
  *
  * <p>NOTE: The aim of this cache is to reduce allocation overhead -- not CPU overhead. Using the
  * cache has higher CPU overhead than simply calling {@link
- * String#getBytes(java.nio.charset.Charset)}.
+ * String#getBytes(java.nio.charset.Charset)}. That trade only pays off on a serializer thread, so
+ * the cache is {@link BackgroundOnly}: on an application thread, the extra CPU becomes request
+ * latency.
  *
  * <p>The cache is thread safe.
  */
@@ -43,6 +46,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;
 

internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java

@@ -1,5 +1,7 @@
 package datadog.trace.api.function;
 
+import datadog.perfcontract.PerfContract;
+import datadog.perfcontract.SuppressPerfContract;
 import java.lang.annotation.Documented;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -12,10 +14,9 @@
  * thread (the foreground; see {@link ForegroundSafe}), where its cost would become customer-visible
  * latency instead.
  *
- * <p>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 <b>not yet enforced</b>; hold to it by hand until the checker lands.
+ * <p>This marker changes no runtime behavior. The checker contract below guides AI review through
+ * the perf-review skill's {@code checks.md}; no static checker enforces it yet (see {@code
+ * APMLP-1645}).
  *
  * <p>The two markers are <b>not symmetric</b> -- see {@link ForegroundSafe} for why it, not this
  * one, is the strictly stronger guarantee.
@@ -26,8 +27,34 @@
  * <p><b>On a method</b> ({@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.
+ *
+ * <p><b>Checker contract.</b> A trigger is a presumptive finding to raise, not an automatic
+ * failure.
+ *
+ * <ul>
+ *   <li><b>Foreground code:</b> a method annotated {@link ForegroundSafe}; a method of a type
+ *       annotated {@code @ForegroundSafe} that has no method-level {@code @BackgroundOnly}; or an
+ *       advice method annotated {@code @Advice.OnMethodEnter} or {@code @Advice.OnMethodExit}.
+ *       ByteBuddy inlines advice into the instrumented application method, so it runs on the
+ *       application thread by construction.
+ *   <li><b>Background-only code:</b> a method annotated {@code @BackgroundOnly}, or a method of a
+ *       type annotated {@code @BackgroundOnly} that has no method-level {@code @ForegroundSafe}.
+ *   <li><b>Trigger:</b> foreground code that directly calls background-only code.
+ *   <li><b>Not a trigger:</b> background-only code calling foreground code; foreground code that
+ *       hands work to a background thread (for example by enqueueing it) without running it; or a
+ *       calling method annotated {@link SuppressPerfContract} with {@code BackgroundOnly.class} in
+ *       its {@code value} and a non-blank reason.
+ *   <li><b>Violation example:</b> a {@code @ForegroundSafe} {@code onSpanFinished(span)} that calls
+ *       a {@code @BackgroundOnly} {@code aggregate(span)}.
+ *   <li><b>Compliant example:</b> the same {@code onSpanFinished(span)} that only enqueues {@code
+ *       span}, so the background thread that drains the queue calls {@code aggregate}.
+ *   <li><b>Out of scope (v1):</b> calls that reach background-only code through intermediate
+ *       methods carrying neither marker. Flag direct calls only; widen this contract once a real
+ *       case shows that direct calls are not enough.
+ * </ul>
  */
 @Documented
-@Retention(RetentionPolicy.SOURCE)
+@PerfContract
+@Retention(RetentionPolicy.CLASS)
 @Target({ElementType.TYPE, ElementType.METHOD})
 public @interface BackgroundOnly {}

internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java

@@ -1,5 +1,7 @@
 package datadog.trace.api.function;
 
+import datadog.perfcontract.PerfContract;
+import datadog.perfcontract.SuppressPerfContract;
 import java.lang.annotation.Documented;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -12,10 +14,12 @@
  * customer-visible latency, as opposed to a background thread the tracer owns and paces itself (see
  * {@link BackgroundOnly}).
  *
- * <p>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 <b>not yet enforced</b>; hold to it by hand until the checker lands.
+ * <p>This marker changes no runtime behavior. It telegraphs the guarantee to readers and marks
+ * where foreground code starts for the rule in {@link BackgroundOnly}'s checker contract: no
+ * {@code @BackgroundOnly} code may be called from a foreground call site. AI review applies that
+ * rule through the perf-review skill's {@code checks.md}; no static checker enforces it yet (see
+ * {@code APMLP-1645}). A deliberate exception is marked with {@link SuppressPerfContract} citing
+ * {@link BackgroundOnly}.
  *
  * <p>The two markers are <b>not symmetric</b>. {@code @ForegroundSafe} is the strictly stronger
  * guarantee: code cheap enough for the foreground is automatically fine to call from a background
@@ -31,6 +35,7 @@
  * type-level one.
  */
 @Documented
-@Retention(RetentionPolicy.SOURCE)
+@PerfContract
+@Retention(RetentionPolicy.CLASS)
 @Target({ElementType.TYPE, ElementType.METHOD})
 public @interface ForegroundSafe {}