From 07fa666c4e5d653d49eb6aab2f85c3e2bab67e73 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 7 Jul 2026 17:59:03 -0400 Subject: [PATCH] language_extension: Hold LspStore weakly in LspAccess::ViaLspStore (#60558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Fix a Windows CI flake in `extension_host extension_store_test::test_extension_store_with_test_extension`, which panicked in GPUI's leak detector with three leaked `LspStore` handles ([example run](https://github.com/zed-industries/zed/actions/runs/28871529605/job/85635418351)). ## Solution `LspAccess::ViaLspStore` held a strong `Entity`, cloned into three `ExtensionHostProxy` registrations by `language_extension::init`. The proxy sits inside an `Arc` cycle (proxy → `LanguageServerRegistryProxy` → `LanguageRegistry` → `ExtensionLspAdapter` → `WasmExtension` → `WasmHost` → proxy), so whenever an extension LSP adapter was registered at app-drop time, the cycle pinned the `LspStore` entity after its owning `Project` dropped. The flake was purely timing: extension reload toggles the adapter registration, and an unclean LSP pipe shutdown on Windows shifted teardown into the pinned window. `ViaLspStore` now holds a `WeakEntity`, upgraded at its sole use site (`remove_language_server`), skipping the stop task when the store is gone — matching the existing `ViaWorkspaces` semantics. A dead store is the expected terminal state after the owning project drops, so no error is logged or propagated. `Project`/`HeadlessProject` remain the sole long-term owners of `LspStore`, which is already the convention everywhere else (e.g. `json_schema_store`, the `lsp_store` message handlers). ## Testing - Ran `cargo nextest run -p extension_host extension_store_test::test_extension_store_with_test_extension` locally: passes. - The flake is timing-dependent (reproduced on Windows CI), so a local pass doesn't prove absence; the fix removes the only strong non-owner handle, which the leak detector reported. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - N/A --------- Co-authored-by: zed-zippy[bot] <234243425+zed-zippy[bot]@users.noreply.github.com> --- .../src/extension_store_test.rs | 6 ++++- .../src/extension_lsp_adapter.rs | 23 +++++++++++-------- .../src/language_extension.rs | 4 ++-- crates/remote_server/src/headless_project.rs | 2 +- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/extension_host/src/extension_store_test.rs b/crates/extension_host/src/extension_store_test.rs index 59b00f936f..aae9dbb942 100644 --- a/crates/extension_host/src/extension_store_test.rs +++ b/crates/extension_host/src/extension_store_test.rs @@ -769,7 +769,11 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { theme_extension::init(proxy.clone(), theme_registry.clone(), cx.executor()); let language_registry = project.read_with(cx, |project, _cx| project.languages().clone()); language_extension::init( - LspAccess::ViaLspStore(project.update(cx, |project, _| project.lsp_store())), + LspAccess::ViaLspStore( + project + .update(cx, |project, _| project.lsp_store()) + .downgrade(), + ), proxy.clone(), language_registry.clone(), ); diff --git a/crates/language_extension/src/extension_lsp_adapter.rs b/crates/language_extension/src/extension_lsp_adapter.rs index 3c28e07e6b..44092bf263 100644 --- a/crates/language_extension/src/extension_lsp_adapter.rs +++ b/crates/language_extension/src/extension_lsp_adapter.rs @@ -79,16 +79,19 @@ impl ExtensionLanguageServerProxy for LanguageServerRegistryProxy { let mut tasks = Vec::new(); match &self.lsp_access { - LspAccess::ViaLspStore(lsp_store) => lsp_store.update(cx, |lsp_store, cx| { - let stop_task = lsp_store.stop_language_servers_for_buffers( - Vec::new(), - HashSet::from_iter([LanguageServerSelector::Name( - language_server_name.clone(), - )]), - cx, - ); - tasks.push(stop_task); - }), + LspAccess::ViaLspStore(lsp_store) => { + if let Ok(stop_task) = lsp_store.update(cx, |lsp_store, cx| { + lsp_store.stop_language_servers_for_buffers( + Vec::new(), + HashSet::from_iter([LanguageServerSelector::Name( + language_server_name.clone(), + )]), + cx, + ) + }) { + tasks.push(stop_task); + } + } LspAccess::ViaWorkspaces(lsp_store_provider) => { if let Ok(lsp_stores) = lsp_store_provider(cx) { for lsp_store in lsp_stores { diff --git a/crates/language_extension/src/language_extension.rs b/crates/language_extension/src/language_extension.rs index 96536b6c02..ffddd3d90f 100644 --- a/crates/language_extension/src/language_extension.rs +++ b/crates/language_extension/src/language_extension.rs @@ -5,13 +5,13 @@ use std::sync::Arc; use anyhow::Result; use extension::{ExtensionGrammarProxy, ExtensionHostProxy, ExtensionLanguageProxy}; -use gpui::{App, Entity}; +use gpui::{App, Entity, WeakEntity}; use language::{LanguageMatcher, LanguageName, LanguageRegistry, LoadedLanguage}; use project::LspStore; #[derive(Clone)] pub enum LspAccess { - ViaLspStore(Entity), + ViaLspStore(WeakEntity), ViaWorkspaces(Arc Result>> + Send + Sync + 'static>), Noop, } diff --git a/crates/remote_server/src/headless_project.rs b/crates/remote_server/src/headless_project.rs index 57f910b5ea..db9ff6524c 100644 --- a/crates/remote_server/src/headless_project.rs +++ b/crates/remote_server/src/headless_project.rs @@ -254,7 +254,7 @@ impl HeadlessProject { cx.subscribe(&lsp_store, Self::on_lsp_store_event).detach(); language_extension::init( - language_extension::LspAccess::ViaLspStore(lsp_store.clone()), + language_extension::LspAccess::ViaLspStore(lsp_store.downgrade()), proxy.clone(), languages.clone(), );