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

@@ -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();
             }
         }
     }

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<String>,
+    ) -> 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<Running> {
         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<bool> {
         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.