From 1150c9d6640933f41a4e57eb67c5ec503ba8f4bb Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Mon, 13 Apr 2026 16:30:38 -0400 Subject: [PATCH] Fix crash when fallback workspace is in the removal set (#53549) When removing a project group, the fallback closure calls `find_or_create_local_workspace` which searches `self.workspaces` for an existing match. Because the workspaces to be removed haven't been removed yet at that point, `workspace_for_paths` can return a doomed workspace, hitting the `assert!` in `MultiWorkspace::remove` and crashing the app. Fix by adding an `excluding` parameter to `find_or_create_local_workspace` so callers inside removal fallbacks can skip workspaces that are about to be removed. Both `remove_project_group` and `sidebar::archive_thread` now pass the appropriate exclusion set. Adds a regression test that deterministically reproduces the crash (the assert fires without the exclusion, passes with it). Release Notes: - Fixed a crash when closing a project group whose fallback workspace matched one being removed. --- crates/recent_projects/src/recent_projects.rs | 8 +- crates/sidebar/src/sidebar.rs | 3 +- crates/workspace/src/multi_workspace.rs | 26 ++++++- crates/workspace/src/multi_workspace_tests.rs | 2 + crates/workspace/src/persistence.rs | 76 +++++++++++++++++++ 5 files changed, 109 insertions(+), 6 deletions(-) diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index f9284fc572e41011c0fe8b204b70531a7af26f8d..f573250dbe98cf15adfbbf13d78a4d8e2ab0fb07 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -1135,8 +1135,12 @@ impl PickerDelegate for RecentProjectsDelegate { cx.defer(move |cx| { if let Some(task) = handle .update(cx, |multi_workspace, window, cx| { - multi_workspace - .find_or_create_local_workspace(path_list, window, cx) + multi_workspace.find_or_create_local_workspace( + path_list, + &[], + window, + cx, + ) }) .log_err() { diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 75a1ae961be2028fc8f63acf506412498156985f..aa5dd33df3e4bbaeaf06cebd519279415042b0d9 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -3193,11 +3193,12 @@ impl Sidebar { .clone() }); + let excluded = [workspace_to_remove.clone()]; let remove_task = multi_workspace.update(cx, |mw, cx| { mw.remove( [workspace_to_remove], move |this, window, cx| { - this.find_or_create_local_workspace(fallback_paths, window, cx) + this.find_or_create_local_workspace(fallback_paths, &excluded, window, cx) }, window, cx, diff --git a/crates/workspace/src/multi_workspace.rs b/crates/workspace/src/multi_workspace.rs index 081220f4766d5c5ae8a0fd73139dc8b0ffdb6ee6..4d87a3d4cddaa8263600508cc51b63a666e95f5c 100644 --- a/crates/workspace/src/multi_workspace.rs +++ b/crates/workspace/src/multi_workspace.rs @@ -987,12 +987,14 @@ impl MultiWorkspace { // Now remove the group. self.project_groups.retain(|group| group.key != *group_key); + let excluded_workspaces = workspaces.clone(); self.remove( workspaces, move |this, window, cx| { if let Some(neighbor_key) = neighbor_key { return this.find_or_create_local_workspace( neighbor_key.path_list().clone(), + &excluded_workspaces, window, cx, ); @@ -1069,8 +1071,21 @@ impl MultiWorkspace { path_list: &PathList, host: Option<&RemoteConnectionOptions>, cx: &App, + ) -> Option> { + self.workspace_for_paths_excluding(path_list, host, &[], cx) + } + + fn workspace_for_paths_excluding( + &self, + path_list: &PathList, + host: Option<&RemoteConnectionOptions>, + excluding: &[Entity], + cx: &App, ) -> Option> { for workspace in self.workspaces() { + if excluding.contains(workspace) { + continue; + } let root_paths = PathList::new(&workspace.read(cx).root_paths(cx)); let key = workspace.read(cx).project_group_key(cx); let host_matches = key.host().as_ref() == host; @@ -1116,7 +1131,7 @@ impl MultiWorkspace { } let Some(connection_options) = host else { - return self.find_or_create_local_workspace(paths, window, cx); + return self.find_or_create_local_workspace(paths, &[], window, cx); }; let app_state = self.workspace().read(cx).app_state().clone(); @@ -1168,13 +1183,18 @@ impl MultiWorkspace { /// or creates a new one (deserializing its saved state from the database). /// Never searches other windows or matches workspaces with a superset of /// the requested paths. + /// + /// `excluding` lists workspaces that should be skipped during the search + /// (e.g. workspaces that are about to be removed). pub fn find_or_create_local_workspace( &mut self, path_list: PathList, + excluding: &[Entity], window: &mut Window, cx: &mut Context, ) -> Task>> { - if let Some(workspace) = self.workspace_for_paths(&path_list, None, cx) { + if let Some(workspace) = self.workspace_for_paths_excluding(&path_list, None, excluding, cx) + { self.activate(workspace.clone(), window, cx); return Task::ready(Ok(workspace)); } @@ -1733,7 +1753,7 @@ impl MultiWorkspace { cx: &mut Context, ) -> Task>> { if self.multi_workspace_enabled(cx) { - self.find_or_create_local_workspace(PathList::new(&paths), window, cx) + self.find_or_create_local_workspace(PathList::new(&paths), &[], window, cx) } else { let workspace = self.workspace().clone(); cx.spawn_in(window, async move |_this, cx| { diff --git a/crates/workspace/src/multi_workspace_tests.rs b/crates/workspace/src/multi_workspace_tests.rs index 8e2345f2f495122e2cc1c6dd2e2d8e010a8a0539..fda616be2b10635a18057c6af2fb87e578c9ccc9 100644 --- a/crates/workspace/src/multi_workspace_tests.rs +++ b/crates/workspace/src/multi_workspace_tests.rs @@ -433,6 +433,7 @@ async fn test_find_or_create_local_workspace_reuses_active_workspace_when_sideba .update_in(cx, |mw, window, cx| { mw.find_or_create_local_workspace( PathList::new(&[PathBuf::from("/root_a")]), + &[], window, cx, ) @@ -491,6 +492,7 @@ async fn test_find_or_create_local_workspace_reuses_active_workspace_after_sideb .update_in(cx, |mw, window, cx| { mw.find_or_create_local_workspace( PathList::new(&[PathBuf::from("/root_a")]), + &[], window, cx, ) diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index e6ee8fbd6b9fb922cc84da2c128be866d35a2dd0..b13911efa7d249715fd0efca6b20296d2c3fbc9c 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -2495,6 +2495,7 @@ pub fn delete_unloaded_items( #[cfg(test)] mod tests { use super::*; + use crate::PathList; use crate::ProjectGroupKey; use crate::{ multi_workspace::MultiWorkspace, @@ -4989,4 +4990,79 @@ mod tests { "After removing the only remaining group, should have an empty workspace" ); } + + /// Regression test for a crash where `find_or_create_local_workspace` + /// returned a workspace that was about to be removed, hitting an assert + /// in `MultiWorkspace::remove`. + /// + /// The scenario: two workspaces share the same root paths (e.g. due to + /// a provisional key mismatch). When the first is removed and the + /// fallback searches for the same paths, `workspace_for_paths` must + /// skip the doomed workspace so the assert in `remove` is satisfied. + #[gpui::test] + async fn test_remove_fallback_skips_excluded_workspaces(cx: &mut gpui::TestAppContext) { + crate::tests::init_test(cx); + cx.update(|cx| { + cx.set_staff(true); + cx.update_flags(true, vec!["agent-v2".to_string()]); + }); + + let fs = fs::FakeFs::new(cx.executor()); + let dir = unique_test_dir(&fs, "shared").await; + + // Two projects that open the same directory — this creates two + // workspaces whose root_paths are identical. + let project_a = Project::test(fs.clone(), [dir.as_path()], cx).await; + let project_b = Project::test(fs.clone(), [dir.as_path()], cx).await; + + let (multi_workspace, cx) = cx + .add_window_view(|window, cx| MultiWorkspace::test_new(project_a.clone(), window, cx)); + + multi_workspace.update(cx, |mw, cx| mw.open_sidebar(cx)); + + let workspace_b = multi_workspace.update_in(cx, |mw, window, cx| { + mw.test_add_workspace(project_b.clone(), window, cx) + }); + cx.run_until_parked(); + + // workspace_a is first in the workspaces vec. + let workspace_a = + multi_workspace.read_with(cx, |mw, _| mw.workspaces().next().cloned().unwrap()); + assert_ne!(workspace_a, workspace_b); + + // Activate workspace_a so removing it triggers the fallback path. + multi_workspace.update_in(cx, |mw, window, cx| { + mw.activate(workspace_a.clone(), window, cx); + }); + cx.run_until_parked(); + + // Remove workspace_a. The fallback searches for the same paths. + // Without the `excluding` parameter, `workspace_for_paths` would + // return workspace_a (first match) and the assert in `remove` + // would fire. With the fix, workspace_a is skipped and + // workspace_b is found instead. + let path_list = PathList::new(std::slice::from_ref(&dir)); + let excluded = vec![workspace_a.clone()]; + multi_workspace.update_in(cx, |mw, window, cx| { + mw.remove( + vec![workspace_a.clone()], + move |this, window, cx| { + this.find_or_create_local_workspace(path_list, &excluded, window, cx) + }, + window, + cx, + ) + .detach_and_log_err(cx); + }); + cx.run_until_parked(); + + // workspace_b should now be active — workspace_a was removed. + multi_workspace.read_with(cx, |mw, _cx| { + assert_eq!( + mw.workspace(), + &workspace_b, + "fallback should have found workspace_b, not the excluded workspace_a" + ); + }); + } }