Conversation
Root cause: otel_thread_ctx_v1 is a C __thread (per-native-thread) TLS
pointer. CRuby caches and recycles native threads, so a new Thread.new
may reuse a native thread previously occupied by a Thread that exited.
On Ruby < 3.3 the extension cleans up via RUBY_EVENT_THREAD_END, but that
event does not fire for threads terminated via Thread#kill, so a killed
thread that called `set` never has its TLS detached. The next Thread.new
that reuses that native thread inherits the stale context record, so
`clear` returns true ("a context record was attached") even though that
Ruby thread never attached one.
This is the flaky test `OTelThreadContext.clear ... returns false when
no context record was attached` (spec/datadog/tracing/otel_thread_context_spec.rb),
which failed on Ruby 2.7 in PR #6344 CI and passed on every other Ruby
version with the same seed: 2.7 and 3.0-3.2 share the
RUBY_EVENT_THREAD_END code path (vulnerable), while 3.3+ uses
RUBY_INTERNAL_THREAD_EVENT_EXITED, which fires for killed threads (not
vulnerable). It is also a real production defect: a customer thread
running traced code that is Thread#kill-ed leaks its OTel thread
context onto the next thread reusing the native thread, so the eBPF
profiler briefly associates the wrong trace context.
Fix: register an on_thread_begin hook on RUBY_EVENT_THREAD_BEGIN for
Ruby < 3.3 that calls ddog_otel_thread_ctx_detach(), so every new Ruby
thread starts from a clean TLS and discards any context left by a
previously killed thread. Ruby 3.3+ is unchanged.
Verified locally: the new regression test fails without the fix
(expected false, got true) and passes with it; spec:core_with_libdatadog_api
(182 examples) passes on Ruby 2.7 and 3.3.
Co-Authored-By: Claude <noreply@anthropic.com>
p-datadog
added
the
AI Generated
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation and regression coverage address the defect; remaining feedback is limited to changelog wording.
Review effort: Lite
Findings: None
What changed in this PR
Fixes stale OpenTelemetry thread context leakage after Thread#kill on Ruby versions below 3.3.
Changes:
- Adds thread-begin TLS cleanup.
- Adds regression coverage for killed-thread reuse.
- Adds a Tracing changelog fragment.
| File | Description |
|---|---|
unreleased/20260925094811.json |
Documents the tracing fix. |
spec/datadog/tracing/otel_thread_context_spec.rb |
Tests fresh-thread behavior after Thread#kill. |
ext/libdatadog_api/otel_thread_context.c |
Registers the thread-begin cleanup hook. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: cecb157a1f
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
💡 Codex Review Clear stale TLS before the thread-begin sampling window
On Ruby < 3.3, when a killed traced thread's native thread is recycled, this callback does not run until Correct the Ruby version scope in the changelog
The AGENTS.md reference: AGENTS.md:L169-L169 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2ba9533 | Docs | View more details | Give us feedback! |
The root-cause mechanism for the killed-thread context leak is documented on the on_thread_begin hook in ext/libdatadog_api/otel_thread_context.c. Restating it in the regression spec duplicated the rationale.
Both the Thread-exit memcheck spec and the fresh-thread regression spec attached a context in a thread and then killed it via identical setup. Extract kill_thread_holding_context so the setup lives in one place.
RUBY_INTERNAL_THREAD_EVENT_EXITED and rb_internal_thread_add_event_hook landed in Ruby 3.2.0, so the RUBY_EVENT_THREAD_END path that leaks a killed thread's context compiles on Ruby < 3.2, not < 3.3. Correct the changelog and the on_thread_begin comment accordingly.
The comment convention forbids the word never; state that a killed thread's context is left attached instead.
BenchmarksBenchmark execution time: 2026-10-04 04:28:48 Comparing candidate commit 2ba9533 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 0 unstable metrics.
|
The fresh-thread regression and the Thread-exit memcheck spec both rely on kill_thread_holding_context having attached a context record before the thread is killed. Read the record back inside the thread and assert it is present, so an unsupported build fails the precondition instead of running the body against a thread that attached nothing.
The it description and the assertion already state that a fresh thread reports no attached context. Rename the result binding to context_present_on_fresh_thread so the assertion reads on its own, and remove the comment that paraphrased it.
Cut the native-thread recycling walkthrough, the Ruby 3.2+ digression, and the clean-TLS claim. Keep the load-bearing hazard: RUBY_EVENT_THREAD_END does not fire for Thread#kill on Ruby < 3.2, and the action the hook takes.
The regression test kills a context-holding thread, then asserts a fresh thread's clear returns false. A single cycle exposes the leaked context only when CRuby recycles the killed thread's native thread, and that reuse is nondeterministic, so the assertion could hold against a no-op reintroduction of the bug. Drive 100 kill/spawn cycles so a fresh thread reliably recycles a killed context-holding thread's native thread and observes the stale context when the on_thread_begin detach is missing. Validated on Ruby 3.1: the loop fails against a no-op on_thread_begin, where nearly every cycle observes the stale context, and passes with the fix.
kill_thread_holding_context runs 100 times in the fresh-thread clear regression test; a bare signal_queue.pop or killed.join would hang the whole suite when a thread fails to signal or terminate. Wrap the pop in Timeout.timeout and assert the join returns within the bound so the helper fails fast.
y9v
left a comment
y9v
left a comment
There was a problem hiding this comment.
Thank you for investigating this! I was ready to dismiss it as a flaky spec
| # A fresh thread exposes the leak only by recycling the killed thread's | ||
| # native thread, which CRuby schedules nondeterministically; many | ||
| # kill/spawn cycles make the stale context reliably surface if the | ||
| # detach regresses. |
There was a problem hiding this comment.
I think this comment could be dropped?
There was a problem hiding this comment.
Naturally, I trimmed it to a single sentence keeping the one part the test name doesn't carry: CRuby schedules that recycling nondeterministically, which is why the test runs many kill/spawn cycles. Fixed in: 026c960c89.
| "type": "Fixed", | ||
| "product": "Tracing", | ||
| "pull_request": "https://github.com/DataDog/dd-trace-rb/pull/6384", | ||
| "message": "Fix the OpenTelemetry Thread Context leaking a killed thread's trace/span IDs onto a new thread reusing a recycled native thread after a traced thread was `Thread#kill`-ed on Ruby < 3.2." |
There was a problem hiding this comment.
as we didn't release this feature yet, I wouldn't add a changelog entry
| signal_queue.pop # ensure we set the thread context before we kill the thread | ||
| killed.kill | ||
| killed.join | ||
| kill_thread_holding_context |
There was a problem hiding this comment.
I wouldn't extract this function, since we only use it once, and it would be better to see what is happening in place
There was a problem hiding this comment.
I researched this and the helper has two call sites, the Thread-exit memcheck spec (line 205) and each cycle of the fresh-thread regression loop (line 230), so I've left it in place for now. Happy to inline both spots if having the steps visible in each test reads better.
| // RUBY_EVENT_THREAD_END does not fire for threads terminated via Thread#kill | ||
| // on Ruby < 3.2, so a killed thread's otel_thread_ctx_v1 TLS stays attached | ||
| // on a native thread CRuby may recycle for a later Ruby thread. Detach on | ||
| // thread begin to drop that stale record before the new thread runs. |
There was a problem hiding this comment.
I would move this comment to native_enable, where we are adding this hook?
There was a problem hiding this comment.
Of course, it moves to native_enable, directly above the on_thread_begin registration so the rationale sits with the wiring. Fixed in: 6cb3c768ff.
|
@p-datadog I can take over from here if you want |
Thread#value blocks until its thread terminates, so a fresh thread stuck inside clear would hang the 100-cycle regression test. Assert the fresh thread terminates within 5 seconds before reading its value, matching the bounded join the spec helper already uses.
Add a reference a reader can check for the on_thread_begin comment's claim that Ruby < 3.2 skips RUBY_EVENT_THREAD_END for threads terminated via Thread#kill: the v3_1_4 thread_do_start function fires the event only after the thread body returns, and Thread#kill unwinds the body via EC_JUMP_TAG(TAG_FATAL).
Address review comment: trim the comment above the fresh-thread regression test. The test name already states the recycling precondition. The loop count needs its own rationale: CRuby schedules native-thread recycling nondeterministically, so many kill/spawn cycles make the leak reliably observable. The comment now carries only that rationale. - Trimmed in spec/datadog/tracing/otel_thread_context_spec.rb:226 Co-Authored-By: Claude <noreply@anthropic.com>
Address review comment: drop the changelog fragment because the fixed defect is unreachable in released versions. DD_TRACE_OTEL_CTX_ENABLED and its Tracing wiring merged in #6297 after the 2.43.0 cut, so the feature first ships with the fix already in it and no released version exhibits the killed-thread context leak. - Removed unreleased/20260925094811.json Co-Authored-By: Claude <noreply@anthropic.com>
Address review comment: place the begin-hook rationale next to the wiring in native_enable, above the rb_add_event_hook registration, rather than above the callback definition. Wording unchanged. - Moved from ext/libdatadog_api/otel_thread_context.c:104 (original location) - Inserted above rb_add_event_hook(on_thread_begin, ...) at ext/libdatadog_api/otel_thread_context.c:179 Co-Authored-By: Claude <noreply@anthropic.com>
|
The deterministic reproducer in #6383 is what moved this past flake suspicion. Thank you for the review, and for the offer to take over: I'll push the remaining edits shortly, so the PR can get a final look after that. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
* u/fix-flaky-otel-thread-context: Link upstream source for OTel thread end event claim Bound fresh-thread wait in OTel clear regression test
What does this PR do?
Fixes the flaky test
spec/datadog/tracing/otel_thread_context_spec.rb:140(Datadog::Tracing::OTelThreadContext.clear ... returns false when no context record was attached), and the underlying production defect that caused it.Motivation:
Flaky test: Ruby 2.7 / build & test (standard) [0] on commit
5f7ee4dc0452, seed2235, in PR #6344. The same task with the same seed passed on Ruby 2.6/3.0/3.1/3.2/3.3/3.4/4.0.Failure:
Root cause:
otel_thread_ctx_v1is a C__thread(per-native-thread) TLS pointer set byddog_otel_thread_ctx_attachand cleared byddog_otel_thread_ctx_detach. CRuby caches and recycles native threads, so a newThread.newmay reuse a native thread previously occupied by aThreadthat exited. On Ruby < 3.2 the extension registerson_thread_endonRUBY_EVENT_THREAD_END, which fires only when a thread exits on its own;Thread#killterminates a thread through a path that bypasses this event. So a thread that calledset(attachingotel_thread_ctx_v1) and is thenThread#kill-ed keeps its TLS attached. When CRuby reuses that native thread for the nextThread.new, the new Ruby thread inherits the stale context record, soclearreturnstrue("a context record was attached") even though that record was attached by the killed thread.The
describe "#set"group's test "releases the thread context when a Thread exits" creates a thread that callssetand is thenThread#kill-ed, leaking its TLS; with a randomized seed the laterdescribe "#clear"test "returns false when no context record was attached" (wrapped in a freshThread.newby thearound(:each)) can reuse that native thread and inherit the stale TLS. This is also a real production defect: a customer thread running traced code that isThread#kill-ed leaks its OTel thread-context record onto the next thread reusing the native thread, so the eBPF profiler briefly associates the wrong trace context with that thread.Ruby 3.2+ uses
RUBY_INTERNAL_THREAD_EVENT_EXITED(on_thread_exited), which fires for killed threads too and detaches the TLS, so 3.2+ stays safe — which is why the flake surfaced on Ruby 2.7 (3.0–3.1 share theRUBY_EVENT_THREAD_ENDpath and are equally vulnerable, but happened to pass in that CI run).Fix: Register an
on_thread_beginhook onRUBY_EVENT_THREAD_BEGINfor Ruby < 3.2 (the#elsebranch that already usesRUBY_EVENT_THREAD_END) that callsddog_otel_thread_ctx_detach(). When a new Ruby thread begins on a recycled native thread, the hook drops the stale record before the thread runs any Ruby code, soset/clearon that thread observe only the context the thread attaches itself.RUBY_EVENT_THREAD_BEGINfires after the native thread has started executing, so an asynchronous reader such as the eBPF profiler can still observe the stale record in the brief window before the hook runs; the hook narrows that residual window. Ruby 3.2+ is unchanged.Related PRs:
Change log entry
A changelog fragment is added in this PR (Fixed / Tracing).
How to test the change?
A new regression test
returns false on fresh threads recycling killed context-holding threads' native threadskills a thread that attached a context and asserts a fresh thread'sclearreturnsfalse. Verified locally:false, gottrue) on Ruby 2.7 and 3.0; it passes on Ruby 3.3, which already detaches the TLS for killed threads.spec:core_with_libdatadog_api(182 examples) passes on Ruby 2.7 and 3.3 across seeds 2235, 1, 42, 999, 30721; 50/50 repeated full-task runs pass on Ruby 2.7 with seed 2235.