Skip to content

Protect r_task() from panics and detect cancelled handlers#1269

Merged
lionel- merged 1 commit into
mainfrom
oak/salsa-28-rtask-unwind
Jun 19, 2026
Merged

Protect r_task() from panics and detect cancelled handlers#1269
lionel- merged 1 commit into
mainfrom
oak/salsa-28-rtask-unwind

Conversation

@lionel-

@lionel- lionel- commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Branched from #1268
Progress towards #1212

Currently a Salsa cancellation panic can't be triggered in an r_task() because all calls to r_task() that query Salsa (for completions) happen on the main loop, during which the Salsa DB is pinned (since the main loop is the sole writer). But I thought better to be safe. We now:

  • Catch all panics in r_task() and reraise them. This is much safer, since it's UB to panic over C frames (top-level-exec and with-calling-handlers frames in particular). If a cancellation is ever raised from r_task() (e.g. if called from a background thread), it would be safely re-raised the same way, and would be caught by spawn_blocking().

  • Catch Salsa cancellation panics in respond() and reply to the frontend with ContentModified (follows Rust-Analyser).

@DavisVaughan DavisVaughan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I was kind of hoping to see something that prevented us from trying to call salsa queries from an r_task! in the first place. Like it would force us to extract out the relevant info from the salsa queries before moveing the results into the r_task!.

It feels like the Salsa model was:

  • main dispatcher gets a request
  • secondary thread spawned to handle request
  • secondary thread tries to query the salsa db, but something has changed and the query is canceled, causing a panic in secondary thread

and thats totally fine because the secondary thread isnt load bearing

But now we are trying to involve the main R thread from the secondary thread, and the main R thread is able to query salsa, which means it may also get a cancellation panic. And that just feels like we are fighting the idea of salsa (as i understand it) where the salsa queries should be contained within those secondary threads where it is totally fine to panic on cancellation.

@lionel- lionel- force-pushed the oak/salsa-28-rtask-unwind branch from a05a82d to 1705d42 Compare June 16, 2026 23:04
@lionel-

lionel- commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

I don't think we're fighting the idea of Salsa, more like we're articulating it to how we set up R thread tasks.

Also it's all temporary, I went for the easy (and correct) stopgap approach.

@lionel- lionel- force-pushed the oak/salsa-27-wire-url branch from d00f7bc to 5cee100 Compare June 19, 2026 14:04
Base automatically changed from oak/salsa-27-wire-url to main June 19, 2026 14:04
@lionel- lionel- force-pushed the oak/salsa-28-rtask-unwind branch from 1705d42 to 4de75ff Compare June 19, 2026 14:05
@lionel- lionel- merged commit 258b30c into main Jun 19, 2026
17 checks passed
@lionel- lionel- deleted the oak/salsa-28-rtask-unwind branch June 19, 2026 14:05
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants