Repository navigation
Shut down on stdin EOF/close so host death can't orphan LSP children - #3
Merged
carldaws merged 9 commits intoSep 25, 2026
Merged
Conversation
A host that dies (SIGKILL) or closes the stdio pipe leaves squiggles and every LSP server it spawned alive indefinitely. SIGINT/SIGTERM were the only shutdown triggers, and the MCP TS SDK never surfaces stdin EOF — StdioServerTransport only wires 'data'/'error' listeners. Watch process.stdin end/close and server.onclose (the SDK reassigns transport.onclose inside connect(), so hooking the transport would clobber its handler), guarded so shutdown runs exactly once; all five triggers share one rejection-safe wrapper. Also suppress EPIPE on stdout/stderr: after host death those pipes are dead, and the resulting 'error' event used to kill the process mid-shutdown, before the LSP children could be reaped. Fixes carldaws#2
Spawn squiggles with a dummy LSP config, trigger lazy LSP spawn via a tools/call, close the host stdin, and assert both processes exit. The dummy fixture answers initialize only (it never answers shutdown), so the only path to reaping it is squiggles's kill() — exactly the path that used to be skipped when EPIPE killed the parent mid-shutdown. Verified the test fails on the pre-fix build (squiggles hangs past 15 s, LSP orphaned) and passes on the fixed build.
shutdown log line appears, not just a clean exit. The previous form of the test passed on the pre-fix build too, because on a project with no LSPs Node's event loop drains naturally once stdin is gone and the process exits 0 by accident — never entering our shutdown path. Asserting that stderrBuf contains 'Shutting down...' catches the actual regression: only the fixed build takes the explicit shutdown path.
- raceWithTimeout helper: exit-listener timeouts are always cleared when the child wins the race (previously the 15s/5s timers outlived the test and fired into a finished run) - wait-for-connect polling: the interval is cleared on the timeout path too, not just on success - replace the fixed 300ms reap sleep with a deadline-bounded poll - heavy test's finally now also kills a leftover dummy LSP on failure - spawn and dummy-LSP config use process.execPath instead of "node"
Interpolating bare paths into a double-quoted YAML flow scalar breaks the moment a path contains a quote or backslash. Use the same escaping as renderConfig in init.ts (JSON.stringify per element) — JSON string escaping is a valid YAML flow scalar.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical shutdown-listener and test execution/coverage issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds host-disconnect shutdown handling to prevent orphaned LSP child processes.
Changes:
- Handles stdin, transport, and signal shutdowns idempotently.
- Suppresses broken-pipe errors during shutdown.
- Adds integration tests and a dummy LSP fixture.
| File | Description |
|---|---|
src/index.ts |
Adds shutdown triggers and EPIPE handling. |
src/__tests__/index.shutdown.test.ts |
Tests stdin-close shutdown and LSP cleanup. |
src/__tests__/__shutdown_fixtures__/dummy-lsp.mjs |
Provides a hanging LSP test fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
|
Thanks for this, it's a great fix, and the tests that fail on the old build made it easy to review. I've pushed two commits on top to match the repo's style:
The Copilot notes were already handled by your later commits, so I've resolved them. I'll squash-merge once CI is green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2.
Problem
When the MCP host dies (SIGKILL, crash, or closing the stdio pipe), squiggles stayed alive holding every LSP server it spawned — the whole tree got reparented and kept consuming memory.
src/index.tsonly handled SIGINT/SIGTERM, and the MCP TS SDK'sStdioServerTransportnever surfaces stdin EOF (it only wiresdata/errorlisteners), so nobody reacted to the one shutdown signal guaranteed to arrive.Fix
In
src/index.ts:process.stdinend/closeandserver.onclose— per your note on the issue, the SDK reassignstransport.oncloseinsideconnect(), so the server hook is the safe one. All routed to the existingshutdownAll()path.errorevent used to kill the process mid-shutdown, beforekill()could reap the LSP children. Found by an adversarial review pass and confirmed with a realistic fixture (LSP that answersinitializeand hangs onshutdown): pre-fix, host SIGKILL leaked the child 2/2; with suppression, reaped 2/2.Tests
Two integration tests in
src/__tests__/index.shutdown.test.ts, plus a dummy LSP fixture that answersinitializeonly — so the only way to reap it iskill():tools/calltriggers a lazy LSP spawn → host closes stdin → assert both squiggles and the LSP exit (~0.6 s).Shutting down...log line appears (~0.2 s).Both are verified to fail on the pre-fix build. The light test's log assertion is load-bearing: on a no-LSP project the pre-fix build also exits 0 by accident (Node's event loop drains once every fd is idle), so a bare exit-code check can't tell the builds apart.