fix(core): resolve spawned process exit before stdio close#30327
Draft
Hona wants to merge 2 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CrossSpawnSpawner implementation in packages/core to distinguish process lifetime (exit) from stdio lifetime (close), so callers can observe exitCode/isRunning without being blocked by stdio staying open (e.g., via a detached descendant inheriting the parent’s stdio).
Changes:
- Track process exit separately from stdio close by introducing distinct
exitedandcloseddeferreds in the spawner. - Resolve
exitCodeandisRunningfrom theexitevent rather thanclose, while keepingkillwaiting onclose. - Add a regression test covering the “detached descendant keeps inherited stdio open” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/cross-spawn-spawner.ts | Splits exit vs close tracking; updates exitCode/isRunning to use exit, and kill paths to await close. |
| packages/core/test/effect/cross-spawn-spawner.test.ts | Adds a regression test ensuring exitCode resolves promptly even when stdio remains open due to a detached descendant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
exitCodeandisRunningfrom the foreground processexiteventclosefor graceful process-group termination while avoiding post-escalation waits on shared stdioWhy
Node distinguishes process lifetime from stdio lifetime:
exitis emitted when the spawned process ends, whilecloseis emitted only after its stdio streams close. Descendants can inherit stdout/stderr and keep those handles open indefinitely after the foreground command exits.Waiting only for
closehangs shell calls. Returning immediately onexitdrops bytes already buffered or waiting behind slow persistence and metadata callbacks. Idle-window timers are also incorrect because a quiet inherited pipe can emit more descendant output later.This PR uses an explicit timer-free boundary: foreground
exitstops further inherited output capture, then already accepted chunks drain through an owned bounded queue before the shell result returns.Domain model
ProcessLifecycle.exited: foreground process endedProcessLifecycle.closed: shared stdio closedterminateProcessGroup: graceful close with the existing opt-in escalation policyreapFailedProcessGroup: best-effort cleanup after a failed foreground processShellCompletion: taggedExited,Aborted, andTimedOutoutcomesTests
bun test test/effect/cross-spawn-spawner.test.tsfrompackages/core: 29 passedbun test test/tool/shell.test.tsfrompackages/opencode: 94 passedbun typecheckfrompackages/corebun typecheckfrompackages/opencodecancel interrupts shell and resolves cleanly: passedbun turbo typecheckvia the push hookgit diff --checkNotes
This deliberately does not use a fixed post-exit sleep or idle timer. Output emitted by inherited background descendants after foreground exit is outside the shell result boundary.