From 6fb841bc06528caf8eef892437a738fc2a59783b Mon Sep 17 00:00:00 2001 From: LynithDev <61880709+LynithDev@users.noreply.github.com> Date: Thu, 23 Jul 2026 18:16:45 +0200 Subject: [PATCH] chore: better sentry error reporting regarding stacktraces and errors --- .github/workflows/oneclient_nightly.yml | 21 +++ .github/workflows/oneclient_release.yml | 19 +++ Cargo.toml | 9 +- packages/oneclient_app/src/utils.rs | 1 - .../src/view/onboarding/downloading/mod.rs | 43 ++++-- packages/oneclient_core/src/auth/msa.rs | 2 +- packages/oneclient_core/src/error.rs | 73 +++++++++- packages/oneclient_core/src/http/mod.rs | 12 ++ packages/oneclient_core/src/java/error.rs | 15 ++ packages/oneclient_core/src/lib.rs | 2 +- packages/oneclient_core/src/reporting.rs | 136 ++++++++++++------ 11 files changed, 269 insertions(+), 64 deletions(-) diff --git a/.github/workflows/oneclient_nightly.yml b/.github/workflows/oneclient_nightly.yml index 3d5fe91e..3cb5d08f 100644 --- a/.github/workflows/oneclient_nightly.yml +++ b/.github/workflows/oneclient_nightly.yml @@ -95,6 +95,27 @@ jobs: echo "dir=target/release" >> "$GITHUB_OUTPUT" fi + # Upload debug info (dSYM / .dwp / .pdb produced by the release profile's + # split-debuginfo) to Sentry so nightly crash reports get real file/function/ + # line stack traces instead of raw addresses. Runs on every main push, so + # each unique build's symbols land in Sentry (matched by debug-id). Skips + # silently unless SENTRY_AUTH_TOKEN secret + SENTRY_ORG/SENTRY_PROJECT vars + # are configured. + - name: Upload debug symbols to Sentry + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + SENTRY_ORG: ${{ vars.SENTRY_ORG }} + SENTRY_PROJECT: ${{ vars.SENTRY_PROJECT }} + OUT_DIR: ${{ steps.outdir.outputs.dir }} + run: | + if [ -z "${SENTRY_AUTH_TOKEN:-}" ] || [ -z "${SENTRY_ORG:-}" ] || [ -z "${SENTRY_PROJECT:-}" ]; then + echo "Sentry credentials not configured (need SENTRY_AUTH_TOKEN secret + SENTRY_ORG/SENTRY_PROJECT vars); skipping debug-symbol upload." + exit 0 + fi + curl -sL https://sentry.io/get-cli/ | bash + sentry-cli debug-files upload --include-sources "$OUT_DIR" + - name: Collect installers id: collect shell: bash diff --git a/.github/workflows/oneclient_release.yml b/.github/workflows/oneclient_release.yml index e74a9b02..3979b3b6 100644 --- a/.github/workflows/oneclient_release.yml +++ b/.github/workflows/oneclient_release.yml @@ -167,6 +167,25 @@ jobs: echo "dir=target/release" >> "$GITHUB_OUTPUT" fi + # Upload debug info (dSYM / .dwp / .pdb produced by the release profile's + # split-debuginfo) to Sentry so crash reports get real file/function/line + # stack traces instead of raw addresses. Skips silently unless the repo has + # a SENTRY_AUTH_TOKEN secret plus SENTRY_ORG / SENTRY_PROJECT variables. + - name: Upload debug symbols to Sentry + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + SENTRY_ORG: ${{ vars.SENTRY_ORG }} + SENTRY_PROJECT: ${{ vars.SENTRY_PROJECT }} + OUT_DIR: ${{ steps.outdir.outputs.dir }} + run: | + if [ -z "${SENTRY_AUTH_TOKEN:-}" ] || [ -z "${SENTRY_ORG:-}" ] || [ -z "${SENTRY_PROJECT:-}" ]; then + echo "Sentry credentials not configured (need SENTRY_AUTH_TOKEN secret + SENTRY_ORG/SENTRY_PROJECT vars); skipping debug-symbol upload." + exit 0 + fi + curl -sL https://sentry.io/get-cli/ | bash + sentry-cli debug-files upload --include-sources "$OUT_DIR" + - name: (Windows) Locate installer if: matrix.platform == 'windows-latest' id: win_installer diff --git a/Cargo.toml b/Cargo.toml index 3bba0564..68023401 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,9 +137,16 @@ freya = { git = "https://github.com/marc2332/freya.git", branch = "fix/blur" } opt-level = 3 lto = "fat" codegen-units = 1 +# Keep the *shipped* binary stripped and small, but still emit debug info into +# separate files (macOS: .dSYM, Linux: .dwp, Windows: .pdb) so Sentry can +# symbolicate crash reports. CI uploads those files with `sentry-cli debug-files +# upload`; they are never shipped to users. Without this, release stack traces +# are just raw addresses. Use "full" instead of "line-tables-only" if you also +# want inlined-frame / variable info (larger debug files, slower builds). strip = true panic = "abort" -debug = false +debug = "line-tables-only" +split-debuginfo = "packed" [profile.release-profiling] inherits = "release" diff --git a/packages/oneclient_app/src/utils.rs b/packages/oneclient_app/src/utils.rs index 40590cc5..4868f125 100644 --- a/packages/oneclient_app/src/utils.rs +++ b/packages/oneclient_app/src/utils.rs @@ -1,6 +1,5 @@ use std::cmp::Ordering; use std::collections::BTreeMap; -use std::sync::OnceLock; use chrono::{Datelike, NaiveDate}; use oneclient_core::clusters::Cluster; diff --git a/packages/oneclient_app/src/view/onboarding/downloading/mod.rs b/packages/oneclient_app/src/view/onboarding/downloading/mod.rs index f5e096bf..76dc4cd2 100644 --- a/packages/oneclient_app/src/view/onboarding/downloading/mod.rs +++ b/packages/oneclient_app/src/view/onboarding/downloading/mod.rs @@ -8,7 +8,8 @@ use oneclient_core::notification::{ GroupedProgressEvent, GroupedProgressSession, Notification, NotificationService, }; use oneclient_core::{ - BundleArchive, BundleFile, Cluster, ImportTarget, MigrationSource, VersionMetadata, + BundleArchive, BundleFile, Cluster, ImportTarget, MigrationSource, SentryExclusion, + VersionMetadata, }; use oneclient_db::models::OverrideType; use tokio::sync::mpsc; @@ -718,13 +719,28 @@ fn run_install_batch(plans: Vec, predownload: bool, handles: Instal })); let fetch_now = predownload && plan.predownload; - if let Err(reason) = install_one(&plan, fetch_now, ¬ifier, &ui_tx).await { - tracing::error!( - cluster_id = plan.cluster_id, - mc_version = %plan.mc_version, - "onboarding install failed: {reason}" - ); - failed.push(InstallFailure { plan, reason }); + if let Err(err) = install_one(&plan, fetch_now, ¬ifier, &ui_tx).await { + // Environmental / expected failures (out of disk, offline, no Java + // build for this version, ...) are surfaced to the user but aren't + // crashes, so log them at warn! (breadcrumb) instead of error! + // (a reported Sentry event). See LauncherError::is_sentry_excluded. + if err.is_sentry_excluded() { + tracing::warn!( + cluster_id = plan.cluster_id, + mc_version = %plan.mc_version, + "onboarding install failed (expected): {err}" + ); + } else { + tracing::error!( + cluster_id = plan.cluster_id, + mc_version = %plan.mc_version, + "onboarding install failed: {err}" + ); + } + failed.push(InstallFailure { + plan, + reason: err.to_string(), + }); } let _ = ui_tx.send(InstallUiEvent::Progress(index + 1, total)); } @@ -739,8 +755,8 @@ async fn install_one( predownload: bool, notifier: &NotificationService, ui_tx: &mpsc::UnboundedSender, -) -> Result<(), String> { - let state = oneclient_core::LauncherState::get().map_err(|err| err.to_string())?; +) -> oneclient_core::LauncherResult<()> { + let state = oneclient_core::LauncherState::get()?; if !plan.overrides.is_empty() { let _ = ui_tx.send(InstallUiEvent::Activity( @@ -748,8 +764,7 @@ async fn install_one( )); } oneclient_core::set_bundle_package_overrides(plan.cluster_id, &plan.overrides, &state.services) - .await - .map_err(|err| err.to_string())?; + .await?; if !predownload { return Ok(()); @@ -791,8 +806,8 @@ async fn install_one( session.finish(); - prepared.map_err(|err| err.to_string())?; - bundles_result.map_err(|err| err.to_string())?; + prepared?; + bundles_result?; Ok(()) } diff --git a/packages/oneclient_core/src/auth/msa.rs b/packages/oneclient_core/src/auth/msa.rs index 9015a3bc..dfc60698 100644 --- a/packages/oneclient_core/src/auth/msa.rs +++ b/packages/oneclient_core/src/auth/msa.rs @@ -787,7 +787,7 @@ async fn parse_json_response Deserialize<'de>>( .map_err(|source| MinecraftAuthError::RequestError { step, source })?; if !status.is_success() { - tracing::error!("[auth] step={step:?} status={status} body={text}"); + tracing::error!(sentry = false, ?step, %status, body = %text, "MSA endpoint returned a non-success status"); } serde_json::from_str(&text).map_err(|source| { diff --git a/packages/oneclient_core/src/error.rs b/packages/oneclient_core/src/error.rs index 43940637..d6620c2e 100644 --- a/packages/oneclient_core/src/error.rs +++ b/packages/oneclient_core/src/error.rs @@ -6,7 +6,7 @@ pub enum LauncherError { JsonError(#[from] serde_json::Error), #[error("Database execution failed: {0}")] - DatabaseError(#[from] sqlx::Error), + SqlError(#[from] sqlx::Error), #[error(transparent)] IoError(#[from] polyio::IOError), @@ -81,4 +81,75 @@ impl LauncherError { } } +pub trait SentryExclusion { + /// Whether this error is expected/environmental noise that should be kept out + /// of Sentry rather than reported as a crash. + fn is_sentry_excluded(&self) -> bool { + false + } +} + +impl SentryExclusion for LauncherError { + fn is_sentry_excluded(&self) -> bool { + match self { + LauncherError::StdIoError(e) => e.is_sentry_excluded(), + LauncherError::IoError(e) => e.is_sentry_excluded(), + LauncherError::RequestError(e) => e.is_sentry_excluded(), + LauncherError::JavaError(e) => e.is_sentry_excluded(), + _ => false, + } + } +} + +impl SentryExclusion for std::io::Error { + fn is_sentry_excluded(&self) -> bool { + use std::io::ErrorKind; + + // Out of disk space. `ErrorKind::StorageFull` is still unstable, so match + // the raw OS codes instead. Codes are platform-gated so a Unix errno can't + // collide with a Windows code (112 is the unrelated EHOSTDOWN on Linux). + if let Some(code) = self.raw_os_error() { + #[cfg(unix)] + if code == 28 { + // ENOSPC + return true; + } + #[cfg(windows)] + if code == 112 || code == 39 { + // ERROR_DISK_FULL / ERROR_HANDLE_DISK_FULL + return true; + } + let _ = code; + } + + // Lost/refused network connections during downloads. + matches!( + self.kind(), + ErrorKind::ConnectionRefused + | ErrorKind::ConnectionReset + | ErrorKind::ConnectionAborted + | ErrorKind::NotConnected + | ErrorKind::TimedOut + ) + } +} + +impl SentryExclusion for reqwest::Error { + fn is_sentry_excluded(&self) -> bool { + // Connectivity problems (offline, connection refused, timed out) are the + // user's network, not a launcher bug. + self.is_timeout() || self.is_connect() + } +} + +impl SentryExclusion for polyio::IOError { + fn is_sentry_excluded(&self) -> bool { + match self { + polyio::IOError::IOError(source) + | polyio::IOError::PathIOError { source, .. } => source.is_sentry_excluded(), + _ => false, + } + } +} + pub type LauncherResult = Result; diff --git a/packages/oneclient_core/src/http/mod.rs b/packages/oneclient_core/src/http/mod.rs index 27851762..5cbf45eb 100644 --- a/packages/oneclient_core/src/http/mod.rs +++ b/packages/oneclient_core/src/http/mod.rs @@ -44,4 +44,16 @@ pub enum RequestError { #[error("Invalid HTTP header value: {0}")] InvalidHeaderValue(#[from] reqwest::header::InvalidHeaderValue), +} + +impl crate::error::SentryExclusion for RequestError { + fn is_sentry_excluded(&self) -> bool { + // Only connectivity / disk failures are noise; malformed requests, bad + // URLs, and unexpected response shapes are bugs worth reporting. + match self { + RequestError::ReqwestError(source) => source.is_sentry_excluded(), + RequestError::IOError(source) => source.is_sentry_excluded(), + _ => false, + } + } } \ No newline at end of file diff --git a/packages/oneclient_core/src/java/error.rs b/packages/oneclient_core/src/java/error.rs index fb7a7214..a825ce43 100644 --- a/packages/oneclient_core/src/java/error.rs +++ b/packages/oneclient_core/src/java/error.rs @@ -40,4 +40,19 @@ pub enum JavaError { #[error("failed to extract archive '{archive}'")] ArchiveExtractFailed { archive: String }, +} + +impl crate::error::SentryExclusion for JavaError { + fn is_sentry_excluded(&self) -> bool { + match self { + // Expected outcomes: no build exists for the requested version, or the + // user cancelled setup. + JavaError::PackageNotFound { .. } | JavaError::Cancelled => true, + // Environmental IO (e.g. out of disk while extracting the runtime). + JavaError::RuntimeCheckError { source, .. } + | JavaError::ArchiveExtract { source, .. } => source.is_sentry_excluded(), + JavaError::PolyIOError(source) => source.is_sentry_excluded(), + _ => false, + } + } } \ No newline at end of file diff --git a/packages/oneclient_core/src/lib.rs b/packages/oneclient_core/src/lib.rs index 95b599a9..fb4fb685 100644 --- a/packages/oneclient_core/src/lib.rs +++ b/packages/oneclient_core/src/lib.rs @@ -54,7 +54,7 @@ pub use clusters::{ Cluster, ClusterError, ClusterManager, ClusterStage, ClusterUpdate, CreateClusterOptions, ensure_from_bundles, ensure_from_versions, estimate_cluster_download, }; -pub use error::{LauncherError, LauncherResult}; +pub use error::{LauncherError, LauncherResult, SentryExclusion}; pub use game::{GameError, LaunchedGame, get_loader_versions, launch_cluster}; pub use images::ImageCacheStore; pub use logs::{ diff --git a/packages/oneclient_core/src/reporting.rs b/packages/oneclient_core/src/reporting.rs index 45b3c883..5c03f335 100644 --- a/packages/oneclient_core/src/reporting.rs +++ b/packages/oneclient_core/src/reporting.rs @@ -1,76 +1,122 @@ use std::panic; +use std::sync::Arc; use std::time::Duration; +use sentry::protocol::{Context, Event}; use sentry::{ClientInitGuard, ClientOptions}; use crate::constants::SENTRY_DSN; const PANIC_FLUSH_TIMEOUT: Duration = Duration::from_secs(2); +const TRACING_FIELDS_CONTEXT: &str = "Rust Tracing Fields"; const ENVIRONMENT: &str = if cfg!(debug_assertions) { - "development" + "development" } else { - "production" + "production" }; +/// `before_send` hook: drop any event a log call explicitly opted out of by +/// setting the `sentry` field to `false`, e.g. +/// +/// ```ignore +/// tracing::error!(sentry = false, "handled MSA failure, don't report as a crash"); +/// ``` +fn drop_opted_out_events(event: Event<'static>) -> Option> { + let opted_out = matches!( + event.contexts.get(TRACING_FIELDS_CONTEXT), + Some(Context::Other(fields)) + if fields.get("sentry").and_then(serde_json::Value::as_bool) == Some(false) + ); + + if opted_out { None } else { Some(event) } +} + pub fn init(enabled: bool) -> Option { - if !enabled { - tracing::debug!("crash reporting disabled by settings"); - return None; - } + if !enabled { + tracing::debug!("crash reporting disabled by settings"); + return None; + } - if cfg!(debug_assertions) && option_env!("ONECLIENT_SENTRY_DSN").is_none() { - tracing::debug!("crash reporting skipped: debug build without an explicit DSN"); - return None; - } + if cfg!(debug_assertions) && option_env!("ONECLIENT_SENTRY_DSN").is_none() { + tracing::debug!("crash reporting skipped: debug build without an explicit DSN"); + return None; + } - let guard = sentry::init(( - SENTRY_DSN, - ClientOptions { - release: Some(format!("oneclient@{}", env!("CARGO_PKG_VERSION")).into()), - environment: Some(ENVIRONMENT.into()), - attach_stacktrace: true, - send_default_pii: false, - ..Default::default() - }, - )); + let guard = sentry::init(( + SENTRY_DSN, + ClientOptions { + release: Some(format!("oneclient@{}", env!("CARGO_PKG_VERSION")).into()), + environment: Some(ENVIRONMENT.into()), + attach_stacktrace: true, + send_default_pii: false, + before_send: Some(Arc::new(drop_opted_out_events)), + ..Default::default() + }, + )); - if !guard.is_enabled() { - tracing::warn!("sentry client failed to start; crash reports will not be sent"); - return None; - } + if !guard.is_enabled() { + tracing::warn!("sentry client failed to start; crash reports will not be sent"); + return None; + } - install_panic_flush(); + install_panic_flush(); - tracing::info!(environment = ENVIRONMENT, "crash reporting enabled"); - Some(guard) + tracing::info!(environment = ENVIRONMENT, "crash reporting enabled"); + Some(guard) } fn install_panic_flush() { - let capture = panic::take_hook(); + let capture = panic::take_hook(); - panic::set_hook(Box::new(move |info| { - capture(info); + panic::set_hook(Box::new(move |info| { + capture(info); - if let Some(client) = sentry::Hub::current().client() { - client.flush(Some(PANIC_FLUSH_TIMEOUT)); - } - })); + if let Some(client) = sentry::Hub::current().client() { + client.flush(Some(PANIC_FLUSH_TIMEOUT)); + } + })); } #[cfg(test)] mod tests { - use super::*; + use super::*; - #[test] - fn bundled_dsn_parses() { - SENTRY_DSN - .parse::() - .expect("bundled sentry DSN should be valid"); - } + #[test] + fn bundled_dsn_parses() { + SENTRY_DSN + .parse::() + .expect("bundled sentry DSN should be valid"); + } - #[test] - fn opting_out_skips_the_client() { - assert!(init(false).is_none()); - } + #[test] + fn opting_out_skips_the_client() { + assert!(init(false).is_none()); + } + + /// Build an event the way `sentry-tracing` would, given the value of a + /// `sentry` field on the log call (or `None` for no such field). + fn event_with_sentry_field(value: Option) -> Event<'static> { + let mut event = Event::default(); + if let Some(value) = value { + let mut fields = std::collections::BTreeMap::new(); + fields.insert("sentry".to_owned(), serde_json::Value::Bool(value)); + event + .contexts + .insert(TRACING_FIELDS_CONTEXT.to_owned(), Context::Other(fields)); + } + event + } + + #[test] + fn drops_events_flagged_sentry_false() { + assert!(drop_opted_out_events(event_with_sentry_field(Some(false))).is_none()); + } + + #[test] + fn keeps_events_without_opt_out() { + assert!(drop_opted_out_events(event_with_sentry_field(Some(true))).is_some()); + assert!(drop_opted_out_events(event_with_sentry_field(None)).is_some()); + assert!(drop_opted_out_events(Event::default()).is_some()); + } }