Skip to content

Commit 0f115e7

Browse files
authored
fix: reap orphaned dev-server child on process exit (#1695)
`agentcore dev` spawns the dev server detached (its own process group) and relies on the dev command's SIGINT/SIGTERM handler to call server.kill(). But setupAltScreenCleanup() registers a global SIGINT/SIGTERM handler at startup that calls process.exit(0), and it runs first — so kill() never runs and the detached child is orphaned, holding the port on every stop (Ctrl+C or `kill $PID`), for both Python and TypeScript dev servers. Register a process.on('exit') reaper in DevServer.start() that SIGKILLs the child's process group. The 'exit' event always fires on process.exit(), so the child is reaped regardless of which shutdown path exits the process. The listener is removed when the child exits on its own. Closes #1690
1 parent 192ff38 commit 0f115e7

2 files changed

Lines changed: 83 additions & 1 deletion

File tree

src/cli/operations/dev/__tests__/dev-server.test.ts

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { DevConfig } from '../config.js';
22
import { DevServer, type DevServerCallbacks, type DevServerOptions, type SpawnConfig } from '../dev-server.js';
33
import { EventEmitter } from 'events';
4-
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
4+
import { type MockInstance, afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
55

66
const mockSpawn = vi.fn();
77
vi.mock('child_process', () => ({
@@ -56,6 +56,8 @@ describe('DevServer', () => {
5656
let options: DevServerOptions;
5757
let server: TestDevServer;
5858
let mockChild: ReturnType<typeof createMockChildProcess>;
59+
let onceSpy: MockInstance;
60+
let removeListenerSpy: MockInstance;
5961

6062
beforeEach(() => {
6163
onLog = vi.fn<DevServerCallbacks['onLog']>();
@@ -65,12 +67,20 @@ describe('DevServer', () => {
6567
server = new TestDevServer(config, options);
6668
mockChild = createMockChildProcess();
6769
mockSpawn.mockReturnValue(mockChild);
70+
onceSpy = vi.spyOn(process, 'once').mockReturnValue(process);
71+
removeListenerSpy = vi.spyOn(process, 'removeListener').mockReturnValue(process);
6872
});
6973

7074
afterEach(() => {
75+
vi.restoreAllMocks();
7176
vi.clearAllMocks();
7277
});
7378

79+
function getRegisteredExitHandler(): (() => void) | undefined {
80+
const call = onceSpy.mock.calls.find(([event]) => event === 'exit');
81+
return call?.[1] as (() => void) | undefined;
82+
}
83+
7484
describe('start()', () => {
7585
it('calls spawn with correct cmd, args, cwd, env, and stdio when prepare succeeds', async () => {
7686
await server.start();
@@ -173,6 +183,47 @@ describe('DevServer', () => {
173183
});
174184
});
175185

186+
describe('exit cleanup', () => {
187+
it('reaps the detached process group on process exit', async () => {
188+
mockChild.pid = 4242;
189+
const processKillSpy = vi.spyOn(process, 'kill').mockImplementation(() => true);
190+
191+
await server.start();
192+
const exitHandler = getRegisteredExitHandler();
193+
expect(exitHandler).toBeDefined();
194+
195+
exitHandler!();
196+
expect(processKillSpy).toHaveBeenCalledWith(-4242, 'SIGKILL');
197+
});
198+
199+
it('does not register an exit reaper when the child has no pid', async () => {
200+
await server.start();
201+
expect(getRegisteredExitHandler()).toBeUndefined();
202+
});
203+
204+
it('removes the exit reaper once the child exits on its own', async () => {
205+
mockChild.pid = 4242;
206+
207+
await server.start();
208+
const exitHandler = getRegisteredExitHandler();
209+
mockChild.emit('exit', 0);
210+
211+
expect(removeListenerSpy).toHaveBeenCalledWith('exit', exitHandler);
212+
});
213+
214+
it('swallows errors when the process group is already gone', async () => {
215+
mockChild.pid = 4242;
216+
vi.spyOn(process, 'kill').mockImplementation(() => {
217+
throw new Error('kill ESRCH');
218+
});
219+
220+
await server.start();
221+
const exitHandler = getRegisteredExitHandler();
222+
223+
expect(() => exitHandler!()).not.toThrow();
224+
});
225+
});
226+
176227
describe('output routing', () => {
177228
it('forwards stdout lines to onLog at info level', async () => {
178229
await server.start();

src/cli/operations/dev/dev-server.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ function isInternalFrame(line: string): boolean {
4848

4949
export abstract class DevServer {
5050
protected child: ChildProcess | null = null;
51+
private onProcessExit: (() => void) | null = null;
5152
private recentStderr: string[] = [];
5253
private inTraceback = false;
5354
private tracebackBuffer: string[] = [];
@@ -77,9 +78,38 @@ export abstract class DevServer {
7778
});
7879

7980
this.attachHandlers();
81+
this.registerExitCleanup();
8082
return this.child;
8183
}
8284

85+
/**
86+
* Reap the detached child's process group when this process exits.
87+
*
88+
* The dev command's SIGINT/SIGTERM handler (which calls kill()) is registered after the
89+
* global handler in setupAltScreenCleanup, which runs first and calls process.exit(0) — so
90+
* kill() never runs and the child (its own process group since it is spawned detached) is
91+
* orphaned, holding the port. The 'exit' event always fires on process.exit(), so use it as
92+
* a last-resort reaper. POSIX only: on Windows the child is not detached and dies with the parent.
93+
*/
94+
private registerExitCleanup(): void {
95+
const pid = this.child?.pid;
96+
if (isWindows || !pid) return;
97+
this.onProcessExit = () => {
98+
try {
99+
process.kill(-pid, 'SIGKILL');
100+
} catch {
101+
// Group already gone — nothing to reap.
102+
}
103+
};
104+
process.once('exit', this.onProcessExit);
105+
}
106+
107+
private clearExitCleanup(): void {
108+
if (!this.onProcessExit) return;
109+
process.removeListener('exit', this.onProcessExit);
110+
this.onProcessExit = null;
111+
}
112+
83113
/** Kill the dev server process tree. Sends SIGTERM to the process group, then SIGKILL after 2 seconds. */
84114
kill(): void {
85115
if (!this.child || this.child.killed) return;
@@ -203,6 +233,7 @@ export abstract class DevServer {
203233
});
204234

205235
this.child?.on('exit', code => {
236+
this.clearExitCleanup();
206237
if (code !== 0 && code !== null && this.recentStderr.length > 0) {
207238
for (const line of this.recentStderr) {
208239
onLog('error', line);

0 commit comments

Comments
 (0)