fix: LSP server should only exit on the exit notification#6001
Conversation
The main loop returned on any notification, so the first non-exit notification a client sent (e.g. textDocument/didOpen) shut the server down. Returning also dropped the connection sender, panicking with "sending on a disconnected channel". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
prql-bot
left a comment
There was a problem hiding this comment.
The fix is correct — exit is the right notification to terminate on, and ignoring others matches LSP semantics. I confirmed both lsp tests pass once the binary is built with --features lsp.
One observation, not a blocker: this regression test won't run in CI. No test job enables the lsp feature — the test-rust matrix uses features: default,test-dbs-external and default (tests.yaml), lsp isn't in default (default = ["cli"]), and no workflow uses --all-features. So the #[cfg(feature = "lsp")] tests — both the new one and the pre-existing lsp test — are never compiled or run in CI; they only guard the fix for developers who run cargo test --features lsp locally.
This gap predates the PR (the existing lsp test has it too), so it's reasonable to leave as-is given lsp is a stub feature. But since the PR's value is the added regression coverage, a maintainer may want to add an lsp-enabled leg to the CI matrix in a follow-up so the test actually protects the fix.
Problem
The LSP server's
main_loopreturnedOk(())on any notification:Connection::initialize()consumes theinitializednotification during the handshake, so the first notification reachingmain_loopin a real session is something liketextDocument/didOpenor$/setTrace— and the server would shut down the moment a client sent one. Worse, returning early drops the connection's sender, so a follow-up message panics withsending on a disconnected channeland the process exits with code 1.The existing
lsptest only sent anexitnotification, so it never exercised a non-exitnotification and masked the bug.Solution
Only the
exitnotification stops the loop; other notifications are logged and ignored.Testing
Added
lsp_ignores_non_exit_notification, which sends a$/setTracenotification beforeexit. Before the fix it fails withsuccess: false,exit_code: 1, andsending on a disconnected channel; after the fix the server logs the notification, continues, and shuts down cleanly onexit. The existinglsptest is unchanged.Found during the nightly code-quality survey.