-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(cli): improve subprocess cleanup on interruption in Goose CLI #3638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
hey, thanks for jumping on this. our thinking was to solve this along these lines: this introduces a cancellation token and should take care of the freezing. it does not kill the process that was started though, but it does feel like the right way forward. |
Thanks for the context! I will refine this PR. |
DOsinga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right idea, but I don't think this is where it should be implemented;
I think we should add code to dispatch_toolcall and forward the cancelation token to extension manager - how we go from there seems tricky
@DOsinga Hi, do you mind elaborating on this? Sorry that I don't have the full context of the stuff you guys are working on. |
|
this is the way: #3782 it forwards the cancelation token to the MCP servers. once we have that merged, we can modify the developer tools to respect that |
@DOsinga I found that cancellation token forwarding to MCP servers is already implemented. My PR (#3638) adds PID-based process management for spawned processes. Should I:
Which approach would you prefer? |
|
thanks @GaryZhous we did merge some changes which look like need some cleaning up as conflicts, I think still worth looking at (changes were to do with how some streams were handled, and simplfying io a bit) |
@michaelneale any suggestions for this PR to move forwards? Thanks in advance! Also, my PR effectively addresses issues like this #3983 |
|
I think this would help it not freeze up when spawning, but won't stop goose itself from freezing if there is a spawned long running process? do I misread it? |
@michaelneale not really, previously, if we let goose run a long-running process like |
97365de to
68102ff
Compare
|
Still in progress? |
|
I can't find back where we discussed this, but I don't think file based tracking of PIDs can work. this would just lead to us killing all processes goose is currently running. The way to do this is to have the extensions react to the cancelation token |
When users create subprocesses through Goose (e.g.,
python3 server.pyornpm run dev) and then interrupt them withCtrl + C(orCmd + Con macOS), the Goose session would "freeze." This occurred because the subprocess continued running in the background, blocking the continuation of the chat session. Users had to manually find and kill these processes using commands like lsof and kill to resume the chat. This pull request introduces enhancements to process management and cleanup in the Goose CLI and MCP modules, particularly when handling shell commands and subprocesses. It ensures better resource cleanup to prevent lingering processes and potential system "freezing." The key changes include adding a process cleanup function, and integrating it with the already-existing token cancellation method, improving shell command execution, and making platform-specific adjustments for better compatibility.Process tracking and cleanup improvements:
FilePidTrackerutility (crates/goose-mcp/src/file_pid_tracker.rs) to persistently track shell subprocess PIDs, enabling reliable cleanup of orphaned processes and registration/unregistration of process information.DeveloperRouter::bash, storing PIDs on process start and removing them on completion or cancellation. Also, shell commands are now wrapped withsetsid bash -con Unix for better isolation.cleanup_shell_processes) in the CLI session module to terminate any leftover shell processes when a shell tool is interrupted. [1] [2]Cancellation support:
call_tool_with_cancellation) in the router, and propagation of cancellation tokens from the MCP server to tool execution. This ensures that cancelled requests terminate subprocesses and unregister PIDs. [1] [2] [3] [4]Dependency and codebase updates:
uuid,tokio-util) to relevant crates. [1] [2]These changes collectively make shell command execution safer and more manageable, especially in cases of interruption or cancellation, and lay the groundwork for robust process lifecycle management across the application.
Before
After