From a880bd2c69c46ba64525cd5aae002b8501b8cad7 Mon Sep 17 00:00:00 2001 From: Ben Liderman <18233089+benldrmn@users.noreply.github.com> Date: Mon, 20 Jul 2026 02:40:23 +0300 Subject: [PATCH 1/2] kill command process group on cancel --- internal/sandbox-sidecar/command/command.go | 23 +++++ .../sandbox-sidecar/command/command_test.go | 92 +++++++++++++++++++ .../sandbox-sidecar/command/suite_test.go | 1 + 3 files changed, 116 insertions(+) diff --git a/internal/sandbox-sidecar/command/command.go b/internal/sandbox-sidecar/command/command.go index 01067cad..7c04e5a4 100644 --- a/internal/sandbox-sidecar/command/command.go +++ b/internal/sandbox-sidecar/command/command.go @@ -95,9 +95,32 @@ func (b *ChrootCommandBuilder) Build(ctx context.Context, pid int, req sidecarap Chroot: fmt.Sprintf("/proc/%d/root", pid), Credential: &syscall.Credential{Uid: 0, Gid: 0}, } + setupProcessGroup(cmd) return cmd, nil } +// setupProcessGroup makes cmd the leader of a new process group and overrides +// context cancellation to SIGKILL the whole group. exec "$@" replaces the shell +// with the user's main process, but that process can fork children that reparent +// (shared PID namespace) when only the leader is killed. Go's default cancel +// signals a single PID, so those children would keep running after the API +// reports the command gone. Signalling the negative pgid reaps the whole tree. +func setupProcessGroup(cmd *exec.Cmd) { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.Setpgid = true + cmd.Cancel = func() error { + // Negative PID targets the process group (pgid == leader PID). + err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + if errors.Is(err, syscall.ESRCH) { + // Group already gone; mirror os.Process.Kill on a finished process. + return os.ErrProcessDone + } + return err + } +} + // --- Input/Output types --- type CreateCommandInput struct { diff --git a/internal/sandbox-sidecar/command/command_test.go b/internal/sandbox-sidecar/command/command_test.go index 70e17aca..f1f25076 100644 --- a/internal/sandbox-sidecar/command/command_test.go +++ b/internal/sandbox-sidecar/command/command_test.go @@ -24,7 +24,9 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" + "syscall" "time" "github.com/danielgtaylor/huma/v2" @@ -59,6 +61,13 @@ func extractSSEData(body string) string { return result.String() } +// processAlive reports whether a process with the given PID currently exists. +// signal 0 performs error checking without delivering a signal: nil means the +// process exists, ESRCH means it does not. +func processAlive(pid int) bool { + return syscall.Kill(pid, 0) == nil +} + var _ = Describe("Command Handlers", func() { var ( commandAPI humatest.TestAPI @@ -913,6 +922,89 @@ var _ = Describe("Command Handlers", func() { }) }) + Describe("process group termination", func() { + // The user's main process forks a child (sleep) that outlives it. With the + // shell wrapper exec "$@", the main process is a process-group leader; the + // child inherits that group. Cancelling (kill or timeout) must SIGKILL the + // whole group. Without process-group signalling, only the leader dies and + // the child reparents (shared PID namespace) and keeps running. + // + // startWithChild launches such a command and returns its command ID plus the + // PID of the forked child once it has recorded itself. + startWithChild := func(extra map[string]any) (string, int) { + f, err := os.CreateTemp("", "sidecar-child-pid-*") + Expect(err).NotTo(HaveOccurred()) + pidFile := f.Name() + Expect(f.Close()).To(Succeed()) + DeferCleanup(func() { _ = os.Remove(pidFile) }) + + req := map[string]any{ + // Fork a long-lived child, record its PID, then block so the leader + // stays alive until it is killed. + "args": []string{"/bin/sh", "-c", `sleep 300 & echo $! > "$CHILD_PID_FILE"; wait`}, + "env": map[string]string{"CHILD_PID_FILE": pidFile}, + } + for k, v := range extra { + req[k] = v + } + body, err := json.Marshal(req) + Expect(err).NotTo(HaveOccurred()) + + resp := commandAPI.Post("/v1/commands", "Content-Type: application/json", strings.NewReader(string(body))) + Expect(resp.Code).To(Equal(http.StatusAccepted)) + var result sidecarapi.CreateCommandResponse + Expect(json.NewDecoder(resp.Body).Decode(&result)).To(Succeed()) + + var childPID int + Eventually(func() bool { + data, readErr := os.ReadFile(pidFile) //nolint:gosec // test-created temp file path + if readErr != nil { + return false + } + pid, convErr := strconv.Atoi(strings.TrimSpace(string(data))) + if convErr != nil { + return false + } + childPID = pid + return true + }, "3s", "20ms").Should(BeTrue()) + + // Best-effort cleanup in case the fix regresses and the child survives. + DeferCleanup(func() { _ = syscall.Kill(childPID, syscall.SIGKILL) }) + return result.ID, childPID + } + + waitExited := func(id string) { + Eventually(func() *int { + resp := commandAPI.Get(fmt.Sprintf("/v1/commands/%s/status", id)) + var status sidecarapi.CommandStatusResponse + Expect(json.NewDecoder(resp.Body).Decode(&status)).To(Succeed()) + return status.ExitCode + }, "5s").ShouldNot(BeNil()) + } + + It("kills the whole process tree on DELETE", func() { + id, childPID := startWithChild(nil) + Expect(processAlive(childPID)).To(BeTrue(), "child should be running before kill") + + resp := commandAPI.Delete(fmt.Sprintf("/v1/commands/%s", id)) + Expect(resp.Code).To(Equal(http.StatusNoContent)) + + waitExited(id) + Eventually(func() bool { return processAlive(childPID) }, "3s", "20ms"). + Should(BeFalse(), "forked child must not survive after the command is killed") + }) + + It("kills the whole process tree on timeout", func() { + id, childPID := startWithChild(map[string]any{"timeoutSeconds": 1}) + Expect(processAlive(childPID)).To(BeTrue(), "child should be running before timeout") + + waitExited(id) + Eventually(func() bool { return processAlive(childPID) }, "3s", "20ms"). + Should(BeFalse(), "forked child must not survive after the command times out") + }) + }) + Describe("working directory", func() { It("runs command in specified cwd", func() { code, result := postCommand(`{"args": ["/bin/sh", "-c", "pwd"], "cwd": "/tmp"}`) diff --git a/internal/sandbox-sidecar/command/suite_test.go b/internal/sandbox-sidecar/command/suite_test.go index 4ed61075..a6627ad6 100644 --- a/internal/sandbox-sidecar/command/suite_test.go +++ b/internal/sandbox-sidecar/command/suite_test.go @@ -45,6 +45,7 @@ func (b *DirectCommandBuilder) Build(ctx context.Context, _ int, req sidecarapi. cmd.Env = env cmd.Dir = req.Cwd cmd.WaitDelay = waitDelayGracePeriod + setupProcessGroup(cmd) return cmd, nil } From 054401d70dddcd5c6fa287244ce00971310b9438 Mon Sep 17 00:00:00 2001 From: Ben Liderman <18233089+benldrmn@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:15:58 +0300 Subject: [PATCH 2/2] remove redundant comment --- internal/sandbox-sidecar/command/command.go | 8 -------- internal/sandbox-sidecar/command/command_test.go | 14 -------------- 2 files changed, 22 deletions(-) diff --git a/internal/sandbox-sidecar/command/command.go b/internal/sandbox-sidecar/command/command.go index 7c04e5a4..e52487d8 100644 --- a/internal/sandbox-sidecar/command/command.go +++ b/internal/sandbox-sidecar/command/command.go @@ -99,22 +99,14 @@ func (b *ChrootCommandBuilder) Build(ctx context.Context, pid int, req sidecarap return cmd, nil } -// setupProcessGroup makes cmd the leader of a new process group and overrides -// context cancellation to SIGKILL the whole group. exec "$@" replaces the shell -// with the user's main process, but that process can fork children that reparent -// (shared PID namespace) when only the leader is killed. Go's default cancel -// signals a single PID, so those children would keep running after the API -// reports the command gone. Signalling the negative pgid reaps the whole tree. func setupProcessGroup(cmd *exec.Cmd) { if cmd.SysProcAttr == nil { cmd.SysProcAttr = &syscall.SysProcAttr{} } cmd.SysProcAttr.Setpgid = true cmd.Cancel = func() error { - // Negative PID targets the process group (pgid == leader PID). err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) if errors.Is(err, syscall.ESRCH) { - // Group already gone; mirror os.Process.Kill on a finished process. return os.ErrProcessDone } return err diff --git a/internal/sandbox-sidecar/command/command_test.go b/internal/sandbox-sidecar/command/command_test.go index f1f25076..872b3477 100644 --- a/internal/sandbox-sidecar/command/command_test.go +++ b/internal/sandbox-sidecar/command/command_test.go @@ -61,9 +61,6 @@ func extractSSEData(body string) string { return result.String() } -// processAlive reports whether a process with the given PID currently exists. -// signal 0 performs error checking without delivering a signal: nil means the -// process exists, ESRCH means it does not. func processAlive(pid int) bool { return syscall.Kill(pid, 0) == nil } @@ -923,14 +920,6 @@ var _ = Describe("Command Handlers", func() { }) Describe("process group termination", func() { - // The user's main process forks a child (sleep) that outlives it. With the - // shell wrapper exec "$@", the main process is a process-group leader; the - // child inherits that group. Cancelling (kill or timeout) must SIGKILL the - // whole group. Without process-group signalling, only the leader dies and - // the child reparents (shared PID namespace) and keeps running. - // - // startWithChild launches such a command and returns its command ID plus the - // PID of the forked child once it has recorded itself. startWithChild := func(extra map[string]any) (string, int) { f, err := os.CreateTemp("", "sidecar-child-pid-*") Expect(err).NotTo(HaveOccurred()) @@ -939,8 +928,6 @@ var _ = Describe("Command Handlers", func() { DeferCleanup(func() { _ = os.Remove(pidFile) }) req := map[string]any{ - // Fork a long-lived child, record its PID, then block so the leader - // stays alive until it is killed. "args": []string{"/bin/sh", "-c", `sleep 300 & echo $! > "$CHILD_PID_FILE"; wait`}, "env": map[string]string{"CHILD_PID_FILE": pidFile}, } @@ -969,7 +956,6 @@ var _ = Describe("Command Handlers", func() { return true }, "3s", "20ms").Should(BeTrue()) - // Best-effort cleanup in case the fix regresses and the child survives. DeferCleanup(func() { _ = syscall.Kill(childPID, syscall.SIGKILL) }) return result.ID, childPID }