Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 114 additions & 0 deletions packages/cli/src/utils/relaunch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,120 @@ describe('relaunchAppInChildProcess', () => {
// Should default to exit code 1
expect(processExitSpy).toHaveBeenCalledWith(1);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it('should forward termination signals to the child and clean up listeners on close', async () => {
process.argv = ['/usr/bin/node', '/app/cli.js'];

const FORWARDED: NodeJS.Signals[] = [
'SIGTERM',
'SIGHUP',
'SIGINT',
'SIGQUIT',
'SIGUSR1',
'SIGUSR2',
];
const baseline = Object.fromEntries(
FORWARDED.map((s) => [s, process.listenerCount(s)]),
) as Record<NodeJS.Signals, number>;

const mockChild = createMockChildProcess(0, false);
const listenerCountsAfterSpawn: Record<string, number> = {};

mockedSpawn.mockImplementation(() => {
// Defer so the caller has wired up its listeners first.
setImmediate(() => {
for (const sig of FORWARDED) {
listenerCountsAfterSpawn[sig] = process.listenerCount(sig);
}
// Trigger SIGTERM forwarder.
process.emit('SIGTERM');
// Then close the child so the promise resolves.
setImmediate(() => mockChild.emit('close', 0));
});
return mockChild;
});

await expect(relaunchAppInChildProcess([], [])).rejects.toThrow(
'PROCESS_EXIT_CALLED',
);

// Each forwarded signal gained exactly one listener while the child was alive.
for (const sig of FORWARDED) {
expect(listenerCountsAfterSpawn[sig]).toBe(baseline[sig] + 1);
}
// SIGTERM was forwarded to the child.
expect(mockChild.kill).toHaveBeenCalledWith('SIGTERM');
// After child close, listener counts returned to baseline.
for (const sig of FORWARDED) {
expect(process.listenerCount(sig)).toBe(baseline[sig]);
}
});

it('should not leak signal listeners when child.send throws synchronously', async () => {
process.argv = ['/usr/bin/node', '/app/cli.js'];

const FORWARDED: NodeJS.Signals[] = [
'SIGTERM',
'SIGHUP',
'SIGINT',
'SIGQUIT',
'SIGUSR1',
'SIGUSR2',
];
const baseline = Object.fromEntries(
FORWARDED.map((s) => [s, process.listenerCount(s)]),
) as Record<NodeJS.Signals, number>;

const mockChild = createMockChildProcess(0, false);
(mockChild.send as unknown as MockInstance).mockImplementation(() => {
throw new Error('IPC send failed');
});
mockedSpawn.mockImplementation(() => mockChild);

await expect(
relaunchAppInChildProcess([], [], {
isReadonly: false,
} as unknown as Parameters<typeof relaunchAppInChildProcess>[2]),
).rejects.toThrow('PROCESS_EXIT_CALLED');

// Listeners are attached AFTER child.send(); a synchronous throw from
// child.send must not leave forwarders registered on the parent.
for (const sig of FORWARDED) {
expect(process.listenerCount(sig)).toBe(baseline[sig]);
}
});

it('should clean up signal listeners on child process error', async () => {
process.argv = ['/usr/bin/node', '/app/cli.js'];

const FORWARDED: NodeJS.Signals[] = [
'SIGTERM',
'SIGHUP',
'SIGINT',
'SIGQUIT',
'SIGUSR1',
'SIGUSR2',
];
const baseline = Object.fromEntries(
FORWARDED.map((s) => [s, process.listenerCount(s)]),
) as Record<NodeJS.Signals, number>;

const mockChild = createMockChildProcess(0, false);
mockedSpawn.mockImplementation(() => {
setImmediate(() => {
mockChild.emit('error', new Error('spawn failed'));
});
return mockChild;
});

await expect(relaunchAppInChildProcess([], [])).rejects.toThrow(
'PROCESS_EXIT_CALLED',
);

for (const sig of FORWARDED) {
expect(process.listenerCount(sig)).toBe(baseline[sig]);
}
});
});
});

Expand Down
47 changes: 46 additions & 1 deletion packages/cli/src/utils/relaunch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,19 +61,64 @@ export async function relaunchAppInChildProcess(
env: newEnv,
});

// Forward termination signals so the whole process tree exits cleanly
// rather than orphaning the child under PID 1 / the user's systemd
// manager. Programmatic signals (e.g. `kill -INT <parent_pid>`,
// systemd/container runtimes, supervising ACP clients) target only the
// parent and would otherwise leave the child orphaned. Terminal-generated
// signals (Ctrl+C, Ctrl+\) are already delivered to the whole foreground
// process group by the terminal; the resulting double-delivery is
// harmless for typical signal handlers and strictly preferable to
// orphaning. Waiting for the child to exit via the installed listeners
// also keeps the terminal state clean by preventing the shell prompt
// from returning mid-cleanup.
const FORWARDED_SIGNALS: NodeJS.Signals[] = [
'SIGTERM',
'SIGHUP',
'SIGINT',
'SIGQUIT',
'SIGUSR1',
'SIGUSR2',
];
const forwarders = new Map<NodeJS.Signals, () => void>();
const removeForwarders = () => {
for (const [sig, handler] of forwarders) {
process.off(sig, handler);
}
forwarders.clear();
};

if (latestAdminSettings) {
child.send({ type: 'admin-settings', settings: latestAdminSettings });
}

// Attach listeners only after any synchronous IPC setup that could throw,
// so a thrown child.send() doesn't leak listeners on the parent process.
for (const sig of FORWARDED_SIGNALS) {
const handler = () => {
try {
child.kill(sig);
} catch {
// Child may already be gone; ignore.
}
};
forwarders.set(sig, handler);
process.on(sig, handler);
}

child.on('message', (msg: { type?: string; settings?: unknown }) => {
if (msg.type === 'admin-settings-update' && msg.settings) {
latestAdminSettings = msg.settings as AdminControlsSettings;
}
});

return new Promise<number>((resolve, reject) => {
child.on('error', reject);
child.on('error', (err) => {
removeForwarders();
reject(err);
});
child.on('close', (code) => {
removeForwarders();
// Resume stdin before the parent process exits.
process.stdin.resume();
resolve(code ?? 1);
Expand Down