Skip to content

Commit e9562a0

Browse files
Dumbrisclaude
andauthored
fix(windows): release the Job Object on every disconnect and Stop path (follow-up to #1234) (#1260)
* fix(windows): release the Job Object on every disconnect and Stop path Follow-up to #1234 (Windows Job Objects for child-process cleanup). The Job was only ever closed on connect/init failure and at mcpproxy exit; the two normal paths never reached it: - core Disconnect: mcp-go's Stdio.Close() kills only the immediate cmd.exe and always returns under mcpClientCloseTimeout, so the "graceful close failed" force-kill step (the only caller of killProcessGroup) was unreachable on Windows. Grandchildren that ignore stdin EOF survived every restart, and the Job handle + the windowsJobs entry leaked per disconnect. Add a releaseProcessGroup platform hook (Unix no-op; Windows pops the entry and closes the Job) and call it unconditionally for non-Docker stdio disconnects. Close any stale entry on PID-reuse overwrite instead of orphaning it. - launcher Stop: reap() waits for the log pumps before closing the Job, but a grandchild holding the inherited pipe handles never lets the pumps reach EOF once cmd.exe alone is killed, so Stop() hung until the caller's ctx expired and the Job was never closed. Route stopLocked's terminate/kill steps through per-platform handle methods that close the Job on Windows (Unix unchanged: same process-group signals). - winjob: use x/sys/windows' exported SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation and JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE instead of hand-rolled structs and a LazyDLL (also fixes the latent 4-byte layout mismatch on 386/arm and the gofmt nit); guard Close/Assign with a mutex now that Stop and reap can both close the job; drop the dead IsProcessAlive/isProcessGroupAlive helpers. Tests (windows-tagged, run on main's windows-latest job): cmd.exe -> ping.exe trees where ping.exe inherits the stdout pipe, so pipe EOF is the proof that the grandchild died. Plus a platform-neutral releaseProcessGroup no-op test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(windows): key Job release on the owning cmd, not just the PID Review round 1 (opencode/astra) on the follow-up: Disconnect captures pgid, mcp-go's Close() reaps cmd.exe, and only then releaseProcessGroup runs — a concurrently connecting server can get the same PID in that window, so a PID-only lookup would close the NEW server's Job. Store the *exec.Cmd with each registry entry and release only a matching owner; the takeover already closed the stale Job. Also gate the test process trees on stdin (set /p) so the Job is attached before the grandchild exists, and wait for ping output before killing cmd.exe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(windows): make launcher kill() a Job-only no-op when a Job exists Review round 1 (chunk 3): after terminate() closed the Job the child is dead but may still be unreaped while the pumps drain; falling through to Process.Kill() there produced a misleading 'kill failed' error. Mirror terminate(): return the Job close result, Process.Kill only on the no-Job fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(windows): carry the owning cmd into killProcessGroup too Review round 2 (chunk 2): Disconnect's force-kill step runs after a Close timeout, and the leaked Close goroutine can still reap the child before killProcessGroup pops the PID slot — same identity gap as the release path. Thread the exec.Cmd through killProcessGroup (Unix ignores it), only take a Job registered for that cmd, skip the kill entirely when the slot belongs to a newer connection, and make the no-Job fallback handle-based (cmd.Process.Kill, ErrProcessDone benign) instead of os.FindProcess(pid). Regression test covers both paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(windows): make the PID-reuse regression observable with a live process Review round 3 (chunk 2): with an unstarted oldCmd and a fake PID a forbidden os.FindProcess fallback returned nil silently, so the test could not fail. Use a live test-owned ping.exe as the new owner (its stdout pipe reaching EOF == it died) so an old-owner release or force-kill that reaches it by PID is caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 527097b commit e9562a0

14 files changed

Lines changed: 641 additions & 124 deletions

‎internal/upstream/core/connection.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ func (c *Client) Connect(ctx context.Context) error {
336336
zap.String("server", c.config.Name),
337337
zap.Int("pgid", c.processGroupID))
338338

339-
if err := killProcessGroup(c.processGroupID, c.logger, c.config.Name); err != nil {
339+
if err := killProcessGroup(c.processGroupID, c.processCmd, c.logger, c.config.Name); err != nil {
340340
c.logger.Error("Failed to clean up process group after connection failure",
341341
zap.String("server", c.config.Name),
342342
zap.Int("pgid", c.processGroupID),

‎internal/upstream/core/connection_lifecycle.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ func (c *Client) DisconnectWithContext(_ context.Context) error {
348348
zap.String("server", serverName),
349349
zap.Int("pgid", pgid))
350350

351-
if err := killProcessGroup(pgid, c.logger, serverName); err != nil {
351+
if err := killProcessGroup(pgid, processCmd, c.logger, serverName); err != nil {
352352
c.logger.Error("Failed to kill process group",
353353
zap.String("server", serverName),
354354
zap.Int("pgid", pgid),
@@ -365,6 +365,18 @@ func (c *Client) DisconnectWithContext(_ context.Context) error {
365365
}
366366
}
367367

368+
// Step 5b: Release the platform process-group resource regardless of
369+
// how the close went. No-op on Unix. On Windows this terminates the
370+
// Job Object (reaching grandchildren mcp-go's Close never touches —
371+
// it only kills the immediate cmd.exe) and drops its registry entry;
372+
// without it, a graceful close leaked the Job handle and left
373+
// EOF-ignoring node.exe/python.exe trees alive until mcpproxy exited.
374+
// processCmd (captured in Step 1) identifies OUR process, so a PID
375+
// that Windows has already handed to a newer connection is left alone.
376+
if !isDocker && pgid > 0 {
377+
releaseProcessGroup(pgid, processCmd, c.logger, serverName)
378+
}
379+
368380
// Step 6: Stop any locally-launched HTTP/SSE upstream. We do this
369381
// AFTER closing the MCP client — the child should see the network
370382
// transport go away first, giving it a clean shutdown signal before

‎internal/upstream/core/connection_stdio.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ func (c *Client) connectStdio(ctx context.Context) error {
323323
zap.String("server", c.config.Name),
324324
zap.Int("pgid", c.processGroupID))
325325

326-
if err := killProcessGroup(c.processGroupID, c.logger, c.config.Name); err != nil {
326+
if err := killProcessGroup(c.processGroupID, c.processCmd, c.logger, c.config.Name); err != nil {
327327
c.logger.Error("Failed to clean up process group after initialization failure",
328328
zap.String("server", c.config.Name),
329329
zap.Int("pgid", c.processGroupID),
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package core
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/assert"
8+
"go.uber.org/zap"
9+
)
10+
11+
// releaseProcessGroup is the platform hook DisconnectWithContext calls on
12+
// every non-Docker stdio disconnect (after the graceful MCP close), so it
13+
// must be cheap and safe for the "nothing to release" cases on every OS:
14+
// the zero/negative sentinels and a PID that never had a Job registered.
15+
func TestReleaseProcessGroup_NothingToRelease(t *testing.T) {
16+
logger := zap.NewNop()
17+
for _, pgid := range []int{0, -1, 999999999} {
18+
start := time.Now()
19+
releaseProcessGroup(pgid, nil, logger, "test-server")
20+
assert.Less(t, time.Since(start), 100*time.Millisecond,
21+
"releaseProcessGroup(%d) must return immediately when there is nothing to release", pgid)
22+
}
23+
}

‎internal/upstream/core/process_unix.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,10 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za
5757
}
5858

5959
// killProcessGroup terminates an entire process group on Unix systems
60-
// This is the proper way to clean up child processes and prevent zombies
61-
func killProcessGroup(pgid int, logger *zap.Logger, serverName string) error {
60+
// This is the proper way to clean up child processes and prevent zombies.
61+
// The cmd argument is the owning process's exec.Cmd; it only matters on
62+
// Windows (PID-reuse identity check) and is ignored here.
63+
func killProcessGroup(pgid int, _ *exec.Cmd, logger *zap.Logger, serverName string) error {
6264
if pgid <= 0 {
6365
return nil
6466
}
@@ -155,3 +157,11 @@ func extractProcessGroupID(cmd *exec.Cmd, logger *zap.Logger, serverName string)
155157

156158
return pgid
157159
}
160+
161+
// releaseProcessGroup is the platform hook DisconnectWithContext calls after
162+
// the graceful MCP close on every non-Docker stdio disconnect. On Unix there
163+
// is nothing to release: process groups are a kernel-side identifier, not a
164+
// handle we hold, so this is a no-op. See process_windows.go for the
165+
// platform that needs it (the Job Object handle + registry entry, keyed by
166+
// PID and disambiguated by the owning cmd).
167+
func releaseProcessGroup(_ int, _ *exec.Cmd, _ *zap.Logger, _ string) {}

‎internal/upstream/core/process_windows.go‎

Lines changed: 135 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package core
44

55
import (
66
"context"
7+
"errors"
78
"os"
89
"os/exec"
910
"sync"
@@ -19,13 +20,21 @@ type ProcessGroup struct {
1920
logger *zap.Logger
2021
}
2122

23+
// windowsJob is one registered Job Object plus the identity of the
24+
// exec.Cmd it was created for. The cmd pointer is what lets a delayed
25+
// release tell "my process" from "a newer process that reused my PID".
26+
type windowsJob struct {
27+
job *winjob.Job
28+
cmd *exec.Cmd
29+
}
30+
2231
// windowsJobs maps the "process group ID" mcpproxy uses on Windows (the
2332
// immediate child's PID — see extractProcessGroupID) to the Job Object that
2433
// PID was assigned to. killProcessGroup only receives that int, so this is
2534
// how it finds the Job to terminate.
2635
var (
2736
windowsJobsMu sync.Mutex
28-
windowsJobs = map[int]*winjob.Job{}
37+
windowsJobs = map[int]windowsJob{}
2938
)
3039

3140
// createProcessGroupCommandFunc creates a custom CommandFunc for Windows systems.
@@ -63,30 +72,34 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za
6372
// node.exe/python.exe/qmcp.exe processes accumulated from normal use in
6473
// under an hour on this host).
6574
//
75+
// cmd is the owning exec.Cmd (see releaseProcessGroup for why identity
76+
// matters: the PID may already belong to a newer connection). Only a Job
77+
// registered for this cmd is terminated. If the slot is held by someone
78+
// else, this connection's process is already gone and nothing is killed.
79+
//
6680
// Falls back to a plain single-process kill if no job was ever registered
6781
// for this PID (e.g. job-object setup itself failed) — degrades to the old
68-
// behaviour rather than doing nothing.
69-
func killProcessGroup(pgid int, logger *zap.Logger, serverName string) error {
82+
// behaviour rather than doing nothing. The fallback goes through cmd's own
83+
// process handle when available (PID-reuse-proof; Windows keeps a PID
84+
// reserved while a handle to it is open) and only resolves by PID for the
85+
// legacy nil-cmd case.
86+
func killProcessGroup(pgid int, cmd *exec.Cmd, logger *zap.Logger, serverName string) error {
7087
if pgid <= 0 {
7188
return nil
7289
}
7390

74-
windowsJobsMu.Lock()
75-
job, ok := windowsJobs[pgid]
76-
if ok {
77-
delete(windowsJobs, pgid)
78-
}
79-
windowsJobsMu.Unlock()
80-
81-
if !ok || job == nil {
91+
job := takeWindowsJob(pgid, cmd)
92+
if job == nil {
93+
if cmd != nil && windowsJobOwnedByOther(pgid, cmd) {
94+
logger.Debug("PID now belongs to a newer connection's Job; nothing to kill for this one",
95+
zap.String("server", serverName),
96+
zap.Int("pid", pgid))
97+
return nil
98+
}
8299
logger.Warn("No Windows Job Object tracked for this process; falling back to killing only the immediate process (any grandchildren will leak)",
83100
zap.String("server", serverName),
84101
zap.Int("pid", pgid))
85-
proc, err := os.FindProcess(pgid)
86-
if err != nil {
87-
return nil // already gone
88-
}
89-
return proc.Kill()
102+
return killImmediateProcess(pgid, cmd)
90103
}
91104

92105
logger.Info("Terminating process tree via Windows Job Object",
@@ -136,9 +149,7 @@ func extractProcessGroupID(cmd *exec.Cmd, logger *zap.Logger, serverName string)
136149
return pid
137150
}
138151

139-
windowsJobsMu.Lock()
140-
windowsJobs[pid] = job
141-
windowsJobsMu.Unlock()
152+
registerWindowsJob(pid, cmd, job, logger, serverName)
142153

143154
logger.Debug("Process group ID extracted and assigned to Windows Job Object",
144155
zap.String("server", serverName),
@@ -147,10 +158,111 @@ func extractProcessGroupID(cmd *exec.Cmd, logger *zap.Logger, serverName string)
147158
return pid
148159
}
149160

150-
// isProcessGroupAlive checks if the process is still running on Windows.
151-
func isProcessGroupAlive(pgid int) bool {
161+
// releaseProcessGroup is the platform hook DisconnectWithContext calls on
162+
// EVERY non-Docker stdio disconnect, whether or not the graceful MCP close
163+
// succeeded. On Unix it is a no-op. Here it terminates whatever is still
164+
// in the Job and drops the windowsJobs entry.
165+
//
166+
// Why it must run on the graceful path too: mcp-go's Stdio.Close() only
167+
// ever kills the immediate child (cmd.exe — every Windows stdio command
168+
// is shell-wrapped) and always returns within ~8s, under the 10s
169+
// mcpClientCloseTimeout, so Disconnect's force-kill step (which is where
170+
// killProcessGroup lives) is effectively unreachable on Windows. Without
171+
// this hook the grandchildren a stdin-EOF-ignoring node.exe/python.exe
172+
// left behind survived every restart, and the Job handle + map entry
173+
// leaked per disconnect until mcpproxy exited (#1234 follow-up, F1).
174+
//
175+
// cmd identifies the connection's own process: by the time this runs the
176+
// graceful close may already have reaped cmd.exe, and Windows reuses PIDs
177+
// eagerly, so a concurrently connecting server can have registered a NEW
178+
// Job under the same PID. Only the entry registered for this exact cmd is
179+
// released; if the slot now belongs to someone else, ours was already
180+
// closed by registerWindowsJob when it took the slot over. A nil cmd
181+
// disables the identity check (not used by production callers).
182+
func releaseProcessGroup(pgid int, cmd *exec.Cmd, logger *zap.Logger, serverName string) {
152183
if pgid <= 0 {
153-
return false
184+
return
185+
}
186+
job := takeWindowsJob(pgid, cmd)
187+
if job == nil {
188+
// Already killed via killProcessGroup, job setup failed at connect
189+
// time (extractProcessGroupID logged a Warn then), or the PID slot
190+
// was taken over by a newer process (see above).
191+
return
192+
}
193+
if err := job.Close(); err != nil {
194+
logger.Warn("Failed to close Windows Job Object on disconnect",
195+
zap.String("server", serverName),
196+
zap.Int("pid", pgid),
197+
zap.Error(err))
198+
return
199+
}
200+
logger.Debug("Released Windows Job Object; any surviving grandchildren terminated",
201+
zap.String("server", serverName),
202+
zap.Int("pid", pgid))
203+
}
204+
205+
// takeWindowsJob removes and returns the Job registered for pid, or nil.
206+
// When cmd is non-nil the entry is only taken if it was registered for
207+
// that same cmd. Popping under the lock gives the caller sole ownership
208+
// of the Close.
209+
func takeWindowsJob(pid int, cmd *exec.Cmd) *winjob.Job {
210+
windowsJobsMu.Lock()
211+
defer windowsJobsMu.Unlock()
212+
entry, ok := windowsJobs[pid]
213+
if !ok || (cmd != nil && entry.cmd != cmd) {
214+
return nil
215+
}
216+
delete(windowsJobs, pid)
217+
return entry.job
218+
}
219+
220+
// windowsJobOwnedByOther reports whether pid has a registered Job that
221+
// belongs to a different cmd — i.e. Windows reused the PID for a newer
222+
// connection after this one's process exited.
223+
func windowsJobOwnedByOther(pid int, cmd *exec.Cmd) bool {
224+
windowsJobsMu.Lock()
225+
defer windowsJobsMu.Unlock()
226+
entry, ok := windowsJobs[pid]
227+
return ok && entry.cmd != cmd
228+
}
229+
230+
// killImmediateProcess is the no-Job fallback: kill just the one process.
231+
// With a cmd we use its own process handle, which cannot alias a reused
232+
// PID; an already-exited child is not an error (ErrProcessDone).
233+
func killImmediateProcess(pid int, cmd *exec.Cmd) error {
234+
if cmd != nil && cmd.Process != nil {
235+
if err := cmd.Process.Kill(); err != nil && !errors.Is(err, os.ErrProcessDone) {
236+
return err
237+
}
238+
return nil
239+
}
240+
proc, err := os.FindProcess(pid)
241+
if err != nil {
242+
return nil // already gone
243+
}
244+
if err := proc.Kill(); err != nil && !errors.Is(err, os.ErrProcessDone) {
245+
return err
246+
}
247+
return nil
248+
}
249+
250+
// registerWindowsJob records the Job for pid. If an entry already exists —
251+
// Windows reused a PID whose earlier Job has not been released yet — the
252+
// old Job is closed first rather than silently dropped with its handle
253+
// (and any straggler still inside it) left open forever. The previous
254+
// owner's later releaseProcessGroup then finds the slot belongs to a
255+
// different cmd and does nothing.
256+
func registerWindowsJob(pid int, cmd *exec.Cmd, job *winjob.Job, logger *zap.Logger, serverName string) {
257+
windowsJobsMu.Lock()
258+
prev, hadPrev := windowsJobs[pid]
259+
windowsJobs[pid] = windowsJob{job: job, cmd: cmd}
260+
windowsJobsMu.Unlock()
261+
262+
if hadPrev {
263+
logger.Warn("Windows Job Object already registered for reused PID; closing the stale one",
264+
zap.String("server", serverName),
265+
zap.Int("pid", pid))
266+
_ = prev.job.Close()
154267
}
155-
return winjob.IsProcessAlive(pgid)
156268
}

0 commit comments

Comments
 (0)