fix(cli): kill the whole child tree when runCommandSilently times out - #2455
fix(cli): kill the whole child tree when runCommandSilently times out#2455fengmk2 wants to merge 1 commit into
Conversation
runCommandSilently resolves on the child's close event, but the timeout killed only the direct child. When arbitrary code run by the child (a migration config worker executing a project's vite config) spawned a grandchild that inherited the piped stdio, the SIGKILL left the grandchild holding the pipe, close never fired, and the caller hung forever. vp migrate showed this as a permanent 'Checking config compatibility' spinner after a CPU burst. Spawn the child in its own process group on POSIX and kill the group on timeout, then destroy our pipe ends so close always fires even when a survivor holds the far end (the Windows path and escaped-group case). The timeout path rejects without reading output, so nothing is lost. The new test wedges a child that hands its pipes to a 15s grandchild; before the fix the promise never settled.
✅ Deploy Preview for viteplus-preview canceled.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e92d970dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: options.envs, | ||
| // Own process group (POSIX) so the timeout can kill the whole tree: the | ||
| // child runs arbitrary project code that may spawn its own children. | ||
| detached: process.platform !== 'win32', |
There was a problem hiding this comment.
On POSIX this places every silent command in a new session, including the no-timeout install, format, and migration commands. Consequently, Ctrl-C signals the foreground process group containing vp but not the detached child; because this runner has no shutdown handler that forwards the signal, a package installation or build script can continue modifying the project after the user cancels the CLI. Limit detached to timeout-bearing invocations (or explicitly terminate the child during parent shutdown) so ordinary commands retain signal propagation.
Useful? React with 👍 / 👎.
1 participant
Bug
runCommandSilentlyresolves on the child'scloseevent, but its timeout killed only the direct child. When arbitrary code run by the child spawned a grandchild that inherited the piped stdio, the SIGKILL left the grandchild holding the pipe,closenever fired, and the caller hung forever.vp migratehits this through the config-compat worker: resolving a project'svite.configexecutes project code, and any lingering stdio-inheriting descendant (an esbuild-style service, a daemon) wedges the migration permanently under a frozen "Checking config compatibility" spinner, at 0% CPU after the initial burst. This likely explains previously observedvp migratehangs at 0% CPU.Fix
closealways fires even when a survivor holds the far end (the Windows path and escaped-group case). The timeout path rejects without reading output, so nothing is lost.Test
The new case wedges a child that hands its pipes to a short-lived grandchild. Before the fix the promise never settled and the test died on its own timeout; with the fix the suite passes in under a second. The compat-runner spec passes unchanged.