Fix cleanup of LSP request status messages (#10336)

Max Brunsfeld , Marshall , and Marshall Bowers created

This fixes a bug in https://github.com/zed-industries/zed/pull/9818,
where the status was not removed if the request failed. It also adds
replication of these new status messages to guests when collaborating.

Release Notes:

- Fixed an issue where the status of failed LSP actions was left in the
status bar

---------

Co-authored-by: Marshall <marshall@zed.dev>
Co-authored-by: Marshall Bowers <elliott.codes@gmail.com>

Change summary

crates/collab/src/tests/integration_tests.rs |  85 ++++++++++++++--
crates/gpui/src/app/test_context.rs          |  27 +++++
crates/project/src/project.rs                | 108 ++++++++++-----------
3 files changed, 151 insertions(+), 69 deletions(-)

Detailed changes

crates/collab/src/tests/integration_tests.rs 🔗

@@ -5,11 +5,12 @@ use crate::{
         TestClient, TestServer,
     },
 };
+use anyhow::{anyhow, Result};
 use call::{room, ActiveCall, ParticipantLocation, Room};
 use client::{User, RECEIVE_TIMEOUT};
 use collections::{HashMap, HashSet};
 use fs::{repository::GitFileStatus, FakeFs, Fs as _, RemoveOptions};
