-
Notifications
You must be signed in to change notification settings - Fork 14.5k
fix(cli): forward termination signals to relaunched child process #28676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
a4e55ba
03f24c1
27586d9
38c31a1
76ebc77
e8fafee
f0cda82
76ca4f9
e8a15af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,23 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||
| type AdminControlsSettings, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@google/gemini-cli-core'; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Signals a supervising process (ACP client, systemd, container runtime) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // may send to the bootstrap parent. Without forwarding, the parent dies on | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // its default disposition while the spawned child is reparented to PID 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // and keeps running - holding the OAuth session and allocated heap until | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // killed manually. See #25590. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // SIGINT/SIGQUIT are intentionally NOT forwarded: when running | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // interactively, the TTY delivers them to the whole foreground process | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // group (parent and child), so forwarding would deliver them twice and | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // could interrupt the child's graceful-shutdown handler. Supervisors use | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // SIGTERM for programmatic termination, which is forwarded. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const FORWARDED_SIGNALS: readonly NodeJS.Signals[] = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 'SIGTERM', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 'SIGHUP', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 'SIGUSR1', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 'SIGUSR2', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forwarding When running interactively in a terminal, the TTY automatically delivers these signals to the entire foreground process group (both the parent and the child process). If the parent also forwards these signals, the child process will receive them twice. If the child process registers a graceful shutdown handler using Since supervisors primarily use const FORWARDED_SIGNALS: readonly NodeJS.Signals[] = [
'SIGTERM',
'SIGHUP',
'SIGUSR1',
'SIGUSR2',
]; |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function relaunchOnExitCode(runner: () => Promise<number>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| while (true) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -71,11 +88,58 @@ export async function relaunchAppInChildProcess( | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Forward termination signals to the child so a supervised parent | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // (kill -TERM <bootstrap-pid>) takes the child down with it instead of | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // orphaning it. Use a Map of {signal -> handler} for precise cleanup on | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // close/error; removeAllListeners would disturb unrelated subscribers, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // and leaking a handler per relaunch iteration trips | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // MaxListenersExceededWarning after ~10 relaunches. #25590. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const forwarders = new Map<NodeJS.Signals, () => void>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const sig of FORWARDED_SIGNALS) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const handler = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| child.kill(sig); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // The child may have already exited; ignore the race. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| forwarders.set(sig, handler); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use on() so that if the child is slow to shut down and a second | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // signal is received, it is still forwarded rather than killing the | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // parent immediately and orphaning the child. The listeners are | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // removed on child close/error anyway. #25590. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| process.on(sig, handler); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+104
to
+127
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using By changing
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const removeForwarders = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const [sig, handler] of forwarders) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| process.off(sig, handler); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| forwarders.clear(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+98
to
+150
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While To prevent this, we should register no-op handlers for |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Promise<number>((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| child.on('error', reject); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| child.on('close', (code) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| child.on('error', (err) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| removeForwarders(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| reject(err); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| child.on('close', (code, signal) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| removeForwarders(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the child process is terminated by a signal, the parent process currently exits with code This misrepresents the termination status to supervisors (like systemd or Kubernetes), which may interpret exit code To propagate the signal termination status correctly, the parent process should kill itself with the same signal after removing the forwarders. Since we cannot directly modify the outer child.on('close', (code, signal) => {
removeForwarders();
// Resume stdin before the parent process exits.
process.stdin.resume();
if (signal) {
try {
process.kill(process.pid, signal);
return;
} catch {
// Fallback if process.kill is not supported
}
}
resolve(code ?? 1);
}); |
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Resume stdin before the parent process exits. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| process.stdin.resume(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Propagate the child's signal termination so supervisors (systemd, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Kubernetes) see a clean signal exit rather than an unexpected code | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // 1 crash. #25590. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (signal) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| process.kill(process.pid, signal); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Do NOT return here: a non-fatal signal (e.g. SIGUSR1) does not | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // terminate this process, so we must still resolve the promise | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // below. If the signal IS fatal, this process exits before | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // resolve runs - which is the desired outcome. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Fall back to exit code 1 if the signal cannot be re-raised. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+164
to
+179
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Since if (signal) {
try {
process.kill(process.pid, signal);
} catch {
// Fall back to exit code 1 if the signal cannot be re-raised.
}
} |
||||||||||||||||||||||||||||||||||||||||||||||||||
| resolve(code ?? 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+164
to
180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because JavaScript is single-threaded, calling To fix this, we should defer the promise resolution using if (signal) {
try {
process.kill(process.pid, signal);
// Defer resolution to allow the event loop to process the signal.
// If the signal is fatal, the process will terminate before the timeout fires.
// If it is non-fatal, we resolve with code ?? 1 after the timeout.
setTimeout(() => {
resolve(code ?? 1);
}, 20);
return;
} catch {
// Fall back to exit code 1 if the signal cannot be re-raised.
}
}
resolve(code ?? 1); |
||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signal propagation logic (
process.kill) is currently untested because calling it directly would terminate the Vitest test runner. To ensure the robustness of the signal propagation and prevent accidental test runner termination in future tests, we should mockprocess.killand add a dedicated test case to verify that the signal is correctly propagated to the parent process.