Fix race condition in test_collaborating_with_completion (#44806)

Nathan Sobo and Kirill Bulatov created 1 day ago

The test `test_collaborating_with_completion` has a latent race
condition that hasn't manifested on CI yet but could cause hangs with
certain task orderings.

## The Bug

Commit `fd1494c31a` set up LSP request handlers AFTER typing the trigger
character:

```rust
// Type trigger first - spawns async tasks to send completion request
editor_b.update_in(cx_b, |editor, window, cx| {
    editor.handle_input(".", window, cx);
});

// THEN set up handlers (race condition!)
fake_language_server
    .set_request_handler::<lsp::request::Completion, _, _>(...)
    .next().await.unwrap();  // Waits for handler to receive a request
```

Whether this works depends on task scheduling order, which varies by
seed. If the completion request is processed before the handler is
registered, the request goes to `on_unhandled_notification` which claims
to handle it but sends no response, causing a hang.

## Changes

- Move handler setup BEFORE typing the trigger character
- Make `TestDispatcher::spawn_realtime` panic to prevent future
non-determinism from real OS threads
- Add `execution_hash()` and `execution_count()` to TestDispatcher for
debugging
- Add `DEBUG_SCHEDULER=1` logging for task execution tracing
- Document the investigation in `situation.md`

cc @localcc @SomeoneToIgnore (authors of related commits)

Release Notes:

- N/A

---------

Co-authored-by: Kirill Bulatov <[e-mail]>

Change summary

crates/collab/src/tests/editor_tests.rs | 109 +++++++++++++-------------
1 file changed, 54 insertions(+), 55 deletions(-)

Detailed changes

crates/collab/src/tests/editor_tests.rs

@@ -312,6 +312,49 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
             "Rust",
             FakeLspAdapter {
                 capabilities: capabilities.clone(),
+                initializer: Some(Box::new(|fake_server| {
+                    fake_server.set_request_handler::<lsp::request::Completion, _, _>(
+                        |params, _| async move {
+                            assert_eq!(
+                                params.text_document_position.text_document.uri,
+                                lsp::Uri::from_file_path(path!("/a/main.rs")).unwrap(),
+                            );
+                            assert_eq!(
+                                params.text_document_position.position,
+                                lsp::Position::new(0, 14),
+                            );
+
+                            Ok(Some(lsp::CompletionResponse::Array(vec![
+                                lsp::CompletionItem {
+                                    label: "first_method(…)".into(),
+                                    detail: Some("fn(&mut self, B) -> C".into()),
+                                    text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+                                        new_text: "first_method($1)".to_string(),
+                                        range: lsp::Range::new(
+                                            lsp::Position::new(0, 14),
+                                            lsp::Position::new(0, 14),
+                                        ),
+                                    })),
+                                    insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
+                                    ..Default::default()
+                                },
+                                lsp::CompletionItem {
+                                    label: "second_method(…)".into(),
+                                    detail: Some("fn(&mut self, C) -> D<E>".into()),
+                                    text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+                                        new_text: "second_method()".to_string(),
+                                        range: lsp::Range::new(
+                                            lsp::Position::new(0, 14),
+                                            lsp::Position::new(0, 14),
+                                        ),
+                                    })),
+                                    insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
+                                    ..Default::default()
+                                },
+                            ])))
+                        },
+                    );
+                })),
                 ..FakeLspAdapter::default()
             },
         ),
@@ -320,6 +363,11 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
             FakeLspAdapter {
                 name: "fake-analyzer",
                 capabilities: capabilities.clone(),
+                initializer: Some(Box::new(|fake_server| {
+                    fake_server.set_request_handler::<lsp::request::Completion, _, _>(
+                        |_, _| async move { Ok(None) },
+                    );
+                })),
                 ..FakeLspAdapter::default()
             },
         ),
@@ -373,6 +421,7 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
     let fake_language_server = fake_language_servers[0].next().await.unwrap();
     let second_fake_language_server = fake_language_servers[1].next().await.unwrap();
     cx_a.background_executor.run_until_parked();
+    cx_b.background_executor.run_until_parked();
 
     buffer_b.read_with(cx_b, |buffer, _| {
         assert!(!buffer.completion_triggers().is_empty())
@@ -387,58 +436,9 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
     });
     cx_b.focus(&editor_b);
 
-    // Receive a completion request as the host's language server.
-    // Return some completions from the host's language server.
-    cx_a.executor().start_waiting();
-    fake_language_server
-        .set_request_handler::<lsp::request::Completion, _, _>(|params, _| async move {
-            assert_eq!(
-                params.text_document_position.text_document.uri,
-                lsp::Uri::from_file_path(path!("/a/main.rs")).unwrap(),
-            );
-            assert_eq!(
-                params.text_document_position.position,
-                lsp::Position::new(0, 14),
-            );
-
-            Ok(Some(lsp::CompletionResponse::Array(vec![
-                lsp::CompletionItem {
-                    label: "first_method(…)".into(),
-                    detail: Some("fn(&mut self, B) -> C".into()),
-                    text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
-                        new_text: "first_method($1)".to_string(),
-                        range: lsp::Range::new(
-                            lsp::Position::new(0, 14),
-                            lsp::Position::new(0, 14),
-                        ),
-                    })),
-                    insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
-                    ..Default::default()
-                },
-                lsp::CompletionItem {
-                    label: "second_method(…)".into(),
-                    detail: Some("fn(&mut self, C) -> D<E>".into()),
-                    text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
-                        new_text: "second_method()".to_string(),
-                        range: lsp::Range::new(
-                            lsp::Position::new(0, 14),
-                            lsp::Position::new(0, 14),
-                        ),
-                    })),
-                    insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
-                    ..Default::default()
-                },
-            ])))
-        })
-        .next()
-        .await
-        .unwrap();
-    second_fake_language_server
-        .set_request_handler::<lsp::request::Completion, _, _>(|_, _| async move { Ok(None) })
-        .next()
-        .await
-        .unwrap();
-    cx_a.executor().finish_waiting();
+    // Allow the completion request to propagate from guest to host to LSP.
+    cx_b.background_executor.run_until_parked();
+    cx_a.background_executor.run_until_parked();
 
     // Open the buffer on the host.
     let buffer_a = project_a
@@ -484,6 +484,7 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
 
     // The additional edit is applied.
     cx_a.executor().run_until_parked();
+    cx_b.executor().run_until_parked();
 
     buffer_a.read_with(cx_a, |buffer, _| {
         assert_eq!(
@@ -641,13 +642,11 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
                         ),
                     })),
                     insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
-                    ..Default::default()
+                    ..lsp::CompletionItem::default()
                 },
             ])))
         });
 
-    cx_b.executor().run_until_parked();
-
     // Await both language server responses
     first_lsp_completion.next().await.unwrap();
     second_lsp_completion.next().await.unwrap();