Skip to content

Commit 8686ec5

Browse files
rremerRoyce Remer
authored andcommitted
fix(process): reap entire process group on cmd.Cancel, not just the parent
Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Royce Remer <royceremer@gmail.com>
1 parent 3005efa commit 8686ec5

4 files changed

Lines changed: 89 additions & 1 deletion

File tree

modules/git/gitcmd/command.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ func (c *Command) Start(ctx context.Context) (retErr error) {
452452
// * if we don't close the parent pipes here, the children process won't exit.
453453
//
454454
// There is no such problem on POSIX, while it won't make things worse by closing the parent pipes also on POSIX.
455-
err := c.cmd.Process.Kill()
455+
err := process.KillCmd(c.cmd)
456456
c.closePipeFiles(c.parentPipeFiles)
457457
return err
458458
}

modules/process/manager_unix.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,10 @@ import (
1414
func SetSysProcAttribute(cmd *exec.Cmd) {
1515
// When Gitea runs SubProcessA -> SubProcessB and SubProcessA gets killed by context timeout, use setpgid to make sure the sub processes can be reaped instead of leaving defunct(zombie) processes.
1616
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
17+
cmd.Cancel = func() error { return KillCmd(cmd) }
18+
}
19+
20+
// KillCmd kills the process group of cmd, ensuring grandchildren are reaped.
21+
func KillCmd(cmd *exec.Cmd) error {
22+
return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
1723
}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
//go:build !windows
5+
6+
package process
7+
8+
import (
9+
"bufio"
10+
"context"
11+
"os"
12+
"os/exec"
13+
"strconv"
14+
"strings"
15+
"syscall"
16+
"testing"
17+
"time"
18+
19+
"github.com/stretchr/testify/assert"
20+
"github.com/stretchr/testify/require"
21+
)
22+
23+
// TestSetSysProcAttributeKillsProcessGroup verifies that cancelling a context
24+
// kills not only the direct child but also grandchildren it spawned.
25+
//
26+
// This mirrors the git upload-pack -> git pack-objects relationship during a
27+
// git clone: if an HTTP client disconnects mid-transfer, all subprocesses must
28+
// die. exec.CommandContext sends SIGKILL to the direct PID only; because
29+
// SetSysProcAttribute sets Setpgid:true the grandchild is in the same process
30+
// group but is NOT killed, leaking it until it finishes on its own.
31+
func TestSetSysProcAttributeKillsProcessGroup(t *testing.T) {
32+
ctx, cancel := context.WithCancel(t.Context())
33+
defer cancel()
34+
35+
// Spawn a shell that itself spawns a long-lived background process and
36+
// prints its PID — mimicking git upload-pack spawning git pack-objects.
37+
r, w, err := os.Pipe()
38+
require.NoError(t, err)
39+
40+
cmd := exec.CommandContext(ctx, "sh", "-c", "sleep 600 & echo $!; wait")
41+
cmd.Stdout = w
42+
SetSysProcAttribute(cmd)
43+
require.NoError(t, cmd.Start())
44+
w.Close() // parent keeps only the read end
45+
46+
// Always kill the process group on test exit so a failing assertion does
47+
// not leak the grandchild (sleep 600) as an orphan. Setpgid:true makes
48+
// the shell's PGID equal its own PID, so -cmd.Process.Pid targets the group.
49+
t.Cleanup(func() {
50+
_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
51+
_ = cmd.Wait()
52+
})
53+
54+
// Block until the shell prints the grandchild PID.
55+
scanner := bufio.NewScanner(r)
56+
require.True(t, scanner.Scan(), "expected grandchild PID on stdout")
57+
grandchildPID, err := strconv.Atoi(strings.TrimSpace(scanner.Text()))
58+
require.NoError(t, err)
59+
r.Close()
60+
61+
// Sanity: grandchild must be alive before we cancel.
62+
grandchild, err := os.FindProcess(grandchildPID)
63+
require.NoError(t, err)
64+
require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel")
65+
66+
// Cancel the context — exec.CommandContext should propagate the kill to the
67+
// whole process group, not just the direct child (the shell).
68+
cancel()
69+
_ = cmd.Wait()
70+
71+
// Poll until the grandchild is gone or the deadline is exceeded.
72+
// Signal(0) returns ESRCH (non-nil) once the process no longer exists.
73+
assert.Eventually(t, func() bool {
74+
return grandchild.Signal(syscall.Signal(0)) != nil
75+
}, 5*time.Second, 10*time.Millisecond,
76+
"grandchild process %d is still running after context cancel — process group was not killed (git pack-objects leak)", grandchildPID)
77+
}

modules/process/manager_windows.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,8 @@ import (
1313
func SetSysProcAttribute(cmd *exec.Cmd) {
1414
// Do nothing
1515
}
16+
17+
// KillCmd kills the process; on Windows there are no process groups.
18+
func KillCmd(cmd *exec.Cmd) error {
19+
return cmd.Process.Kill()
20+
}

0 commit comments

Comments
 (0)