fix(agent): bash tool spawns no longer hang on an inherited stdin (PRI-3243) - #418
Merged
Merged
Conversation
…I-3243)
The host process runner spawned every child with stdin as an open pipe
that nothing wrote to or closed. A command that falls back to reading
stdin, such as `head -8 $(find ... | head -1)` when find matches nothing,
blocked until killed by hand. That is the 47-minute Dex wedge.
Add RuntimeProcessOptions.stdin ('ignore' | 'pipe'). The host runner now
defaults to 'ignore' (/dev/null, immediate EOF) and only opens a pipe when
asked. RuntimeStdioClientTransport, which talks to MCP servers over stdio,
asks for 'pipe'. The bash tool and background shell jobs pass 'ignore'
explicitly. The projected container runner does not honor the option yet;
the doc comment says so.
Abort and timeout handling is unchanged: the bash tool still kills its
direct child only.
Split out of #413; the process-group half of that PR moves to a
follow-up.
obra
pushed a commit
to ada-sen/lace
that referenced
this pull request
Sep 26, 2026
…e (PRI-3250) ## Problem obra#418 (PRI-3243) gave RuntimeProcessOptions a stdin?: 'ignore' | 'pipe' option, defaulted to 'ignore', so a foreground bash spawn doesn't hang on a command that unexpectedly reads stdin (e.g. head with no file args). bash.ts passes stdin: 'ignore' explicitly. But that only reaches the host runtime (HostProcessRunner.start() in host.ts). The container runtime's ProjectedContainerProcessRunner.start() (tools/runtime/projected-container.ts) is the RuntimeProcessRunner implementation used whenever a tool executes inside a container, and it never read opts.stdin at all: optionsFor() doesn't forward it, and start() always calls containerManager.execStream(), which always spawns its underlying docker/container/plane CLI with -i and a piped stdin (see docker-container.ts's, apple-container.ts's and plane-runtime.ts's execStream() implementations). Nothing ever wrote to or closed that pipe, so a container-run command reading stdin because it got zero file args would hang forever, same shape as PRI-3243, just one layer down and unaffected by that fix. This is the container-runtime half PRI-3250 calls out as left out of obra#418's scope. Some execStream consumers genuinely need the piped stdin (an in-container MCP server driven interactively via RuntimeStdioClientTransport-equivalent plumbing, or ContainerExecFileSystem/ContainerExecNetworkClient's own low-level exec-with-stdin paths, which already write + end the pipe correctly and are unaffected). This fix only changes the ProjectedContainerProcessRunner.start()/exec() path that backs the bash tool (and other RuntimeProcessRunner callers) in container mode. ## Fix - ProjectedContainerProcessRunner.start() now ends the execStream handle's stdin immediately (with an error listener to swallow EPIPE) whenever opts.stdin !== 'pipe' (the default), mirroring HostProcessRunner's 'ignore' semantics without needing to change the three container runtimes' own execStream() signatures. - The returned RuntimeProcessHandle.stdin is undefined in that case, so a caller can't be misled into writing to an already-ended stream; it's only the live handle when the caller opted into stdin: 'pipe'. ## Tests Added to packages/agent/src/tools/runtime/__tests__/projected-container.test.ts: - 'ends the container stdin pipe immediately when the caller does not opt into it' — asserts end() is called once, the stream is writableEnded, and the returned handle.stdin is undefined. Fails against the pre-fix code (end() never called). - 'does not end the container stdin pipe when the caller opts into stdin: pipe' — asserts the opposite for an MCP-server-shaped stdin: 'pipe' caller. Gates: full packages/agent test suite (3656 passed, 35 skipped, 3 pre-existing failures unrelated to this change — confirmed by running them in isolation on this same worktree before this commit: resource-resolver.test.ts fails because this fresh worktree has no built dist/, and track-compaction.fixedpoint.test.ts's one failure was a parallel-run timing flake that passes standalone), npm run lint, npm run typecheck — all clean.
This was referenced Sep 26, 2026
obra
added a commit
that referenced
this pull request
Sep 26, 2026
…e (PRI-3250) (#415) * fix(agent): honor RuntimeProcessOptions.stdin in the container runtime (PRI-3250) ## Problem #418 (PRI-3243) gave RuntimeProcessOptions a stdin?: 'ignore' | 'pipe' option, defaulted to 'ignore', so a foreground bash spawn doesn't hang on a command that unexpectedly reads stdin (e.g. head with no file args). bash.ts passes stdin: 'ignore' explicitly. But that only reaches the host runtime (HostProcessRunner.start() in host.ts). The container runtime's ProjectedContainerProcessRunner.start() (tools/runtime/projected-container.ts) is the RuntimeProcessRunner implementation used whenever a tool executes inside a container, and it never read opts.stdin at all: optionsFor() doesn't forward it, and start() always calls containerManager.execStream(), which always spawns its underlying docker/container/plane CLI with -i and a piped stdin (see docker-container.ts's, apple-container.ts's and plane-runtime.ts's execStream() implementations). Nothing ever wrote to or closed that pipe, so a container-run command reading stdin because it got zero file args would hang forever, same shape as PRI-3243, just one layer down and unaffected by that fix. This is the container-runtime half PRI-3250 calls out as left out of #418's scope. Some execStream consumers genuinely need the piped stdin (an in-container MCP server driven interactively via RuntimeStdioClientTransport-equivalent plumbing, or ContainerExecFileSystem/ContainerExecNetworkClient's own low-level exec-with-stdin paths, which already write + end the pipe correctly and are unaffected). This fix only changes the ProjectedContainerProcessRunner.start()/exec() path that backs the bash tool (and other RuntimeProcessRunner callers) in container mode. ## Fix - ProjectedContainerProcessRunner.start() now ends the execStream handle's stdin immediately (with an error listener to swallow EPIPE) whenever opts.stdin !== 'pipe' (the default), mirroring HostProcessRunner's 'ignore' semantics without needing to change the three container runtimes' own execStream() signatures. - The returned RuntimeProcessHandle.stdin is undefined in that case, so a caller can't be misled into writing to an already-ended stream; it's only the live handle when the caller opted into stdin: 'pipe'. ## Tests Added to packages/agent/src/tools/runtime/__tests__/projected-container.test.ts: - 'ends the container stdin pipe immediately when the caller does not opt into it' — asserts end() is called once, the stream is writableEnded, and the returned handle.stdin is undefined. Fails against the pre-fix code (end() never called). - 'does not end the container stdin pipe when the caller opts into stdin: pipe' — asserts the opposite for an MCP-server-shaped stdin: 'pipe' caller. Gates: full packages/agent test suite (3656 passed, 35 skipped, 3 pre-existing failures unrelated to this change — confirmed by running them in isolation on this same worktree before this commit: resource-resolver.test.ts fails because this fresh worktree has no built dist/, and track-compaction.fixedpoint.test.ts's one failure was a parallel-run timing flake that passes standalone), npm run lint, npm run typecheck — all clean. * fix(agent): pass stdin:'pipe' to every container stdin writer; fail loudly, not silently (PRI-3250) jc's review of #415 (on d449f51) found the fix itself broke container mode: RuntimeProcessRunner.start() now ends stdin by default (correctly, per this PR), but the only two production callers that write to stdin never opted into stdin:'pipe'. Audited every caller of RuntimeProcessRunner start()/exec() in container mode: - bash.ts (foreground bash tool) — passes stdin:'ignore' explicitly (PRI-3243); doesn't write to stdin. Fine. - shell-job.ts (background bash jobs) — passes stdin:'ignore' explicitly; doesn't write to stdin. Fine. - runtime-stdio-transport.ts (in-container MCP server) — passes stdin:'pipe' explicitly and genuinely drives it. Fine, unaffected. - container-exec-fs.ts's writeTextFile — BROKEN. No opts passed at all, so the runner defaulted to 'ignore' and writeTextFile always threw "write stream unavailable". Broke file_write/file_edit for every container-mode runtime. - container-exec-network.ts's fetch — BROKEN for requests with a body. No stdin option passed; `if (handle.stdin)` silently skipped the body write when handle.stdin was undefined, so curl sent an empty POST with no error raised. - container-exec-fs.ts's stat/readTextFile/mkdir/readdir and file_find.ts/ripgrep_search.ts — all use process.exec(), never write to stdin. Unaffected either way. Fix: - writeTextFile now passes { stdin: 'pipe' }. Kept (already correct) the existing throw when handle.stdin is still missing. - fetch now passes stdin: 'pipe' only when there's a body to write (nothing needed for GET/no-body); added a throw when hasBody is true but handle.stdin is missing, matching writeTextFile's fail-loud pattern instead of silently sending an empty body. - Corrected the PR body/commit-message claim that these two call sites have their own stdin paths "unaffected by this change" — both go through the exact ProjectedContainerProcessRunner.start() this PR changes. Tests: - The unit-level fakes in container-exec-fs.test.ts and container-exec-network.test.ts previously ignored `opts` entirely, so they could not observe a missing stdin:'pipe' — exactly how this regression passed review. Updated both to honor opts.stdin (live stdin only for 'pipe'), matching the real runner's behavior. - Added composed tests to projected-container.test.ts that wire the REAL ProjectedContainerProcessRunner to the REAL ContainerExecFileSystem / ContainerExecNetworkClient (the actual production integration seam, not a hand-rolled fake): writeTextFile succeeds and the real content arrives over the container's stdin pipe; a POST body arrives intact. Verified: both fail on d449f51 (writeTextFile throws "write stream unavailable"; the POST body test observes an empty write) and pass after this fix. - Strengthened the existing (jc: "vacuous, minor") "does not end the container stdin pipe when the caller opts into stdin: pipe" test: it previously only asserted `end()` wasn't called and `stdin` was reference-equal, both true of pre-fix code too since pre-fix always left the pipe live. Now also writes through the returned handle and confirms the same bytes arrive on the container-side pipe. Gates: packages/agent unit suite (366 files / 3663 tests passed, 35 skipped), npm run lint, npm run typecheck — all clean. * test(agent): cover the container stdin fail-loud branches; trim review narration (PRI-3250) - container-exec-fs.test.ts / container-exec-network.test.ts: the fakes can now withhold stdin even for a 'pipe' caller and count kill() calls. New tests assert writeTextFile and a body-bearing fetch kill the process and throw "write stream unavailable" instead of writing nothing, and that a body-less fetch does not need stdin at all. Both throw tests fail if the guard is reverted to the old silent skip. - projected-container.test.ts: the 'pipe' opt-in test's comment now says what it guards (the fix must not end or hide the pipe for 'pipe' callers) and that a runner ignoring opts.stdin would also pass it. The composed-runner describe block is renamed to say what it covers. - Comments in container-exec-fs.ts, container-exec-network.ts, projected-container.ts and the tests now give the reason for the code and drop review-history narration and commit SHAs. - types.ts: RuntimeProcessOptions.stdin is now documented as honored by every runtime, including the projected container runtime. --------- Co-authored-by: Jesse Vincent <jesse@fsck.com>
ada-sen
added a commit
to ada-sen/lace
that referenced
this pull request
Sep 26, 2026
…l window (PRI-3243) This PR now covers only the detached-process-group / kill-model rework. The stdin fix shipped separately as obra#418; obra#414 (foreground timeout) and obra#415 (container-runtime stdin) are already on main. This is rebuilt from scratch on top of that main, not rebased. jc's review of the previous version of this PR found the detached-group fix incomplete: a subagent kill only gives the subagent 500ms (job-control.ts's killJob/killAllRunningJobs, both called with waitMs: 500, forceKill: true) before SIGKILLing it outright. The prior attempt's shutdown()-time reap used a 2s SIGTERM->SIGKILL grace period — comfortably past that 500ms deadline, so a TERM-ignoring descendant of a detached foreground bash command would still be mid-poll when the parent's SIGKILL ended the subagent process outright, abandoning the detached group exactly as before. Fix, same shape as before but re-timed to fit the real deadline: - bash.ts spawns its foreground command detached (POSIX) again and kills the whole process group (kill(-pid)), not just the shell, on both abort and timeout. - process-group-registry.ts tracks every detached command's pid; main.ts's shutdown() calls killAllTrackedProcessGroups() as the very first thing it does (before workspace teardown, before anything else that can hang), SIGTERMing every tracked group, polling for it to empty out, and SIGKILLing any survivor. - The grace period for that escalation is now 150ms, not 2s -- far under the parent's 500ms deadline with room for signal-delivery and poll-loop latency, so the reap reliably finishes before the parent's SIGKILL can land on the subagent process itself. Why this design and not the others considered: - A pgid-file/handshake so the PARENT tracks and kills descendant groups itself would work too, but needs a new IPC surface between parent and child and still has to solve the same "how fast can this finish" problem -- it doesn't remove the timing constraint, just moves the deadline to a different process. Reaping from the subagent's own SIGTERM handler needs no protocol change: the subagent already gets the SIGTERM, and it already owns the process-group-registry that named these pids. - Not detaching at all would lose the ability to kill a pipeline (e.g. sleep | cat) from a plain abort inside the SAME process, which is the behavior obra#414's review already validated for this PR; dropping it would regress that. - A larger grace period is what jc's review actually found broken: it has to be short specifically because the parent's budget is short. Tests: - New: agent-process.shutdown-process-group.e2e.test.ts. Spawns a real lace-agent process, gives it a foreground bash command with BOTH a plain `&` background child and a foreground pipeline whose first stage traps (ignores) SIGTERM, then kills the agent process the same way job-control.ts's killJob does -- SIGTERM, wait only 500ms, SIGKILL if still running -- and asserts neither descendant survives. - Fails on plain main (no detached spawn at all: the bash-tool child is never signaled when the agent process exits, so it's simply reparented to init). - Fails on main + a detached-only spawn with no registry/shutdown reap (confirmed by temporarily reverting just the main.ts change): the detached group is abandoned to init the moment the agent process exits, since nothing ever signals a group outside the agent's own. - Passes with the full fix. - New: process-group-registry.test.ts (tracking/untracking, SIGKILL escalation for a TERM-ignoring group within the 150ms/500ms budget, and no-op on an empty or already-untracked registry). - bash.test.ts: added the process-group abort test (pipeline member killed on abort, not just the shell) and the no-unhandled-rejection-on-abort test. Gates: packages/agent full suite (`npm run test`) -- 374 files / 3851 tests passed, 35 skipped (unit phase) + 48 files / 276 tests passed, 20 skipped (E2E phase), 0 failed. `npm run lint`, `npm run typecheck` -- both clean. No orphan processes left on the host after the suite run.
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.
What
The host process runner spawned every child with stdin as an open pipe that nothing wrote to or closed. A command that falls back to reading stdin blocked until someone killed it. The Dex wedge was
head -8 $(find ... | head -1): find matched nothing,headgot no file argument, and it waited on stdin for 47 minutes.This PR:
RuntimeProcessOptions.stdin('ignore' | 'pipe').'ignore'(/dev/null, so reads get immediate EOF). It opens a pipe only when asked. The bounded host runtime delegates to it, so it gets the same behavior.RuntimeStdioClientTransport(MCP servers over stdio) ask for'pipe'.'ignore'explicitly.The projected container runner does not honor the option yet: container children still get an open stdin pipe. The doc comment on
RuntimeProcessOptions.stdinsays so. The container side is #415.Abort and timeout behavior is unchanged from main: the bash tool still kills its direct child only.
Why a separate PR
Split out of #413 per Jesse's decision. #413 did two things: this stdin fix, and a process-group kill model (detached foreground commands, a process-group registry, a shutdown reaper, group kill on abort). Review found the process-group half orphans a subagent's in-flight command tree and its background children when a parent group-kills the subagent. That half moves to a follow-up. #414 and #415 will be rebased onto this PR.
Tests
bash.test.ts: real-process tests running the reported repro (head -8 $(find <nonexistent> ...)) and a barecat. Both return promptly with exit 0. On main, both hang until the 10s test timeout (verified by reverting the implementation with the tests in place).host.test.ts: a started process gets no stdin handle by default and a stdin-readingcatexits 0. Withstdin: 'pipe'the handle is writable andcatechoes what was written. The default-stdin test fails on main.shell-job.test.tsandruntime-stdio-transport.test.ts: updated spawn-option expectations.npm run build,npm run typecheck,npm run lintandnpm testall pass locally.