fix(cli): forward termination signals to relaunched child process - #25605
fix(cli): forward termination signals to relaunched child process#25605Sway-Chan wants to merge 5 commits into
Conversation
|
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. |
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 child processes spawned during application relaunch were not correctly terminated when the parent process received termination signals. By installing signal forwarders, the parent now ensures that termination signals are propagated to the child, preventing it from becoming an orphan process. The implementation includes robust cleanup logic to ensure that signal listeners are properly removed, maintaining system stability and preventing resource leaks. 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 implements signal forwarding in relaunchAppInChildProcess to ensure child processes are terminated when the parent receives termination signals, along with necessary listener cleanup and unit tests. A review comment points out that forwarding SIGINT and SIGQUIT in interactive TTY sessions causes double-delivery of signals and changes the parent's termination behavior, potentially leading to terminal hangs; it suggests refining the logic and updating the documentation.
bcc87f7 to
016e442
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements signal forwarding from the parent process to the child process in the relaunchAppInChildProcess utility to prevent orphaned processes. It includes logic to clean up signal listeners when the child process terminates or encounters an error, along with new unit tests covering these scenarios. Feedback was provided regarding the potential for process leaks when SIGINT and SIGQUIT are conditionally bypassed in TTY mode, as well as a potential listener leak if an error occurs during IPC initialization before listeners are attached.
relaunchAppInChildProcess spawned a full child but did not install signal handlers on the parent. When a supervisor (ACP client, systemd, container runtime) signalled the parent PID, the child was reparented to PID 1 / the user's systemd manager and continued to hold the OAuth session and allocated heap. Install forwarders for SIGTERM/HUP/INT/QUIT/USR1/USR2 before awaiting the child, and remove them on close/error to avoid listener leaks across relaunch iterations. Closes google-gemini#25590
016e442 to
4804bf8
Compare
|
Thanks for the detailed re-review. I've adopted both suggestions in
Updated the unit tests to match (removed the TTY-skip test, added a listener-leak-on-IPC-throw test). |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements signal forwarding in the relaunchAppInChildProcess utility to ensure that termination signals sent to the parent process are correctly propagated to the child process, preventing orphaned processes. It introduces a mechanism to manage and clean up signal listeners on the parent process during the child's lifecycle, specifically handling error and close events. Additionally, new unit tests have been added to verify signal forwarding, listener cleanup, and resilience against synchronous IPC errors. I have no feedback to provide.
|
Gentle ping for maintainer review. Current status:
This PR fixes the orphaned relaunched child process when the parent receives programmatic termination signals. |
| // Should default to exit code 1 | ||
| expect(processExitSpy).toHaveBeenCalledWith(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Try using parameterized tests and consider refactoring the signal forwarding tests to use it.each. Add vi.restoreAllMocks() to the afterEach block to prevent side effects. Additionally I think there's a redundant signal list in the tests
|
Thank you for your interest in contributing to the project! We are closing this PR due to inactivity. |
Summary
relaunchAppInChildProcessspawns a full-memory child vianode:child_process.spawn, but the bootstrap parent did not install signal handlers to forward termination signals to the child. When the parent receivesSIGTERM/SIGHUPfrom a supervising process (e.g. an ACP client, systemd, a container runtime), the bootstrap exits but the child is reparented to PID 1 / the user'ssystemd --usermanager and keeps running, holding the OAuth session and allocated heap indefinitely.This PR installs forwarders for the standard termination signals before awaiting the child, and removes them on
close/errorto avoid listener leaks across relaunch iterations.Closes #25590
Reproduction (pre-patch)
Interactive Ctrl+C does not surface the bug because
SIGINTis delivered to the foreground process group by the controlling tty. Only programmatickill(pid, signal)against the parent exposes the leak — which is the normal path for any supervisor.Root cause
packages/cli/src/utils/relaunch.ts, functionrelaunchAppInChildProcess:close, but never callsprocess.on('SIGTERM'/'SIGHUP'/...)to proxy signals tochild.kill(sig).Fix
Install a
Map<NodeJS.Signals, handler>of forwarders forSIGTERM,SIGHUP,SIGINT,SIGQUIT,SIGUSR1,SIGUSR2immediately afterspawn. Each handler callschild.kill(sig)insidetry/catchto tolerate the race where the child has already exited. The forwarders are removed on bothchild.on('close')andchild.on('error')so that the per-iteration listener count stays bounded (otherwise Node logs aMaxListenersExceededwarning after ~10 relaunches).Design notes:
Map<signal, handler>rather thanremoveAllListenerskeeps cleanup precise and avoids disturbing any other listeners the process may have installed.detached: trueis intentionally not introduced — it would change PGID/foreground-tty semantics and could break interactiveCtrl+C.stdio: 'inherit'is unchanged; signal forwarding is orthogonal to stdio wiring.Tests
Added two tests in
packages/cli/src/utils/relaunch.test.ts:should forward termination signals to the child and clean up listeners on close— verifies:spawn, each of the six forwarded signals has exactly one additional listener onprocess.SIGTERMon the parent triggerschild.kill('SIGTERM').should clean up signal listeners on child process error— verifies that if the child emitserror, listeners are still removed (no leak on failure path).All tests pass locally (
vitest run packages/cli/src/utils/relaunch.test.ts: 10/10).Downstream workaround (for users on older versions)
Setting
GEMINI_CLI_NO_RELAUNCH=trueskips the relaunch, which also avoids the bug — but it disables the--max-old-space-sizetuning that the relaunch is designed to apply. This PR fixes the underlying issue so both features work together.Compatibility
Ctrl+Cstill routed via tty).stdiowiring or IPC messaging.