Repository navigation
fix: forward signals from parent to child process to prevent orphans - #29427
dylanyunlon wants to merge 1 commit into
Conversation
When the parent process (bootstrap or relaunch wrapper) receives a termination signal such as SIGTERM or SIGHUP, the child process is now notified instead of being silently orphaned. Root cause: both index.ts (lightweight parent) and relaunch.ts (relaunchAppInChildProcess) spawned a child with stdio:'inherit' but never proxied signals. The parent either swallowed them with empty handlers (index.ts) or let the default disposition kill it while the child kept running (relaunch.ts). Fix: - Add SignalForwarder class in @google/gemini-cli-core that installs precise per-signal handlers, forwards them to the child via child.kill(), and escalates to SIGKILL after a configurable grace period (default 5s) for fatal signals (SIGTERM, SIGHUP, SIGQUIT). - Integrate into relaunch.ts via the shared SignalForwarder class. - Integrate into index.ts with inline forwarding (avoids importing the full core package to keep startup fast). - Clean up handlers on child 'close' and 'error' events to prevent listener leaks across relaunch iterations. - Add isChildProcess() helper to processUtils.ts. - Update cleanup.ts JSDoc to document interaction with forwarding. - Add GEMINI_CLI_NO_RELAUNCH to test-rig.ts clean env. - Add build verification in build_package.js for the new module. Tests: - 31 unit tests for SignalForwarder class (install, remove, forwarding, escalation, idempotency, edge cases). - 7 unit tests for signal forwarding in relaunch.test.ts (SIGTERM, SIGHUP, SIGUSR1/2, error handling, listener leak prevention). - 3 integration tests spawning real processes verifying OS-level signal delivery (SIGTERM, SIGHUP, SIGUSR1). - 3 unit tests for isChildProcess(). - 2 unit tests for cleanup signal handler coexistence. Fixes google-gemini#25590
|
You already have 7 pull requests open. Please work on getting existing PRs merged before opening more. |
|
📊 PR Size: size/XL
|
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 become orphaned when the parent process receives termination signals. By implementing a robust signal forwarding mechanism, the parent now correctly proxies signals to the child and enforces a grace period before escalating to SIGKILL. This ensures that child processes are properly cleaned up, improving system stability and resource management. 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 the 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 counterproductive. 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 from the parent bootstrap process to the spawned child process to prevent orphaned child processes when the parent is terminated (addressing issue #25590). It introduces a SignalForwarder utility class in packages/core that handles forwarding of standard termination signals (SIGTERM, SIGHUP, SIGINT, SIGQUIT, SIGUSR1, SIGUSR2) and escalates to SIGKILL after a grace period for fatal signals. The parent CLI process utilizes this forwarder, and appropriate cleanup mechanisms are added to prevent listener leaks. Comprehensive unit, integration, and end-to-end tests are added to verify the behavior. I have no additional feedback to provide as the implementation is robust and well-tested.
Summary
Fixes #25590
When the parent process (bootstrap in
index.tsor relaunch wrapper inrelaunch.ts) receives a termination signal (SIGTERM,SIGHUP, etc.), the child process was silently orphaned - reparented to PID 1 and left running indefinitely.Root cause
Two spawn paths, same bug:
index.ts(lightweight parent)() => {}that swallowed SIGTERM/SIGHUP/SIGINT without forwarding to childrelaunch.ts(relaunchAppInChildProcess)Fix
New:
SignalForwarderclass (@google/gemini-cli-core)processthat forward tochild.kill(sig)SIGKILLafter configurable grace period (default 5s) for fatal signalsinstall()/remove()prevents listener leaks across relaunch iterationstry/catchguards the race where signal arrives just after child exits (ESRCH)Modified:
relaunch.tsSignalForwarderfrom corecloseanderroreventsModified:
index.tsSignalForwarderclassSupporting changes
processUtils.ts: addedisChildProcess()helpercleanup.ts: documented interaction between child cleanup handlers and parent signal forwardingtest-rig.ts: setGEMINI_CLI_NO_RELAUNCH=truein test harness clean envbuild_package.js: added build verification for the new moduleAST call chain coverage
Test coverage
signalForwarding.test.tsrelaunch.test.tssignalForwarding.integration.test.tsprocessUtils.test.tsisChildProcess()cleanup.test.tsTotal: 46 new tests, all passing
Reproduction
Stats