Fix settings schema missing extension-provided LSP settings on remote - #62355
Fix settings schema missing extension-provided LSP settings on remote#62355vancez wants to merge 5 commits into
Conversation
Three changes ensure that language servers provided by extensions are included in the settings schemas served by remote servers: - Register connections opened via open_remote_project_with_existing_connection (dev servers, multi-workspace reopen, git worktree picker) with the extension store, so that extensions are synced to those servers. Previously only the open_remote_project flow did this, so setup-N servers never received any extension sync and their schemas omitted extension language servers. - Emit ExtensionsInstalledChanged from HeadlessExtensionStore after sync and install complete, so that the remote process invalidates its cached settings schemas. The local ExtensionStore already emitted this event; without it the remote schema cache stayed stale for the lifetime of the server process. - Include available LSP adapters in the project settings schema, matching the user settings schema (this was fixed for 'settings' in zed-industries#46766 but never applied to 'project_settings').
|
We require contributors to sign our Contributor License Agreement, and we don't have @vancez on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
zed-community-bot
Bot
added
the
first contribution
|
@cla-bot check |
cla-bot
Bot
added
the
cla-signed
|
The cla-bot has been summoned, and re-checked this pull request! |
dinocosta
added
platform:remote
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Thank you, but we cannot change the dependencies of a fundamental crate that easy.
I think there's a way out, posted that and a few style notes.
- Add remote::OnRemoteClientCreated global callback invoked when a remote project is created, covering dev server, worktree picker and other connection paths - Replace hand-written dedup with sort_unstable + dedup in settings schema handlers - Remove redundant comments
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Thank you, this looks almost ready.
Main concern is that
definitely not there yet, can we add one, otherwise it may regress in the future?
- Emit RemoteClientEvent::Reconnected when a remote connection re-establishes, and re-sync extensions to that client since a restarted server process starts with an empty extension store - Sync only the newly registered client in register_remote_client instead of re-syncing every registered connection - Add a test covering the OnRemoteClientCreated callback invoked when a remote project is created
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Still seeing a bunch of this in new subscriptions if you want to fix more along the way with the other fixes.
| .await | ||
| .log_err(); | ||
| }) | ||
| .detach(); |
There was a problem hiding this comment.
Before this PR, every sync went through ssh_registered_tx into the single loop task in ExtensionStore::new, so syncs to one server were serialized.
A detached task here is unordered relative to that loop, so a registration sync can race a debounce-driven update_remote_clients pass to the same client (window: opening a remote project within RELOAD_DEBOUNCE_DURATION of an extension index change).
Both would see the same missing_extensions, so the worst case per collision is a duplicate upload plus the losing install_extension uninstalling and reinstalling the extension the winner just loaded.
The detached future also captures the strong client handle across the whole upload, keeping the RemoteClient alive after the project is closed.
I think we do not need the spawn at all: make the channel carry a payload, e.g. UnboundedSender<Option<WeakEntity<RemoteClient>>>, where Some means "sync only this client, without the debounce" and None keeps the current debounced full pass for reconnects?
Then this function goes back to a synchronous push + send as before the PR, the loop task stays the sole sync owner, and we still avoid the K round-trips from the other comment.
|
|
||
| cx.subscribe(&client, |this, _client, event, _cx| { | ||
| if matches!(event, RemoteClientEvent::Reconnected) { | ||
| this.ssh_registered_tx.unbounded_send(()).ok(); |
There was a problem hiding this comment.
Great, thank you for the reconnect fix: but this is also syncing all the clients now?
I think if we do the .detach() fix below that should go away due to Option<WeakEntity usage, but wanted to raise the awareness as something to be re-checked when fixed.
| } | ||
|
|
||
| #[gpui::test] | ||
| async fn test_remote_project_creation_triggers_on_remote_client_created_callback( |
There was a problem hiding this comment.
That's a great start, thank you — do we want to cover the reconnect case too, as there's definitely some issues with that based on the other comments.
| } | ||
| } | ||
|
|
||
| if extensions_changed { |
There was a problem hiding this comment.
If every load fails or all are missing (e.g. on the very initial load), we would notify for nothing?
Seems redundant to do so and we can check things better to exclude this case.
- Registering a client syncs only that client through the extension sync channel instead of a detached task, keeping the loop task the sole sync owner - Reconnects still trigger a debounced pass over all clients - Notify listeners only when extensions were actually loaded or removed - Add tests for per-client sync on registration and re-sync on reconnect
3 participants
Objective
Fixes an issue where language servers provided by extensions are not included in the settings schemas served by remote servers. When connecting to a remote server via the
open_remote_project_with_existing_connectionflow (dev servers, multi-workspace reopen, git worktree picker), setup-N servers never received any extension sync, so their schemas omitted extension-provided language servers, breaking language support for remote development.Solution
Three changes ensure that language servers provided by extensions are included in the settings schemas served by remote servers:
Register connections opened via
open_remote_project_with_existing_connectionwith the extension store: dev servers, multi-workspace reopen, and the git worktree picker now register their connections with the extension store so that extensions are synced to those servers. Previously only theopen_remote_projectflow did this, so setup-N servers never received any extension sync and their schemas omitted extension language servers.Emit
ExtensionsInstalledChangedfromHeadlessExtensionStoreafter sync and install complete: this allows the remote process to invalidate its cached settings schemas. The localExtensionStorealready emitted this event; without it, the remote schema cache stayed stale for the lifetime of the server process.Include available LSP adapters in the project settings schema: this matches the user settings schema (this was fixed for
settingsin json_schema_store: Include available LSP adapters in settings schema #46766 but never applied toproject_settings).Testing
open_remote_project_with_existing_connectionflow (including dev servers and the git worktree picker), extension language servers appear in the schema served by the remote server.ExtensionsInstalledChangedis emitted after sync and install complete, and that the remote schema cache is correctly invalidated and rebuilt.project_settingsschema includes the available LSP adapters, matching the behavior of the user settings schema.Self-Review Checklist:
Showcase
Server / Project — before the fix
Local
Release Notes: