fix(sandbox): kill in-container process group on command timeout#107
Merged
Conversation
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.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
odek | f1d44cc | Commit Preview URL Branch Preview URL |
Jul 26 2026, 09:43 AM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the reported finding (verified against current code and a live container before fixing):
Change
shellToolandparallelShellToolnow run sandboxed commands throughwrapSandboxCommand(cmd/odek/shell.go):docker exec -w /workspace <c> sh -c 'echo $$ > /tmp/.odek-cmd-<pid>-<seq>.pid; sh -c "$1"; rc=$?; rm -f <pidfile>; exit $rc' odek-cmd <command>. The command string travels as positional argument$1and is never interpolated into the wrapper, so quoting cannot break out of it.docker execprocesses are their own process-group leaders (pgid == pid) and that forked children share the group. On timeout or cancellation, odek follows up withdocker exec <c> sh -c 'kill -KILL -$(cat <pidfile>) 2>/dev/null; rm -f <pidfile>'— tearing down the command and its forked children (sleep 300 & sleep 300dies completely). Previously it lingered until session end.setsid/setpgidescape the group; the follow-up no-ops if the container is already gone; the pidfile self-cleans on normal exit and lives on a per-container tmpfs otherwise.Scope note: the mitigation's "container restart after N stale kills" escalation is deliberately not implemented — the group-kill follow-up addresses the leak directly without mid-session container churn.
Tests
ODEK_E2E=true+ Docker):TestE2E_SandboxTimeoutKillsInContainerProcessesandTestE2E_SandboxParallelTimeoutKillsInContainerProcessesassert nosleep 300processes remain after a 1s timeout while PID 1 (sleep infinity) stays healthy. Both fail against the pre-fix code (verified by stashing the fix).TestShellTool_BuildCmd_Dockerupdated for the new argv shape (wrapper +$1dispatch, command never interpolated) and the returned follow-up;buildCmdcallers updated for the new signature.go test ./... -count=1✅ (28 packages),go vet✅,golangci-lint→ 0 issues ✅Docs:
AGENTS.mdgains an "In-container timeout kill" hardening bullet; the stale "lingers until the container is torn down" comment inshell.gois corrected.