From 696377acc2f43970ebed26cee0f41f200d084adf Mon Sep 17 00:00:00 2001 From: arookieofc <2128194521@qq.com> Date: Sat, 31 Oct 2026 05:48:59 +0800 Subject: [PATCH] fix(lsp): publish a failed start's status before waking waiters (#505) A request waiting for a language server woke when the start failed or the instance closed, then read the status while it still said `Starting`, so its error could be `jdtls is not ready (Starting)` instead of the crash. The status is now published first, for launch failures, crashes and giving up. A close during shutdown also wakes waiters, so they do not wait out their timeout. This fixes the crash test that was failing on `main`. Co-authored-by: a-rookie-of-C-language <186315048+a-rookie-of-C-language@users.noreply.github.com> --- crates/ide-lsp/src/lib.rs | 35 ++++++++++++++----- crates/ide-lsp/src/server.rs | 66 +++++++++++++++++++++++++++++++----- 2 files changed, 84 insertions(+), 17 deletions(-) diff --git a/crates/ide-lsp/src/lib.rs b/crates/ide-lsp/src/lib.rs index 532f7644..784675a3 100644 --- a/crates/ide-lsp/src/lib.rs +++ b/crates/ide-lsp/src/lib.rs @@ -462,24 +462,37 @@ impl LspManager { let cfg = server.effective_config(); let message = cfg.install_hint.clone().unwrap_or_else(|| format!("`{}` was not found on PATH", cfg.command)); - if server.finish_start(generation, Conn::Missing) { - server.set_status(&self.sink, ide_core::event::ServerStatus::Missing, Some(message)); + if server.fail_start( + generation, + Conn::Missing, + &self.sink, + ide_core::event::ServerStatus::Missing, + Some(message), + ) { self.attach_fallbacks(); } return; } Err(LaunchError::Config(message)) => { tracing::warn!("{}: {message}", server.name); - if server.finish_start(generation, Conn::Failed) { - server.set_status(&self.sink, ide_core::event::ServerStatus::Misconfigured, Some(message)); - } + server.fail_start( + generation, + Conn::Failed, + &self.sink, + ide_core::event::ServerStatus::Misconfigured, + Some(message), + ); return; } Err(LaunchError::Failed(message)) => { tracing::warn!("{}: {message}", server.name); - if server.finish_start(generation, Conn::Failed) { - server.set_status(&self.sink, ide_core::event::ServerStatus::Crashed, Some(message)); - } + server.fail_start( + generation, + Conn::Failed, + &self.sink, + ide_core::event::ServerStatus::Crashed, + Some(message), + ); return; } }; @@ -564,6 +577,7 @@ impl LspManager { }; self.clear_server_state(&server); if intentional { + server.announce_down(); return; } let tail = server.stderr_tail(5); @@ -580,6 +594,9 @@ impl LspManager { match server.record_crash() { Some(delay) => { server.set_status(&self.sink, ServerStatus::Crashed, Some(format!("{message}\nRestarting…"))); + // Woken after the crash is reported: a request that finds no instance + // reads this status to say why. The restart sets `Starting` again. + server.announce_down(); tokio::time::sleep(delay).await; if server.is_idle_gen(generation) { self.ensure_started(&server); @@ -592,6 +609,8 @@ impl LspManager { ServerStatus::Crashed, Some(format!("{message}\nCrashed repeatedly; restart it manually.")), ); + // Requests still waiting for a start read this status for their error. + server.announce_down(); } } } diff --git a/crates/ide-lsp/src/server.rs b/crates/ide-lsp/src/server.rs index d32a9fbe..018c81ee 100644 --- a/crates/ide-lsp/src/server.rs +++ b/crates/ide-lsp/src/server.rs @@ -168,15 +168,27 @@ impl Server { Some(st.generation) } - /// Starting → Missing/Failed. - pub fn finish_start(&self, generation: u64, conn: Conn) -> bool { - let mut st = self.state.lock(); - if st.generation == generation && matches!(st.conn, Conn::Starting) { + /// Starting → Missing/Failed, publishing the status the failed start reports + /// before waiters wake: a request that wakes on `Phase::Down` reads the status + /// to say why it has no server, and must not still see `Starting`. + pub fn fail_start( + &self, + generation: u64, + conn: Conn, + sink: &SharedSink, + status: ServerStatus, + message: Option, + ) -> bool { + { + let mut st = self.state.lock(); + if st.generation != generation || !matches!(st.conn, Conn::Starting) { + return false; + } st.conn = conn; - self.phase.send_replace(Phase::Down); - return true; } - false + self.set_status(sink, status, message); + self.announce_down(); + true } /// Starting → Ready. False when the start was superseded or cancelled. @@ -197,6 +209,12 @@ impl Server { } } + /// Wakes requests waiting for a start that ended without an instance, after + /// its status is published. + pub fn announce_down(&self) { + self.phase.send_replace(Phase::Down); + } + pub fn running(&self) -> Option { match &self.state.lock().conn { Conn::Ready(r) => Some(r.clone()), @@ -252,7 +270,8 @@ impl Server { } /// Instance `generation`'s connection closed. `None` if superseded, else whether - /// the close was intentional. + /// the close was intentional. Waiters are woken by [`Self::announce_down`] once + /// the crash is reported, not here: they read the status to say why. pub fn on_closed(&self, generation: u64) -> Option { let mut st = self.state.lock(); if st.generation != generation { @@ -261,7 +280,6 @@ impl Server { match &st.conn { Conn::Ready(_) | Conn::Starting => { st.conn = Conn::Idle; - self.phase.send_replace(Phase::Down); Some(false) } _ => Some(true), @@ -535,6 +553,36 @@ mod tests { assert_eq!(s.record_crash(), Some(Duration::from_millis(500))); } + #[tokio::test(flavor = "multi_thread")] + async fn a_failed_start_reports_its_status_before_waiters_wake() { + // The race behind "jdtls is not ready (Starting)": a request woken by the + // phase change reads the status to say why it has no server, so the status + // must already be the failure's. + let s = Arc::new(server()); + let sink: SharedSink = Arc::new(|_: IdeEvent| {}); + let generation = s.begin_start().unwrap(); + let waiter = { + let s = s.clone(); + tokio::spawn(async move { + let run = s.wait_ready(Duration::from_secs(30)).await; + (run.is_none(), s.status()) + }) + }; + tokio::task::yield_now().await; + assert!(s.fail_start(generation, Conn::Failed, &sink, ServerStatus::Crashed, Some("boom".into()))); + assert_eq!(waiter.await.unwrap(), (true, ServerStatus::Crashed)); + } + + #[test] + fn a_superseded_start_is_not_announced() { + let s = server(); + let generation = s.begin_start().unwrap(); + s.begin_stop(); // abandons the start and invalidates its generation + let sink: SharedSink = Arc::new(|_: IdeEvent| {}); + assert!(!s.fail_start(generation, Conn::Failed, &sink, ServerStatus::Crashed, Some("boom".into()))); + assert_eq!(s.status(), ServerStatus::Stopped); + } + #[test] fn crash_excerpt_keeps_the_exception_line() { // Node.js 24 crash of vue-language-server (issue #25), verbatim shape.