Align stdio shutdown sequence with spec - #765
Conversation
Sorry, something went wrong.
|
Hey @ihrpr! Do you have a sec to review this PR? It makes the SDK follow the stdio shutdown procedure to the letter! |
Sorry, something went wrong.
ihrpr
left a comment
There was a problem hiding this comment.
Thank you @davenpi for working on this.
There are a lot of issues reported around the shutdown for stdio, if would be great to have additional tests and verify on different platforms that the solution works.
Sorry, something went wrong.
felixweinberger
left a comment
There was a problem hiding this comment.
Hi @davenpi, thank you for making this PR!
As @ihrpr mentioned we have a couple of different fixes all related to shutdown behavior, and we'll need to reconcile this with #555 which unifies behavior across Windows and POSIX (no more special handling for Windows).
We'll want to land #555 first, and then land your change here on top - ideally with testing for both.
I went ahead and created #1044 with both of these changes + tests for each - if you could take a look at the last commit here (with tests for #765) and add them to this PR + rebase on main that would be great!
Sorry, something went wrong.
The MCP specification recommends closing stdin first to allow servers to exit gracefully before resorting to signals. This approach gives well-behaved servers the opportunity to detect stdin closure and perform clean shutdown without forceful termination. The shutdown sequence now follows a graceful escalation path: first closing stdin and waiting 2 seconds for voluntary exit, then sending SIGTERM if needed, and finally using SIGKILL as a last resort. This minimizes the risk of data loss or corruption while ensuring cleanup always completes. This unified approach works consistently across all platforms and improves compatibility with MCP servers that monitor stdin for lifecycle management. resolves #765 Co-authored-by: davenpi <davenport.ianc@gmail.com>
|
Thank you again @davenpi for submitting this PR. I spent yesterday and today trying to unify all the different approaches and fixes we have pending in this process termination space at the moment, as there are several interrelated fixes that either conflict or depend on each other - specifically #555, #729, #765, and #850. I've added your change to #1044 as a draft with you as a co-author + added extensive regression testing. Would you be OK with consolidating this change into #1044 for the comprehensive testing & process handling introduced there? |
Sorry, something went wrong.
|
Thanks @felixweinberger for taking the time to coordinate all this! Consolidation sounds good. |
Sorry, something went wrong.
The MCP specification recommends closing stdin first to allow servers to exit gracefully before resorting to signals. This approach gives well-behaved servers the opportunity to detect stdin closure and perform clean shutdown without forceful termination. The shutdown sequence now follows a graceful escalation path: first closing stdin and waiting 2 seconds for voluntary exit, then sending SIGTERM if needed, and finally using SIGKILL as a last resort. This minimizes the risk of data loss or corruption while ensuring cleanup always completes. This unified approach works consistently across all platforms and improves compatibility with MCP servers that monitor stdin for lifecycle management. resolves #765 Co-authored-by: davenpi <davenport.ianc@gmail.com>
The MCP specification recommends closing stdin first to allow servers to exit gracefully before resorting to signals. This approach gives well-behaved servers the opportunity to detect stdin closure and perform clean shutdown without forceful termination. The shutdown sequence now follows a graceful escalation path: first closing stdin and waiting 2 seconds for voluntary exit, then sending SIGTERM if needed, and finally using SIGKILL as a last resort. This minimizes the risk of data loss or corruption while ensuring cleanup always completes. This unified approach works consistently across all platforms and improves compatibility with MCP servers that monitor stdin for lifecycle management. resolves #765 Co-authored-by: davenpi <davenport.ianc@gmail.com>
Sorry, something went wrong.
Align the stdio shutdown sequence with the MCP spec: close the server’s input, wait for exit, send SIGTERM if needed, and SIGKILL as a last resort.
Motivation and Context
The MCP spec recommends a careful shutdown sequence for stdio transport to avoid leaving orphaned or stuck server processes. This change follows those steps, making shutdowns more reliable and predictable.
How Has This Been Tested?
All tests pass (except one unrelated skipped test).
Breaking Changes
None
Types of changes
Checklist
Additional context
None