Skip to content

Commit 8d8bc49

Browse files
authored
fix(profiling): clear finished physical span links (#19529)
## Description This is the first PR in the span-attribution stack. It fixes stale endpoint and span attribution for physical thread stacks before the later PRs introduce logical asyncio and gevent targets. ### Problem The stack profiler samples threads independently of application execution, so it keeps a native per-thread copy of the span metadata needed to label profile samples. That copy must follow the lifetime of the tracing context. Otherwise, work that runs after a span finishes can still be labeled with the finished span ID, local-root span ID, and endpoint. Context activation events are necessary, but they are not sufficient to keep these copies accurate. Consider this sequence: 1. A worker thread activates a span and publishes a native span link. 2. Another thread finishes that span. 3. The worker continues running without performing another context activation. Finishing the span does not produce an activation event on the worker. Without a separate finish-time cleanup path, its copied link remains stale and subsequent samples are incorrectly attributed to the finished trace. A local root can also finish before one of its children. In that case the child is still valid and can continue producing profile samples, so cleanup for the root must not remove mappings whose current span is that unfinished child. ### Solution This PR makes the native `ThreadSpanLinks` store own both directions of each physical association under one mutex: - thread ID to copied span metadata; - current span ID to every thread carrying a copy of that exact span. It then uses two complementary lifecycle signals: - `ddtrace.context_provider.activate` handles immediate state replacement on the current thread. Activating a span or propagated `Context` publishes its mapping, restoring a parent replaces it, and activating `None` or a `Context` without a span ID clears it. - `trace.span_finish` performs global reverse invalidation for the exact span that finished. It removes every copied physical-thread mapping for that span, including mappings published on another thread that may never receive another activation. It deliberately preserves mappings for unfinished children when their local root finishes first. Updating the forward and reverse indexes atomically prevents the two views from diverging. Re-linking a thread removes its previous reverse-index entries first, so a later finish event for an older span cannot erase the thread's newer attribution. Listener registration occurs only after the collector completes fallible initialization, and both listeners are removed during shutdown. Fork handling reconstructs the native mutex and inherited indexes without traversing potentially inconsistent parent state. ### Review follow-up Native lifecycle bookkeeping now records in-flight physical span publications before releasing the GIL. If a span finishes while publication is waiting on the native mutex, cleanup is deferred until the final publication completes. This preserves the GIL release introduced in #14852 to avoid thread-pool deadlocks. ## Testing Added coverage for: - immediate physical-link clearing on context deactivation; - finished child-span cleanup across physical threads; - cross-thread finish cleanup before the linked worker performs another activation; - preservation of active child links when the local root finishes first; - reverse-index replacement when a thread advances to a new span; - nested child-to-parent restoration; - cleanup on reused thread-pool workers. Validation completed on the reordered physical-only PR: - Python 3.13 profiling suite in normal and fast-copy modes: `359 passed, 35 skipped, 2 xfailed` per mode - Python 3.13 focused cross-thread finish regression: `1 passed` in each mode - Python 3.11 profiling suite after hardening the reused-worker assertion: `309 passed, 85 skipped, 2 xfailed` in both normal and fast-copy modes - Reused-worker regression repeated 11 times in both modes: `22 passed` - Control experiment with the span-finish listener disabled: the cross-thread regression failed in both modes because post-finish samples retained the endpoint, confirming activation alone is insufficient - `scripts/lint checks` - targeted style and typing checks - `scripts/lint cformat` - `scripts/lint profiling-native-check` - rebased onto current `main` at `3efdcbe3af` - `git diff --check` ## Risks - Physical activation updates perform expected O(1) forward and reverse-index mutations under the existing span-link mutex. - Finished-span cleanup visits only threads indexed by the exact span that finished. - The reverse index adds bounded memory proportional to currently linked physical threads. - The rollback is to disable stack profiling or revert this PR. ## Additional Notes - Next, common logical stack helpers: #19420 - Gevent integration: #19379 - Asyncio integration: #19346 - Includes a customer-facing profiling fix release note. Before <img width="1732" height="905" alt="Screenshot 2026-08-06 at 10 15 34 AM" src="https://github.com/user-attachments/assets/d6a69ad3-1324-4328-b241-c3dc2fdf7cd5" /> After <img width="1749" height="908" alt="Screenshot 2026-08-06 at 10 15 24 AM" src="https://github.com/user-attachments/assets/cef79a9e-ddc9-4299-b3ff-492126e13ad2" /> Co-authored-by: taegyun.kim <taegyun.kim@datadoghq.com>
1 parent df0dc65 commit 8d8bc49

12 files changed

Lines changed: 533 additions & 42 deletions

File tree

ddtrace/internal/datadog/profiling/stack/__init__.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ def link_span(span: typing.Optional[typing.Union[context.Context, ddspan.Span]])
3535
elif isinstance(span, context.Context) and span.span_id is not None:
3636
local_root_span_id, span_type = context_meta.read_profiler_link(span)
3737
_stack.link_span(span.span_id, local_root_span_id, span_type)
38+
else:
39+
_stack.clear_span()
40+
41+
def _unlink_finished_span(span: ddspan.Span) -> None:
42+
"""Remove physical-thread attribution derived from a finished span."""
43+
_stack.unlink_finished_span(span.span_id)
3844

3945
def link_origin_task(task_id: int, task_name: str) -> None:
4046
"""

ddtrace/internal/datadog/profiling/stack/__init__.pyi

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ def resume_sampling() -> None: ...
8080

8181
# span <-> profile association
8282
def link_span(span: Optional[Union[context.Context, ddspan.Span]]) -> None: ...
83+
def _unlink_finished_span(span: ddspan.Span) -> None: ...
8384

8485
# Thread management
8586
def register_thread(python_thread_id: int, native_id: int, name: str) -> None: ...

ddtrace/internal/datadog/profiling/stack/_stack.pyi

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ def link_span(
4141
) -> None: ...
4242
def unlink_span(expected_span_id: int) -> None: ...
4343
def clear_span() -> None: ...
44+
def unlink_finished_span(span_id: int) -> None: ...
4445

4546
# executor worker thread <-> originating asyncio task association
4647
def link_origin_task(

ddtrace/internal/datadog/profiling/stack/include/thread_span_links.hpp

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
#pragma once
22

3-
#include <memory>
3+
#include <cstddef>
44
#include <mutex>
55
#include <optional>
66
#include <stdint.h>
77
#include <string>
88
#include <unordered_map>
9+
#include <unordered_set>
10+
#include <utility>
911

1012
namespace Datadog {
1113

@@ -47,13 +49,35 @@ class ThreadSpanLinks
4749
const std::optional<Span> get_active_span_from_thread_id(uint64_t thread_id);
4850
void unlink_span(uint64_t thread_id);
4951
void unlink_span(uint64_t thread_id, uint64_t expected_span_id);
52+
void unlink_finished_span(uint64_t span_id);
5053
void reset();
5154

55+
// These lifecycle methods run with the GIL held, before or after a native map mutation that releases it.
56+
void on_link_start(uint64_t span_id);
57+
bool on_link_end(uint64_t span_id);
58+
bool on_span_finish(uint64_t span_id);
59+
5260
static void postfork_child();
5361

5462
private:
63+
using ThreadIdSet = std::unordered_set<uint64_t>;
64+
using ThreadIdToSpanMap = std::unordered_map<uint64_t, Span>;
65+
using SpanToThreadMap = std::unordered_map<uint64_t, ThreadIdSet>;
66+
67+
struct PendingSpanLink
68+
{
69+
size_t count = 0;
70+
bool finished = false;
71+
};
72+
73+
void remove_thread_locked(uint64_t thread_id);
74+
5575
std::mutex mtx;
56-
std::unordered_map<uint64_t, std::unique_ptr<Span>> thread_id_to_span;
76+
ThreadIdToSpanMap thread_id_to_span;
77+
SpanToThreadMap span_to_threads;
78+
79+
// Protected by the GIL. This bridges lifecycle callback order to mutations that release it above.
80+
std::unordered_map<uint64_t, PendingSpanLink> pending_span_links;
5781

5882
// Private Constructor/Destructor
5983
ThreadSpanLinks() = default;

ddtrace/internal/datadog/profiling/stack/src/stack.cpp

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,10 +162,19 @@ stack_link_span_impl(PyObject* self, PyObject* args, PyObject* kwargs)
162162
span_type = empty_string.c_str();
163163
}
164164

165+
auto& links = ThreadSpanLinks::get_instance();
166+
links.on_link_start(span_id);
167+
165168
Py_BEGIN_ALLOW_THREADS;
166-
ThreadSpanLinks::get_instance().link_span(thread_id, span_id, local_root_span_id, std::string(span_type));
169+
links.link_span(thread_id, span_id, local_root_span_id, std::string(span_type));
167170
Py_END_ALLOW_THREADS;
168171

172+
if (links.on_link_end(span_id)) {
173+
Py_BEGIN_ALLOW_THREADS;
174+
links.unlink_finished_span(span_id);
175+
Py_END_ALLOW_THREADS;
176+
}
177+
169178
Py_RETURN_NONE;
170179
}
171180

@@ -214,6 +223,26 @@ stack_clear_span(PyObject* self, PyObject* args)
214223
Py_RETURN_NONE;
215224
}
216225

226+
static PyObject*
227+
stack_unlink_finished_span(PyObject* self, PyObject* args)
228+
{
229+
(void)self;
230+
uint64_t span_id;
231+
232+
if (!PyArg_ParseTuple(args, "K", &span_id)) {
233+
return nullptr;
234+
}
235+
236+
auto& links = ThreadSpanLinks::get_instance();
237+
if (links.on_span_finish(span_id)) {
238+
Py_BEGIN_ALLOW_THREADS;
239+
links.unlink_finished_span(span_id);
240+
Py_END_ALLOW_THREADS;
241+
}
242+
243+
Py_RETURN_NONE;
244+
}
245+
217246
// Records the asyncio task that offloaded work to the current (worker) thread.
218247
// The thread id is derived from the calling thread's state (this runs on the
219248
// worker thread), matching how stack_link_span_impl resolves it.
@@ -1010,6 +1039,10 @@ static PyMethodDef stack_methods[] = {
10101039
METH_VARARGS,
10111040
"Clear the span linked to the current thread if its ID matches the expected span ID" },
10121041
{ "clear_span", stack_clear_span, METH_NOARGS, "Clear the span linked to the current thread" },
1042+
{ "unlink_finished_span",
1043+
stack_unlink_finished_span,
1044+
METH_VARARGS,
1045+
"Clear every physical-thread link derived from a finished span" },
10131046
{ "link_origin_task",
10141047
reinterpret_cast<PyCFunction>(stack_link_origin_task),
10151048
METH_VARARGS | METH_KEYWORDS,

ddtrace/internal/datadog/profiling/stack/src/thread_span_links.cpp

Lines changed: 89 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4,42 +4,59 @@
44
#include <optional>
55
#include <stdint.h>
66
#include <string>
7+
#include <utility>
78

89
namespace Datadog {
10+
11+
void
12+
ThreadSpanLinks::remove_thread_locked(uint64_t thread_id)
13+
{
14+
auto thread_it = thread_id_to_span.find(thread_id);
15+
if (thread_it == thread_id_to_span.end()) {
16+
return;
17+
}
18+
19+
const auto& span = thread_it->second;
20+
auto span_it = span_to_threads.find(span.span_id);
21+
if (span_it != span_to_threads.end()) {
22+
span_it->second.erase(thread_id);
23+
if (span_it->second.empty()) {
24+
span_to_threads.erase(span_it);
25+
}
26+
}
27+
thread_id_to_span.erase(thread_it);
28+
}
29+
930
void
1031
ThreadSpanLinks::link_span(uint64_t thread_id, uint64_t span_id, uint64_t local_root_span_id, std::string span_type)
1132
{
1233
std::lock_guard<std::mutex> lock(mtx);
1334

14-
auto it = thread_id_to_span.find(thread_id);
15-
if (it == thread_id_to_span.end()) {
16-
thread_id_to_span[thread_id] = std::make_unique<Span>(span_id, local_root_span_id, std::move(span_type));
17-
} else {
18-
it->second->span_id = span_id;
19-
it->second->local_root_span_id = local_root_span_id;
20-
it->second->span_type = std::move(span_type);
21-
}
35+
remove_thread_locked(thread_id);
36+
Span span(span_id, local_root_span_id, std::move(span_type));
37+
thread_id_to_span.try_emplace(thread_id, std::move(span));
38+
// Index only the current span. A local root can finish before an active child, and finishing it must not remove the
39+
// child's attribution before that child finishes.
40+
span_to_threads[span_id].insert(thread_id);
2241
}
2342

2443
const std::optional<Span>
2544
ThreadSpanLinks::get_active_span_from_thread_id(uint64_t thread_id)
2645
{
2746
std::lock_guard<std::mutex> lock(mtx);
2847

29-
std::optional<Span> span;
3048
auto it = thread_id_to_span.find(thread_id);
31-
if (it != thread_id_to_span.end()) {
32-
span = *(it->second);
49+
if (it == thread_id_to_span.end()) {
50+
return std::nullopt;
3351
}
34-
return span;
52+
return it->second;
3553
}
3654

3755
void
3856
ThreadSpanLinks::unlink_span(uint64_t thread_id)
3957
{
4058
std::lock_guard<std::mutex> lock(mtx);
41-
42-
thread_id_to_span.erase(thread_id); // This is a no-op if the key is not found
59+
remove_thread_locked(thread_id);
4360
}
4461

4562
void
@@ -48,16 +65,64 @@ ThreadSpanLinks::unlink_span(uint64_t thread_id, uint64_t expected_span_id)
4865
std::lock_guard<std::mutex> lock(mtx);
4966

5067
auto it = thread_id_to_span.find(thread_id);
51-
if (it != thread_id_to_span.end() && it->second->span_id == expected_span_id) {
52-
thread_id_to_span.erase(it);
68+
if (it != thread_id_to_span.end() && it->second.span_id == expected_span_id) {
69+
remove_thread_locked(thread_id);
70+
}
71+
}
72+
73+
void
74+
ThreadSpanLinks::unlink_finished_span(uint64_t span_id)
75+
{
76+
std::lock_guard<std::mutex> lock(mtx);
77+
78+
auto span_it = span_to_threads.find(span_id);
79+
if (span_it == span_to_threads.end()) {
80+
return;
5381
}
82+
83+
for (const auto thread_id : span_it->second) {
84+
thread_id_to_span.erase(thread_id);
85+
}
86+
span_to_threads.erase(span_it);
5487
}
5588

5689
void
5790
ThreadSpanLinks::reset()
5891
{
5992
std::lock_guard<std::mutex> lock(mtx);
6093
thread_id_to_span.clear();
94+
span_to_threads.clear();
95+
}
96+
97+
void
98+
ThreadSpanLinks::on_link_start(uint64_t span_id)
99+
{
100+
++pending_span_links[span_id].count;
101+
}
102+
103+
bool
104+
ThreadSpanLinks::on_link_end(uint64_t span_id)
105+
{
106+
auto pending_span = pending_span_links.find(span_id);
107+
if (--pending_span->second.count != 0) {
108+
return false;
109+
}
110+
111+
const bool finished = pending_span->second.finished;
112+
pending_span_links.erase(pending_span);
113+
return finished;
114+
}
115+
116+
bool
117+
ThreadSpanLinks::on_span_finish(uint64_t span_id)
118+
{
119+
auto pending_span = pending_span_links.find(span_id);
120+
if (pending_span == pending_span_links.end()) {
121+
return true;
122+
}
123+
124+
pending_span->second.finished = true;
125+
return false;
61126
}
62127

63128
void
@@ -66,16 +131,14 @@ ThreadSpanLinks::postfork_child()
66131
auto& instance = get_instance();
67132
// NB placement-new to re-init and leak the mutex because doing anything else is UB
68133
new (&instance.mtx) std::mutex();
69-
// thread_id_to_span may be in a mid-mutation state if fork raced with
70-
// link_span/unlink_span. Its inherited pointers may be inconsistent,
71-
// so calling clear (or letting the destructor run) would traverse the same
72-
// corrupted linked-list state, which is UB. Instead, reconstruct the map in
73-
// place with placement-new without inspecting its contents. This intentionally
74-
// leaks the old map's heap allocations (nodes/buckets), but that memory belonged
75-
// to the parent's address space snapshot and is a bounded, one-time leak per
76-
// fork in the child; freeing it safely is impossible given the possible
77-
// corruption, so leaking is the correct trade-off.
78-
new (&instance.thread_id_to_span) std::unordered_map<uint64_t, std::unique_ptr<Span>>();
134+
// Either map may be in a mid-mutation state if fork raced with a link or unlink. Its inherited pointers may be
135+
// inconsistent, so calling clear (or letting the destructor run) would traverse corrupted linked-list state,
136+
// which is UB. Reconstruct the maps in place without inspecting their contents. This intentionally leaks the old
137+
// maps' heap allocations, because their possibly corrupted pointers cannot be safely traversed or freed in the
138+
// child.
139+
new (&instance.thread_id_to_span) std::unordered_map<uint64_t, Span>();
140+
new (&instance.span_to_threads) SpanToThreadMap();
141+
new (&instance.pending_span_links) std::unordered_map<uint64_t, PendingSpanLink>();
79142
}
80143

81144
} // namespace Datadog

ddtrace/internal/datadog/profiling/stack/test/test_thread_span_links.cpp

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,59 @@ TEST(ThreadSpanLinks, ClearFinished)
100100
}
101101
}
102102

103+
TEST(ThreadSpanLinks, UnlinkFinishedSpanAcrossThreads)
104+
{
105+
auto& links = Datadog::ThreadSpanLinks::get_instance();
106+
links.reset();
107+
108+
links.link_span(100, 1000, 1000, "web");
109+
links.link_span(101, 1001, 1000, "web");
110+
links.link_span(102, 1001, 1000, "web");
111+
links.link_span(103, 2001, 2000, "worker");
112+
113+
links.unlink_finished_span(1000);
114+
EXPECT_EQ(links.get_active_span_from_thread_id(100), std::nullopt);
115+
EXPECT_EQ(links.get_active_span_from_thread_id(101), Datadog::Span(1001, 1000, "web"));
116+
EXPECT_EQ(links.get_active_span_from_thread_id(102), Datadog::Span(1001, 1000, "web"));
117+
EXPECT_EQ(links.get_active_span_from_thread_id(103), Datadog::Span(2001, 2000, "worker"));
118+
119+
links.unlink_finished_span(1001);
120+
EXPECT_EQ(links.get_active_span_from_thread_id(101), std::nullopt);
121+
EXPECT_EQ(links.get_active_span_from_thread_id(102), std::nullopt);
122+
EXPECT_EQ(links.get_active_span_from_thread_id(103), Datadog::Span(2001, 2000, "worker"));
123+
}
124+
125+
TEST(ThreadSpanLinks, RelinkUpdatesFinishedSpanIndex)
126+
{
127+
auto& links = Datadog::ThreadSpanLinks::get_instance();
128+
links.reset();
129+
130+
links.link_span(201, 3001, 3000, "old");
131+
links.link_span(201, 4001, 4000, "new");
132+
133+
links.unlink_finished_span(3000);
134+
EXPECT_EQ(links.get_active_span_from_thread_id(201), Datadog::Span(4001, 4000, "new"));
135+
links.unlink_finished_span(4001);
136+
EXPECT_EQ(links.get_active_span_from_thread_id(201), std::nullopt);
137+
}
138+
139+
TEST(ThreadSpanLinks, FinishingWhileLinkingDefersCleanup)
140+
{
141+
auto& links = Datadog::ThreadSpanLinks::get_instance();
142+
constexpr uint64_t thread_id = 301;
143+
constexpr uint64_t span_id = 3001;
144+
links.reset();
145+
146+
links.on_link_start(span_id);
147+
EXPECT_FALSE(links.on_span_finish(span_id));
148+
149+
links.link_span(thread_id, span_id, span_id, "web");
150+
EXPECT_TRUE(links.on_link_end(span_id));
151+
152+
links.unlink_finished_span(span_id);
153+
EXPECT_EQ(links.get_active_span_from_thread_id(thread_id), std::nullopt);
154+
}
155+
103156
int
104157
main(int argc, char** argv)
105158
{

0 commit comments

Comments
 (0)