fix(codex): prevent uncaught exceptions from sandbox subprocess stream errors#102326
fix(codex): prevent uncaught exceptions from sandbox subprocess stream errors#102326zw-xysk wants to merge 1 commit into
Conversation
…m errors
Three Codex modules spawn child processes and register stdout/stderr
data listeners but omit error handlers. When a child process pipe emits
an error (EPIPE during process crash, destroyed stream during teardown),
the missing listener causes an uncaught exception that terminates the
entire Node.js process — taking down the Codex sandbox and all active
code execution.
Add stdout/stderr error handlers in all three locations:
- sandbox-exec-server/processes.ts: log via embeddedAgentLog.warn()
- sandbox-exec-server/http.ts: log via embeddedAgentLog.warn()
- node-cli-sessions.ts: ignore (matching src/process/exec.ts pattern,
process-level error handler already rejects the promise)
All three children already have child.on('error') for spawn-level failures
(ENOENT, etc.), which does NOT cover independent stream errors on the
stdout/stderr Readable streams.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close: this PR should not stay as a third landing candidate because two existing open, proof-positive PRs already cover the same Codex stream-error work, and this branch handles the Codex CLI resume path less safely than the existing focused fix. Canonical path: Close this combined branch and proceed with the two existing narrower, proof-positive PRs: one for sandbox process/HTTP stream errors and one for Codex CLI resume lifecycle errors. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this combined branch and proceed with the two existing narrower, proof-positive PRs: one for sandbox process/HTTP stream errors and one for Codex CLI resume lifecycle errors. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection and Node's EventEmitter contract: current main attaches stdout/stderr data handlers without stream error listeners, so an emitted stream error is unhandled. I did not run a failing current-main reproduction in this read-only review. Is this the best way to solve the issue? No. The sandbox listener part is already covered by #101777, and the resume path should use the stronger terminate, wait, and report shape in #102127 rather than swallowing stream errors. Security review: Security review cleared: Cleared: the diff adds child-process stream listeners/logging only and does not change dependencies, workflows, permissions, secrets, package metadata, or install scripts. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 6630138598b3. |
clawsweeper
Bot
added
proof: sufficient
|
🦞✅ Reason: structured ClawSweeper close marker: close-required (sha=6b31a94e08237a9223a266df7c935c72b9a29785) Closed:
|
1 participant
What Problem This Solves
Three Codex modules spawn child processes and register
stdout/stderrdatalisteners but omiterrorhandlers on the streams themselves. When a child process pipe emits an error — a real risk when a sandbox process crashes (EPIPE), is killed during timeouts, or its stream is destroyed during teardown — the missing listener causes an uncaught exception that terminates the entire Node.js process, taking down the Codex sandbox and all active code execution with it.The existing
child.on("error")handlers in all three files only catch spawn-level failures (ENOENT when the binary is missing, etc.). They do not cover stream errors onstdout/stderr, because in Node.js these are independentReadablestreams with their own event dispatch.Affected locations:
sandbox-exec-server/processes.tssandbox-exec-server/http.tsnode-cli-sessions.tsChanges
extensions/codex/src/app-server/sandbox-exec-server/processes.ts(+6):child.stdout.on("error", handler)logging viaembeddedAgentLog.warn()child.stderr.on("error", handler)logging viaembeddedAgentLog.warn()extensions/codex/src/app-server/sandbox-exec-server/http.ts(+6):params.child.stdout.on("error", handler)logging viaembeddedAgentLog.warn()params.child.stderr.on("error", handler)logging viaembeddedAgentLog.warn()extensions/codex/src/node-cli-sessions.ts(+4):child.stdout.on("error", () => {})— stream errors are benign; the process-level error handler already rejects the promisechild.stderr.on("error", () => {})— sameEvidence
Unit tests (29/29 passed, 3 test files):
Real behavior proof (platinum):
Proof method: spawned real Node.js child processes with piped stdout/stderr, forcibly destroyed each stream to simulate EPIPE, and verified both stdout and stderr error handlers fire without crashing the process.
Review