diff --git a/internal/wrapper/native.go b/internal/wrapper/native.go index 26aaf62..e9140ef 100644 --- a/internal/wrapper/native.go +++ b/internal/wrapper/native.go @@ -29,7 +29,13 @@ func execInherit(args []string) int { cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { + setNewProcessGroup(cmd) + if err := cmd.Start(); err != nil { + return 127 + } + stopSignals := forwardSignals(cmd.Process) + defer stopSignals() + if err := cmd.Wait(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { return exitErr.ExitCode() } diff --git a/internal/wrapper/run.go b/internal/wrapper/run.go index 33f40fa..8c2b75c 100644 --- a/internal/wrapper/run.go +++ b/internal/wrapper/run.go @@ -91,6 +91,7 @@ func Run(tool string, args []string) int { } cmd.Env = env cmd.Stdin = os.Stdin + setNewProcessGroup(cmd) exitCode = runRawStdout(cmd) default: relay := filepath.Join(paths.BaseUnix, "bin", toolRelayName) @@ -100,6 +101,7 @@ func Run(tool string, args []string) int { cmd := exec.Command(wineBin, append([]string{toolExePath}, rewritten...)...) cmd.Env = buildEnv(paths) cmd.Stdin = os.Stdin + setNewProcessGroup(cmd) exitCode = runFiltered(cmd, s.stdoutFilter, s.stderrFilter) } } @@ -137,6 +139,7 @@ func runViaToolRelay(wineBin, relayExe, exePath string, args []string, paths *wi cmdArgs := append([]string{relayExe, exePath}, args...) cmd := exec.Command(wineBin, cmdArgs...) cmd.Env = append(buildEnv(paths), "MSVCGOWINE_STDOUT="+stdoutFifo, "MSVCGOWINE_STDERR="+stderrFifo) + setNewProcessGroup(cmd) if devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0); err == nil { defer devNull.Close() cmd.Stdout = devNull @@ -147,6 +150,8 @@ func runViaToolRelay(wineBin, relayExe, exePath string, args []string, paths *wi fmt.Fprintln(os.Stderr, "vintner:", err) return 1 } + stopSignals := forwardSignals(cmd.Process) + defer stopSignals() var wg sync.WaitGroup wg.Add(2) @@ -204,6 +209,8 @@ func runRawStdout(cmd *exec.Cmd) int { fmt.Fprintln(os.Stderr, "vintner:", err) return 1 } + stopSignals := forwardSignals(cmd.Process) + defer stopSignals() doneOut := make(chan struct{}) doneErr := make(chan struct{}) @@ -278,6 +285,8 @@ func runFiltered(cmd *exec.Cmd, stdoutF, stderrF lineFilter) int { fmt.Fprintln(os.Stderr, "vintner:", err) return 1 } + stopSignals := forwardSignals(cmd.Process) + defer stopSignals() doneOut := make(chan struct{}) doneErr := make(chan struct{}) diff --git a/internal/wrapper/signals.go b/internal/wrapper/signals.go new file mode 100644 index 0000000..8f0890a --- /dev/null +++ b/internal/wrapper/signals.go @@ -0,0 +1,68 @@ +package wrapper + +import ( + "os" + "os/exec" + "os/signal" + "syscall" + "time" +) + +// killGrace bounds how long a forwarded SIGINT/SIGTERM gets to make a +// subprocess tree exit on its own before escalating to SIGKILL - long +// enough for wineserver to tear down a Windows process tree cleanly, short +// enough that an unresponsive one doesn't hang vintner's own shutdown. +const killGrace = 5 * time.Second + +// setNewProcessGroup puts cmd's eventual child in its own process group +// (pgid = its own pid) instead of inheriting vintner's. Without this, a +// caller that signals vintner by PID alone (a CI runner enforcing a +// timeout, a supervisor's `kill `) never reaches the wine/wineserver +// tree underneath it, which is then reparented to init and keeps running - +// wasting CPU, holding file locks, leaving stray FIFOs/temp files behind. +// (Interactive Ctrl-C already reaches every process in the terminal's +// foreground group regardless of this, but forwardSignals below handles +// that case too now that the child has moved to its own group.) +func setNewProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +// forwardSignals relays SIGINT/SIGTERM received by vintner itself to +// proc's entire process group (proc must have been started via a cmd that +// called setNewProcessGroup, making proc.Pid also the group id), escalating +// to SIGKILL after killGrace if the group hasn't exited by then. Callers +// must call the returned stop func once the process has actually exited +// (e.g. right after cmd.Wait() returns), both to stop listening for +// signals and to cancel a pending escalation. +func forwardSignals(proc *os.Process) (stop func()) { + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + done := make(chan struct{}) + + go func() { + pgid := -proc.Pid + for { + select { + case sig := <-sigCh: + s, ok := sig.(syscall.Signal) + if !ok { + continue + } + _ = syscall.Kill(pgid, s) + select { + case <-time.After(killGrace): + _ = syscall.Kill(pgid, syscall.SIGKILL) + case <-done: + return + } + case <-done: + return + } + } + }() + + return func() { + signal.Stop(sigCh) + close(done) + } +} diff --git a/internal/wrapper/signals_test.go b/internal/wrapper/signals_test.go new file mode 100644 index 0000000..147df08 --- /dev/null +++ b/internal/wrapper/signals_test.go @@ -0,0 +1,43 @@ +package wrapper + +import ( + "os" + "os/exec" + "syscall" + "testing" + "time" +) + +// TestForwardSignalsKillsChild verifies the actual mechanism that keeps a +// wine subprocess from being orphaned: a SIGTERM delivered to the current +// process (mimicking `kill `, not an interactive Ctrl-C) must +// reach a child started with setNewProcessGroup, even though it's no longer +// in the same process group. +func TestForwardSignalsKillsChild(t *testing.T) { + cmd := exec.Command("sleep", "30") + setNewProcessGroup(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("starting sleep: %v", err) + } + stop := forwardSignals(cmd.Process) + defer stop() + + // signal.Notify (inside forwardSignals) intercepts this rather than + // letting it terminate the test binary itself. + if err := syscall.Kill(os.Getpid(), syscall.SIGTERM); err != nil { + t.Fatalf("signaling self: %v", err) + } + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected the child to be killed by the forwarded signal, but it exited successfully") + } + case <-time.After(3 * time.Second): + cmd.Process.Kill() + t.Fatal("child was still running 3s after the signal should have been forwarded") + } +}