fix: don't panic the LSP server on malformed client input - #6173
fix: don't panic the LSP server on malformed client input#6173prql-bot wants to merge 3 commits into
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Self-review. The fix itself checks out: main_loop's only caller is updated, the MethodMismatch fallthrough is unchanged, and the exit-notification arm that lets the test's server thread terminate is pre-existing (added in an earlier change), not something the test depends on this PR to add.
I verified the CI change actually does what its comment claims, since a feature-list edit that silently resolves to nothing would leave the regression test dead: cargo tree --workspace --no-default-features --features=default,test-dbs-external,lsp -i lsp-server resolves lsp-server v0.10.0 └── prqlc, and the same command without lsp errors with did not match any packages. cli::lsp is behind #[cfg(feature = "lsp")] in cli/mod.rs and cli is a module of main.rs, so the test is a --bins unit test — it runs under nextest, but note cargo test -p prqlc --lib will never pick it up if you're iterating locally. Ran it both ways: passes with the feature, and cargo clippy --all-targets --features=default,lsp -- -D warnings and cargo fmt --check are clean.
One finding, inline: the test blocks on an untimed recv().
prql-bot
left a comment
There was a problem hiding this comment.
Self-review of the follow-up commit. The timeout change addresses the previous finding correctly: both waits are bounded, and routing the loop's result through an mpsc channel rather than JoinHandle::join is what makes the second wait boundable. I re-checked the terminate-after claim in the doc comment — .config/nextest.toml sets slow-timeout periods only, with no terminate-after anywhere, so an unbounded wait really would have run out the job's clock rather than being killed. cli::lsp::tests::invalid_params_get_an_error_response passes, and cargo clippy --all-targets --no-default-features --features=default,lsp -- -D warnings and cargo fmt --check are clean.
One gap: of the two panic sites this PR exists to fix, only the textDocument/definition one has a regression test. The run() side — dropping the InitializeParams deserialization — is not covered by anything. The two pre-existing cli::test::lsp* tests send params: {"capabilities": {}}, which deserializes fine under the old code too (serde treats the missing processId as None, since implicit-default applies to Option fields), so neither of them exercises the removed unwrap. Nothing currently stops someone re-introducing a typed deserialize there. I pushed a test that closes it — lsp_unmodelled_initialize_params, sending initialize with params: {}. capabilities: ClientCapabilities in lsp-types 0.97 is a non-Option field with no #[serde(default)], so it's the minimal payload that fails; against the base lsp.rs the test fails with a panic — missing field "capabilities" at lsp.rs:45 — and exit code 101, and it passes on this branch.
Unrelated to the diff: build-prqlc (macos-15, aarch64-apple-darwin, default,test-dbs) is red on Failed to CreateArtifact: Unable to make request: ENOTFOUND in actions/upload-artifact — an infra flake, not a build failure, and not part of check-ok-to-merge (which is green). The push re-triggers it.
The two pre-existing lsp CLI tests send `params: {"capabilities": {}}`, which deserializes into `InitializeParams` fine, so neither exercised the `unwrap` that was removed. `params: {}` omits the required `capabilities` field and panics the server against the base.
The LSP stub takes the whole server down on input a client controls. Two sites, both flagged by CLAUDE.md's "never panic on user input or recoverable errors" rule:
main_loopdeserialized the initialize payload intoInitializeParamsand.unwrap()ed it — into a binding (_params) that is never read. Any client whose payload doesn't match our pinnedlsp-types0.97 model killed the server before the loop even started, for a value we discard.textDocument/definitionrequest whose params fail to deserialize hitpanic!("{err:?}"). One malformed request from one client ends the session; a conforming client sees the transport close rather than an error for the request it sent.This drops the unused deserialization (the handshake itself is what matters, and
connection.initializestill performs it) and replies to undeserializable params with a JSON-RPCInvalidParamserror, leaving the loop running. Theserde_json::to_valueon our own fixedServerCapabilitiesis genuinely infallible, so it becomes.expectwith a reason rather than a bareunwrap.The regression test drives
main_loopoverConnection::memory(): it sendstextDocument/definitionwith{"not": "a position"}, asserts anInvalidParamsresponse comes back with the right request id, then sendsexitand joins the server thread to confirm the loop survived. Against the old code it fails the way the bug does — the server thread panics withJsonError { ... missing field "textDocument" }and the test'srecv()getsRecvErrorfrom the closed channel.lspis off by default and wasn't in any CI feature set, so the test would never have run. Thex86_64-unknown-linux-gnuPR-path leg now passesdefault,test-dbs-external,lsp. Both crates are already inCargo.lockas optional deps, so this adds two small crates to that leg's build and no new resolution.Scope note, not changed here: requests other than
textDocument/definitionstill fall through theMethodMismatcharm with no response at all, so a conforming client waits indefinitely rather than gettingMethodNotFound. That's a behavior change beyond fixing the panics, and the stub answers no other method today, so I left it — happy to follow up if it's wanted.