Skip to content

fix(lsp): omit params on shutdown and exit instead of sending null - #404

Merged
bug-ops merged 2 commits into
bug-ops:mainfrom
dergachoff:fix/omit-null-params-on-shutdown
Sep 9, 2026
Merged

fix(lsp): omit params on shutdown and exit instead of sending null#404
bug-ops merged 2 commits into
bug-ops:mainfrom
dergachoff:fix/omit-null-params-on-shutdown

Conversation

@dergachoff

Copy link
Copy Markdown
Contributor

Problem

Every mcpls shutdown against tsgo waits the full 3s CHILD_EXIT_GRACE and kills the child. Closes #403.

Root cause

LspClient::request/notify always emit Some(params), so the Value::Null passed by LspServer::shutdown serializes as "params": null. tsgo rejects that on shutdown with -32602 expected empty, got: null and ignores the following exit.

Fix

request and notify map Value::Null to None; the notification send path now serializes through JsonRpcNotification so skip_serializing_if drops the key. Non-null params are unchanged. Fixed in the client rather than the two call sites so any future params: void method is covered. Changelog entry under Unreleased.

Validation

  • cargo +nightly fmt --check
  • cargo clippy --all-targets --all-features --workspace -- -D warnings (stable 1.98.1)
  • cargo nextest run --workspace: 812 passed
  • New tests in client.rs::tests::void_params_wire: request with null params omits the key, notify with null params omits the key, notify with {} params keeps it
  • Live: release build against tsgo 7.0.0-dev.20260611.2, shutdown returns result: null and the child exits in under 10ms with LSP server shut down successfully, versus the 3s kill fallback on v0.5.0

dergachoff added a commit to dergachoff/mcpls that referenced this pull request Sep 9, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation rust Rust code changes mcpls-core mcpls-core crate changes labels Sep 9, 2026
@bug-ops
bug-ops requested a lite review from Copilot September 9, 2026 16:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is narrowly scoped, aligns with JSON-RPC optional params semantics, and is covered by focused wire-level tests for both request and notification paths.

Pull request overview

This PR fixes JSON-RPC wire output for LSP params: void methods by omitting the params field entirely (instead of sending "params": null), preventing tsgo from rejecting shutdown/exit and allowing the LSP server process to exit promptly.

Changes:

  • Map serde_json::Value::Null request/notification params to None so #[serde(skip_serializing_if = "Option::is_none")] drops the params key on the wire.
  • Serialize notifications via JsonRpcNotification (instead of a manual json! object) to consistently apply skip_serializing_if.
  • Add wire-level tests asserting that null params omit the key while {} params are preserved; add a changelog entry under Unreleased.
File summaries
File Description
crates/mcpls-core/src/lsp/client.rs Omits params when null for requests/notifications, switches notification serialization to JsonRpcNotification, and adds wire-format tests.
CHANGELOG.md Documents the tsgo shutdown/exit fix under ## [Unreleased].
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

tsgo rejects `"params": null` on `shutdown` with -32602 and never exits,
so every shutdown fell through to the 3s kill fallback. Null params are now
dropped in LspClient::request/notify so the key is omitted on the wire.
@bug-ops
bug-ops force-pushed the fix/omit-null-params-on-shutdown branch from 25c3c11 to 3c09318 Compare September 9, 2026 16:46
@bug-ops

bug-ops commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Thanks for the fix: clean root cause analysis, and appreciate routing it through the client layer.

@bug-ops
bug-ops enabled auto-merge (squash) September 9, 2026 16:49
@bug-ops
bug-ops merged commit 2b8798e into bug-ops:main Sep 9, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation mcpls-core mcpls-core crate changes rust Rust code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shutdown and exit are sent with "params": null, tsgo rejects them and mcpls hits the 3s kill fallback

3 participants