Repository navigation
fix(transport): send SIGTERM before killing a child process on graceful shutdown - #1379
fayerman-source wants to merge 2 commits 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 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. The gap was already there during the first wait, but the second wait made it much easier to hit. graceful_shutdown now keeps the child in ChildWithCleanup through every await and clears it only after the child has exited or been killed. If the future is cancelled partway, dropping the transport still kills the child. That also covers a transport.close() error, which used to drop the child without killing it.
Added test_tokio_child_process_cancelled_graceful_shutdown_still_cleans_up: the child ignores both stdin EOF and SIGTERM, graceful_shutdown is cancelled by a timeout during the second wait, and the test checks that the process is gone after the transport is dropped. It fails on the previous commit (the child survives).
…utdown completes graceful_shutdown took the child out of ChildWithCleanup before its waits, so a caller that cancelled the shutdown future (for example with a timeout during the second wait) dropped the child without killing it. The child now stays in the guard and is cleared only after it has exited or been killed; a cancelled shutdown leaves it to the guard's drop kill. Adds a Unix test that cancels the shutdown during the second wait and checks the child is gone after the transport is dropped.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.The child now stays in its
ChildWithCleanupguard until it has exited or been killed, so cancelling the shutdown future partway (for example under a caller'stimeout) still leaves the child to be killed when the transport is dropped. Previously it was taken out of the guard before the first wait.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_cancelled_graceful_shutdown_still_cleans_up(Unix): the child ignores both stdin EOF andSIGTERM, the shutdown is cancelled by a timeout during the second wait, and the test checks the process is gone after the transport is dropped. It fails without the guard change (the child survives).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: 4 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