fix(windows): clean up child processes via Job Objects - #1234
Open
LocoLoboZ wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
internal/upstream/core/process_windows.goandinternal/upstream/launcher/launcher_windows.gowere both TODO stubs.killProcessGroupdid nothing, andterminateProcess/killProcessonly 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.exeprocesses 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(newinternal/winjobpackage). Job membership is inherited by anything the process spawns afterwards, so closing the job reaches the whole tree in one call — the Windows equivalent ofkill(-pgid, ...)on the existing Unix path.Two details that mattered in testing:
cmd.Start()succeeds, before the MCPinitialize()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 slownpxdownload, bulk server-add contention, a timeout mid-handshake). This was the actual cause of most of the 413 leaked processes observed.golang.org/x/sys/windows(as vendored here, v0.47.0) does not exportSet/QueryInformationJobObject, onlyCreateJobObject/AssignProcessToJobObject/TerminateJobObject.winjob.gobindsSetInformationJobObjectdirectly viaNewLazySystemDLLrather than adding a new dependency for one call.Same fix applied to the separate
launcherpackage (HTTP/SSE launcher-managed servers withCommandset), 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
cmd.exe→ping.exe(grandchild), assign to job,Close()→ grandchild confirmed killed.mcpproxy upstream restart <name>, few seconds apart) → 0 leaked processes, where every restart leaked one before this fix.go build/go vetclean forwindows/amd64.internal/upstream/launchertest 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
processGroupIDfield across concurrentConnect/Disconnectcalls on oneClient, is not Windows-specific, and is not addressed here.