Ensure stdio client closes underlying process #818
Conversation
|
/cc @Skn0tt pinging for a review :] |
felixweinberger
force-pushed
the
main
branch
from
0845a57 to
c94ba4b
Compare
|
Rebased and polished tests. It seems that something unrelated from main is affecting CI. I ran the affected tests locally and they passed. |
commit: |
|
Hey, I'd be happy to look at getting this merged if you could fix ci :) |
|
Hey @mattzcarey, I'll take a look |
Co-authored-by: TomasRup <[email protected]>
Ensure proper disposal of mock streams to prevent resource leaks and avoid process hangs.
|
I'm looking into reproducing the error that happens in CI. On my mac all tests pass |
|
I get the same results on a linux x64 machine with node 24 |
- Make test servers use ts and init them with tsx - Add test to check if hanging server actually gets killed
|
@mattzcarey after a battle with CI 😅 this is good to review |
mattzcarey
requested review from
KKonstantinov,
Skn0tt and
felixweinberger
and removed request for
Skn0tt and
ihrpr
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
LGTM, though would be a great opportunity to match the behavior to the spec more precisely while we're in this space
| }); | ||
| }); | ||
|
|
||
| this._abortController.abort(); |
There was a problem hiding this comment.
I think according to the spec this should be after the stdin closure.
There was a problem hiding this comment.
|
|
||
| // waits the underlying process to exit cleanly otherwise after 1s kills it | ||
| await Promise.race([closePromise, new Promise(resolve => setTimeout(resolve, 1_000).unref())]); | ||
|
|
There was a problem hiding this comment.
Should we add a step to send a SIGTERM if this didn't end the process to match the spec?
https://modelcontextprotocol.io/specification/2025-11-25/basic/lifecycle#stdio
nit: should we increase the timeout to 2s to match the Python SDK?
There was a problem hiding this comment.
Pretty sure abort calls SIGTERM already. Changed to 2s :)
|
Hey @johnnyasantoss we merged #1200 since I couldnt push to this HEAD. You are credited in the commits, closing this as completed :) |
4 participants
Fixes #271
Motivation and Context
I'm also seeing a bunch of process get forgotten on my computer (especially when using
docker run --rm -i ...)How Has This Been Tested?
Yes, both with a node server and a server running on a container (my usual problem as docker does not necessarily delivers signals)
Breaking Changes
none
Types of changes
Checklist
Additional context
I followed the work previously done by #314