fix(lsp): do not sent de-registrations for watched files on shutdown request#17318
Conversation
github-actions
bot
added
A-editor
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Sysix
force-pushed
the
12-23-fix_lsp_do_not_sent_de-registrations_for_watched_files_on_shutdown_request
branch
from
ba077b9 to
2e138a3
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the Helix editor would timeout waiting for the language server to shutdown. The fix removes the capability unregistration requests for file watchers during shutdown, based on the understanding that clients should automatically handle cleanup of registrations when a server shuts down.
- Removed watcher unregistration logic during the shutdown process
- Updated test infrastructure to no longer expect or acknowledge unregistration requests during shutdown
- Simplified the shutdown flow to only clear diagnostics and the file system
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/oxc_language_server/src/backend.rs |
Removed the unregistration logic for file watchers in the shutdown() method and updated documentation to reflect that clients handle cleanup automatically |
crates/oxc_language_server/src/tests.rs |
Removed shutdown_with_watchers() test helper method and updated all test cases to use the simplified shutdown() method without expecting unregistration acknowledgments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sysix
force-pushed
the
12-23-fix_lsp_do_not_sent_de-registrations_for_watched_files_on_shutdown_request
branch
from
2e138a3 to
917c6a9
Compare
Merge activity
|
…request (#17318) TLDR: it is the client's fault to respect server registrations after shutdown Some client (like helix editor) do not send an acknowledgment to the deregistration, because it already sent the shutdown request. (Some) notification are somehow possible. [The LSP Specs](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#shutdown) are unclear about this behavior. So to support fully one more editor, we expect that the deregistration of the file watchers are automatically handled by the client. Why should he still watch for them, when can not report to the server :) closes #17190
graphite-app
bot
force-pushed
the
12-23-fix_lsp_do_not_sent_de-registrations_for_watched_files_on_shutdown_request
branch
from
917c6a9 to
b7e8bca
Compare
graphite-app
bot
deleted the
12-23-fix_lsp_do_not_sent_de-registrations_for_watched_files_on_shutdown_request
branch
|
@Sysix Can confirm, after this change Helix editor is no longer blocked by the ox lsp's, thank you for the fix. |
TLDR: it is the client's fault to respect server registrations after shutdown
Some client (like helix editor) do not send an acknowledgment to the deregistration, because it already sent the shutdown request. (Some) notification are somehow possible. The LSP Specs are unclear about this behavior.
So to support fully one more editor, we expect that the deregistration of the file watchers are automatically handled by the client. Why should he still watch for them, when can not report to the server :)
closes #17190