fix: close server on process disconnect#414
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces a process lifecycle manager (installProcessLifecycle) to gracefully handle server shutdown on events like stdin closure, SIGINT, and SIGTERM, and integrates it into the main server entry point. The review feedback suggests extending this lifecycle manager to handle IPC disconnect events (including registering/deregistering listeners and adding corresponding unit tests) and recommends force-exiting the process with exit(1) if the server close operation fails during a stdin-initiated shutdown to prevent the process from hanging.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export type ShutdownReason = 'stdin-end' | 'stdin-close' | 'SIGINT' | 'SIGTERM'; |
There was a problem hiding this comment.
The PR title and description mention handling process disconnect, but the 'disconnect' event is not currently registered or defined in ShutdownReason. In Node.js, when a process is spawned with an IPC channel, the 'disconnect' event is emitted on process when the parent process disconnects. We should add 'disconnect' to the ShutdownReason type and register a listener for it.
| export type ShutdownReason = 'stdin-end' | 'stdin-close' | 'SIGINT' | 'SIGTERM'; | |
| export type ShutdownReason = 'stdin-end' | 'stdin-close' | 'SIGINT' | 'SIGTERM' | 'disconnect'; |
| const onStdinEnd = () => { | ||
| void shutdown('stdin-end'); | ||
| }; | ||
| const onStdinClose = () => { | ||
| void shutdown('stdin-close'); | ||
| }; | ||
| const onSigint = () => { | ||
| void shutdown('SIGINT'); | ||
| }; | ||
| const onSigterm = () => { | ||
| void shutdown('SIGTERM'); | ||
| }; | ||
|
|
||
| function dispose() { | ||
| if (disposed) { | ||
| return; | ||
| } | ||
|
|
||
| disposed = true; | ||
| stdin.off('end', onStdinEnd); | ||
| stdin.off('close', onStdinClose); | ||
| signals.off('SIGINT', onSigint); | ||
| signals.off('SIGTERM', onSigterm); | ||
| } |
There was a problem hiding this comment.
Define the onDisconnect handler and ensure it is properly cleaned up in dispose() when the lifecycle controller is disposed.
const onStdinEnd = () => {
void shutdown('stdin-end');
};
const onStdinClose = () => {
void shutdown('stdin-close');
};
const onSigint = () => {
void shutdown('SIGINT');
};
const onSigterm = () => {
void shutdown('SIGTERM');
};
const onDisconnect = () => {
void shutdown('disconnect');
};
function dispose() {
if (disposed) {
return;
}
disposed = true;
stdin.off('end', onStdinEnd);
stdin.off('close', onStdinClose);
signals.off('SIGINT', onSigint);
signals.off('SIGTERM', onSigterm);
signals.off('disconnect', onDisconnect);
}| stdin.once('end', onStdinEnd); | ||
| stdin.once('close', onStdinClose); | ||
| signals.once('SIGINT', onSigint); | ||
| signals.once('SIGTERM', onSigterm); |
There was a problem hiding this comment.
Register the 'disconnect' event listener on the signals event source (which defaults to process) to handle IPC disconnects.
| stdin.once('end', onStdinEnd); | |
| stdin.once('close', onStdinClose); | |
| signals.once('SIGINT', onSigint); | |
| signals.once('SIGTERM', onSigterm); | |
| stdin.once('end', onStdinEnd); | |
| stdin.once('close', onStdinClose); | |
| signals.once('SIGINT', onSigint); | |
| signals.once('SIGTERM', onSigterm); | |
| signals.once('disconnect', onDisconnect); |
| if (requestedExitCode !== undefined) { | ||
| exit(closeFailed ? 1 : requestedExitCode); | ||
| } else if (closeFailed) { | ||
| setExitCode(1); | ||
| } |
There was a problem hiding this comment.
If options.close() fails during a stdin-initiated shutdown (stdin-end or stdin-close), setting process.exitCode = 1 via setExitCode(1) might not terminate the process if there are active handles (e.g., unclosed sockets, timers, or pending I/O) keeping the event loop alive. To prevent the process from hanging indefinitely on a failed shutdown, it is safer to force-exit the process using exit(1) when closeFailed is true.
| if (requestedExitCode !== undefined) { | |
| exit(closeFailed ? 1 : requestedExitCode); | |
| } else if (closeFailed) { | |
| setExitCode(1); | |
| } | |
| if (requestedExitCode !== undefined) { | |
| exit(closeFailed ? 1 : requestedExitCode); | |
| } else if (closeFailed) { | |
| exit(1); | |
| } |
| it('closes immediately when stdin had already ended', async () => { | ||
| const stdin = new FakeStdin(); | ||
| stdin.readableEnded = true; | ||
| const close = jest.fn(async () => {}); | ||
| const controller = installProcessLifecycle({ | ||
| close, | ||
| stdin, | ||
| signals: new EventEmitter(), | ||
| exit: jest.fn<(code: number) => void>(), | ||
| }); | ||
|
|
||
| await controller.getShutdownPromise(); | ||
|
|
||
| expect(close).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Add a unit test to verify that the process lifecycle controller correctly handles the 'disconnect' event by closing the server.
it('closes immediately when stdin had already ended', async () => {
const stdin = new FakeStdin();
stdin.readableEnded = true;
const close = jest.fn(async () => {});
const controller = installProcessLifecycle({
close,
stdin,
signals: new EventEmitter(),
exit: jest.fn<(code: number) => void>(),
});
await controller.getShutdownPromise();
expect(close).toHaveBeenCalledTimes(1);
});
it('closes once when process disconnects', async () => {
const stdin = new FakeStdin();
const signals = new EventEmitter();
const close = jest.fn(async () => {});
const exit = jest.fn<(code: number) => void>();
const controller = installProcessLifecycle({
close,
stdin,
signals,
exit,
});
signals.emit('disconnect');
await controller.getShutdownPromise();
expect(close).toHaveBeenCalledTimes(1);
expect(exit).not.toHaveBeenCalled();
});| it('removes listeners without closing when disposed', () => { | ||
| const stdin = new FakeStdin(); | ||
| const signals = new EventEmitter(); | ||
| const close = jest.fn(async () => {}); | ||
| const controller = installProcessLifecycle({ close, stdin, signals }); | ||
|
|
||
| controller.dispose(); | ||
| stdin.emit('end'); | ||
| signals.emit('SIGTERM'); | ||
|
|
||
| expect(close).not.toHaveBeenCalled(); | ||
| expect(stdin.listenerCount('end')).toBe(0); | ||
| expect(stdin.listenerCount('close')).toBe(0); | ||
| expect(signals.listenerCount('SIGINT')).toBe(0); | ||
| expect(signals.listenerCount('SIGTERM')).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Update the dispose test to verify that the 'disconnect' event listener is also removed and ignored after disposal.
it('removes listeners without closing when disposed', () => {
const stdin = new FakeStdin();
const signals = new EventEmitter();
const close = jest.fn(async () => {});
const controller = installProcessLifecycle({ close, stdin, signals });
controller.dispose();
stdin.emit('end');
signals.emit('SIGTERM');
signals.emit('disconnect');
expect(close).not.toHaveBeenCalled();
expect(stdin.listenerCount('end')).toBe(0);
expect(stdin.listenerCount('close')).toBe(0);
expect(signals.listenerCount('SIGINT')).toBe(0);
expect(signals.listenerCount('SIGTERM')).toBe(0);
expect(signals.listenerCount('disconnect')).toBe(0);
});
Adds idempotent process lifecycle shutdown handling, wires it into the workspace server entrypoint, and covers disconnect, signal, error, and repeated-shutdown behavior. Local validation passed: 625 tests, lint, TypeScript checks, Prettier check, and build. This Draft PR does not include a release or deployment.