-use futures::StreamExt as _;
+use futures::{channel::mpsc, StreamExt as _};
 use gpui::{
     px, size, AppContext, BackgroundExecutor, BorrowAppContext, Model, Modifiers, MouseButton,
     MouseDownEvent, TestAppContext,
@@ -21,6 +22,7 @@ use language::{
 };
 use live_kit_client::MacOSDisplay;
 use lsp::LanguageServerId;
+use parking_lot::Mutex;
 use project::{
     search::SearchQuery, DiagnosticSummary, FormatTrigger, HoverBlockKind, Project, ProjectPath,
     SearchResult,
@@ -4662,6 +4664,7 @@ async fn test_references(
     let mut fake_language_servers = client_a.language_registry().register_fake_lsp_adapter(
         "Rust",
         FakeLspAdapter {
+            name: "my-fake-lsp-adapter",
             capabilities: lsp::ServerCapabilities {
                 references_provider: Some(lsp::OneOf::Left(true)),
                 ..Default::default()
@@ -4698,12 +4701,40 @@ async fn test_references(
 
     // Request references to a symbol as the guest.
     let fake_language_server = fake_language_servers.next().await.unwrap();
-    fake_language_server.handle_request::<lsp::request::References, _, _>(|params, _| async move {
+    let (lsp_response_tx, rx) = mpsc::unbounded::<Result<Option<Vec<lsp::Location>>>>();
+    fake_language_server.handle_request::<lsp::request::References, _, _>({
+        let rx = Arc::new(Mutex::new(Some(rx)));
+        move |params, _| {
+            assert_eq!(
+                params.text_document_position.text_document.uri.as_str(),
+                "file:///root/dir-1/one.rs"
+            );
+            let rx = rx.clone();
+            async move {
+                let mut response_rx = rx.lock().take().unwrap();
+                let result = response_rx.next().await.unwrap();
+                *rx.lock() = Some(response_rx);
+                result
+            }
+        }
+    });
+
+    let references = project_b.update(cx_b, |p, cx| p.references(&buffer_b, 7, cx));
+
+    // User is informed that a request is pending.
+    executor.run_until_parked();
+    project_b.read_with(cx_b, |project, _| {
+        let status = project.language_server_statuses().next().cloned().unwrap();
+        assert_eq!(status.name, "my-fake-lsp-adapter");
         assert_eq!(
-            params.text_document_position.text_document.uri.as_str(),
-            "file:///root/dir-1/one.rs"
+            status.pending_work.values().next().unwrap().message,
+            Some("Finding references...".into())
         );
-        Ok(Some(vec![
+    });
+
+    // Cause the language server to respond.
+    lsp_response_tx
+        .unbounded_send(Ok(Some(vec![
             lsp::Location {
                 uri: lsp::Url::from_file_path("/root/dir-1/two.rs").unwrap(),
                 range: lsp::Range::new(lsp::Position::new(0, 24), lsp::Position::new(0, 27)),
@@ -4716,16 +4747,18 @@ async fn test_references(
                 uri: lsp::Url::from_file_path("/root/dir-2/three.rs").unwrap(),
                 range: lsp::Range::new(lsp::Position::new(0, 37), lsp::Position::new(0, 40)),
             },
-        ]))
-    });
-
-    let references = project_b
-        .update(cx_b, |p, cx| p.references(&buffer_b, 7, cx))
-        .await
+        ])))
         .unwrap();
-    cx_b.read(|cx| {
+
+    let references = references.await.unwrap();
+    executor.run_until_parked();
+    project_b.read_with(cx_b, |project, cx| {
+        // User is informed that a request is no longer pending.
+        let status = project.language_server_statuses().next().unwrap();
+        assert!(status.pending_work.is_empty());
+
         assert_eq!(references.len(), 3);
-        assert_eq!(project_b.read(cx).worktrees().count(), 2);
+        assert_eq!(project.worktrees().count(), 2);
 
         let two_buffer = references[0].buffer.read(cx);
         let three_buffer = references[2].buffer.read(cx);
@@ -4743,6 +4776,32 @@ async fn test_references(
         assert_eq!(references[1].range.to_offset(two_buffer), 35..38);
         assert_eq!(references[2].range.to_offset(three_buffer), 37..40);
     });
+
+    let references = project_b.update(cx_b, |p, cx| p.references(&buffer_b, 7, cx));
+
+    // User is informed that a request is pending.
+    executor.run_until_parked();
+    project_b.read_with(cx_b, |project, _| {
+        let status = project.language_server_statuses().next().cloned().unwrap();
+        assert_eq!(status.name, "my-fake-lsp-adapter");
+        assert_eq!(
+            status.pending_work.values().next().unwrap().message,
+            Some("Finding references...".into())
+        );
+    });
+
+    // Cause the LSP request to fail.
+    lsp_response_tx
+        .unbounded_send(Err(anyhow!("can't find references")))
+        .unwrap();
+    references.await.unwrap_err();
+
+    // User is informed that the request is no longer pending.
+    executor.run_until_parked();
+    project_b.read_with(cx_b, |project, _| {
+        let status = project.language_server_statuses().next().unwrap();
+        assert!(status.pending_work.is_empty());
+    });
 }
 
 #[gpui::test(iterations = 10)]

crates/gpui/src/app/test_context.rs 🔗

@@ -463,7 +463,7 @@ impl TestAppContext {
     }
 }
 
-impl<T: Send> Model<T> {
+impl<T: 'static> Model<T> {
     /// Block until the next event is emitted by the model, then return it.
     pub fn next_event<Evt>(&self, cx: &mut TestAppContext) -> Evt
     where
@@ -491,6 +491,31 @@ impl<T: Send> Model<T> {
         }
         panic!("no event received")
     }
+
+    /// Returns a future that resolves when the model notifies.
+    pub fn next_notification(&self, cx: &TestAppContext) -> impl Future<Output = ()> {
+        use postage::prelude::{Sink as _, Stream as _};
+
+        let (mut tx, mut rx) = postage::mpsc::channel(1);
+        let mut cx = cx.app.app.borrow_mut();
+        let subscription = cx.observe(self, move |_, _| {
+            tx.try_send(()).ok();
+        });
+
+        let duration = if std::env::var("CI").is_ok() {
+            Duration::from_secs(5)
+        } else {
+            Duration::from_secs(1)
+        };
+
+        async move {
+            let notification = crate::util::timeout(duration, rx.recv())
+                .await
+                .expect("next notification timed out");
+            drop(subscription);
+            notification.expect("model dropped while test was waiting for its next notification")
+        }
+    }
 }
 
 impl<V: 'static> View<V> {

crates/project/src/project.rs 🔗

@@ -347,7 +347,7 @@ pub enum LanguageServerState {
     },
 }
 
-#[derive(Serialize)]
+#[derive(Clone, Debug, Serialize)]
 pub struct LanguageServerStatus {
     pub name: String,
     pub pending_work: BTreeMap<String, LanguageServerProgress>,
@@ -3924,19 +3924,6 @@ impl Project {
                         },
                         cx,
                     );
-                    self.enqueue_buffer_ordered_message(
-                        BufferOrderedMessage::LanguageServerUpdate {
-                            language_server_id,
-                            message: proto::update_language_server::Variant::WorkStart(
-                                proto::LspWorkStart {
-                                    token,
-                                    message: report.message,
-                                    percentage: report.percentage,
-                                },
-                            ),
-                        },
-                    )
-                    .ok();
                 }
             }
             lsp::WorkDoneProgress::Report(report) => {
@@ -3984,15 +3971,6 @@ impl Project {
                     .ok();
                 } else {
                     self.on_lsp_work_end(language_server_id, token.clone(), cx);
-                    self.enqueue_buffer_ordered_message(
-                        BufferOrderedMessage::LanguageServerUpdate {
-                            language_server_id,
-                            message: proto::update_language_server::Variant::WorkEnd(
-                                proto::LspWorkEnd { token },
-                            ),
-                        },
-                    )
-                    .ok();
                 }
             }
         }
@@ -4006,9 +3984,21 @@ impl Project {
         cx: &mut ModelContext<Self>,
     ) {
         if let Some(status) = self.language_server_statuses.get_mut(&language_server_id) {
-            status.pending_work.insert(token, progress);
+            status.pending_work.insert(token.clone(), progress.clone());
             cx.notify();
         }
+
+        if self.is_local() {
+            self.enqueue_buffer_ordered_message(BufferOrderedMessage::LanguageServerUpdate {
+                language_server_id,
+                message: proto::update_language_server::Variant::WorkStart(proto::LspWorkStart {
+                    token,
+                    message: progress.message,
+                    percentage: progress.percentage.map(|p| p as u32),
+                }),
+            })
+            .ok();
+        }
     }
 
     fn on_lsp_work_progress(
@@ -4049,6 +4039,16 @@ impl Project {
             status.pending_work.remove(&token);
             cx.notify();
         }
+
+        if self.is_local() {
+            self.enqueue_buffer_ordered_message(BufferOrderedMessage::LanguageServerUpdate {
+                language_server_id,
+                message: proto::update_language_server::Variant::WorkEnd(proto::LspWorkEnd {
+                    token,
+                }),
+            })
+            .ok();
+        }
     }
 
     fn on_lsp_did_change_watched_files(
@@ -6721,7 +6721,7 @@ impl Project {
                     let lsp_request = language_server.request::<R::LspRequest>(lsp_params);
 
                     let id = lsp_request.id();
-                    if status.is_some() {
+                    let _cleanup = if status.is_some() {
                         cx.update(|cx| {
                             this.update(cx, |this, cx| {
                                 this.on_lsp_work_start(
@@ -6737,22 +6737,35 @@ impl Project {
                             })
                         })
                         .log_err();
-                    }
 
-                    let result = lsp_request.await;
-                    let response = match result {
-                        Ok(response) => response,
-
-                        Err(err) => {
-                            log::warn!(
-                                "Generic lsp request to {} failed: {}",
-                                language_server.name(),
-                                err
-                            );
-                            return Err(err);
-                        }
+                        Some(defer(|| {
+                            cx.update(|cx| {
+                                this.update(cx, |this, cx| {
+                                    this.on_lsp_work_end(
+                                        language_server.server_id(),
+                                        id.to_string(),
+                                        cx,
+                                    );
+                                })
+                            })
+                            .log_err();
+                        }))
+                    } else {
+                        None
                     };
-                    let result = request
+
+                    let result = lsp_request.await;
+
+                    let response = result.map_err(|err| {
+                        log::warn!(
+                            "Generic lsp request to {} failed: {}",
+                            language_server.name(),
+                            err
+                        );
+                        err
+                    })?;
+
+                    request
                         .response_from_lsp(
                             response,
                             this.upgrade().ok_or_else(|| anyhow!("no app context"))?,
@@ -6760,22 +6773,7 @@ impl Project {
                             language_server.server_id(),
                             cx.clone(),
                         )
-                        .await;
-
-                    if status.is_some() {
-                        cx.update(|cx| {
-                            this.update(cx, |this, cx| {
-                                this.on_lsp_work_end(
-                                    language_server.server_id(),
-                                    id.to_string(),
-                                    cx,
-                                );
-                            })
-                        })
-                        .log_err();
-                    }
-
-                    result
+                        .await
                 });
             }
         } else if let Some(project_id) = self.remote_id() {