Consistently catch LSP panics

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;

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<bool> = 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,19 +729,11 @@ impl Console {
     /// caught and converted to `anyhow::Error`, which `harp::register`'s
     /// `r_unwrap()` then surfaces as a clean R error.
     pub fn with<T>(f: impl FnOnce(&Console) -> anyhow::Result<T>) -> anyhow::Result<T> {
-        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(payload) => {
+                let message = panic::message(&payload);
+                Err(anyhow!("Panic in Console callback: {message}"))
             },
         }
     }

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;

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<T>(f: impl FnOnce() -> T) -> Option<T> {
+pub(crate) fn catch_cancellation<T>(f: impl FnOnce() -> T) -> Option<T> {
     salsa::Cancelled::catch(AssertUnwindSafe(f)).ok()
 }

crates/ark/src/lsp/analysis/pool.rs

@@ -6,19 +6,20 @@
 //
 
 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::lsp::main_loop::LspServiceContext;
+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
@@ -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<LspServiceContext>) -> Self {
+        Self::with_threads(analysis_threads(), service_context)
     }
 
-    fn with_threads(threads: usize) -> Self {
+    fn with_threads(threads: usize, service_context: Arc<LspServiceContext>) -> 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<dyn FnOnce(WorldStateSnapshot) + Send>,
 }
 
-fn work(shared: Arc<Shared>) {
+fn work(shared: Arc<Shared>, service_context: Arc<LspServiceContext>) {
     // `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
@@ -202,12 +204,12 @@ 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(payload) =
+        panic::catch_unwind(Recovery::Always, || catch_cancellation(|| run(snapshot)))
+    {
+        let message = panic::message(&payload);
+        lsp::log_error!("An analysis task panicked: {message}");
+        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();
@@ -250,4 +254,28 @@ 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 context = Arc::new(LspServiceContext::new());
+        let pool = AnalysisPool::with_threads(1, context);
+
+        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();
+    }
 }