Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions internal/sandbox-sidecar/command/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,24 @@ 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
}

func setupProcessGroup(cmd *exec.Cmd) {
if cmd.SysProcAttr == nil {
cmd.SysProcAttr = &syscall.SysProcAttr{}
}
cmd.SysProcAttr.Setpgid = true
cmd.Cancel = func() error {
err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
if errors.Is(err, syscall.ESRCH) {
return os.ErrProcessDone
}
return err
}
}

// --- Input/Output types ---

type CreateCommandInput struct {
Expand Down
78 changes: 78 additions & 0 deletions internal/sandbox-sidecar/command/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ import (
"os"
"path/filepath"
"sort"
"strconv"
"strings"
"syscall"
"time"

"github.com/danielgtaylor/huma/v2"
Expand Down Expand Up @@ -59,6 +61,10 @@ func extractSSEData(body string) string {
return result.String()
}

func processAlive(pid int) bool {
return syscall.Kill(pid, 0) == nil
Comment on lines +64 to +65

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat killed zombies as exited in process-tree test

In environments where PID 1 does not promptly reap orphaned children, the child killed by the process-group SIGKILL remains as a zombie reparented to PID 1; kill(pid, 0) still returns nil for zombies. That makes the new Eventually(... processAlive(childPID) ...).Should(BeFalse()) assertions time out even though the child is no longer running, so these tests fail/flap in containerized CI. Consider checking /proc/<pid>/status and treating State: Z as exited, or arranging for the child to be reaped.

Useful? React with 👍 / 👎.

}

var _ = Describe("Command Handlers", func() {
var (
commandAPI humatest.TestAPI
Expand Down Expand Up @@ -913,6 +919,78 @@ var _ = Describe("Command Handlers", func() {
})
})

Describe("process group termination", func() {
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{
"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())

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"}`)
Expand Down
1 change: 1 addition & 0 deletions internal/sandbox-sidecar/command/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down