Skip to content

fix(windows): clean up child processes via Job Objects - #1234

Open
LocoLoboZ wants to merge 1 commit into
smart-mcp-proxy:mainfrom
LocoLoboZ:fix/windows-job-object-child-process-cleanup
Open

fix(windows): clean up child processes via Job Objects#1234
LocoLoboZ wants to merge 1 commit into
smart-mcp-proxy:mainfrom
LocoLoboZ:fix/windows-job-object-child-process-cleanup

Conversation

@LocoLoboZ

Copy link
Copy Markdown

Problem

internal/upstream/core/process_windows.go and internal/upstream/launcher/launcher_windows.go were both TODO stubs. killProcessGroup did nothing, and terminateProcess/killProcess only ever killed the one PID mcpproxy itself spawned (e.g. cmd.exe), never the grandchildren it spawns (node.exe, python.exe, the actual MCP server process).

Every stdio server restart, reconnect, or disconnect on Windows leaks its grandchild process tree permanently. Confirmed 413 orphaned node.exe/python.exe/qmcp.exe processes accumulated from under an hour of normal use with ~15 configured servers on one machine.

Fix

Wraps each spawned process in a Windows Job Object configured with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE (new internal/winjob package). Job membership is inherited by anything the process spawns afterwards, so closing the job reaches the whole tree in one call — the Windows equivalent of kill(-pgid, ...) on the existing Unix path.

Two details that mattered in testing:

  1. The job must be assigned right after cmd.Start() succeeds, before the MCP initialize() handshake — not after a successful handshake. Assigning only on success misses every leak caused by a restart/disconnect interrupting an in-progress connect attempt, which is the dominant real-world case (a slow npx download, bulk server-add contention, a timeout mid-handshake). This was the actual cause of most of the 413 leaked processes observed.
  2. golang.org/x/sys/windows (as vendored here, v0.47.0) does not export Set/QueryInformationJobObject, only CreateJobObject/AssignProcessToJobObject/TerminateJobObject. winjob.go binds SetInformationJobObject directly via NewLazySystemDLL rather than adding a new dependency for one call.

Same fix applied to the separate launcher package (HTTP/SSE launcher-managed servers with Command set), which had the identical gap and an explicit // left to a follow-up (matching the TODO already in internal/upstream/core/process_windows.go) comment pointing at this file.

Verification

  • Standalone reproduction: spawn cmd.exeping.exe (grandchild), assign to job, Close() → grandchild confirmed killed.
  • 6 sequential real restarts of a stdio server (mcpproxy upstream restart <name>, few seconds apart) → 0 leaked processes, where every restart leaked one before this fix.
  • go build/go vet clean for windows/amd64.
  • Existing internal/upstream/launcher test suite passes unmodified.

Known remaining gap (out of scope for this PR)

Firing many restarts of the same server at the exact same instant (an artificial stress case constructed for testing, not normal operation — mcpproxy's own reconcile loop restarts sequentially) can still leak a few processes. That traces to a separate race on the shared processGroupID field across concurrent Connect/Disconnect calls on one Client, is not Windows-specific, and is not addressed here.

internal/upstream/core/process_windows.go and
internal/upstream/launcher/launcher_windows.go were both TODO stubs:
killProcessGroup did nothing, and terminateProcess/killProcess only ever
killed the one PID mcpproxy itself spawned (e.g. cmd.exe), never the
grandchildren it spawns (node.exe, python.exe, the actual MCP server).
Every stdio server restart, reconnect, or disconnect on Windows leaked
its grandchild process tree permanently — confirmed 413 orphaned
node.exe/python.exe/qmcp.exe processes accumulated from under an hour
of normal use on one machine.

This wraps each spawned process in a Windows Job Object configured with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE (new internal/winjob package). Job
membership is inherited by anything the process spawns afterwards, so
closing the job reaches the whole tree in one call — the Windows
equivalent of kill(-pgid, ...) on the existing Unix path.

Two details that matter:

- The job must be assigned to the stdio-path process right after
  cmd.Start() succeeds, BEFORE the MCP initialize() handshake — not
  after a successful handshake. Assigning only on success misses every
  leak caused by a restart/disconnect interrupting an in-progress
  connect attempt, which is the dominant real-world case (a slow npx
  download, bulk server-add contention, a timeout mid-handshake).

- golang.org/x/sys/windows (as vendored here) does not export
  Set/QueryInformationJobObject, only CreateJobObject/
  AssignProcessToJobObject/TerminateJobObject. winjob.go binds
  SetInformationJobObject directly via NewLazySystemDLL rather than
  adding a dependency.

Same fix applied to the separate launcher package (HTTP/SSE
launcher-managed servers), which had the identical gap and an explicit
TODO comment pointing at this file.

Verified:
- Standalone reproduction: spawn cmd.exe -> ping.exe (grandchild),
  assign to job, Close -> grandchild confirmed killed.
- 6 sequential real restarts of a stdio server -> 0 leaked processes
  (previously leaked every time).
- go build/vet clean for windows/amd64; existing
  internal/upstream/launcher test suite passes unmodified.

Known remaining gap, out of scope for this patch: firing many restarts
of the same server at the exact same instant (an artificial stress
case, not normal operation) can still leak a few processes — that's a
separate race on the shared processGroupID field across concurrent
Connect/Disconnect calls on one Client, not Windows-specific, and not
addressed here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant