Never reclaim dictionary storage after a timed-out drain

[rkennke]

What does this PR do?:

Stops StringDictionary from freeing or resetting buffer storage while a guarded accessor may still be using it.

• New RefCountGuard::tryWaitForRefCountsToClear(targets, n). It waits only for guards on the given resources and returns false on timeout instead of logging and returning normally.
• StringDictionary::clearAll() now returns [[nodiscard]] bool:
  • It waits only for guards on its own three buffers, so traffic on other dictionaries can neither delay nor fail the reset.
  • On timeout it changes nothing. There is no buffer clear, _next_id reset, counter reset or generation bump, and it returns false. A partial reset would be worse than none: restarting _next_id without clearing would reissue ids existing entries still use. A dictionary that wasn't reset stays consistent.
• clearStandby() now waits for guards on its target buffer and skips the clear on timeout. The skipped buffer becomes active on the next rotate() with its old entries. That's harmless because ids are only reassigned by clearAll(), and the buffer is cleared the next time it is the clear target.
• Profiler::start() logs a warning for any dictionary it could not reset. It only signals a ContextValueCache reset when the context-value map was actually reset.
• Removes waitForAllRefCountsToClear(), which no longer has callers.
• Updates doc/architecture/StringDictionary.md. It still described the _accepting recheck as removed, which it isn't, and claimed the arena made a stale read safe, which it doesn't: extra arena chunks and overflow nodes are freed.

Motivation:

PROF-16136. clearAll() disabled new accesses, called waitForAllRefCountsToClear() and then cleared all buffers unconditionally. On timeout (~500 ms) the drain only logged a warning, so a lookup that had already passed the _accepting recheck could resume into freed overflow SBTable nodes or arena chunks.

Two things made this more likely than the ticket describes:

• The drain was global. It needed one scan in which every guard slot in the process was empty. Profiler::start() resets three dictionaries back to back, and JNI callers (recordTraceRoot, registerConstant0, lookupClass) keep taking guards on the other two while one drains. So it could time out from churn alone, with no stalled thread. lookupClass() does not take _class_map_lock, so the class map is exposed too.
• The rotation path had the same bug one cycle later. rotate()'s waitForRefCountToClear(old_active) also proceeds on timeout in release builds. clearStandby() then cleared that buffer two rotations later with no drain at all.

Additional Notes:

• CallTraceStorage follows the same "free after a drain" pattern (processTraces(), clearTableOnly(), destructor), but it is latent there, not reachable. Every production caller of processTraces()/clear() holds lockAll(), every put() runs inside a stripe lock, and lockAll() waits without a time limit, so its drain cannot time out. The destructor never runs because the Profiler singleton is never deleted. A stacked PR documents that contract and hardens the timeout path.
• rotate() still uses waitForRefCountToClear, which aborts on timeout in debug builds. That is now a diagnostic only, since clearStandby() handles a straggler safely.
• A reset whose drain times out costs about 0.5–1 s in the new tests, longer under ASan. There is no test-only timeout override, to keep production code unchanged for tests.

How to test the change?:

New gtests:

• StringDictionaryReclamationTest.ClearAllKeepsStorageWhileGuardHeld and ClearStandbyKeepsBufferWhileGuardHeld. A helper thread holds a real RefCountGuard and a pointer to a key in a non-first arena chunk (memory clear() frees), then re-reads it after the reset.
  • Without the fix: in debug, the generation, buffer-size and id assertions fail. Under ASan, heap-use-after-free from StringArena::reset() ← clearAll() / clearStandby().
  • With the fix: passes in debug, ASan and TSan.
• StringDictionaryReclamationTest.ClearAllIgnoresGuardsOnOtherDictionaries: a guard on another dictionary neither fails nor delays the reset.
• RefCountGuardTryWaitTest.* (4 tests) for the new drain primitive.
• Existing callers (liveness/reference-chain tests, stress tests, fuzzer) now assert clearAll()'s result.

Results:

