From b13bd97fadef842edbfd5bc20e94564893f69d43 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 3 Oct 2026 22:28:57 +0000 Subject: [PATCH 01/12] feat(core): Add thread registry that maps OS thread ids to the current hub Behind the thread-registry feature. Each thread registers a slot on first use of its hub, SwitchGuard keeps the slot pointing at the hub current on that thread, and the slot is removed when the thread exits. Hub::for_os_thread looks a hub up by the id current_os_thread_id returns, which is the id a crash handler sees for the crashing thread. --- sentry-core/Cargo.toml | 4 + sentry-core/src/hub_impl.rs | 35 +++++- sentry-core/src/lib.rs | 4 + sentry-core/src/thread_registry.rs | 166 +++++++++++++++++++++++++++++ 4 files changed, 204 insertions(+), 5 deletions(-) create mode 100644 sentry-core/src/thread_registry.rs diff --git a/sentry-core/Cargo.toml b/sentry-core/Cargo.toml index c9f08c49..4dc5720f 100644 --- a/sentry-core/Cargo.toml +++ b/sentry-core/Cargo.toml @@ -27,6 +27,7 @@ default = [] client = ["rand"] test = ["client", "release-health"] release-health = [] +thread-registry = ["client", "dep:libc"] logs = [] metrics = [] @@ -39,6 +40,9 @@ serde_json = { workspace = true } url = { workspace = true } uuid = { workspace = true, features = ["v4", "serde"], optional = true } +[target.'cfg(unix)'.dependencies] +libc = { workspace = true, optional = true } + [dev-dependencies] # Because we re-export all the public API in `sentry`, we actually run all the # doctests using the `sentry` crate. This also takes care of the doctest diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index e08b009e..4a36772b 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -19,11 +19,16 @@ thread_local! { /// [`PROCESS_HUB`] are identical, i.e. `Arc::ptr_eq(&PROCESS_HUB, &THREAD_HUB)` is true. /// On any other thread, the [`THREAD_HUB`] is created as a new hub based off of the /// [`PROCESS_HUB`]. - static THREAD_HUB: RefCell> = if thread::current().id() == PROCESS_HUB.thread { - PROCESS_HUB.hub.clone() - } else { - Hub::new_from_top(&PROCESS_HUB.hub).into() - }.into() + static THREAD_HUB: RefCell> = { + let hub: Arc = if thread::current().id() == PROCESS_HUB.thread { + PROCESS_HUB.hub.clone() + } else { + Hub::new_from_top(&PROCESS_HUB.hub).into() + }; + #[cfg(feature = "thread-registry")] + crate::thread_registry::set_current_hub(&hub); + hub.into() + } } /// A guard that temporarily swaps the active hub in thread-local storage. @@ -61,6 +66,8 @@ impl SwitchGuard { return None; } std::mem::swap(&mut *thread_hub, &mut hub); + #[cfg(feature = "thread-registry")] + crate::thread_registry::set_current_hub(&thread_hub); Some(hub) }); SwitchGuard { @@ -80,6 +87,8 @@ impl SwitchGuard { .try_with(|thread_hub| { let mut thread_hub = thread_hub.borrow_mut(); std::mem::swap(&mut *thread_hub, &mut hub); + #[cfg(feature = "thread-registry")] + crate::thread_registry::set_current_hub(&thread_hub); hub }) .ok() @@ -164,6 +173,22 @@ impl Hub { PROCESS_HUB.hub.clone() } + /// Returns the hub that is current on the thread with the given + /// operating system thread id. + /// + /// The id is the one [`current_os_thread_id`](crate::current_os_thread_id) + /// returns on that thread. A thread is known once it has used + /// [`Hub::current`] or [`Hub::run`]; hubs switched with [`Hub::run`] + /// are tracked. Returns `None` for threads that never touched a hub + /// or have exited. + /// + /// This is meant for crash reporters that learn the crashing thread + /// from the OS and want its scope. + #[cfg(feature = "thread-registry")] + pub fn for_os_thread(os_thread_id: u64) -> Option> { + crate::thread_registry::hub_for_os_thread(os_thread_id) + } + /// Invokes the callback with the default hub. #[deprecated = "Use `Hub::current` instead; this function offers no performance benefit."] pub fn with(f: F) -> R diff --git a/sentry-core/src/lib.rs b/sentry-core/src/lib.rs index b04e00fe..ff80060f 100644 --- a/sentry-core/src/lib.rs +++ b/sentry-core/src/lib.rs @@ -146,6 +146,10 @@ mod client; mod hub_impl; #[cfg(feature = "client")] mod session; +#[cfg(feature = "thread-registry")] +mod thread_registry; +#[cfg(feature = "thread-registry")] +pub use crate::thread_registry::current_os_thread_id; #[cfg(feature = "client")] pub use crate::clientoptions::MaxRequestBodySize; diff --git a/sentry-core/src/thread_registry.rs b/sentry-core/src/thread_registry.rs new file mode 100644 index 00000000..5a734246 --- /dev/null +++ b/sentry-core/src/thread_registry.rs @@ -0,0 +1,166 @@ +//! Maps operating system thread ids to the hub that is current on each +//! thread. +//! +//! Out-of-process crash reporters learn the crashing thread from the OS, +//! not from Rust. This registry lets another thread in the same process +//! find the [`Hub`] that thread was using, so its scope can be reported +//! with the crash. + +use std::collections::HashMap; +use std::sync::{Arc, LazyLock, Mutex, PoisonError, RwLock, Weak}; + +use crate::Hub; + +/// Returns the id the operating system uses for the current thread. +/// +/// This is the id a crash handler sees for the crashing thread: +/// +/// - Linux and Android: the kernel thread id from `gettid`. +/// - Windows: `GetCurrentThreadId`. +/// - macOS: the Mach port name of the thread, as reported in exception +/// messages and returned by `pthread_mach_thread_np`. +/// - Other unix targets: `pthread_self`. +/// +/// It is not related to [`std::thread::ThreadId`]. +pub fn current_os_thread_id() -> u64 { + #[cfg(any(target_os = "linux", target_os = "android"))] + { + // SAFETY: `gettid` takes no arguments and cannot fail. + unsafe { libc::syscall(libc::SYS_gettid) as u64 } + } + #[cfg(target_os = "macos")] + { + // SAFETY: `pthread_self` is always valid for the calling thread. + unsafe { libc::pthread_mach_thread_np(libc::pthread_self()) as u64 } + } + #[cfg(windows)] + { + extern "system" { + fn GetCurrentThreadId() -> u32; + } + // SAFETY: takes no arguments and cannot fail. + unsafe { GetCurrentThreadId() as u64 } + } + #[cfg(not(any( + target_os = "linux", + target_os = "android", + target_os = "macos", + windows + )))] + { + // SAFETY: `pthread_self` is always valid for the calling thread. + unsafe { libc::pthread_self() as u64 } + } +} + +/// One thread's entry in the registry. +struct Slot { + os_thread_id: u64, + hub: RwLock>, +} + +static REGISTRY: LazyLock>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// Owns a thread's slot and removes it when the thread exits. +struct SlotGuard(Arc); + +impl SlotGuard { + fn register() -> Self { + let slot = Arc::new(Slot { + os_thread_id: current_os_thread_id(), + hub: RwLock::new(Weak::new()), + }); + REGISTRY + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(slot.os_thread_id, slot.clone()); + SlotGuard(slot) + } +} + +impl Drop for SlotGuard { + fn drop(&mut self) { + let mut registry = REGISTRY.lock().unwrap_or_else(PoisonError::into_inner); + // The OS can reuse the id for a new thread before this one is + // fully gone, so only remove the entry if it is still ours. + if registry + .get(&self.0.os_thread_id) + .is_some_and(|slot| Arc::ptr_eq(slot, &self.0)) + { + registry.remove(&self.0.os_thread_id); + } + } +} + +thread_local! { + static SLOT: SlotGuard = SlotGuard::register(); +} + +/// Records `hub` as the hub current on the calling thread. +/// +/// Does nothing while the thread is shutting down and its slot is gone. +pub(crate) fn set_current_hub(hub: &Arc) { + let _ = SLOT.try_with(|slot| { + *slot.0.hub.write().unwrap_or_else(PoisonError::into_inner) = Arc::downgrade(hub); + }); +} + +/// Returns the hub current on the thread with the given OS id. +pub(crate) fn hub_for_os_thread(os_thread_id: u64) -> Option> { + let slot = REGISTRY + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(&os_thread_id) + .cloned()?; + let hub = slot.hub.read().unwrap_or_else(PoisonError::into_inner); + hub.upgrade() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::mpsc; + + #[test] + fn registers_current_hub_per_thread() { + let (id_tx, id_rx) = mpsc::channel(); + let (done_tx, done_rx) = mpsc::channel::<()>(); + + let worker = std::thread::spawn(move || { + let hub = Hub::current(); + id_tx.send((current_os_thread_id(), hub)).unwrap(); + // Keep the thread alive until the main thread has looked it up. + done_rx.recv().ok(); + }); + + let (id, hub) = id_rx.recv().unwrap(); + let found = Hub::for_os_thread(id).expect("worker thread is registered"); + assert!(Arc::ptr_eq(&found, &hub)); + assert!(!Arc::ptr_eq(&found, &Hub::main())); + + done_tx.send(()).unwrap(); + worker.join().unwrap(); + assert!(Hub::for_os_thread(id).is_none(), "slot is removed on exit"); + } + + #[test] + fn follows_hub_run_switching() { + let id = current_os_thread_id(); + let outer = Hub::current(); + let inner = Arc::new(Hub::new_from_top(&outer)); + + Hub::run(inner.clone(), || { + let found = Hub::for_os_thread(id).unwrap(); + assert!(Arc::ptr_eq(&found, &inner)); + }); + + let found = Hub::for_os_thread(id).unwrap(); + assert!(Arc::ptr_eq(&found, &outer)); + } + + #[test] + fn unknown_thread_has_no_hub() { + assert!(Hub::for_os_thread(u64::MAX).is_none()); + } +} From e9cf4848ba0d957cbaedfcd24ad48065dd1400df Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 3 Oct 2026 22:28:57 +0000 Subject: [PATCH 02/12] feat(minidump): Report the scope of the thread that crashed Nothing is sent to the crash reporter until the crash. The on_crash hook in minidumper-child names the crashing OS thread; a parked helper thread serializes the scope of the hub current on that thread (inline on macOS, where other threads are suspended) and the handler sends it before the dump request. The reporter merges it with the fatal event. This replaces the manual set_user/set_tag/set_extra/add_breadcrumb mirror, which cost one message per scope write. Depends on the on_crash hook from the feat/on-crash-hook branch of minidumper-child via a crates-io patch until it is released. --- CHANGELOG.md | 7 + Cargo.lock | 5 +- Cargo.toml | 4 + sentry-minidump/Cargo.toml | 3 +- sentry-minidump/README.md | 22 ++- sentry-minidump/examples/app.rs | 41 ++++- sentry-minidump/src/lib.rs | 131 +++++++--------- sentry-minidump/src/scope_sync.rs | 244 ++++++++++++++++++++++++++++++ sentry-minidump/tests/e2e.rs | 20 ++- 9 files changed, 384 insertions(+), 93 deletions(-) create mode 100644 sentry-minidump/src/scope_sync.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8094ca51..09cb823c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## Unreleased + +### Features + +- The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The manual `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods on `MinidumpIntegration` are removed, and `scope_timeout` bounds the wait for the scope. +- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread. + ## 0.49.3 ### Features diff --git a/Cargo.lock b/Cargo.lock index 609acf0c..80297ea2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2351,8 +2351,7 @@ dependencies = [ [[package]] name = "minidumper-child" version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1dd1d5c43819abcc73ced5a1287c58fdc715c9f993b7cc9debc6ae7cb8421994" +source = "git+https://github.com/timfish/minidumper-child?branch=feat%2Fon-crash-hook#ebba9e44e458141c10afd33a3bc07267f1eeca15" dependencies = [ "crash-handler", "minidumper", @@ -3673,6 +3672,7 @@ dependencies = [ "anyhow", "criterion", "futures", + "libc", "log", "rand 0.10.2", "rayon", @@ -3713,7 +3713,6 @@ dependencies = [ "sadness-generator", "sentry", "sentry-core", - "serde", "serde_json", ] diff --git a/Cargo.toml b/Cargo.toml index 40679ba4..d22b8dd2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -106,3 +106,7 @@ uname = "0.1.1" ureq = { version = "3.0.11", default-features = false } url = "2.2.2" uuid = "1.0.0" + +[patch.crates-io] +# Pending release of the `on_crash` hook. +minidumper-child = { git = "https://github.com/timfish/minidumper-child", branch = "feat/on-crash-hook" } diff --git a/sentry-minidump/Cargo.toml b/sentry-minidump/Cargo.toml index 7b61eee0..6487611c 100644 --- a/sentry-minidump/Cargo.toml +++ b/sentry-minidump/Cargo.toml @@ -20,9 +20,8 @@ name = "minidump" path = "examples/app.rs" [dependencies] -sentry-core = { workspace = true, features = ["client"] } +sentry-core = { workspace = true, features = ["client", "thread-registry"] } minidumper-child = { workspace = true } -serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } [dev-dependencies] diff --git a/sentry-minidump/README.md b/sentry-minidump/README.md index 49467ee7..29673233 100644 --- a/sentry-minidump/README.md +++ b/sentry-minidump/README.md @@ -39,16 +39,22 @@ crash reporter for the whole process; there is no per-client isolation. Only the first initialization that has a DSN starts the reporter; later calls do nothing. -## Scope sync +## Scope -Scope changes do not cross the process boundary on their own. Send them -to the crash reporter through the integration: +The crash event carries the scope of the thread that crashed: user, +tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, +after the scope's event processors have run. Nothing is sent to the +crash reporter until the crash. At that moment the crash handler names +the crashing OS thread, a helper thread serializes the scope of the hub +current on that thread, and the handler sends it to the reporter before +it requests the minidump. Hubs bound with `Hub::run` are tracked, so a +server with one hub per request reports the scope of the request that +crashed. A thread that never used Sentry falls back to the main hub. -```rust -sentry::with_integration(|minidump: &sentry_minidump::MinidumpIntegration, _| { - minidump.set_user(Some(user.clone())); -}); -``` +The helper waits at most `scope_timeout` for the scope. If the crash +happened while the crashing thread held the allocator or the hub lock, +the helper cannot finish and the event goes out without the scope. +Attachments on the scope are not carried. ## Platforms diff --git a/sentry-minidump/examples/app.rs b/sentry-minidump/examples/app.rs index 125d4437..b4ddac52 100644 --- a/sentry-minidump/examples/app.rs +++ b/sentry-minidump/examples/app.rs @@ -2,8 +2,11 @@ //! //! This is also executed by the end to end test. +use std::sync::Arc; use std::time::Duration; +use sentry::Hub; + fn main() { let minidump = sentry_minidump::MinidumpIntegration::new() .crashes_dir(std::env::temp_dir().join("sentry-minidump-example")) @@ -28,13 +31,41 @@ fn main() { ); // Everything after here runs in the app process only. - sentry::with_integration(|minidump: &sentry_minidump::MinidumpIntegration, _| { - minidump.set_user(Some(sentry::User { - username: Some("john_doe".into()), - email: Some("john@doe.town".into()), + sentry::configure_scope(|scope| { + scope.set_user(Some(sentry::User { + username: Some("main_thread".into()), ..Default::default() })); + scope.set_tag("shared", "from_main"); + }); + + // The worker hub copies the main scope now. Changes the main thread + // makes after this point do not reach it. + let worker_hub = Arc::new(Hub::new_from_top(Hub::current())); + + sentry::configure_scope(|scope| { + scope.set_tag("main_only", "true"); }); - unsafe { sadness_generator::raise_segfault() }; + std::thread::spawn(move || { + Hub::run(worker_hub, || { + sentry::configure_scope(|scope| { + scope.set_user(Some(sentry::User { + username: Some("john_doe".into()), + email: Some("john@doe.town".into()), + ..Default::default() + })); + scope.set_tag("thread", "worker"); + }); + sentry::add_breadcrumb(sentry::Breadcrumb { + message: Some("about to crash".into()), + ..Default::default() + }); + + // The crash event carries this hub's scope, not the main thread's. + unsafe { sadness_generator::raise_segfault() }; + }) + }) + .join() + .ok(); } diff --git a/sentry-minidump/src/lib.rs b/sentry-minidump/src/lib.rs index 7e4c3bbb..ad5e1747 100644 --- a/sentry-minidump/src/lib.rs +++ b/sentry-minidump/src/lib.rs @@ -31,17 +31,22 @@ //! Only the first initialization that has a DSN starts the reporter; later //! calls do nothing. //! -//! # Scope sync +//! # Scope //! -//! Scope changes do not cross the process boundary on their own. Send them -//! to the crash reporter through the integration: +//! The crash event carries the scope of the thread that crashed: user, +//! tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, +//! after the scope's event processors have run. Nothing is sent to the +//! crash reporter until the crash. At that moment the crash handler names +//! the crashing OS thread, a helper thread serializes the scope of the hub +//! current on that thread, and the handler sends it to the reporter before +//! it requests the minidump. Hubs bound with [`Hub::run`] are tracked, so a +//! server with one hub per request reports the scope of the request that +//! crashed. A thread that never used Sentry falls back to the main hub. //! -//! ```rust -//! # let user = sentry::User::default(); -//! sentry::with_integration(|minidump: &sentry_minidump::MinidumpIntegration, _| { -//! minidump.set_user(Some(user.clone())); -//! }); -//! ``` +//! The helper waits at most [`scope_timeout`](MinidumpIntegration::scope_timeout) +//! for the scope. If the crash happened while the crashing thread held the +//! allocator or the hub lock, the helper cannot finish and the event goes +//! out without the scope. Attachments on the scope are not carried. //! //! # Platforms //! @@ -51,6 +56,8 @@ #![doc(html_logo_url = "https://sentry-brand.storage.googleapis.com/sentry-glyph-black.png")] #![deny(unsafe_code)] +mod scope_sync; + use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::{exit, Command}; @@ -58,9 +65,11 @@ use std::sync::{Arc, Once, OnceLock}; use std::time::Duration; use minidumper_child::{ClientHandle, MinidumperChild}; -use sentry_core::protocol::{Attachment, AttachmentType, Breadcrumb, Event, User, Value}; +use sentry_core::protocol::{Attachment, AttachmentType, Value}; use sentry_core::{sentry_debug, Client, ClientOptions, Hub, Integration, Level, Scope}; +use crate::scope_sync::{ScopeReceiver, ScopeSync}; + /// The default environment variable that marks the crash reporter process. const DEFAULT_SERVER_ENV_VAR: &str = "_SENTRY_CRASH_REPORTER_SERVER"; @@ -70,18 +79,12 @@ const DEFAULT_FLUSH_TIMEOUT: Duration = Duration::from_secs(5); /// The default `argv[0]` of the crash reporter process, shown by `ps`. const DEFAULT_PROCESS_NAME: &str = "Crash Reporter (Sentry Rust SDK)"; +/// The default time the crash handler waits for the scope to be serialized. +const DEFAULT_SCOPE_TIMEOUT: Duration = Duration::from_secs(2); + type OnProcess = dyn Fn(&mut Command) + Send + Sync + 'static; type BeforeCapture = dyn Fn(&mut Scope, &Path) + Send + Sync + 'static; -/// An update to the crash reporter's scope, sent over the socket. -#[derive(serde::Deserialize, serde::Serialize, Debug, Clone, PartialEq)] -enum ScopeUpdate { - AddBreadcrumb(Breadcrumb), - SetUser(Option), - SetExtra(String, Option), - SetTag(String, Option), -} - /// Captures native crashes as minidumps and sends them to Sentry. /// /// Build it with [`MinidumpIntegration::new`] and the builder methods, then @@ -100,6 +103,7 @@ pub struct MinidumpIntegration { on_process: Option>, before_capture: Option>, flush_timeout: Duration, + scope_timeout: Duration, client_connect_timeout: Option, server_stale_timeout: Option, handle: OnceLock, @@ -113,6 +117,7 @@ impl std::fmt::Debug for MinidumpIntegration { .field("inherit_args", &self.inherit_args) .field("process_name", &self.process_name) .field("flush_timeout", &self.flush_timeout) + .field("scope_timeout", &self.scope_timeout) .field("client_connect_timeout", &self.client_connect_timeout) .field("server_stale_timeout", &self.server_stale_timeout) .finish_non_exhaustive() @@ -129,6 +134,7 @@ impl Default for MinidumpIntegration { on_process: None, before_capture: None, flush_timeout: DEFAULT_FLUSH_TIMEOUT, + scope_timeout: DEFAULT_SCOPE_TIMEOUT, client_connect_timeout: None, server_stale_timeout: None, handle: OnceLock::new(), @@ -239,6 +245,16 @@ impl MinidumpIntegration { self } + /// Sets how long the crash handler waits for the crashing thread's + /// scope to be serialized before it requests the minidump without it. + /// + /// Defaults to 2 seconds. See the [crate docs](crate#scope). + #[must_use] + pub fn scope_timeout(mut self, timeout: Duration) -> Self { + self.scope_timeout = timeout; + self + } + /// Sets how long the app waits for the crash reporter to accept a /// connection. #[must_use] @@ -255,42 +271,19 @@ impl MinidumpIntegration { self } - /// Sends a scope update to the crash reporter process. - /// - /// Does nothing when there is no crash reporter, because this is the - /// reporter process, the DSN was unset, or the spawn failed. - fn send(&self, update: &ScopeUpdate) { - if let Some(handle) = self.handle.get() { - if let Ok(buffer) = serde_json::to_vec(update) { - handle.send_message(0, buffer).ok(); - } - } - } - - /// Adds a breadcrumb to the crash reporter's scope. - pub fn add_breadcrumb(&self, breadcrumb: Breadcrumb) { - self.send(&ScopeUpdate::AddBreadcrumb(breadcrumb)); - } - - /// Sets the user on the crash reporter's scope. - pub fn set_user(&self, user: Option) { - self.send(&ScopeUpdate::SetUser(user)); - } - - /// Sets or removes an extra value on the crash reporter's scope. - pub fn set_extra(&self, key: String, value: Option) { - self.send(&ScopeUpdate::SetExtra(key, value)); - } - - /// Sets or removes a tag on the crash reporter's scope. - pub fn set_tag(&self, key: String, value: Option) { - self.send(&ScopeUpdate::SetTag(key, value)); - } - /// Builds the child from the stored settings. - fn build_child(&self) -> MinidumperChild { + /// + /// `scope_sync` is set in the app process only; the reporter has no + /// crash handler of its own. + fn build_child(&self, scope_sync: Option>) -> MinidumperChild { let mut child = MinidumperChild::new().with_server_env_var(self.server_env_var.clone()); + if let Some(scope_sync) = scope_sync { + child = child.on_crash(move |crash_context, sender| { + scope_sync.on_crash(crash_context, sender); + }); + } + if let Some(dir) = &self.crashes_dir { child = child.with_crashes_dir(dir.clone()); } @@ -322,27 +315,19 @@ impl MinidumpIntegration { } }); - child = child.on_message(|_kind, buffer| { - if let Ok(update) = serde_json::from_slice::(&buffer) { - let hub = Hub::current(); - match update { - ScopeUpdate::AddBreadcrumb(b) => hub.add_breadcrumb(b), - ScopeUpdate::SetUser(u) => hub.configure_scope(|scope| scope.set_user(u)), - ScopeUpdate::SetExtra(k, v) => hub.configure_scope(|scope| match v { - Some(v) => scope.set_extra(&k, v), - None => scope.remove_extra(&k), - }), - ScopeUpdate::SetTag(k, v) => hub.configure_scope(|scope| match v { - Some(v) => scope.set_tag(&k, &v), - None => scope.remove_tag(&k), - }), - } - } + let receiver = Arc::new(ScopeReceiver::default()); + child = child.on_message({ + let receiver = receiver.clone(); + move |kind, buffer| receiver.on_message(kind, &buffer) }); let flush_timeout = self.flush_timeout; let before_capture = self.before_capture.clone(); child.on_minidump(move |buffer, path| { + // The scope of the crashing thread arrived before the dump. + let mut event = receiver.take_event(); + event.level = Level::Fatal; + // The client lives on the current hub, bound in the reporter // process before the server starts. Resolve it at crash time // rather than capturing it here. @@ -370,10 +355,7 @@ impl MinidumpIntegration { } }, || { - hub.capture_event(Event { - level: Level::Fatal, - ..Default::default() - }); + hub.capture_event(event); }, ); @@ -456,7 +438,7 @@ impl Integration for MinidumpIntegration { SETUP.call_once(|| { // Crash reporter process: run the minidump server; never returns. if self.is_crash_reporter_process() { - run_crash_reporter(self.build_child(), options, self.flush_timeout); + run_crash_reporter(self.build_child(None), options, self.flush_timeout); } setup_run = true; @@ -466,7 +448,8 @@ impl Integration for MinidumpIntegration { if self.handle.get().is_some() { return; } - match self.build_child().spawn() { + let scope_sync = ScopeSync::start(self.scope_timeout); + match self.build_child(Some(scope_sync)).spawn() { Ok(handle) => { let _ = self.handle.set(handle); } diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs new file mode 100644 index 00000000..bcf3ce94 --- /dev/null +++ b/sentry-minidump/src/scope_sync.rs @@ -0,0 +1,244 @@ +//! Moves the crashing thread's scope to the crash reporter. +//! +//! Nothing crosses the process boundary until a crash. The crash handler +//! then names the crashing OS thread, the scope of the hub current on that +//! thread is turned into an [`Event`], and the handler sends the JSON to +//! the reporter before it requests the minidump. + +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex, PoisonError}; +use std::time::Duration; + +use minidumper_child::{CrashContext, MessageSender}; +use sentry_core::protocol::Event; +use sentry_core::Hub; + +/// Message kinds on the socket between the app and the reporter. +pub(crate) const MSG_SCOPE_CHUNK: u32 = 1; +pub(crate) const MSG_SCOPE_END: u32 = 2; + +/// The reporter reads one message in one call and does not reassemble +/// partial reads on every platform, so keep each message small. +const CHUNK_SIZE: usize = 16 * 1024; + +/// Hand-off between the crash handler and the code that serializes the +/// scope. +pub(crate) struct ScopeSync { + done: AtomicBool, + buffer: Mutex>, + #[cfg(not(target_os = "macos"))] + helper: Helper, +} + +/// The parked thread that does the serializing on Linux and Windows, +/// where the crash handler runs on the crashing thread and may not +/// allocate. +#[cfg(not(target_os = "macos"))] +struct Helper { + thread_id: std::sync::atomic::AtomicU64, + requested: AtomicBool, + thread: std::thread::Thread, + timeout: Duration, +} + +impl ScopeSync { + /// Prepares the hand-off. On Linux and Windows this starts the helper + /// thread, which parks until a crash happens. + pub(crate) fn start(timeout: Duration) -> Arc { + #[cfg(target_os = "macos")] + { + let _ = timeout; + Arc::new(ScopeSync { + done: AtomicBool::new(false), + buffer: Mutex::new(Vec::new()), + }) + } + + #[cfg(not(target_os = "macos"))] + { + use std::sync::atomic::AtomicU64; + use std::thread; + + let (tx, rx) = std::sync::mpsc::sync_channel::>(1); + + let handle = thread::Builder::new() + .name("sentry-minidump-scope".into()) + .spawn(move || { + let Ok(sync) = rx.recv() else { return }; + while !sync.helper.requested.load(Ordering::Acquire) { + thread::park(); + } + let bytes = scope_bytes(sync.helper.thread_id.load(Ordering::Acquire)); + *sync.buffer.lock().unwrap_or_else(PoisonError::into_inner) = bytes; + sync.done.store(true, Ordering::Release); + }) + .expect("spawn scope helper thread"); + + let sync = Arc::new(ScopeSync { + done: AtomicBool::new(false), + buffer: Mutex::new(Vec::new()), + helper: Helper { + thread_id: AtomicU64::new(0), + requested: AtomicBool::new(false), + thread: handle.thread().clone(), + timeout, + }, + }); + tx.send(sync.clone()).expect("helper thread is waiting"); + sync + } + } + + /// Runs inside the crash handler. + /// + /// On Linux and Windows this is the crashing thread, so it must not + /// allocate or take locks. It only touches atomics, wakes the helper, + /// sleeps, and writes from a buffer the helper filled. On macOS every + /// other thread is suspended, so the work runs inline here instead. + pub(crate) fn on_crash(&self, crash_context: &CrashContext, sender: &MessageSender<'_>) { + let thread_id = crashing_thread_id(crash_context); + + #[cfg(target_os = "macos")] + { + *self.buffer.lock().unwrap_or_else(PoisonError::into_inner) = scope_bytes(thread_id); + self.done.store(true, Ordering::Release); + } + + #[cfg(not(target_os = "macos"))] + { + self.helper.thread_id.store(thread_id, Ordering::Release); + self.helper.requested.store(true, Ordering::Release); + self.helper.thread.unpark(); + + let step = Duration::from_millis(1); + let mut waited = Duration::ZERO; + while !self.done.load(Ordering::Acquire) { + if waited >= self.helper.timeout { + return; + } + std::thread::sleep(step); + waited = waited.saturating_add(step); + } + } + + // The lock was released before `done` was set, so this cannot block. + if let Ok(buffer) = self.buffer.try_lock() { + send_chunks(sender, &buffer); + } + } +} + +fn send_chunks(sender: &MessageSender<'_>, bytes: &[u8]) { + for chunk in bytes.chunks(CHUNK_SIZE) { + if sender.send_message(MSG_SCOPE_CHUNK, chunk).is_err() { + return; + } + } + sender.send_message(MSG_SCOPE_END, &[]).ok(); +} + +/// The id of the crashing thread as [`sentry_core::current_os_thread_id`] +/// reports it on that thread. +fn crashing_thread_id(crash_context: &CrashContext) -> u64 { + #[cfg(any(target_os = "linux", target_os = "android"))] + { + crash_context.tid as u64 + } + #[cfg(target_os = "windows")] + { + crash_context.thread_id as u64 + } + #[cfg(target_os = "macos")] + { + crash_context.thread as u64 + } +} + +/// Serializes the scope of the hub current on `thread_id`. +/// +/// Falls back to the main hub for threads that never used Sentry, which +/// is the scope such a thread would have inherited. +fn scope_bytes(thread_id: u64) -> Vec { + let hub = Hub::for_os_thread(thread_id).unwrap_or_else(Hub::main); + let scope = hub.configure_scope(|scope| scope.clone()); + scope + .apply_to_event(Event::default()) + .and_then(|event| serde_json::to_vec(&event).ok()) + .unwrap_or_default() +} + +/// Collects the scope messages in the reporter process. +#[derive(Default)] +pub(crate) struct ScopeReceiver { + chunks: Mutex>, + event: Mutex>>, +} + +impl ScopeReceiver { + pub(crate) fn on_message(&self, kind: u32, buffer: &[u8]) { + match kind { + MSG_SCOPE_CHUNK => self + .chunks + .lock() + .unwrap_or_else(PoisonError::into_inner) + .extend_from_slice(buffer), + MSG_SCOPE_END => { + let bytes = std::mem::take( + &mut *self.chunks.lock().unwrap_or_else(PoisonError::into_inner), + ); + if let Ok(event) = serde_json::from_slice::>(&bytes) { + *self.event.lock().unwrap_or_else(PoisonError::into_inner) = Some(event); + } + } + _ => {} + } + } + + /// Returns the received scope as an event, or an empty event. + pub(crate) fn take_event(&self) -> Event<'static> { + self.event + .lock() + .unwrap_or_else(PoisonError::into_inner) + .take() + .unwrap_or_default() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn receiver_reassembles_chunks() { + let event = Event { + message: Some("hello".into()), + ..Default::default() + }; + let bytes = serde_json::to_vec(&event).unwrap(); + let receiver = ScopeReceiver::default(); + let (a, b) = bytes.split_at(bytes.len() / 2); + receiver.on_message(MSG_SCOPE_CHUNK, a); + receiver.on_message(MSG_SCOPE_CHUNK, b); + receiver.on_message(MSG_SCOPE_END, &[]); + assert_eq!(receiver.take_event().message.as_deref(), Some("hello")); + assert!(receiver.take_event().message.is_none()); + } + + #[test] + fn receiver_ignores_bad_json_and_other_kinds() { + let receiver = ScopeReceiver::default(); + receiver.on_message(99, b"ignored"); + receiver.on_message(MSG_SCOPE_CHUNK, b"not json"); + receiver.on_message(MSG_SCOPE_END, &[]); + assert!(receiver.take_event().message.is_none()); + } + + #[test] + fn scope_bytes_uses_the_thread_hub() { + let hub = Arc::new(Hub::new(None, Default::default())); + hub.configure_scope(|scope| scope.set_tag("thread", "worker")); + let bytes = Hub::run(hub, || scope_bytes(sentry_core::current_os_thread_id())); + let event: Event<'static> = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); + } +} diff --git a/sentry-minidump/tests/e2e.rs b/sentry-minidump/tests/e2e.rs index 118da062..7cbd980e 100644 --- a/sentry-minidump/tests/e2e.rs +++ b/sentry-minidump/tests/e2e.rs @@ -101,10 +101,28 @@ fn captures_minidump_from_crash() { Some("example") ); - // Set through `with_integration` before the crash. + // The scope of the worker thread that crashed, not the main thread's. let user = event.user.as_ref().expect("event has a user"); assert_eq!(user.username.as_deref(), Some("john_doe")); assert_eq!(user.email.as_deref(), Some("john@doe.town")); + assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); + assert_eq!( + event.breadcrumbs.last().and_then(|b| b.message.as_deref()), + Some("about to crash") + ); + + // Inherited from the main scope when the worker hub was created. + assert_eq!( + event.tags.get("shared").map(String::as_str), + Some("from_main") + ); + + // Set on the main thread after the worker hub was created. + assert!( + !event.tags.contains_key("main_only"), + "main thread scope leaked into the crash event: {:?}", + event.tags + ); let attachment = envelope .items() From 395bd3a29b0767b75a6ee84fe4b23beccc4a298d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 4 Oct 2026 11:34:24 +0100 Subject: [PATCH 03/12] Remove both `expect` calls --- sentry-minidump/src/scope_sync.rs | 47 +++++++++++++++++++------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs index bcf3ce94..542956b9 100644 --- a/sentry-minidump/src/scope_sync.rs +++ b/sentry-minidump/src/scope_sync.rs @@ -37,7 +37,8 @@ pub(crate) struct ScopeSync { struct Helper { thread_id: std::sync::atomic::AtomicU64, requested: AtomicBool, - thread: std::thread::Thread, + /// Unset when the helper thread failed to start. + thread: std::sync::OnceLock, timeout: Duration, } @@ -57,34 +58,41 @@ impl ScopeSync { #[cfg(not(target_os = "macos"))] { use std::sync::atomic::AtomicU64; + use std::sync::OnceLock; use std::thread; - let (tx, rx) = std::sync::mpsc::sync_channel::>(1); + let sync = Arc::new(ScopeSync { + done: AtomicBool::new(false), + buffer: Mutex::new(Vec::new()), + helper: Helper { + thread_id: AtomicU64::new(0), + requested: AtomicBool::new(false), + thread: OnceLock::new(), + timeout, + }, + }); - let handle = thread::Builder::new() + let helper_sync = sync.clone(); + let spawned = thread::Builder::new() .name("sentry-minidump-scope".into()) .spawn(move || { - let Ok(sync) = rx.recv() else { return }; + let sync = helper_sync; while !sync.helper.requested.load(Ordering::Acquire) { thread::park(); } let bytes = scope_bytes(sync.helper.thread_id.load(Ordering::Acquire)); *sync.buffer.lock().unwrap_or_else(PoisonError::into_inner) = bytes; sync.done.store(true, Ordering::Release); - }) - .expect("spawn scope helper thread"); + }); - let sync = Arc::new(ScopeSync { - done: AtomicBool::new(false), - buffer: Mutex::new(Vec::new()), - helper: Helper { - thread_id: AtomicU64::new(0), - requested: AtomicBool::new(false), - thread: handle.thread().clone(), - timeout, - }, - }); - tx.send(sync.clone()).expect("helper thread is waiting"); + match spawned { + Ok(handle) => { + let _ = sync.helper.thread.set(handle.thread().clone()); + } + Err(err) => { + sentry_core::sentry_debug!("could not start scope helper thread: {err}"); + } + } sync } } @@ -106,9 +114,12 @@ impl ScopeSync { #[cfg(not(target_os = "macos"))] { + let Some(helper_thread) = self.helper.thread.get() else { + return; + }; self.helper.thread_id.store(thread_id, Ordering::Release); self.helper.requested.store(true, Ordering::Release); - self.helper.thread.unpark(); + helper_thread.unpark(); let step = Duration::from_millis(1); let mut waited = Duration::ZERO; From af0d98bca70c0945b818086288b360b4ee7b4fcf Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 13:09:35 +0100 Subject: [PATCH 04/12] build(deps): Use minidumper-child 0.6 0.6 has the on_crash hook, so the git patch is no longer needed. --- Cargo.lock | 5 +++-- Cargo.toml | 6 +----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 80297ea2..28354a27 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2350,8 +2350,9 @@ dependencies = [ [[package]] name = "minidumper-child" -version = "0.5.0" -source = "git+https://github.com/timfish/minidumper-child?branch=feat%2Fon-crash-hook#ebba9e44e458141c10afd33a3bc07267f1eeca15" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb9f826d139b8d99d4ed81d2372fb0398d702cac213a18dd9775167df347b00c" dependencies = [ "crash-handler", "minidumper", diff --git a/Cargo.toml b/Cargo.toml index d22b8dd2..f688340f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,7 +58,7 @@ http = "1.0.0" httpdate = "1.0.0" libc = "0.2.66" log = "0.4.8" -minidumper-child = "0.5" +minidumper-child = "0.6" native-tls = "0.2.8" opentelemetry = { version = "0.32.0", default-features = false } opentelemetry_sdk = { version = "0.32.1", default-features = false } @@ -106,7 +106,3 @@ uname = "0.1.1" ureq = { version = "3.0.11", default-features = false } url = "2.2.2" uuid = "1.0.0" - -[patch.crates-io] -# Pending release of the `on_crash` hook. -minidumper-child = { git = "https://github.com/timfish/minidumper-child", branch = "feat/on-crash-hook" } From 7c2c97a6cb038fe2222912efd21af1f0bcb64504 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 13:31:05 +0100 Subject: [PATCH 05/12] meta(changelog): List the removed minidump scope methods as breaking --- CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09cb823c..ed9c87dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,14 @@ ## Unreleased +### Breaking Changes + +- Removed the `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods from `MinidumpIntegration`. The crash event now gets the scope of the crashing thread on its own, so remove these calls ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). + ### Features -- The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The manual `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods on `MinidumpIntegration` are removed, and `scope_timeout` bounds the wait for the scope. -- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread. +- The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The new `scope_timeout` option bounds the wait for the scope ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). ## 0.49.3 From db9875b25b10a1109dd3a5aaeab363d0595f9b94 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 13:39:07 +0100 Subject: [PATCH 06/12] fix(minidump): Use the scope helper thread on macOS too On macOS the crash handler serialized the scope inline, with no timeout. A crash while the crashing thread held the hub lock, the registry mutex or the allocator lock blocked the handler forever, so no minidump was written. The helper thread now runs on every platform. On macOS the handler resumes it with thread_resume, because crash-handler suspends every other thread before the callback. The handler then waits at most scope_timeout, as on Linux and Windows, and requests the dump without scope if the helper is blocked. --- Cargo.lock | 1 + Cargo.toml | 1 + sentry-minidump/Cargo.toml | 3 + sentry-minidump/src/scope_sync.rs | 171 +++++++++++++++--------------- 4 files changed, 89 insertions(+), 87 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 28354a27..9bf9e7f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3710,6 +3710,7 @@ dependencies = [ name = "sentry-minidump" version = "0.49.3" dependencies = [ + "mach2", "minidumper-child", "sadness-generator", "sentry", diff --git a/Cargo.toml b/Cargo.toml index f688340f..5a578fb5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ http = "1.0.0" httpdate = "1.0.0" libc = "0.2.66" log = "0.4.8" +mach2 = "0.4" minidumper-child = "0.6" native-tls = "0.2.8" opentelemetry = { version = "0.32.0", default-features = false } diff --git a/sentry-minidump/Cargo.toml b/sentry-minidump/Cargo.toml index 6487611c..1aa16baa 100644 --- a/sentry-minidump/Cargo.toml +++ b/sentry-minidump/Cargo.toml @@ -24,6 +24,9 @@ sentry-core = { workspace = true, features = ["client", "thread-registry"] } minidumper-child = { workspace = true } serde_json = { workspace = true } +[target.'cfg(target_os = "macos")'.dependencies] +mach2 = { workspace = true } + [dev-dependencies] sentry = { workspace = true, features = ["test", "ureq"] } sadness-generator = { workspace = true } diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs index 542956b9..1e466d53 100644 --- a/sentry-minidump/src/scope_sync.rs +++ b/sentry-minidump/src/scope_sync.rs @@ -5,8 +5,11 @@ //! thread is turned into an [`Event`], and the handler sends the JSON to //! the reporter before it requests the minidump. -use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex, PoisonError}; +#[cfg(target_os = "macos")] +use std::sync::atomic::AtomicU32; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::{Arc, Mutex, OnceLock, PoisonError}; +use std::thread::{self, Thread}; use std::time::Duration; use minidumper_child::{CrashContext, MessageSender}; @@ -21,115 +24,94 @@ pub(crate) const MSG_SCOPE_END: u32 = 2; /// partial reads on every platform, so keep each message small. const CHUNK_SIZE: usize = 16 * 1024; -/// Hand-off between the crash handler and the code that serializes the -/// scope. +/// Hand-off between the crash handler and the helper thread that +/// serializes the scope. +/// +/// The crash handler may run on the crashing thread, which can hold the +/// allocator or a hub lock. So the handler only touches atomics, wakes the +/// helper and waits at most `timeout`. All the work that can block runs +/// on the helper. pub(crate) struct ScopeSync { done: AtomicBool, buffer: Mutex>, - #[cfg(not(target_os = "macos"))] - helper: Helper, -} - -/// The parked thread that does the serializing on Linux and Windows, -/// where the crash handler runs on the crashing thread and may not -/// allocate. -#[cfg(not(target_os = "macos"))] -struct Helper { - thread_id: std::sync::atomic::AtomicU64, + thread_id: AtomicU64, requested: AtomicBool, /// Unset when the helper thread failed to start. - thread: std::sync::OnceLock, + thread: OnceLock, + /// The helper's Mach port, or 0 before the helper has started. + #[cfg(target_os = "macos")] + helper_port: AtomicU32, timeout: Duration, } impl ScopeSync { - /// Prepares the hand-off. On Linux and Windows this starts the helper - /// thread, which parks until a crash happens. + /// Starts the helper thread, which parks until a crash happens. pub(crate) fn start(timeout: Duration) -> Arc { - #[cfg(target_os = "macos")] - { - let _ = timeout; - Arc::new(ScopeSync { - done: AtomicBool::new(false), - buffer: Mutex::new(Vec::new()), - }) - } - - #[cfg(not(target_os = "macos"))] - { - use std::sync::atomic::AtomicU64; - use std::sync::OnceLock; - use std::thread; - - let sync = Arc::new(ScopeSync { - done: AtomicBool::new(false), - buffer: Mutex::new(Vec::new()), - helper: Helper { - thread_id: AtomicU64::new(0), - requested: AtomicBool::new(false), - thread: OnceLock::new(), - timeout, - }, + let sync = Arc::new(ScopeSync { + done: AtomicBool::new(false), + buffer: Mutex::new(Vec::new()), + thread_id: AtomicU64::new(0), + requested: AtomicBool::new(false), + thread: OnceLock::new(), + #[cfg(target_os = "macos")] + helper_port: AtomicU32::new(0), + timeout, + }); + + let helper_sync = sync.clone(); + let spawned = thread::Builder::new() + .name("sentry-minidump-scope".into()) + .spawn(move || { + let sync = helper_sync; + #[cfg(target_os = "macos")] + sync.helper_port.store( + sentry_core::current_os_thread_id() as u32, + Ordering::Release, + ); + while !sync.requested.load(Ordering::Acquire) { + thread::park(); + } + let bytes = scope_bytes(sync.thread_id.load(Ordering::Acquire)); + *sync.buffer.lock().unwrap_or_else(PoisonError::into_inner) = bytes; + sync.done.store(true, Ordering::Release); }); - let helper_sync = sync.clone(); - let spawned = thread::Builder::new() - .name("sentry-minidump-scope".into()) - .spawn(move || { - let sync = helper_sync; - while !sync.helper.requested.load(Ordering::Acquire) { - thread::park(); - } - let bytes = scope_bytes(sync.helper.thread_id.load(Ordering::Acquire)); - *sync.buffer.lock().unwrap_or_else(PoisonError::into_inner) = bytes; - sync.done.store(true, Ordering::Release); - }); - - match spawned { - Ok(handle) => { - let _ = sync.helper.thread.set(handle.thread().clone()); - } - Err(err) => { - sentry_core::sentry_debug!("could not start scope helper thread: {err}"); - } + match spawned { + Ok(handle) => { + let _ = sync.thread.set(handle.thread().clone()); + } + Err(err) => { + sentry_core::sentry_debug!("could not start scope helper thread: {err}"); } - sync } + sync } /// Runs inside the crash handler. /// - /// On Linux and Windows this is the crashing thread, so it must not - /// allocate or take locks. It only touches atomics, wakes the helper, - /// sleeps, and writes from a buffer the helper filled. On macOS every - /// other thread is suspended, so the work runs inline here instead. + /// It must not allocate or take locks. It only touches atomics, wakes + /// the helper, sleeps, and writes from a buffer the helper filled. pub(crate) fn on_crash(&self, crash_context: &CrashContext, sender: &MessageSender<'_>) { - let thread_id = crashing_thread_id(crash_context); + let Some(helper_thread) = self.thread.get() else { + return; + }; + self.thread_id + .store(crashing_thread_id(crash_context), Ordering::Release); + self.requested.store(true, Ordering::Release); #[cfg(target_os = "macos")] - { - *self.buffer.lock().unwrap_or_else(PoisonError::into_inner) = scope_bytes(thread_id); - self.done.store(true, Ordering::Release); - } + resume_helper(self.helper_port.load(Ordering::Acquire)); + + helper_thread.unpark(); - #[cfg(not(target_os = "macos"))] - { - let Some(helper_thread) = self.helper.thread.get() else { + let step = Duration::from_millis(1); + let mut waited = Duration::ZERO; + while !self.done.load(Ordering::Acquire) { + if waited >= self.timeout { return; - }; - self.helper.thread_id.store(thread_id, Ordering::Release); - self.helper.requested.store(true, Ordering::Release); - helper_thread.unpark(); - - let step = Duration::from_millis(1); - let mut waited = Duration::ZERO; - while !self.done.load(Ordering::Acquire) { - if waited >= self.helper.timeout { - return; - } - std::thread::sleep(step); - waited = waited.saturating_add(step); } + thread::sleep(step); + waited = waited.saturating_add(step); } // The lock was released before `done` was set, so this cannot block. @@ -139,6 +121,21 @@ impl ScopeSync { } } +/// On macOS the crash handler runs on its own thread and suspends every +/// other thread first, the helper included. +#[cfg(target_os = "macos")] +fn resume_helper(port: u32) { + if port == 0 { + return; + } + // SAFETY: The helper only exits after it sets `done`, so `port` + // still names it. `thread_resume` only lowers its suspend count. + #[expect(unsafe_code, reason = "Mach call to resume the helper thread")] + unsafe { + mach2::thread_act::thread_resume(port); + } +} + fn send_chunks(sender: &MessageSender<'_>, bytes: &[u8]) { for chunk in bytes.chunks(CHUNK_SIZE) { if sender.send_message(MSG_SCOPE_CHUNK, chunk).is_err() { From 34a9e12e08e5ec87b1b2f841b8e516915cad99cf Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 14:01:57 +0100 Subject: [PATCH 07/12] feat(core): Add Hub::scope_snapshot behind the thread-registry feature It copies the hub's current scope under the read lock only. configure_scope also writes the scope back under the write lock, so a reader that only needs a copy can block on another thread's read lock. --- CHANGELOG.md | 2 +- sentry-core/src/hub_impl.rs | 9 +++++++++ sentry-core/src/thread_registry.rs | 13 +++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed9c87dd..fe5df4d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ ### Features - The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The new `scope_timeout` option bounds the wait for the scope ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). -- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread, and `Hub::scope_snapshot`, which copies a hub's scope without taking its write lock ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). ## 0.49.3 diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 4a36772b..2218c3e3 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -189,6 +189,15 @@ impl Hub { crate::thread_registry::hub_for_os_thread(os_thread_id) } + /// Returns a copy of the hub's current scope. + /// + /// Unlike [`Hub::configure_scope`], this only takes the hub's read + /// lock, so it does not wait for other threads that read the scope. + #[cfg(feature = "thread-registry")] + pub fn scope_snapshot(&self) -> Scope { + self.with_current_scope(Scope::clone) + } + /// Invokes the callback with the default hub. #[deprecated = "Use `Hub::current` instead; this function offers no performance benefit."] pub fn with(f: F) -> R diff --git a/sentry-core/src/thread_registry.rs b/sentry-core/src/thread_registry.rs index 5a734246..59555efa 100644 --- a/sentry-core/src/thread_registry.rs +++ b/sentry-core/src/thread_registry.rs @@ -163,4 +163,17 @@ mod tests { fn unknown_thread_has_no_hub() { assert!(Hub::for_os_thread(u64::MAX).is_none()); } + + #[test] + fn scope_snapshot_works_while_the_scope_is_read() { + let hub = Hub::new(None, Default::default()); + hub.configure_scope(|scope| scope.set_tag("thread", "worker")); + + // A write lock here would never be granted. + let snapshot = hub.with_current_scope(|_| hub.scope_snapshot()); + let event = snapshot + .apply_to_event(Default::default()) + .expect("no event processors drop the event"); + assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); + } } From 7504c8150d4c37390f62da431cb14bacbe07220d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 14:01:57 +0100 Subject: [PATCH 08/12] fix(minidump): Avoid two ways to lose the crashing thread's scope The helper now copies the scope with Hub::scope_snapshot. With configure_scope it took the hub's write lock, and blocked until scope_timeout if the crashing thread held the read lock. On macOS, start now waits until the helper has stored its Mach port. A crash right after init could otherwise find no port, leave the helper suspended and send the event without scope. --- sentry-minidump/src/scope_sync.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs index 1e466d53..546d3406 100644 --- a/sentry-minidump/src/scope_sync.rs +++ b/sentry-minidump/src/scope_sync.rs @@ -8,6 +8,8 @@ #[cfg(target_os = "macos")] use std::sync::atomic::AtomicU32; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +#[cfg(target_os = "macos")] +use std::sync::Barrier; use std::sync::{Arc, Mutex, OnceLock, PoisonError}; use std::thread::{self, Thread}; use std::time::Duration; @@ -38,7 +40,7 @@ pub(crate) struct ScopeSync { requested: AtomicBool, /// Unset when the helper thread failed to start. thread: OnceLock, - /// The helper's Mach port, or 0 before the helper has started. + /// The helper's Mach port. #[cfg(target_os = "macos")] helper_port: AtomicU32, timeout: Duration, @@ -58,16 +60,26 @@ impl ScopeSync { timeout, }); + // On macOS the handler needs the helper's port to resume it, so + // `start` waits until the helper has stored it. + #[cfg(target_os = "macos")] + let port_stored = Arc::new(Barrier::new(2)); + #[cfg(target_os = "macos")] + let helper_port_stored = port_stored.clone(); + let helper_sync = sync.clone(); let spawned = thread::Builder::new() .name("sentry-minidump-scope".into()) .spawn(move || { let sync = helper_sync; #[cfg(target_os = "macos")] - sync.helper_port.store( - sentry_core::current_os_thread_id() as u32, - Ordering::Release, - ); + { + sync.helper_port.store( + sentry_core::current_os_thread_id() as u32, + Ordering::Release, + ); + helper_port_stored.wait(); + } while !sync.requested.load(Ordering::Acquire) { thread::park(); } @@ -78,6 +90,8 @@ impl ScopeSync { match spawned { Ok(handle) => { + #[cfg(target_os = "macos")] + port_stored.wait(); let _ = sync.thread.set(handle.thread().clone()); } Err(err) => { @@ -125,9 +139,6 @@ impl ScopeSync { /// other thread first, the helper included. #[cfg(target_os = "macos")] fn resume_helper(port: u32) { - if port == 0 { - return; - } // SAFETY: The helper only exits after it sets `done`, so `port` // still names it. `thread_resume` only lowers its suspend count. #[expect(unsafe_code, reason = "Mach call to resume the helper thread")] @@ -168,8 +179,7 @@ fn crashing_thread_id(crash_context: &CrashContext) -> u64 { /// is the scope such a thread would have inherited. fn scope_bytes(thread_id: u64) -> Vec { let hub = Hub::for_os_thread(thread_id).unwrap_or_else(Hub::main); - let scope = hub.configure_scope(|scope| scope.clone()); - scope + hub.scope_snapshot() .apply_to_event(Event::default()) .and_then(|event| serde_json::to_vec(&event).ok()) .unwrap_or_default() From 1d517e72873b51901d20f998bf585f1ae3ac48ab Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 15:15:13 +0100 Subject: [PATCH 09/12] fix(core): Only register threads with a valid OS thread id current_os_thread_id now returns Option and exists only on Linux, Android, macOS and Windows. The pthread_self fallback also compiled on non-unix targets, where libc is not a dependency. On Linux a seccomp filter can make gettid return -1 without running it. Cast to u64, every such thread registered under u64::MAX and overwrote the others. A failed gettid now gives None, and a thread without an id is not registered. --- sentry-core/src/lib.rs | 10 +++- sentry-core/src/thread_registry.rs | 92 +++++++++++++++++++++++------- sentry-minidump/src/scope_sync.rs | 11 ++-- 3 files changed, 87 insertions(+), 26 deletions(-) diff --git a/sentry-core/src/lib.rs b/sentry-core/src/lib.rs index ff80060f..43391a49 100644 --- a/sentry-core/src/lib.rs +++ b/sentry-core/src/lib.rs @@ -148,7 +148,15 @@ mod hub_impl; mod session; #[cfg(feature = "thread-registry")] mod thread_registry; -#[cfg(feature = "thread-registry")] +#[cfg(all( + feature = "thread-registry", + any( + target_os = "linux", + target_os = "android", + target_os = "macos", + windows + ) +))] pub use crate::thread_registry::current_os_thread_id; #[cfg(feature = "client")] diff --git a/sentry-core/src/thread_registry.rs b/sentry-core/src/thread_registry.rs index 59555efa..f1a65677 100644 --- a/sentry-core/src/thread_registry.rs +++ b/sentry-core/src/thread_registry.rs @@ -19,19 +19,27 @@ use crate::Hub; /// - Windows: `GetCurrentThreadId`. /// - macOS: the Mach port name of the thread, as reported in exception /// messages and returned by `pthread_mach_thread_np`. -/// - Other unix targets: `pthread_self`. +/// +/// Returns `None` when the OS refuses the call, for example when a +/// seccomp filter blocks `gettid`. /// /// It is not related to [`std::thread::ThreadId`]. -pub fn current_os_thread_id() -> u64 { +#[cfg(any( + target_os = "linux", + target_os = "android", + target_os = "macos", + windows +))] +pub fn current_os_thread_id() -> Option { #[cfg(any(target_os = "linux", target_os = "android"))] { - // SAFETY: `gettid` takes no arguments and cannot fail. - unsafe { libc::syscall(libc::SYS_gettid) as u64 } + // SAFETY: `gettid` takes no arguments. + thread_id_from_syscall(unsafe { libc::syscall(libc::SYS_gettid) }) } #[cfg(target_os = "macos")] { // SAFETY: `pthread_self` is always valid for the calling thread. - unsafe { libc::pthread_mach_thread_np(libc::pthread_self()) as u64 } + Some(unsafe { libc::pthread_mach_thread_np(libc::pthread_self()) }.into()) } #[cfg(windows)] { @@ -39,7 +47,28 @@ pub fn current_os_thread_id() -> u64 { fn GetCurrentThreadId() -> u32; } // SAFETY: takes no arguments and cannot fail. - unsafe { GetCurrentThreadId() as u64 } + Some(unsafe { GetCurrentThreadId() }.into()) + } +} + +/// Turns the result of the `gettid` syscall into a thread id. +/// +/// A seccomp filter can make the syscall return -1 without running it. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn thread_id_from_syscall(ret: libc::c_long) -> Option { + u64::try_from(ret).ok().filter(|&id| id > 0) +} + +/// The id to register the current thread under, if the OS reports one. +fn registry_id() -> Option { + #[cfg(any( + target_os = "linux", + target_os = "android", + target_os = "macos", + windows + ))] + { + current_os_thread_id() } #[cfg(not(any( target_os = "linux", @@ -48,14 +77,14 @@ pub fn current_os_thread_id() -> u64 { windows )))] { - // SAFETY: `pthread_self` is always valid for the calling thread. - unsafe { libc::pthread_self() as u64 } + None } } /// One thread's entry in the registry. struct Slot { - os_thread_id: u64, + /// `None` when the thread has no OS id and is not in the registry. + os_thread_id: Option, hub: RwLock>, } @@ -66,35 +95,40 @@ static REGISTRY: LazyLock>>> = struct SlotGuard(Arc); impl SlotGuard { - fn register() -> Self { + fn register(os_thread_id: Option) -> Self { let slot = Arc::new(Slot { - os_thread_id: current_os_thread_id(), + os_thread_id, hub: RwLock::new(Weak::new()), }); - REGISTRY - .lock() - .unwrap_or_else(PoisonError::into_inner) - .insert(slot.os_thread_id, slot.clone()); + if let Some(id) = os_thread_id { + REGISTRY + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(id, slot.clone()); + } SlotGuard(slot) } } impl Drop for SlotGuard { fn drop(&mut self) { + let Some(id) = self.0.os_thread_id else { + return; + }; let mut registry = REGISTRY.lock().unwrap_or_else(PoisonError::into_inner); // The OS can reuse the id for a new thread before this one is // fully gone, so only remove the entry if it is still ours. if registry - .get(&self.0.os_thread_id) + .get(&id) .is_some_and(|slot| Arc::ptr_eq(slot, &self.0)) { - registry.remove(&self.0.os_thread_id); + registry.remove(&id); } } } thread_local! { - static SLOT: SlotGuard = SlotGuard::register(); + static SLOT: SlotGuard = SlotGuard::register(registry_id()); } /// Records `hub` as the hub current on the calling thread. @@ -129,7 +163,7 @@ mod tests { let worker = std::thread::spawn(move || { let hub = Hub::current(); - id_tx.send((current_os_thread_id(), hub)).unwrap(); + id_tx.send((current_os_thread_id().unwrap(), hub)).unwrap(); // Keep the thread alive until the main thread has looked it up. done_rx.recv().ok(); }); @@ -146,7 +180,7 @@ mod tests { #[test] fn follows_hub_run_switching() { - let id = current_os_thread_id(); + let id = current_os_thread_id().unwrap(); let outer = Hub::current(); let inner = Arc::new(Hub::new_from_top(&outer)); @@ -164,6 +198,24 @@ mod tests { assert!(Hub::for_os_thread(u64::MAX).is_none()); } + #[test] + fn thread_without_id_is_not_registered() { + let guard = SlotGuard::register(None); + assert!(!REGISTRY + .lock() + .unwrap() + .values() + .any(|slot| Arc::ptr_eq(slot, &guard.0))); + } + + #[cfg(any(target_os = "linux", target_os = "android"))] + #[test] + fn failed_gettid_has_no_id() { + assert_eq!(thread_id_from_syscall(-1), None); + assert_eq!(thread_id_from_syscall(0), None); + assert_eq!(thread_id_from_syscall(42), Some(42)); + } + #[test] fn scope_snapshot_works_while_the_scope_is_read() { let hub = Hub::new(None, Default::default()); diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs index 546d3406..5283cde6 100644 --- a/sentry-minidump/src/scope_sync.rs +++ b/sentry-minidump/src/scope_sync.rs @@ -74,10 +74,9 @@ impl ScopeSync { let sync = helper_sync; #[cfg(target_os = "macos")] { - sync.helper_port.store( - sentry_core::current_os_thread_id() as u32, - Ordering::Release, - ); + // The Mach port of the current thread is a `u32`. + let port = sentry_core::current_os_thread_id().map_or(0, |id| id as u32); + sync.helper_port.store(port, Ordering::Release); helper_port_stored.wait(); } while !sync.requested.load(Ordering::Acquire) { @@ -255,7 +254,9 @@ mod tests { fn scope_bytes_uses_the_thread_hub() { let hub = Arc::new(Hub::new(None, Default::default())); hub.configure_scope(|scope| scope.set_tag("thread", "worker")); - let bytes = Hub::run(hub, || scope_bytes(sentry_core::current_os_thread_id())); + let bytes = Hub::run(hub, || { + scope_bytes(sentry_core::current_os_thread_id().unwrap()) + }); let event: Event<'static> = serde_json::from_slice(&bytes).unwrap(); assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); } From 7773d58ffab15618eb72c3a3311359091ccc1a5b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 8 Oct 2026 15:29:35 +0100 Subject: [PATCH 10/12] feat(minidump): Keep the scope methods, deprecated set_user, set_tag, set_extra and add_breadcrumb on MinidumpIntegration work as in 0.49.3 again, so this change is not breaking. Each call still sends a scope update to the reporter, which applies it to its hub. The crash event already passes through that hub, so these values merge with the crashing thread's scope. --- CHANGELOG.md | 8 +-- Cargo.lock | 1 + sentry-minidump/Cargo.toml | 5 ++ sentry-minidump/examples/deprecated.rs | 21 +++++++ sentry-minidump/src/lib.rs | 79 +++++++++++++++++++++++++- sentry-minidump/tests/e2e.rs | 44 ++++++++++---- 6 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 sentry-minidump/examples/deprecated.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5df4d1..ea15f845 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,15 @@ ## Unreleased -### Breaking Changes - -- Removed the `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods from `MinidumpIntegration`. The crash event now gets the scope of the crashing thread on its own, so remove these calls ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). - ### Features - The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The new `scope_timeout` option bounds the wait for the scope ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). - Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread, and `Hub::scope_snapshot`, which copies a hub's scope without taking its write lock ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +### Deprecations + +- Deprecated the `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods on `MinidumpIntegration`. The crash event now carries the scope of the crashing thread, so set these values with `sentry::configure_scope` and `sentry::add_breadcrumb` instead ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). + ## 0.49.3 ### Features diff --git a/Cargo.lock b/Cargo.lock index 9bf9e7f9..26a69039 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3715,6 +3715,7 @@ dependencies = [ "sadness-generator", "sentry", "sentry-core", + "serde", "serde_json", ] diff --git a/sentry-minidump/Cargo.toml b/sentry-minidump/Cargo.toml index 1aa16baa..d5dcb8ea 100644 --- a/sentry-minidump/Cargo.toml +++ b/sentry-minidump/Cargo.toml @@ -19,9 +19,14 @@ workspace = true name = "minidump" path = "examples/app.rs" +[[example]] +name = "minidump-deprecated" +path = "examples/deprecated.rs" + [dependencies] sentry-core = { workspace = true, features = ["client", "thread-registry"] } minidumper-child = { workspace = true } +serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } [target.'cfg(target_os = "macos")'.dependencies] diff --git a/sentry-minidump/examples/deprecated.rs b/sentry-minidump/examples/deprecated.rs new file mode 100644 index 00000000..15e6c56d --- /dev/null +++ b/sentry-minidump/examples/deprecated.rs @@ -0,0 +1,21 @@ +//! Crashes after setting a tag with a deprecated `MinidumpIntegration` +//! method. +//! +//! The end to end test runs this to check that the deprecated methods +//! still reach the crash event. + +use sentry_minidump::MinidumpIntegration; + +fn main() { + let minidump = MinidumpIntegration::new() + .crashes_dir(std::env::temp_dir().join("sentry-minidump-example")); + + let _guard = sentry::init(sentry::ClientOptions::new().add_integration(minidump)); + + sentry::with_integration(|minidump: &MinidumpIntegration, _| { + #[expect(deprecated)] + minidump.set_tag("deprecated".into(), Some("set_tag".into())); + }); + + unsafe { sadness_generator::raise_segfault() }; +} diff --git a/sentry-minidump/src/lib.rs b/sentry-minidump/src/lib.rs index ad5e1747..b03ff927 100644 --- a/sentry-minidump/src/lib.rs +++ b/sentry-minidump/src/lib.rs @@ -65,7 +65,7 @@ use std::sync::{Arc, Once, OnceLock}; use std::time::Duration; use minidumper_child::{ClientHandle, MinidumperChild}; -use sentry_core::protocol::{Attachment, AttachmentType, Value}; +use sentry_core::protocol::{Attachment, AttachmentType, Breadcrumb, User, Value}; use sentry_core::{sentry_debug, Client, ClientOptions, Hub, Integration, Level, Scope}; use crate::scope_sync::{ScopeReceiver, ScopeSync}; @@ -82,9 +82,41 @@ const DEFAULT_PROCESS_NAME: &str = "Crash Reporter (Sentry Rust SDK)"; /// The default time the crash handler waits for the scope to be serialized. const DEFAULT_SCOPE_TIMEOUT: Duration = Duration::from_secs(2); +/// Message kind of a [`ScopeUpdate`] on the socket to the reporter. +const MSG_SCOPE_UPDATE: u32 = 0; + type OnProcess = dyn Fn(&mut Command) + Send + Sync + 'static; type BeforeCapture = dyn Fn(&mut Scope, &Path) + Send + Sync + 'static; +/// An update to the crash reporter's scope, sent over the socket by the +/// deprecated scope methods. +#[derive(serde::Deserialize, serde::Serialize, Debug, Clone, PartialEq)] +enum ScopeUpdate { + AddBreadcrumb(Breadcrumb), + SetUser(Option), + SetExtra(String, Option), + SetTag(String, Option), +} + +impl ScopeUpdate { + /// Applies the update to the reporter's current hub. + fn apply(self) { + let hub = Hub::current(); + match self { + ScopeUpdate::AddBreadcrumb(b) => hub.add_breadcrumb(b), + ScopeUpdate::SetUser(u) => hub.configure_scope(|scope| scope.set_user(u)), + ScopeUpdate::SetExtra(k, v) => hub.configure_scope(|scope| match v { + Some(v) => scope.set_extra(&k, v), + None => scope.remove_extra(&k), + }), + ScopeUpdate::SetTag(k, v) => hub.configure_scope(|scope| match v { + Some(v) => scope.set_tag(&k, &v), + None => scope.remove_tag(&k), + }), + } + } +} + /// Captures native crashes as minidumps and sends them to Sentry. /// /// Build it with [`MinidumpIntegration::new`] and the builder methods, then @@ -271,6 +303,42 @@ impl MinidumpIntegration { self } + /// Sends a scope update to the crash reporter process. + /// + /// Does nothing when there is no crash reporter, because this is the + /// reporter process, the DSN was unset, or the spawn failed. + fn send(&self, update: &ScopeUpdate) { + if let Some(handle) = self.handle.get() { + if let Ok(buffer) = serde_json::to_vec(update) { + handle.send_message(MSG_SCOPE_UPDATE, buffer).ok(); + } + } + } + + /// Adds a breadcrumb to the crash reporter's scope. + #[deprecated = "The crash event now carries the scope of the crashing thread. Use `sentry::add_breadcrumb` instead."] + pub fn add_breadcrumb(&self, breadcrumb: Breadcrumb) { + self.send(&ScopeUpdate::AddBreadcrumb(breadcrumb)); + } + + /// Sets the user on the crash reporter's scope. + #[deprecated = "The crash event now carries the scope of the crashing thread. Use `sentry::configure_scope` instead."] + pub fn set_user(&self, user: Option) { + self.send(&ScopeUpdate::SetUser(user)); + } + + /// Sets or removes an extra value on the crash reporter's scope. + #[deprecated = "The crash event now carries the scope of the crashing thread. Use `sentry::configure_scope` instead."] + pub fn set_extra(&self, key: String, value: Option) { + self.send(&ScopeUpdate::SetExtra(key, value)); + } + + /// Sets or removes a tag on the crash reporter's scope. + #[deprecated = "The crash event now carries the scope of the crashing thread. Use `sentry::configure_scope` instead."] + pub fn set_tag(&self, key: String, value: Option) { + self.send(&ScopeUpdate::SetTag(key, value)); + } + /// Builds the child from the stored settings. /// /// `scope_sync` is set in the app process only; the reporter has no @@ -318,7 +386,14 @@ impl MinidumpIntegration { let receiver = Arc::new(ScopeReceiver::default()); child = child.on_message({ let receiver = receiver.clone(); - move |kind, buffer| receiver.on_message(kind, &buffer) + move |kind, buffer| match kind { + MSG_SCOPE_UPDATE => { + if let Ok(update) = serde_json::from_slice::(&buffer) { + update.apply(); + } + } + _ => receiver.on_message(kind, &buffer), + } }); let flush_timeout = self.flush_timeout; diff --git a/sentry-minidump/tests/e2e.rs b/sentry-minidump/tests/e2e.rs index 7cbd980e..855d758c 100644 --- a/sentry-minidump/tests/e2e.rs +++ b/sentry-minidump/tests/e2e.rs @@ -12,7 +12,7 @@ use std::sync::mpsc; use std::thread; use std::time::{Duration, Instant}; -use sentry::protocol::{AttachmentType, EnvelopeItem}; +use sentry::protocol::{AttachmentType, EnvelopeItem, Event}; use sentry::{Envelope, Level}; /// Reads one HTTP request and returns its body. @@ -48,8 +48,13 @@ fn read_request_body(stream: &mut impl Read) -> Vec { buf[header_end..body_end].to_vec() } -#[test] -fn captures_minidump_from_crash() { +/// Runs an example that crashes on purpose and returns the envelope its +/// crash reporter uploads. +/// +/// The example has to be a separate binary because the crash reporter +/// re-executes it and the crash event is sent from that second process, +/// not the test process. +fn crash_envelope(example: &str) -> Envelope { let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind listener"); let port = listener .local_addr() @@ -65,11 +70,8 @@ fn captures_minidump_from_crash() { let _ = tx.send(body); }); - // Runs `examples/app.rs`, which crashes on purpose. It has to be a - // separate binary because the crash reporter re-executes it and the - // crash event is sent from that second process, not the test process. let example_process = Command::new(env!("CARGO")) - .args(["run", "--quiet", "--example", "minidump"]) + .args(["run", "--quiet", "--example", example]) .current_dir(env!("CARGO_MANIFEST_DIR")) .env("SENTRY_DSN", format!("http://dsn@127.0.0.1:{port}/0")) .spawn() @@ -83,15 +85,24 @@ fn captures_minidump_from_crash() { .recv_timeout(Duration::from_secs(5)) .expect("received an envelope"); - let envelope = Envelope::from_slice(&body).expect("parse envelope"); + Envelope::from_slice(&body).expect("parse envelope") +} - let event = envelope +fn crash_event(envelope: &Envelope) -> &Event<'static> { + envelope .items() .find_map(|item| match item { EnvelopeItem::Event(event) => Some(event), _ => None, }) - .expect("envelope has an event"); + .expect("envelope has an event") +} + +#[test] +fn captures_minidump_from_crash() { + // Runs `examples/app.rs`. + let envelope = crash_envelope("minidump"); + let event = crash_event(&envelope); assert_eq!(event.level, Level::Fatal); @@ -158,3 +169,16 @@ fn wait_for_timeout(mut process: Child, timeout: Duration) -> ExitStatus { process.kill().expect("error while killing process"); panic!("Process did not exit within timeout."); } + +#[test] +fn deprecated_scope_methods_reach_crash_event() { + // Runs `examples/deprecated.rs`. + let envelope = crash_envelope("minidump-deprecated"); + let event = crash_event(&envelope); + + assert_eq!(event.level, Level::Fatal); + assert_eq!( + event.tags.get("deprecated").map(String::as_str), + Some("set_tag") + ); +} From 45f33901e1d978c052ff84d560c244f30650c764 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 13 Oct 2026 13:01:12 +0100 Subject: [PATCH 11/12] feat(minidump): Make per-thread scope opt-in with minidump-scope Address review feedback and make the thread registry opt-in, since it adds a few nanoseconds to every hub switch. By default the crash event carries the main hub's scope. --- .github/workflows/test.yml | 5 ++ CHANGELOG.md | 6 +- Cargo.lock | 1 + sentry-core/README.md | 6 ++ sentry-core/benches/scope_benchmark.rs | 27 ++++++ sentry-core/src/hub_impl.rs | 41 ++++----- sentry-core/src/lib.rs | 16 ++-- sentry-core/src/thread_registry.rs | 109 +++++++++--------------- sentry-minidump/Cargo.toml | 13 ++- sentry-minidump/README.md | 22 +++-- sentry-minidump/examples/large_scope.rs | 19 +++++ sentry-minidump/src/lib.rs | 27 +++--- sentry-minidump/src/scope_sync.rs | 36 ++++---- sentry-minidump/tests/e2e.rs | 83 +++++++++++++----- sentry/Cargo.toml | 1 + sentry/README.md | 3 + sentry/src/lib.rs | 3 + 17 files changed, 255 insertions(+), 163 deletions(-) create mode 100644 sentry-minidump/examples/large_scope.rs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c2793d90..f4502115 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -31,6 +31,11 @@ jobs: - name: Run cargo test run: cargo test --workspace --all-features --all-targets --locked + # `--all-features` turns on `scope`, so also test the default mode, + # which sends the main hub's scope. + - name: Test sentry-minidump default features + run: cargo test -p sentry-minidump --locked + - name: Run cargo doc tests run: cargo test --workspace --all-features --doc --locked diff --git a/CHANGELOG.md b/CHANGELOG.md index ea15f845..cc314084 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +4,12 @@ ### Features -- The `sentry-minidump` crash event now carries the scope of the thread that crashed, including hubs bound with `Hub::run`. Nothing is sent to the crash reporter until the crash: the crash handler names the crashing OS thread and a helper thread serializes that thread's scope before the minidump is requested. The new `scope_timeout` option bounds the wait for the scope ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). -- Added the `thread-registry` feature to `sentry-core`, with `Hub::for_os_thread` and `current_os_thread_id`, which map operating system thread ids to the hub current on each thread, and `Hub::scope_snapshot`, which copies a hub's scope without taking its write lock ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +- The `sentry-minidump` crash event now carries the main hub's scope. Nothing is sent to the crash reporter until the crash: a helper thread serializes the scope before the minidump is requested. The new `scope_timeout` option bounds the wait for the scope. With the new `minidump-scope` feature, the crash event carries the scope of the thread that crashed instead, including hubs bound with `Hub::run`. This adds a few nanoseconds to each hub switch ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +- Added `Hub::scope_for_os_thread` to `sentry-core`, which returns the scope of the hub current on the thread with a given operating system thread id. It tracks threads only with the new `thread-registry` feature, which `minidump-scope` turns on, and otherwise returns the main hub's scope ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). ### Deprecations -- Deprecated the `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods on `MinidumpIntegration`. The crash event now carries the scope of the crashing thread, so set these values with `sentry::configure_scope` and `sentry::add_breadcrumb` instead ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). +- Deprecated the `set_user`, `set_tag`, `set_extra` and `add_breadcrumb` methods on `MinidumpIntegration`. The crash event now carries the main hub's scope, or with `minidump-scope` the crashing thread's scope, so set these values with `sentry::configure_scope` and `sentry::add_breadcrumb` instead ([#1340](https://github.com/getsentry/sentry-rust/pull/1340)). ## 0.49.3 diff --git a/Cargo.lock b/Cargo.lock index 26a69039..1b972e97 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3710,6 +3710,7 @@ dependencies = [ name = "sentry-minidump" version = "0.49.3" dependencies = [ + "libc", "mach2", "minidumper-child", "sadness-generator", diff --git a/sentry-core/README.md b/sentry-core/README.md index b294ba5b..051bfc3b 100644 --- a/sentry-core/README.md +++ b/sentry-core/README.md @@ -95,6 +95,12 @@ functionality. - `feature = "test"`: Activates the [`test`] module, which can be used to write integration tests. It comes with a test transport which can capture all sent events for inspection. +- `feature = "thread-registry"`: Tracks the hub current on each thread, + so a crash reporter can get the crashing thread's scope with + `Hub::scope_for_os_thread`. Every hub switch also updates the + registry. In the `hub-switch` benchmark this takes `Hub::run` from + about 3.5 ns to about 11 ns. The `minidump-scope` feature of `sentry` + turns this on. [Sentry]: https://sentry.io/ [`sentry`]: https://crates.io/crates/sentry diff --git a/sentry-core/benches/scope_benchmark.rs b/sentry-core/benches/scope_benchmark.rs index 51fdf372..bda7dc51 100644 --- a/sentry-core/benches/scope_benchmark.rs +++ b/sentry-core/benches/scope_benchmark.rs @@ -14,6 +14,14 @@ //! We have the following tests: //! * [`scope_with_breadcrumbs`] //! * [`scope_with_tags`] +//! * Switching hubs with [`sentry::Hub::run`], and the first use of a hub on +//! a new thread. The `thread-registry` feature adds work to both, so +//! compare runs with and without it: +//! +//! ```text +//! $ cargo bench -p sentry-core --bench scope_benchmark -- hub-switch +//! $ cargo bench -p sentry-core --bench scope_benchmark --features thread-registry -- hub-switch +//! ``` //! //! We do test the following permutations: //! * No active Hub is bound, meaning most API functions will just noop @@ -192,6 +200,25 @@ fn scope_benchmark(c: &mut Criterion) { } group.finish(); + + #[cfg(feature = "client")] + { + let mut group = c.benchmark_group("hub-switch"); + + group.bench_function("run", |b| { + let hub = Arc::new(discarding_hub()); + b.iter(|| sentry::Hub::run(hub.clone(), || ())) + }); + group.bench_function("new-thread", |b| { + b.iter(|| { + std::thread::spawn(|| drop(sentry::Hub::current())) + .join() + .unwrap() + }) + }); + + group.finish(); + } } criterion_group!(benches, scope_benchmark); diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 2218c3e3..32f2ddc5 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -173,29 +173,30 @@ impl Hub { PROCESS_HUB.hub.clone() } - /// Returns the hub that is current on the thread with the given - /// operating system thread id. + /// Returns the scope of the hub that is current on the thread with the + /// given operating system thread id. /// - /// The id is the one [`current_os_thread_id`](crate::current_os_thread_id) - /// returns on that thread. A thread is known once it has used - /// [`Hub::current`] or [`Hub::run`]; hubs switched with [`Hub::run`] - /// are tracked. Returns `None` for threads that never touched a hub - /// or have exited. + /// The id is the one a crash handler reports for the crashing thread: + /// the kernel thread id on Linux and Android, `GetCurrentThreadId` on + /// Windows, and the Mach port name on macOS. /// - /// This is meant for crash reporters that learn the crashing thread - /// from the OS and want its scope. - #[cfg(feature = "thread-registry")] - pub fn for_os_thread(os_thread_id: u64) -> Option> { - crate::thread_registry::hub_for_os_thread(os_thread_id) - } - - /// Returns a copy of the hub's current scope. + /// Threads are only tracked with the `thread-registry` feature, and + /// then hubs bound with [`Hub::run`] are tracked too. Without it, and + /// for a thread that never used a hub or has exited, this returns the + /// main hub's scope. That is the scope such a thread inherits when it + /// first uses [`Hub::current`]. /// - /// Unlike [`Hub::configure_scope`], this only takes the hub's read - /// lock, so it does not wait for other threads that read the scope. - #[cfg(feature = "thread-registry")] - pub fn scope_snapshot(&self) -> Scope { - self.with_current_scope(Scope::clone) + /// This only takes the hub's read lock, so it does not wait for other + /// threads that read the scope. + pub fn scope_for_os_thread(os_thread_id: u64) -> Arc { + #[cfg(feature = "thread-registry")] + let hub = crate::thread_registry::hub_for_os_thread(os_thread_id).unwrap_or_else(Hub::main); + #[cfg(not(feature = "thread-registry"))] + let hub = { + let _ = os_thread_id; + Hub::main() + }; + hub.inner.with(|stack| stack.top().scope.clone()) } /// Invokes the callback with the default hub. diff --git a/sentry-core/src/lib.rs b/sentry-core/src/lib.rs index 43391a49..89d63ddf 100644 --- a/sentry-core/src/lib.rs +++ b/sentry-core/src/lib.rs @@ -93,6 +93,12 @@ //! - `feature = "test"`: Activates the [`test`] module, which can be used to //! write integration tests. It comes with a test transport which can capture //! all sent events for inspection. +//! - `feature = "thread-registry"`: Tracks the hub current on each thread, +//! so a crash reporter can get the crashing thread's scope with +//! [`Hub::scope_for_os_thread`]. Every hub switch also updates the +//! registry. In the `hub-switch` benchmark this takes [`Hub::run`] from +//! about 3.5 ns to about 11 ns. The `minidump-scope` feature of `sentry` +//! turns this on. //! //! [Sentry]: https://sentry.io/ //! [`sentry`]: https://crates.io/crates/sentry @@ -148,16 +154,6 @@ mod hub_impl; mod session; #[cfg(feature = "thread-registry")] mod thread_registry; -#[cfg(all( - feature = "thread-registry", - any( - target_os = "linux", - target_os = "android", - target_os = "macos", - windows - ) -))] -pub use crate::thread_registry::current_os_thread_id; #[cfg(feature = "client")] pub use crate::clientoptions::MaxRequestBodySize; diff --git a/sentry-core/src/thread_registry.rs b/sentry-core/src/thread_registry.rs index f1a65677..6f49f390 100644 --- a/sentry-core/src/thread_registry.rs +++ b/sentry-core/src/thread_registry.rs @@ -18,19 +18,14 @@ use crate::Hub; /// - Linux and Android: the kernel thread id from `gettid`. /// - Windows: `GetCurrentThreadId`. /// - macOS: the Mach port name of the thread, as reported in exception -/// messages and returned by `pthread_mach_thread_np`. +/// messages. `sentry-minidump` expects exactly the value of +/// `pthread_mach_thread_np(pthread_self())`. /// -/// Returns `None` when the OS refuses the call, for example when a -/// seccomp filter blocks `gettid`. +/// Returns `None` on other targets, and when the OS refuses the call, for +/// example when a seccomp filter blocks `gettid`. /// /// It is not related to [`std::thread::ThreadId`]. -#[cfg(any( - target_os = "linux", - target_os = "android", - target_os = "macos", - windows -))] -pub fn current_os_thread_id() -> Option { +fn current_os_thread_id() -> Option { #[cfg(any(target_os = "linux", target_os = "android"))] { // SAFETY: `gettid` takes no arguments. @@ -49,27 +44,6 @@ pub fn current_os_thread_id() -> Option { // SAFETY: takes no arguments and cannot fail. Some(unsafe { GetCurrentThreadId() }.into()) } -} - -/// Turns the result of the `gettid` syscall into a thread id. -/// -/// A seccomp filter can make the syscall return -1 without running it. -#[cfg(any(target_os = "linux", target_os = "android"))] -fn thread_id_from_syscall(ret: libc::c_long) -> Option { - u64::try_from(ret).ok().filter(|&id| id > 0) -} - -/// The id to register the current thread under, if the OS reports one. -fn registry_id() -> Option { - #[cfg(any( - target_os = "linux", - target_os = "android", - target_os = "macos", - windows - ))] - { - current_os_thread_id() - } #[cfg(not(any( target_os = "linux", target_os = "android", @@ -81,10 +55,17 @@ fn registry_id() -> Option { } } +/// Turns the result of the `gettid` syscall into a thread id. +/// +/// A seccomp filter can make the syscall return -1 without running it. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn thread_id_from_syscall(ret: libc::c_long) -> Option { + u64::try_from(ret).ok().filter(|&id| id > 0) +} + /// One thread's entry in the registry. struct Slot { - /// `None` when the thread has no OS id and is not in the registry. - os_thread_id: Option, + os_thread_id: u64, hub: RwLock>, } @@ -95,40 +76,36 @@ static REGISTRY: LazyLock>>> = struct SlotGuard(Arc); impl SlotGuard { - fn register(os_thread_id: Option) -> Self { + fn register(os_thread_id: u64) -> Self { let slot = Arc::new(Slot { os_thread_id, hub: RwLock::new(Weak::new()), }); - if let Some(id) = os_thread_id { - REGISTRY - .lock() - .unwrap_or_else(PoisonError::into_inner) - .insert(id, slot.clone()); - } + REGISTRY + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(os_thread_id, slot.clone()); SlotGuard(slot) } } impl Drop for SlotGuard { fn drop(&mut self) { - let Some(id) = self.0.os_thread_id else { - return; - }; let mut registry = REGISTRY.lock().unwrap_or_else(PoisonError::into_inner); // The OS can reuse the id for a new thread before this one is // fully gone, so only remove the entry if it is still ours. if registry - .get(&id) + .get(&self.0.os_thread_id) .is_some_and(|slot| Arc::ptr_eq(slot, &self.0)) { - registry.remove(&id); + registry.remove(&self.0.os_thread_id); } } } thread_local! { - static SLOT: SlotGuard = SlotGuard::register(registry_id()); + /// `None` when the thread has no OS id, so it is not registered. + static SLOT: Option = current_os_thread_id().map(SlotGuard::register); } /// Records `hub` as the hub current on the calling thread. @@ -136,7 +113,9 @@ thread_local! { /// Does nothing while the thread is shutting down and its slot is gone. pub(crate) fn set_current_hub(hub: &Arc) { let _ = SLOT.try_with(|slot| { - *slot.0.hub.write().unwrap_or_else(PoisonError::into_inner) = Arc::downgrade(hub); + if let Some(slot) = slot { + *slot.0.hub.write().unwrap_or_else(PoisonError::into_inner) = Arc::downgrade(hub); + } }); } @@ -158,6 +137,9 @@ mod tests { #[test] fn registers_current_hub_per_thread() { + // The first thread to use a hub creates the main hub. Do that here, + // so the worker gets a hub of its own. + Hub::main(); let (id_tx, id_rx) = mpsc::channel(); let (done_tx, done_rx) = mpsc::channel::<()>(); @@ -169,13 +151,13 @@ mod tests { }); let (id, hub) = id_rx.recv().unwrap(); - let found = Hub::for_os_thread(id).expect("worker thread is registered"); + let found = hub_for_os_thread(id).expect("worker thread is registered"); assert!(Arc::ptr_eq(&found, &hub)); assert!(!Arc::ptr_eq(&found, &Hub::main())); done_tx.send(()).unwrap(); worker.join().unwrap(); - assert!(Hub::for_os_thread(id).is_none(), "slot is removed on exit"); + assert!(hub_for_os_thread(id).is_none(), "slot is removed on exit"); } #[test] @@ -185,27 +167,17 @@ mod tests { let inner = Arc::new(Hub::new_from_top(&outer)); Hub::run(inner.clone(), || { - let found = Hub::for_os_thread(id).unwrap(); + let found = hub_for_os_thread(id).unwrap(); assert!(Arc::ptr_eq(&found, &inner)); }); - let found = Hub::for_os_thread(id).unwrap(); + let found = hub_for_os_thread(id).unwrap(); assert!(Arc::ptr_eq(&found, &outer)); } #[test] fn unknown_thread_has_no_hub() { - assert!(Hub::for_os_thread(u64::MAX).is_none()); - } - - #[test] - fn thread_without_id_is_not_registered() { - let guard = SlotGuard::register(None); - assert!(!REGISTRY - .lock() - .unwrap() - .values() - .any(|slot| Arc::ptr_eq(slot, &guard.0))); + assert!(hub_for_os_thread(u64::MAX).is_none()); } #[cfg(any(target_os = "linux", target_os = "android"))] @@ -217,13 +189,16 @@ mod tests { } #[test] - fn scope_snapshot_works_while_the_scope_is_read() { - let hub = Hub::new(None, Default::default()); + fn scope_for_os_thread_uses_the_thread_hub() { + let hub = Arc::new(Hub::new(None, Default::default())); hub.configure_scope(|scope| scope.set_tag("thread", "worker")); - // A write lock here would never be granted. - let snapshot = hub.with_current_scope(|_| hub.scope_snapshot()); - let event = snapshot + let scope = Hub::run(hub.clone(), || { + let id = current_os_thread_id().unwrap(); + // A write lock here would never be granted. + hub.with_current_scope(|_| Hub::scope_for_os_thread(id)) + }); + let event = scope .apply_to_event(Default::default()) .expect("no event processors drop the event"); assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); diff --git a/sentry-minidump/Cargo.toml b/sentry-minidump/Cargo.toml index d5dcb8ea..b50d19c1 100644 --- a/sentry-minidump/Cargo.toml +++ b/sentry-minidump/Cargo.toml @@ -15,6 +15,12 @@ rust-version = { workspace = true } [lints] workspace = true +[features] +default = [] +# Sends the scope of the crashing thread instead of the main hub's scope. +# Every hub switch then also updates a registry of threads. +scope = ["sentry-core/thread-registry"] + [[example]] name = "minidump" path = "examples/app.rs" @@ -23,13 +29,18 @@ path = "examples/app.rs" name = "minidump-deprecated" path = "examples/deprecated.rs" +[[example]] +name = "minidump-large-scope" +path = "examples/large_scope.rs" + [dependencies] -sentry-core = { workspace = true, features = ["client", "thread-registry"] } +sentry-core = { workspace = true, features = ["client"] } minidumper-child = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } [target.'cfg(target_os = "macos")'.dependencies] +libc = { workspace = true } mach2 = { workspace = true } [dev-dependencies] diff --git a/sentry-minidump/README.md b/sentry-minidump/README.md index 29673233..483641db 100644 --- a/sentry-minidump/README.md +++ b/sentry-minidump/README.md @@ -41,15 +41,19 @@ calls do nothing. ## Scope -The crash event carries the scope of the thread that crashed: user, -tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, -after the scope's event processors have run. Nothing is sent to the -crash reporter until the crash. At that moment the crash handler names -the crashing OS thread, a helper thread serializes the scope of the hub -current on that thread, and the handler sends it to the reporter before -it requests the minidump. Hubs bound with `Hub::run` are tracked, so a -server with one hub per request reports the scope of the request that -crashed. A thread that never used Sentry falls back to the main hub. +The crash event carries the main hub's scope: user, tags, extra, +contexts, breadcrumbs, transaction and fingerprint, after the scope's +event processors have run. Nothing is sent to the crash reporter until +the crash. At that moment a helper thread serializes the scope, and the +crash handler sends it to the reporter before it requests the minidump. + +With the `scope` feature (`minidump-scope` on `sentry`), the crash +event carries the scope of the hub current on the thread that crashed +instead. Hubs bound with `Hub::run` are tracked, so a server with one +hub per request reports the scope of the request that crashed. A thread +that never used Sentry falls back to the main hub. To track hubs, every +hub switch also updates a registry of threads, which adds a few +nanoseconds to each switch. The helper waits at most `scope_timeout` for the scope. If the crash happened while the crashing thread held the allocator or the hub lock, diff --git a/sentry-minidump/examples/large_scope.rs b/sentry-minidump/examples/large_scope.rs new file mode 100644 index 00000000..d9668641 --- /dev/null +++ b/sentry-minidump/examples/large_scope.rs @@ -0,0 +1,19 @@ +//! Crashes with a scope much larger than one scope message. +//! +//! The end to end test runs this to check that the scope arrives whole +//! on every platform. + +use sentry_minidump::MinidumpIntegration; + +fn main() { + let minidump = MinidumpIntegration::new() + .crashes_dir(std::env::temp_dir().join("sentry-minidump-example")); + + let _guard = sentry::init(sentry::ClientOptions::new().add_integration(minidump)); + + sentry::configure_scope(|scope| { + scope.set_extra("large", "x".repeat(32 * 1024).into()); + }); + + unsafe { sadness_generator::raise_segfault() }; +} diff --git a/sentry-minidump/src/lib.rs b/sentry-minidump/src/lib.rs index b03ff927..8438daef 100644 --- a/sentry-minidump/src/lib.rs +++ b/sentry-minidump/src/lib.rs @@ -33,15 +33,19 @@ //! //! # Scope //! -//! The crash event carries the scope of the thread that crashed: user, -//! tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, -//! after the scope's event processors have run. Nothing is sent to the -//! crash reporter until the crash. At that moment the crash handler names -//! the crashing OS thread, a helper thread serializes the scope of the hub -//! current on that thread, and the handler sends it to the reporter before -//! it requests the minidump. Hubs bound with [`Hub::run`] are tracked, so a -//! server with one hub per request reports the scope of the request that -//! crashed. A thread that never used Sentry falls back to the main hub. +//! The crash event carries the main hub's scope: user, tags, extra, +//! contexts, breadcrumbs, transaction and fingerprint, after the scope's +//! event processors have run. Nothing is sent to the crash reporter until +//! the crash. At that moment a helper thread serializes the scope, and the +//! crash handler sends it to the reporter before it requests the minidump. +//! +//! With the `scope` feature (`minidump-scope` on `sentry`), the crash +//! event carries the scope of the hub current on the thread that crashed +//! instead. Hubs bound with [`Hub::run`] are tracked, so a server with one +//! hub per request reports the scope of the request that crashed. A thread +//! that never used Sentry falls back to the main hub. To track hubs, every +//! hub switch also updates a registry of threads, which adds a few +//! nanoseconds to each switch. //! //! The helper waits at most [`scope_timeout`](MinidumpIntegration::scope_timeout) //! for the scope. If the crash happened while the crashing thread held the @@ -68,7 +72,7 @@ use minidumper_child::{ClientHandle, MinidumperChild}; use sentry_core::protocol::{Attachment, AttachmentType, Breadcrumb, User, Value}; use sentry_core::{sentry_debug, Client, ClientOptions, Hub, Integration, Level, Scope}; -use crate::scope_sync::{ScopeReceiver, ScopeSync}; +use crate::scope_sync::{ScopeReceiver, ScopeSync, MSG_SCOPE_UPDATE}; /// The default environment variable that marks the crash reporter process. const DEFAULT_SERVER_ENV_VAR: &str = "_SENTRY_CRASH_REPORTER_SERVER"; @@ -82,9 +86,6 @@ const DEFAULT_PROCESS_NAME: &str = "Crash Reporter (Sentry Rust SDK)"; /// The default time the crash handler waits for the scope to be serialized. const DEFAULT_SCOPE_TIMEOUT: Duration = Duration::from_secs(2); -/// Message kind of a [`ScopeUpdate`] on the socket to the reporter. -const MSG_SCOPE_UPDATE: u32 = 0; - type OnProcess = dyn Fn(&mut Command) + Send + Sync + 'static; type BeforeCapture = dyn Fn(&mut Scope, &Path) + Send + Sync + 'static; diff --git a/sentry-minidump/src/scope_sync.rs b/sentry-minidump/src/scope_sync.rs index 5283cde6..d1351401 100644 --- a/sentry-minidump/src/scope_sync.rs +++ b/sentry-minidump/src/scope_sync.rs @@ -19,12 +19,17 @@ use sentry_core::protocol::Event; use sentry_core::Hub; /// Message kinds on the socket between the app and the reporter. +/// +/// `MSG_SCOPE_UPDATE` carries a `ScopeUpdate` from the deprecated scope +/// methods on `MinidumpIntegration`. +pub(crate) const MSG_SCOPE_UPDATE: u32 = 0; pub(crate) const MSG_SCOPE_CHUNK: u32 = 1; pub(crate) const MSG_SCOPE_END: u32 = 2; -/// The reporter reads one message in one call and does not reassemble -/// partial reads on every platform, so keep each message small. -const CHUNK_SIZE: usize = 16 * 1024; +/// The reporter reads each message with one `recv`. A local socket on +/// macOS buffers at most 8 KiB, so a larger message never arrives. Keep +/// each message, with its header, well below that. +const CHUNK_SIZE: usize = 4 * 1024; /// Hand-off between the crash handler and the helper thread that /// serializes the scope. @@ -74,8 +79,10 @@ impl ScopeSync { let sync = helper_sync; #[cfg(target_os = "macos")] { - // The Mach port of the current thread is a `u32`. - let port = sentry_core::current_os_thread_id().map_or(0, |id| id as u32); + // SAFETY: `pthread_self` is always valid for the + // calling thread. + #[expect(unsafe_code, reason = "reads the helper thread's Mach port")] + let port = unsafe { libc::pthread_mach_thread_np(libc::pthread_self()) }; sync.helper_port.store(port, Ordering::Release); helper_port_stored.wait(); } @@ -155,8 +162,8 @@ fn send_chunks(sender: &MessageSender<'_>, bytes: &[u8]) { sender.send_message(MSG_SCOPE_END, &[]).ok(); } -/// The id of the crashing thread as [`sentry_core::current_os_thread_id`] -/// reports it on that thread. +/// The id of the crashing thread, which is the id `sentry-core` registers +/// that thread under. fn crashing_thread_id(crash_context: &CrashContext) -> u64 { #[cfg(any(target_os = "linux", target_os = "android"))] { @@ -177,8 +184,7 @@ fn crashing_thread_id(crash_context: &CrashContext) -> u64 { /// Falls back to the main hub for threads that never used Sentry, which /// is the scope such a thread would have inherited. fn scope_bytes(thread_id: u64) -> Vec { - let hub = Hub::for_os_thread(thread_id).unwrap_or_else(Hub::main); - hub.scope_snapshot() + Hub::scope_for_os_thread(thread_id) .apply_to_event(Event::default()) .and_then(|event| serde_json::to_vec(&event).ok()) .unwrap_or_default() @@ -251,13 +257,9 @@ mod tests { } #[test] - fn scope_bytes_uses_the_thread_hub() { - let hub = Arc::new(Hub::new(None, Default::default())); - hub.configure_scope(|scope| scope.set_tag("thread", "worker")); - let bytes = Hub::run(hub, || { - scope_bytes(sentry_core::current_os_thread_id().unwrap()) - }); - let event: Event<'static> = serde_json::from_slice(&bytes).unwrap(); - assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); + fn scope_bytes_falls_back_to_the_main_hub() { + Hub::main().configure_scope(|scope| scope.set_tag("hub", "main")); + let event: Event<'static> = serde_json::from_slice(&scope_bytes(u64::MAX)).unwrap(); + assert_eq!(event.tags.get("hub").map(String::as_str), Some("main")); } } diff --git a/sentry-minidump/tests/e2e.rs b/sentry-minidump/tests/e2e.rs index 855d758c..55d6e307 100644 --- a/sentry-minidump/tests/e2e.rs +++ b/sentry-minidump/tests/e2e.rs @@ -70,8 +70,12 @@ fn crash_envelope(example: &str) -> Envelope { let _ = tx.send(body); }); + let mut args = vec!["run", "--quiet", "--example", example]; + if cfg!(feature = "scope") { + args.extend(["--features", "scope"]); + } let example_process = Command::new(env!("CARGO")) - .args(["run", "--quiet", "--example", example]) + .args(args) .current_dir(env!("CARGO_MANIFEST_DIR")) .env("SENTRY_DSN", format!("http://dsn@127.0.0.1:{port}/0")) .spawn() @@ -112,28 +116,47 @@ fn captures_minidump_from_crash() { Some("example") ); - // The scope of the worker thread that crashed, not the main thread's. - let user = event.user.as_ref().expect("event has a user"); - assert_eq!(user.username.as_deref(), Some("john_doe")); - assert_eq!(user.email.as_deref(), Some("john@doe.town")); - assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); - assert_eq!( - event.breadcrumbs.last().and_then(|b| b.message.as_deref()), - Some("about to crash") - ); - - // Inherited from the main scope when the worker hub was created. - assert_eq!( - event.tags.get("shared").map(String::as_str), - Some("from_main") - ); - - // Set on the main thread after the worker hub was created. - assert!( - !event.tags.contains_key("main_only"), - "main thread scope leaked into the crash event: {:?}", - event.tags - ); + if cfg!(feature = "scope") { + // The scope of the worker thread that crashed, not the main thread's. + let user = event.user.as_ref().expect("event has a user"); + assert_eq!(user.username.as_deref(), Some("john_doe")); + assert_eq!(user.email.as_deref(), Some("john@doe.town")); + assert_eq!(event.tags.get("thread").map(String::as_str), Some("worker")); + assert_eq!( + event.breadcrumbs.last().and_then(|b| b.message.as_deref()), + Some("about to crash") + ); + + // Inherited from the main scope when the worker hub was created. + assert_eq!( + event.tags.get("shared").map(String::as_str), + Some("from_main") + ); + + // Set on the main thread after the worker hub was created. + assert!( + !event.tags.contains_key("main_only"), + "main thread scope leaked into the crash event: {:?}", + event.tags + ); + } else { + // The main hub's scope, not the worker hub's. + let user = event.user.as_ref().expect("event has a user"); + assert_eq!(user.username.as_deref(), Some("main_thread")); + assert_eq!( + event.tags.get("shared").map(String::as_str), + Some("from_main") + ); + assert_eq!( + event.tags.get("main_only").map(String::as_str), + Some("true") + ); + assert!( + !event.tags.contains_key("thread"), + "worker scope reached the crash event: {:?}", + event.tags + ); + } let attachment = envelope .items() @@ -182,3 +205,17 @@ fn deprecated_scope_methods_reach_crash_event() { Some("set_tag") ); } + +#[test] +fn large_scope_reaches_crash_event() { + // Runs `examples/large_scope.rs`. + let envelope = crash_envelope("minidump-large-scope"); + let event = crash_event(&envelope); + + let large = event + .extra + .get("large") + .and_then(|value| value.as_str()) + .expect("event has the large extra"); + assert_eq!(large.len(), 32 * 1024); +} diff --git a/sentry/Cargo.toml b/sentry/Cargo.toml index 6ef72738..4219a437 100644 --- a/sentry/Cargo.toml +++ b/sentry/Cargo.toml @@ -42,6 +42,7 @@ actix = ["sentry-actix"] debug-images = ["sentry-debug-images"] log = ["sentry-log"] minidump = ["dep:sentry-minidump"] +minidump-scope = ["minidump", "sentry-minidump?/scope"] slog = ["sentry-slog"] tower = ["sentry-tower"] tower-http = ["tower", "sentry-tower/http"] diff --git a/sentry/README.md b/sentry/README.md index bb06d728..fb88584e 100644 --- a/sentry/README.md +++ b/sentry/README.md @@ -79,6 +79,7 @@ extra setup to function properly. | `debug-images` | ✅ | 🔌 | | | | `log` | | 🔌 | | Requires extra setup; See [`sentry-log`]'s documentation. | | `minidump` | | 🔌 | | Linux, macOS and Windows only. See [`sentry-minidump`]'s documentation. | +| `minidump-scope` | | 🔌 | | Sends the crashing thread's scope. Adds a few ns to each hub switch. | | `slog` | | 🔌 | | Requires extra setup; See [`sentry-slog`]'s documentation. | | `reqwest` | ✅ | | | | | `native-tls` | ✅ | | | `reqwest` must be enabled. | @@ -133,6 +134,8 @@ extra setup to function properly. ### Integrations - `actix`: Enables support for the `actix-web` crate. - `minidump`: Captures native crashes as minidumps. Linux, macOS and Windows only. +- `minidump-scope`: Sends the scope of the crashing thread with the minidump, instead of the + main hub's scope. - `tower`: Enables support for the `tower` crate and those using it. - `tracing`: Enables support for the `tracing` crate and those using it. - `opentelemetry`: Enables support for the `opentelemetry` and `opentelemetry-sdk` crates. diff --git a/sentry/src/lib.rs b/sentry/src/lib.rs index 89baa998..3ae59a9d 100644 --- a/sentry/src/lib.rs +++ b/sentry/src/lib.rs @@ -71,6 +71,7 @@ //! | `debug-images` | ✅ | 🔌 | | | //! | `log` | | 🔌 | | Requires extra setup; See [`sentry-log`]'s documentation. | //! | `minidump` | | 🔌 | | Linux, macOS and Windows only. See [`sentry-minidump`]'s documentation. | +//! | `minidump-scope` | | 🔌 | | Sends the crashing thread's scope. Adds a few ns to each hub switch. | //! | `slog` | | 🔌 | | Requires extra setup; See [`sentry-slog`]'s documentation. | //! | `reqwest` | ✅ | | | | //! | `native-tls` | ✅ | | | `reqwest` must be enabled. | @@ -125,6 +126,8 @@ //! ## Integrations //! - `actix`: Enables support for the `actix-web` crate. //! - `minidump`: Captures native crashes as minidumps. Linux, macOS and Windows only. +//! - `minidump-scope`: Sends the scope of the crashing thread with the minidump, instead of the +//! main hub's scope. //! - `tower`: Enables support for the `tower` crate and those using it. //! - `tracing`: Enables support for the `tracing` crate and those using it. //! - `opentelemetry`: Enables support for the `opentelemetry` and `opentelemetry-sdk` crates. From ed4d915150801c45549df2de7122ad244a6a3e86 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 13 Oct 2026 16:59:17 +0100 Subject: [PATCH 12/12] test(core): Hold the read lock on another thread --- sentry-core/src/thread_registry.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/sentry-core/src/thread_registry.rs b/sentry-core/src/thread_registry.rs index 6f49f390..049199e8 100644 --- a/sentry-core/src/thread_registry.rs +++ b/sentry-core/src/thread_registry.rs @@ -195,8 +195,24 @@ mod tests { let scope = Hub::run(hub.clone(), || { let id = current_os_thread_id().unwrap(); - // A write lock here would never be granted. - hub.with_current_scope(|_| Hub::scope_for_os_thread(id)) + + // Another thread holds the read lock, so a write lock here + // would never be granted. + let (locked_tx, locked_rx) = mpsc::channel(); + let (done_tx, done_rx) = mpsc::channel::<()>(); + let reader_hub = hub.clone(); + let reader = std::thread::spawn(move || { + reader_hub.with_current_scope(|_| { + locked_tx.send(()).unwrap(); + done_rx.recv().ok(); + }); + }); + + locked_rx.recv().unwrap(); + let scope = Hub::scope_for_os_thread(id); + done_tx.send(()).unwrap(); + reader.join().unwrap(); + scope }); let event = scope .apply_to_event(Default::default())