Add KillProcessGroup option to handle process groups#1313
Add KillProcessGroup option to handle process groups#1313mkaaad wants to merge 6 commits intomvdan:masterfrom
Conversation
…oup signals This builds upon the reapplication of "interp: send interrupt/kill signals to the whole process group" (mvdan#1244) (excluding cmd/gosh/main.go and cmd/gosh/main_test.go, which reverts commit e74afc1), adding a KillProcessGroup RunnerOption (defaulting to false) so users can opt in to process group behavior, restoring single-process signal delivery as the default.
- Add CREATE_NEW_PROCESS_GROUP flag in prepareCommand for console events - Use GenerateConsoleCtrlEvent in interruptCommand before falling back to TerminateJobObject - Fix process handle management with OpenProcess for proper job assignment - Clean up job handles automatically after process exit - Add missing postStartCommand implementations for non-Windows platforms
|
I admit I don't really feel like maintaining the windows code because I don't understand it nor can I really test it well. Does anyone actually need windows support for this? The original patch only did this for unix. |
|
Also cc @andreynering for thoughts on whether an option like this makes sense. My intuition is that an option here is unintuitive and should be avoided, but if we can't get reasonable behavior for all users without two modes, perhaps it's the best we can do. |
|
I'm actually trying to fix an issue I encountered with Crush: When using this agent to work on a Go project, I ran into a problem where processes couldn’t be terminated. I found that Crush executes commands using: But when a cancel signal is sent to the provided ctx , the process is not actually killed. I checked the issues and saw other users are facing the same problem: So I want to create a patch for users who run into this. Since most people hitting this issue are users of the interp package, |
|
Also, there’s another related issue in the Crush repo: The community previously tried to fix this bug by adding a timeout watcher |
- Introduce jobEntry struct with mutex to prevent double-close races between killCommand/interruptCommand and the cleanup goroutine - Fix procHandle lifetime: single OpenProcess with SYNCHRONIZE passed to cleanup goroutine, eliminating the second OpenProcess call - Add PROCESS_SUSPEND_RESUME to OpenProcess flags required by NtResumeProcess - Replace ResumeThread with NtResumeProcess to resume all threads at once - Fix CTRL_C_EVENT → CTRL_BREAK_EVENT (CREATE_NEW_PROCESS_GROUP processes ignore CTRL_C_EVENT) - Use LoadAndDelete in interruptCommand to prevent double-terminate races - Return error from postStartCommand on all platforms; caller in handler.go now propagates setup failures instead of silently discarding them Optimized with assistance from Claude (Crush).
|
The same problem when using in crush on Windows 10 |
|
This is an annoying issue, but unfortunately the trade-off of interactive commands not working anymore is a big deal IMO. I wrote some explanation for the revert here: #1244. Ideally, we should spend some time investigating a solution that works for all use uses, but I don't have time available for that now. |
|
Echoing what @andreynering said, I think further research needs to happen here because presumably, Bash implements an interactive shell that does the right thing without needing a global option like this. We should ideally do the same. |
|
How do you feel about combining the process group mode toggle with interp.Interactive(false)? @mvdan @andreynering |
Remove the separate KillProcessGroup option and instead enable process group killing automatically when Interactive is false (non-interactive mode).
4 participants
Reverted the previous change that unconditionally killed the entire process group (main.go and main_test.go were not reverted).
To prevent interactive commands from failing, I add an explicit switch KillProcessGroup to the Runner. It defaults to false to keep the original behavior. Set it to true when user want to ensure all child processes are terminated.
This maintains backward compatibility while solving the orphaned process problem.