• gtest: gtestDebug (full suite) passes. ASan and TSan pass for stringDictionary_ut and refCountGuard_ut, and compileFuzzer succeeds.
• testDebug (Java): 224 tests, 1 failure: VirtualThreadWallClockTest.samplesCarrierFramesFromBlockingVT[cstack=vmx] ("no carrier-thread frames", a continuation-unwind check unrelated to dictionaries). It passed when the class was rerun on its own.

For Datadog employees:

• If this PR touches code that signs or publishes builds or packages, or handles
  credentials of any kind, I've requested a security review (run the dd:platform-security-review
  skill, or file a request via the PSEC review form).
  bewaire also runs automatically on every PR.
• This PR doesn't touch any of that.
• JIRA: PROF-16136

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-06T13:43:23.027842Z	71c668a	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[datadog-official]

Start-time dictionary drain failures increment dictionary_drain_timeouts, but the following counter reset erases them before the recording can report them.

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit 71c668a · @DataDog review to ask questions}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 71c668aa20

ℹ️ 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".

[dd-octo-sts]

✅ All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 f9b0f288

[dd-octo-sts]

CI Test Results

Run: #37782741352 | Commit: ad32032 | Duration: 17m 47s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-amd64/debug
8	-	✅	-	-
8-ibm	-	✅	-	-
8-j9	✅	✅	-	-
8-librca	-	-	✅	✅
8-orcl	-	✅	-	-
11	-	✅	-	-
11-j9	✅	✅	-	-
11-librca	-	-	✅	✅
17	✅	✅	-	-
17-graal	✅	✅	-	-
17-j9	✅	✅	-	-
17-librca	-	-	✅	✅
21	✅	✅	-	-
21-graal	✅	✅	-	-
21-librca	-	-	✅	✅
25	✅	✅	-	-
25-graal	✅	✅	-	-
25-librca	-	-	✅	✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0

---

Updated: 2026-10-08 13:48:01 UTC

[rkennke]

@jbachorik thanks for the review. All five are addressed in e30f3d0:

1. DICTIONARY_KEYS after a skipped clearStandby() (comment): I left the behavior and fixed the comment instead. DICTIONARY_KEYS / KEYS_BYTES never tracked buffer contents. Only lookup() / bounded_lookup() count, when they assign a new id, and the copies rotate() makes via copyFrom() are not counted. So even in the normal case the gauge means "keys newly assigned since the last clearStandby()", and a skipped clear doesn't change that. A skipped clearAll() zeroes them exactly as a successful one does. The comment on clearStandby() now says this, instead of the misleading "track only post-clearStandby inserts".
2. _class_map_lock (comment): removed, both the guard and the member. Nothing else takes it.
3. Timeout reporting split three ways (comment): clearAll() and clearStandby() now only return whether they reset, without touching counters or logging. Profiler::reportDrainTimeout() is the one place that increments DICTIONARY_DRAIN_TIMEOUTS and logs. It's called from resetRecordingState() for start() and from rotateDictsAndRun() for the standby clears. resetRecordingState() covers the whole fresh-start sequence: dictionary reset, call-trace clear, Counters::reset(), gauge re-seed, then reporting. So there's no erase-then-reapply step and no failed_dictionary_resets plumbing anymore. The remaining increment inside RefCountGuard::waitForRefCountToClear() (used by rotate()) is shared with CallTraceStorage, so I left it there.
4. "No entry lost during rotate()" invariant (comment): the row now says the guarantee only holds when the drain completes. After a release-build timeout, a straggler's insert may miss the dump snapshot; it stays memory-safe because clearStandby() drains before clearing.
5. Doc intro still calling the race benign (comment): rewritten. The arena does not make a stale reader safe. The seq_cst _accepting recheck stops a missed caller before it touches buffer data, and a timed-out drain resets nothing.

Re the gtest-tsan-arm64 failure on 31ab198: it was CallTraceStorageTest.PutWithExistingIdNoInfiniteLoopWhenFull hitting its fixed 10 s deadline under TSan on a slow runner. That test is untouched by this PR, and the same job passed on 6320a58, which contains 31ab198 unchanged. The new push re-runs CI.

[jbachorik]

Thanks for cleaning this up. Looking good!