fix(sandbox): kill in-container process group on command timeout (#107)

When a sandboxed shell command timed out (or the turn was cancelled), only
the host-side docker exec client was killed - Docker does not propagate the
signal, so the in-container process kept running (CPU/memory, half-written
files in /workspace) until the container was torn down at session end.
Repeated timeouts accumulated runaway processes inside the container.

shellTool and parallelShellTool now run sandboxed commands under a
pid-marker wrapper (wrapSandboxCommand): the wrapper records the
container-side pid of its group-leading shell in a per-invocation pidfile
under /tmp, with the command passed as $1 (never interpolated, so quoting
cannot break out). docker exec processes are their own process-group
leaders (pgid == pid, verified empirically on alpine), so on timeout/cancel
odek follows up with docker exec ... kill -KILL -<pgid>, tearing down the
command and every child it forked. Children that call setsid/setpgid
escape the group - the follow-up is best-effort, not a hard guarantee.

The "container restart after N stale kills" escalation from the report is
deliberately not implemented: the group-kill follow-up addresses the leak
directly without mid-session container churn.

E2E tests (gated on ODEK_E2E + docker) prove no in-container survivors
after shell and parallel_shell timeouts while the container init stays
healthy; both fail against the pre-fix code. Unit tests updated for the
new buildCmd signature and argv shape.

Author: jkyberneees

AGENTS.md

@@ -174,6 +174,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **`~/.odek` trust-anchor protection** (`internal/danger/classifier.go`, `cmd/odek/file_tool.go`) — generic file tools reject writes to `~/.odek/config.json`, `secrets.env`, `IDENTITY.md`, `skills/`, `sessions/`, `audit/`, `plans/`, `schedules.json`, `schedule-state.json`, `mcp_approvals.json`, `mcp_tool_approvals.json`, `project_sandbox_approvals.json`, `restart.json`, `telegram.lock`, and related state files. These paths classify as `system_write` and must be modified through their dedicated subsystems. Matching is case-insensitive so variants such as `CONFIG.JSON` or `SECRETS.ENV` are also blocked on case-insensitive filesystems (e.g. macOS APFS). Shell reads of these trust anchors are also escalated to `system_write`.
 - **Nonce'd tool-result delimiter** (`internal/loop/loop.go`) — the static `┌── TOOL RESULT: ... └── END TOOL RESULT: ...` delimiter is now unique per tool call via a random hex nonce embedded in both the opening and closing lines. A tool or MCP server can no longer forge the closing delimiter to break out of the data framing and inject instructions.
 - **`parallel_shell` context + process-group kill** (`cmd/odek/perf_tools.go`) — commands now run via `exec.CommandContext` bound to the agent context, in their own process group. Cancellation or timeout kills the whole group (negative PID), so `sh -c 'sleep 3600 &'` cannot leave orphaned children. Per-command timeouts are also capped at 30 minutes.
+- **In-container timeout kill** (`cmd/odek/shell.go::wrapSandboxCommand`) — killing the host-side `docker exec` client on timeout/cancel does not terminate the in-container process (Docker does not propagate the signal). Sandboxed `shell`/`parallel_shell` commands therefore run under a pid-marker wrapper that records the container-side group leader's pid in `/tmp/.odek-cmd-*.pid`; on cancellation odek follows up with `docker exec … kill -KILL -<pgid>`, so a timed-out command cannot keep burning CPU/memory or leave half-written files until the container is torn down. Children that call setsid/setpgid escape the group — best-effort, not a guarantee.
 - **`batch_patch` trusted-class propagation** (`cmd/odek/perf_tools.go`) — `batch_patch` now passes its cached `trustedClasses` to `CheckOperation`, matching `write_file` and `patch`. A trusted `local_write` class is honored across all patches in the batch instead of re-prompting per patch.
 - **Browser link URL wrapping** (`cmd/odek/browser_tool.go`) — interactive element text was already wrapped as untrusted, but link URLs in `clickableRef.URL` were returned raw. They are now wrapped too, while an unexported `rawURL` is kept for internal click resolution.
 - **Browser post-redirect URL attribution** (`cmd/odek/browser_tool.go`) — `browser_navigate` now uses `resp.Request.URL` (the final post-redirect URL) for the snapshot URL, the untrusted-content source, and relative-link click resolution, instead of attributing content to the original requested URL.

cmd/odek/perf_tools.go

@@ -498,8 +498,11 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry {
 	defer cancel()
 
 	var shCmd *exec.Cmd
+	var killInContainer func()
 	if t.containerName != "" {
-		shCmd = exec.CommandContext(ctx, "docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", cmd.Command)
+		argv, followUp := wrapSandboxCommand(t.containerName, cmd.Command)
+		shCmd = exec.CommandContext(ctx, "docker", argv...)
+		killInContainer = followUp
 	} else {
 		shCmd = exec.CommandContext(ctx, "sh", "-c", cmd.Command)
 	}
@@ -521,6 +524,12 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry {
 	shCmd.WaitDelay = 3 * time.Second
 
 	err := shCmd.Run()
+	// Killing the host-side `docker exec` client does not terminate the
+	// in-container process (Docker does not propagate the signal); kill its
+	// process group explicitly so a timed-out command cannot linger.
+	if ctx.Err() != nil && killInContainer != nil {
+		killInContainer()
+	}
 	entry.Stdout = strings.TrimSpace(stdout.String())
 	entry.Stderr = strings.TrimSpace(stderr.String())
 	entry.DurationMs = time.Since(start).Milliseconds()

cmd/odek/sandbox_e2e_test.go

@@ -218,3 +218,80 @@ func TestE2E_SandboxFileInjection_MultipleFiles(t *testing.T) {
 		}
 	}
 }
+
+// TestE2E_SandboxTimeoutKillsInContainerProcesses verifies finding 7's fix:
+// when a sandboxed shell command times out, the in-container process group
+// is killed — not just the host-side `docker exec` client. Before the fix,
+// the in-container processes kept running until the container was destroyed.
+func TestE2E_SandboxTimeoutKillsInContainerProcesses(t *testing.T) {
+	skipIfNoE2E(t)
+
+	workDir := t.TempDir()
+	containerName := fmt.Sprintf("odek-test-timeout-%d", time.Now().UnixNano())
+	args := sandbox.BuildRunArgs(sandboxConfig{
+		Image:   "alpine:latest",
+		Network: "none",
+	}, containerName, workDir, "alpine:latest")
+	createCmd := exec.Command("docker", args...)
+	createCmd.Stderr = os.Stderr
+	if err := createCmd.Run(); err != nil {
+		t.Fatalf("create container: %v", err)
+	}
+	defer exec.Command("docker", "rm", "-f", containerName).Run()
+	time.Sleep(500 * time.Millisecond)
+
+	st := &shellTool{containerName: containerName, timeout: 1 * time.Second}
+	// Fork a background child AND block in the foreground: both must die.
+	_, err := st.Call(`{"command": "sleep 300 & sleep 300"}`)
+	if err == nil || !strings.Contains(err.Error(), "timed out") {
+		t.Fatalf("expected timeout error, got: %v", err)
+	}
+
+	// The follow-up runs synchronously before Call returns. PID 1 is
+	// "sleep infinity" — assert no leftover "sleep 300" processes remain.
+	out, err := exec.Command("docker", "exec", containerName, "ps").CombinedOutput()
+	if err != nil {
+		t.Fatalf("docker exec ps: %v\n%s", err, out)
+	}
+	if strings.Contains(string(out), "sleep 300") {
+		t.Errorf("in-container processes survived the timeout:\n%s", out)
+	}
+	if !strings.Contains(string(out), "sleep infinity") {
+		t.Errorf("container init process missing — unexpected container state:\n%s", out)
+	}
+}
+
+// TestE2E_SandboxParallelTimeoutKillsInContainerProcesses is the
+// parallel_shell analogue: a per-command timeout must kill the in-container
+// process group, not just the host-side client.
+func TestE2E_SandboxParallelTimeoutKillsInContainerProcesses(t *testing.T) {
+	skipIfNoE2E(t)
+
+	workDir := t.TempDir()
+	containerName := fmt.Sprintf("odek-test-ptimeout-%d", time.Now().UnixNano())
+	args := sandbox.BuildRunArgs(sandboxConfig{
+		Image:   "alpine:latest",
+		Network: "none",
+	}, containerName, workDir, "alpine:latest")
+	createCmd := exec.Command("docker", args...)
+	createCmd.Stderr = os.Stderr
+	if err := createCmd.Run(); err != nil {
+		t.Fatalf("create container: %v", err)
+	}
+	defer exec.Command("docker", "rm", "-f", containerName).Run()
+	time.Sleep(500 * time.Millisecond)
+
+	pt := &parallelShellTool{containerName: containerName}
+	_, err := pt.Call(`{"commands": [{"command": "sleep 300 & sleep 300", "timeout": 1}]}`)
+	if err != nil {
+		t.Fatalf("parallel_shell Call: %v", err)
+	}
+
+	out, err := exec.Command("docker", "exec", containerName, "ps").CombinedOutput()
+	if err != nil {
+		t.Fatalf("docker exec ps: %v\n%s", err, out)
+	}
+	if strings.Contains(string(out), "sleep 300") {
+		t.Errorf("in-container processes survived the parallel_shell timeout:\n%s", out)
+	}
+}

cmd/odek/shell.go

@@ -5,9 +5,11 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 	"os/exec"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"syscall"
 	"time"
 
@@ -161,10 +163,10 @@ func (t *shellTool) Call(args string) (string, error) {
 
 	// Bound execution: cancel with the agent context (Ctrl-C / turn timeout)
 	// and a generous backstop timeout so a stuck command can never wedge the
-	// agent forever. Note: in sandbox mode this kills the host-side
-	// `docker exec` client, which unblocks the agent, but Docker does not
-	// propagate the signal to the in-container process — that lingers until the
-	// container is torn down at session end.
+	// agent forever. In sandbox mode this kills the host-side `docker exec`
+	// client, which unblocks the agent — but Docker does not propagate the
+	// signal to the in-container process, so buildCmd also returns a
+	// follow-up that kills the in-container process group explicitly.
 	base := t.toolCtx()
 	timeout := t.timeout
 	if timeout <= 0 {
@@ -173,7 +175,7 @@ func (t *shellTool) Call(args string) (string, error) {
 	ctx, cancel := context.WithTimeout(base, timeout)
 	defer cancel()
 
-	cmd := t.buildCmd(ctx, input.Command)
+	cmd, killInContainer := t.buildCmd(ctx, input.Command)
 	// Run the command in its own process group and, on cancel/timeout, kill the
 	// WHOLE group — not just the `sh` leader. `sh -c "<cmd>"` may fork children
 	// (e.g. `sleep`); killing only the leader leaves them alive holding the
@@ -198,6 +200,15 @@ func (t *shellTool) Call(args string) (string, error) {
 
 	err := cmd.Run()
 
+	// On timeout/cancel the host-side `docker exec` client was killed by the
+	// group kill above, but the in-container process survives it (Docker does
+	// not propagate the signal). Kill its process group explicitly so a
+	// timed-out command cannot keep burning CPU/memory or leave half-written
+	// files in /workspace until the container is torn down.
+	if ctx.Err() != nil && killInContainer != nil {
+		killInContainer()
+	}
+
 	// Surface cancellation/timeout as a clear, actionable error rather than an
 	// opaque "signal: killed".
 	if ctxErr := ctx.Err(); ctxErr != nil {
@@ -283,15 +294,52 @@ func (t *shellTool) promptUser(cmd, description string) error {
 // buildCmd constructs the exec.Cmd for the given shell command.
 //
 // When sandbox mode is active (containerName is non-empty), the command
-// is wrapped in "docker exec -w /workspace <container> sh -c <cmd>".
-// The -w /workspace flag ensures the command runs in the working directory
-// that was mounted into the container during setupSandbox().
+// is wrapped in "docker exec -w /workspace <container>" with a pid-marker
+// wrapper (see wrapSandboxCommand), and the returned follow-up kills the
+// in-container process group — call it when the command's context is
+// cancelled or times out. The -w /workspace flag ensures the command runs
+// in the working directory that was mounted into the container during
+// setupSandbox(). The follow-up is nil in host mode.
 //
 // When running on the host (default), the command executes via "sh -c"
 // in odek's current working directory.
-func (t *shellTool) buildCmd(ctx context.Context, command string) *exec.Cmd {
+func (t *shellTool) buildCmd(ctx context.Context, command string) (*exec.Cmd, func()) {
 	if t.containerName != "" {
-		return exec.CommandContext(ctx, "docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", command)
+		argv, followUp := wrapSandboxCommand(t.containerName, command)
+		return exec.CommandContext(ctx, "docker", argv...), followUp
+	}
+	return exec.CommandContext(ctx, "sh", "-c", command), nil
+}
+
+// sandboxCmdSeq numbers sandboxed command invocations so each gets a unique
+// pid-marker file inside the container.
+var sandboxCmdSeq atomic.Uint64
+
+// wrapSandboxCommand builds the "docker exec" argv that runs command inside
+// the container with a pid-marker wrapper, plus a follow-up function that
+// kills the in-container process group and removes the marker.
+//
+// Killing the host-side `docker exec` client on timeout/cancel does NOT
+// terminate the in-container process — Docker does not propagate the signal
+// — so without a follow-up, timed-out commands keep running (CPU/memory,
+// half-written files in /workspace) until the container is destroyed.
+//
+// The wrapper records the container-side pid of the shell in a
+// per-invocation pidfile. docker exec processes are their own process-group
+// leaders (pgid == pid; verified on alpine and debian images), so
+// signalling the negative pid tears down the command and every child it
+// forked. Children that call setsid/setpgid themselves escape the group —
+// this is best-effort, not a hard guarantee. The command string travels as
+// a positional argument ($1), never interpolated into the wrapper, so
+// quoting cannot break out of it.
+func wrapSandboxCommand(containerName, command string) (argv []string, followUp func()) {
+	pidFile := fmt.Sprintf("/tmp/.odek-cmd-%d-%d.pid", os.Getpid(), sandboxCmdSeq.Add(1))
+	wrapper := "echo $$ > " + pidFile + "; sh -c \"$1\"; rc=$?; rm -f " + pidFile + "; exit $rc"
+	argv = []string{"exec", "-w", "/workspace", containerName, "sh", "-c", wrapper, "odek-cmd", command}
+	followUp = func() {
+		// Best-effort: the container may already be gone (session cleanup).
+		_ = exec.Command("docker", "exec", containerName, "sh", "-c",
+			"kill -KILL -$(cat "+pidFile+") 2>/dev/null; rm -f "+pidFile).Run()
 	}
-	return exec.CommandContext(ctx, "sh", "-c", command)
+	return argv, followUp
 }

cmd/odek/shell_test.go

@@ -189,20 +189,39 @@ func TestShellTool_Call_StdoutAndStderr(t *testing.T) {
 
 func TestShellTool_BuildCmd_Local(t *testing.T) {
 	st := &shellTool{}
-	cmd := st.buildCmd(context.Background(), "echo test")
+	cmd, followUp := st.buildCmd(context.Background(), "echo test")
 	args := cmd.Args
 	if args[0] != "sh" || args[1] != "-c" || args[2] != "echo test" {
 		t.Errorf("local cmd args = %v, want [sh -c 'echo test']", args)
 	}
+	if followUp != nil {
+		t.Error("host mode must not return an in-container follow-up")
+	}
 }
 
 func TestShellTool_BuildCmd_Docker(t *testing.T) {
 	st := &shellTool{containerName: "odek-12345"}
-	cmd := st.buildCmd(context.Background(), "echo test")
+	cmd, followUp := st.buildCmd(context.Background(), "echo test")
 	args := cmd.Args
-	expected := []string{"docker", "exec", "-w", "/workspace", "odek-12345", "sh", "-c", "echo test"}
-	if !stringSlicesEqual(args, expected) {
-		t.Errorf("docker cmd args = %v, want %v", args, expected)
+	// Shape: docker exec -w /workspace <container> sh -c <wrapper> odek-cmd <command>
+	if len(args) != 10 {
+		t.Fatalf("docker cmd args = %v, want 10 elements", args)
+	}
+	if args[0] != "docker" || args[1] != "exec" || args[2] != "-w" || args[3] != "/workspace" ||
+		args[4] != "odek-12345" || args[5] != "sh" || args[6] != "-c" || args[8] != "odek-cmd" || args[9] != "echo test" {
+		t.Errorf("docker cmd args = %v, unexpected shape", args)
+	}
+	// The wrapper records the group-leader pid in a marker file and the
+	// command travels as $1, never interpolated.
+	wrapper := args[7]
+	if !strings.Contains(wrapper, "echo $$ > /tmp/.odek-cmd-") || !strings.Contains(wrapper, `sh -c "$1"`) {
+		t.Errorf("wrapper = %q, want pid-marker + $1 dispatch", wrapper)
+	}
+	if strings.Contains(wrapper, "echo test") {
+		t.Errorf("wrapper = %q, command must not be interpolated into it", wrapper)
+	}
+	if followUp == nil {
+		t.Error("sandbox mode must return an in-container kill follow-up")
 	}
 }
 
@@ -362,7 +381,7 @@ func TestShellTool_CheckApproval(t *testing.T) {
 
 func TestShellTool_BuildCmd_Default(t *testing.T) {
 	st := &shellTool{}
-	cmd := st.buildCmd(context.Background(), "echo hello")
+	cmd, _ := st.buildCmd(context.Background(), "echo hello")
 	if cmd.Args[0] != "sh" {
 		t.Errorf("expected sh, got %s", cmd.Args[0])
 	}