From 6622a6ded48ed34d7e85893a7c35b299cf710193 Mon Sep 17 00:00:00 2001
From: James Read <contact@jread.com>
Date: Thu, 4 Jul 2024 01:12:28 +0100
Subject: [PATCH] bugfix: Kill the process group, not just the parent process
 (#328), , thanks @ioqy, @vvrein (#346)

* bugfix: Kill the process group, not just the parent process (#328), thanks @ioqy, @vvrein

* bugfix: Kill the process group, not just the parent process (#328), thanks @ioqy, @vvrein

* bugfix: Kill the process group, not just the parent process (#328), thanks @ioqy, @vvrein
---
 internal/executor/executor.go         | 13 +++----------
 internal/executor/executor_unix.go    | 25 +++++++++++++++++++++++++
 internal/executor/executor_windows.go | 17 +++++++++++++++++
 internal/grpcapi/grpcApi.go           |  4 ++--
 4 files changed, 47 insertions(+), 12 deletions(-)
 create mode 100644 internal/executor/executor_unix.go
 create mode 100644 internal/executor/executor_windows.go

diff --git a/internal/executor/executor.go b/internal/executor/executor.go
index a671ce7..443ef06 100644
--- a/internal/executor/executor.go
+++ b/internal/executor/executor.go
@@ -15,9 +15,7 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"path"
-	"runtime"
 	"strings"
 	"sync"
 	"time"
@@ -355,14 +353,6 @@ func notifyListeners(req *ExecutionRequest) {
 	}
 }
 
-func wrapCommandInShell(ctx context.Context, finalParsedCommand string) *exec.Cmd {
-	if runtime.GOOS == "windows" {
-		return exec.CommandContext(ctx, "cmd", "/C", finalParsedCommand)
-	}
-
-	return exec.CommandContext(ctx, "sh", "-c", finalParsedCommand)
-}
-
 func appendErrorToStderr(err error, logEntry *InternalLogEntry) {
 	if err != nil {
 		logEntry.Output = err.Error() + "\n\n" + logEntry.Output
@@ -422,6 +412,9 @@ func stepExec(req *ExecutionRequest) bool {
 	appendErrorToStderr(waiterr, req.logEntry)
 
 	if ctx.Err() == context.DeadlineExceeded {
+		log.Warnf("Command timed out: %v", req.finalParsedCommand)
+		// The context timeout should kill the process, but let's make sure.
+		req.executor.Kill(req.logEntry)
 		req.logEntry.TimedOut = true
 	}
 
diff --git a/internal/executor/executor_unix.go b/internal/executor/executor_unix.go
new file mode 100644
index 0000000..54ef686
--- /dev/null
+++ b/internal/executor/executor_unix.go
@@ -0,0 +1,25 @@
+//go:build !windows
+// +build !windows
+
+package executor
+
+import (
+	"context"
+	"os/exec"
+	"syscall"
+)
+
+func (e *Executor) Kill(execReq *InternalLogEntry) error {
+	// A negative PID means to kill the whole process group. This is *nix specific behavior.
+	return syscall.Kill(-execReq.Process.Pid, syscall.SIGKILL)
+}
+
+func wrapCommandInShell(ctx context.Context, finalParsedCommand string) *exec.Cmd {
+	cmd := exec.CommandContext(ctx, "sh", "-c", finalParsedCommand)
+
+	// This is to ensure that the process group is killed when the parent process is killed.
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+
+	return cmd
+
+}
diff --git a/internal/executor/executor_windows.go b/internal/executor/executor_windows.go
new file mode 100644
index 0000000..6e27678
--- /dev/null
+++ b/internal/executor/executor_windows.go
@@ -0,0 +1,17 @@
+//go:build windows
+// +build windows
+
+package executor
+
+import (
+	"context"
+	"os/exec"
+)
+
+func (e *Executor) Kill(execReq *InternalLogEntry) error {
+	return execReq.Process.Kill()
+}
+
+func wrapCommandInShell(ctx context.Context, finalParsedCommand string) *exec.Cmd {
+	return exec.CommandContext(ctx, "cmd", "/C", finalParsedCommand)
+}
diff --git a/internal/grpcapi/grpcApi.go b/internal/grpcapi/grpcApi.go
index 307c2fd..94139ab 100644
--- a/internal/grpcapi/grpcApi.go
+++ b/internal/grpcapi/grpcApi.go
@@ -35,14 +35,14 @@ func (api *oliveTinAPI) KillAction(ctx ctx.Context, req *pb.KillActionRequest) (
 		ExecutionTrackingId: req.ExecutionTrackingId,
 	}
 
-	execReq, found := api.executor.Logs[req.ExecutionTrackingId]
+	execReqLogEntry, found := api.executor.Logs[req.ExecutionTrackingId]
 
 	ret.Found = found
 
 	if found {
 		log.Warnf("Killing execution request by tracking ID: %v", req.ExecutionTrackingId)
 
-		err := execReq.Process.Kill()
+		err := api.executor.Kill(execReqLogEntry)
 
 		if err != nil {
 			log.Warnf("Killing execution request err: %v", err)