feat: add additional process kill for sigterm and sigkill #604
Conversation
WalkthroughAdds a configurable graceful-termination mechanism to stdio transport. Introduces terminateDuration with a default of 5 seconds, option to override, and a multi-stage Close(): close stdin/stderr, wait up to duration, send SIGTERM, wait again, then SIGKILL if needed. Constructors updated; imports for time/syscall added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/stdio.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/transport/stdio.go (2)
client/transport/interface.go (2)
JSONRPCResponse(67-72)RequestHandler(38-38)util/logger.go (2)
Logger(8-11)DefaultLogger(16-20)
| if err, ok := wait(); ok { | ||
| return err | ||
| } | ||
|
|
||
| // Note the condition here: if sending SIGTERM fails, don't wait and just | ||
| // move on to SIGKILL. | ||
| if err := c.cmd.Process.Signal(syscall.SIGTERM); err == nil { | ||
| if err, ok := wait(); ok { | ||
| return err | ||
| } | ||
| } | ||
| // Sending SIGKILL if the server does not exit within a reasonable time after SIGTERM | ||
| if err := c.cmd.Process.Kill(); err != nil { | ||
| return err | ||
| } | ||
| if err, ok := wait(); ok { | ||
| return err | ||
| } | ||
| return fmt.Errorf("unresponsive subprocess") |
There was a problem hiding this comment.
Avoid returning an error when the child already exited.
wait() can fall through when cmd.Wait() finishes just as the timeout fires. In that case resChan already holds the exit result, but we still hit the fallback path. Because the process is already gone, Process.Signal/Process.Kill return os.ErrProcessDone, which bubbles up and causes Close() to report an error even though the subprocess shut down cleanly. Please drain resChan (or treat os.ErrProcessDone as success) before signalling, so we don’t misreport a failure when the child already exited.
🤖 Prompt for AI Agents
In client/transport/stdio.go around lines 247 to 265, Close() can return an
error when the child already exited because we signal/kill without first
draining the wait result; Process.Signal/Process.Kill then return
os.ErrProcessDone and bubbles up. Fix by first draining the wait result channel
(or calling wait() and treating os.ErrProcessDone as success) before attempting
to Signal/Kill; if wait() indicates the process already exited, return that
result (or nil if successful) and do not call Signal/Kill, otherwise proceed
with SIGTERM/SIGKILL as now.
Description
This adds a fallback to help clean up a process that may not be closing. Currently, processes can be held open, causing the close function to not finish.
I believe this fixes #131 and #498.
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This is largely pulled from the official MCP-Go SDK https://github.com/modelcontextprotocol/go-sdk/blob/ab092510d20a23bed672a27164957e0ff1104bc7/mcp/cmd.go#L100
Summary by CodeRabbit
New Features
Bug Fixes