75c7116 fix(mcp): always kill a reserved process group; treat /proc scan as advisory · zut

M packages/agent/mcp/proc_unix.go +19 -9


@@ -42,9 +42,9 @@ type processGuard struct {
42	42		leader int // pid of the group leader (== original pgid)
43	43		proc *os.Process
44	44		kill func(pid int, sig syscall.Signal) error // syscall.Kill; replaced in tests
45		-	// zombie is set once the leader has exited but not been reaped. A probe
46		-	// with signal 0 cannot tell an empty group from one whose only member is
47		-	// that zombie, so liveness is then read from the process table.
	45	+	// zombie is set once the leader has exited but not been reaped. The pgid
	46	+	// is then reserved, so cleanup always ends with a group SIGKILL; the
	47	+	// process table is read only to end the SIGTERM grace period early.
48	48		zombie bool
49	49
50	50		once sync.Once

@@ -115,12 +115,10 @@ func (g *processGuard) alive() bool {
115	115		return false
116	116		}
117	117		if g.zombie {
118		-	if live, known := liveGroupMembers(g.pgid); known {
119		-	if !live {
120		-	g.pgid = 0
121		-	}
122		-	return live
123		-	}
	118	+	// A signal-0 probe would see the zombie leader. The /proc scan is
	119	+	// advisory only (not an atomic snapshot), so it never clears pgid.
	120	+	live, known := liveGroupMembers(g.pgid)
	121	+	return live \|\| !known
124	122		}
125	123		err := g.kill(-g.pgid, 0)
126	124		if errors.Is(err, syscall.ESRCH) {

@@ -144,6 +142,18 @@ func (g *processGuard) cleanup() {
144	142		g.proc = nil
145	143		g.mu.Unlock()
146	144		}()
	145	+	g.mu.Lock()
	146	+	reserved := g.zombie
	147	+	g.mu.Unlock()
	148	+	if reserved {
	149	+	// The unreaped leader keeps the pgid ours: give members a grace
	150	+	// period, then kill the group unconditionally. The kernel delivers
	151	+	// a group signal to every member, including ones forked meanwhile.
	152	+	g.terminate(false)
	153	+	g.waitGone(300 * time.Millisecond)
	154	+	g.terminate(true)
	155	+	return
	156	+	}
147	157		if !g.alive() {
148	158		return
149	159		}

 	leader int // pid of the group leader (== original pgid)
 	proc   *os.Process
 	kill   func(pid int, sig syscall.Signal) error // syscall.Kill; replaced in tests
-	// zombie is set once the leader has exited but not been reaped. A probe
+	// zombie is set once the leader has exited but not been reaped. The pgid
-	// with signal 0 cannot tell an empty group from one whose only member is
+	// is then reserved, so cleanup always ends with a group SIGKILL; the
-	// that zombie, so liveness is then read from the process table.
+	// process table is read only to end the SIGTERM grace period early.
 	zombie bool
 	once sync.Once
 		return false
 	}
 	if g.zombie {
-		if live, known := liveGroupMembers(g.pgid); known {
+		// A signal-0 probe would see the zombie leader. The /proc scan is
-			if !live {
+		// advisory only (not an atomic snapshot), so it never clears pgid.
-				g.pgid = 0
+		live, known := liveGroupMembers(g.pgid)
-			}
+		return live || !known
-			return live
-		}
 	}
 	err := g.kill(-g.pgid, 0)
 	if errors.Is(err, syscall.ESRCH) {
 			g.proc = nil
 			g.mu.Unlock()
 		}()
+		g.mu.Lock()
+		reserved := g.zombie
+		g.mu.Unlock()
+		if reserved {
+			// The unreaped leader keeps the pgid ours: give members a grace
+			// period, then kill the group unconditionally. The kernel delivers
+			// a group signal to every member, including ones forked meanwhile.
+			g.terminate(false)
+			g.waitGone(300 * time.Millisecond)
+			g.terminate(true)
+			return
+		}
 		if !g.alive() {
 			return
 		}

M packages/agent/mcp/proc_wait_linux.go +6 -2


@@ -26,7 +26,8 @@ func waitLeaderExited(pid int) bool {
26	26		}
27	27
28	28		// liveGroupMembers reports whether any non-zombie process is in process group
29		-	// pgid, reading /proc. known is false when /proc cannot be read.
	29	+	// pgid, reading /proc. known is false when /proc or a member's stat cannot be
	30	+	// read. The scan is not atomic, so callers use it only as a hint.
30	31		func liveGroupMembers(pgid int) (live, known bool) {
31	32		entries, err := os.ReadDir("/proc")
32	33		if err != nil {

@@ -39,9 +40,12 @@ func liveGroupMembers(pgid int) (live, known bool) {
39	40		}
40	41		sawProc = true
41	42		raw, err := os.ReadFile("/proc/" + e.Name() + "/stat")
42		-	if err != nil {
	43	+	if errors.Is(err, os.ErrNotExist) \|\| errors.Is(err, unix.ESRCH) {
43	44		continue // exited meanwhile
44	45		}
	46	+	if err != nil {
	47	+	return false, false
	48	+	}
45	49		state, group, ok := parseProcStat(string(raw))
46	50		if !ok \|\| group != pgid \|\| state == 'Z' \|\| state == 'X' {
47	51		continue

 }
 // liveGroupMembers reports whether any non-zombie process is in process group
-// pgid, reading /proc. known is false when /proc cannot be read.
+// pgid, reading /proc. known is false when /proc or a member's stat cannot be
+// read. The scan is not atomic, so callers use it only as a hint.
 func liveGroupMembers(pgid int) (live, known bool) {
 	entries, err := os.ReadDir("/proc")
 	if err != nil {
 		}
 		sawProc = true
 		raw, err := os.ReadFile("/proc/" + e.Name() + "/stat")
-		if err != nil {
+		if errors.Is(err, os.ErrNotExist) || errors.Is(err, unix.ESRCH) {
 			continue // exited meanwhile
 		}
+		if err != nil {
+			return false, false
+		}
 		state, group, ok := parseProcStat(string(raw))
 		if !ok || group != pgid || state == 'Z' || state == 'X' {
 			continue

M packages/agent/mcp/proc_wait_linux_test.go +29 -0


@@ -5,6 +5,8 @@ package mcp
5	5		import (
6	6		"os"
7	7		"strconv"
	8	+	"sync"
	9	+	"syscall"
8	10		"testing"
9	11		"time"
10	12		)

@@ -109,3 +111,30 @@ func TestStdioGroupCleanedBeforeLeaderIsReaped(t *testing.T) {
109	111		return os.IsNotExist(err)
110	112		})
111	113		}
	114	+
	115	+	// While the leader is an unreaped zombie the pgid is reserved, so cleanup must
	116	+	// end with a group SIGKILL even when the (non-atomic) /proc scan sees no live
	117	+	// member: a descendant may have forked between the scan and its end.
	118	+	func TestReservedGroupIsKilledDespiteEmptyScan(t *testing.T) {
	119	+	var mu sync.Mutex
	120	+	var sigs []syscall.Signal
	121	+	g := &processGuard{pgid: 1 << 30, leader: 1 << 30, zombie: true,
	122	+	kill: func(pid int, sig syscall.Signal) error {
	123	+	mu.Lock()
	124	+	defer mu.Unlock()
	125	+	if pid != -(1 << 30) {
	126	+	t.Errorf("signal to %d, want group", pid)
	127	+	}
	128	+	sigs = append(sigs, sig)
	129	+	return nil
	130	+	}}
	131	+	g.cleanup()
	132	+	mu.Lock()
	133	+	defer mu.Unlock()
	134	+	if len(sigs) == 0 \|\| sigs[len(sigs)-1] != syscall.SIGKILL {
	135	+	t.Fatalf("signals = %v, want a final group SIGKILL", sigs)
	136	+	}
	137	+	if g.pgid != 0 {
	138	+	t.Error("pgid retained after cleanup")
	139	+	}
	140	+	}


@@ -5,6 +5,8 @@ package mcp
5	import (	5		import (
6	"os"	6		"os"
7	"strconv"	7		"strconv"
		8	+	"sync"
		9	+	"syscall"
8	"testing"	10		"testing"
9	"time"	11		"time"
10	)	12		)

@@ -109,3 +111,30 @@ func TestStdioGroupCleanedBeforeLeaderIsReaped(t *testing.T) {
109	return os.IsNotExist(err)	111		return os.IsNotExist(err)
110	})	112		})
111	}	113		}
		114	+
		115	+	// While the leader is an unreaped zombie the pgid is reserved, so cleanup must
		116	+	// end with a group SIGKILL even when the (non-atomic) /proc scan sees no live
		117	+	// member: a descendant may have forked between the scan and its end.
		118	+	func TestReservedGroupIsKilledDespiteEmptyScan(t *testing.T) {
		119	+	var mu sync.Mutex
		120	+	var sigs []syscall.Signal
		121	+	g := &processGuard{pgid: 1 << 30, leader: 1 << 30, zombie: true,
		122	+	kill: func(pid int, sig syscall.Signal) error {
		123	+	mu.Lock()
		124	+	defer mu.Unlock()
		125	+	if pid != -(1 << 30) {
		126	+	t.Errorf("signal to %d, want group", pid)
		127	+	}
		128	+	sigs = append(sigs, sig)
		129	+	return nil
		130	+	}}
		131	+	g.cleanup()
		132	+	mu.Lock()
		133	+	defer mu.Unlock()
		134	+	if len(sigs) == 0 \|\| sigs[len(sigs)-1] != syscall.SIGKILL {
		135	+	t.Fatalf("signals = %v, want a final group SIGKILL", sigs)
		136	+	}
		137	+	if g.pgid != 0 {
		138	+	t.Error("pgid retained after cleanup")
		139	+	}
		140	+	}