Add @ForegroundSafe/@BackgroundOnly declared-contract reachability check (J16) - #12474
Conversation
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).
…ndSafe / @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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…qh/foreground-background-only-check
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…tory 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 <noreply@anthropic.com>
…ride 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 <noreply@anthropic.com>
…16 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 <noreply@anthropic.com>
dougqh
added
tag: no release notes
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6310c5078
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * @param <K> key type | ||
| * @param <V> value type | ||
| */ | ||
| @ForegroundSafe |
There was a problem hiding this comment.
@ForegroundSafe to fully bounded methods
On a cache miss or eviction, computeIfAbsent calls the caller-supplied producer.apply through produceAndStoreValue, so its cost can be blocking or otherwise background-only. This type-level annotation also covers clear() and visit(), which traverse the entire backing array, with visit() additionally invoking an arbitrary consumer. For a slow producer or large cache, the declared “every method is foreground-safe” contract is therefore false and can cause reviewers or the planned checker to accept unbounded work on an application thread; scope the marker to operations whose complete transitive work is bounded or define callback-aware semantics instead.
Useful? React with 👍 / 👎.
| 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. |
There was a problem hiding this comment.
When an interface has both foreground-safe and background-only implementations in the reviewed files, this instruction requires a confident finding merely because a background-only implementation is “in hand,” even if the call-site variable is explicitly initialized with the safe implementation. The text itself admits that neither hop proves the receiver resolves to C, contradicting the later requirement that only a provable boundary crossing count and creating deterministic false-positive reviews. Track the receiver's creation or assignment to the call site and propagate the marker only when C can actually back that reference.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
…lse-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 <noreply@anthropic.com>
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
LLM ValidationLLM Validation Gate — dd-apm-sdk-review✅ PASS
AnalysisChanged instruction file(s): 1 case(s) dipped but stayed within the pass bar.
Results
Cases
Per-dimension scores, token usage, latency, and estimated cost are in the CI job logs. |
1 participant
Summary
datadog.trace.api.function.ForegroundSafe/.BackgroundOnlyannotations declaring a one-way foreground/background cost contract, applied toFixedSizeCache/EncodingCacheas initial markers.background-only-contract) to thedd-apm-sdk-reviewskill's performance lens: flags a foreground call path reaching@BackgroundOnlycode, including a declaration-site inheritance-widening violation and a bounded two-hop interface-propagation extension.docs/instrumentation_design_guidelines.md.CLASSretention so a future bytecode-level checker (APMLP-1645) can consume the markers..llm-validationfixtures covering the interface-hop and inheritance-widening cases (13 gate cases total, up from 11).Part of APMLP-1513 / APMLP-1645.
Test plan
./gradlew :internal-api:spotlessJavaCheckpasses.llm-validationgate run (13 cases) via the platform CI jobdd-apm-sdk-review8-lens re-review of this branch: APPROVE_WITH_COMMENTS, no P0s (module-placement P1 explicitly deferred — annotations stay ininternal-apifor now, pending a future move of the whole toolbox into its own component)🤖 Generated with Claude Code