[ty] Gracefully handle salsa cancellations and panics in background request handlers #18254

MichaReiser · 2025-05-22T13:02:36Z

Summary

Run the request and notification handlers in a catch_unwind that ensures that a handler failure doesn't tear down the entire server process. ty now also responds with an appropriate error instead of just never responding to the client's request. This has the advantage that e.g. VS code stops sending new diangostic pull requests for a file on which the server crashed before.

I considered wrapping local notifications handlers in catch_unwind but decided against it. Local notifications are used to update the server state. An out of date server state can lead to all sort of errors because the ranges are of. That's why I think it's better to still crash the server.

Test Plan

Tested that ty shows an error pop up and the backtrace when a background task panics. The server keeps running
Tested that ty shows an error error pop up and the backtrace when a local task panics. the server crashes.

github-actions · 2025-05-22T13:05:24Z

`mypy_primer` results

No ecosystem changes detected ✅

MichaReiser · 2025-05-22T13:04:42Z

crates/ty_project/src/db.rs


    /// Checks all open files in the project and its dependencies.
-    pub fn check(&self) -> Result<Vec<Diagnostic>, Cancelled> {
+    pub fn check(&self) -> Vec<Diagnostic> {


I decided to remove all Results from here for now because the server already uses many salsa APIs that aren't wrapped in a salsa::Cancelled::catch. Instead, I decided that the server request handlers catch salsa cancellations (only necessary for background tasks because:

Only local tasks can mutate the DB

All local tasks run on the main loop thread.

I'm not hugely familiar with this part of the code, but this rationale sounds reasonable

MichaReiser · 2025-05-22T13:05:04Z

crates/ty_server/src/server/api.rs

    let (id, params) = cast_request::<R>(req)?;
    Ok(Task::local(|session, notifier, requester, responder| {
-        let _span = tracing::trace_span!("request", %id, method = R::METHOD).entered();
+        let _span = tracing::debug_span!("request", %id, method = R::METHOD).entered();


The span is very useful and tracing is just way too verbose.

MichaReiser · 2025-05-22T13:05:36Z

crates/ty_server/src/server/api.rs

+        Ok(response) => Some(response),
+        Err(error) => {
+            if error.payload.downcast_ref::<salsa::Cancelled>().is_some() {
+                // Request was cancelled by Salsa. TOOD: Retry


I leave this for another PR, there's already enough going on in this PR

github-actions · 2025-05-22T13:14:05Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ty_server/src/server/api.rs

dcreager · 2025-05-23T15:20:47Z

crates/ty_project/src/db.rs


    /// Checks all open files in the project and its dependencies.
-    pub fn check(&self) -> Result<Vec<Diagnostic>, Cancelled> {
+    pub fn check(&self) -> Vec<Diagnostic> {


I'm not hugely familiar with this part of the code, but this rationale sounds reasonable

dhruvmanila

Looks good!

dhruvmanila · 2025-05-26T10:21:52Z

crates/ty_server/src/server/api.rs

+                let message = format!("request handler {error}");
+
+                Some(Err(Error {
+                    code: lsp_server::ErrorCode::InternalError,
+                    error: anyhow!(message),
+                }))


nit: we could probably use LSPResult::with_failure_code?

Oh, I didn't know about this method. I'll leave it as is because I use the lsp_server Error type in #18273

dhruvmanila · 2025-05-26T10:26:14Z

crates/ty_server/src/server.rs

-        // The new PanicInfoHook name requires MSRV >= 1.82
-        #[expect(deprecated)]
-        type PanicHook = Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>;
+        type PanicHook = Box<dyn Fn(&PanicHookInfo<'_>) + 'static + Sync + Send>;


Can you make this change for ruff_server as well?

I plan to backport this entire stack to ruff

crates/ty_server/src/server/api.rs

MichaReiser added server Related to the LSP server ty Multi-file analysis & type inference labels May 22, 2025

MichaReiser force-pushed the micha/panic-handlers-backround-threads branch from 09d5dbc to 3f64170 Compare May 22, 2025 13:07

MichaReiser changed the title ~~[ty] Abort process if worker thread panics~~ [ty] Gracefully handle salsa cancellations and panics in background handlers May 22, 2025

MichaReiser commented May 22, 2025

View reviewed changes

MichaReiser marked this pull request as ready for review May 22, 2025 13:08

MichaReiser requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners May 22, 2025 13:08

MichaReiser requested review from dhruvmanila and removed request for AlexWaygood, carljm, dcreager and sharkdp May 22, 2025 13:08

MichaReiser changed the title ~~[ty] Gracefully handle salsa cancellations and panics in background handlers~~ [ty] Gracefully handle salsa cancellations and panics in background requests May 23, 2025

dcreager approved these changes May 23, 2025

View reviewed changes

dhruvmanila approved these changes May 26, 2025

View reviewed changes

MichaReiser force-pushed the micha/server-panic branch from 8fcdf96 to ccd14a5 Compare May 26, 2025 11:59

MichaReiser force-pushed the micha/panic-handlers-backround-threads branch from 3f64170 to 836fc57 Compare May 26, 2025 12:07

Base automatically changed from micha/server-panic to main May 26, 2025 12:09

MichaReiser added 2 commits May 26, 2025 14:10

[ty] Run background tasks with panic handler

e9a4782

Updat error message

3fe52c6

MichaReiser force-pushed the micha/panic-handlers-backround-threads branch from 836fc57 to 3fe52c6 Compare May 26, 2025 12:10

MichaReiser changed the title ~~[ty] Gracefully handle salsa cancellations and panics in background requests~~ [ty] Gracefully handle salsa cancellations and panics in background request handlers May 26, 2025

Clippy

7301123

MichaReiser merged commit d8216fa into main May 26, 2025
35 checks passed

MichaReiser deleted the micha/panic-handlers-backround-threads branch May 26, 2025 12:37

MichaReiser mentioned this pull request May 28, 2025

Review used APIs in the LSP astral-sh/ty#83

Closed

[ty] Gracefully handle salsa cancellations and panics in background request handlers #18254

[ty] Gracefully handle salsa cancellations and panics in background request handlers #18254

Uh oh!

Conversation

MichaReiser commented May 22, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented May 22, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented May 22, 2025 •

edited

Loading

`ruff-ecosystem` results