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

## 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 `HubSwitchGuard`s. 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`](https://github.com/getsentry/sentry-rust/pull/957/changes#diff-203b34b6e9f8f4bad9c8c43e05c781e119eea7b27648f257bfe7b35e912ba2b1) 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`](https://github.com/getsentry/sentry-rust/pull/957/changes#diff-203b34b6e9f8f4bad9c8c43e05c781e119eea7b27648f257bfe7b35e912ba2b1) 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](https://github.com/getsentry/sentry-rust/pull/957/changes#diff-5acb70e20dc764b608e1acf81b57fea59308624b7c2bc87906b310ff8b1f0eb2L372).

Forking the hub ensures proper isolation, so the span gets set back at the right time (I also [suspect](https://github.com/getsentry/sentry-rust/pull/957/changes#r2773227449) 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`](https://github.com/getsentry/sentry-rust/pull/957/changes#diff-aa629e96442a0995ed2fd39dd68a18dd4be293732d2435b9aa5e03c848e12c38) ensures that entering the same span in multiple threads produces the correct span tree. Basically, it is a reproduction of #946. [`span_reentrancy.rs`](https://github.com/getsentry/sentry-rust/pull/957/changes#diff-203b34b6e9f8f4bad9c8c43e05c781e119eea7b27648f257bfe7b35e912ba2b1) also only passes with this change.

## Issues

- Fixes #943
- Fixes [RUST-130](https://linear.app/getsentry/issue/RUST-130/hubswitchguard-should-not-be-send)
- Fixes #946
- Fixes [RUST-132](https://linear.app/getsentry/issue/RUST-132/entering-the-same-span-several-times-causes-esoteric-behavior)

Author: szokeasaurusrex

CHANGELOG.md

@@ -7,6 +7,11 @@
 - Added a `Envelope::into_items` method, which returns an iterator over owned [`EnvelopeItem`s](https://docs.rs/sentry/0.46.2/sentry/protocol/enum.EnvelopeItem.html) in the [`Envelope`](https://docs.rs/sentry/0.46.2/sentry/struct.Envelope.html) ([#983](https://github.com/getsentry/sentry-rust/pull/983)).
 - Expose transport utilities ([#949](https://github.com/getsentry/sentry-rust/pull/949))
 
+### Fixes
+
+- Fixed thread corruption bug where `HubSwitchGuard` could be dropped on wrong thread ([#957](https://github.com/getsentry/sentry-rust/pull/957))
+  - **Breaking change**: `sentry_core::HubSwitchGuard` is now `!Send`, preventing it from being moved across threads. Code that previously sent the guard to another thread will no longer compile.
+
 ## 0.46.2
 
 ### New Features

sentry-core/src/hub_impl.rs

@@ -1,5 +1,6 @@
 use std::cell::{Cell, UnsafeCell};
-use std::sync::{Arc, LazyLock, PoisonError, RwLock};
+use std::marker::PhantomData;
+use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock};
 use std::thread;
 
 use crate::Scope;
@@ -19,10 +20,14 @@ thread_local! {
     );
 }
 
-/// A Hub switch guard used to temporarily swap
-/// active hub in thread local storage.
+/// A guard that temporarily swaps the active hub in thread-local storage.
+///
+/// This type is `!Send` because it manages thread-local state and must be
+/// dropped on the same thread where it was created.
 pub struct SwitchGuard {
     inner: Option<(Arc<Hub>, bool)>,
+    /// Makes this type `!Send` while keeping it `Sync`.
+    _not_send: PhantomData<MutexGuard<'static, ()>>,
 }
 
 impl SwitchGuard {
@@ -41,7 +46,10 @@ impl SwitchGuard {
             let was_process_hub = is_process_hub.replace(false);
             Some((hub, was_process_hub))
         });
-        SwitchGuard { inner }
+        SwitchGuard {
+            inner,
+            _not_send: PhantomData,
+        }
     }
 
     fn swap(&mut self) -> Option<Arc<Hub>> {

sentry-tracing/src/layer.rs sentry-tracing/src/layer/mod.rssentry-tracing/src/layer.rs renamed to sentry-tracing/src/layer/mod.rs

@@ -5,7 +5,7 @@ use std::sync::Arc;
 
 use bitflags::bitflags;
 use sentry_core::protocol::Value;
-use sentry_core::{Breadcrumb, TransactionOrSpan};
+use sentry_core::{Breadcrumb, Hub, HubSwitchGuard, TransactionOrSpan};
 use tracing_core::field::Visit;
 use tracing_core::{span, Event, Field, Level, Metadata, Subscriber};
 use tracing_subscriber::layer::{Context, Layer};
@@ -16,6 +16,9 @@ use crate::SENTRY_NAME_FIELD;
 use crate::SENTRY_OP_FIELD;
 use crate::SENTRY_TRACE_FIELD;
 use crate::TAGS_PREFIX;
+use span_guard_stack::SpanGuardStack;
+
+mod span_guard_stack;
 
 bitflags! {
     /// The action that Sentry should perform for a given [`Event`]
@@ -234,9 +237,7 @@ fn record_fields<'a, K: AsRef<str> + Into<Cow<'a, str>>>(
 /// the *current* span.
 pub(super) struct SentrySpanData {
     pub(super) sentry_span: TransactionOrSpan,
-    parent_sentry_span: Option<TransactionOrSpan>,
     hub: Arc<sentry_core::Hub>,
-    hub_switch_guard: Option<sentry_core::HubSwitchGuard>,
 }
 
 impl<S> Layer<S> for SentryLayer<S>
@@ -334,12 +335,7 @@ where
         set_default_attributes(&mut sentry_span, span.metadata());
 
         let mut extensions = span.extensions_mut();
-        extensions.insert(SentrySpanData {
-            sentry_span,
-            parent_sentry_span,
-            hub,
-            hub_switch_guard: None,
-        });
+        extensions.insert(SentrySpanData { sentry_span, hub });
     }
 
     /// Sets entered span as *current* sentry span. A tracing span can be
@@ -350,34 +346,47 @@ where
             None => return,
         };
 
-        let mut extensions = span.extensions_mut();
-        if let Some(data) = extensions.get_mut::<SentrySpanData>() {
-            data.hub_switch_guard = Some(sentry_core::HubSwitchGuard::new(data.hub.clone()));
-            data.hub.configure_scope(|scope| {
+        let extensions = span.extensions();
+        if let Some(data) = extensions.get::<SentrySpanData>() {
+            // We fork the hub (based on the hub associated with the span)
+            // upon entering the span. This prevents data leakage if the span
+            // is entered and exited multiple times.
+            //
+            // Further, Hubs are meant to manage thread-local state, even
+            // though they can be shared across threads. As the span may being
+            // entered on a different thread than where it was created, we need
+            // to use a new hub to avoid altering state on the original thread.
+            let hub = Arc::new(Hub::new_from_top(&data.hub));
+
+            hub.configure_scope(|scope| {
                 scope.set_span(Some(data.sentry_span.clone()));
-            })
-        }
-    }
+            });
 
-    /// Set exited span's parent as *current* sentry span.
-    fn on_exit(&self, id: &span::Id, ctx: Context<'_, S>) {
-        let span = match ctx.span(id) {
-            Some(span) => span,
-            None => return,
-        };
+            let guard = HubSwitchGuard::new(hub);
 
-        let mut extensions = span.extensions_mut();
-        if let Some(data) = extensions.get_mut::<SentrySpanData>() {
-            data.hub.configure_scope(|scope| {
-                scope.set_span(data.parent_sentry_span.clone());
+            SPAN_GUARDS.with(|guards| {
+                guards.borrow_mut().push(id.clone(), guard);
             });
-            data.hub_switch_guard.take();
         }
     }
 
+    /// Drop the current span's [`HubSwitchGuard`] to restore the parent [`Hub`].
+    fn on_exit(&self, id: &span::Id, _: Context<'_, S>) {
+        SPAN_GUARDS.with(|guards| guards.borrow_mut().pop(id.clone()));
+    }
+
     /// When a span gets closed, finish the underlying sentry span, and set back
     /// its parent as the *current* sentry span.
     fn on_close(&self, id: span::Id, ctx: Context<'_, S>) {
+        // Ensure all remaining Hub guards are dropped, to restore the original
+        // Hub.
+        //
+        // By this point, the span probably should be fully executed, but we should
+        // still ensure the guard is dropped in case this expectation is violated.
+        SPAN_GUARDS.with(|guards| {
+            guards.borrow_mut().remove(&id);
+        });
+
         let span = match ctx.span(&id) {
             Some(span) => span,
             None => return,
@@ -503,6 +512,9 @@ fn extract_span_data(
 
 thread_local! {
     static VISITOR_BUFFER: RefCell<String> = const { RefCell::new(String::new()) };
+    /// Hub switch guards keyed by span ID. Stored in thread-local so guards are
+    /// always dropped on the same thread where they were created.
+    static SPAN_GUARDS: RefCell<SpanGuardStack> = RefCell::new(SpanGuardStack::new());
 }
 
 /// Records all span fields into a `BTreeMap`, reusing a mutable `String` as buffer.

sentry-tracing/src/layer/span_guard_stack.rs

@@ -0,0 +1,118 @@
+//! This module contains code for stack-like storage for `HubSwitchGuard`s keyed
+//! by tracing span ID.
+
+use std::collections::hash_map::Entry;
+use std::collections::HashMap;
+
+use sentry_core::HubSwitchGuard;
+use tracing_core::span::Id as SpanId;
+
+/// Holds per-span stacks of `HubSwitchGuard`s to handle span re-entrancy.
+///
+/// Each time a span is entered, we should push a new guard onto the stack.
+/// When the span exits, we should pop the guard from the stack.
+pub(super) struct SpanGuardStack {
+    /// The map of span IDs to their respective guard stacks.
+    guards: HashMap<SpanId, Vec<HubSwitchGuard>>,
+}
+
+impl SpanGuardStack {
+    /// Creates an empty guard stack map.
+    pub(super) fn new() -> Self {
+        Self {
+            guards: HashMap::new(),
+        }
+    }
+
+    /// Pushes a guard for the given span ID, creating the stack if needed.
+    pub(super) fn push(&mut self, id: SpanId, guard: HubSwitchGuard) {
+        self.guards.entry(id).or_default().push(guard);
+    }
+
+    /// Pops the most recent guard for the span ID, removing the stack when empty.
+    pub(super) fn pop(&mut self, id: SpanId) -> Option<HubSwitchGuard> {
+        match self.guards.entry(id) {
+            Entry::Occupied(mut entry) => {
+                let stack = entry.get_mut();
+                let guard = stack.pop();
+                if stack.is_empty() {
+                    entry.remove();
+                }
+                guard
+            }
+            Entry::Vacant(_) => None,
+        }
+    }
+
+    /// Removes all guards for the span ID without returning them.
+    ///
+    /// This function guarantees that the guards are dropped in LIFO order.
+    /// That way, the hub which was active when the span was first entered
+    /// will be the one active after this function returns.
+    ///
+    /// Typically, remove should only get called once the span is fully
+    /// exited, so this removal order guarantee is mostly just defensive.
+    pub(super) fn remove(&mut self, id: &SpanId) {
+        self.guards
+            .remove(id)
+            .into_iter()
+            .flatten()
+            .rev() // <- we drop in reverse order
+            .for_each(drop);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::SpanGuardStack;
+    use sentry_core::{Hub, HubSwitchGuard};
+    use std::sync::Arc;
+    use tracing_core::span::Id as SpanId;
+
+    #[test]
+    fn pop_is_lifo() {
+        let initial = Hub::current();
+        let hub_a = Arc::new(Hub::new_from_top(initial.clone()));
+        let hub_b = Arc::new(Hub::new_from_top(hub_a.clone()));
+
+        let mut stack = SpanGuardStack::new();
+        let id = SpanId::from_u64(1);
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_a.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_b.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_b));
+
+        drop(stack.pop(id.clone()).expect("guard for hub_b"));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        drop(stack.pop(id.clone()).expect("guard for hub_a"));
+        assert!(Arc::ptr_eq(&Hub::current(), &initial));
+
+        assert!(stack.pop(id).is_none());
+    }
+
+    #[test]
+    fn remove_drops_all_guards_in_lifo_order() {
+        let initial = Hub::current();
+        let hub_a = Arc::new(Hub::new_from_top(initial.clone()));
+        let hub_b = Arc::new(Hub::new_from_top(hub_a.clone()));
+
+        assert!(!Arc::ptr_eq(&hub_b, &initial));
+        assert!(!Arc::ptr_eq(&hub_a, &initial));
+        assert!(!Arc::ptr_eq(&hub_a, &hub_b));
+
+        let mut stack = SpanGuardStack::new();
+        let id = SpanId::from_u64(2);
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_a.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_a));
+
+        stack.push(id.clone(), HubSwitchGuard::new(hub_b.clone()));
+        assert!(Arc::ptr_eq(&Hub::current(), &hub_b));
+
+        stack.remove(&id);
+        assert!(Arc::ptr_eq(&Hub::current(), &initial));
+    }
+}

sentry-tracing/tests/span_cross_thread.rs

@@ -0,0 +1,77 @@
+mod shared;
+
+use sentry::protocol::Context;
+use std::thread;
+use std::time::Duration;
+
+#[test]
+fn cross_thread_span_entries_share_transaction() {
+    let transport = shared::init_sentry(1.0);
+
+    let span = tracing::info_span!("foo");
+    let span2 = span.clone();
+
+    let handle1 = thread::spawn(move || {
+        let _guard = span.enter();
+        let _bar_span = tracing::info_span!("bar").entered();
+        thread::sleep(Duration::from_millis(100));
+    });
+
+    let handle2 = thread::spawn(move || {
+        thread::sleep(Duration::from_millis(10));
+        let _guard = span2.enter();
+        let _baz_span = tracing::info_span!("baz").entered();
+        thread::sleep(Duration::from_millis(50));
+    });
+
+    handle1.join().unwrap();
+    handle2.join().unwrap();
+
+    let data = transport.fetch_and_clear_envelopes();
+    let transactions: Vec<_> = data
+        .into_iter()
+        .flat_map(|envelope| {
+            envelope
+                .items()
+                .filter_map(|item| match item {
+                    sentry::protocol::EnvelopeItem::Transaction(transaction) => {
+                        Some(transaction.clone())
+                    }
+                    _ => None,
+                })
+                .collect::<Vec<_>>()
+        })
+        .collect();
+
+    assert_eq!(
+        transactions.len(),
+        1,
+        "expected a single transaction for cross-thread span entries"
+    );
+
+    let transaction = &transactions[0];
+    assert_eq!(transaction.name.as_deref(), Some("foo"));
+
+    let trace = match transaction
+        .contexts
+        .get("trace")
+        .expect("transaction should include trace context")
+    {
+        Context::Trace(trace) => trace,
+        unexpected => panic!("expected trace context but got {unexpected:?}"),
+    };
+
+    let bar_span = transaction
+        .spans
+        .iter()
+        .find(|span| span.description.as_deref() == Some("bar"))
+        .expect("expected span \"bar\" to be recorded in the transaction");
+    let baz_span = transaction
+        .spans
+        .iter()
+        .find(|span| span.description.as_deref() == Some("baz"))
+        .expect("expected span \"baz\" to be recorded in the transaction");
+
+    assert_eq!(bar_span.parent_span_id, Some(trace.span_id));
+    assert_eq!(baz_span.parent_span_id, Some(trace.span_id));
+}