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

@@ -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
 	}
 

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
+
+}

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)
+}

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)