From 8686ec54e43ec7a0a95ab10528dc4381dedcc911 Mon Sep 17 00:00:00 2001 From: Royce Remer Date: Wed, 26 Aug 2026 10:28:33 -0700 Subject: [PATCH 1/8] fix(process): reap entire process group on cmd.Cancel, not just the parent Assisted-by: Claude Sonnet 4.6 (1M context) Signed-off-by: Royce Remer --- modules/git/gitcmd/command.go | 2 +- modules/process/manager_unix.go | 6 +++ modules/process/manager_unix_test.go | 77 ++++++++++++++++++++++++++++ modules/process/manager_windows.go | 5 ++ 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 modules/process/manager_unix_test.go diff --git a/modules/git/gitcmd/command.go b/modules/git/gitcmd/command.go index c55b04cd31854..f9d7bf9f7bed6 100644 --- a/modules/git/gitcmd/command.go +++ b/modules/git/gitcmd/command.go @@ -452,7 +452,7 @@ func (c *Command) Start(ctx context.Context) (retErr error) { // * if we don't close the parent pipes here, the children process won't exit. // // There is no such problem on POSIX, while it won't make things worse by closing the parent pipes also on POSIX. - err := c.cmd.Process.Kill() + err := process.KillCmd(c.cmd) c.closePipeFiles(c.parentPipeFiles) return err } diff --git a/modules/process/manager_unix.go b/modules/process/manager_unix.go index c5be906b35e6d..ff1e8edf4dbe3 100644 --- a/modules/process/manager_unix.go +++ b/modules/process/manager_unix.go @@ -14,4 +14,10 @@ import ( func SetSysProcAttribute(cmd *exec.Cmd) { // 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. cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { return KillCmd(cmd) } +} + +// KillCmd kills the process group of cmd, ensuring grandchildren are reaped. +func KillCmd(cmd *exec.Cmd) error { + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } diff --git a/modules/process/manager_unix_test.go b/modules/process/manager_unix_test.go new file mode 100644 index 0000000000000..fafa5cb5d0789 --- /dev/null +++ b/modules/process/manager_unix_test.go @@ -0,0 +1,77 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !windows + +package process + +import ( + "bufio" + "context" + "os" + "os/exec" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSetSysProcAttributeKillsProcessGroup verifies that cancelling a context +// kills not only the direct child but also grandchildren it spawned. +// +// This mirrors the git upload-pack -> git pack-objects relationship during a +// git clone: if an HTTP client disconnects mid-transfer, all subprocesses must +// die. exec.CommandContext sends SIGKILL to the direct PID only; because +// SetSysProcAttribute sets Setpgid:true the grandchild is in the same process +// group but is NOT killed, leaking it until it finishes on its own. +func TestSetSysProcAttributeKillsProcessGroup(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + // Spawn a shell that itself spawns a long-lived background process and + // prints its PID — mimicking git upload-pack spawning git pack-objects. + r, w, err := os.Pipe() + require.NoError(t, err) + + cmd := exec.CommandContext(ctx, "sh", "-c", "sleep 600 & echo $!; wait") + cmd.Stdout = w + SetSysProcAttribute(cmd) + require.NoError(t, cmd.Start()) + w.Close() // parent keeps only the read end + + // Always kill the process group on test exit so a failing assertion does + // not leak the grandchild (sleep 600) as an orphan. Setpgid:true makes + // the shell's PGID equal its own PID, so -cmd.Process.Pid targets the group. + t.Cleanup(func() { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Wait() + }) + + // Block until the shell prints the grandchild PID. + scanner := bufio.NewScanner(r) + require.True(t, scanner.Scan(), "expected grandchild PID on stdout") + grandchildPID, err := strconv.Atoi(strings.TrimSpace(scanner.Text())) + require.NoError(t, err) + r.Close() + + // Sanity: grandchild must be alive before we cancel. + grandchild, err := os.FindProcess(grandchildPID) + require.NoError(t, err) + require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel") + + // Cancel the context — exec.CommandContext should propagate the kill to the + // whole process group, not just the direct child (the shell). + cancel() + _ = cmd.Wait() + + // Poll until the grandchild is gone or the deadline is exceeded. + // Signal(0) returns ESRCH (non-nil) once the process no longer exists. + assert.Eventually(t, func() bool { + return grandchild.Signal(syscall.Signal(0)) != nil + }, 5*time.Second, 10*time.Millisecond, + "grandchild process %d is still running after context cancel — process group was not killed (git pack-objects leak)", grandchildPID) +} diff --git a/modules/process/manager_windows.go b/modules/process/manager_windows.go index 44a84f220315e..bcc6053a4d01f 100644 --- a/modules/process/manager_windows.go +++ b/modules/process/manager_windows.go @@ -13,3 +13,8 @@ import ( func SetSysProcAttribute(cmd *exec.Cmd) { // Do nothing } + +// KillCmd kills the process; on Windows there are no process groups. +func KillCmd(cmd *exec.Cmd) error { + return cmd.Process.Kill() +} From bc21fddeca28b009513e486d3c3a78c2c728e59a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 27 Aug 2026 18:12:08 +0800 Subject: [PATCH 2/8] refactor --- cmd/serv.go | 8 +-- modules/git/gitcmd/command.go | 21 +++---- modules/git/gpg.go | 4 +- modules/graceful/manager.go | 4 -- modules/markup/external/external.go | 4 +- modules/process/command.go | 51 +++++++++++++++++ modules/process/command_unix.go | 28 ++++++++++ modules/process/command_unix_test.go | 67 +++++++++++++++++++++++ modules/process/command_windows.go | 19 +++++++ modules/process/error.go | 25 --------- modules/process/manager.go | 3 - modules/process/manager_exec.go | 79 --------------------------- modules/process/manager_test.go | 23 -------- modules/process/manager_unix.go | 23 -------- modules/process/manager_unix_test.go | 77 -------------------------- modules/process/manager_windows.go | 20 ------- modules/ssh/ssh.go | 4 +- services/asymkey/commit.go | 4 +- services/asymkey/sign.go | 3 +- services/mailer/sender/sendmail.go | 4 +- tests/integration/gpg_ssh_git_test.go | 6 +- 21 files changed, 186 insertions(+), 291 deletions(-) create mode 100644 modules/process/command.go create mode 100644 modules/process/command_unix.go create mode 100644 modules/process/command_unix_test.go create mode 100644 modules/process/command_windows.go delete mode 100644 modules/process/error.go delete mode 100644 modules/process/manager_exec.go delete mode 100644 modules/process/manager_unix.go delete mode 100644 modules/process/manager_unix_test.go delete mode 100644 modules/process/manager_windows.go diff --git a/cmd/serv.go b/cmd/serv.go index eddbbcb3ab98a..c7b69266f4ca3 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -9,7 +9,6 @@ import ( "fmt" "net/url" "os" - "os/exec" "path/filepath" "strconv" "strings" @@ -289,7 +288,7 @@ func runServ(ctx context.Context, c *cli.Command) error { return nil } - var command *exec.Cmd + var command *process.Cmd gitBinPath := filepath.Dir(gitcmd.GitExecutable) // e.g. /usr/bin gitBinVerb := filepath.Join(gitBinPath, verb) // e.g. /usr/bin/git-upload-pack if _, err := os.Stat(gitBinVerb); err != nil { @@ -298,15 +297,14 @@ func runServ(ctx context.Context, c *cli.Command) error { verbFields := strings.SplitN(verb, "-", 2) if len(verbFields) == 2 { // use git binary with the sub-command part: "C:\...\bin\git.exe", "upload-pack", ... - command = exec.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], results.RepoStoragePath) + command = process.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], results.RepoStoragePath) } } if command == nil { // by default, use the verb (it has been checked above by allowedCommands) - command = exec.CommandContext(ctx, gitBinVerb, results.RepoStoragePath) + command = process.CommandContext(ctx, gitBinVerb, results.RepoStoragePath) } - process.SetSysProcAttribute(command) command.Dir = setting.RepoRootPath command.Stdout = os.Stdout command.Stdin = os.Stdin diff --git a/modules/git/gitcmd/command.go b/modules/git/gitcmd/command.go index f9d7bf9f7bed6..4a4eaedd23024 100644 --- a/modules/git/gitcmd/command.go +++ b/modules/git/gitcmd/command.go @@ -11,7 +11,6 @@ import ( "fmt" "io" "os" - "os/exec" "path/filepath" "strings" "time" @@ -47,7 +46,7 @@ type Command struct { // otherwise some git commands might overwrite git dir internal files by a repo file. gitDir string - cmd *exec.Cmd + cmd *process.Cmd cmdCtx context.Context cmdCancel process.CancelCauseFunc @@ -432,30 +431,24 @@ func (c *Command) Start(ctx context.Context) (retErr error) { c.cmdStartTime = time.Now() - c.cmd = exec.CommandContext(c.cmdCtx, c.prog, append(c.configArgs, c.args...)...) + c.cmd = process.CommandContext(c.cmdCtx, c.prog, append(c.configArgs, c.args...)...) if c.cmdEnv == nil { c.cmd.Env = os.Environ() } else { c.cmd.Env = c.cmdEnv } - process.SetSysProcAttribute(c.cmd) c.cmd.Env = append(c.cmd.Env, CommonGitCmdEnvs()...) c.cmd.Dir = c.gitDir c.cmd.Stdout = c.cmdStdout c.cmd.Stdin = c.cmdStdin c.cmd.Stderr = c.cmdStderr - c.cmd.Cancel = func() error { - // Golang's default cmd.Cancel only calls Process.Kill(), but here we need to close the parent pipes together: - // * for some commands like "git --batch-xxx", Windows git might have 2 processes (a wrapper and a real git process) - // * on Windows, if parent process is killed (context canceled), the children process won't be killed, and the pipe handles are still open. - // * if we don't close the parent pipes here, the children process won't exit. - // - // There is no such problem on POSIX, while it won't make things worse by closing the parent pipes also on POSIX. - err := process.KillCmd(c.cmd) + c.cmd.WithOnCancelGracefully(func() error { + // Need to close the pipes to notify all sub processes to exit. + // Especially on Windows: there is no process group, and we didn't implement process job object (like process group). c.closePipeFiles(c.parentPipeFiles) - return err - } + return nil + }) return c.cmd.Start() } diff --git a/modules/git/gpg.go b/modules/git/gpg.go index 480c1db430a1b..26b55d22e1bbd 100644 --- a/modules/git/gpg.go +++ b/modules/git/gpg.go @@ -27,7 +27,7 @@ type CommitSignSettings struct { cachedPublicKeyContent atomic.Pointer[string] } -func (css *CommitSignSettings) PublicKeyContent() (string, error) { +func (css *CommitSignSettings) PublicKeyContent(ctx context.Context) (string, error) { cached := css.cachedPublicKeyContent.Load() if cached != nil { return *cached, nil @@ -43,7 +43,7 @@ func (css *CommitSignSettings) PublicKeyContent() (string, error) { return s, nil } - content, stderr, err := process.GetManager().Exec("gpg -a --export", "gpg", "-a", "--export", css.KeyID) + content, stderr, err := process.CommandContext(ctx, "gpg", "-a", "--export", css.KeyID).OutputString() if err != nil { return "", fmt.Errorf("unable to get default signing key: %s, %s, %w", css.KeyID, stderr, err) } diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index 24d4c3f61e31e..e2595a459248d 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -11,7 +11,6 @@ import ( "gitea.dev/modules/gtprof" "gitea.dev/modules/log" - "gitea.dev/modules/process" "gitea.dev/modules/setting" ) @@ -62,9 +61,6 @@ func InitManager(ctx context.Context) { func initManager(ctx context.Context) { initOnce.Do(func() { manager = newGracefulManager(ctx) - - // Set the process default context to the HammerContext - process.DefaultContext = manager.HammerContext() }) } diff --git a/modules/markup/external/external.go b/modules/markup/external/external.go index 0ff010c6ad533..65efbdef3009d 100644 --- a/modules/markup/external/external.go +++ b/modules/markup/external/external.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "os" - "os/exec" "strings" "gitea.dev/modules/markup" @@ -147,7 +146,7 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. processCtx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("Render [%s] for %s", cmdProg, baseLinkSrc)) defer finished() - cmd := exec.CommandContext(processCtx, cmdProg, cmdArgs...) + cmd := process.CommandContext(processCtx, cmdProg, cmdArgs...) cmd.Env = append( os.Environ(), "GITEA_PREFIX_SRC="+baseLinkSrc, @@ -159,7 +158,6 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. var stderr bytes.Buffer cmd.Stdout = output cmd.Stderr = &stderr - process.SetSysProcAttribute(cmd) if err := cmd.Run(); err != nil { return fmt.Errorf("%s render run command %s %v failed: %w\nStderr: %s", p.Name(), cmdProg, shellquote.Join(cmdArgs...), err, stderr.String()) diff --git a/modules/process/command.go b/modules/process/command.go new file mode 100644 index 0000000000000..edd1f04576cb5 --- /dev/null +++ b/modules/process/command.go @@ -0,0 +1,51 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package process + +import ( + "bytes" + "context" + "os/exec" +) + +type Cmd struct { + *exec.Cmd + + onCancelUserFunc func() error + termGraceful bool +} + +func (c *Cmd) WithOnCancelGracefully(userFunc func() error) *Cmd { + c.termGraceful, c.onCancelUserFunc = true, userFunc + return c +} + +func (c *Cmd) WithOnCancelForceKill(userFunc func() error) *Cmd { + c.termGraceful, c.onCancelUserFunc = false, userFunc + return c +} + +func (c *Cmd) WithDir(dir string) *Cmd { + c.Cmd.Dir = dir + return c +} + +func (c *Cmd) OutputString() (string, string, error) { + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + c.Cmd.Stdout = stdout + c.Cmd.Stderr = stderr + err := c.Cmd.Run() + return stdout.String(), stderr.String(), err +} + +// CommandContext returns a wrapped exec.Cmd which kills the process group when the context is canceled. +// By default, it uses graceful termination (SIGTERM) on Unix-like systems to make the subprocesses have chances +// to clean up (e.g. remove temporary files or lock files). +func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { + c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} + setSysProcAttribute(c.Cmd) + c.Cmd.Cancel = c.onCancel + c.termGraceful = true + return c +} diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go new file mode 100644 index 0000000000000..407b3b4998dcc --- /dev/null +++ b/modules/process/command_unix.go @@ -0,0 +1,28 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package process + +import ( + "os/exec" + "syscall" + + "gitea.dev/modules/util" +) + +func setSysProcAttribute(cmd *exec.Cmd) { + // When Gitea runs SubProcessA -> SubProcessB and SubProcessA gets killed by context cancel, + // use process group to make sure the sub processes can be killed and reaped instead of leaving defunct(zombie) processes. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +func (c *Cmd) onCancel() error { + if c.onCancelUserFunc != nil { + if err := c.onCancelUserFunc(); err != nil { + return err + } + } + sig := util.Iif(c.termGraceful, syscall.SIGTERM, syscall.SIGKILL) + // kill the whole process group + return syscall.Kill(-c.Process.Pid, sig) +} diff --git a/modules/process/command_unix_test.go b/modules/process/command_unix_test.go new file mode 100644 index 0000000000000..b18ba67726456 --- /dev/null +++ b/modules/process/command_unix_test.go @@ -0,0 +1,67 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !windows + +package process + +import ( + "bufio" + "context" + "os" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCommandContextCancelKillProcessGroup(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + // When executing external commands, there can be multiple subprocesses involved. + // e.g.: Gitea -> "git upload-pack" -> "git pack-objects". + // If the context is canceled (HTTP client disconnects), all subprocesses must terminate. + + // Spawn a shell that itself spawns a long-lived background process and + // prints its PID — mimicking git upload-pack spawning git pack-objects. + + r, w, err := os.Pipe() + require.NoError(t, err) + + cmd := CommandContext(ctx, "sh", "-c", "sleep 600 & echo $!; wait") + cmd.Stdout = w + require.NoError(t, cmd.Start()) + _ = w.Close() // parent keeps only the read end + + t.Cleanup(func() { + // make sure our test doesn't leave a zombie process even if test fails + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Wait() + }) + + // Block until the shell prints the grandchild PID. + scanner := bufio.NewScanner(r) + require.True(t, scanner.Scan(), "expected grandchild PID on stdout") + grandchildPID, err := strconv.Atoi(strings.TrimSpace(scanner.Text())) + require.NoError(t, err) + _ = r.Close() + + // Sanity: grandchild must be alive before we cancel. + grandchild, err := os.FindProcess(grandchildPID) + require.NoError(t, err) + require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel") + + // Cancel the context + cancel() + _ = cmd.Wait() + + // Subprocess should not exist after context cancel (killed by process group) + assert.Eventually(t, func() bool { + return grandchild.Signal(syscall.Signal(0)) != nil + }, 5*time.Second, 10*time.Millisecond) +} diff --git a/modules/process/command_windows.go b/modules/process/command_windows.go new file mode 100644 index 0000000000000..6fc58fae4fa47 --- /dev/null +++ b/modules/process/command_windows.go @@ -0,0 +1,19 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package process + +import "os/exec" + +// There is no graceful way to kill a process on Windows at the moment + +func setSysProcAttribute(cmd *exec.Cmd) {} + +func (c *Cmd) onCancel() error { + if c.onCancelUserFunc != nil { + if err := c.onCancelUserFunc(); err != nil { + return err + } + } + return c.Process.Kill() +} diff --git a/modules/process/error.go b/modules/process/error.go deleted file mode 100644 index 8f02f652585ea..0000000000000 --- a/modules/process/error.go +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package process - -import "fmt" - -// Error is a wrapped error describing the error results of Process Execution -type Error struct { - PID IDType - Description string - Err error - CtxErr error - Stdout string - Stderr string -} - -func (err *Error) Error() string { - return fmt.Sprintf("exec(%s:%s) failed: %v(%v) stdout: %s stderr: %s", err.PID, err.Description, err.Err, err.CtxErr, err.Stdout, err.Stderr) -} - -// Unwrap implements the unwrappable implicit interface for go1.13 Unwrap() -func (err *Error) Unwrap() error { - return err.Err -} diff --git a/modules/process/manager.go b/modules/process/manager.go index 74e7c405062e5..062c21509e72a 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -23,9 +23,6 @@ import ( var ( manager *Manager managerInit sync.Once - - // DefaultContext is the default context to run processing commands in - DefaultContext = context.Background() ) type ( diff --git a/modules/process/manager_exec.go b/modules/process/manager_exec.go deleted file mode 100644 index c98317374837b..0000000000000 --- a/modules/process/manager_exec.go +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package process - -import ( - "bytes" - "context" - "io" - "os/exec" - "time" -) - -// Exec a command and use the default timeout. -func (pm *Manager) Exec(desc, cmdName string, args ...string) (string, string, error) { - return pm.ExecDir(DefaultContext, -1, "", desc, cmdName, args...) -} - -// ExecTimeout a command and use a specific timeout duration. -func (pm *Manager) ExecTimeout(timeout time.Duration, desc, cmdName string, args ...string) (string, string, error) { - return pm.ExecDir(DefaultContext, timeout, "", desc, cmdName, args...) -} - -// ExecDir a command and use the default timeout. -func (pm *Manager) ExecDir(ctx context.Context, timeout time.Duration, dir, desc, cmdName string, args ...string) (string, string, error) { - return pm.ExecDirEnv(ctx, timeout, dir, desc, nil, cmdName, args...) -} - -// ExecDirEnv runs a command in given path and environment variables, and waits for its completion -// up to the given timeout (or DefaultTimeout if -1 is given). -// Returns its complete stdout and stderr -// outputs and an error, if any (including timeout) -func (pm *Manager) ExecDirEnv(ctx context.Context, timeout time.Duration, dir, desc string, env []string, cmdName string, args ...string) (string, string, error) { - return pm.ExecDirEnvStdIn(ctx, timeout, dir, desc, env, nil, cmdName, args...) -} - -// ExecDirEnvStdIn runs a command in given path and environment variables with provided stdIN, and waits for its completion -// up to the given timeout (or DefaultTimeout if timeout <= 0 is given). -// Returns its complete stdout and stderr -// outputs and an error, if any (including timeout) -func (pm *Manager) ExecDirEnvStdIn(ctx context.Context, timeout time.Duration, dir, desc string, env []string, stdIn io.Reader, cmdName string, args ...string) (string, string, error) { - if timeout <= 0 { - timeout = 60 * time.Second - } - - stdOut := new(bytes.Buffer) - stdErr := new(bytes.Buffer) - - ctx, _, finished := pm.AddContextTimeout(ctx, timeout, desc) - defer finished() - - cmd := exec.CommandContext(ctx, cmdName, args...) - cmd.Dir = dir - cmd.Env = env - cmd.Stdout = stdOut - cmd.Stderr = stdErr - if stdIn != nil { - cmd.Stdin = stdIn - } - SetSysProcAttribute(cmd) - - if err := cmd.Start(); err != nil { - return "", "", err - } - - err := cmd.Wait() - if err != nil { - err = &Error{ - PID: GetPID(ctx), - Description: desc, - Err: err, - CtxErr: ctx.Err(), - Stdout: stdOut.String(), - Stderr: stdErr.String(), - } - } - - return stdOut.String(), stdErr.String(), err -} diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 0d637c8acc3fd..bf2e84bf1c0cc 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -6,7 +6,6 @@ package process import ( "context" "testing" - "time" "github.com/stretchr/testify/assert" ) @@ -87,25 +86,3 @@ func TestManager_Remove(t *testing.T) { _, exists := pm.processMap[GetPID(p2Ctx)] assert.False(t, exists, "PID %d is in the list but shouldn't", GetPID(p2Ctx)) } - -func TestExecTimeoutNever(t *testing.T) { - // TODO Investigate how to improve the time elapsed per round. - maxLoops := 10 - for i := 1; i < maxLoops; i++ { - _, stderr, err := GetManager().ExecTimeout(5*time.Second, "ExecTimeout", "git", "--version") - if err != nil { - t.Fatalf("git --version: %v(%s)", err, stderr) - } - } -} - -func TestExecTimeoutAlways(t *testing.T) { - maxLoops := 100 - for i := 1; i < maxLoops; i++ { - _, stderr, err := GetManager().ExecTimeout(100*time.Microsecond, "ExecTimeout", "sleep", "5") - // TODO Simplify logging and errors to get precise error type. E.g. checking "if err != context.DeadlineExceeded". - if err == nil { - t.Fatalf("sleep 5 secs: %v(%s)", err, stderr) - } - } -} diff --git a/modules/process/manager_unix.go b/modules/process/manager_unix.go deleted file mode 100644 index ff1e8edf4dbe3..0000000000000 --- a/modules/process/manager_unix.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -//go:build !windows - -package process - -import ( - "os/exec" - "syscall" -) - -// SetSysProcAttribute sets the common SysProcAttrs for commands -func SetSysProcAttribute(cmd *exec.Cmd) { - // 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. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - cmd.Cancel = func() error { return KillCmd(cmd) } -} - -// KillCmd kills the process group of cmd, ensuring grandchildren are reaped. -func KillCmd(cmd *exec.Cmd) error { - return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) -} diff --git a/modules/process/manager_unix_test.go b/modules/process/manager_unix_test.go deleted file mode 100644 index fafa5cb5d0789..0000000000000 --- a/modules/process/manager_unix_test.go +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2026 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -//go:build !windows - -package process - -import ( - "bufio" - "context" - "os" - "os/exec" - "strconv" - "strings" - "syscall" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestSetSysProcAttributeKillsProcessGroup verifies that cancelling a context -// kills not only the direct child but also grandchildren it spawned. -// -// This mirrors the git upload-pack -> git pack-objects relationship during a -// git clone: if an HTTP client disconnects mid-transfer, all subprocesses must -// die. exec.CommandContext sends SIGKILL to the direct PID only; because -// SetSysProcAttribute sets Setpgid:true the grandchild is in the same process -// group but is NOT killed, leaking it until it finishes on its own. -func TestSetSysProcAttributeKillsProcessGroup(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - - // Spawn a shell that itself spawns a long-lived background process and - // prints its PID — mimicking git upload-pack spawning git pack-objects. - r, w, err := os.Pipe() - require.NoError(t, err) - - cmd := exec.CommandContext(ctx, "sh", "-c", "sleep 600 & echo $!; wait") - cmd.Stdout = w - SetSysProcAttribute(cmd) - require.NoError(t, cmd.Start()) - w.Close() // parent keeps only the read end - - // Always kill the process group on test exit so a failing assertion does - // not leak the grandchild (sleep 600) as an orphan. Setpgid:true makes - // the shell's PGID equal its own PID, so -cmd.Process.Pid targets the group. - t.Cleanup(func() { - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - _ = cmd.Wait() - }) - - // Block until the shell prints the grandchild PID. - scanner := bufio.NewScanner(r) - require.True(t, scanner.Scan(), "expected grandchild PID on stdout") - grandchildPID, err := strconv.Atoi(strings.TrimSpace(scanner.Text())) - require.NoError(t, err) - r.Close() - - // Sanity: grandchild must be alive before we cancel. - grandchild, err := os.FindProcess(grandchildPID) - require.NoError(t, err) - require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel") - - // Cancel the context — exec.CommandContext should propagate the kill to the - // whole process group, not just the direct child (the shell). - cancel() - _ = cmd.Wait() - - // Poll until the grandchild is gone or the deadline is exceeded. - // Signal(0) returns ESRCH (non-nil) once the process no longer exists. - assert.Eventually(t, func() bool { - return grandchild.Signal(syscall.Signal(0)) != nil - }, 5*time.Second, 10*time.Millisecond, - "grandchild process %d is still running after context cancel — process group was not killed (git pack-objects leak)", grandchildPID) -} diff --git a/modules/process/manager_windows.go b/modules/process/manager_windows.go deleted file mode 100644 index bcc6053a4d01f..0000000000000 --- a/modules/process/manager_windows.go +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -//go:build windows - -package process - -import ( - "os/exec" -) - -// SetSysProcAttribute sets the common SysProcAttrs for commands -func SetSysProcAttribute(cmd *exec.Cmd) { - // Do nothing -} - -// KillCmd kills the process; on Windows there are no process groups. -func KillCmd(cmd *exec.Cmd) error { - return cmd.Process.Kill() -} diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index c80b83370b737..5995ad16abfb3 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -75,7 +75,7 @@ func sessionHandler(session *sshSession) int { } } - cmd := exec.CommandContext(ctx, setting.AppPath, args...) + cmd := process.CommandContext(ctx, setting.AppPath, args...) cmd.Env = append( os.Environ(), "SSH_ORIGINAL_COMMAND="+session.rawCmd, @@ -104,8 +104,6 @@ func sessionHandler(session *sshSession) int { } defer stdin.Close() - process.SetSysProcAttribute(cmd) - wg := &sync.WaitGroup{} if err = cmd.Start(); err != nil { diff --git a/services/asymkey/commit.go b/services/asymkey/commit.go index ef1a910155fbd..0d6a50f058456 100644 --- a/services/asymkey/commit.go +++ b/services/asymkey/commit.go @@ -298,7 +298,7 @@ func verifyCommitSignByGPGSettings(ctx context.Context, gpgSettings *git.CommitS } // Otherwise we have to parse the key - pubKeyContent, err := gpgSettings.PublicKeyContent() + pubKeyContent, err := gpgSettings.PublicKeyContent(ctx) if err != nil { log.Error("gpgSettings.PublicKeyContent: %v", err) return nil @@ -420,7 +420,7 @@ func parseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUs // Try the configured instance-wide SSH public key if instanceSettings := getInstanceCommitSignSettings(git.SigningKeyFormatSSH); instanceSettings != nil { - pubKeyContent, err := instanceSettings.PublicKeyContent() + pubKeyContent, err := instanceSettings.PublicKeyContent(ctx) if err != nil { log.Error("commitSignSettings.PublicKeyContent: %v", err) } else { diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index b7c47e3ab53f8..e95aa3439a8bb 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -122,8 +122,7 @@ func PublicSigningKey(ctx context.Context) (content, format string, err error) { return string(content), signingKey.Format, nil } - content, stderr, err := process.GetManager().ExecDir(ctx, -1, setting.Git.HomePath, - "gpg --export -a", "gpg", "--export", "-a", signingKey.KeyID) + content, stderr, err := process.CommandContext(ctx, "gpg", "--export", "-a", signingKey.KeyID).WithDir(setting.Git.HomePath).OutputString() if err != nil { log.Error("Unable to get default signing key: %s, %s, %v", signingKey, stderr, err) return "", signingKey.Format, err diff --git a/services/mailer/sender/sendmail.go b/services/mailer/sender/sendmail.go index 6f7fadc31dee3..ce438a78ea689 100644 --- a/services/mailer/sender/sendmail.go +++ b/services/mailer/sender/sendmail.go @@ -6,7 +6,6 @@ package sender import ( "fmt" "io" - "os/exec" "strings" "gitea.dev/modules/graceful" @@ -47,12 +46,11 @@ func (s *SendmailSender) Send(from string, to []string, msg io.WriterTo) error { ctx, _, finished := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) defer finished() - cmd := exec.CommandContext(ctx, setting.MailService.SendmailPath, args...) + cmd := process.CommandContext(ctx, setting.MailService.SendmailPath, args...) pipe, err := cmd.StdinPipe() if err != nil { return err } - process.SetSysProcAttribute(cmd) if err = cmd.Start(); err != nil { _ = pipe.Close() diff --git a/tests/integration/gpg_ssh_git_test.go b/tests/integration/gpg_ssh_git_test.go index 130cfb65341c6..75203db2def58 100644 --- a/tests/integration/gpg_ssh_git_test.go +++ b/tests/integration/gpg_ssh_git_test.go @@ -39,7 +39,7 @@ func TestGPGGit(t *testing.T) { t.Setenv("GNUPGHOME", tmpDir) // Need to create a root key - rootKeyPair, err := importTestingKey() + rootKeyPair, err := importTestingKey(t) require.NoError(t, err, "importTestingKey") defer test.MockVariableValue(&setting.Repository.Signing.SigningKey, rootKeyPair.PrimaryKey.KeyIdShortString())() @@ -403,9 +403,9 @@ func crudActionCreateFile(_ *testing.T, ctx APITestContext, user *user_model.Use }, callback...) } -func importTestingKey() (*openpgp.Entity, error) { +func importTestingKey(t *testing.T) (*openpgp.Entity, error) { keyPath := filepath.Join(setting.GetGiteaTestSourceRoot(), "tests/integration/private-testing.key") - if _, _, err := process.GetManager().Exec("gpg --import "+keyPath, "gpg", "--import", keyPath); err != nil { + if _, _, err := process.CommandContext(t.Context(), "gpg", "--import", keyPath).OutputString(); err != nil { return nil, err } keyringFile, err := os.Open(keyPath) From 5c8b79cf2a91d2f584460c2c0374f23fe36467c1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 27 Aug 2026 18:38:39 +0800 Subject: [PATCH 3/8] fix lint --- modules/process/command_unix.go | 2 ++ modules/process/command_unix_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go index 407b3b4998dcc..6edb1ca2eacbf 100644 --- a/modules/process/command_unix.go +++ b/modules/process/command_unix.go @@ -1,6 +1,8 @@ // Copyright 2026 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT +//go:build unix + package process import ( diff --git a/modules/process/command_unix_test.go b/modules/process/command_unix_test.go index b18ba67726456..0f81e9b2ccd44 100644 --- a/modules/process/command_unix_test.go +++ b/modules/process/command_unix_test.go @@ -1,7 +1,7 @@ // Copyright 2026 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT -//go:build !windows +//go:build unix package process From 9e336b1e8f551c3510c964f1fc528f532372a32c Mon Sep 17 00:00:00 2001 From: Royce Remer Date: Thu, 27 Aug 2026 10:00:16 -0700 Subject: [PATCH 4/8] fix(process): introduce command.WithWaitDelay() defaulting to 10s to escalate SIGINT -> SIGKILL Assisted-by: Claude Sonnet 4.6 (1M context) Signed-off-by: Royce Remer --- modules/process/command.go | 10 ++++ modules/process/command_unix.go | 8 ++++ modules/process/command_unix_test.go | 69 ++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/modules/process/command.go b/modules/process/command.go index edd1f04576cb5..bdd8fa11044dc 100644 --- a/modules/process/command.go +++ b/modules/process/command.go @@ -7,8 +7,12 @@ import ( "bytes" "context" "os/exec" + "time" ) +// defaultWaitDelay is how long CommandContext waits after sending SIGTERM before escalating to SIGKILL. +const defaultWaitDelay = 10 * time.Second + type Cmd struct { *exec.Cmd @@ -31,6 +35,11 @@ func (c *Cmd) WithDir(dir string) *Cmd { return c } +func (c *Cmd) WithWaitDelay(d time.Duration) *Cmd { + c.Cmd.WaitDelay = d + return c +} + func (c *Cmd) OutputString() (string, string, error) { stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} c.Cmd.Stdout = stdout @@ -46,6 +55,7 @@ func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} setSysProcAttribute(c.Cmd) c.Cmd.Cancel = c.onCancel + c.Cmd.WaitDelay = defaultWaitDelay c.termGraceful = true return c } diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go index 6edb1ca2eacbf..a215fc21bdf26 100644 --- a/modules/process/command_unix.go +++ b/modules/process/command_unix.go @@ -8,6 +8,7 @@ package process import ( "os/exec" "syscall" + "time" "gitea.dev/modules/util" ) @@ -25,6 +26,13 @@ func (c *Cmd) onCancel() error { } } sig := util.Iif(c.termGraceful, syscall.SIGTERM, syscall.SIGKILL) + if sig == syscall.SIGTERM && c.Cmd.WaitDelay > 0 { + pgid, delay := c.Process.Pid, c.Cmd.WaitDelay + go func() { + time.Sleep(delay) + _ = syscall.Kill(-pgid, syscall.SIGKILL) + }() + } // kill the whole process group return syscall.Kill(-c.Process.Pid, sig) } diff --git a/modules/process/command_unix_test.go b/modules/process/command_unix_test.go index 0f81e9b2ccd44..6abf47a48647c 100644 --- a/modules/process/command_unix_test.go +++ b/modules/process/command_unix_test.go @@ -65,3 +65,72 @@ func TestCommandContextCancelKillProcessGroup(t *testing.T) { return grandchild.Signal(syscall.Signal(0)) != nil }, 5*time.Second, 10*time.Millisecond) } + +func TestCommandContextWaitDelayOrphansGrandchild(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + r, w, err := os.Pipe() + require.NoError(t, err) + + // Shell ignores SIGTERM; grandchild inherits SIG_IGN and also survives the group SIGTERM. + cmd := CommandContext(ctx, "sh", "-c", "trap '' TERM; sleep 600 & echo $!; wait"). + WithWaitDelay(1 * time.Second) + cmd.Stdout = w + require.NoError(t, cmd.Start()) + _ = w.Close() + + var grandchildPID int + t.Cleanup(func() { + // Kill by process group — the PGID survives the group leader's death, so this reaches orphaned sleep. + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Wait() + if grandchildPID > 0 { + _ = syscall.Kill(grandchildPID, syscall.SIGKILL) + } + }) + + scanner := bufio.NewScanner(r) + require.True(t, scanner.Scan(), "expected grandchild PID on stdout") + grandchildPID, err = strconv.Atoi(strings.TrimSpace(scanner.Text())) + require.NoError(t, err) + _ = r.Close() + + grandchild, err := os.FindProcess(grandchildPID) + require.NoError(t, err) + require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel") + + cancel() + _ = cmd.Wait() // blocks ~1s until WaitDelay SIGKILL kills the direct child (shell) + + assert.Eventually(t, func() bool { + return grandchild.Signal(syscall.Signal(0)) != nil + }, 5*time.Second, 10*time.Millisecond, "grandchild was orphaned: WaitDelay SIGKILL did not reach the process group") +} + +func TestCommandContextSIGTERMIgnoredHangs(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + cmd := CommandContext(ctx, "sh", "-c", "trap '' TERM; sleep 600").WithWaitDelay(1 * time.Second) + require.NoError(t, cmd.Start()) + + waitDone := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(waitDone) + }() + + t.Cleanup(func() { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + <-waitDone + }) + + cancel() + + select { + case <-waitDone: + case <-time.After(5 * time.Second): + t.Fatal("process did not exit after context cancel: SIGTERM ignored and no SIGKILL escalation occurred") + } +} From 3b7449f8ae8de08a32fbc7423869850c25545f3b Mon Sep 17 00:00:00 2001 From: Royce Remer Date: Thu, 27 Aug 2026 11:07:32 -0700 Subject: [PATCH 5/8] fix(process): Always check ProcessState before reaping by pid Assisted-by: Claude Sonnet 4.6 (1M context) Signed-off-by: Royce Remer --- modules/process/command_unix.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go index a215fc21bdf26..bbb6c7ee706e8 100644 --- a/modules/process/command_unix.go +++ b/modules/process/command_unix.go @@ -27,12 +27,19 @@ func (c *Cmd) onCancel() error { } sig := util.Iif(c.termGraceful, syscall.SIGTERM, syscall.SIGKILL) if sig == syscall.SIGTERM && c.Cmd.WaitDelay > 0 { - pgid, delay := c.Process.Pid, c.Cmd.WaitDelay + delay := c.Cmd.WaitDelay go func() { time.Sleep(delay) - _ = syscall.Kill(-pgid, syscall.SIGKILL) + _ = c.signalProcessGroup(syscall.SIGKILL) }() } - // kill the whole process group + return c.signalProcessGroup(sig) +} + +// signalProcessGroup sends sig to the process group, skipping if the process has already been reaped. +func (c *Cmd) signalProcessGroup(sig syscall.Signal) error { + if c.Cmd.ProcessState != nil { + return nil + } return syscall.Kill(-c.Process.Pid, sig) } From b08ea8ad424b5bcbd9d39c1b42035701715a7457 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 28 Aug 2026 02:32:48 +0800 Subject: [PATCH 6/8] revert pid reuse data-race --- modules/process/command.go | 10 ---- modules/process/command_unix.go | 17 +------ modules/process/command_unix_test.go | 69 ---------------------------- 3 files changed, 1 insertion(+), 95 deletions(-) diff --git a/modules/process/command.go b/modules/process/command.go index bdd8fa11044dc..edd1f04576cb5 100644 --- a/modules/process/command.go +++ b/modules/process/command.go @@ -7,12 +7,8 @@ import ( "bytes" "context" "os/exec" - "time" ) -// defaultWaitDelay is how long CommandContext waits after sending SIGTERM before escalating to SIGKILL. -const defaultWaitDelay = 10 * time.Second - type Cmd struct { *exec.Cmd @@ -35,11 +31,6 @@ func (c *Cmd) WithDir(dir string) *Cmd { return c } -func (c *Cmd) WithWaitDelay(d time.Duration) *Cmd { - c.Cmd.WaitDelay = d - return c -} - func (c *Cmd) OutputString() (string, string, error) { stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} c.Cmd.Stdout = stdout @@ -55,7 +46,6 @@ func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} setSysProcAttribute(c.Cmd) c.Cmd.Cancel = c.onCancel - c.Cmd.WaitDelay = defaultWaitDelay c.termGraceful = true return c } diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go index bbb6c7ee706e8..6edb1ca2eacbf 100644 --- a/modules/process/command_unix.go +++ b/modules/process/command_unix.go @@ -8,7 +8,6 @@ package process import ( "os/exec" "syscall" - "time" "gitea.dev/modules/util" ) @@ -26,20 +25,6 @@ func (c *Cmd) onCancel() error { } } sig := util.Iif(c.termGraceful, syscall.SIGTERM, syscall.SIGKILL) - if sig == syscall.SIGTERM && c.Cmd.WaitDelay > 0 { - delay := c.Cmd.WaitDelay - go func() { - time.Sleep(delay) - _ = c.signalProcessGroup(syscall.SIGKILL) - }() - } - return c.signalProcessGroup(sig) -} - -// signalProcessGroup sends sig to the process group, skipping if the process has already been reaped. -func (c *Cmd) signalProcessGroup(sig syscall.Signal) error { - if c.Cmd.ProcessState != nil { - return nil - } + // kill the whole process group return syscall.Kill(-c.Process.Pid, sig) } diff --git a/modules/process/command_unix_test.go b/modules/process/command_unix_test.go index 6abf47a48647c..0f81e9b2ccd44 100644 --- a/modules/process/command_unix_test.go +++ b/modules/process/command_unix_test.go @@ -65,72 +65,3 @@ func TestCommandContextCancelKillProcessGroup(t *testing.T) { return grandchild.Signal(syscall.Signal(0)) != nil }, 5*time.Second, 10*time.Millisecond) } - -func TestCommandContextWaitDelayOrphansGrandchild(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - - r, w, err := os.Pipe() - require.NoError(t, err) - - // Shell ignores SIGTERM; grandchild inherits SIG_IGN and also survives the group SIGTERM. - cmd := CommandContext(ctx, "sh", "-c", "trap '' TERM; sleep 600 & echo $!; wait"). - WithWaitDelay(1 * time.Second) - cmd.Stdout = w - require.NoError(t, cmd.Start()) - _ = w.Close() - - var grandchildPID int - t.Cleanup(func() { - // Kill by process group — the PGID survives the group leader's death, so this reaches orphaned sleep. - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - _ = cmd.Wait() - if grandchildPID > 0 { - _ = syscall.Kill(grandchildPID, syscall.SIGKILL) - } - }) - - scanner := bufio.NewScanner(r) - require.True(t, scanner.Scan(), "expected grandchild PID on stdout") - grandchildPID, err = strconv.Atoi(strings.TrimSpace(scanner.Text())) - require.NoError(t, err) - _ = r.Close() - - grandchild, err := os.FindProcess(grandchildPID) - require.NoError(t, err) - require.NoError(t, grandchild.Signal(syscall.Signal(0)), "grandchild should be alive before cancel") - - cancel() - _ = cmd.Wait() // blocks ~1s until WaitDelay SIGKILL kills the direct child (shell) - - assert.Eventually(t, func() bool { - return grandchild.Signal(syscall.Signal(0)) != nil - }, 5*time.Second, 10*time.Millisecond, "grandchild was orphaned: WaitDelay SIGKILL did not reach the process group") -} - -func TestCommandContextSIGTERMIgnoredHangs(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - - cmd := CommandContext(ctx, "sh", "-c", "trap '' TERM; sleep 600").WithWaitDelay(1 * time.Second) - require.NoError(t, cmd.Start()) - - waitDone := make(chan struct{}) - go func() { - _ = cmd.Wait() - close(waitDone) - }() - - t.Cleanup(func() { - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - <-waitDone - }) - - cancel() - - select { - case <-waitDone: - case <-time.After(5 * time.Second): - t.Fatal("process did not exit after context cancel: SIGTERM ignored and no SIGKILL escalation occurred") - } -} From 4a84a8cb010d618b16ec5ee521c16d394e50bed2 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 28 Aug 2026 02:38:25 +0800 Subject: [PATCH 7/8] comment --- modules/process/command.go | 3 +++ modules/process/command_unix.go | 2 ++ 2 files changed, 5 insertions(+) diff --git a/modules/process/command.go b/modules/process/command.go index edd1f04576cb5..9e0a53023a084 100644 --- a/modules/process/command.go +++ b/modules/process/command.go @@ -46,6 +46,9 @@ func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} setSysProcAttribute(c.Cmd) c.Cmd.Cancel = c.onCancel + + // Unlike exec.CommandContext, we use graceful termination by default to avoid corrupting data or leaving lock files behind. + // If some processes don't respond to SIGTERM, can switch to WithOnCancelForceKill (SIGKILL) to force kill them. c.termGraceful = true return c } diff --git a/modules/process/command_unix.go b/modules/process/command_unix.go index 6edb1ca2eacbf..a2f437e5da051 100644 --- a/modules/process/command_unix.go +++ b/modules/process/command_unix.go @@ -26,5 +26,7 @@ func (c *Cmd) onCancel() error { } sig := util.Iif(c.termGraceful, syscall.SIGTERM, syscall.SIGKILL) // kill the whole process group + // ATTENTION: do not access PID after Wait or in other goroutine, it will just cause PID reuse data-race. + // There is no easy solution to implement "first SIGTERM then SIGKILL" in a safe way, only one signal can be sent to the process group. return syscall.Kill(-c.Process.Pid, sig) } From 0b260ff8d867a19f8451ebebbed9c87f6e040748 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 29 Aug 2026 14:07:18 +0800 Subject: [PATCH 8/8] add lint rule to deny exec.CommandContext --- .golangci.yml | 5 +++++ modules/process/command.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 22ea9b9d2b6f8..1a674eb3dec69 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -62,6 +62,10 @@ linters: desc: "migrations must not depend on the models package. HINT: MIGRATION-STRUCT-FROZEN" - pkg: gitea.dev/modules/structs desc: "migrations must not depend on modules/structs. HINT: MIGRATION-STRUCT-FROZEN" + forbidigo: + forbid: + - pattern: '^(fmt\.Print(|f|ln)|print|println)$' # default + - pattern: '^exec\.CommandContext$' # use our wrapper for graceful termination modernize: disable: - embedlit @@ -140,6 +144,7 @@ linters: - linters: - dupl - errcheck + - forbidigo - staticcheck - unparam path: _test\.go diff --git a/modules/process/command.go b/modules/process/command.go index 9e0a53023a084..290d6dbc5d2ee 100644 --- a/modules/process/command.go +++ b/modules/process/command.go @@ -43,7 +43,7 @@ func (c *Cmd) OutputString() (string, string, error) { // By default, it uses graceful termination (SIGTERM) on Unix-like systems to make the subprocesses have chances // to clean up (e.g. remove temporary files or lock files). func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { - c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} + c := &Cmd{Cmd: exec.CommandContext(ctx, name, arg...)} //nolint:forbidigo // wrap it setSysProcAttribute(c.Cmd) c.Cmd.Cancel = c.onCancel