Fix lookup-then-read race in GenerationalUtf8Cache.getUtf8 by dougqh · Pull Request #11917 · DataDog/dd-trace-java

dougqh · 2026-07-12T03:51:01Z

What This Does

Fixes a latent lookup-then-read data race in GenerationalUtf8Cache.getUtf8().

In current releases, this bug could not be triggered because the caches are only used in a single thread. However, the caches were intended to be thread-safe, so I'm making them thread-safe.

Motivation

Avoid a future bug by making Utf8Caches adhere to the intended contract.

From Claude...
The method finds a matching slot with lookupEntryIndex(), then reads that slot from the array in a separate step. Between the two, another thread can mutate the slot:

recalibrate() (purge) and eviction can null it → NPE on the subsequent hit().
promotion nulls the eden slot after promoting into tenured.
a slot can be reassigned to a different value's entry → getUtf8() returns that value's bytes for the requested string — silent payload corruption.

Additional Notes

From Claude...
CacheEntry identity is immutable (adjHash / value / valueUtf8 are final), so the read paths now re-validate the loaded reference against the request:

CacheEntry entry = entries[matchingIndex];
if (entry != null && entry.matches(adjHash, value)) { ... }

on both the tenured and eden paths. A null-or-mismatched slot falls through and is treated as a cache miss (re-encode). The residual races on hit()'s score/lastUsedMs writes are intentionally left as-is — they only nudge LRU/eviction bookkeeping and never affect the returned bytes.

Test

Adds concurrentAccess_neverThrowsOrReturnsWrongBytes — 8 reader threads hammering getUtf8() over a value set larger than capacity, with a concurrent recalibrate() loop, asserting every result equals the requested value's UTF-8. Verified it fails (NPE / wrong bytes) without the fix and passes with it.

🤖 Generated with Claude Code

Additional Changes

Made the intended contract explicit and gave it teeth:

Added a documentation-only @ThreadSafe annotation (datadog.trace.api.annotation) and applied it to both GenerationalUtf8Cache and SimpleUtf8Cache. (SimpleUtf8Cache was already correct — its lookup returns a validated entry reference instead of re-reading the slot by index.)
Split the UTF8 cache benchmarks into single-threaded (Utf8Benchmark) and multi-threaded (Utf8ConcurrentBenchmark, shared Utf8Workload). The single-threaded variant reflects how the caches are driven today and recalibrates inline; the concurrent variant uses JMH @Group to model the intended concurrent drive (a dedicated recalibrate thread + worker lookup threads) and doubles as a guardrail. A multi-threaded benchmark like this would have hit the NPE and surfaced the race sooner.

From Claude: measured cost of the matches() re-validation (single-thread, -f3, with vs without fix, simple cache as unchanged control): allocation flat (−0.03%) and throughput within run-to-run noise — the changed benchmark moved less than the untouched control. No measurable cost.

getUtf8() looks up a matching slot via lookupEntryIndex() and then reads that slot from the array in a second step. Between the two, another thread can mutate the slot: recalibrate()/eviction can null it, and promotion nulls the eden slot after promoting into tenured. The array read therefore returned either null (-> NPE on the following hit()) or, worse, a *different* value's entry whose bytes were then returned as if they were the requested value's -- silent payload corruption. This never manifests today because trace serialization runs on a single thread (TraceProcessingWorker), but the cache is built to allow concurrent access, so the race is a latent bug against that contract. CacheEntry identity is immutable (adjHash/value/valueUtf8 are final), so the fix re-validates the loaded reference against the request (entry != null && entry.matches(adjHash, value)) on both the tenured and eden read paths; a null-or-mismatched slot is treated as a miss. The residual races on hit()'s score/lastUsedMs writes are benign -- they only nudge LRU/eviction bookkeeping and never affect returned bytes. Adds a concurrent regression test that fails (NPE / wrong bytes) without the fix and passes with it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

datadog-prod-us1-5 · 2026-07-12T04:05:34Z

🎯 Code Coverage (details)
• Patch Coverage: 92.31%
• Overall Coverage: 58.74% (+1.69%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 8d60959 | Docs | Datadog PR Page | Give us feedback!}

dd-octo-sts · 2026-07-12T04:12:31Z

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.01 s	14.01 s	[-0.7%; +0.7%] (no difference)
startup:insecure-bank:tracing:Agent	12.91 s	12.94 s	[-1.3%; +0.8%] (no difference)
startup:petclinic:appsec:Agent	17.37 s	16.52 s	[+0.7%; +9.5%] (maybe worse)
startup:petclinic:iast:Agent	17.41 s	17.46 s	[-1.1%; +0.5%] (no difference)
startup:petclinic:profiling:Agent	17.29 s	17.28 s	[-1.2%; +1.2%] (no difference)
startup:petclinic:sca:Agent	17.49 s	16.92 s	[-1.2%; +7.8%] (no difference)
startup:petclinic:tracing:Agent	16.52 s	16.13 s	[-1.9%; +6.6%] (no difference)

Commit: 8d609595 · CI Pipeline · Benchmarking Platform UI

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

datadog-prod-us1-5

More details

PR successfully fixes a latent lookup-then-read race in GenerationalUtf8Cache that could cause NPE or silent payload corruption under concurrent access. The fix adds post-read validation against the original request using final fields, and the comprehensive concurrent test with 8 reader threads + recalibrate loop confirms it handles all three race scenarios (null slot, slot reassignment, promotion nulling). No issues found.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Datadog Autotest · Commit 2961bb5 · What is Autotest? · Any feedback? Reach out in #autotest}

amarziali · 2026-07-13T13:17:32Z

