From e4f90b5da2b8a6d2b267edc02b333b7d7908d4d9 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 2 Oct 2025 22:14:12 -0600 Subject: [PATCH] Fix race-condition in autosave (#39409) This removes a long-standing thing we've done, which is send a `DidSave` notification to the language server for the clean parts of a multi-buffer. However, it seems like the intent of that notification is to tell the language server to reload the file from disk. As we didn't actually write those files to disk, it seems clearer to not send this notification; and just remove this whole code-path. Release Notes: - Fixed a race where autosave in a multibuffer could cause unsaved buffers to appear saved --- crates/editor/src/editor_tests.rs | 114 +++++++++++++++++++++++++++++- crates/editor/src/items.rs | 21 +----- crates/language/src/buffer.rs | 5 +- 3 files changed, 117 insertions(+), 23 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index ef7aadc2d0142174d1e22e86da3bbd6901e5aa29..c01e585023d1eb49c1656c182117ad5cbf573087 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -14,7 +14,7 @@ use crate::{ }; use buffer_diff::{BufferDiff, DiffHunkSecondaryStatus, DiffHunkStatus, DiffHunkStatusKind}; use collections::HashMap; -use futures::StreamExt; +use futures::{StreamExt, channel::oneshot}; use gpui::{ BackgroundExecutor, DismissEvent, Rgba, SemanticVersion, TestAppContext, UpdateGlobal, VisualTestContext, WindowBounds, WindowOptions, div, @@ -26296,6 +26296,118 @@ async fn test_paste_url_from_other_app_creates_markdown_link_selectively_in_mult )); } +#[gpui::test] +async fn test_race_in_multibuffer_save(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "first.rs": "# First Document\nSome content here.", + "second.rs": "Plain text content for second file.", + }), + ) + .await; + + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let workspace = cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + + let language = rust_lang(); + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + language_registry.add(language.clone()); + let mut fake_servers = language_registry.register_fake_lsp( + "Rust", + FakeLspAdapter { + ..FakeLspAdapter::default() + }, + ); + + let buffer1 = project + .update(cx, |project, cx| { + project.open_local_buffer(PathBuf::from(path!("/project/first.rs")), cx) + }) + .await + .unwrap(); + let buffer2 = project + .update(cx, |project, cx| { + project.open_local_buffer(PathBuf::from(path!("/project/second.rs")), cx) + }) + .await + .unwrap(); + + let multi_buffer = cx.new(|cx| { + let mut multi_buffer = MultiBuffer::new(Capability::ReadWrite); + multi_buffer.set_excerpts_for_path( + PathKey::for_buffer(&buffer1, cx), + buffer1.clone(), + [Point::zero()..buffer1.read(cx).max_point()], + 3, + cx, + ); + multi_buffer.set_excerpts_for_path( + PathKey::for_buffer(&buffer2, cx), + buffer2.clone(), + [Point::zero()..buffer1.read(cx).max_point()], + 3, + cx, + ); + multi_buffer + }); + + let (editor, cx) = cx.add_window_view(|window, cx| { + Editor::new( + EditorMode::full(), + multi_buffer, + Some(project.clone()), + window, + cx, + ) + }); + + let fake_language_server = fake_servers.next().await.unwrap(); + + buffer1.update(cx, |buffer, cx| buffer.edit([(0..0, "hello!")], None, cx)); + + let save = editor.update_in(cx, |editor, window, cx| { + assert!(editor.is_dirty(cx)); + + editor.save( + SaveOptions { + format: true, + autosave: true, + }, + project, + window, + cx, + ) + }); + let (start_edit_tx, start_edit_rx) = oneshot::channel(); + let (done_edit_tx, done_edit_rx) = oneshot::channel(); + let mut done_edit_rx = Some(done_edit_rx); + let mut start_edit_tx = Some(start_edit_tx); + + fake_language_server.set_request_handler::(move |_, _| { + start_edit_tx.take().unwrap().send(()).unwrap(); + let done_edit_rx = done_edit_rx.take().unwrap(); + async move { + done_edit_rx.await.unwrap(); + Ok(None) + } + }); + + start_edit_rx.await.unwrap(); + buffer2 + .update(cx, |buffer, cx| buffer.edit([(0..0, "world!")], None, cx)) + .unwrap(); + + done_edit_tx.send(()).unwrap(); + + save.await.unwrap(); + cx.update(|_, cx| assert!(editor.is_dirty(cx))); +} + #[track_caller] fn extract_color_inlays(editor: &Editor, cx: &App) -> Vec { editor diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 0571c0ea1c02696a21885ded04e322e458c54664..7a6a3cd464bb4156cc62b31558e57094a241fb58 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -832,12 +832,11 @@ impl Item for Editor { // let mut buffers_to_save = let buffers_to_save = if self.buffer.read(cx).is_singleton() && !options.autosave { - buffers.clone() + buffers } else { buffers - .iter() + .into_iter() .filter(|buffer| buffer.read(cx).is_dirty()) - .cloned() .collect() }; @@ -863,22 +862,6 @@ impl Item for Editor { .await?; } - // Notify about clean buffers for language server events - let buffers_that_were_not_saved: Vec<_> = buffers - .into_iter() - .filter(|b| !buffers_to_save.contains(b)) - .collect(); - - for buffer in buffers_that_were_not_saved { - buffer - .update(cx, |buffer, cx| { - let version = buffer.saved_version().clone(); - let mtime = buffer.saved_mtime(); - buffer.did_save(version, mtime, cx); - }) - .ok(); - } - Ok(()) }) } diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index ecb7c9d0460b9821270ba909cf6a76b44f6217a5..82b879861886982016b80b94ced9e5907a7e7d16 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -1312,9 +1312,8 @@ impl Buffer { mtime: Option, cx: &mut Context, ) { - self.saved_version = version; - self.has_unsaved_edits - .set((self.saved_version().clone(), false)); + self.saved_version = version.clone(); + self.has_unsaved_edits.set((version, false)); self.has_conflict = false; self.saved_mtime = mtime; self.was_changed();