# This is a combination of 8 commits.

# This is the 1st commit message:

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

HubSwitchGuard manages thread-local hub state but was Send, allowing it
to be moved to another thread. When dropped on the wrong thread, it would
corrupt that thread's hub state instead of restoring the original thread.

To fix this, add PhantomData<MutexGuard<'static, ()>> to make the guard
!Send while keeping it Sync. This prevents the guard from being moved
across threads at compile time.

Additionally, refactor sentry-tracing to store guards in thread-local
storage keyed by span ID instead of in span extensions. This fixes a
related bug where multiple threads entering the same span would clobber
each other's guards.

Fixes #943
Refs RUST-130

Co-Authored-By: Claude <noreply@anthropic.com>

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));
+}