bkinnightskytw
/
agent-orchestrator
mirror of https://github.com/ComposioHQ/agent-orchestrator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(cli): address review — use exit handler, verify PIDs, filter kills (#645) 

1. Replace SIGINT/SIGTERM handlers with a process `exit` handler to avoid
   conflicting with the shutdown handler that flushes lifecycle state and
   exits with the correct code (130 for SIGINT).

2. Expand dashboard process pattern to match dev mode (next dev, ao-web)
   in addition to production (next-server, start-all.js).

3. Only kill dashboard-matching PIDs from lsof output, leaving unrelated
   co-listeners (sidecars, SO_REUSEPORT) untouched.

4. Use killDashboardOnPort for the first port attempt too, preventing
   blind kills on the configured port when running.json is stale.

5. Add test for mixed-PID filtering on a single port.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
adil 2026-04-15 03:01:41 +05:30
parent 47e14782d1
commit 80932f367a
2 changed files with 83 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
packages/cli/__tests__/commands/start.test.ts
View File

@ -1022,11 +1022,24 @@ describe("start command — orchestrator session strategy display", () => {
// ---------------------------------------------------------------------------
describe("stop command", () => {
/** Helper: mock exec to simulate a dashboard process on a given port. */
function mockDashboardOnPort(dashboardPort: number, pid = "12345"): void {
mockExec.mockImplementation(async (cmd: string, args: string[] = []) => {
if (cmd === "kill") return { stdout: "", stderr: "" };
if (cmd === "ps") return { stdout: "node /fake/web/dist-server/start-all.js", stderr: "" };
if (cmd === "lsof") {
const portArg = args.find((a) => a.startsWith(":"));
if (portArg === `:${dashboardPort}`) return { stdout: pid, stderr: "" };
}
throw new Error("no process");
});
}
it("stops orchestrator session and dashboard", async () => {
mockConfigRef.current = makeConfig({ "my-app": makeProject() });
mockSessionManager.get.mockResolvedValue({ id: "app-orchestrator", status: "running" });
mockSessionManager.kill.mockResolvedValue(undefined);
mockExec.mockResolvedValue({ stdout: "12345", stderr: "" });
mockDashboardOnPort(3000);
await program.parseAsync(["node", "test", "stop"]);
@ -1059,7 +1072,7 @@ describe("stop command", () => {
mockConfigRef.current = makeConfig({ "my-app": makeProject() });
mockSessionManager.get.mockResolvedValue({ id: "app-orchestrator", status: "running" });
mockSessionManager.kill.mockResolvedValue(undefined);
mockExec.mockResolvedValue({ stdout: "12345", stderr: "" });
mockDashboardOnPort(3000);
await program.parseAsync(["node", "test", "stop", "--purge-session"]);
@ -1073,13 +1086,7 @@ describe("stop command", () => {
mockSessionManager.get.mockResolvedValue({ id: "app-orchestrator", status: "running" });
mockSessionManager.kill.mockResolvedValue(undefined);
// Port 3000 has nothing, but port 3001 has the orphaned dashboard
mockExec.mockImplementation(async (cmd: string, args: string[] = []) => {
if (cmd === "kill") return { stdout: "", stderr: "" };
if (cmd === "ps") return { stdout: "node /fake/web/dist-server/start-all.js", stderr: "" };
const portArg = args.find((a) => a.startsWith(":"));
if (portArg === ":3001") return { stdout: "99999", stderr: "" };
throw new Error("no process");
});
mockDashboardOnPort(3001, "99999");
await program.parseAsync(["node", "test", "stop"]);
@ -1104,9 +1111,11 @@ describe("stop command", () => {
if (pid === "22222") return { stdout: "node /fake/web/dist-server/start-all.js", stderr: "" };
return { stdout: "", stderr: "" };
}
const portArg = args.find((a) => a.startsWith(":"));
if (portArg === ":3001") return { stdout: "11111", stderr: "" };
if (portArg === ":3002") return { stdout: "22222", stderr: "" };
if (cmd === "lsof") {
const portArg = args.find((a) => a.startsWith(":"));
if (portArg === ":3001") return { stdout: "11111", stderr: "" };
if (portArg === ":3002") return { stdout: "22222", stderr: "" };
}
throw new Error("no process");
});
@ -1119,6 +1128,39 @@ describe("stop command", () => {
// Should skip port 3001 (python) and find the dashboard on 3002
expect(output).toContain("was on port 3002");
});
it("only kills dashboard PIDs when port has mixed processes", async () => {
mockConfigRef.current = makeConfig({ "my-app": makeProject() });
mockSessionManager.get.mockResolvedValue({ id: "app-orchestrator", status: "running" });
mockSessionManager.kill.mockResolvedValue(undefined);
// Port 3000 has two processes: a dashboard and an unrelated sidecar
mockExec.mockImplementation(async (cmd: string, args: string[] = []) => {
if (cmd === "kill") {
// Only the dashboard PID should be killed, not the sidecar
expect(args).toEqual(["11111"]);
return { stdout: "", stderr: "" };
}
if (cmd === "ps") {
const pid = args[1];
if (pid === "11111") return { stdout: "node /fake/web/dist-server/start-all.js", stderr: "" };
if (pid === "22222") return { stdout: "nginx: worker process", stderr: "" };
return { stdout: "", stderr: "" };
}
if (cmd === "lsof") {
const portArg = args.find((a) => a.startsWith(":"));
if (portArg === ":3000") return { stdout: "11111\n22222", stderr: "" };
}
throw new Error("no process");
});
await program.parseAsync(["node", "test", "stop"]);
const output = vi
.mocked(console.log)
.mock.calls.map((c) => c.join(" "))
.join("\n");
expect(output).toContain("Dashboard stopped");
});
});
// ---------------------------------------------------------------------------

80
packages/cli/src/commands/start.ts
View File

@ -1145,20 +1145,14 @@ async function runStartup(
// Keep dashboard process alive if it was started
if (dashboardProcess) {
// Ensure the dashboard child is killed when the parent exits (e.g. Ctrl+C).
// Node.js does not guarantee signal propagation to child processes.
// Registering a SIGINT handler suppresses Node's default exit, so we
// must call process.exit() ourselves after cleaning up the child.
/* c8 ignore start -- signal handlers only fire on process termination */
const killAndExit = (): void => {
try {
dashboardProcess?.kill("SIGTERM");
} catch {
// already dead
}
process.exit();
};
const killDashboardOnly = (): void => {
// Kill the dashboard child when the parent exits for any reason
// (Ctrl+C, SIGTERM from `ao stop`, normal exit, etc.).
// We use the `exit` event instead of SIGINT/SIGTERM to avoid
// conflicting with the shutdown handler in registerStart that
// flushes lifecycle state and calls process.exit() with the
// correct exit code (130 for SIGINT, 0 for SIGTERM).
/* c8 ignore start -- exit handler only fires on process termination */
const killDashboardChild = (): void => {
try {
dashboardProcess?.kill("SIGTERM");
} catch {
@ -1166,14 +1160,10 @@ async function runStartup(
}
};
/* c8 ignore stop */
process.on("SIGINT", killAndExit);
process.on("SIGTERM", killAndExit);
process.on("exit", killDashboardOnly);
process.on("exit", killDashboardChild);
dashboardProcess.on("exit", (code) => {
process.removeListener("SIGINT", killAndExit);
process.removeListener("SIGTERM", killAndExit);
process.removeListener("exit", killDashboardOnly);
process.removeListener("exit", killDashboardChild);
if (openAbort) openAbort.abort();
if (code !== 0 && code !== null) {
console.error(chalk.red(`Dashboard exited with code ${code}`));
@ -1190,25 +1180,13 @@ async function runStartup(
* Uses lsof to find the process listening on the port, then kills it.
* Best effort — if it fails, just warn the user.
*/
async function killOnPort(port: number): Promise<boolean> {
try {
const { stdout } = await exec("lsof", ["-ti", `:${port}`]);
const pids = stdout
.trim()
.split("\n")
.filter((p) => p.length > 0);
if (pids.length === 0) return false;
await exec("kill", pids);
return true;
} catch {
return false;
}
}
/** Pattern matching AO dashboard processes (production and dev mode). */
const DASHBOARD_CMD_PATTERN = /next-server|start-all\.js|next dev|ao-web/;
/**
* Check whether a process listening on the given port is an AO dashboard
* (next-server / node running start-all.js). Only kill if it matches,
* to avoid terminating unrelated services during the port-range scan.
* (next-server, start-all.js, or next dev). Only kills matching PIDs,
* leaving unrelated co-listeners (sidecars, SO_REUSEPORT) untouched.
*/
async function killDashboardOnPort(port: number): Promise<boolean> {
try {
@ -1219,20 +1197,21 @@ async function killDashboardOnPort(port: number): Promise<boolean> {
.filter((p) => p.length > 0);
if (pids.length === 0) return false;
// Verify at least one PID is an AO dashboard process
const isDashboard = await Promise.all(
pids.map(async (pid) => {
try {
const { stdout: cmdline } = await exec("ps", ["-p", pid, "-o", "args="]);
return /next-server|start-all\.js/.test(cmdline);
} catch {
return false;
// Filter to only dashboard PIDs
const dashboardPids: string[] = [];
for (const pid of pids) {
try {
const { stdout: cmdline } = await exec("ps", ["-p", pid, "-o", "args="]);
if (DASHBOARD_CMD_PATTERN.test(cmdline)) {
dashboardPids.push(pid);
}
}),
);
if (!isDashboard.some(Boolean)) return false;
} catch {
// process vanished — skip
}
}
if (dashboardPids.length === 0) return false;
await exec("kill", pids);
await exec("kill", dashboardPids);
return true;
} catch {
return false;
@ -1240,9 +1219,8 @@ async function killDashboardOnPort(port: number): Promise<boolean> {
}
async function stopDashboard(port: number): Promise<void> {
// 1. Try the expected port — no process-name check needed because
// the caller already knows a dashboard was started on this port
if (await killOnPort(port)) {
// 1. Try the expected port — verify it's a dashboard before killing
if (await killDashboardOnPort(port)) {
console.log(chalk.green("Dashboard stopped"));
return;
}