Also updates CI to always run this - that way we ensure that disabling
or enabling tests does not become stale at some point and thus can
safely add this.
Release Notes:
- N/A
- [GPUI] N/A
The crash handler models the crashing user as a fixed `UserInfo` struct
of `metrics_id` and `is_staff`. That is a reasonable fit for Zed and a
poor extension point in general: reporting any further piece of session
state with a crash means changing this crate, and every embedder along
with it.
The limitation has already produced a workaround in another consumer of
this crate. Wanting one field that `UserInfo` has no room for is enough
to push an embedder off the IPC channel entirely, onto persisting that
state beside the minidump and correlating it back at upload time. That
needs an atomic write, a retention window for records whose minidump
never uploads, and a cross-check so a stale record cannot be attributed
to the wrong crash — none of which the existing socket would have
required.
This replaces the `UserInfo` message with a generic `SetTags` message
carrying one or more updates. Each key is the Sentry submission field
the value is uploaded under, so an application can attach its own
session state and reach any part of Sentry's event schema without this
crate modelling what it means. Updates merge by key, and a `None` value
removes one, which is how an application withdraws state from crashes it
has not produced yet. A batch is applied while holding the tag-map lock
once, so a crash cannot observe part of one logical state change.
`set_user_info` keeps its signature and is now shorthand for setting the
two identity tags in one batch, so Zed's call site is unchanged.
`upload_minidump` loses its two bespoke user blocks in favour of
forwarding the map, and keeps its installation-id fallback by checking
whether the session ever reported a user.
Two consequences worth flagging for review. The first is that this crate
now knows it reports to Sentry, where previously it emitted neutral data
that `reliability.rs` translated. I think that is honest rather than
regrettable — its consumers upload to Sentry, and pretending otherwise
is what gave `UserInfo` a shape that fits only one of them. The
alternative considered was a well-known-key mapping table in the
uploader, which keeps the crate agnostic at the cost of a translation
layer that can silently file a value under the wrong namespace.
The second is a one-time compatibility loss. A crash is uploaded by the
build that starts after it, so an upgrade always reads crash data
written by the previous build. Crashes pending across this change carry
`user_info` and no `tags`, and will upload without user attribution.
They still parse and still upload; only the identity is dropped, for one
release. A test covers that.
Testing:
- New unit test for batch, merge, and removal semantics, driven through
`on_message` so it exercises the real wire format.
- New unit test that crash data written before `tags` existed still
deserializes.
- `cargo test -p crashes`, `./script/clippy -p crashes -p zed`, `cargo
check -p zed -p remote_server`, `cargo fmt --check`.
Release Notes:
- N/A
---------
Co-authored-by: morgankrey <morgan@zed.dev>
On Windows, the panic hook generated a minidump by calling
`simulate_exception`, but that function only invokes the crash callback
and then returns. Rust could therefore unwind a panicking background
thread while leaving the rest of the application running.
Abort unconditionally after minidump generation, matching the existing
Linux and macOS behavior. The Windows-specific simulation still runs
first so the crash sidecar can persist the exception context and panic
metadata before the process terminates.
Testing:
- `cargo fmt -p crashes --check`
- `cargo nextest run -p crashes`
Release Notes:
- Fixed Windows processes remaining alive after a background thread
panic.
Handle the `WM_QUERYENDSESSION` and `WM_ENDSESSION` messages, which are
used by Windows when shutting down an application. For example, Restart
Manager uses this mechanism to gracefully close applications (e.g. when
installing an update to the application).
Release Notes:
- N/A
Linux crash reports for runtime-initiated aborts currently carry no
information about why the runtime aborted. A sizable family of Sentry
issues (FR-108 / [ZED-9SC](https://zed-dev.sentry.io/issues/7581925141/)
among them) consists of glibc's malloc integrity checks detecting heap
corruption and aborting from inside `malloc`/`realloc`/`free`, so each
event lands on a random victim stack and the underlying problem stays
invisible and ungroupable.
glibc records the diagnostic it prints before aborting ("free(): invalid
pointer", "double free or corruption", assertion failures,
stack-smashing reports) in the private `__abort_msg` global specifically
so it can be recovered post-mortem. This resolves that symbol's address
at startup and sends it to the crash-handler process; when a crash comes
in, the handler reads the message out of the crashed process's memory
with `process_vm_readv` while the client is still parked in its signal
handler awaiting the dump acknowledgement. The message is then attached
to crash uploads as a searchable `abort_message` tag plus a full-text
context. The crashed process does no work itself, which matters because
in exactly these crashes the crashed thread may hold the allocator's
arena lock, so it cannot safely allocate or format anything. Everything
degrades gracefully to the current behavior when the symbol is
unavailable (musl, future glibc changes).
With the tag in place, Sentry fingerprint rules can fold these
corruption aborts into a single tracked issue instead of minting a new
single-event issue per victim stack, and the message text distinguishes
double-frees from out-of-bounds metadata corruption when hunting the
actual culprit.
Release Notes:
- N/A
Moves the crash handler subprocess spawn off the main thread to speed up
startup, especially on Windows where process creation is slower.
Previously `crashes::init` did part of its work synchronously when it
was *called*, including `spawn_crash_handler`, which launches the `zed
--crash-handler` child process (a synchronous `CreateProcessW` on
Windows). Because `init` is evaluated inline as the argument to
`background_executor().spawn(crashes::init(...))` in `main.rs`, that
subprocess spawn ran on the main thread during startup rather than on
the executor.
This PR makes `connect_and_keepalive` a fully `async fn`, so all of that
work, including the subprocess spawn, now runs on the background
executor instead of blocking the main thread.
I don't have a Windows machine to capture before/after numbers, but the
crash handler spawn is clearly on the startup critical path. Logs in the
Windows slow startup reports show it taking ~100–700ms between `spawning
crash handler process` and `connected to crash handler process` (e.g.
#40621, #54856), all of which previously blocked the main thread.
Related to #49442
Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content adheres to Zed's UI standards
([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
and
[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)
guidelines)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
Release Notes:
- Improved startup performance
Self-Review Checklist:
- [ ] I've reviewed my own diff for quality, security, and reliability
- [ ] Unsafe blocks (if any) have justifying comments
- [ ] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [ ] Tests cover the new/changed behavior
- [ ] Performance impact has been considered and is acceptable
Closes #ISSUE
Release Notes:
- N/A or Added/Fixed/Improved ...
String panics are a non-trivial percentage of the crashes we see at Zed,
and
doubly unfortunately they may incidentally include the contents of a
user's
buffer.
Although this hasn't happened yet (to my knowledge), I don't want to be
in the position of having received sensitive information this way.
See also https://github.com/rust-lang/rust/pull/153677
Release Notes:
- N/A
Before this change the crash handler uploaded crashes before sign-in had
happened.
Now we get the metrics_id correctly. This allows for us to tie crashes
reported
on Github to users who have opted into telemetry (users who have opted
into
crash reporting but not telemetry will not have a metrics_id).
Release Notes:
- Fixed crash reporter metadata collection
---------
Co-authored-by: Miguel Raz Guzmán Macedo <miguel@zed.dev>
#50176 added `recoverable_panic` to swallow mermaid panics, then #50280
fixed it in upstream. This PR removes the workaround so future panics
reach Sentry. Also bumps `mermaid-rs-renderer` to fix a hex parsing
panic.
Release Notes:
- N/A
The mermaid renderer can fail to render certain diagrams, and we already
have a fallback for that. It's not worth crashing Zed over it.
Wrapping `catch_unwind` alone doesn't work because our panic hooks
terminate the process before unwinding begins (we intentionally
terminate so the crash handler subprocess can generate a minidump), so
there was no way to gracefully handle panics from third-party code until
now (in rare cases where we need it, like in this case).
To handle this gracefully, we added `crashes::recoverable_panic` which
tells the panic hook to stand down on the current thread so the unwind
can proceed and be caught. This should be used sparingly since caught
panics bypass crash reporting.
Release Notes:
- Fixed a crash when rendering mermaid diagrams in markdown preview.
We see a number of crashes in Sentry that appear to be crashes in
wasmtime.
This shouldn't happen, as wasmtime is designed to run untrusted code
"safely".
Looking into this, it seems likely that the problem is that we race with
wasmtime
when installing signal handlers. If wasmtime's handlers are installed
before ours,
then any signals that it intends to handle (like out of bounds memory
access) will
reach our handlers before its; which causes us to assume the app has
crashed.
This changes fixes our crash handler initialization to ensure we always
create
our signal handler first, and reverts a previous attempt to fix this
from #40883
Closes #ISSUE
Before you mark this PR as ready for review, make sure that you have:
- [ ] Added a solid test coverage and/or screenshots from doing manual
testing
- [ ] Done a self-review taking into account security and performance
aspects
- [ ] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
Release Notes:
- Linux: Fixed crashes that could happen due to our crash handler
erroneously catching signals intended for wasmtime.
https://github.com/zed-industries/zed/pull/40883 implemented this
incorrectly. It was marking a random background thread as a wasm thread
(whatever thread picked up the wasm epoch timer background task),
instead of marking the threads that actually run the wasm extension.
This has two implications:
1. it didn't prevent extension panics from tearing down as planned
2. Worse, it actually made us hide legit panics in sentry for one of our
background workers.
Now 2 still technically applies for all tokio threads after this, but we
basically only use these for wasm extensions in the main zed binary.
Release Notes:
- Fixed extension panics crashing Zed on Linux
Closes#41471
We were killing the crash handler when it received a second copy of any
of the messages, but this GPU specs one is sent on each new window
rather than once at startup. We could gate the sending to only happen
once, but it's simpler to just allow multiple gpu specs messages.
Release Notes:
- N/A
Set a TLS bit to skip invoking the crash handler when a detached thread
panics.
cc @P1n3appl3 - is this at odds with what we need the crash handler to
do?
May close#39289, cannot repro without a nightly build
Release Notes:
- Fixed extension panics crashing Zed on Linux
Co-authored-by: dino <dinojoaocosta@gmail.com>
We've been considering removing workspace-hack for a couple reasons:
- Lukas ran into a situation where its build script seemed to be causing
spurious rebuilds. This seems more likely to be a cargo bug than an
issue with workspace-hack itself (given that it has an empty build
script), but we don't necessarily want to take the time to hunt that
down right now.
- Marshall mentioned hakari interacts poorly with automated crate
updates (in our case provided by rennovate) because you'd need to have
`cargo hakari generate && cargo hakari manage-deps` after their changes
and we prefer to not have actions that make commits.
Currently removing workspace-hack causes our workspace to grow from
~1700 to ~2000 crates being built (depending on platform), which is
mainly a problem when you're building the whole workspace or running
tests across the the normal and remote binaries (which is where
feature-unification nets us the most sharing). It doesn't impact
incremental times noticeably when you're just iterating on `-p zed`, and
we'll hopefully get these savings back in the future when
rust-lang/cargo#14774 (which re-implements the functionality of hakari)
is finished.
Release Notes:
- N/A
This allows to filter by main zed binary or remote server crashes, as
well as easily tell whether a crash happened in a remote-server binary
or not.
Release Notes:
- N/A
std commands can block for an arbitrary duration and so runs risk of
blocking tasks for too long. This replaces all such uses where sensible
with async processes.
Release Notes:
- N/A *or* Added/Fixed/Improved ...
@maxdeviant We can eventually turn down the panic telemetry endpoint,
but should probably leave it up while there's still a bunch of stable
users hitting it.
@maxbrunsfeld We're optimistic that this change also fixed the macos
crashed-thread misreporting. We think it was because the
`CrashContext::exception` was getting set to `None` only on macos, while
on linux it was getting a real exception value from the sigtrap. Now
we've unified and it uses `SIGABRT` on both platforms (I need to double
check that this works as expected for windows).
We unconditionally set `RUST_BACKTRACE=1` for the current process so
that we see backtraces when running in a terminal by default. This
should be fine but I just wanted to note it since it's a bit abnormal.
Release Notes:
- N/A
---------
Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
Closes #ISSUE
Adds system GPU collection to crash reporting. Currently this is Linux
only.
The system GPUs are determined by reading the `/sys/class/drm` directory
structure, rather than using the exisiting `gpui::Window::gpu_specs()`
method in order to gather more information, and so that the GPU context
is not dependent on Vulkan context initialization (i.e. we still get GPU
info when Zed fails to start because Vulkan failed to initialize).
Unfortunately, the `blade` APIs do not support querying which GPU _will_
be used, so we do not know which GPU was attempted to be used when
Vulkan context initialization fails, however, when Vulkan initialization
succeeds, we send a message to the crash handler containing the result
of `gpui::Window::gpu_specs()` to include the "Active" gpu in any crash
report that may occur
Release Notes:
- N/A *or* Added/Fixed/Improved ...
On the latest build @maxbrunsfeld got a panic that hung zed. It appeared
that the hang occured after the minidump had been successfully written,
so our theory on what happened is that the `suspend_all_other_threads`
call in the crash handler suspended the panicking thread (due to the
signal from simulate_exception being received on a different thread),
and then when the crash handler returned everything was suspended so the
panic hook never made it to the `process::abort`.
This change makes the crash handler avoid _both_ the current and the
panicking thread which should avoid that scenario.
Release Notes:
- N/A
We see a bunch of crash events with truncated minidumps where they have
a valid header but no events. We think this is due to an issue
generating them, so we're attaching the relevant result to the uploaded
tags.
Release Notes:
- N/A
Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
This should improve our detection of which thread crashed since they
wont be able to resume while the minidump is being generated.
Release Notes:
- N/A
The minidump-based crash reporting is now entirely separate from our
legacy panic_hook-based reporting. This should improve the association
of minidumps with their metadata and give us more consistent crash
reports.
Release Notes:
- N/A
---------
Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
We only want minidumps to be generated on actual release builds. Now we
avoid spawning crash handler processes for dev builds. To test
minidumping you can still set the `ZED_GENERATE_MINIDUMPS` env var which
force-enable the feature.
Release Notes:
- N/A
- [x] Handle uploading minidumps from the remote_server
- [x] Associate minidumps with panics with some sort of ID (we don't use
session_id on the remote)
- [x] Update the protobufs and client/server code to request panics
- [x] Upload minidumps with no corresponding panic
- [x] Fill in panic info when there _is_ a corresponding panic
- [x] Use an env var for the sentry endpoint instead of hardcoding it
Release Notes:
- Zed now generates minidumps for crash reporting
---------
Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Finn Evers