fix!(core): Make HubSwitchGuard !Send to prevent thread corruption

[szokeasaurusrex]

Description

This PR does three important things, which are all interdependent, so I think it makes sense to do it all in a single PR.

(1) Making HubSwitchGuard !Send

This PR makes HubSwitchGuard !Send by adding PhantomData<MutexGuard<'static, ()>> while keeping it Sync. The type system now prevents the guard from being moved across threads at compile time.

This change is important because HubSwitchGuard manages thread-local hub state, but was previously Send, allowing it to be moved to another thread. When dropped on the wrong thread, it could corrupt that thread's hub state instead of restoring the original thread. This change resolves #943.

Tests related to this change: !Send is enforced by the compiler, so there are no additional tests here.

(2) A new stack for spans to manage HubSwitchGuards

As HubSwitchGuard is now !Send, we needed a new way to manage the HubSwitchGuard associated with a given span, in the tracing integration. Previously, we just had the guards live directly on the span via the SentrySpanData type, but this no longer works, since spans need to be Send. So, we now declare a thread-local mapping from span IDs to HubSwitchGuards. The guards are stored in a stack, as each span will have one guard per span entry (spans can be entered multiple times, even on the same thread, and without the stack, we would restore the original hub too early). We drop the guards on span exit in LIFO order.

Tests related to this change: span_reentrancy.rs contains tests to validate correct span tree structure when re-entering the same span multiple times. A previous iteration of this PR only allowed a single guard per span; the test failed against this implementation because it produces an incorrect span structure (two transactions instead of one). The test only passes with the guard stack.

(3) Forking the Hub on each span (re-)entry

Change (2) is insufficient to make the span_reentrancy.rs test pass because with only that change, there is still the fundamental problem that each span does not get its own Hub to manage state. Thus, in that test, I believe we were actually only ever using one hub, because there was no place we were forking the hub. So, on span exit, we prematurely set the parent span on the hub.

Forking the hub ensures proper isolation, so the span gets set back at the right time (I also suspect we don't need to manually set it back, but I am unsure).

This change resolves #946.

Tests related to this change: span_cross_thread.rs ensures that entering the same span in multiple threads produces the correct span tree. Basically, it is a reproduction of #946. span_reentrancy.rs also only passes with this change.

Issues

• Fixes HubSwitchGuard should not be Send #943
• Fixes RUST-130
• Fixes Entering the same span several times causes esoteric behavior #946
• Fixes RUST-132

[linear]

RUST-130 HubSwitchGuard should not be Send

RUST-132 Entering the same span several times causes esoteric behavior

[lcian]

Looks good to me.

[szokeasaurusrex]

This current implementation is still flawed; if the same span is re-entered in the same thread (possible in async contexts), the second entry overwrites the original HubSwitchGuard, corrupting the behavior. I am working on a fix

[lcian]

@szokeasaurusrex it rewrites the original guard with the same one I think, so what's the issue there?

[lcian]

I've tested some scenarios manually and didn't find any issues. I assume you're also testing manully.
Would be nice to add these as automated tests as well.

[szokeasaurusrex]

@lcian Codex AI agent identified the potential issue here as I was working on #946. I have a test locally which reproduces the behavior. Will commit it with a more detailed explanation of the problem tomorrow 👍

I suppose it's possible that Codex is wrong and the local test is doing something which is not supposed to ever happen (the scenario is admittedly a bit contrived), but in any case, I think it's worth it to investigate properly. So, that is what I'm doing now. I will let you know if I need any assistance

[szokeasaurusrex]

I have made some pretty substantial changes here. Would appreciate a re-review.

Please also see the updated description 🙏

[szokeasaurusrex]

@lcian This reproduces the bug I was talking about. With the original implementation, span_a and span_b ended up as two separate transactions because dropping _reenter_a restored the original hub. There were two underlying issues here that were causing this behavior; I fixed both of them here.

The first fix was that I changed the SPAN_GUARDS so that it would not just have a single guard per span, but rather, to have a stack of guards that are pushed on each entry and popped on each exit. This way, we only restored the original hub on the final exit from the span.

That first fix was only part of the problem, though. To get the span structure correct here, I also needed to fix #946. For that, we needed to be forking the hub, as you suggested last week. Otherwise, if we just use the hub on the span directly, we end up not actually getting proper scope isolation.

[szokeasaurusrex]

Now that we are forking the hub, I actually don't think we need to be setting the parent span on the hub here; the hub we restore should already have the parent span set (unless some other code intentionally changed the span on that hub, which we probably should respect).

Suggested change

	data.hub.configure_scope(|scope| {
	scope.set_span(data.parent_sentry_span.clone());
	});

What do you think @lcian? Am I missing something?

[lcian]

Yeah, I agree. And with that we won't even need to store parent_sentry_span in the extensions anymore.

PR is substantially change, should be re-reviewed

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[szokeasaurusrex]

@lcian I did some vibe-benchmarks as we discussed (#1006). There does seem to be some added overhead from forking the hub, but in absolute terms, it is quite small (~0.15 μs per span entry), which I think is acceptable

[lcian]

Sounds good @szokeasaurusrex, thank you. Yep, that seems an acceptable overhead.

[sentry]

Bug: The on_close handler only cleans up span guards from the current thread's local storage. If a span is entered on one thread but dropped on another, the guard will leak on the original thread.
_{Severity: MEDIUM}

Suggested Fix

The mechanism for tracking guards should not rely solely on thread-local storage. Consider using a global, thread-safe collection, such as a Mutex-protected HashMap, to map a span ID to its active guards. This would allow on_close to find and drop all guards associated with a span, regardless of which thread it is executing on.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: sentry-tracing/src/layer/mod.rs#L374-L376

Potential issue: The `on_close` function is intended to clean up any remaining
`HubSwitchGuard`s for a span when it is dropped. However, it uses a thread-local storage
(`SPAN_GUARDS`) to perform this cleanup. If a span is entered on one thread (Thread A)
but the final reference to it is dropped on another (Thread B), `on_close` will execute
on Thread B. It will attempt to remove the guard from Thread B's local storage, where it
does not exist. This causes the guard to be leaked on Thread A, leaving the Sentry Hub
on that thread in an incorrect state indefinitely, which can lead to lost or
misattributed events.

_{Did we get this right? 👍 / 👎 to inform future reviews.}