fix(core): prevent PTY resource leak by terminating active executions on process exit#24694
fix(core): prevent PTY resource leak by terminating active executions on process exit#24694swamig wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
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 a critical resource leak where spawned subprocesses, specifically PTY shells, become orphaned when the gemini-cli process is terminated unexpectedly. By implementing global signal listeners and a centralized cleanup mechanism, the application now ensures that all active executions are properly terminated, preventing the accumulation of zombie processes and terminal slot exhaustion. 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
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a cleanupAll method to the ExecutionLifecycleService and adds global process signal listeners to ensure subprocesses are terminated when the process exits. However, the review feedback correctly identifies that placing these listeners in the core package creates side effects on module import and prematurely terminates the process, which bypasses the CLI's existing graceful shutdown logic and could result in telemetry data loss. It is recommended to remove the listeners from the core package and instead register the cleanup method with the CLI's central cleanup utility.
| // --- Global Cleanup Hooks --- | ||
| // Ensures that if the gemini-cli process exits or is killed, | ||
| // any spawned subprocesses (like node-pty shells) are terminated. | ||
| let hasCleanedUp = false; | ||
| const handleProcessExit = () => { | ||
| if (hasCleanedUp) return; | ||
| hasCleanedUp = true; | ||
| ExecutionLifecycleService.cleanupAll(); | ||
| }; | ||
|
|
||
| process.on('exit', handleProcessExit); | ||
| process.on('SIGINT', () => { | ||
| handleProcessExit(); | ||
| process.exit(130); | ||
| }); | ||
| process.on('SIGTERM', () => { | ||
| handleProcessExit(); | ||
| process.exit(143); | ||
| }); |
There was a problem hiding this comment.
Adding global process signal listeners (SIGINT, SIGTERM) in a core library package is problematic for two reasons:
- Side Effects on Import: Libraries should generally avoid modifying global state or attaching process-level listeners upon module import. This makes the code harder to test and can lead to unexpected behavior in different environments.
- Preempting Graceful Shutdown: The
process.exit()calls in these handlers will terminate the process immediately aftercleanupAll()runs. This preempts the CLI's own graceful shutdown logic inpackages/cli/src/utils/cleanup.ts, which is responsible for critical asynchronous tasks like flushing telemetry, closing browser sessions, and disposing of configurations. If this core listener fires first, telemetry data will be lost.
These listeners should be removed from the core package. Instead, ExecutionLifecycleService.cleanupAll() should be registered with the CLI's existing cleanup utility (e.g., via registerSyncCleanup in packages/cli/src/utils/cleanup.ts) to ensure it runs as part of the established shutdown sequence.
gemini-cli
bot
added
the
status/need-issue
1 participant
Issue
When running
gemini-cli, any spawned subprocesses (likenode-ptyshells) managed byExecutionLifecycleServiceare left orphaned if the CLI process is forcefully exited (e.g., viaSIGINT/Ctrl+C). This creates background zombies that hoard terminal slots (PTYs) on macOS and Linux, eventually leading to a Denial of Service (e.g., "no more PTYs" or "503 open terminals").Fix
This patch adds a static
cleanupAll()method toExecutionLifecycleServiceand registers global process exit/signal hooks. This ensures that whenevergemini-cliexits, it explicitly destroys all remaining PTY instances by invoking thekill()lifecycle method for each active execution.