fix: reap orphaned dev-server child on process exit#1695
Conversation
`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 aws#1690
|
This directly closes the gap left by #1438. That PR made the dev child Root cause, mechanism, and a before/after A/B are in the description; unit tests added; @avi-alpert (author of #1438) / @jariy17 — could you take a look? And could a maintainer apply |
|
Claude Security Review: no high-confidence findings. (run) |
tejaskash
left a comment
There was a problem hiding this comment.
LGTM, thank you for your contribution
Description
agentcore devspawns the dev server detached (its own process group, added in #1438) and relies on the dev command'sSIGINT/SIGTERMhandler (onShutdownSignal, registered withprocess.once) to callserver.kill()and tree-kill the group. ButsetupAltScreenCleanup()(src/cli/cli.ts, called first thing inmain()) registers a globalprocess.on('SIGINT'/'SIGTERM')handler that callsprocess.exit(0). Node runs signal listeners in registration order, and this global handler is registered before the dev command's — so it fires first,process.exit(0)terminates the process, andserver.kill()never runs. The detached child is then reparented to PID 1 and keeps holding the port.This reproduces on every stop — both interactive
Ctrl+C(SIGINT) and programmatickill $PID(SIGTERM) — for both Python (uvicorn) and TypeScript/Node dev servers. #1438 correctly fixed the reparenting mechanics (detachedspawn + process-group tree-kill inkill()), but the compensating handler it added is preempted by the pre-existing globalprocess.exit(0), so the orphan is back in 0.22.0 (and now also onCtrl+C, since the detached child no longer receives the terminal's group SIGINT directly).Fix
Register a
process.on('exit')reaper inDevServer.start()that sendsSIGKILLto the detached child's process group. The'exit'event always fires onprocess.exit(), so the child is reaped no matter which path exits the process (the global cleanup handler, an uncaught exception inindex.ts, or a normal exit).kill()'s gracefulSIGTERM-then-SIGKILLpath is unchanged and still runs when it is reached; this is a last-resort net for when it isn't. The listener is removed when the child exits on its own, to avoid a stale reaper firing at final process exit. POSIX-only, matching the POSIX-onlydetachedspawn (on Windows the child is not detached and dies with the parent).Related Issue
Closes #1690
Documentation PR
N/A
Type of Change
Testing
Added unit tests in
dev-server.test.tscovering the reaper: itSIGKILLs the child's process group on exit, is skipped when there is no pid, is removed once the child exits on its own, and swallowsESRCHwhen the group is already gone.Verified end-to-end on macOS with a TypeScript agent, before vs. after this change, mirroring the repro in #1690 / #1440:
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshots (N/A — no asset changes)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.