fix(core): resolve spawn completion on exit, not only close (Windows detached-child hang)#29831
fix(core): resolve spawn completion on exit, not only close (Windows detached-child hang)#29831Hona wants to merge 7 commits into
Conversation
The shell tool hangs on Windows when a command spawns a detached child that inherits stdio (e.g. a backgrounded web server or playwright-cli). The cross-spawn spawner resolved process completion only on the child's `close` event, which fires after *all* stdio streams close. A detached child keeps the inherited pipe's write end open, so `close` is delayed indefinitely even though the main process already exited -- and exitCode (and thus the shell tool) never resolves. Resolve on whichever of `exit`/`close` fires first. `exit` returns promptly once the main process is gone (fixing the hang); `close` is kept as the required backstop for failed spawns, where cross-spawn emits spawn->error->close but never exit, so exit-only would hang that path forever. Closes anomalyco#24731
There was a problem hiding this comment.
Pull request overview
Fixes a Windows hang in the shell tool when a command spawns a detached child that inherits stdio (e.g. playwright-cli). The CrossSpawnSpawner previously completed only on the child's "close" event, which can be delayed indefinitely if a detached child keeps the parent's stdio pipes open. The fix completes the exit Deferred on whichever of "exit" or "close" fires first, while preserving the "close" listener as a backstop for the ENOENT path where cross-spawn never emits "exit".
Changes:
- Resolve the spawn completion Deferred in the
"exit"handler in addition to the existing"close"handler, withend/exitguards so the value is consistent regardless of order. - Added an explanatory comment documenting why both events are needed.
- Added a regression test (using
fx.live) that spawns a detached daemon inheriting stdio and assertsexitCoderesolves within 5s, reaping the daemon afterward.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/cross-spawn-spawner.ts | Complete on first of exit/close; keep close as ENOENT backstop. |
| packages/core/test/effect/cross-spawn-spawner.test.ts | New regression test for detached child stdio inheritance (#24731); trailing blank lines added. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ed pipes Replace the fixed 100ms post-exit drain (which truncated output when a slow per-chunk consumer was still working through a backlog) with joining the output reader fiber. For a normal command this returns the instant the stdio streams reach EOF -- full output, no fixed delay, matching upstream's backpressure-synced behavior. A detached child can hold the pipe open forever with no further output, so an idle watcher that resets on every chunk bounds only that case; a command still producing output is never cut off.
|
Is it ready to merge? Will it be included in the next release? |
Summary
Fix shell commands hanging after their main process exits while a child still holds stdout/stderr open.
Fix
exitedseparately from stream/groupclosed.exitCode/isRunningresolve onexited; kill escalation (forceKillAfter) waits onclosedso resistant descendants are still SIGKILLed.Why no temp file / no fixed drain
closeonly fires once every holder of the stdio write-end closes; a detached grandchild that inherited the pipe keeps it open, soclosenever comes. We cannot control the grandchild's stdio, so completion must key off processexit. "Exited" does not mean "fully read" -- a small produced-but-unconsumed backlog can remain, which is why we join the reader. A fixed-duration drain truncates that backlog for slow consumers; the idle-reset watcher does not.Tests
exitCode(core spawner).Closes #24731
Closes #24784
Closes #20902
Closes #29822
Closes #25038
Closes #28697
Closes #25938
Refs #28654
Refs #22012