fix(core): use close event instead of exit in child_process fallback#25695
Conversation
…oogle-gemini#24923) Switch ShellExecutionService to listen for the child process 'close' event instead of 'exit' so exit handling occurs after stdio streams are closed. Update unit tests to emit the 'close' event in the mocked child processes to match the new behavior. Modified files: packages/core/src/services/shellExecutionService.ts and packages/core/src/services/shellExecutionService.test.ts.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where stdout could be lost during the execution of fast commands. By switching the event listener from 'exit' to 'close', the service now correctly waits for the process streams to be fully closed before finalizing the execution result. The changes also include necessary updates to the test suite to ensure that mock child processes simulate the standard Node.js lifecycle events correctly. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the ShellExecutionService to listen for the close event instead of the exit event when executing child processes, ensuring that all stdio streams are fully drained before resolving. Corresponding updates were made to the test suite to emit the close event in mock child processes. However, switching to close may cause the CLI to hang if a command spawns a long-lived background process that keeps the stdout/stderr pipes open, which could be a significant regression for a terminal-first tool.
| child.on('close', (code, signal) => { | ||
| handleExit(code, signal); | ||
| }); |
There was a problem hiding this comment.
Switching from the exit event to the close event is a common fix for ensuring all buffered stdout/stderr data is captured, especially for fast-running commands. However, this change introduces a potential regression: the close event waits for all stdio streams to be closed. If the child process spawns a long-lived background process (e.g., command &) that inherits the stdout/stderr pipes and does not close them, the close event will not fire until that background process terminates. This can cause the CLI to hang indefinitely or appear unresponsive for commands that have technically finished their primary execution.
Since this is a terminal-first tool, this hang could be a significant UX issue. Consider if a hybrid approach (e.g., listening to exit but waiting for a short timeout or for the end event on individual streams) might be more robust, or at least document this as a known limitation for backgrounded processes in the fallback path.
There was a problem hiding this comment.
@gemini-code-assist The hanging scenario shouldn't be an issue here. the shell tool already has an inactivity timeout that kills the entire process group (including grandchildren) if nothing is produced. Background commands also return early via is_background without blocking.
close is the standard way to handle this per Node.js docs: https://nodejs.org/docs/latest-v20.x/api/child_process.html#event-close
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the ShellExecutionService to listen for the 'close' event instead of the 'exit' event when a child process terminates. This change ensures that all stdio streams are fully closed before the exit handler is executed, providing more reliable output handling. Corresponding updates were made to the test suite to simulate the 'close' event in various execution scenarios. I have no feedback to provide.
…oogle-gemini#25695) Co-authored-by: Tommaso Sciortino <sciortino@gmail.com>
…oogle-gemini#25695) Co-authored-by: Tommaso Sciortino <sciortino@gmail.com>
Summary
Switch
child.on('exit')tochild.on('close')inchildProcessFallback()to fix stdout being lost on fast commands.Details
child.on('exit')→child.on('close')inchildProcessFallback()closeafterexit(matching real Node.js behavior)Related Issues
Fixes #24923
How to Validate
npm test -w @google/gemini-cli-core -- src/services/shellExecutionService.test.tsnpm run preflightPre-Merge Checklist