fix(flows): ignore EPIPE on shell action stdin - #529
Conversation
Attach an error listener on the flow shell child stdin pipe and skip writes after the stream has ended. A large stdin to a child that exits without reading used to raise uncaught EPIPE and kill the host. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 other
|
Codex review: needs maintainer review before merge. Reviewed August 31, 2026, 5:50 AM ET / 09:50 UTC. ClawSweeper reviewWhat this changesThe branch handles early child-stdin closure in Flow shell actions, adds a host-process regression test, and records the user-facing fix in the changelog. Merge readinessKeep open: the final head contains a focused fix for a real Flow shell-action host crash, resolves the prior review findings, and has maintainer-prepared landing evidence. Priority: P2 Review scores
Verification
How this fits togetherFlow shell actions launch local child processes from a workflow definition and pass optional stdin to them. The executor captures the child result and returns it to the FlowRunner as the step outcome. flowchart LR
A[Flow definition] --> B[Shell action]
B --> C[Shell executor]
C --> D[Child process]
C --> E[Child stdin pipe]
E --> F{Input closes early?}
F --> G[Child exit status]
G --> H[Flow step result]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the focused stdin-pipe handling and regression coverage while retaining child exit status as the shell action’s outcome authority. Do we have a high-confidence way to reproduce the issue? Yes — the supplied built-CLI before/after proof exercises a FlowRunner shell action with 1 MiB stdin and an early-exiting child, reproducing the current-main EPIPE crash and the fixed result. Is this the best way to solve the issue? Yes — attaching the listener before writes and preserving the established child-exit result path is the narrowest fix; the final branch also covers the host-level failure mode. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ea9a6c6859db. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
Maintainer triage: LAND recommended using the prepared branch triage/pr529-20260831, head The built CLI reproduces the actual failure on current main and succeeds with the prepared patch: The fixture runs a real flow shell action with 1 MiB stdin and a Node child that exits successfully without reading it. Separate source-blind CLI checks passed early exit 0, exact delivery of 1,441,792 Unicode bytes, empty input, and early exit 7. The latter returns a structured flow failure; none crashes with EPIPE. Validation: eight focused shell tests; clean Codex autoreview of the final branch; full |
Make child-exit ownership explicit and wait for stdio closure in the subprocess regression harness. Add the user-facing changelog credit. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
Keep the prepared stdin behavior and regression intact, integrate the approved tooling and heartbeat changes, and append the credited fix to Unreleased. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Applied the approved preparation to this contributor branch as new commits, retaining Sebastien’s original commit and co-author credit. Integrated current main without rewriting history; the only reconciliation conflict was the changelog, where the stdin fix is appended under Unreleased with thanks to @SebTardif. Final head: The repeated built-CLI proof runs a real shell action with 1 MiB stdin and a child that exits before reading it: CLI exit 0, flow completed, child exit 0, empty stderr, no EPIPE. Original before/after evidence. Validation: fresh reconciliation autoreview clean; |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Landed as 9b6e8ea after all exact-head CI jobs passed. The landing validation includes the repeated real EPIPE proof, full checks, documentation checks, and clean Codex review. The Unreleased changelog includes contributor thanks. Thanks @SebTardif for fixing this host-crash path! |
2 participants
What Problem This Solves
Fixes an issue where users running
defineFlow/shell()/acpx flowwithShellActionExecution.stdinwould lose the whole CLI when the child exited without reading that stdin. Node treats anerrorevent on the child stdin pipe as fatal when no listener is attached, so a large write (1 MiB) to a child that doessetImmediate(() => process.exit(0))raised uncaughtEPIPEand killed the host.Why This Change Was Made
runShellActionalready pipes stdin and settles the action from the childexitevent. It did not attach anerrorlistener onchild.stdin, and it calledwrite/endeven after the stream had already ended. This change ignores pipe-death codes on stdin (EPIPE,EIO,ECONNRESET,ERR_STREAM_DESTROYED) and skipswrite/endwhen the stream is no longer writable. The existingexithandler still records status.This is not the CLI
process.stdoutpath from closed #441. That helper can throw or exit. Child stdin must not. Open #507 covers terminal stdout/stderr listeners. Merged #501 timed out hung process-list helpers. Merged #498 covers finalize persist errors. None of those touch flow shell stdin.The missing listener dates to #179 (
697ee1f, 2026-03-26, "feat: add experimental acpx flows runtime and examples"), 156 days onmain.User Impact
A flow that feeds stdin into a short-lived shell child keeps the ACP host alive. The shell action still completes from the child's exit status. Operators no longer lose the CLI because leftover stdin raised
EPIPE.Evidence
terminal output from live
nodeagainst compiled publicdefineFlow/shell()/FlowRunner(dist/flows.js).Same script: 1 MiB
stdintoprocess.execPath -e "setImmediate(() => process.exit(0))".Before, on unpatched
src/flows/executors/shell.tscompiled todist, the host throws and exits 1:After, on the patched compiled
runShellActionpath, the same public flow stays in-process and exits 0:Real behavior proof
Behavior or issue addressed: Flow
shell()stdin write to a child that exits without reading used to raise uncaughtEPIPEand kill the host process.Real environment tested: macOS, Node v26.7.0, acpx compiled from this branch at
/tmp/oc-pr-acpx-F003(dist/flows.js).Exact steps or command run after this patch:
The script imports
defineFlow,shell, andFlowRunnerfrom compileddist/flows.js, then runs a one-node flow whoseexecreturnscommand: process.execPath,args: ["-e", "setImmediate(() => process.exit(0))"], andstdinof 1 MiB.Evidence after fix: terminal output from the patched compiled public API (shown above). The host prints
hostAlive: true,status: "completed",exitCode: 0,stdinBytes: 1048576, and the Node process exits 0 with noEPIPEon stderr.Observed result after fix: The same 1 MiB stdin write that previously emitted
Unhandled 'error' event/write EPIPEnow completes insideFlowRunner. The host PID stays alive through the flow result print.What was not tested: Windows cmd.exe spawn, and a child that reads only part of stdin before exiting.
Related: #179 (origin), #507 (terminal stdout/stderr, different file), #501, #498, #441. Same missing-listener class in openclaw/openclaw #102047 and #100410.