From b2e24e0c924b6dd51e4aea3af7daeaa17787e624 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 8 Sep 2026 15:28:13 +0200 Subject: [PATCH 1/9] Extract panic hook into own module and catch background LSP panics --- crates/ark/src/console.rs | 1 - crates/ark/src/console/console_repl.rs | 30 +--- crates/ark/src/lib.rs | 1 + crates/ark/src/lsp/analysis/pool.rs | 13 +- crates/ark/src/lsp/io_pool.rs | 12 +- crates/ark/src/main.rs | 65 +-------- crates/ark/src/panic.rs | 183 +++++++++++++++++++++++++ 7 files changed, 200 insertions(+), 105 deletions(-) create mode 100644 crates/ark/src/panic.rs diff --git a/crates/ark/src/console.rs b/crates/ark/src/console.rs index 317dfe88b..8610f02a8 100644 --- a/crates/ark/src/console.rs +++ b/crates/ark/src/console.rs @@ -126,7 +126,6 @@ pub(crate) use console_debug::FrameSource; use console_error::stack_overflow_occurred; use console_filter::strip_step_lines; use console_filter::ConsoleFilter; -pub use console_repl::catching_panics; pub(crate) use console_repl::console_inputs; pub(crate) use console_repl::r_busy; pub(crate) use console_repl::r_interrupt_events; diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index fc62b8cc4..fda0f216f 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -13,12 +13,13 @@ use std::path::Path; use std::rc::Rc; -use stdext::panic_message; use stdext::DebugRefCell; use super::*; use crate::dap::dap_notebook; use crate::data_explorer::r_data_explorer::POSITRON_DATA_EXPLORER_MIME; +use crate::panic; +use crate::panic::Recovery; use crate::r_task::QueuedRTask; use crate::r_task::RTask; use crate::r_task::TryIdleTask; @@ -34,18 +35,6 @@ const DEBUG_COMMANDS: &[&str] = &["c", "cont", "f", "help", "n", "s", "where", " // These are not transient evals: they represent deliberate debugger navigation. const DEBUG_COMMANDS_CONTINUE: &[&str] = &["n", "f", "c", "cont", "Q"]; -thread_local! { - /// When `true`, the global panic hook should return early instead of - /// aborting, so that `catch_unwind` can catch the panic in `Console::with`. - static CATCHING_PANICS: Cell = const { Cell::new(false) }; -} - -/// Returns `true` when we are inside a `Console::with` catch boundary. -/// Checked by the global panic hook to decide whether to abort. -pub fn catching_panics() -> bool { - CATCHING_PANICS.get() -} - /// Used to wait for complete R startup in `Console::wait_initialized()` or /// check for it in `Console::is_initialized()`. /// @@ -740,20 +729,9 @@ impl Console { /// caught and converted to `anyhow::Error`, which `harp::register`'s /// `r_unwrap()` then surfaces as a clean R error. pub fn with(f: impl FnOnce(&Console) -> anyhow::Result) -> anyhow::Result { - if cfg!(debug_assertions) { - return f(Console::get()); - } - - CATCHING_PANICS.set(true); - let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(Console::get()))); - CATCHING_PANICS.set(false); - - match result { + match panic::catch_unwind(Recovery::ReleaseOnly, || f(Console::get())) { Ok(result) => result, - Err(panic) => { - let msg = panic_message(panic.as_ref()); - Err(anyhow!("Panic in Console callback: {msg}")) - }, + Err(msg) => Err(anyhow!("Panic in Console callback: {msg}")), } } diff --git a/crates/ark/src/lib.rs b/crates/ark/src/lib.rs index 02e367e42..f282d3b80 100644 --- a/crates/ark/src/lib.rs +++ b/crates/ark/src/lib.rs @@ -31,6 +31,7 @@ pub mod lsp; pub mod methods; pub mod modules; pub mod modules_utils; +pub mod panic; pub mod plots; pub mod r_task; pub mod repos; diff --git a/crates/ark/src/lsp/analysis/pool.rs b/crates/ark/src/lsp/analysis/pool.rs index 4181a3dec..5522252e7 100644 --- a/crates/ark/src/lsp/analysis/pool.rs +++ b/crates/ark/src/lsp/analysis/pool.rs @@ -6,19 +6,19 @@ // use std::collections::VecDeque; -use std::panic::AssertUnwindSafe; use std::sync::Arc; use std::sync::Condvar; use std::sync::Mutex; use std::sync::MutexGuard; use aether_path::FilePath; -use stdext::panic_message; use stdext::spawn; use super::catch_cancellation; use super::snapshot::WorldStateSnapshot; use crate::lsp; +use crate::panic; +use crate::panic::Recovery; /// Enough threads that a handful of open files all get diagnosed in parallel, /// few enough that they don't crowd out the main loop or the R session we share @@ -202,12 +202,9 @@ fn run_entry(entry: Entry) { return; } - let task = AssertUnwindSafe(|| catch_cancellation(|| run(snapshot))); - if let Err(err) = std::panic::catch_unwind(task) { - lsp::log_error!( - "An analysis task panicked: {msg}", - msg = panic_message(err.as_ref()) - ); + if let Err(msg) = panic::catch_unwind(Recovery::Always, || catch_cancellation(|| run(snapshot))) + { + lsp::log_error!("An analysis task panicked: {msg}"); } } diff --git a/crates/ark/src/lsp/io_pool.rs b/crates/ark/src/lsp/io_pool.rs index e2c579258..9b992fecf 100644 --- a/crates/ark/src/lsp/io_pool.rs +++ b/crates/ark/src/lsp/io_pool.rs @@ -5,13 +5,12 @@ // // -use std::panic::AssertUnwindSafe; - use crossbeam::channel::Sender; -use stdext::panic_message; use stdext::spawn_with_stack_size; use crate::lsp; +use crate::panic; +use crate::panic::Recovery; type Job = Box; @@ -54,10 +53,7 @@ impl IoPool { } fn run_job(job: Job) { - if let Err(err) = std::panic::catch_unwind(AssertUnwindSafe(job)) { - lsp::log_error!( - "An I/O job panicked: {msg}", - msg = panic_message(err.as_ref()) - ); + if let Err(msg) = panic::catch_unwind(Recovery::Always, job) { + lsp::log_error!("An I/O job panicked: {msg}"); } } diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index 59410b097..6dd23ca1b 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -7,15 +7,14 @@ #![allow(unused_unsafe)] -use std::cell::Cell; use std::env; use amalthea::kernel; use amalthea::kernel_spec::KernelSpec; use anyhow::Context; -use ark::console::catching_panics; use ark::console::SessionMode; use ark::logger; +use ark::panic; use ark::repos::DefaultRepos; use ark::signals::initialize_signal_block; use ark::start::start_kernel; @@ -23,13 +22,8 @@ use ark::traps::register_trap_handlers; use crossbeam::channel::unbounded; use harp::command::r_home_setup; use notify::Watcher; -use stdext::panic_message; use stdext::unwrap; -thread_local! { - pub static ON_R_THREAD: Cell = const { Cell::new(false) }; -} - fn print_usage() { println!("Ark {}, an R Kernel.", ark::BUILD_VERSION); print!( @@ -83,7 +77,7 @@ https://github.com/posit-dev/ark/blob/main/doc/configuration.md } fn main() -> anyhow::Result<()> { - ON_R_THREAD.set(true); + panic::mark_r_thread(); // Block signals in this thread (and any child threads). initialize_signal_block(); @@ -381,60 +375,7 @@ fn main() -> anyhow::Result<()> { String::from("--no-restore-data"), ]); - // This causes panics on background threads to propagate on the main - // thread. If we don't propagate a background thread panic, the program - // keeps running in an unstable state as all communications with this - // thread will error out or panic. - // https://stackoverflow.com/questions/35988775/how-can-i-cause-a-panic-on-a-thread-to-immediately-end-the-main-thread - // - // A better way to manage panics on background threads would be to ensure - // that we join all spawned threads up to the main thread. - let old_hook = std::panic::take_hook(); - std::panic::set_hook(Box::new(move |panic_info| { - let info = panic_info.payload(); - - let loc = if let Some(location) = panic_info.location() { - format!("In file '{}' at line {}:", location.file(), location.line(),) - } else { - String::from("No location information:") - }; - - let msg = panic_message(info); - - // Top-level-exec and try-catch errors already contain a backtrace - // for the R thread so don't repeat it if we see one. Only perform - // this check on the R thread because we do want other threads' - // backtraces if the panic occurred elsewhere. - let trace = if ON_R_THREAD.get() && msg.contains("\n{R_BACKTRACE_HEADER}\n") { - String::new() - } else { - format!("Backtrace:\n{}", std::backtrace::Backtrace::force_capture()) - }; - - log::error!("Panic! {loc} {msg}\n{trace}"); - - // `Console::with()` catches panics with `catch_unwind` in release - // builds. Return early so the catch handler can convert the panic - // to an `anyhow::Error`. The backtrace is already logged above. - if catching_panics() { - return; - } - - // We don't want the threads managed by a Tokio runtime to `abort()` the - // process since their panics are caught and handled in other ways. - // This escape hatch is a hack that will also be activated by other - // Tokio contexts than just the LSP. - if tokio::runtime::Handle::try_current().is_ok() { - return; - } - - // Give some time to flush log - log::logger().flush(); - std::thread::sleep(std::time::Duration::from_millis(250)); - - old_hook(panic_info); - std::process::abort(); - })); + panic::install(); let Some(connection_file) = connection_file else { return Err(anyhow::anyhow!( diff --git a/crates/ark/src/panic.rs b/crates/ark/src/panic.rs new file mode 100644 index 000000000..e25279186 --- /dev/null +++ b/crates/ark/src/panic.rs @@ -0,0 +1,183 @@ +// +// panic.rs +// +// Copyright (C) 2026 Posit Software, PBC. All rights reserved. +// +// + +//! Logs panics before `catch_unwind()` handles them. `catch_unwind()` prevents a +//! recovered panic from aborting the process. Kept outside `main.rs` so tests can +//! install it. + +use std::cell::Cell; + +use stdext::panic_message; + +/// Install the global panic hook. +/// +/// This causes panics on background threads to propagate on the main +/// thread. If we don't propagate a background thread panic, the program +/// keeps running in an unstable state as all communications with this +/// thread will error out or panic. +/// https://stackoverflow.com/questions/35988775/how-can-i-cause-a-panic-on-a-thread-to-immediately-end-the-main-thread +/// +/// Log panics and abort the process unless a recovery boundary or Tokio handles them. +pub fn install() { + let old_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |panic_info| { + let info = panic_info.payload(); + + let loc = if let Some(location) = panic_info.location() { + format!("In file '{}' at line {}:", location.file(), location.line(),) + } else { + String::from("No location information:") + }; + + let msg = panic_message(info); + + // Top-level-exec and try-catch errors already contain a backtrace + // for the R thread so don't repeat it if we see one. Only perform + // this check on the R thread because we do want other threads' + // backtraces if the panic occurred elsewhere. + let trace = if on_r_thread() && msg.contains("\n{R_BACKTRACE_HEADER}\n") { + String::new() + } else { + format!("Backtrace:\n{}", std::backtrace::Backtrace::force_capture()) + }; + + log::error!("Panic! {loc} {msg}\n{trace}"); + + // A boundary has a `catch_unwind()` waiting for this panic. The + // backtrace is already logged above. + if recovers_panic() { + // Return and let the panic continue unwinding to the catch site + return; + } + + // A current Tokio handle may be unrelated to the LSP, but Tokio captures task + // panics for its caller to handle. + if tokio::runtime::Handle::try_current().is_ok() { + return; + } + + // Leave time for the log sink to write the flushed panic before aborting. + log::logger().flush(); + std::thread::sleep(std::time::Duration::from_millis(250)); + + old_hook(panic_info); + std::process::abort(); + })); +} + +thread_local! { + static ON_R_THREAD: Cell = const { Cell::new(false) }; +} + +/// Mark the calling thread as the main R thread, so the panic hook knows +/// whether an R backtrace is already present in the panic message. +pub fn mark_r_thread() { + ON_R_THREAD.set(true); +} + +fn on_r_thread() -> bool { + ON_R_THREAD.get() +} + +/// Which builds a boundary recovers panics in. +#[derive(Clone, Copy)] +pub(crate) enum Recovery { + /// Recover in every build. + Always, + /// Recover in release builds only. A panic in an R callback aborts during + /// development instead of surfacing as an R error. + ReleaseOnly, +} + +thread_local! { + static BOUNDARY: Cell> = const { Cell::new(None) }; +} + +/// Whether a `catch_unwind()` boundary is waiting to recover this panic. +fn recovers_panic() -> bool { + match BOUNDARY.get() { + None => false, + Some(Recovery::Always) => true, + Some(Recovery::ReleaseOnly) => !cfg!(debug_assertions), + } +} + +/// Runs `f` inside a `catch_unwind()` boundary. `Err` carries the panic message, and +/// unwind safety is asserted on the caller's behalf. +pub(crate) fn catch_unwind(recovery: Recovery, f: impl FnOnce() -> T) -> Result { + let _boundary = catch_boundary(recovery); + std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) + .map_err(|payload| panic_message(payload.as_ref())) +} + +/// Guard that preserves a `catch_unwind()` recovery boundary for the panic hook. +/// +/// Restores the preceding flag in `Drop::drop()` so a nested boundary cannot disable an +/// outer boundary. +struct CatchBoundary { + previous: Option, +} + +impl Drop for CatchBoundary { + fn drop(&mut self) { + BOUNDARY.set(self.previous); + } +} + +/// Prevent the hook from aborting a panic handled by `catch_unwind()`. +fn catch_boundary(recovery: Recovery) -> CatchBoundary { + let previous = BOUNDARY.get(); + BOUNDARY.set(Some(recovery)); + CatchBoundary { previous } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_recovers_panic_false_without_boundary() { + assert!(!recovers_panic()); + } + + #[test] + fn test_recovers_panic_true_inside_always_boundary() { + let _boundary = catch_boundary(Recovery::Always); + assert!(recovers_panic()); + } + + #[test] + fn test_nested_always_boundary_recovers_inside_release_only() { + let _outer = catch_boundary(Recovery::ReleaseOnly); + { + let _inner = catch_boundary(Recovery::Always); + assert!(recovers_panic()); + } + assert_eq!(recovers_panic(), !cfg!(debug_assertions)); + } + + #[test] + fn test_dropping_boundary_leaves_none() { + { + let _boundary = catch_boundary(Recovery::Always); + assert!(recovers_panic()); + } + assert!(!recovers_panic()); + } + + #[test] + fn test_catch_unwind_passes_through_ok() { + let result = catch_unwind(Recovery::Always, || 1 + 1); + assert_eq!(result, Ok(2)); + } + + #[test] + fn test_catch_unwind_converts_panic_to_err() { + let result = catch_unwind(Recovery::Always, || panic!("oh no")); + assert_eq!(result, Err(String::from("oh no"))); + } +} From ef742de9c6000df6822badca51447a0e215baa65 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 8 Sep 2026 17:03:04 +0200 Subject: [PATCH 2/9] Fix panic handling in LSP loop --- crates/ark/src/lsp/analysis.rs | 2 +- crates/ark/src/lsp/backend.rs | 106 +++++--------------------- crates/ark/src/lsp/main_loop.rs | 128 ++++++++++++++++++++++++++------ crates/ark/src/panic.rs | 37 ++++++++- 4 files changed, 160 insertions(+), 113 deletions(-) diff --git a/crates/ark/src/lsp/analysis.rs b/crates/ark/src/lsp/analysis.rs index 6a372852e..7e94b945d 100644 --- a/crates/ark/src/lsp/analysis.rs +++ b/crates/ark/src/lsp/analysis.rs @@ -25,6 +25,6 @@ pub(crate) use snapshot::WorldStateSnapshot; pub(crate) use warmup::warm_workspace_index; /// Run `f`, swallowing a salsa cancellation as `None`. Any other panic propagates. -fn catch_cancellation(f: impl FnOnce() -> T) -> Option { +pub(crate) fn catch_cancellation(f: impl FnOnce() -> T) -> Option { salsa::Cancelled::catch(AssertUnwindSafe(f)).ok() } diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 3380112b5..67fcaa71b 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -10,7 +10,6 @@ use std::path::PathBuf; use std::sync::atomic::Ordering; use std::sync::Arc; -use std::time::Duration; use amalthea::comm::server_comm::ServerStartMessage; use amalthea::comm::server_comm::ServerStartedMessage; @@ -55,10 +54,6 @@ use crate::lsp::statement_range::StatementRangeParams; use crate::lsp::statement_range::StatementRangeResponse; use crate::r_task; -// This enum is useful for two things. First it allows us to distinguish a -// normal request failure from a crash. In the latter case we send a -// notification to the client so the user knows the LSP has crashed. -// // Once the LSP has crashed all requests respond with an error. This prevents // any handler from running while we process the message to shut down the // server. The `Disabled` enum variant is an indicator of this state. We could @@ -67,13 +62,12 @@ use crate::r_task; #[expect(clippy::large_enum_variant)] pub(crate) enum RequestResponse { Disabled, - Crashed(anyhow::Error), Result(LspResult), } // Based on https://stackoverflow.com/a/69324393/1725177 macro_rules! cast_response { - ($self:expr, $target:expr, $pat:path) => {{ + ($target:expr, $pat:path) => {{ match $target { RequestResponse::Result(Ok($pat(resp))) => Ok(resp), RequestResponse::Result(Ok(_)) => { @@ -88,15 +82,6 @@ macro_rules! cast_response { // is set, which then surfaces in the client's error popup. LspError::Anyhow(err) => Err(new_jsonrpc_error(format!("{err}"))), }, - RequestResponse::Crashed(err) => { - // Notify user that the LSP has crashed and is no longer active - report_crash($self.client()).await; - - // The backtrace is reported via `err` and eventually shows up - // in the LSP logs on the client side - let _ = $self.shutdown_tx.send(()).await; - Err(new_jsonrpc_error(format!("{err:?}"))) - }, RequestResponse::Disabled => Err(new_jsonrpc_error(String::from( "The LSP server has crashed and is now shut down!", ))), @@ -104,36 +89,6 @@ macro_rules! cast_response { }}; } -/// Send via `request::ShowMessageRequest` not `notification::ShowMessage` so that we can -/// ensure that the message has been received on the frontend side. We are about to shut -/// the LSP down, and sending out a fire-and-forget notification often won't get sent out -/// before shutdown occurs. The request returns control to us when the user acknowledges -/// the message. It doesn't matter if that takes awhile because we shut down right after, -/// and we've already flipped the `LSP_HAS_CRASHED` global flag, but we do bound it with -/// a 5 second timeout just in case the user ignores the message entirely, so we can still -/// shutdown. -async fn report_crash(client: &Client) { - let user_message = concat!( - "The R language server has crashed and has been disabled. ", - "Smart features such as completions will no longer work in this session. ", - "Please report this crash to https://github.com/posit-dev/positron/issues ", - "with full logs (see https://positron.posit.co/troubleshooting.html#python-and-r-logs)." - ); - let request = client.send_request::(ShowMessageRequestParams { - typ: MessageType::ERROR, - message: String::from(user_message), - actions: None, - }); - match tokio::time::timeout(Duration::from_secs(5), request).await { - Ok(result) => { - result.log_err(); - }, - Err(_) => { - log::warn!("Timed out waiting for frontend to acknowledge LSP crash notification"); - }, - } -} - #[derive(Debug)] #[expect(clippy::large_enum_variant)] pub(crate) enum LspMessage { @@ -240,16 +195,9 @@ impl From for LspError { #[derive(Debug)] struct Backend { - /// Shutdown notifier used to unwind tower-lsp and disconnect from the - /// client when an LSP handler panics. - shutdown_tx: tokio::sync::mpsc::Sender<()>, - /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, - /// Copy of the Client, for reporting crash messages. - client: Client, - /// Handle to the LSP loops. Drop it to shut the loops down and drop all /// owned state. _main_loop: LoopHandles, @@ -264,30 +212,36 @@ impl Backend { let (response_tx, mut response_rx) = tokio_unbounded_channel::(); // Relay request to main loop - self.events_tx + if self + .events_tx .send(Event::Lsp(LspMessage::Request(request, response_tx))) - .unwrap(); + .is_err() + { + return RequestResponse::Disabled; + } // Wait for response from main loop - response_rx.recv().await.unwrap() + match response_rx.recv().await { + Some(response) => response, + None => RequestResponse::Disabled, + } } fn notify(&self, notif: LspNotification) { // Relay notification to main loop - self.events_tx + if self + .events_tx .send(Event::Lsp(LspMessage::Notification(notif))) - .unwrap(); - } - - fn client(&self) -> &Client { - &self.client + .is_err() + { + log::error!("Can't relay notification, the main loop is gone"); + } } } impl LanguageServer for Backend { async fn initialize(&self, params: InitializeParams) -> Result { cast_response!( - self, self.request(LspRequest::Initialize(params)).await, LspResponse::Initialize ) @@ -320,7 +274,6 @@ impl LanguageServer for Backend { params: WorkspaceSymbolParams, ) -> Result> { let info: Option> = cast_response!( - self, self.request(LspRequest::WorkspaceSymbol(params)).await, LspResponse::WorkspaceSymbol )?; @@ -332,7 +285,6 @@ impl LanguageServer for Backend { params: DocumentSymbolParams, ) -> Result> { cast_response!( - self, self.request(LspRequest::DocumentSymbol(params)).await, LspResponse::DocumentSymbol ) @@ -340,7 +292,6 @@ impl LanguageServer for Backend { async fn folding_range(&self, params: FoldingRangeParams) -> Result>> { cast_response!( - self, self.request(LspRequest::FoldingRange(params)).await, LspResponse::FoldingRange ) @@ -351,7 +302,6 @@ impl LanguageServer for Backend { params: ExecuteCommandParams, ) -> jsonrpc::Result> { cast_response!( - self, self.request(LspRequest::ExecuteCommand(params)).await, LspResponse::ExecuteCommand ) @@ -375,7 +325,6 @@ impl LanguageServer for Backend { async fn completion(&self, params: CompletionParams) -> Result> { cast_response!( - self, self.request(LspRequest::Completion(params)).await, LspResponse::Completion ) @@ -383,7 +332,6 @@ impl LanguageServer for Backend { async fn completion_resolve(&self, item: CompletionItem) -> Result { cast_response!( - self, self.request(LspRequest::CompletionResolve(item)).await, LspResponse::CompletionResolve ) @@ -391,7 +339,6 @@ impl LanguageServer for Backend { async fn hover(&self, params: HoverParams) -> Result> { cast_response!( - self, self.request(LspRequest::Hover(params)).await, LspResponse::Hover ) @@ -399,7 +346,6 @@ impl LanguageServer for Backend { async fn signature_help(&self, params: SignatureHelpParams) -> Result> { cast_response!( - self, self.request(LspRequest::SignatureHelp(params)).await, LspResponse::SignatureHelp ) @@ -410,7 +356,6 @@ impl LanguageServer for Backend { params: GotoDefinitionParams, ) -> Result> { cast_response!( - self, self.request(LspRequest::GotoDefinition(params)).await, LspResponse::GotoDefinition ) @@ -421,7 +366,6 @@ impl LanguageServer for Backend { params: GotoImplementationParams, ) -> Result> { cast_response!( - self, self.request(LspRequest::GotoImplementation(params)).await, LspResponse::GotoImplementation ) @@ -432,7 +376,6 @@ impl LanguageServer for Backend { params: SelectionRangeParams, ) -> Result>> { cast_response!( - self, self.request(LspRequest::SelectionRange(params)).await, LspResponse::SelectionRange ) @@ -440,7 +383,6 @@ impl LanguageServer for Backend { async fn references(&self, params: ReferenceParams) -> Result>> { cast_response!( - self, self.request(LspRequest::References(params)).await, LspResponse::References ) @@ -451,7 +393,6 @@ impl LanguageServer for Backend { params: TextDocumentPositionParams, ) -> Result> { cast_response!( - self, self.request(LspRequest::PrepareRename(params)).await, LspResponse::PrepareRename ) @@ -459,7 +400,6 @@ impl LanguageServer for Backend { async fn rename(&self, params: RenameParams) -> Result> { cast_response!( - self, self.request(LspRequest::Rename(params)).await, LspResponse::Rename ) @@ -470,7 +410,6 @@ impl LanguageServer for Backend { params: DocumentOnTypeFormattingParams, ) -> Result>> { cast_response!( - self, self.request(LspRequest::OnTypeFormatting(params)).await, LspResponse::OnTypeFormatting ) @@ -478,7 +417,6 @@ impl LanguageServer for Backend { async fn code_action(&self, params: CodeActionParams) -> Result> { cast_response!( - self, self.request(LspRequest::CodeAction(params)).await, LspResponse::CodeAction ) @@ -506,7 +444,6 @@ impl Backend { params: StatementRangeParams, ) -> jsonrpc::Result> { cast_response!( - self, self.request(LspRequest::StatementRange(params)).await, LspResponse::StatementRange ) @@ -517,7 +454,6 @@ impl Backend { params: HelpTopicParams, ) -> jsonrpc::Result> { cast_response!( - self, self.request(LspRequest::HelpTopic(params)).await, LspResponse::HelpTopic ) @@ -528,7 +464,6 @@ impl Backend { params: VirtualDocumentParams, ) -> tower_lsp_server::jsonrpc::Result { cast_response!( - self, self.request(LspRequest::VirtualDocument(params)).await, LspResponse::VirtualDocument ) @@ -539,7 +474,6 @@ impl Backend { params: InputBoundariesParams, ) -> tower_lsp_server::jsonrpc::Result { cast_response!( - self, self.request(LspRequest::InputBoundaries(params)).await, LspResponse::InputBoundaries ) @@ -590,11 +524,11 @@ pub(crate) fn start_lsp( let (shutdown_tx, mut shutdown_rx) = tokio::sync::mpsc::channel::<()>(1); let init = |client: Client| { - let state = GlobalState::new(client.clone(), r_home, console_notification_tx); + let state = GlobalState::new(client, r_home, console_notification_tx); let events_tx = state.events_tx(); // Start main loop and hold onto the handle that keeps it alive - let main_loop = state.start(); + let main_loop = state.start(shutdown_tx); // Forward event channel along to `Console`. // This also updates an outdated channel after a reconnect. @@ -610,9 +544,7 @@ pub(crate) fn start_lsp( }); Backend { - shutdown_tx, events_tx, - client, _main_loop: main_loop, } }; diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index cf1f0658f..bbd02b4fe 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -13,6 +13,7 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use std::sync::Arc; use std::sync::RwLock; +use std::time::Duration; use aether_path::FilePath; use anyhow::anyhow; @@ -22,16 +23,18 @@ use oak_scan::DbScan; use oak_scan::ScanCompleted; use oak_scan::ScanRequest; use oak_scan::ScanScheduler; -use stdext::panic_message; use stdext::result::ResultExt; use stdext::spawn; use tokio::runtime::Handle; use tokio::sync::mpsc::unbounded_channel as tokio_unbounded_channel; +use tokio::sync::mpsc::Sender; use tokio::sync::oneshot; use tower_lsp_server::jsonrpc; use tower_lsp_server::ls_types as lsp_types; +use tower_lsp_server::ls_types::request; use tower_lsp_server::ls_types::Diagnostic; use tower_lsp_server::ls_types::MessageType; +use tower_lsp_server::ls_types::ShowMessageRequestParams; use tower_lsp_server::ls_types::Uri; use tower_lsp_server::Client; @@ -39,6 +42,7 @@ use super::backend::RequestResponse; use crate::console::ConsoleNotification; use crate::lsp; use crate::lsp::analysis; +use crate::lsp::analysis::catch_cancellation; use crate::lsp::analysis::AnalysisPool; use crate::lsp::analysis::DiagnosticsReady; use crate::lsp::analysis::DiagnosticsState; @@ -64,6 +68,8 @@ use crate::lsp::state_handlers; use crate::lsp::state_handlers::ConsoleInputs; use crate::lsp::traits::url::UriExt; use crate::lsp::watchdog::Watchdog; +use crate::panic; +use crate::panic::Recovery; pub(crate) type TokioUnboundedSender = tokio::sync::mpsc::UnboundedSender; pub(crate) type TokioUnboundedReceiver = tokio::sync::mpsc::UnboundedReceiver; @@ -329,7 +335,7 @@ impl GlobalState { /// /// The returned [`LoopHandles`] owns everything the loops need. Drop it to /// shut the loops down and release the owned state. - pub(crate) fn start(self) -> LoopHandles { + pub(crate) fn start(self, server_shutdown_tx: Sender<()>) -> LoopHandles { let mut aux = tokio::task::JoinSet::<()>::new(); // The auxiliary loop is fully async and never blocks. Must be started @@ -346,8 +352,20 @@ impl GlobalState { // thread that we're in control of. let (shutdown_tx, shutdown_rx) = oneshot::channel(); let handle = Handle::current(); + let main_loop = spawn!("oak-main-loop", move || { - handle.block_on(self.main_loop(shutdown_rx)); + let outcome = panic::catch_unwind(Recovery::Always, { + let server_shutdown_tx = server_shutdown_tx.clone(); + move || handle.block_on(self.main_loop(shutdown_rx, server_shutdown_tx)) + }); + + // Handle panics that bypass `handle_event()`'s recovery boundary. + // Use `try_send()` because this thread's Tokio runtime may already be gone. + if let Err(msg) = outcome { + lsp::log_error!("Panic in the main loop: {msg}"); + LSP_HAS_CRASHED.store(true, Ordering::Release); + server_shutdown_tx.try_send(()).log_err(); + } }); LoopHandles { @@ -361,7 +379,11 @@ impl GlobalState { /// /// This takes ownership of all global state and handles one by one LSP /// requests, notifications, and other internal events. - async fn main_loop(mut self, mut shutdown_rx: oneshot::Receiver<()>) { + async fn main_loop( + mut self, + mut shutdown_rx: oneshot::Receiver<()>, + server_shutdown_tx: Sender<()>, + ) { loop { tokio::select! { _ = &mut shutdown_rx => { @@ -373,8 +395,22 @@ impl GlobalState { lsp::log_info!("Main loop stopping: event channel closed"); break; }; - if let Err(err) = self.handle_event(event).await { - lsp::log_error!("Failure while handling event:\n{err:?}") + + let outcome = + panic::catch_unwind_async(Recovery::Always, self.handle_event(event)).await; + + match outcome { + Ok(Ok(())) => {}, + Ok(Err(err)) => lsp::log_error!("Failure while handling event:\n{err:?}"), + Err(msg) => { + // `report_crash()` reads only the `Client` handle. Drop `self` after the + // panic because a handler may have partially written its state. + lsp::log_error!("Panic while handling event: {msg}"); + LSP_HAS_CRASHED.store(true, Ordering::Release); + report_crash(&self.client).await; + let _ = server_shutdown_tx.send(()).await; + break; + }, } } } @@ -458,7 +494,7 @@ impl GlobalState { match request { LspRequest::Initialize(params) => { - respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world, &self.events_tx), LspResponse::Initialize)?; + respond_exclusive(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world, &self.events_tx), LspResponse::Initialize)?; }, LspRequest::WorkspaceSymbol(params) => { respond(tx, || handlers::handle_symbol(params, &self.world), LspResponse::WorkspaceSymbol)?; @@ -679,6 +715,37 @@ impl GlobalState { } } +/// Send via `request::ShowMessageRequest` not `notification::ShowMessage` so +/// that we can ensure that the message has been received on the frontend side. +/// We are about to shut the LSP down, and sending out a fire-and-forget +/// notification often won't get sent out before shutdown occurs. The request +/// returns control to us when the user acknowledges the message. It doesn't +/// matter if that takes awhile because we shut down right after, and we've +/// already flipped the `LSP_HAS_CRASHED` global flag. We do bound it with a 5 +/// second timeout just in case the user ignores the message entirely, so we can +/// still shutdown. +async fn report_crash(client: &Client) { + let user_message = concat!( + "The R language server has crashed and has been disabled. ", + "Smart features such as completions will no longer work in this session. ", + "Please report this crash to https://github.com/posit-dev/positron/issues ", + "with full logs (see https://positron.posit.co/troubleshooting.html#python-and-r-logs)." + ); + let request = client.send_request::(ShowMessageRequestParams { + typ: MessageType::ERROR, + message: String::from(user_message), + actions: None, + }); + match tokio::time::timeout(Duration::from_secs(5), request).await { + Ok(result) => { + result.log_err(); + }, + Err(_) => { + log::warn!("Timed out waiting for frontend to acknowledge LSP crash notification"); + }, + } +} + /// Build the LSP's [`SourceHandler`], or `None` to disable source fetching fn source_handler(r_home: &Path) -> Option> { // Also reported to the LSP output channel from `handle_initialized()`. @@ -809,28 +876,46 @@ fn respond( response: impl FnOnce() -> LspResult, into_lsp_response: impl FnOnce(T) -> LspResponse, ) -> anyhow::Result<()> { - let response = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(response)) { - Ok(Ok(t)) => RequestResponse::Result(Ok(into_lsp_response(t))), - Ok(Err(e)) => RequestResponse::Result(Err(e)), - Err(err) if err.downcast_ref::().is_some() => { + let response = match panic::catch_unwind(Recovery::Always, || catch_cancellation(response)) { + Ok(Some(Ok(value))) => RequestResponse::Result(Ok(into_lsp_response(value))), + Ok(Some(Err(err))) => RequestResponse::Result(Err(err)), + Ok(None) => { // A salsa write cancelled an oak query while the handler ran. // Report `ContentModified` so the client knows the content moved // under us and re-requests. RequestResponse::Result(Err(LspError::JsonRpc(jsonrpc::Error::content_modified()))) }, - Err(err) => { - // Set global crash flag to disable the LSP - LSP_HAS_CRASHED.store(true, Ordering::Release); + Err(msg) => RequestResponse::Result(Err(LspError::Anyhow(anyhow!( + "Panic while handling request: {msg}" + )))), + }; - let msg = panic_message(err.as_ref()); + send_response(response_tx, response) +} - // This creates an uninformative backtrace that is reported in the - // LSP logs. Note that the relevant backtrace is the one created by - // our panic hook and reported via the _kernel_ logs. - RequestResponse::Crashed(anyhow!("Panic occurred while handling request: {msg}")) - }, +/// Run a handler that holds an exclusive world-state borrow. +/// +/// We don't recover from panics in these handlers because they could leave +/// leave the world state half-built. Let them reach `main_loop()`'s recovery +/// boundary, which terminates the session. +fn respond_exclusive( + response_tx: TokioUnboundedSender, + response: impl FnOnce() -> LspResult, + into_lsp_response: impl FnOnce(T) -> LspResponse, +) -> anyhow::Result<()> { + let response = match catch_cancellation(response) { + Some(Ok(value)) => RequestResponse::Result(Ok(into_lsp_response(value))), + Some(Err(err)) => RequestResponse::Result(Err(err)), + None => RequestResponse::Result(Err(LspError::JsonRpc(jsonrpc::Error::content_modified()))), }; + send_response(response_tx, response) +} + +fn send_response( + response_tx: TokioUnboundedSender, + response: RequestResponse, +) -> anyhow::Result<()> { let out = match response { RequestResponse::Result(Ok(_)) => Ok(()), RequestResponse::Result(Err(ref error)) => { @@ -841,9 +926,6 @@ fn respond( lsp::log_info!("Error while handling request:\n{error:?}"); Ok(()) }, - RequestResponse::Crashed(ref error) => { - Err(anyhow!("Crashed while handling request:\n{error:?}")) - }, RequestResponse::Disabled => Err(anyhow!("Received impossible `Disabled` response state")), }; diff --git a/crates/ark/src/panic.rs b/crates/ark/src/panic.rs index e25279186..3fcd38a95 100644 --- a/crates/ark/src/panic.rs +++ b/crates/ark/src/panic.rs @@ -10,6 +10,9 @@ //! install it. use std::cell::Cell; +use std::future::Future; +use std::panic::AssertUnwindSafe; +use std::task::Poll; use stdext::panic_message; @@ -20,8 +23,6 @@ use stdext::panic_message; /// keeps running in an unstable state as all communications with this /// thread will error out or panic. /// https://stackoverflow.com/questions/35988775/how-can-i-cause-a-panic-on-a-thread-to-immediately-end-the-main-thread -/// -/// Log panics and abort the process unless a recovery boundary or Tokio handles them. pub fn install() { let old_hook = std::panic::take_hook(); std::panic::set_hook(Box::new(move |panic_info| { @@ -114,6 +115,26 @@ pub(crate) fn catch_unwind(recovery: Recovery, f: impl FnOnce() -> T) -> Resu .map_err(|payload| panic_message(payload.as_ref())) } +/// Recover panics while polling a future. Enter the recovery boundary for each poll so +/// unrelated work on the polling thread cannot inherit it. +pub(crate) async fn catch_unwind_async( + recovery: Recovery, + future: impl Future, +) -> Result { + let mut future = Box::pin(future); + + std::future::poll_fn(move |cx| { + let _boundary = catch_boundary(recovery); + + match std::panic::catch_unwind(AssertUnwindSafe(|| future.as_mut().poll(cx))) { + Ok(Poll::Pending) => Poll::Pending, + Ok(Poll::Ready(value)) => Poll::Ready(Ok(value)), + Err(payload) => Poll::Ready(Err(panic_message(payload.as_ref()))), + } + }) + .await +} + /// Guard that preserves a `catch_unwind()` recovery boundary for the panic hook. /// /// Restores the preceding flag in `Drop::drop()` so a nested boundary cannot disable an @@ -180,4 +201,16 @@ mod tests { let result = catch_unwind(Recovery::Always, || panic!("oh no")); assert_eq!(result, Err(String::from("oh no"))); } + + #[tokio::test] + async fn test_catch_unwind_async_passes_through_ok() { + let result = catch_unwind_async(Recovery::Always, async { 1 + 1 }).await; + assert_eq!(result, Ok(2)); + } + + #[tokio::test] + async fn test_catch_unwind_async_converts_panic_to_err() { + let result = catch_unwind_async(Recovery::Always, async { panic!("oh no") }).await; + assert_eq!(result, Err(String::from("oh no"))); + } } From 5839b6bfb3b8762a671abc4102a5428d787357d2 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 9 Sep 2026 10:09:21 +0200 Subject: [PATCH 3/9] Add integration tests for panic catching --- crates/ark/src/lsp/analysis/pool.rs | 23 +++++++++++++ crates/ark/src/lsp/backend.rs | 39 ++++++++++++++++++++-- crates/ark/src/lsp/io_pool.rs | 20 +++++++++++ crates/ark/src/lsp/main_loop.rs | 10 ++++++ crates/ark/tests/integration/lsp.rs | 51 ++++++++++++++++++++--------- crates/ark_test/src/lsp_client.rs | 2 +- 6 files changed, 125 insertions(+), 20 deletions(-) diff --git a/crates/ark/src/lsp/analysis/pool.rs b/crates/ark/src/lsp/analysis/pool.rs index 5522252e7..5bcb8a2f8 100644 --- a/crates/ark/src/lsp/analysis/pool.rs +++ b/crates/ark/src/lsp/analysis/pool.rs @@ -247,4 +247,27 @@ mod tests { .unwrap(); assert!(!ran.load(Ordering::Acquire)); } + + /// Install the production hook so a missing `catch_unwind()` aborts the + /// process instead of silently losing the worker panic. + #[test] + fn test_pool_survives_panicking_task() { + crate::panic::install(); + + let state = WorldState::default(); + let pool = AnalysisPool::with_threads(1); + + pool.spawn(state.snapshot(), |_snapshot| { + panic!("Test panic in an analysis task") + }); + + let (barrier_tx, barrier_rx) = std::sync::mpsc::channel(); + pool.spawn(state.snapshot(), move |_snapshot| { + barrier_tx.send(()).unwrap() + }); + + barrier_rx + .recv_timeout(std::time::Duration::from_secs(10)) + .unwrap(); + } } diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 67fcaa71b..37929364c 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -106,6 +106,8 @@ pub(crate) enum LspNotification { DidChangeTextDocument(DidChangeTextDocumentParams), DidSaveTextDocument(DidSaveTextDocumentParams), DidCloseTextDocument(DidCloseTextDocumentParams), + #[cfg(feature = "testing")] + TestPanic, } #[derive(Debug)] @@ -132,6 +134,8 @@ pub(crate) enum LspRequest { CodeAction(CodeActionParams), VirtualDocument(VirtualDocumentParams), InputBoundaries(InputBoundariesParams), + #[cfg(feature = "testing")] + TestPanic, } #[derive(Debug)] @@ -158,6 +162,8 @@ pub(crate) enum LspResponse { CodeAction(Option), VirtualDocument(VirtualDocumentResponse), InputBoundaries(InputBoundariesResponse), + #[cfg(feature = "testing")] + TestPanic(()), } pub(crate) type LspResult = std::result::Result; @@ -482,8 +488,26 @@ impl Backend { async fn notification(&self, params: Option) { log::info!("Received Positron notification: {:?}", params); } + + #[cfg(feature = "testing")] + async fn test_panic(&self, _params: Option) -> jsonrpc::Result<()> { + cast_response!( + self.request(LspRequest::TestPanic).await, + LspResponse::TestPanic + ) + } + + #[cfg(feature = "testing")] + async fn test_panic_notification(&self, _params: Option) { + self.notify(LspNotification::TestPanic); + } } +#[cfg(feature = "testing")] +pub(crate) static ARK_TEST_PANIC_REQUEST: &str = "ark/testPanic"; +#[cfg(feature = "testing")] +pub(crate) static ARK_TEST_PANIC_NOTIFICATION: &str = "ark/testPanicNotification"; + pub(crate) fn start_lsp( r_home: PathBuf, runtime: Arc, @@ -549,7 +573,7 @@ pub(crate) fn start_lsp( } }; - let (service, socket) = LspService::build(init) + let builder = LspService::build(init) .custom_method( statement_range::POSITRON_STATEMENT_RANGE_REQUEST, Backend::statement_range, @@ -561,8 +585,17 @@ pub(crate) fn start_lsp( input_boundaries::POSITRON_INPUT_BOUNDARIES_REQUEST, Backend::input_boundaries, ) - .custom_method("positron/notification", Backend::notification) - .finish(); + .custom_method("positron/notification", Backend::notification); + + #[cfg(feature = "testing")] + let builder = builder + .custom_method(ARK_TEST_PANIC_REQUEST, Backend::test_panic) + .custom_method( + ARK_TEST_PANIC_NOTIFICATION, + Backend::test_panic_notification, + ); + + let (service, socket) = builder.finish(); let server = Server::new(read, write, socket); diff --git a/crates/ark/src/lsp/io_pool.rs b/crates/ark/src/lsp/io_pool.rs index 9b992fecf..b66176a05 100644 --- a/crates/ark/src/lsp/io_pool.rs +++ b/crates/ark/src/lsp/io_pool.rs @@ -57,3 +57,23 @@ fn run_job(job: Job) { lsp::log_error!("An I/O job panicked: {msg}"); } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Install the production hook so a missing `catch_unwind()` aborts the + /// process instead of silently losing the worker panic. + #[test] + fn test_pool_survives_panicking_job() { + crate::panic::install(); + + let pool = IoPool::new("test-io-pool", 1, stdext::DEFAULT_STACK_SIZE); + pool.submit(|| panic!("Test panic in an I/O job")); + + let (tx, rx) = std::sync::mpsc::channel(); + pool.submit(move || tx.send(()).unwrap()); + + rx.recv_timeout(std::time::Duration::from_secs(10)).unwrap(); + } +} diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index bbd02b4fe..f613bbc96 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -486,6 +486,11 @@ impl GlobalState { LspNotification::DidCloseTextDocument(params) => { state_handlers::did_close(params, &mut self.world)?; }, + + #[cfg(feature = "testing")] + LspNotification::TestPanic => { + panic!("Test panic in a notification handler"); + }, } }, @@ -561,6 +566,11 @@ impl GlobalState { LspRequest::InputBoundaries(params) => { respond(tx, || handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)?; }, + + #[cfg(feature = "testing")] + LspRequest::TestPanic => { + respond(tx, || -> LspResult<()> { panic!("Test panic in a request handler") }, LspResponse::TestPanic)?; + }, }; }, }, diff --git a/crates/ark/tests/integration/lsp.rs b/crates/ark/tests/integration/lsp.rs index fe77ec2e3..ee5eb9c60 100644 --- a/crates/ark/tests/integration/lsp.rs +++ b/crates/ark/tests/integration/lsp.rs @@ -24,16 +24,11 @@ fn test_lsp_init() { assert!(lsp.server_capabilities().completion_provider.is_some()); } -// Reproduces https://github.com/posit-dev/ark/issues/1361: an abrupt client -// disconnect (TCP reset, e.g. the extension host being recycled) hits an -// `unreachable!()` in our pinned tower-lsp fork's transport and panics the -// `ark-lsp` thread. +// An abrupt client disconnect must not panic the `ark-lsp` thread. #[test] fn test_lsp_survives_abrupt_disconnect() { - // The panic happens on the `ark-lsp` thread, so a plain `#[should_panic]` - // or `catch_unwind` on this (the main test) thread can't see it. Install - // a hook that records panics by thread name instead, and chain to the - // previous hook so panic output still gets printed. + // Capture `ark-lsp` panics because `catch_unwind()` on this test thread + // cannot observe them. Chain the prior hook so the panic remains in test output. let panic_message: Arc>> = Arc::new(Mutex::new(None)); let panic_message_hook = Arc::clone(&panic_message); let previous_hook = std::panic::take_hook(); @@ -49,34 +44,58 @@ fn test_lsp_survives_abrupt_disconnect() { lsp.disconnect_abruptly(); - // Give the LSP thread a moment to observe the reset. + // Wait for the `ark-lsp` thread to observe the reset. std::thread::sleep(Duration::from_millis(200)); if let Some(message) = panic_message.lock().unwrap().take() { panic!("`ark-lsp` thread panicked on abrupt disconnect: {message}"); } - // The LSP should still be usable after a client disconnects abruptly. let lsp2 = frontend.start_lsp(); assert!(lsp2.server_capabilities().completion_provider.is_some()); } -// Reproduces https://github.com/ebkalderon/tower-lsp/issues/399 and #424: -// some clients send `exit` but never close their own end of the connection -// afterward. The server must hang up on its own rather than waiting forever -// for more input that will never arrive. +// `exit` must close the server side even if the client socket remains open. #[test] fn test_lsp_exits_promptly_after_exit_without_client_close() { let frontend = DummyArkFrontend::lock(); let mut lsp = frontend.start_lsp(); - // Sends `shutdown`/`exit` but deliberately leaves our end of the socket - // open, unlike the client's normal teardown on `Drop`. + // `shutdown()` sends `shutdown`/`exit` without closing the client socket. lsp.shutdown(); lsp.expect_server_closes_connection(Duration::from_secs(5)); } +// A panicking request handler must return an error without ending the LSP session. +#[test] +fn test_lsp_panicking_request_is_task_local() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + let message = lsp.send_request_expect_error("ark/testPanic", json!({})); + assert!(message.contains("Panic while handling request")); + + let uri = lsp.open_document("test_panic_request.R", "x <- 1\n"); + lsp.completions(&uri, 0, 0); +} + +// A notification handler panic must show a crash dialog before closing the LSP connection. +#[test] +fn test_lsp_panicking_notification_ends_session() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + lsp.send_notification("ark/testPanicNotification", json!({})); + + // Read `window/showMessageRequest` first because shutdown races it. + lsp.recv_server_request("window/showMessageRequest"); + lsp.expect_server_closes_connection(Duration::from_secs(5)); + + // Skip `shutdown()` because the server has already closed the connection. + lsp.disconnect_abruptly(); +} + // The two cases below test errors that don't depend on the rename // implementation's resolution capabilities. New-name validation always // applies (R language constraints), so these tests stay valid once diff --git a/crates/ark_test/src/lsp_client.rs b/crates/ark_test/src/lsp_client.rs index 41ac8bbfa..e5a81e955 100644 --- a/crates/ark_test/src/lsp_client.rs +++ b/crates/ark_test/src/lsp_client.rs @@ -399,7 +399,7 @@ impl LspClient { /// /// Skips benign server notifications. Panics on unexpected messages. #[track_caller] - fn recv_server_request(&mut self, expected_method: &str) { + pub fn recv_server_request(&mut self, expected_method: &str) { loop { match self.recv_any() { LspMessage::ServerRequest { id, method, params } => { From 86eea1f7442317704b254b302bb0a23bd9abbc05 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 9 Sep 2026 10:42:51 +0200 Subject: [PATCH 4/9] Add mechanism for expected concurrent log messages --- crates/ark/tests/integration/lsp.rs | 4 ++++ crates/ark_test/src/lsp_client.rs | 20 ++++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/ark/tests/integration/lsp.rs b/crates/ark/tests/integration/lsp.rs index ee5eb9c60..82217e3df 100644 --- a/crates/ark/tests/integration/lsp.rs +++ b/crates/ark/tests/integration/lsp.rs @@ -86,6 +86,10 @@ fn test_lsp_panicking_notification_ends_session() { let frontend = DummyArkFrontend::lock(); let mut lsp = frontend.start_lsp(); + // Allow the expected panic log because its auxiliary loop can deliver it + // before or after the crash dialog. + lsp.allow_log_message("Panic while handling event"); + lsp.send_notification("ark/testPanicNotification", json!({})); // Read `window/showMessageRequest` first because shutdown races it. diff --git a/crates/ark_test/src/lsp_client.rs b/crates/ark_test/src/lsp_client.rs index e5a81e955..247a68bc3 100644 --- a/crates/ark_test/src/lsp_client.rs +++ b/crates/ark_test/src/lsp_client.rs @@ -36,6 +36,8 @@ pub struct LspClient { server_capabilities: Option, /// Buffered diagnostics notifications, keyed by document URI diagnostics: std::collections::HashMap>, + /// Expected error and warning log message substrings. + allowed_log_messages: Vec, /// Set by `disconnect_abruptly()` so `Drop` skips the graceful `shutdown`/`exit` sequence killed: bool, } @@ -58,10 +60,17 @@ impl LspClient { open_documents: Vec::new(), server_capabilities: None, diagnostics: std::collections::HashMap::new(), + allowed_log_messages: Vec::new(), killed: false, }) } + /// Allow an expected server error or warning without hiding unexpected logs. + /// Only messages containing `substring` are ignored. + pub fn allow_log_message(&mut self, substring: &str) { + self.allowed_log_messages.push(substring.to_string()); + } + /// Sever the connection with a TCP reset instead of a graceful close. /// /// A zero `SO_LINGER` makes the close abortive, so the socket sends a `RST` @@ -481,7 +490,7 @@ impl LspClient { }, (false, true, false) => { - let diagnostics = Self::check_server_notification(&message); + let diagnostics = self.check_server_notification(&message); LspMessage::Notification { diagnostics } }, @@ -491,6 +500,7 @@ impl LspClient { /// Check a server notification, returning parsed diagnostics if applicable. fn check_server_notification( + &self, message: &serde_json::Map, ) -> Option { let method = message["method"].as_str().unwrap_or("unknown"); @@ -505,7 +515,13 @@ impl LspClient { let text = message["params"]["message"] .as_str() .unwrap_or("(no message)"); - panic!("LSP server {level}: {text}"); + if !self + .allowed_log_messages + .iter() + .any(|allowed| text.contains(allowed)) + { + panic!("LSP server {level}: {text}"); + } } None }, From 91286ad12cef7959481050320d028bc46912ffad Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 11 Sep 2026 11:37:16 +0200 Subject: [PATCH 5/9] Show toast notification when LSP handler panics --- crates/ark/src/lsp/backend.rs | 10 +- crates/ark/src/lsp/main_loop.rs | 143 +++++++++++++++++++++------- crates/ark/tests/integration/lsp.rs | 31 ++++++ crates/ark_test/src/lsp_client.rs | 36 ++++++- 4 files changed, 183 insertions(+), 37 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 37929364c..b877d62b9 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -501,12 +501,19 @@ impl Backend { async fn test_panic_notification(&self, _params: Option) { self.notify(LspNotification::TestPanic); } + + #[cfg(feature = "testing")] + async fn test_panic_main_loop(&self, _params: Option) { + let _ = self.events_tx.send(Event::TestPanicMainLoop); + } } #[cfg(feature = "testing")] pub(crate) static ARK_TEST_PANIC_REQUEST: &str = "ark/testPanic"; #[cfg(feature = "testing")] pub(crate) static ARK_TEST_PANIC_NOTIFICATION: &str = "ark/testPanicNotification"; +#[cfg(feature = "testing")] +pub(crate) static ARK_TEST_PANIC_MAIN_LOOP: &str = "ark/testPanicMainLoop"; pub(crate) fn start_lsp( r_home: PathBuf, @@ -593,7 +600,8 @@ pub(crate) fn start_lsp( .custom_method( ARK_TEST_PANIC_NOTIFICATION, Backend::test_panic_notification, - ); + ) + .custom_method(ARK_TEST_PANIC_MAIN_LOOP, Backend::test_panic_main_loop); let (service, socket) = builder.finish(); diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index f613bbc96..c59980b76 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -7,6 +7,8 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::mem::discriminant; +use std::mem::Discriminant; use std::path::Path; use std::path::PathBuf; use std::sync::atomic::AtomicBool; @@ -99,6 +101,8 @@ pub(crate) enum Event { OakScanCompleted(ScanCompleted), SourceCompleted(SourceCompleted), DiagnosticsReady(DiagnosticsReady), + #[cfg(feature = "testing")] + TestPanicMainLoop, } #[derive(Debug)] @@ -152,6 +156,9 @@ pub(crate) struct GlobalState { /// LSP client shared with tower-lsp and the log loop client: Client, + /// Request handlers whose panic has already been reported to the user. + reported_request_panics: HashSet>, + /// Event channels for the main loop. The tower-lsp methods forward /// notifications and requests here via `Event::Lsp`. We also receive /// messages from the kernel via `Event::Kernel`, and from ourselves via @@ -321,6 +328,7 @@ impl GlobalState { world, lsp_state, client, + reported_request_panics: HashSet::new(), events_tx, events_rx, } @@ -352,18 +360,28 @@ impl GlobalState { // thread that we're in control of. let (shutdown_tx, shutdown_rx) = oneshot::channel(); let handle = Handle::current(); + let client = self.client.clone(); let main_loop = spawn!("oak-main-loop", move || { let outcome = panic::catch_unwind(Recovery::Always, { let server_shutdown_tx = server_shutdown_tx.clone(); + let handle = handle.clone(); move || handle.block_on(self.main_loop(shutdown_rx, server_shutdown_tx)) }); // Handle panics that bypass `handle_event()`'s recovery boundary. - // Use `try_send()` because this thread's Tokio runtime may already be gone. if let Err(msg) = outcome { lsp::log_error!("Panic in the main loop: {msg}"); LSP_HAS_CRASHED.store(true, Ordering::Release); + + let report = panic::catch_unwind(Recovery::Always, || { + handle.block_on(report_crash(&client)) + }); + if let Err(msg) = report { + log::error!("Panic while reporting an LSP crash: {msg}"); + } + + // The runtime may be shutting down, so don't wait for channel capacity. server_shutdown_tx.try_send(()).log_err(); } }); @@ -396,6 +414,11 @@ impl GlobalState { break; }; + #[cfg(feature = "testing")] + if matches!(event, Event::TestPanicMainLoop) { + panic!("Test panic outside the event recovery boundary"); + } + let outcome = panic::catch_unwind_async(Recovery::Always, self.handle_event(event)).await; @@ -497,81 +520,88 @@ impl GlobalState { LspMessage::Request(request, tx) => { lsp::log_info!("{request:#?}"); - match request { + let request_kind = discriminant(&request); + let outcome = match request { LspRequest::Initialize(params) => { - respond_exclusive(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world, &self.events_tx), LspResponse::Initialize)?; + respond_exclusive(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world, &self.events_tx), LspResponse::Initialize)? }, LspRequest::WorkspaceSymbol(params) => { - respond(tx, || handlers::handle_symbol(params, &self.world), LspResponse::WorkspaceSymbol)?; + respond(tx, || handlers::handle_symbol(params, &self.world), LspResponse::WorkspaceSymbol)? }, LspRequest::DocumentSymbol(params) => { - respond(tx, || handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)?; + respond(tx, || handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)? }, LspRequest::FoldingRange(params) => { - respond(tx, || handlers::handle_folding_range(params, &self.world), LspResponse::FoldingRange)?; + respond(tx, || handlers::handle_folding_range(params, &self.world), LspResponse::FoldingRange)? }, LspRequest::ExecuteCommand(_params) => { let response = handlers::handle_execute_command(&self.client).await; - respond(tx, || response, LspResponse::ExecuteCommand)?; + respond(tx, || response, LspResponse::ExecuteCommand)? }, LspRequest::Completion(params) => { - respond(tx, || handlers::handle_completion(params, &self.world), LspResponse::Completion)?; + respond(tx, || handlers::handle_completion(params, &self.world), LspResponse::Completion)? }, LspRequest::CompletionResolve(params) => { - respond(tx, || handlers::handle_completion_resolve(params), LspResponse::CompletionResolve)?; + respond(tx, || handlers::handle_completion_resolve(params), LspResponse::CompletionResolve)? }, LspRequest::Hover(params) => { - respond(tx, || handlers::handle_hover(params, &self.world), LspResponse::Hover)?; + respond(tx, || handlers::handle_hover(params, &self.world), LspResponse::Hover)? }, LspRequest::SignatureHelp(params) => { - respond(tx, || handlers::handle_signature_help(params, &self.world), LspResponse::SignatureHelp)?; + respond(tx, || handlers::handle_signature_help(params, &self.world), LspResponse::SignatureHelp)? }, LspRequest::GotoDefinition(params) => { - respond(tx, || handlers::handle_goto_definition(params, &self.world), LspResponse::GotoDefinition)?; + respond(tx, || handlers::handle_goto_definition(params, &self.world), LspResponse::GotoDefinition)? }, LspRequest::GotoImplementation(_params) => { // TODO - respond(tx, || Ok(None), LspResponse::GotoImplementation)?; + respond(tx, || Ok(None), LspResponse::GotoImplementation)? }, LspRequest::SelectionRange(params) => { - respond(tx, || handlers::handle_selection_range(params, &self.world), LspResponse::SelectionRange)?; + respond(tx, || handlers::handle_selection_range(params, &self.world), LspResponse::SelectionRange)? }, LspRequest::References(params) => { - respond(tx, || handlers::handle_references(params, &self.world), LspResponse::References)?; + respond(tx, || handlers::handle_references(params, &self.world), LspResponse::References)? }, LspRequest::PrepareRename(params) => { - respond(tx, || handlers::handle_prepare_rename(params, &self.world), LspResponse::PrepareRename)?; + respond(tx, || handlers::handle_prepare_rename(params, &self.world), LspResponse::PrepareRename)? }, LspRequest::Rename(params) => { - respond(tx, || handlers::handle_rename(params, &self.world), LspResponse::Rename)?; + respond(tx, || handlers::handle_rename(params, &self.world), LspResponse::Rename)? }, LspRequest::StatementRange(params) => { - respond(tx, || handlers::handle_statement_range(params, &self.world), LspResponse::StatementRange)?; + respond(tx, || handlers::handle_statement_range(params, &self.world), LspResponse::StatementRange)? }, LspRequest::HelpTopic(params) => { - respond(tx, || handlers::handle_help_topic(params, &self.world), LspResponse::HelpTopic)?; + respond(tx, || handlers::handle_help_topic(params, &self.world), LspResponse::HelpTopic)? }, LspRequest::OnTypeFormatting(params) => { if let Some(path) = params.text_document_position.text_document.uri.to_document_path().log_err() { state_handlers::did_change_formatting_options(&path, ¶ms.options, &mut self.world); } - respond(tx, || handlers::handle_indent(params, &self.world), LspResponse::OnTypeFormatting)?; + respond(tx, || handlers::handle_indent(params, &self.world), LspResponse::OnTypeFormatting)? }, LspRequest::CodeAction(params) => { - respond(tx, || handlers::handle_code_action(params, &self.lsp_state, &self.world), LspResponse::CodeAction)?; + respond(tx, || handlers::handle_code_action(params, &self.lsp_state, &self.world), LspResponse::CodeAction)? }, LspRequest::VirtualDocument(params) => { - respond(tx, || handlers::handle_virtual_document(params, &self.world), LspResponse::VirtualDocument)?; + respond(tx, || handlers::handle_virtual_document(params, &self.world), LspResponse::VirtualDocument)? }, LspRequest::InputBoundaries(params) => { - respond(tx, || handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)?; + respond(tx, || handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)? }, #[cfg(feature = "testing")] LspRequest::TestPanic => { - respond(tx, || -> LspResult<()> { panic!("Test panic in a request handler") }, LspResponse::TestPanic)?; + respond(tx, || -> LspResult<()> { panic!("Test panic in a request handler") }, LspResponse::TestPanic)? }, }; + + if outcome == RequestOutcome::Panicked && + self.reported_request_panics.insert(request_kind) + { + report_request_panic(&self.client).await; + } }, }, @@ -669,6 +699,9 @@ impl GlobalState { ); } }, + + #[cfg(feature = "testing")] + Event::TestPanicMainLoop => unreachable!(), } lsp::log_info!("Finished handling event in {}ms", loop_tick.elapsed().as_millis()); @@ -756,6 +789,19 @@ async fn report_crash(client: &Client) { } } +async fn report_request_panic(client: &Client) { + client + .show_message( + MessageType::ERROR, + concat!( + "An R language server feature encountered an internal error. ", + "The request failed, but the language server is still running. ", + "See the R Kernel and R Language Server logs for the panic and backtrace." + ), + ) + .await; +} + /// Build the LSP's [`SourceHandler`], or `None` to disable source fetching fn source_handler(r_home: &Path) -> Option> { // Also reported to the LSP output channel from `handle_initialized()`. @@ -863,6 +909,12 @@ pub(super) fn dispatch_scan_requests( } } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum RequestOutcome { + Handled, + Panicked, +} + /// Respond to a request from the LSP /// /// We receive requests from the LSP client with a response channel. Once we @@ -885,22 +937,42 @@ fn respond( response_tx: TokioUnboundedSender, response: impl FnOnce() -> LspResult, into_lsp_response: impl FnOnce(T) -> LspResponse, -) -> anyhow::Result<()> { - let response = match panic::catch_unwind(Recovery::Always, || catch_cancellation(response)) { - Ok(Some(Ok(value))) => RequestResponse::Result(Ok(into_lsp_response(value))), - Ok(Some(Err(err))) => RequestResponse::Result(Err(err)), +) -> anyhow::Result { + let (response, outcome) = match panic::catch_unwind(Recovery::Always, || { + catch_cancellation(response) + }) { + Ok(Some(Ok(value))) => ( + RequestResponse::Result(Ok(into_lsp_response(value))), + RequestOutcome::Handled, + ), + Ok(Some(Err(err))) => (RequestResponse::Result(Err(err)), RequestOutcome::Handled), Ok(None) => { // A salsa write cancelled an oak query while the handler ran. // Report `ContentModified` so the client knows the content moved // under us and re-requests. - RequestResponse::Result(Err(LspError::JsonRpc(jsonrpc::Error::content_modified()))) + ( + RequestResponse::Result(Err(LspError::JsonRpc(jsonrpc::Error::content_modified()))), + RequestOutcome::Handled, + ) + }, + Err(msg) => { + // The panic hook emits the backtrace to the kernel logs. Mention + // the panic in the LSP log too for cross-reference. + lsp::log_error!( + "Panic while handling request: {msg}. \ + See the R kernel log for the full panic backtrace." + ); + ( + RequestResponse::Result(Err(LspError::Anyhow(anyhow!( + "Panic while handling request: {msg}" + )))), + RequestOutcome::Panicked, + ) }, - Err(msg) => RequestResponse::Result(Err(LspError::Anyhow(anyhow!( - "Panic while handling request: {msg}" - )))), }; - send_response(response_tx, response) + send_response(response_tx, response)?; + Ok(outcome) } /// Run a handler that holds an exclusive world-state borrow. @@ -912,14 +984,15 @@ fn respond_exclusive( response_tx: TokioUnboundedSender, response: impl FnOnce() -> LspResult, into_lsp_response: impl FnOnce(T) -> LspResponse, -) -> anyhow::Result<()> { +) -> anyhow::Result { let response = match catch_cancellation(response) { Some(Ok(value)) => RequestResponse::Result(Ok(into_lsp_response(value))), Some(Err(err)) => RequestResponse::Result(Err(err)), None => RequestResponse::Result(Err(LspError::JsonRpc(jsonrpc::Error::content_modified()))), }; - send_response(response_tx, response) + send_response(response_tx, response)?; + Ok(RequestOutcome::Handled) } fn send_response( diff --git a/crates/ark/tests/integration/lsp.rs b/crates/ark/tests/integration/lsp.rs index 82217e3df..a268d8264 100644 --- a/crates/ark/tests/integration/lsp.rs +++ b/crates/ark/tests/integration/lsp.rs @@ -73,11 +73,28 @@ fn test_lsp_panicking_request_is_task_local() { let frontend = DummyArkFrontend::lock(); let mut lsp = frontend.start_lsp(); + lsp.allow_log_message("Panic while handling request"); + + let message = lsp.send_request_expect_error("ark/testPanic", json!({})); + assert!(message.contains("Panic while handling request")); + + let toast = lsp.recv_show_message(); + assert_eq!( + toast, + concat!( + "An R language server feature encountered an internal error. ", + "The request failed, but the language server is still running. ", + "See the R Kernel and R Language Server logs for the panic and backtrace." + ) + ); + + // The same handler still logs and returns an error, but doesn't repeat its toast. let message = lsp.send_request_expect_error("ark/testPanic", json!({})); assert!(message.contains("Panic while handling request")); let uri = lsp.open_document("test_panic_request.R", "x <- 1\n"); lsp.completions(&uri, 0, 0); + assert!(lsp.show_messages().is_empty()); } // A notification handler panic must show a crash dialog before closing the LSP connection. @@ -100,6 +117,20 @@ fn test_lsp_panicking_notification_ends_session() { lsp.disconnect_abruptly(); } +// A panic outside the per-event boundary must still show the crash dialog before shutdown. +#[test] +fn test_lsp_panicking_main_loop_reports_crash() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + lsp.allow_log_message("Panic in the main loop"); + lsp.send_notification("ark/testPanicMainLoop", json!({})); + + lsp.recv_server_request("window/showMessageRequest"); + lsp.expect_server_closes_connection(Duration::from_secs(5)); + lsp.disconnect_abruptly(); +} + // The two cases below test errors that don't depend on the rename // implementation's resolution capabilities. New-name validation always // applies (R language constraints), so these tests stay valid once diff --git a/crates/ark_test/src/lsp_client.rs b/crates/ark_test/src/lsp_client.rs index 247a68bc3..44c7c52aa 100644 --- a/crates/ark_test/src/lsp_client.rs +++ b/crates/ark_test/src/lsp_client.rs @@ -36,6 +36,8 @@ pub struct LspClient { server_capabilities: Option, /// Buffered diagnostics notifications, keyed by document URI diagnostics: std::collections::HashMap>, + /// Buffered `window/showMessage` notifications. + show_messages: Vec, /// Expected error and warning log message substrings. allowed_log_messages: Vec, /// Set by `disconnect_abruptly()` so `Drop` skips the graceful `shutdown`/`exit` sequence @@ -60,6 +62,7 @@ impl LspClient { open_documents: Vec::new(), server_capabilities: None, diagnostics: std::collections::HashMap::new(), + show_messages: Vec::new(), allowed_log_messages: Vec::new(), killed: false, }) @@ -71,6 +74,30 @@ impl LspClient { self.allowed_log_messages.push(substring.to_string()); } + /// Receive the next `window/showMessage` notification. Buffers diagnostics. + #[track_caller] + pub fn recv_show_message(&mut self) -> String { + loop { + if !self.show_messages.is_empty() { + return self.show_messages.remove(0); + } + + match self.recv_any() { + LspMessage::Notification { diagnostics } => { + if let Some(params) = diagnostics { + self.diagnostics.insert(params.uri, params.diagnostics); + } + }, + other => panic!("Expected `window/showMessage`, got: {other:?}"), + } + } + } + + /// Buffered `window/showMessage` notifications consumed while awaiting other messages. + pub fn show_messages(&self) -> &[String] { + &self.show_messages + } + /// Sever the connection with a TCP reset instead of a graceful close. /// /// A zero `SO_LINGER` makes the close abortive, so the socket sends a `RST` @@ -500,7 +527,7 @@ impl LspClient { /// Check a server notification, returning parsed diagnostics if applicable. fn check_server_notification( - &self, + &mut self, message: &serde_json::Map, ) -> Option { let method = message["method"].as_str().unwrap_or("unknown"); @@ -525,6 +552,13 @@ impl LspClient { } None }, + "window/showMessage" => { + let text = message["params"]["message"] + .as_str() + .unwrap_or("(no message)"); + self.show_messages.push(text.to_string()); + None + }, "textDocument/publishDiagnostics" => { let params: lsp_types::PublishDiagnosticsParams = serde_json::from_value(message["params"].clone()) From 836d2b39a61a4afaa5b726e304a662104ed560c8 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 11 Sep 2026 14:09:45 +0200 Subject: [PATCH 6/9] Preserve Salsa cancellations --- crates/ark/src/console/console_repl.rs | 5 +- crates/ark/src/lsp/analysis/pool.rs | 6 +- crates/ark/src/lsp/backend.rs | 22 ++++++- crates/ark/src/lsp/io_pool.rs | 5 +- crates/ark/src/lsp/main_loop.rs | 83 +++++++++++++++++++++----- crates/ark/src/panic.rs | 63 ++++++++++++++----- crates/ark/src/r_task.rs | 27 ++++++--- crates/ark/tests/integration/lsp.rs | 29 +++++++++ 8 files changed, 197 insertions(+), 43 deletions(-) diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index fda0f216f..6516c51a3 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -731,7 +731,10 @@ impl Console { pub fn with(f: impl FnOnce(&Console) -> anyhow::Result) -> anyhow::Result { match panic::catch_unwind(Recovery::ReleaseOnly, || f(Console::get())) { Ok(result) => result, - Err(msg) => Err(anyhow!("Panic in Console callback: {msg}")), + Err(payload) => { + let message = panic::message(&payload); + Err(anyhow!("Panic in Console callback: {message}")) + }, } } diff --git a/crates/ark/src/lsp/analysis/pool.rs b/crates/ark/src/lsp/analysis/pool.rs index 5bcb8a2f8..f107a7623 100644 --- a/crates/ark/src/lsp/analysis/pool.rs +++ b/crates/ark/src/lsp/analysis/pool.rs @@ -202,9 +202,11 @@ fn run_entry(entry: Entry) { return; } - if let Err(msg) = panic::catch_unwind(Recovery::Always, || catch_cancellation(|| run(snapshot))) + if let Err(payload) = + panic::catch_unwind(Recovery::Always, || catch_cancellation(|| run(snapshot))) { - lsp::log_error!("An analysis task panicked: {msg}"); + let message = panic::message(&payload); + lsp::log_error!("An analysis task panicked: {message}"); } } diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index b877d62b9..4a8862dc8 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -108,6 +108,10 @@ pub(crate) enum LspNotification { DidCloseTextDocument(DidCloseTextDocumentParams), #[cfg(feature = "testing")] TestPanic, + #[cfg(feature = "testing")] + TestCancelRTask, + #[cfg(feature = "testing")] + TestPanicRTask, } #[derive(Debug)] @@ -502,6 +506,16 @@ impl Backend { self.notify(LspNotification::TestPanic); } + #[cfg(feature = "testing")] + async fn test_cancel_r_task(&self, _params: Option) { + self.notify(LspNotification::TestCancelRTask); + } + + #[cfg(feature = "testing")] + async fn test_panic_r_task(&self, _params: Option) { + self.notify(LspNotification::TestPanicRTask); + } + #[cfg(feature = "testing")] async fn test_panic_main_loop(&self, _params: Option) { let _ = self.events_tx.send(Event::TestPanicMainLoop); @@ -514,6 +528,10 @@ pub(crate) static ARK_TEST_PANIC_REQUEST: &str = "ark/testPanic"; pub(crate) static ARK_TEST_PANIC_NOTIFICATION: &str = "ark/testPanicNotification"; #[cfg(feature = "testing")] pub(crate) static ARK_TEST_PANIC_MAIN_LOOP: &str = "ark/testPanicMainLoop"; +#[cfg(feature = "testing")] +pub(crate) static ARK_TEST_CANCEL_R_TASK: &str = "ark/testCancelRTask"; +#[cfg(feature = "testing")] +pub(crate) static ARK_TEST_PANIC_R_TASK: &str = "ark/testPanicRTask"; pub(crate) fn start_lsp( r_home: PathBuf, @@ -601,7 +619,9 @@ pub(crate) fn start_lsp( ARK_TEST_PANIC_NOTIFICATION, Backend::test_panic_notification, ) - .custom_method(ARK_TEST_PANIC_MAIN_LOOP, Backend::test_panic_main_loop); + .custom_method(ARK_TEST_PANIC_MAIN_LOOP, Backend::test_panic_main_loop) + .custom_method(ARK_TEST_CANCEL_R_TASK, Backend::test_cancel_r_task) + .custom_method(ARK_TEST_PANIC_R_TASK, Backend::test_panic_r_task); let (service, socket) = builder.finish(); diff --git a/crates/ark/src/lsp/io_pool.rs b/crates/ark/src/lsp/io_pool.rs index b66176a05..78dd39277 100644 --- a/crates/ark/src/lsp/io_pool.rs +++ b/crates/ark/src/lsp/io_pool.rs @@ -53,8 +53,9 @@ impl IoPool { } fn run_job(job: Job) { - if let Err(msg) = panic::catch_unwind(Recovery::Always, job) { - lsp::log_error!("An I/O job panicked: {msg}"); + if let Err(payload) = panic::catch_unwind(Recovery::Always, job) { + let message = panic::message(&payload); + lsp::log_error!("An I/O job panicked: {message}"); } } diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index c59980b76..870a3868a 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -370,15 +370,17 @@ impl GlobalState { }); // Handle panics that bypass `handle_event()`'s recovery boundary. - if let Err(msg) = outcome { - lsp::log_error!("Panic in the main loop: {msg}"); + if let Err(payload) = outcome { + let message = panic::message(&payload); + lsp::log_error!("Panic in the main loop: {message}"); LSP_HAS_CRASHED.store(true, Ordering::Release); let report = panic::catch_unwind(Recovery::Always, || { handle.block_on(report_crash(&client)) }); - if let Err(msg) = report { - log::error!("Panic while reporting an LSP crash: {msg}"); + if let Err(payload) = report { + let message = panic::message(&payload); + log::error!("Panic while reporting an LSP crash: {message}"); } // The runtime may be shutting down, so don't wait for channel capacity. @@ -425,14 +427,18 @@ impl GlobalState { match outcome { Ok(Ok(())) => {}, Ok(Err(err)) => lsp::log_error!("Failure while handling event:\n{err:?}"), - Err(msg) => { - // `report_crash()` reads only the `Client` handle. Drop `self` after the - // panic because a handler may have partially written its state. - lsp::log_error!("Panic while handling event: {msg}"); - LSP_HAS_CRASHED.store(true, Ordering::Release); - report_crash(&self.client).await; - let _ = server_shutdown_tx.send(()).await; - break; + Err(payload) => match classify_event_unwind(payload) { + EventUnwind::Cancelled => {}, + EventUnwind::Panicked(payload) => { + // `report_crash()` reads only the `Client` handle. Drop `self` after + // the panic because a handler may have partially written its state. + let message = panic::message(&payload); + lsp::log_error!("Panic while handling event: {message}"); + LSP_HAS_CRASHED.store(true, Ordering::Release); + report_crash(&self.client).await; + let _ = server_shutdown_tx.send(()).await; + break; + }, }, } } @@ -514,6 +520,18 @@ impl GlobalState { LspNotification::TestPanic => { panic!("Test panic in a notification handler"); }, + #[cfg(feature = "testing")] + LspNotification::TestCancelRTask => { + crate::r_task(|| { + std::panic::resume_unwind(Box::new( + salsa::Cancelled::PendingWrite, + )) + }); + }, + #[cfg(feature = "testing")] + LspNotification::TestPanicRTask => { + crate::r_task(|| panic!("Test panic in an R task")); + }, } }, @@ -892,6 +910,19 @@ impl GlobalState { } } +enum EventUnwind { + Cancelled, + Panicked(panic::PanicPayload), +} + +fn classify_event_unwind(payload: panic::PanicPayload) -> EventUnwind { + if payload.is::() { + EventUnwind::Cancelled + } else { + EventUnwind::Panicked(payload) + } +} + /// Run each [`ScanRequest`] on `pool`. Each job runs the pure-I/O /// [`ScanRequest::run`] and ships the [`ScanCompleted`] back to the main loop as /// [`Event::OakScanCompleted`], where the scheduler then applies it. @@ -955,16 +986,17 @@ fn respond( RequestOutcome::Handled, ) }, - Err(msg) => { + Err(payload) => { // The panic hook emits the backtrace to the kernel logs. Mention // the panic in the LSP log too for cross-reference. + let message = panic::message(&payload); lsp::log_error!( - "Panic while handling request: {msg}. \ + "Panic while handling request: {message}. \ See the R kernel log for the full panic backtrace." ); ( RequestResponse::Result(Err(LspError::Anyhow(anyhow!( - "Panic while handling request: {msg}" + "Panic while handling request: {message}" )))), RequestOutcome::Panicked, ) @@ -1223,14 +1255,35 @@ mod tests { use tower_lsp_server::jsonrpc; use url::Url; + use super::classify_event_unwind; use super::respond; use super::tokio_unbounded_channel; + use super::EventUnwind; use crate::lsp::backend::LspError; use crate::lsp::backend::LspResponse; use crate::lsp::backend::RequestResponse; use crate::lsp::state::WorldState; use crate::lsp::traits::url::UrlExt; + #[test] + fn test_salsa_cancellation_is_not_classified_as_event_panic() { + let payload = Box::new(salsa::Cancelled::PendingWrite); + let outcome = classify_event_unwind(payload); + + assert!(matches!(outcome, EventUnwind::Cancelled)); + } + + #[test] + fn test_genuine_panic_is_classified_as_event_panic() { + let payload = Box::new(String::from("oh no")); + let outcome = classify_event_unwind(payload); + + let EventUnwind::Panicked(payload) = outcome else { + panic!("Expected a panic"); + }; + assert_eq!(crate::panic::message(&payload), "oh no"); + } + /// A `salsa::Cancelled` re-raised out of a request handler (by `r_task`, /// after catching it on the R thread) must not crash the LSP. `respond` /// recognises the payload and answers `ContentModified` so the client diff --git a/crates/ark/src/panic.rs b/crates/ark/src/panic.rs index 3fcd38a95..d65c2a555 100644 --- a/crates/ark/src/panic.rs +++ b/crates/ark/src/panic.rs @@ -16,6 +16,8 @@ use std::task::Poll; use stdext::panic_message; +pub(crate) type PanicPayload = Box; + /// Install the global panic hook. /// /// This causes panics on background threads to propagate on the main @@ -99,7 +101,7 @@ thread_local! { } /// Whether a `catch_unwind()` boundary is waiting to recover this panic. -fn recovers_panic() -> bool { +pub(crate) fn recovers_panic() -> bool { match BOUNDARY.get() { None => false, Some(Recovery::Always) => true, @@ -107,20 +109,24 @@ fn recovers_panic() -> bool { } } -/// Runs `f` inside a `catch_unwind()` boundary. `Err` carries the panic message, and -/// unwind safety is asserted on the caller's behalf. -pub(crate) fn catch_unwind(recovery: Recovery, f: impl FnOnce() -> T) -> Result { +/// Runs `f` inside a `catch_unwind()` boundary. `Err` preserves the panic payload so +/// callers can distinguish control-flow unwinds such as `salsa::Cancelled` from genuine +/// panics. Unwind safety is asserted on the caller's behalf. +pub(crate) fn catch_unwind( + recovery: Recovery, + f: impl FnOnce() -> T, +) -> Result { let _boundary = catch_boundary(recovery); std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) - .map_err(|payload| panic_message(payload.as_ref())) } /// Recover panics while polling a future. Enter the recovery boundary for each poll so -/// unrelated work on the polling thread cannot inherit it. +/// unrelated work on the polling thread cannot inherit it. Preserve the panic payload so +/// the caller can distinguish control-flow unwinds from genuine panics. pub(crate) async fn catch_unwind_async( recovery: Recovery, future: impl Future, -) -> Result { +) -> Result { let mut future = Box::pin(future); std::future::poll_fn(move |cx| { @@ -129,12 +135,16 @@ pub(crate) async fn catch_unwind_async( match std::panic::catch_unwind(AssertUnwindSafe(|| future.as_mut().poll(cx))) { Ok(Poll::Pending) => Poll::Pending, Ok(Poll::Ready(value)) => Poll::Ready(Ok(value)), - Err(payload) => Poll::Ready(Err(panic_message(payload.as_ref()))), + Err(payload) => Poll::Ready(Err(payload)), } }) .await } +pub(crate) fn message(payload: &PanicPayload) -> String { + panic_message(payload.as_ref()) +} + /// Guard that preserves a `catch_unwind()` recovery boundary for the panic hook. /// /// Restores the preceding flag in `Drop::drop()` so a nested boundary cannot disable an @@ -193,24 +203,49 @@ mod tests { #[test] fn test_catch_unwind_passes_through_ok() { let result = catch_unwind(Recovery::Always, || 1 + 1); - assert_eq!(result, Ok(2)); + let Ok(value) = result else { + panic!("Expected a value"); + }; + assert_eq!(value, 2); } #[test] - fn test_catch_unwind_converts_panic_to_err() { + fn test_catch_unwind_preserves_panic_payload() { let result = catch_unwind(Recovery::Always, || panic!("oh no")); - assert_eq!(result, Err(String::from("oh no"))); + let Err(payload) = result else { + panic!("Expected a panic payload"); + }; + assert_eq!(message(&payload), "oh no"); } #[tokio::test] async fn test_catch_unwind_async_passes_through_ok() { let result = catch_unwind_async(Recovery::Always, async { 1 + 1 }).await; - assert_eq!(result, Ok(2)); + let Ok(value) = result else { + panic!("Expected a value"); + }; + assert_eq!(value, 2); } #[tokio::test] - async fn test_catch_unwind_async_converts_panic_to_err() { + async fn test_catch_unwind_async_preserves_panic_payload() { let result = catch_unwind_async(Recovery::Always, async { panic!("oh no") }).await; - assert_eq!(result, Err(String::from("oh no"))); + let Err(payload) = result else { + panic!("Expected a panic payload"); + }; + assert_eq!(message(&payload), "oh no"); + } + + #[tokio::test] + async fn test_catch_unwind_async_preserves_salsa_cancellation() { + let result = catch_unwind_async(Recovery::Always, async { + std::panic::resume_unwind(Box::new(salsa::Cancelled::PendingWrite)) + }) + .await; + + let Err(payload) = result else { + panic!("Expected a cancellation payload"); + }; + assert!(payload.is::()); } } diff --git a/crates/ark/src/r_task.rs b/crates/ark/src/r_task.rs index 93321f3b0..9d14883ab 100644 --- a/crates/ark/src/r_task.rs +++ b/crates/ark/src/r_task.rs @@ -25,6 +25,8 @@ use uuid::Uuid; use crate::console::Console; use crate::console::ConsoleOutputCapture; use crate::fixtures::r_test_init; +use crate::panic; +use crate::panic::Recovery; /// Task channels for idle-time tasks (top-level only) static IDLE_TASKS: LazyLock = LazyLock::new(TaskChannels::new); @@ -296,16 +298,24 @@ where // Instead of scoping the task with a thread join, we send it on the R // thread and block the thread until a completion channel wakes us up. - // Stores the outcome of `f`. We catch any unwind on the R thread instead of - // letting it escape the closure: the closure runs inside `r_sandbox`'s - // `try_catch`, and a Rust unwind crossing those C frames is UB. The payload - // is ferried back and re-raised below, on the calling thread. + // Stores the outcome of `f`. If the caller can recover panics, catch any unwind + // on the R thread instead of letting it escape through `r_sandbox`'s C frames. + // The payload is ferried back and re-raised below, on the calling thread. + // + // Without a recovering caller, don't install a boundary on the R thread. The + // production panic hook must abort rather than let the process continue after + // losing a thread. + let caller_recovers_panic = panic::recovers_panic(); let result: SharedOption> = SharedOption::default(); { let result = Arc::clone(&result); let closure = move || { - let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); + let caught = if caller_recovers_panic { + panic::catch_unwind(Recovery::Always, f) + } else { + Ok(f()) + }; *result.lock().unwrap() = Some(caught); }; @@ -364,9 +374,10 @@ where } } - // The closure ran to completion: it caught its own unwind, and an R-level - // error would have panicked above. Re-raise on this thread any panic the - // closure caught on the R thread. + // The closure ran to completion, and an R-level error would have panicked above. + // If the caller has a recovery boundary, re-raise any Rust panic caught on the + // R thread. If there is no recovery boundary, the production panic hook aborts + // before reaching here. let caught = result.lock().unwrap().take().unwrap(); match caught { Ok(value) => value, diff --git a/crates/ark/tests/integration/lsp.rs b/crates/ark/tests/integration/lsp.rs index a268d8264..8d1e7741f 100644 --- a/crates/ark/tests/integration/lsp.rs +++ b/crates/ark/tests/integration/lsp.rs @@ -117,6 +117,35 @@ fn test_lsp_panicking_notification_ends_session() { lsp.disconnect_abruptly(); } +// Salsa cancellation is control flow, not a crash. Its typed unwind payload must +// survive the trip through `r_task()` and the async event boundary. +#[test] +fn test_lsp_cancellation_across_r_task_keeps_running() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + lsp.send_notification("ark/testCancelRTask", json!({})); + + let uri = lsp.open_document("test_cancel_r_task.R", "x <- 1\n"); + lsp.completions(&uri, 0, 0); + assert!(lsp.show_messages().is_empty()); +} + +// A genuine panic raised in `r_task()` must return to the caller's recovery +// boundary rather than aborting the R process. +#[test] +fn test_lsp_panic_across_r_task_ends_session() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + lsp.allow_log_message("Panic while handling event"); + lsp.send_notification("ark/testPanicRTask", json!({})); + + lsp.recv_server_request("window/showMessageRequest"); + lsp.expect_server_closes_connection(Duration::from_secs(5)); + lsp.disconnect_abruptly(); +} + // A panic outside the per-event boundary must still show the crash dialog before shutdown. #[test] fn test_lsp_panicking_main_loop_reports_crash() { From 60813548b92928045bedbb82e953e0c7b36aa0fa Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 11 Sep 2026 16:48:50 +0200 Subject: [PATCH 7/9] Report background task panics to user once --- crates/ark/src/lsp/analysis/pool.rs | 1 + crates/ark/src/lsp/io_pool.rs | 1 + crates/ark/src/lsp/main_loop.rs | 55 +++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/crates/ark/src/lsp/analysis/pool.rs b/crates/ark/src/lsp/analysis/pool.rs index f107a7623..b6d9b3317 100644 --- a/crates/ark/src/lsp/analysis/pool.rs +++ b/crates/ark/src/lsp/analysis/pool.rs @@ -207,6 +207,7 @@ fn run_entry(entry: Entry) { { let message = panic::message(&payload); lsp::log_error!("An analysis task panicked: {message}"); + crate::lsp::main_loop::report_background_panic(); } } diff --git a/crates/ark/src/lsp/io_pool.rs b/crates/ark/src/lsp/io_pool.rs index 78dd39277..e308493f0 100644 --- a/crates/ark/src/lsp/io_pool.rs +++ b/crates/ark/src/lsp/io_pool.rs @@ -56,6 +56,7 @@ fn run_job(job: Job) { if let Err(payload) = panic::catch_unwind(Recovery::Always, job) { let message = panic::message(&payload); lsp::log_error!("An I/O job panicked: {message}"); + crate::lsp::main_loop::report_background_panic(); } } diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 870a3868a..521830f12 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -92,6 +92,7 @@ pub(crate) type TokioUnboundedReceiver = tokio::sync::mpsc::UnboundedReceiver static AUXILIARY_EVENT_TX: RwLock>> = RwLock::new(None); pub static LSP_HAS_CRASHED: AtomicBool = AtomicBool::new(false); +static BACKGROUND_PANIC_REPORTED: AtomicBool = AtomicBool::new(false); #[derive(Debug)] #[expect(clippy::large_enum_variant)] @@ -133,6 +134,7 @@ pub(crate) struct DidCloseVirtualDocumentParams { pub(crate) enum AuxiliaryEvent { Log(lsp_types::MessageType, String), PublishDiagnostics(DiagnosticsPublication), + ShowMessage(lsp_types::MessageType, String), Shutdown, } @@ -1086,6 +1088,9 @@ impl AuxiliaryState { AuxiliaryEvent::PublishDiagnostics(publication) => { self.publish_diagnostics(publication).await }, + AuxiliaryEvent::ShowMessage(level, message) => { + self.client.show_message(level, message).await + }, AuxiliaryEvent::Shutdown => break, } } @@ -1169,6 +1174,36 @@ fn send_auxiliary(event: AuxiliaryEvent) { }) } +pub(crate) fn report_background_panic() { + // Only report once to avoid spamming the user + if BACKGROUND_PANIC_REPORTED.swap(true, Ordering::AcqRel) { + return; + } + + let Ok(auxiliary_event_tx) = AUXILIARY_EVENT_TX.read() else { + log::warn!("Can't lock auxiliary event sender to report a background panic"); + return; + }; + let Some(auxiliary_event_tx) = auxiliary_event_tx.as_ref() else { + log::warn!("Can't report a background panic before the LSP is initialized"); + return; + }; + + let event = AuxiliaryEvent::ShowMessage( + MessageType::ERROR, + String::from( + "An R language server background task encountered an internal error. \ + Some smart features may be temporarily unavailable. \ + See https://positron.posit.co/troubleshooting.html#python-and-r-logs \ + for full logs and report the problem at \ + https://github.com/posit-dev/positron/issues.", + ), + ); + if let Err(err) = auxiliary_event_tx.send(event) { + log::warn!("LSP is shut down, can't report a background panic:\n{err:?}"); + } +} + /// Initialise the auxiliary channel for unit tests that exercise LSP /// handlers calling `publish_diagnostics` / `log` / similar. /// @@ -1256,15 +1291,35 @@ mod tests { use url::Url; use super::classify_event_unwind; + use super::init_aux_for_test; + use super::report_background_panic; use super::respond; use super::tokio_unbounded_channel; + use super::AuxiliaryEvent; use super::EventUnwind; + use super::MessageType; use crate::lsp::backend::LspError; use crate::lsp::backend::LspResponse; use crate::lsp::backend::RequestResponse; use crate::lsp::state::WorldState; use crate::lsp::traits::url::UrlExt; + #[test] + fn test_background_panic_is_reported_once() { + let mut events_rx = init_aux_for_test(); + + report_background_panic(); + report_background_panic(); + + let event = events_rx.try_recv(); + let Ok(AuxiliaryEvent::ShowMessage(level, message)) = event else { + panic!("Expected a show-message event"); + }; + assert_eq!(level, MessageType::ERROR); + assert!(message.contains("background task encountered an internal error")); + assert!(events_rx.try_recv().is_err()); + } + #[test] fn test_salsa_cancellation_is_not_classified_as_event_panic() { let payload = Box::new(salsa::Cancelled::PendingWrite); From 955c0ed7eef71879b58fc8ba6ecf6bf5c80e75a4 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 16 Sep 2026 09:37:31 +0200 Subject: [PATCH 8/9] Scope LSP crash state to the session that crashed --- crates/ark/src/lsp/backend.rs | 17 +++- crates/ark/src/lsp/main_loop.rs | 114 +++++++++++++++-------- crates/ark/src/lsp/tests.rs | 2 +- crates/ark/src/lsp/tests/utils/client.rs | 17 +++- crates/ark/src/lsp/tests/utils/mod.rs | 2 +- crates/ark/tests/integration/lsp.rs | 25 +++++ 6 files changed, 129 insertions(+), 48 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 4a8862dc8..5e602737f 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -8,7 +8,6 @@ #![allow(deprecated)] use std::path::PathBuf; -use std::sync::atomic::Ordering; use std::sync::Arc; use amalthea::comm::server_comm::ServerStartMessage; @@ -33,7 +32,7 @@ use tower_lsp_server::LanguageServer; use tower_lsp_server::LspService; use tower_lsp_server::Server; -use super::main_loop::LSP_HAS_CRASHED; +use super::main_loop::CrashFlag; use crate::console::Console; use crate::console::ConsoleNotification; use crate::lsp::handlers::VirtualDocumentParams; @@ -208,6 +207,10 @@ struct Backend { /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, + /// Set as soon as the main loop panics, so requests arriving before the + /// connection closes get a clean error. + crashed: Arc, + /// Handle to the LSP loops. Drop it to shut the loops down and drop all /// owned state. _main_loop: LoopHandles, @@ -215,7 +218,7 @@ struct Backend { impl Backend { async fn request(&self, request: LspRequest) -> RequestResponse { - if LSP_HAS_CRASHED.load(Ordering::Acquire) { + if self.crashed.is_set() { return RequestResponse::Disabled; } @@ -238,6 +241,10 @@ impl Backend { } fn notify(&self, notif: LspNotification) { + if self.crashed.is_set() { + return; + } + // Relay notification to main loop if self .events_tx @@ -575,9 +582,10 @@ pub(crate) fn start_lsp( let init = |client: Client| { let state = GlobalState::new(client, r_home, console_notification_tx); let events_tx = state.events_tx(); + let crashed = Arc::new(CrashFlag::new()); // Start main loop and hold onto the handle that keeps it alive - let main_loop = state.start(shutdown_tx); + let main_loop = state.start(shutdown_tx, Arc::clone(&crashed)); // Forward event channel along to `Console`. // This also updates an outdated channel after a reconnect. @@ -594,6 +602,7 @@ pub(crate) fn start_lsp( Backend { events_tx, + crashed, _main_loop: main_loop, } }; diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 521830f12..144bf0bb8 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -91,8 +91,24 @@ pub(crate) type TokioUnboundedReceiver = tokio::sync::mpsc::UnboundedReceiver /// LSPs to send log messages and tasks to the newer LSPs. static AUXILIARY_EVENT_TX: RwLock>> = RwLock::new(None); -pub static LSP_HAS_CRASHED: AtomicBool = AtomicBool::new(false); -static BACKGROUND_PANIC_REPORTED: AtomicBool = AtomicBool::new(false); +/// Latches when the LSP main loop panics. Scoped to one LSP session, so a +/// reconnect starts clean in case the panic was transient. +#[derive(Debug)] +pub(crate) struct CrashFlag(AtomicBool); + +impl CrashFlag { + pub(crate) fn new() -> Self { + Self(AtomicBool::new(false)) + } + + pub(crate) fn is_set(&self) -> bool { + self.0.load(Ordering::Acquire) + } + + pub(crate) fn set(&self) { + self.0.store(true, Ordering::Release); + } +} #[derive(Debug)] #[expect(clippy::large_enum_variant)] @@ -134,7 +150,7 @@ pub(crate) struct DidCloseVirtualDocumentParams { pub(crate) enum AuxiliaryEvent { Log(lsp_types::MessageType, String), PublishDiagnostics(DiagnosticsPublication), - ShowMessage(lsp_types::MessageType, String), + ReportBackgroundPanic, Shutdown, } @@ -266,6 +282,7 @@ impl LspState { /// The auxiliary loop currently handles: /// - Log messages. /// - Diagnostics publication. +/// - Background task panic reports. struct AuxiliaryState { client: Client, auxiliary_event_rx: TokioUnboundedReceiver, @@ -273,6 +290,7 @@ struct AuxiliaryState { /// open file, but most runs produce the same result, so we skip the publish /// when it matches what the client already has. published_diagnostics: HashMap>, + background_panic_reported: bool, } impl GlobalState { @@ -345,7 +363,11 @@ impl GlobalState { /// /// The returned [`LoopHandles`] owns everything the loops need. Drop it to /// shut the loops down and release the owned state. - pub(crate) fn start(self, server_shutdown_tx: Sender<()>) -> LoopHandles { + pub(crate) fn start( + self, + server_shutdown_tx: Sender<()>, + crashed: Arc, + ) -> LoopHandles { let mut aux = tokio::task::JoinSet::<()>::new(); // The auxiliary loop is fully async and never blocks. Must be started @@ -368,14 +390,15 @@ impl GlobalState { let outcome = panic::catch_unwind(Recovery::Always, { let server_shutdown_tx = server_shutdown_tx.clone(); let handle = handle.clone(); - move || handle.block_on(self.main_loop(shutdown_rx, server_shutdown_tx)) + let crashed = Arc::clone(&crashed); + move || handle.block_on(self.main_loop(shutdown_rx, server_shutdown_tx, crashed)) }); // Handle panics that bypass `handle_event()`'s recovery boundary. if let Err(payload) = outcome { let message = panic::message(&payload); lsp::log_error!("Panic in the main loop: {message}"); - LSP_HAS_CRASHED.store(true, Ordering::Release); + crashed.set(); let report = panic::catch_unwind(Recovery::Always, || { handle.block_on(report_crash(&client)) @@ -405,6 +428,7 @@ impl GlobalState { mut self, mut shutdown_rx: oneshot::Receiver<()>, server_shutdown_tx: Sender<()>, + crashed: Arc, ) { loop { tokio::select! { @@ -436,7 +460,7 @@ impl GlobalState { // the panic because a handler may have partially written its state. let message = panic::message(&payload); lsp::log_error!("Panic while handling event: {message}"); - LSP_HAS_CRASHED.store(true, Ordering::Release); + crashed.set(); report_crash(&self.client).await; let _ = server_shutdown_tx.send(()).await; break; @@ -784,9 +808,9 @@ impl GlobalState { /// notification often won't get sent out before shutdown occurs. The request /// returns control to us when the user acknowledges the message. It doesn't /// matter if that takes awhile because we shut down right after, and we've -/// already flipped the `LSP_HAS_CRASHED` global flag. We do bound it with a 5 -/// second timeout just in case the user ignores the message entirely, so we can -/// still shutdown. +/// already set the session's `CrashFlag`. We do bound it with a 5 second +/// timeout just in case the user ignores the message entirely, so we can still +/// shutdown. async fn report_crash(client: &Client) { let user_message = concat!( "The R language server has crashed and has been disabled. ", @@ -1074,6 +1098,7 @@ impl AuxiliaryState { client, auxiliary_event_rx, published_diagnostics: HashMap::new(), + background_panic_reported: false, } } @@ -1088,9 +1113,7 @@ impl AuxiliaryState { AuxiliaryEvent::PublishDiagnostics(publication) => { self.publish_diagnostics(publication).await }, - AuxiliaryEvent::ShowMessage(level, message) => { - self.client.show_message(level, message).await - }, + AuxiliaryEvent::ReportBackgroundPanic => self.report_background_panic().await, AuxiliaryEvent::Shutdown => break, } } @@ -1110,6 +1133,25 @@ impl AuxiliaryState { } } + async fn report_background_panic(&mut self) { + if std::mem::replace(&mut self.background_panic_reported, true) { + return; + } + + self.client + .show_message( + MessageType::ERROR, + String::from( + "An R language server background task encountered an internal error. \ + Some smart features may be temporarily unavailable. \ + See https://positron.posit.co/troubleshooting.html#python-and-r-logs \ + for full logs and report the problem at \ + https://github.com/posit-dev/positron/issues.", + ), + ) + .await; + } + /// Publish diagnostics, skipping the client round-trip when the set is /// identical to what we last sent for that file. Only non-empty sets are /// remembered, and an absent entry counts as empty. So an empty result is @@ -1175,11 +1217,6 @@ fn send_auxiliary(event: AuxiliaryEvent) { } pub(crate) fn report_background_panic() { - // Only report once to avoid spamming the user - if BACKGROUND_PANIC_REPORTED.swap(true, Ordering::AcqRel) { - return; - } - let Ok(auxiliary_event_tx) = AUXILIARY_EVENT_TX.read() else { log::warn!("Can't lock auxiliary event sender to report a background panic"); return; @@ -1189,17 +1226,7 @@ pub(crate) fn report_background_panic() { return; }; - let event = AuxiliaryEvent::ShowMessage( - MessageType::ERROR, - String::from( - "An R language server background task encountered an internal error. \ - Some smart features may be temporarily unavailable. \ - See https://positron.posit.co/troubleshooting.html#python-and-r-logs \ - for full logs and report the problem at \ - https://github.com/posit-dev/positron/issues.", - ), - ); - if let Err(err) = auxiliary_event_tx.send(event) { + if let Err(err) = auxiliary_event_tx.send(AuxiliaryEvent::ReportBackgroundPanic) { log::warn!("LSP is shut down, can't report a background panic:\n{err:?}"); } } @@ -1291,33 +1318,40 @@ mod tests { use url::Url; use super::classify_event_unwind; - use super::init_aux_for_test; use super::report_background_panic; use super::respond; + use super::send_auxiliary; use super::tokio_unbounded_channel; use super::AuxiliaryEvent; + use super::AuxiliaryState; use super::EventUnwind; - use super::MessageType; use crate::lsp::backend::LspError; use crate::lsp::backend::LspResponse; use crate::lsp::backend::RequestResponse; use crate::lsp::state::WorldState; + use crate::lsp::tests::utils::client::TestClient; use crate::lsp::traits::url::UrlExt; - #[test] - fn test_background_panic_is_reported_once() { - let mut events_rx = init_aux_for_test(); + #[tokio::test] + async fn test_background_panic_is_reported_once() { + let client = TestClient::new(&[]).await; + let auxiliary = AuxiliaryState::new(client.client()); + let auxiliary_loop = tokio::spawn(auxiliary.start()); report_background_panic(); report_background_panic(); + send_auxiliary(AuxiliaryEvent::Shutdown); + auxiliary_loop.await.unwrap(); - let event = events_rx.try_recv(); - let Ok(AuxiliaryEvent::ShowMessage(level, message)) = event else { - panic!("Expected a show-message event"); - }; - assert_eq!(level, MessageType::ERROR); + // The socket is FIFO, so a round-trip flushes the notifications the peer + // has not read yet. + let _ = client.client().configuration(vec![]).await; + + let notifications = client.notifications(); + assert_eq!(notifications.len(), 1); + assert_eq!(notifications[0].0, "window/showMessage"); + let message = notifications[0].1["message"].as_str().unwrap(); assert!(message.contains("background task encountered an internal error")); - assert!(events_rx.try_recv().is_err()); } #[test] diff --git a/crates/ark/src/lsp/tests.rs b/crates/ark/src/lsp/tests.rs index a2fc33e40..f75a39993 100644 --- a/crates/ark/src/lsp/tests.rs +++ b/crates/ark/src/lsp/tests.rs @@ -9,4 +9,4 @@ mod source_handler; mod sources; mod state; mod state_handlers; -mod utils; +pub(crate) mod utils; diff --git a/crates/ark/src/lsp/tests/utils/client.rs b/crates/ark/src/lsp/tests/utils/client.rs index 7d3af937b..8eb77c5c6 100644 --- a/crates/ark/src/lsp/tests/utils/client.rs +++ b/crates/ark/src/lsp/tests/utils/client.rs @@ -35,6 +35,7 @@ pub(crate) struct TestClient { client: Client, settings: Arc>>, requests: Arc>>, + notifications: Arc>>, /// Aborts the peer on drop. _peer: JoinSet<()>, @@ -68,18 +69,21 @@ impl TestClient { .collect(), )); let requests = Arc::new(Mutex::new(Vec::new())); + let notifications = Arc::new(Mutex::new(Vec::new())); let mut peer = JoinSet::new(); peer.spawn(answer_requests( socket, Arc::clone(&settings), Arc::clone(&requests), + Arc::clone(¬ifications), )); Self { client, settings, requests, + notifications, _peer: peer, } } @@ -98,11 +102,15 @@ impl TestClient { .insert(section.to_string(), value); } - /// Methods of the requests the peer has answered, in order. Notifications - /// aren't recorded. + /// Methods of the requests the peer has answered, in order. pub(crate) fn answered_requests(&self) -> Vec { self.requests.lock().unwrap().clone() } + + /// Methods and params of the notifications the server has sent, in order. + pub(crate) fn notifications(&self) -> Vec<(String, Value)> { + self.notifications.lock().unwrap().clone() + } } /// Answer every request the server sends until it drops its [`Client`]. @@ -110,6 +118,7 @@ async fn answer_requests( socket: ClientSocket, settings: Arc>>, requests: Arc>>, + notifications: Arc>>, ) { let (mut incoming, mut outgoing) = socket.split(); @@ -119,6 +128,10 @@ async fn answer_requests( // A notification, such as `textDocument/publishDiagnostics`, carries no // id and gets no reply. let Some(id) = id else { + notifications + .lock() + .unwrap() + .push((method.to_string(), params.unwrap_or(Value::Null))); continue; }; diff --git a/crates/ark/src/lsp/tests/utils/mod.rs b/crates/ark/src/lsp/tests/utils/mod.rs index b90f16ca1..81d60e1a2 100644 --- a/crates/ark/src/lsp/tests/utils/mod.rs +++ b/crates/ark/src/lsp/tests/utils/mod.rs @@ -1,4 +1,4 @@ -mod client; +pub(crate) mod client; mod description_writer; mod events; mod namespace_writer; diff --git a/crates/ark/tests/integration/lsp.rs b/crates/ark/tests/integration/lsp.rs index 8d1e7741f..fc764f538 100644 --- a/crates/ark/tests/integration/lsp.rs +++ b/crates/ark/tests/integration/lsp.rs @@ -160,6 +160,31 @@ fn test_lsp_panicking_main_loop_reports_crash() { lsp.disconnect_abruptly(); } +// A crash is scoped to the session that crashed, so the next one must serve +// requests rather than answer every one of them with `Disabled`. +#[test] +fn test_lsp_reconnect_after_crash_is_functional() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + lsp.allow_log_message("Panic while handling event"); + lsp.send_notification("ark/testPanicNotification", json!({})); + + lsp.recv_server_request("window/showMessageRequest"); + lsp.expect_server_closes_connection(Duration::from_secs(5)); + lsp.disconnect_abruptly(); + + let mut lsp = frontend.start_lsp(); + assert!(lsp.server_capabilities().completion_provider.is_some()); + + let uri = lsp.open_document("reconnect_after_crash.R", "pas"); + let items = lsp.completions(&uri, 0, 3); + let labels: Vec<&str> = items.iter().map(|item| item.label.as_str()).collect(); + assert!(labels.contains(&"paste")); + + assert!(lsp.show_messages().is_empty()); +} + // The two cases below test errors that don't depend on the rename // implementation's resolution capabilities. New-name validation always // applies (R language constraints), so these tests stay valid once From a249d357dadc3426b378ecae8bc49868539ea4ac Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 16 Sep 2026 12:10:09 +0200 Subject: [PATCH 9/9] Scope background panic reports to their LSP session --- crates/ark/src/lsp/analysis/pool.rs | 25 +++++--- crates/ark/src/lsp/backend.rs | 17 +++-- crates/ark/src/lsp/io_pool.rs | 20 ++++-- crates/ark/src/lsp/main_loop.rs | 98 +++++++++++++++++++---------- 4 files changed, 102 insertions(+), 58 deletions(-) diff --git a/crates/ark/src/lsp/analysis/pool.rs b/crates/ark/src/lsp/analysis/pool.rs index b6d9b3317..da82da7be 100644 --- a/crates/ark/src/lsp/analysis/pool.rs +++ b/crates/ark/src/lsp/analysis/pool.rs @@ -17,6 +17,7 @@ use stdext::spawn; use super::catch_cancellation; use super::snapshot::WorldStateSnapshot; use crate::lsp; +use crate::lsp::main_loop::LspServiceContext; use crate::panic; use crate::panic::Recovery; @@ -41,11 +42,11 @@ pub(crate) struct AnalysisPool { } impl AnalysisPool { - pub(crate) fn new() -> Self { - Self::with_threads(analysis_threads()) + pub(crate) fn new(service_context: Arc) -> Self { + Self::with_threads(analysis_threads(), service_context) } - fn with_threads(threads: usize) -> Self { + fn with_threads(threads: usize, service_context: Arc) -> Self { let shared = Arc::new(Shared { queue: Mutex::new(Queue { entries: VecDeque::new(), @@ -56,7 +57,8 @@ impl AnalysisPool { for _ in 0..threads { let shared = Arc::clone(&shared); - spawn!("oak-analysis", move || work(shared)); + let service_context = Arc::clone(&service_context); + spawn!("oak-analysis", move || work(shared, service_context)); } Self { shared } @@ -156,12 +158,12 @@ struct Entry { run: Box, } -fn work(shared: Arc) { +fn work(shared: Arc, service_context: Arc) { // `run_entry` takes the entry by value, so the snapshot has dropped by the // time we ask for the next one. A worker parked on `next_entry` doesn't // hold a db handle and can't block a writer. while let Some(entry) = shared.next_entry() { - run_entry(entry); + run_entry(entry, &service_context); } } @@ -192,7 +194,7 @@ impl Shared { } } -fn run_entry(entry: Entry) { +fn run_entry(entry: Entry, service_context: &LspServiceContext) { let Entry { snapshot, run, .. } = entry; // A writer parked on this handle would only cancel the task at its first @@ -207,7 +209,7 @@ fn run_entry(entry: Entry) { { let message = panic::message(&payload); lsp::log_error!("An analysis task panicked: {message}"); - crate::lsp::main_loop::report_background_panic(); + service_context.report_background_panic(); } } @@ -218,6 +220,7 @@ mod tests { use std::sync::Arc; use super::AnalysisPool; + use crate::lsp::main_loop::LspServiceContext; use crate::lsp::state::WorldState; /// A queued task whose snapshot is already cancelled must be dropped without @@ -229,7 +232,8 @@ mod tests { #[test] fn test_pool_drops_cancelled_task_without_running() { let state = WorldState::default(); - let pool = AnalysisPool::with_threads(1); + let context = Arc::new(LspServiceContext::new()); + let pool = AnalysisPool::with_threads(1, context); let cancelled = state.snapshot(); cancelled.cancellation_token().cancel(); @@ -258,7 +262,8 @@ mod tests { crate::panic::install(); let state = WorldState::default(); - let pool = AnalysisPool::with_threads(1); + let context = Arc::new(LspServiceContext::new()); + let pool = AnalysisPool::with_threads(1, context); pool.spawn(state.snapshot(), |_snapshot| { panic!("Test panic in an analysis task") diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 5e602737f..5a53e2e58 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -32,7 +32,7 @@ use tower_lsp_server::LanguageServer; use tower_lsp_server::LspService; use tower_lsp_server::Server; -use super::main_loop::CrashFlag; +use super::main_loop::LspServiceContext; use crate::console::Console; use crate::console::ConsoleNotification; use crate::lsp::handlers::VirtualDocumentParams; @@ -207,9 +207,8 @@ struct Backend { /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, - /// Set as soon as the main loop panics, so requests arriving before the - /// connection closes get a clean error. - crashed: Arc, + /// State shared with the main loop and its background pools. + service_context: Arc, /// Handle to the LSP loops. Drop it to shut the loops down and drop all /// owned state. @@ -218,7 +217,7 @@ struct Backend { impl Backend { async fn request(&self, request: LspRequest) -> RequestResponse { - if self.crashed.is_set() { + if self.service_context.has_crashed() { return RequestResponse::Disabled; } @@ -241,7 +240,7 @@ impl Backend { } fn notify(&self, notif: LspNotification) { - if self.crashed.is_set() { + if self.service_context.has_crashed() { return; } @@ -582,10 +581,10 @@ pub(crate) fn start_lsp( let init = |client: Client| { let state = GlobalState::new(client, r_home, console_notification_tx); let events_tx = state.events_tx(); - let crashed = Arc::new(CrashFlag::new()); + let service_context = Arc::clone(state.service_context()); // Start main loop and hold onto the handle that keeps it alive - let main_loop = state.start(shutdown_tx, Arc::clone(&crashed)); + let main_loop = state.start(shutdown_tx); // Forward event channel along to `Console`. // This also updates an outdated channel after a reconnect. @@ -602,7 +601,7 @@ pub(crate) fn start_lsp( Backend { events_tx, - crashed, + service_context, _main_loop: main_loop, } }; diff --git a/crates/ark/src/lsp/io_pool.rs b/crates/ark/src/lsp/io_pool.rs index e308493f0..ef401a1db 100644 --- a/crates/ark/src/lsp/io_pool.rs +++ b/crates/ark/src/lsp/io_pool.rs @@ -5,10 +5,13 @@ // // +use std::sync::Arc; + use crossbeam::channel::Sender; use stdext::spawn_with_stack_size; use crate::lsp; +use crate::lsp::main_loop::LspServiceContext; use crate::panic; use crate::panic::Recovery; @@ -30,14 +33,20 @@ impl IoPool { /// of stack. Each lane picks its own size from the deepest call tree its /// jobs can reach, so use [`stdext::DEFAULT_STACK_SIZE`] unless you've /// bounded that. - pub(crate) fn new(name: &'static str, threads: usize, stack_size: usize) -> Self { + pub(crate) fn new( + name: &'static str, + threads: usize, + stack_size: usize, + service_context: Arc, + ) -> Self { let (jobs_tx, jobs_rx) = crossbeam::channel::unbounded::(); for _ in 0..threads { let jobs_rx = jobs_rx.clone(); + let service_context = Arc::clone(&service_context); spawn_with_stack_size!(name, stack_size, move || { while let Ok(job) = jobs_rx.recv() { - run_job(job); + run_job(job, &service_context); } }); } @@ -52,11 +61,11 @@ impl IoPool { } } -fn run_job(job: Job) { +fn run_job(job: Job, service_context: &LspServiceContext) { if let Err(payload) = panic::catch_unwind(Recovery::Always, job) { let message = panic::message(&payload); lsp::log_error!("An I/O job panicked: {message}"); - crate::lsp::main_loop::report_background_panic(); + service_context.report_background_panic(); } } @@ -70,7 +79,8 @@ mod tests { fn test_pool_survives_panicking_job() { crate::panic::install(); - let pool = IoPool::new("test-io-pool", 1, stdext::DEFAULT_STACK_SIZE); + let context = Arc::new(LspServiceContext::new()); + let pool = IoPool::new("test-io-pool", 1, stdext::DEFAULT_STACK_SIZE, context); pool.submit(|| panic!("Test panic in an I/O job")); let (tx, rx) = std::sync::mpsc::channel(); diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 144bf0bb8..db1f00362 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -91,22 +91,35 @@ pub(crate) type TokioUnboundedReceiver = tokio::sync::mpsc::UnboundedReceiver /// LSPs to send log messages and tasks to the newer LSPs. static AUXILIARY_EVENT_TX: RwLock>> = RwLock::new(None); -/// Latches when the LSP main loop panics. Scoped to one LSP session, so a -/// reconnect starts clean in case the panic was transient. +/// State shared across one LSP service. #[derive(Debug)] -pub(crate) struct CrashFlag(AtomicBool); +pub(crate) struct LspServiceContext { + crashed: AtomicBool, + background_panic_reported: AtomicBool, +} -impl CrashFlag { +impl LspServiceContext { pub(crate) fn new() -> Self { - Self(AtomicBool::new(false)) + Self { + crashed: AtomicBool::new(false), + background_panic_reported: AtomicBool::new(false), + } } - pub(crate) fn is_set(&self) -> bool { - self.0.load(Ordering::Acquire) + pub(crate) fn has_crashed(&self) -> bool { + self.crashed.load(Ordering::Acquire) } - pub(crate) fn set(&self) { - self.0.store(true, Ordering::Release); + pub(crate) fn set_crashed(&self) { + self.crashed.store(true, Ordering::Release); + } + + pub(crate) fn report_background_panic(&self) { + if self.background_panic_reported.swap(true, Ordering::AcqRel) { + return; + } + + report_background_panic(); } } @@ -243,6 +256,9 @@ pub(crate) struct LspState { /// behind a background task that can't drop its db snapshot. See /// [`crate::lsp::watchdog`]. pub(crate) watchdog: Watchdog, + + /// State shared with the backend, main loop, and background pools. + service_context: Arc, } impl LspState { @@ -250,25 +266,34 @@ impl LspState { console_notification_tx: TokioUnboundedSender, source_scheduler: SourceScheduler, ) -> Self { + let service_context = Arc::new(LspServiceContext::new()); + Self { capabilities: Capabilities::default(), console_notification_tx, oak_scheduler: ScanScheduler::new(), source_scheduler, - analysis_pool: AnalysisPool::new(), + analysis_pool: AnalysisPool::new(Arc::clone(&service_context)), diagnostics: DiagnosticsState::default(), // Stack size: `ScanRequest::run()` walks the filesystem with // `ignore::Walk` and `WalkDir`, both iterative, and parses // DESCRIPTION line by line. - scan_pool: IoPool::new("oak-scan", 1, stdext::SMALL_STACK_SIZE), + scan_pool: IoPool::new( + "oak-scan", + 1, + stdext::SMALL_STACK_SIZE, + Arc::clone(&service_context), + ), // Full stack because a fetch runs a rustls handshake, zstd and tar // decoding, and an R subprocess. source_pool: IoPool::new( "oak-source", SOURCE_POOL_THREADS, stdext::DEFAULT_STACK_SIZE, + Arc::clone(&service_context), ), watchdog: Watchdog::new(), + service_context, } } } @@ -290,7 +315,6 @@ struct AuxiliaryState { /// open file, but most runs produce the same result, so we skip the publish /// when it matches what the client already has. published_diagnostics: HashMap>, - background_panic_reported: bool, } impl GlobalState { @@ -359,15 +383,15 @@ impl GlobalState { self.events_tx.clone() } + pub(crate) fn service_context(&self) -> &Arc { + &self.lsp_state.service_context + } + /// Start the main and auxiliary loops. /// /// The returned [`LoopHandles`] owns everything the loops need. Drop it to /// shut the loops down and release the owned state. - pub(crate) fn start( - self, - server_shutdown_tx: Sender<()>, - crashed: Arc, - ) -> LoopHandles { + pub(crate) fn start(self, server_shutdown_tx: Sender<()>) -> LoopHandles { let mut aux = tokio::task::JoinSet::<()>::new(); // The auxiliary loop is fully async and never blocks. Must be started @@ -385,20 +409,27 @@ impl GlobalState { let (shutdown_tx, shutdown_rx) = oneshot::channel(); let handle = Handle::current(); let client = self.client.clone(); + let service_context = Arc::clone(&self.lsp_state.service_context); let main_loop = spawn!("oak-main-loop", move || { let outcome = panic::catch_unwind(Recovery::Always, { let server_shutdown_tx = server_shutdown_tx.clone(); let handle = handle.clone(); - let crashed = Arc::clone(&crashed); - move || handle.block_on(self.main_loop(shutdown_rx, server_shutdown_tx, crashed)) + let service_context = Arc::clone(&service_context); + move || { + handle.block_on(self.main_loop( + shutdown_rx, + server_shutdown_tx, + service_context, + )) + } }); // Handle panics that bypass `handle_event()`'s recovery boundary. if let Err(payload) = outcome { let message = panic::message(&payload); lsp::log_error!("Panic in the main loop: {message}"); - crashed.set(); + service_context.set_crashed(); let report = panic::catch_unwind(Recovery::Always, || { handle.block_on(report_crash(&client)) @@ -428,7 +459,7 @@ impl GlobalState { mut self, mut shutdown_rx: oneshot::Receiver<()>, server_shutdown_tx: Sender<()>, - crashed: Arc, + service_context: Arc, ) { loop { tokio::select! { @@ -460,7 +491,7 @@ impl GlobalState { // the panic because a handler may have partially written its state. let message = panic::message(&payload); lsp::log_error!("Panic while handling event: {message}"); - crashed.set(); + service_context.set_crashed(); report_crash(&self.client).await; let _ = server_shutdown_tx.send(()).await; break; @@ -1098,7 +1129,6 @@ impl AuxiliaryState { client, auxiliary_event_rx, published_diagnostics: HashMap::new(), - background_panic_reported: false, } } @@ -1133,11 +1163,7 @@ impl AuxiliaryState { } } - async fn report_background_panic(&mut self) { - if std::mem::replace(&mut self.background_panic_reported, true) { - return; - } - + async fn report_background_panic(&self) { self.client .show_message( MessageType::ERROR, @@ -1216,7 +1242,7 @@ fn send_auxiliary(event: AuxiliaryEvent) { }) } -pub(crate) fn report_background_panic() { +fn report_background_panic() { let Ok(auxiliary_event_tx) = AUXILIARY_EVENT_TX.read() else { log::warn!("Can't lock auxiliary event sender to report a background panic"); return; @@ -1318,13 +1344,13 @@ mod tests { use url::Url; use super::classify_event_unwind; - use super::report_background_panic; use super::respond; use super::send_auxiliary; use super::tokio_unbounded_channel; use super::AuxiliaryEvent; use super::AuxiliaryState; use super::EventUnwind; + use super::LspServiceContext; use crate::lsp::backend::LspError; use crate::lsp::backend::LspResponse; use crate::lsp::backend::RequestResponse; @@ -1333,13 +1359,16 @@ mod tests { use crate::lsp::traits::url::UrlExt; #[tokio::test] - async fn test_background_panic_is_reported_once() { + async fn test_background_panic_is_reported_once_per_session() { let client = TestClient::new(&[]).await; let auxiliary = AuxiliaryState::new(client.client()); let auxiliary_loop = tokio::spawn(auxiliary.start()); + let first_session = LspServiceContext::new(); + let second_session = LspServiceContext::new(); - report_background_panic(); - report_background_panic(); + first_session.report_background_panic(); + first_session.report_background_panic(); + second_session.report_background_panic(); send_auxiliary(AuxiliaryEvent::Shutdown); auxiliary_loop.await.unwrap(); @@ -1348,8 +1377,9 @@ mod tests { let _ = client.client().configuration(vec![]).await; let notifications = client.notifications(); - assert_eq!(notifications.len(), 1); + assert_eq!(notifications.len(), 2); assert_eq!(notifications[0].0, "window/showMessage"); + assert_eq!(notifications[1].0, "window/showMessage"); let message = notifications[0].1["message"].as_str().unwrap(); assert!(message.contains("background task encountered an internal error")); }