From 395b9ffaaa78d2480e27f86bcdc6b47772aaabe3 Mon Sep 17 00:00:00 2001 From: "Hagay M." Date: Tue, 7 Jul 2026 11:09:03 +0300 Subject: [PATCH] fix: reap orphaned dev-server child on process exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../dev/__tests__/dev-server.test.ts | 53 ++++++++++++++++++- src/cli/operations/dev/dev-server.ts | 31 +++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/cli/operations/dev/__tests__/dev-server.test.ts b/src/cli/operations/dev/__tests__/dev-server.test.ts index b6ca6085b..3e00e0c14 100644 --- a/src/cli/operations/dev/__tests__/dev-server.test.ts +++ b/src/cli/operations/dev/__tests__/dev-server.test.ts @@ -1,7 +1,7 @@ import type { DevConfig } from '../config.js'; import { DevServer, type DevServerCallbacks, type DevServerOptions, type SpawnConfig } from '../dev-server.js'; import { EventEmitter } from 'events'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { type MockInstance, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockSpawn = vi.fn(); vi.mock('child_process', () => ({ @@ -56,6 +56,8 @@ describe('DevServer', () => { let options: DevServerOptions; let server: TestDevServer; let mockChild: ReturnType; + let onceSpy: MockInstance; + let removeListenerSpy: MockInstance; beforeEach(() => { onLog = vi.fn(); @@ -65,12 +67,20 @@ describe('DevServer', () => { server = new TestDevServer(config, options); mockChild = createMockChildProcess(); mockSpawn.mockReturnValue(mockChild); + onceSpy = vi.spyOn(process, 'once').mockReturnValue(process); + removeListenerSpy = vi.spyOn(process, 'removeListener').mockReturnValue(process); }); afterEach(() => { + vi.restoreAllMocks(); vi.clearAllMocks(); }); + function getRegisteredExitHandler(): (() => void) | undefined { + const call = onceSpy.mock.calls.find(([event]) => event === 'exit'); + return call?.[1] as (() => void) | undefined; + } + describe('start()', () => { it('calls spawn with correct cmd, args, cwd, env, and stdio when prepare succeeds', async () => { await server.start(); @@ -173,6 +183,47 @@ describe('DevServer', () => { }); }); + describe('exit cleanup', () => { + it('reaps the detached process group on process exit', async () => { + mockChild.pid = 4242; + const processKillSpy = vi.spyOn(process, 'kill').mockImplementation(() => true); + + await server.start(); + const exitHandler = getRegisteredExitHandler(); + expect(exitHandler).toBeDefined(); + + exitHandler!(); + expect(processKillSpy).toHaveBeenCalledWith(-4242, 'SIGKILL'); + }); + + it('does not register an exit reaper when the child has no pid', async () => { + await server.start(); + expect(getRegisteredExitHandler()).toBeUndefined(); + }); + + it('removes the exit reaper once the child exits on its own', async () => { + mockChild.pid = 4242; + + await server.start(); + const exitHandler = getRegisteredExitHandler(); + mockChild.emit('exit', 0); + + expect(removeListenerSpy).toHaveBeenCalledWith('exit', exitHandler); + }); + + it('swallows errors when the process group is already gone', async () => { + mockChild.pid = 4242; + vi.spyOn(process, 'kill').mockImplementation(() => { + throw new Error('kill ESRCH'); + }); + + await server.start(); + const exitHandler = getRegisteredExitHandler(); + + expect(() => exitHandler!()).not.toThrow(); + }); + }); + describe('output routing', () => { it('forwards stdout lines to onLog at info level', async () => { await server.start(); diff --git a/src/cli/operations/dev/dev-server.ts b/src/cli/operations/dev/dev-server.ts index d44d4c765..3d208eb51 100644 --- a/src/cli/operations/dev/dev-server.ts +++ b/src/cli/operations/dev/dev-server.ts @@ -48,6 +48,7 @@ function isInternalFrame(line: string): boolean { export abstract class DevServer { protected child: ChildProcess | null = null; + private onProcessExit: (() => void) | null = null; private recentStderr: string[] = []; private inTraceback = false; private tracebackBuffer: string[] = []; @@ -77,9 +78,38 @@ export abstract class DevServer { }); this.attachHandlers(); + this.registerExitCleanup(); return this.child; } + /** + * Reap the detached child's process group when this process exits. + * + * The dev command's SIGINT/SIGTERM handler (which calls kill()) is registered after the + * global handler in setupAltScreenCleanup, which runs first and calls process.exit(0) — so + * kill() never runs and the child (its own process group since it is spawned detached) is + * orphaned, holding the port. The 'exit' event always fires on process.exit(), so use it as + * a last-resort reaper. POSIX only: on Windows the child is not detached and dies with the parent. + */ + private registerExitCleanup(): void { + const pid = this.child?.pid; + if (isWindows || !pid) return; + this.onProcessExit = () => { + try { + process.kill(-pid, 'SIGKILL'); + } catch { + // Group already gone — nothing to reap. + } + }; + process.once('exit', this.onProcessExit); + } + + private clearExitCleanup(): void { + if (!this.onProcessExit) return; + process.removeListener('exit', this.onProcessExit); + this.onProcessExit = null; + } + /** Kill the dev server process tree. Sends SIGTERM to the process group, then SIGKILL after 2 seconds. */ kill(): void { if (!this.child || this.child.killed) return; @@ -203,6 +233,7 @@ export abstract class DevServer { }); this.child?.on('exit', code => { + this.clearExitCleanup(); if (code !== 0 && code !== null && this.recentStderr.length > 0) { for (const line of this.recentStderr) { onLog('error', line);