+@Documented
+@Retention(RetentionPolicy.CLASS)
+@Target(ElementType.TYPE)
+public @interface ThreadSafe {}


I think adding this should be brought to a larger audience. We already use @NotThreadSafe (parts of spotbug that can be used for static analysis) so that perhaps consider everything threadsafe and mark exceptions with that?

Good catch, that's definitely not what I intended either.
We certainly don't need our own annotation for ThreadSafe.
I'll make sure it gets fixed.

@group

Follow-on cleanup to the getUtf8 race fix: - Mark both `GenerationalUtf8Cache` and `SimpleUtf8Cache` `@ThreadSafe` (`javax.annotation.concurrent.ThreadSafe`, the annotation already used across the codebase), making the intended concurrency contract explicit. (`SimpleUtf8Cache` was already correct — its lookup returns a validated entry reference rather than re-reading the slot by index.) - Split the UTF8 cache benchmarks into a single-threaded `Utf8Benchmark` and a multi-threaded `Utf8ConcurrentBenchmark`, sharing `Utf8Workload`. The single-threaded variant reflects how the caches are driven today (serialization is single-threaded) and drives recalibrate inline; the concurrent variant uses @group to model the intended concurrent drive pattern (a dedicated recalibrate thread + worker lookup threads on the shared cache) and doubles as a concurrency guardrail. A @threads>1 benchmark like this would have hit the NPE and surfaced the race sooner. Measured cost of the matches() re-validation (single-thread, -f3, with vs without fix, `simple` as unchanged control): allocation flat (-0.03%) and throughput within run-to-run noise (changed benchmark moved less than the untouched control), i.e. no measurable cost. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

amarziali

Thanks for having fixed this concurrency issue. It sounds ok to me. I left some minor comment

amarziali · 2026-07-13T13:32:53Z

-        // mark promoted first - to avoid racy insertions
-        this.promotions += 1;
+          boolean evicted = lruInsert(this.tenuredEntries, MAX_TENURED_PROBES, edenEntry);
+          if (evicted) this.tenuredEvictions += 1;


nit: use { on the if

Not my preference, but I can if you wish.

I was pretty sure we had somewhere written to prefer using brackets on if but I cannot find anymore so perhaps I imagined it 🤣 do as you prefer!

amarziali · 2026-07-13T13:33:42Z


-        boolean evicted = lruInsert(this.tenuredEntries, MAX_TENURED_PROBES, edenEntry);
-        if (evicted) this.tenuredEvictions += 1;
+          edenEntries[matchingEdenIndex] = null;


this looks also fragile to me and lack of a cas that would solve the case if also another thread possibly replaces that slot with a different new entry?

The code is deliberately designed not to use CAS to avoid the overhead.
The safety of the code comes from the atomicity of the reference assignment and safety of final fields in Java.

But that does put the burden on the caller to double-check that the Entry matches the expectations -- which is admittedly, how the bug happened in the first place.

amarziali · 2026-07-13T13:34:48Z

 @SuppressFBWarnings(
    value = "IS2_INCONSISTENT_SYNC",
    justification =
        "stat updates are deliberately racy - sync is only used to prevent simultaneous bulk updates")


Is that concerning the issue we're fixing? If yes it can be removed (actually that class should have had a @NotThreadSafe but we seem having silenced this issue

No, not exactly. There are still some deliberately racy elements inside the cache, but not issues that jeopardize correctness for the callers.

dd-octo-sts · 2026-07-13T15:27:34Z

/merge

gh-worker-devflow-routing-ef8351 · 2026-07-13T15:27:38Z

View all feedbacks in Devflow UI.

2026-07-13 15:27:37 UTC ℹ️ Start processing command /merge

2026-07-13 15:27:42 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 2h (p90).

2026-07-13 17:28:26 UTC ❌ MergeQueue: The build pipeline has timeout

The merge request has been interrupted because the build 5400882391554179487 took longer than expected. The current limit for the base branch 'master' is 120 minutes.

dougqh added comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM type: bug fix Bug fix labels Jul 12, 2026

dougqh marked this pull request as ready for review July 13, 2026 13:02

dougqh requested a review from a team as a code owner July 13, 2026 13:02

dougqh requested a review from amarziali July 13, 2026 13:02

datadog-prod-us1-5 Bot reviewed Jul 13, 2026

View reviewed changes

amarziali reviewed Jul 13, 2026

View reviewed changes

dougqh force-pushed the dougqh/utf8-cache-concurrency-fix branch from 2961bb5 to 8d60959 Compare July 13, 2026 13:28

amarziali approved these changes Jul 13, 2026

View reviewed changes

dougqh added this pull request to the merge queue Jul 13, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 13, 2026

Uh oh!

Conversation

dougqh commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What This Does

Motivation

Additional Notes

Test

Additional Changes

Uh oh!

datadog-prod-us1-5 Bot commented Jul 12, 2026 • edited by datadog-datadog-prod-us1 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-sts Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

datadog-prod-us1-5 Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amarziali Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarziali left a comment

Choose a reason for hiding this comment

Uh oh!

amarziali Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

amarziali Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

amarziali Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarziali Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

dd-octo-sts Bot commented Jul 13, 2026

Uh oh!

gh-worker-devflow-routing-ef8351 Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dougqh commented Jul 12, 2026 •

edited

Loading

datadog-prod-us1-5 Bot commented Jul 12, 2026 •

edited by datadog-datadog-prod-us1 Bot

Loading

dd-octo-sts Bot commented Jul 12, 2026 •

edited

Loading

dougqh Jul 13, 2026 •

edited

Loading

dougqh Jul 13, 2026 •

edited

Loading

gh-worker-devflow-routing-ef8351 Bot commented Jul 13, 2026 •

edited

Loading