Repository navigation
fix(transport): send SIGTERM before killing a child process on graceful shutdown - #1379
fayerman-source wants to merge 1 commit into
Conversation
…ul shutdown The MCP stdio shutdown sequence is: close the server's stdin and wait, send SIGTERM if it doesn't exit, then SIGKILL if it still doesn't exit. `TokioChildProcess::graceful_shutdown` closed stdin and waited, then went straight to `kill()`, so a server that doesn't exit on EOF never got a catchable signal to clean up before SIGKILL. On Unix, send SIGTERM after the first wait and wait again before killing. Non-Unix platforms keep wait-then-kill. `libc` moves from a Linux-only to a Unix target dependency; it is already in the tree there through tokio and process-wrap. Refs modelcontextprotocol#347
github-actions
Bot
added
T-config
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@connor4312 is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32405fc515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[cfg(unix)] | ||
| match child.signal(libc::SIGTERM) { | ||
| Ok(()) => { | ||
| if wait_for_exit(&mut child).await?.is_some() { |
There was a problem hiding this comment.
When a caller bounds the public shutdown future with a timeout between the two waits (for example, timeout(Duration::from_secs(5), child.graceful_shutdown()) for a default child that ignores SIGTERM), this newly added await is cancelled after child has already been removed from ChildWithCleanup. Dropping the local Box<dyn ChildWrapper> does not run the transport's kill-on-drop task, and the now-empty TokioChildProcess cannot kill it later, so the subprocess is left running; before this change that timeout exceeded the single three-second wait and normally observed the forced kill. Keep the child in a cleanup guard across this await and disarm it only after wait/kill completes.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Refs #347
Motivation and Context
#347 asked for the full stdio shutdown sequence from the spec (Basic > Lifecycle > Shutdown > stdio): close the server's input stream, wait, send
SIGTERMif it doesn't exit, thenSIGKILLif it still doesn't exit.TokioChildProcess::graceful_shutdown(#364) does the first two steps, then callskill()(SIGKILL on Unix). A server that doesn't exit on EOF never gets a catchable signal, so cleanup in its signal handler or exit path is skipped.After the first wait times out,
graceful_shutdownnow sendsSIGTERMthroughChildWrapper::signalon Unix and waits up toMAX_WAIT_ON_DROP_SECSagain before killing. With a process-group wrapper the signal reaches the whole group. Non-Unix platforms keep wait-then-kill, since they have noSIGTERM. A failedsignalcall is logged and falls through tokill().libcmoves from a Linux-only to acfg(unix)target dependency forlibc::SIGTERM. It is already in the tree on every Unix target through tokio and process-wrap, so there is no new crate.Drop for ChildWithCleanupstill kills directly. It runs in a sync context and can't wait out grace periods, so it's left as is.How Has This Been Tested?
test_tokio_child_process_graceful_shutdown_sends_sigterm_before_kill(Unix): the child issh -cwith aTERMtrap that writes a marker file and exits, and a loop that ignores stdin EOF. The test asserts that the marker exists aftergraceful_shutdownand that shutdown finished before the second timeout. It fails onmainwith "child was killed without receiving SIGTERM first".test_tokio_child_process_graceful_shutdownandtest_tokio_child_process_dropstill pass.Validation:
cargo test -p rmcp --features client,transport-child-process --lib transport::child_process: 3 passed.cargo test --all-featuresandcargo test -p rmcp --lib --no-default-features: passed.cargo clippy --all-targets --all-features -- -D warnings,cargo clippy -p rmcp --lib --no-default-features --features transport-child-process -- -D warningsandcargo +nightly fmt --all -- --check: clean.Breaking Changes
None. The public API is unchanged. A child that ignores both stdin EOF and
SIGTERMnow takes up to2 × MAX_WAIT_ON_DROP_SECS(6 s) to be killed, up from 3 s.Types of changes