🐛 fix: exit when the MCP client closes stdio, releasing spawned LSP servers (fixes #47) - #53
Open
sudhirj wants to merge 2 commits into
Open
🐛 fix: exit when the MCP client closes stdio, releasing spawned LSP servers (fixes #47)#53sudhirj wants to merge 2 commits into
sudhirj wants to merge 2 commits into
Conversation
An MCP client that exits simply closes its end of the pipe, usually without signalling us. cclsp handled SIGINT and SIGTERM only, so on a plain stdio disconnect it kept running — and every LSP server it had spawned stayed resident with it. Long-lived servers are not cheap (pylsp holds a few hundred MB), so across repeated client invocations the orphans accumulate into real memory pressure. StdioServerTransport subscribes to stdin's 'data' and 'error' events but never 'end' or 'close', so Server.onclose does not fire on EOF and the SDK offers no hook for this. Listen for stdin ending directly. Handlers now live in src/shutdown.ts behind installShutdownHandlers(), taking the process structurally so the behaviour is unit-testable, and guarding against disposing twice — stdin emits both 'end' and 'close', and signals can race. SIGHUP is covered too, which a terminal closing sends and which was previously ignored. Reported for Windows in ktnyt#47, but nothing here is platform-specific: the missing handler is missing everywhere. Reproduced on Linux by closing the client's end of stdio and confirming both cclsp and its tsc child survive; after this change the process exits 0 and takes the child with it.
There was a problem hiding this comment.
Pull request overview
This PR fixes a lifecycle leak in cclsp’s MCP stdio mode by ensuring the server exits (and disposes spawned LSP servers) when the MCP client disconnects by closing its end of stdin, not just when POSIX signals are delivered.
Changes:
- Add centralized shutdown handler installation (
installShutdownHandlers) that listens forstdinEOF/close and termination signals. - Wire the new shutdown path into
index.ts, including routingmain().catch(...)through the same shutdown function. - Add unit tests covering signals, stdin events, and idempotency for multiple shutdown triggers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/shutdown.ts |
Introduces shutdown handler installation for stdin EOF/close + signals, and returns a callable shutdown function. |
src/shutdown.test.ts |
Adds unit tests verifying handler registration, behavior on stdin/signal triggers, and idempotency. |
index.ts |
Switches from inline SIGINT/SIGTERM handling to using installShutdownHandlers, and reuses it for fatal main() errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`ServerManager.dispose()` kills each child process without a guard, and `kill()` can throw — EPERM on a process we no longer own. With the call sitting bare before `proc.exit()`, that throw skipped the exit and left cclsp resident with every server it spawned, which is the exact leak this handler exists to close. Dispose inside a `try`, log the failure, and exit from a `finally`.
sudhirj
added a commit
to Common-Pattern/cclsp
that referenced
this pull request
Aug 19, 2026
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 #47.
Not Windows-specific
#47 is reported on Windows, but nothing about the cause is platform-dependent — the handler is simply absent, so the behaviour is the same everywhere. Reproduced on Linux (Node 24, cclsp
main@ 93414a1) by doing what an exiting MCP client does: close the client's end of stdio, send no signal, and see what is left.With this change, the same script reports
exit code: 0and both processes gone.(The aliveness check reads
/proc/<pid>/statand treats stateZas dead — an exited-but-unreaped child still has a/procentry, and checking only for existence reports a zombie as alive. Worth mentioning in case anyone else writes this test.)Cause
index.tshandledSIGINTandSIGTERMonly. A client that exits normally closes the pipe without signalling, so neither fires.The SDK does not help here:
StdioServerTransportsubscribes to stdin'sdataanderrorevents but neverendorclose, soServer.onclosenever fires on EOF. There is no transport-level hook to use, and stdin has to be observed directly.The cost is what #47 describes — each orphaned server holds its memory (pylsp a few hundred MB), so repeated client invocations accumulate them.
Change
Handlers move into
src/shutdown.tsbehindinstallShutdownHandlers(client, proc = process):endandclose— the actual fix.SIGHUPalongside the existingSIGINT/SIGTERM. A closing terminal sends it, and it was previously ignored, leaking the same way.endandclose, and signals can race, sodispose()must not run twice.procis taken structurally, so the behaviour is unit-testable without spawning processes.main().catch(...)now routes through the same path instead of duplicating dispose-then-exit.Testing
src/shutdown.test.ts, 8 cases: each signal, each stdin event, the multiple-trigger case, and that the returned shutdown function follows the same path.Branched from
main@ 93414a1, where the suite already reports203 pass / 5 skip / 19 fail; those 19 are untouched by this change.Note on scope
This kills the LSP child and exits immediately, matching what
SIGINT/SIGTERMalready did. It does not wait for children to acknowledge termination or escalate toSIGKILL— a server ignoringSIGTERMwould still be orphaned. That seemed like a separate concern, but happy to add it if you would rather this be belt-and-braces.Independent of #52; either can merge first.