fix(cli): make forwardSignalsToChild idempotent via WeakSet guard
Prevents duplicate SIGINT/SIGTERM handlers if called more than once for the same ChildProcess — avoids double killProcessTree and racing process.exit(1) from stacked SIGKILL fallback timers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
750a7140d4
commit
aa859debc8
|
|
@ -1,13 +1,20 @@
|
|||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { EventEmitter } from "node:events";
|
||||
import type { ChildProcess } from "node:child_process";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
|
||||
const { mockExecFile } = vi.hoisted(() => ({
|
||||
const { mockExecFile, mockKillProcessTree } = vi.hoisted(() => ({
|
||||
mockExecFile: vi.fn(),
|
||||
mockKillProcessTree: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
vi.mock("node:child_process", () => ({
|
||||
execFile: mockExecFile,
|
||||
}));
|
||||
|
||||
vi.mock("@composio/ao-core", () => ({
|
||||
killProcessTree: mockKillProcessTree,
|
||||
}));
|
||||
|
||||
import {
|
||||
exec,
|
||||
execSilent,
|
||||
|
|
@ -16,8 +23,13 @@ import {
|
|||
gh,
|
||||
getTmuxSessions,
|
||||
getTmuxActivity,
|
||||
forwardSignalsToChild,
|
||||
} from "../../src/lib/shell.js";
|
||||
|
||||
function makeFakeChild(): ChildProcess {
|
||||
return new EventEmitter() as unknown as ChildProcess;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
mockExecFile.mockReset();
|
||||
});
|
||||
|
|
@ -182,3 +194,66 @@ describe("getTmuxActivity", () => {
|
|||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("forwardSignalsToChild", () => {
|
||||
afterEach(() => {
|
||||
process.removeAllListeners("SIGINT");
|
||||
process.removeAllListeners("SIGTERM");
|
||||
mockKillProcessTree.mockClear();
|
||||
});
|
||||
|
||||
it("registers SIGINT and SIGTERM listeners on the process", () => {
|
||||
const child = makeFakeChild();
|
||||
const before = process.listenerCount("SIGINT");
|
||||
forwardSignalsToChild(1234, child);
|
||||
expect(process.listenerCount("SIGINT")).toBe(before + 1);
|
||||
expect(process.listenerCount("SIGTERM")).toBe(before + 1);
|
||||
});
|
||||
|
||||
it("is idempotent — calling twice for the same child registers only one handler per signal", () => {
|
||||
const child = makeFakeChild();
|
||||
const before = process.listenerCount("SIGINT");
|
||||
forwardSignalsToChild(1234, child);
|
||||
forwardSignalsToChild(1234, child);
|
||||
expect(process.listenerCount("SIGINT")).toBe(before + 1);
|
||||
expect(process.listenerCount("SIGTERM")).toBe(before + 1);
|
||||
});
|
||||
|
||||
it("allows independent registration for different child objects", () => {
|
||||
const childA = makeFakeChild();
|
||||
const childB = makeFakeChild();
|
||||
const before = process.listenerCount("SIGINT");
|
||||
forwardSignalsToChild(1234, childA);
|
||||
forwardSignalsToChild(5678, childB);
|
||||
expect(process.listenerCount("SIGINT")).toBe(before + 2);
|
||||
expect(process.listenerCount("SIGTERM")).toBe(before + 2);
|
||||
});
|
||||
|
||||
it("removes SIGTERM handler when SIGINT fires", () => {
|
||||
const child = makeFakeChild();
|
||||
forwardSignalsToChild(1234, child);
|
||||
const sigtermBefore = process.listenerCount("SIGTERM");
|
||||
process.emit("SIGINT");
|
||||
expect(process.listenerCount("SIGTERM")).toBe(sigtermBefore - 1);
|
||||
expect(mockKillProcessTree).toHaveBeenCalledWith(1234, "SIGTERM");
|
||||
});
|
||||
|
||||
it("removes SIGINT handler when SIGTERM fires", () => {
|
||||
const child = makeFakeChild();
|
||||
forwardSignalsToChild(1234, child);
|
||||
const sigintBefore = process.listenerCount("SIGINT");
|
||||
process.emit("SIGTERM");
|
||||
expect(process.listenerCount("SIGINT")).toBe(sigintBefore - 1);
|
||||
expect(mockKillProcessTree).toHaveBeenCalledWith(1234, "SIGTERM");
|
||||
});
|
||||
|
||||
it("removes both signal handlers when child exits normally", () => {
|
||||
const child = makeFakeChild();
|
||||
forwardSignalsToChild(1234, child);
|
||||
const sigintBefore = process.listenerCount("SIGINT");
|
||||
const sigtermBefore = process.listenerCount("SIGTERM");
|
||||
child.emit("exit", 0, null);
|
||||
expect(process.listenerCount("SIGINT")).toBe(sigintBefore - 1);
|
||||
expect(process.listenerCount("SIGTERM")).toBe(sigtermBefore - 1);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -62,8 +62,14 @@ export async function getTmuxSessions(): Promise<string[]> {
|
|||
* Sends SIGTERM immediately, then escalates to SIGKILL after 5 s if the child
|
||||
* has not exited — prevents the parent from hanging indefinitely with no signal
|
||||
* handlers registered. Cleans up automatically when the child exits normally.
|
||||
*
|
||||
* Idempotent: calling with the same child object more than once is a no-op.
|
||||
*/
|
||||
const _signalForwardedChildren = new WeakSet<ChildProcess>();
|
||||
|
||||
export function forwardSignalsToChild(pid: number, child: ChildProcess): void {
|
||||
if (_signalForwardedChildren.has(child)) return;
|
||||
_signalForwardedChildren.add(child);
|
||||
let fallback: ReturnType<typeof setTimeout> | undefined;
|
||||
|
||||
const forward = (): void => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue