ref(crash): Replace sentry-native with minidumper#1978
Conversation
|
|
||
| if let Some(crash_db) = crash_db { | ||
| symbolicator_crash::init(crash_db, (*sentry).clone()); | ||
| } |
There was a problem hiding this comment.
Crash handler without Sentry DSN
Medium Severity
Crash reporting now starts whenever a crash_db path exists, without requiring sentry_dsn. The previous logic only installed native crash handling when both a DSN and database path were set. Deployments with a default cache_dir (for example Docker’s /data) but no Sentry DSN now spawn an extra crash-reporter process and IPC setup even though uploads cannot be configured.
Reviewed by Cursor Bugbot for commit 7dd4dcb. Configure here.
| // is safe to call. | ||
| unsafe { logging::init_logging(&config) }; | ||
|
|
||
| #[cfg(feature = "symbolicator-crash")] | ||
| { | ||
| let crash_db = config._crash_db.clone().or_else(|| { | ||
| config |
There was a problem hiding this comment.
Bug: The crash reporter child process doesn't exit after initialization and proceeds to execute the main application command, such as starting the server, leading to resource conflicts.
Severity: CRITICAL
Suggested Fix
Add an early exit guard in cli.rs immediately after the symbolicator_crash::init() call. Check if the current process is the crash reporter using symbolicator_crash::is_crash_reporter_process() and, if so, return Ok(()) to terminate its execution gracefully before it reaches the main command dispatch.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: crates/symbolicator/src/cli.rs#L119-L125
Potential issue: The `symbolicator_crash::init()` function spawns a non-blocking child
process to act as a crash reporter. This child process re-executes the application with
the same command-line arguments but does not exit after its initialization. It continues
to the main command dispatch logic in `cli.rs`, attempting to run the primary command
(e.g., `server::run`) concurrently with the main process. This will cause failures, such
as a port conflict when both processes try to bind to the same address, and general
resource contention.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d7d0662. Configure here.
| error = &err as &dyn std::error::Error, | ||
| "Failed to initialize crash reporting process" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Crash reporter continues after spawn
High Severity
When the re-executed crash-reporter process fails spawn(), init only logs a warning and returns, so execute() keeps going and can run the normal CLI (e.g. run) with _SYMBOLICATOR_CRASH_REPORTER_PROCESS still set and Sentry scoped as crash-reporter, instead of exiting.
Reviewed by Cursor Bugbot for commit d7d0662. Configure here.


Mostly a copy of getsentry/relay#6158. This uses a Rust-native, out-of-process crash reporter instead of
sentry-native. This means thesentry-nativesubmodule is gone entirely.